fix: Better handle associations deletions (#10622)

migrations/20250110102114-fix-deleted-expense-relationships.ts

@@ -0,0 +1,54 @@
+'use strict';
+
+import logger from '../server/lib/logger';
+
+/** @type {import('sequelize-cli').Migration} */
+module.exports = {
+  async up(queryInterface) {
+    const [deletedComments] = await queryInterface.sequelize.query(`
+      UPDATE "Comments"
+      SET "deletedAt" = e."deletedAt"
+      FROM "Expenses" e
+      WHERE "Comments"."ExpenseId" IS NOT NULL
+      AND "Comments"."ExpenseId" = e."id"
+      AND e."deletedAt" IS NOT NULL
+      AND "Comments"."deletedAt" IS NULL
+      RETURNING e."id"
+    `);
+
+    const [deletedItems] = await queryInterface.sequelize.query(`
+      UPDATE "ExpenseItems"
+      SET "deletedAt" = e."deletedAt"
+      FROM "Expenses" e
+      WHERE "ExpenseItems"."ExpenseId" = e."id"
+      AND e."deletedAt" IS NOT NULL
+      AND "ExpenseItems"."deletedAt" IS NULL
+      RETURNING e."id"
+    `);
+
+    logger.info(`Expenses: Fixed ${deletedComments.length} deleted comments, ${deletedItems.length} deleted items`);
+
+    if (deletedComments.length === 0 || deletedItems.length === 0) {
+      await queryInterface.sequelize.query(
+        `
+        INSERT INTO "MigrationLogs"
+        ("createdAt", "type", "description", "CreatedByUserId", "data")
+        VALUES (NOW(), 'MIGRATION', 'Fix deleted expense relationships', NULL, :data)
+      `,
+        {
+          type: queryInterface.sequelize.QueryTypes.INSERT,
+          replacements: {
+            data: JSON.stringify({
+              deletedComments,
+              deletedItems,
+            }),
+          },
+        },
+      );
+    }
+  },
+
+  async down() {
+    console.log(`No rollback, check the MigrationLogs table for the data`);
+  },
+};

server/graphql/v2/mutation/ExpenseMutations.ts

@@ -277,11 +277,6 @@ const expenseMutations = {
       await twoFactorAuthLib.enforceForAccountsUserIsAdminOf(req, accountsFor2FA);
 
       // Cancel recurring expense
-      const recurringExpense = await expense.getRecurringExpense();
-      if (recurringExpense) {
-        await recurringExpense.destroy();
-      }
-
       await expense.destroy();
       return expense;
     },

server/models/Expense.ts

@@ -967,7 +967,17 @@ Expense.init(
     tableName: 'Expenses',
     hooks: {
       async afterDestroy(expense: Expense) {
-        await ExpenseItem.destroy({ where: { ExpenseId: expense.id } });
+        // Not considering ExpensesAttachedFiles because they don't support soft delete (they should)
+        const promises = [
+          ExpenseItem.destroy({ where: { ExpenseId: expense.id } }),
+          models.Comment.destroy({ where: { ExpenseId: expense.id } }),
+        ];
+
+        if (expense.RecurringExpenseId) {
+          promises.push(RecurringExpense.destroy({ where: { id: expense.RecurringExpenseId } }));
+        }
+
+        await Promise.all(promises);
       },
     },
   },

server/models/Update.ts

@@ -162,10 +162,15 @@ class Update extends Model<InferAttributes<Update>, InferCreationAttributes<Upda
   };
 
   delete = async function (remoteUser) {
-    await Comment.destroy({ where: { UpdateId: this.id } });
-    await Update.update({ deletedAt: new Date(), LastEditedByUserId: remoteUser.id }, { where: { id: this.id } });
-
-    return this;
+    return sequelize.transaction(async transaction => {
+      await Comment.destroy({ where: { UpdateId: this.id }, transaction });
+      await Update.update(
+        { deletedAt: new Date(), LastEditedByUserId: remoteUser.id },
+        { where: { id: this.id }, transaction },
+      );
+
+      return this;
+    });
   };
 
   // Returns the User model of the User that created this Update

test/server/graphql/v2/mutation/ExpenseMutations-legacy.test.js

@@ -27,6 +27,7 @@ import { randEmail, randUrl } from '../../../../stores';
 import {
   fakeAccountingCategory,
   fakeCollective,
+  fakeComment,
   fakeConnectedAccount,
   fakeExpense,
   fakeExpenseItem,
@@ -1177,11 +1178,16 @@ describe('server/graphql/v2/mutation/ExpenseMutations', () => {
     describe('can delete rejected expenses', () => {
       it('if owner', async () => {
         const expense = await fakeExpense({ status: expenseStatus.REJECTED });
+        const item = await fakeExpenseItem({ ExpenseId: expense.id });
+        const comment = await fakeComment({ ExpenseId: expense.id });
         const result = await graphqlQueryV2(deleteExpenseMutation, prepareGQLParams(expense), expense.User);
 
         expect(result.data.deleteExpense.legacyId).to.eq(expense.id);
-        await expense.reload({ paranoid: false });
-        expect(expense.deletedAt).to.exist;
+
+        for (const model of [expense, item, comment]) {
+          const deletedModel = await model.reload({ paranoid: false });
+          expect(deletedModel.deletedAt).to.exist;
+        }
       });
 
       it('if collective admin', async () => {

test/server/models/Update.test.js

@@ -3,7 +3,14 @@ import { flatten, reduce, times } from 'lodash';
 
 import models, { Op } from '../../../server/models';
 import { randEmail } from '../../stores';
-import { fakeCollective, fakeMember, fakeOrganization, fakeUpdate, fakeUser } from '../../test-helpers/fake-data';
+import {
+  fakeCollective,
+  fakeComment,
+  fakeMember,
+  fakeOrganization,
+  fakeUpdate,
+  fakeUser,
+} from '../../test-helpers/fake-data';
 import * as utils from '../../utils';
 
 const addRandomMemberUsers = (collective, count, role) => {
@@ -120,6 +127,21 @@ describe('server/models/Update', () => {
       expect(uniqueSlug).to.not.be.equal(update.slug);
       expect(/-\d+$/.test(update.slug)).to.be.true;
     });
+
+    it('deletes related comments and record the user who deleted it', async () => {
+      const user = await fakeUser();
+      const update = await fakeUpdate({ id: 4242 });
+      const comment = await fakeComment({ UpdateId: update.id });
+
+      await update.delete(user);
+      await update.reload({ paranoid: false });
+
+      expect(update.LastEditedByUserId).to.equal(user.id);
+      expect(update.deletedAt).to.not.be.null;
+
+      const comments = await models.Comment.findAll({ where: { UpdateId: update.id } });
+      expect(comments.length).to.equal(0);
+    });
   });
 
   describe('Update audience', () => {