fix(Policies): nested data inception

opencollective · Jan 30, 2025 · c4e7cb3 · c4e7cb3
1 parent 421e385
commit c4e7cb3
Show file tree

Hide file tree

Showing 3 changed files with 120 additions and 9 deletions.
diff --git a/migrations/20250130072503-fix-nested-collecives-data.js b/migrations/20250130072503-fix-nested-collecives-data.js
@@ -0,0 +1,42 @@
+'use strict';
+
+/** @type {import('sequelize-cli').Migration} */
+module.exports = {
+  async up(queryInterface, Sequelize) {
+    await queryInterface.sequelize.query(`
+      WITH updated_collectives AS (
+        UPDATE "Collectives"
+        SET "data" =
+          -- Merge "data" into nested data, to make sure newly created keys aren't dropped
+          (data->'data' || data)
+          -- Make sure any policy created in (data->'data'->'policies'->'EXPENSE_POLICIES') is merged with the top level policies
+          || JSONB_BUILD_OBJECT(
+            'policies',
+            COALESCE(data->'data'->'policies', '{}') || COALESCE(data->'policies', '{}')
+          )
+        WHERE data ? 'data'
+        RETURNING id, data
+      ) INSERT INTO "MigrationLogs" ("type", "description", "createdAt", "data")
+        SELECT
+          'MIGRATION',
+          '20250130072503-fix-nested-collecives-data',
+          NOW(),
+          CASE WHEN (SELECT COUNT(*) FROM updated_collectives) = 0
+            THEN '[]'
+            ELSE jsonb_agg(jsonb_build_object('id', id, 'data', data))
+          END
+        FROM updated_collectives
+        RETURNING id
+    `);
+
+    await queryInterface.sequelize.query(`
+      UPDATE "Collectives"
+      SET "data" = data - 'data'
+      WHERE data ? 'data'
+    `);
+  },
+
+  async down(queryInterface, Sequelize) {
+    console.log('Please look at the migration logs to see the data that was migrated');
+  },
+};
diff --git a/server/models/Collective.ts b/server/models/Collective.ts
@@ -3746,7 +3746,7 @@ Collective.init(
               receiptPolicy: sanitizeHTML(expensePolicy, optsSanitizeHtmlForSimplified),
             },
           };
-          this.setDataValue('data', { data: { ...data, policies: newPolicies } });
+          this.setDataValue('data', { ...data, policies: newPolicies });
         } else {
           const data = this.getDataValue('data');
           const newPolicies = {
@@ -3757,7 +3757,7 @@ Collective.init(
               receiptPolicy: '',
             },
           };
-          this.setDataValue('data', { data: { ...data, policies: newPolicies } });
+          this.setDataValue('data', { ...data, policies: newPolicies });
         }
       },
     },

diff --git a/test/server/graphql/v2/mutation/AccountMutations.test.ts b/test/server/graphql/v2/mutation/AccountMutations.test.ts
@@ -1007,7 +1007,15 @@ describe('server/graphql/v2/mutation/AccountMutations', () => {
       expect(result.errors).to.not.exist;
 
       await collective.reload();
-      expect(collective.data.policies).to.deep.equal({ [POLICIES.EXPENSE_AUTHOR_CANNOT_APPROVE]: { enabled: true } });
+      expect(collective.data.policies).to.deep.equal({
+        [POLICIES.EXPENSE_AUTHOR_CANNOT_APPROVE]: { enabled: true },
+        // Expense policies from `expensePolicy` field set at account creation
+        [POLICIES.EXPENSE_POLICIES]: {
+          invoicePolicy: 'Be reasonable',
+          receiptPolicy: 'Be reasonable',
+          titlePolicy: '',
+        },
+      });
 
       // Check activity
       const activity = await models.Activity.findOne({
@@ -1018,8 +1026,25 @@ describe('server/graphql/v2/mutation/AccountMutations', () => {
       expect(activity).to.exist;
       expect(activity.CollectiveId).to.equal(collective.id);
       expect(activity.data).to.deep.equal({
-        previousData: { policies: {} },
-        newData: { policies: { [POLICIES.EXPENSE_AUTHOR_CANNOT_APPROVE]: { enabled: true } } },
+        previousData: {
+          policies: {
+            [POLICIES.EXPENSE_POLICIES]: {
+              invoicePolicy: 'Be reasonable',
+              receiptPolicy: 'Be reasonable',
+              titlePolicy: '',
+            },
+          },
+        },
+        newData: {
+          policies: {
+            [POLICIES.EXPENSE_AUTHOR_CANNOT_APPROVE]: { enabled: true },
+            [POLICIES.EXPENSE_POLICIES]: {
+              invoicePolicy: 'Be reasonable',
+              receiptPolicy: 'Be reasonable',
+              titlePolicy: '',
+            },
+          },
+        },
       });
     });
 
@@ -1035,6 +1060,11 @@ describe('server/graphql/v2/mutation/AccountMutations', () => {
       expect(collective.data.policies).to.deep.eq({
         [POLICIES.EXPENSE_AUTHOR_CANNOT_APPROVE]: { enabled: true },
         [POLICIES.COLLECTIVE_MINIMUM_ADMINS]: { numberOfAdmins: 42 },
+        [POLICIES.EXPENSE_POLICIES]: {
+          invoicePolicy: 'Be reasonable',
+          receiptPolicy: 'Be reasonable',
+          titlePolicy: '',
+        },
       });
 
       // Check activity
@@ -1046,11 +1076,25 @@ describe('server/graphql/v2/mutation/AccountMutations', () => {
       expect(activity).to.exist;
       expect(activity.CollectiveId).to.equal(collective.id);
       expect(activity.data).to.deep.equal({
-        previousData: { policies: { [POLICIES.EXPENSE_AUTHOR_CANNOT_APPROVE]: { enabled: true } } },
+        previousData: {
+          policies: {
+            [POLICIES.EXPENSE_AUTHOR_CANNOT_APPROVE]: { enabled: true },
+            [POLICIES.EXPENSE_POLICIES]: {
+              invoicePolicy: 'Be reasonable',
+              receiptPolicy: 'Be reasonable',
+              titlePolicy: '',
+            },
+          },
+        },
         newData: {
           policies: {
             [POLICIES.EXPENSE_AUTHOR_CANNOT_APPROVE]: { enabled: true },
             [POLICIES.COLLECTIVE_MINIMUM_ADMINS]: { numberOfAdmins: 42 },
+            [POLICIES.EXPENSE_POLICIES]: {
+              invoicePolicy: 'Be reasonable',
+              receiptPolicy: 'Be reasonable',
+              titlePolicy: '',
+            },
           },
         },
       });
@@ -1065,7 +1109,13 @@ describe('server/graphql/v2/mutation/AccountMutations', () => {
       expect(result.errors).to.not.exist;
 
       await collective.reload();
-      expect(collective.data.policies).to.be.empty;
+      expect(collective.data.policies).to.deep.equal({
+        [POLICIES.EXPENSE_POLICIES]: {
+          invoicePolicy: 'Be reasonable',
+          receiptPolicy: 'Be reasonable',
+          titlePolicy: '',
+        },
+      });
 
       // Check activity
       const activity = await models.Activity.findOne({
@@ -1076,11 +1126,24 @@ describe('server/graphql/v2/mutation/AccountMutations', () => {
       expect(activity).to.exist;
       expect(activity.CollectiveId).to.equal(collective.id);
       expect(activity.data).to.deep.equal({
-        newData: { policies: {} },
+        newData: {
+          policies: {
+            [POLICIES.EXPENSE_POLICIES]: {
+              invoicePolicy: 'Be reasonable',
+              receiptPolicy: 'Be reasonable',
+              titlePolicy: '',
+            },
+          },
+        },
         previousData: {
           policies: {
             [POLICIES.EXPENSE_AUTHOR_CANNOT_APPROVE]: { enabled: true },
             [POLICIES.COLLECTIVE_MINIMUM_ADMINS]: { numberOfAdmins: 42 },
+            [POLICIES.EXPENSE_POLICIES]: {
+              invoicePolicy: 'Be reasonable',
+              receiptPolicy: 'Be reasonable',
+              titlePolicy: '',
+            },
           },
         },
       });
@@ -1095,7 +1158,13 @@ describe('server/graphql/v2/mutation/AccountMutations', () => {
       expect(result.errors).to.have.lengthOf(1);
 
       await collective.reload();
-      expect(collective.data.policies).to.be.empty;
+      expect(collective.data.policies).to.deep.equal({
+        [POLICIES.EXPENSE_POLICIES]: {
+          invoicePolicy: 'Be reasonable',
+          receiptPolicy: 'Be reasonable',
+          titlePolicy: '',
+        },
+      });
     });
   });