chore: avoid duplicates (#5381)

This PR handles the case where a single strategy is used in multiple
change requests. Instead of listing the strategy several times in the
output, we consolidate the entries and add a new `changeRequestIds`
property. This is a non-empty list that points to all the change
requests it is used in.

This is required for us to be able to link back to the change requests
from the UI overview.

src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts

@@ -9,6 +9,7 @@ let db: ITestDb;
 let user: IUser;
 
 const CR_ID = 123456;
+const CR_ID_2 = 234567;
 const FLAG_NAME = 'crarm-test-flag';
 
 let readModel: IChangeRequestSegmentUsageReadModel;
@@ -32,16 +33,22 @@ afterAll(async () => {
 });
 
 afterEach(async () => {
-    await db.rawDatabase.table('change_requests').where('id', CR_ID).delete();
+    await db.rawDatabase
+        .table('change_requests')
+        .where('id', CR_ID)
+        .orWhere('id', CR_ID_2)
+        .delete();
+
     await db.rawDatabase
         .table('change_request_events')
         .where('change_request_id', CR_ID)
+        .orWhere('change_request_id', CR_ID_2)
         .delete();
 });
 
-const createCR = async (state) => {
+const createCR = async (state, changeRequestId = CR_ID) => {
     await db.rawDatabase.table('change_requests').insert({
-        id: CR_ID,
+        id: changeRequestId,
         environment: 'default',
         state,
         project: 'default',
@@ -52,52 +59,72 @@ const createCR = async (state) => {
     });
 };
 
-const addChangeRequestChange = async (flagName, action, change) => {
+const addChangeRequestChange = async (
+    flagName,
+    action,
+    change,
+    changeRequestId,
+) => {
     await db.rawDatabase.table('change_request_events').insert({
         feature: flagName,
         action,
         payload: change,
         created_at: '2023-01-01 00:01:00',
-        change_request_id: CR_ID,
+        change_request_id: changeRequestId,
         created_by: user.id,
     });
 };
 
-const addStrategyToCr = async (segmentId: number, flagName: string) => {
-    await addChangeRequestChange(flagName, 'addStrategy', {
-        name: 'flexibleRollout',
-        title: '',
-        disabled: false,
-        segments: [segmentId],
-        variants: [],
-        parameters: {
-            groupId: flagName,
-            rollout: '100',
-            stickiness: 'default',
+const addStrategyToCr = async (
+    segmentId: number,
+    flagName: string,
+    changeRequestId = CR_ID,
+) => {
+    await addChangeRequestChange(
+        flagName,
+        'addStrategy',
+        {
+            name: 'flexibleRollout',
+            title: '',
+            disabled: false,
+            segments: [segmentId],
+            variants: [],
+            parameters: {
+                groupId: flagName,
+                rollout: '100',
+                stickiness: 'default',
+            },
+            constraints: [],
         },
-        constraints: [],
-    });
+        changeRequestId,
+    );
 };
 
 const updateStrategyInCr = async (
     strategyId: string,
     segmentId: number,
     flagName: string,
+    changeRequestId = CR_ID,
 ) => {
-    await addChangeRequestChange(flagName, 'updateStrategy', {
-        id: strategyId,
-        name: 'flexibleRollout',
-        title: '',
-        disabled: false,
-        segments: [segmentId],
-        variants: [],
-        parameters: {
-            groupId: flagName,
-            rollout: '100',
-            stickiness: 'default',
+    await addChangeRequestChange(
+        flagName,
+        'updateStrategy',
+        {
+            id: strategyId,
+            name: 'flexibleRollout',
+            title: '',
+            disabled: false,
+            segments: [segmentId],
+            variants: [],
+            parameters: {
+                groupId: flagName,
+                rollout: '100',
+                stickiness: 'default',
+            },
+            constraints: [],
         },
-        constraints: [],
-    });
+        changeRequestId,
+    );
 };
 
 test.each([
@@ -127,6 +154,7 @@ test.each([
                     strategyName: 'flexibleRollout',
                     environment: 'default',
                     featureName: FLAG_NAME,
+                    changeRequestIds: [CR_ID],
                 },
             ]);
         } else {
@@ -165,6 +193,7 @@ test.each([
                     strategyName: 'flexibleRollout',
                     environment: 'default',
                     featureName: FLAG_NAME,
+                    changeRequestIds: [CR_ID],
                 },
             ]);
         } else {
@@ -172,3 +201,35 @@ test.each([
         }
     },
 );
+
+test(`If the same strategy appears in multiple CRs with the same segment, they should all be listed in its changeRequestIds`, async () => {
+    await createCR('In review', CR_ID);
+    await createCR('In review', CR_ID_2);
+
+    const segmentId = 3;
+    const strategyId = randomId();
+
+    await updateStrategyInCr(strategyId, segmentId, FLAG_NAME, CR_ID);
+    await updateStrategyInCr(strategyId, segmentId, FLAG_NAME, CR_ID_2);
+
+    const result = await readModel.getStrategiesUsedInActiveChangeRequests(
+        segmentId,
+    );
+
+    expect(result).toHaveLength(1);
+
+    expect(result).toMatchObject([
+        {
+            id: strategyId,
+            projectId: 'default',
+            strategyName: 'flexibleRollout',
+            environment: 'default',
+            featureName: FLAG_NAME,
+        },
+    ]);
+
+    const crIds = result[0].changeRequestIds;
+    expect(crIds).toContain(CR_ID);
+    expect(crIds).toContain(CR_ID_2);
+    expect(crIds).toHaveLength(2);
+});

src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.ts

@@ -3,6 +3,7 @@ type NewStrategy = {
     featureName: string;
     strategyName: string;
     environment: string;
+    changeRequestIds: [string, string[]];
 };
 
 type ExistingStrategy = NewStrategy & { id: string };

src/lib/features/change-request-segment-usage-service/sql-change-request-segment-usage-read-model.ts

@@ -13,17 +13,6 @@ export class ChangeRequestSegmentUsageReadModel
         this.db = db;
     }
 
-    mapRow = (row): ChangeRequestStrategy => {
-        const { payload, project, environment, feature } = row;
-        return {
-            projectId: project,
-            featureName: feature,
-            environment: environment,
-            strategyName: payload.name,
-            ...(payload.id ? { id: payload.id } : {}),
-        };
-    };
-
     public async getStrategiesUsedInActiveChangeRequests(
         segmentId: number,
     ): Promise<ChangeRequestStrategy[]> {
@@ -38,8 +27,35 @@ export class ChangeRequestSegmentUsageReadModel
         const queryResult = await query;
         const strategies = queryResult.rows
             .filter((row) => row.payload?.segments?.includes(segmentId))
-            .map(this.mapRow);
+            .map((row) => {
+                const { payload, project, environment, feature } = row;
+                return {
+                    projectId: project,
+                    featureName: feature,
+                    environment: environment,
+                    strategyName: payload.name,
+                    ...(payload.id ? { id: payload.id } : {}),
+                    changeRequestId: row.change_request_id,
+                };
+            });
 
-        return strategies;
+        const deduped = strategies.reduce((acc, strategy) => {
+            const { changeRequestId, ...rest } = strategy;
+
+            const existingData = acc[strategy.id];
+
+            if (existingData) {
+                existingData.changeRequestIds.push(strategy.changeRequestId);
+            } else {
+                acc[strategy.id] = {
+                    ...rest,
+                    changeRequestIds: [strategy.changeRequestId],
+                };
+            }
+
+            return acc;
+        }, {});
+
+        return Object.values(deduped);
     }
 }

src/lib/features/segment/segment-controller.ts

@@ -369,6 +369,7 @@ export class SegmentsController extends Controller {
                 featureName: strategy.featureName,
                 strategyName: strategy.strategyName,
                 environment: strategy.environment,
+                changeRequestIds: strategy.changeRequestIds,
             });
 
             res.json({

src/test/e2e/api/admin/segment.e2e.test.ts

@@ -529,6 +529,7 @@ describe('detect strategy usage in change requests', () => {
                 featureName: toggle.name,
                 projectId: 'default',
                 strategyName: 'flexibleRollout',
+                changeRequestIds: [CR_ID],
             },
         ]);
         expect(strategies).toStrictEqual([]);
@@ -576,7 +577,9 @@ describe('detect strategy usage in change requests', () => {
         const { strategies, changeRequestStrategies } =
             await fetchSegmentStrategies(segment.id);
 
-        expect(changeRequestStrategies).toMatchObject([{ id: strategyId }]);
+        expect(changeRequestStrategies).toMatchObject([
+            { id: strategyId, changeRequestIds: [CR_ID] },
+        ]);
         expect(strategies).toStrictEqual([]);
     });