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([]); });