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 4bf1b60a5d..8132637362 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 @@ -10,6 +10,9 @@ let user: IUser; const CR_ID = 123456; const CR_ID_2 = 234567; + +const CR_TITLE = 'My change request'; + const FLAG_NAME = 'crarm-test-flag'; let readModel: IChangeRequestSegmentUsageReadModel; @@ -46,7 +49,11 @@ afterEach(async () => { .delete(); }); -const createCR = async (state, changeRequestId = CR_ID) => { +const createCR = async ( + state, + changeRequestId = CR_ID, + changeRequestTitle: string | null = CR_TITLE, +) => { await db.rawDatabase.table('change_requests').insert({ id: changeRequestId, environment: 'default', @@ -55,7 +62,7 @@ const createCR = async (state, changeRequestId = CR_ID) => { created_by: user.id, created_at: '2023-01-01 00:00:00', min_approvals: 1, - title: 'My change request', + title: changeRequestTitle, }); }; @@ -136,7 +143,7 @@ test.each([ ['Cancelled', false], ['Applied', false], ])( - 'addStrategy events in %s CRs should show up only of the CR is active', + 'addStrategy events in %s CRs should show up only if the CR is active', async (state, isActiveCr) => { await createCR(state); @@ -154,7 +161,7 @@ test.each([ strategyName: 'flexibleRollout', environment: 'default', featureName: FLAG_NAME, - changeRequestIds: [CR_ID], + changeRequest: { id: CR_ID, title: CR_TITLE }, }, ]); } else { @@ -172,7 +179,7 @@ test.each([ ['Cancelled', false], ['Applied', false], ])( - `updateStrategy events in %s CRs should show up only of the CR is active`, + `updateStrategy events in %s CRs should show up only if the CR is active`, async (state, isActiveCr) => { await createCR(state); @@ -193,7 +200,7 @@ test.each([ strategyName: 'flexibleRollout', environment: 'default', featureName: FLAG_NAME, - changeRequestIds: [CR_ID], + changeRequest: { id: CR_ID, title: CR_TITLE }, }, ]); } else { @@ -202,9 +209,9 @@ 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); +test(`If the same strategy appears in multiple CRs with the same segment, each segment should be listed as its own entry`, async () => { + await createCR('In review', CR_ID, CR_TITLE); + await createCR('In review', CR_ID_2, null); const segmentId = 3; const strategyId = randomId(); @@ -216,20 +223,22 @@ test(`If the same strategy appears in multiple CRs with the same segment, they s segmentId, ); - expect(result).toHaveLength(1); + expect(result).toHaveLength(2); - 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); + expect(result).toContainEqual({ + id: strategyId, + projectId: 'default', + strategyName: 'flexibleRollout', + environment: 'default', + featureName: FLAG_NAME, + changeRequest: { id: CR_ID, title: CR_TITLE }, + }); + expect(result).toContainEqual({ + id: strategyId, + projectId: 'default', + strategyName: 'flexibleRollout', + environment: 'default', + featureName: FLAG_NAME, + changeRequest: { id: CR_ID_2, title: null }, + }); }); 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 6e5aa98b5a..3b910e43a6 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 @@ -1,9 +1,11 @@ +type ChangeRequestInfo = { id: number; title: string | null }; + type NewStrategy = { projectId: string; featureName: string; strategyName: string; environment: string; - changeRequestIds: [string, string[]]; + changeRequest: ChangeRequestInfo; }; 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 e5cb9a03df..9076c11282 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 @@ -17,7 +17,7 @@ export class ChangeRequestSegmentUsageReadModel segmentId: number, ): Promise { const query = this.db.raw( - `SELECT events.*, cr.project, cr.environment + `SELECT events.*, cr.project, cr.environment, cr.title FROM change_request_events events JOIN change_requests cr ON events.change_request_id = cr.id WHERE cr.state NOT IN ('Applied', 'Cancelled', 'Rejected') @@ -35,27 +35,13 @@ export class ChangeRequestSegmentUsageReadModel environment: environment, strategyName: payload.name, ...(payload.id ? { id: payload.id } : {}), - changeRequestId: row.change_request_id, + changeRequest: { + id: row.change_request_id, + title: row.title || null, + }, }; }); - 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); + return strategies; } } diff --git a/src/lib/features/segment/segment-controller.ts b/src/lib/features/segment/segment-controller.ts index 833b05d353..8ecf1b5e80 100644 --- a/src/lib/features/segment/segment-controller.ts +++ b/src/lib/features/segment/segment-controller.ts @@ -369,7 +369,7 @@ export class SegmentsController extends Controller { featureName: strategy.featureName, strategyName: strategy.strategyName, environment: strategy.environment, - changeRequestIds: strategy.changeRequestIds, + changeRequest: strategy.changeRequest, }); 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 3845e8f965..438e1efe46 100644 --- a/src/test/e2e/api/admin/segment.e2e.test.ts +++ b/src/test/e2e/api/admin/segment.e2e.test.ts @@ -431,6 +431,7 @@ test('Should show usage in features and projects', async () => { }); describe('detect strategy usage in change requests', () => { + const CR_TITLE = 'My change request'; const CR_ID = 54321; let user; @@ -447,7 +448,7 @@ describe('detect strategy usage in change requests', () => { created_by: user.id, created_at: '2023-01-01 00:00:00', min_approvals: 1, - title: 'My change request', + title: CR_TITLE, }); }); afterAll(async () => { @@ -531,7 +532,7 @@ describe('detect strategy usage in change requests', () => { featureName: toggle.name, projectId: 'default', strategyName: 'flexibleRollout', - changeRequestIds: [CR_ID], + changeRequest: { id: CR_ID, title: CR_TITLE }, }, ]); expect(strategies).toStrictEqual([]); @@ -580,7 +581,10 @@ describe('detect strategy usage in change requests', () => { await fetchSegmentStrategies(segment.id); expect(changeRequestStrategies).toMatchObject([ - { id: strategyId, changeRequestIds: [CR_ID] }, + { + id: strategyId, + changeRequest: { id: CR_ID, title: CR_TITLE }, + }, ]); expect(strategies).toStrictEqual([]); });