From f46d5a9269ccf7c8e55fc19b8c741f3b80e77411 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 27 Nov 2023 11:20:39 +0100 Subject: [PATCH] chore: update segment cr return values (#5405) This PR updates the returned value about segments to also include the CR title and to be one list item per strategy per change request. This means that if the same strategy is used multiple times in multiple change requests, they each get their own line (as has been discussed with Nicolae). Because of this, this pr removes a collection step in the query and fixes some test cases. --- ...e-request-segment-usage-read-model.test.ts | 57 +++++++++++-------- ...change-request-segment-usage-read-model.ts | 4 +- ...change-request-segment-usage-read-model.ts | 26 ++------- .../features/segment/segment-controller.ts | 2 +- src/test/e2e/api/admin/segment.e2e.test.ts | 10 +++- 5 files changed, 50 insertions(+), 49 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 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([]); });