From fac25789220653c6d892567e27c85761142874de Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Wed, 22 Nov 2023 10:26:35 +0100 Subject: [PATCH] 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. --- ...e-request-segment-usage-read-model.test.ts | 123 +++++++++++++----- ...change-request-segment-usage-read-model.ts | 1 + ...change-request-segment-usage-read-model.ts | 42 ++++-- .../features/segment/segment-controller.ts | 1 + src/test/e2e/api/admin/segment.e2e.test.ts | 5 +- 5 files changed, 127 insertions(+), 45 deletions(-) diff --git a/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts b/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts index 489900b128..4bf1b60a5d 100644 --- a/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts +++ b/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); +}); diff --git a/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.ts b/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.ts index 7eead7db03..6e5aa98b5a 100644 --- a/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.ts +++ b/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 }; diff --git a/src/lib/features/change-request-segment-usage-service/sql-change-request-segment-usage-read-model.ts b/src/lib/features/change-request-segment-usage-service/sql-change-request-segment-usage-read-model.ts index 6c437ff3ee..e5cb9a03df 100644 --- a/src/lib/features/change-request-segment-usage-service/sql-change-request-segment-usage-read-model.ts +++ b/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 { @@ -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); } } diff --git a/src/lib/features/segment/segment-controller.ts b/src/lib/features/segment/segment-controller.ts index 58bfc3e6a2..833b05d353 100644 --- a/src/lib/features/segment/segment-controller.ts +++ b/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({ diff --git a/src/test/e2e/api/admin/segment.e2e.test.ts b/src/test/e2e/api/admin/segment.e2e.test.ts index 03ea5280d2..1824561f6c 100644 --- a/src/test/e2e/api/admin/segment.e2e.test.ts +++ b/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([]); });