Mirrors/unleash.unleash
1
0
Fork 0
mirror of https://github.com/Unleash/unleash.git synced 2025-04-15 01:16:22 +02:00
Code Issues Projects Releases Wiki Activity

chore: update segment cr return values (#5405)

Browse Source 
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.
This commit is contained in:
Thomas Heartman 2023-11-27 11:20:39 +01:00 committed by GitHub
parent 0a43d341c0
commit f46d5a9269
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 50 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -10,6 +10,9 @@ let user: IUser;
const CR_ID = 123456; const CR_ID = 123456;
const CR_ID_2 = 234567; const CR_ID_2 = 234567;
const CR_TITLE = 'My change request';
const FLAG_NAME = 'crarm-test-flag'; const FLAG_NAME = 'crarm-test-flag';
let readModel: IChangeRequestSegmentUsageReadModel; let readModel: IChangeRequestSegmentUsageReadModel;
@ -46,7 +49,11 @@ afterEach(async () => {
.delete(); .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({ await db.rawDatabase.table('change_requests').insert({
id: changeRequestId, id: changeRequestId,
environment: 'default', environment: 'default',
@ -55,7 +62,7 @@ const createCR = async (state, changeRequestId = CR_ID) => {
created_by: user.id, created_by: user.id,
created_at: '2023-01-01 00:00:00', created_at: '2023-01-01 00:00:00',
min_approvals: 1, min_approvals: 1,
title: 'My change request', title: changeRequestTitle,
}); });
}; };
@ -136,7 +143,7 @@ test.each([
['Cancelled', false], ['Cancelled', false],
['Applied', 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) => { async (state, isActiveCr) => {
await createCR(state); await createCR(state);
@ -154,7 +161,7 @@ test.each([
strategyName: 'flexibleRollout', strategyName: 'flexibleRollout',
environment: 'default', environment: 'default',
featureName: FLAG_NAME, featureName: FLAG_NAME,
changeRequestIds: [CR_ID], changeRequest: { id: CR_ID, title: CR_TITLE },
}, },
]); ]);
} else { } else {
@ -172,7 +179,7 @@ test.each([
['Cancelled', false], ['Cancelled', false],
['Applied', 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) => { async (state, isActiveCr) => {
await createCR(state); await createCR(state);
@ -193,7 +200,7 @@ test.each([
strategyName: 'flexibleRollout', strategyName: 'flexibleRollout',
environment: 'default', environment: 'default',
featureName: FLAG_NAME, featureName: FLAG_NAME,
changeRequestIds: [CR_ID], changeRequest: { id: CR_ID, title: CR_TITLE },
}, },
]); ]);
} else { } 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 () => { 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); await createCR('In review', CR_ID, CR_TITLE);
await createCR('In review', CR_ID_2); await createCR('In review', CR_ID_2, null);
const segmentId = 3; const segmentId = 3;
const strategyId = randomId(); const strategyId = randomId();
@ -216,20 +223,22 @@ test(`If the same strategy appears in multiple CRs with the same segment, they s
segmentId, segmentId,
); );
expect(result).toHaveLength(1); expect(result).toHaveLength(2);
expect(result).toMatchObject([ expect(result).toContainEqual({
{ id: strategyId,
id: strategyId, projectId: 'default',
projectId: 'default', strategyName: 'flexibleRollout',
strategyName: 'flexibleRollout', environment: 'default',
environment: 'default', featureName: FLAG_NAME,
featureName: FLAG_NAME, changeRequest: { id: CR_ID, title: CR_TITLE },
}, });
]); expect(result).toContainEqual({
id: strategyId,
const crIds = result[0].changeRequestIds; projectId: 'default',
expect(crIds).toContain(CR_ID); strategyName: 'flexibleRollout',
expect(crIds).toContain(CR_ID_2); environment: 'default',
expect(crIds).toHaveLength(2); featureName: FLAG_NAME,
changeRequest: { id: CR_ID_2, title: null },
});
}); });

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

@ -1,9 +1,11 @@
type ChangeRequestInfo = { id: number; title: string | null };
type NewStrategy = { type NewStrategy = {
projectId: string; projectId: string;
featureName: string; featureName: string;
strategyName: string; strategyName: string;
environment: string; environment: string;
changeRequestIds: [string, string[]]; changeRequest: ChangeRequestInfo;
}; };
type ExistingStrategy = NewStrategy & { id: string }; type ExistingStrategy = NewStrategy & { id: string };

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

@ -17,7 +17,7 @@ export class ChangeRequestSegmentUsageReadModel
segmentId: number, segmentId: number,
): Promise<ChangeRequestStrategy[]> { ): Promise<ChangeRequestStrategy[]> {
const query = this.db.raw( const query = this.db.raw(
`SELECT events.*, cr.project, cr.environment `SELECT events.*, cr.project, cr.environment, cr.title
FROM change_request_events events FROM change_request_events events
JOIN change_requests cr ON events.change_request_id = cr.id JOIN change_requests cr ON events.change_request_id = cr.id
WHERE cr.state NOT IN ('Applied', 'Cancelled', 'Rejected') WHERE cr.state NOT IN ('Applied', 'Cancelled', 'Rejected')
@ -35,27 +35,13 @@ export class ChangeRequestSegmentUsageReadModel
environment: environment, environment: environment,
strategyName: payload.name, strategyName: payload.name,
...(payload.id ? { id: payload.id } : {}), ...(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) => { return strategies;
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);
} }
} }

2
src/lib/features/segment/segment-controller.ts
View File

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

10
src/test/e2e/api/admin/segment.e2e.test.ts
View File

@ -431,6 +431,7 @@ test('Should show usage in features and projects', async () => {
}); });
describe('detect strategy usage in change requests', () => { describe('detect strategy usage in change requests', () => {
const CR_TITLE = 'My change request';
const CR_ID = 54321; const CR_ID = 54321;
let user; let user;
@ -447,7 +448,7 @@ describe('detect strategy usage in change requests', () => {
created_by: user.id, created_by: user.id,
created_at: '2023-01-01 00:00:00', created_at: '2023-01-01 00:00:00',
min_approvals: 1, min_approvals: 1,
title: 'My change request', title: CR_TITLE,
}); });
}); });
afterAll(async () => { afterAll(async () => {
@ -531,7 +532,7 @@ describe('detect strategy usage in change requests', () => {
featureName: toggle.name, featureName: toggle.name,
projectId: 'default', projectId: 'default',
strategyName: 'flexibleRollout', strategyName: 'flexibleRollout',
changeRequestIds: [CR_ID], changeRequest: { id: CR_ID, title: CR_TITLE },
}, },
]); ]);
expect(strategies).toStrictEqual([]); expect(strategies).toStrictEqual([]);
@ -580,7 +581,10 @@ describe('detect strategy usage in change requests', () => {
await fetchSegmentStrategies(segment.id); await fetchSegmentStrategies(segment.id);
expect(changeRequestStrategies).toMatchObject([ expect(changeRequestStrategies).toMatchObject([
{ id: strategyId, changeRequestIds: [CR_ID] }, {
id: strategyId,
changeRequest: { id: CR_ID, title: CR_TITLE },
},
]); ]);
expect(strategies).toStrictEqual([]); expect(strategies).toStrictEqual([]);
}); });