From 8337885e4796c9f8288ab92a6dcf2d1f8506e164 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Wed, 22 Nov 2023 07:51:04 +0100 Subject: [PATCH] feat: return CR uses of segments when flag is active (#5378) This PR changes the payload of the strategiesBySegment endpoint when the flag is active. In addition to returning just the strategies, the object will also contain a new property, called `changeRequestStrategies` containing the strategies that are used in change requests. This PR does not update the schema. That can be done later when the changes go into beta. This also allows us some time to iterate on the payload without changing the public API. ## Discussion points: Should `strategies` and `changeRequestStrategies` ever contain duplicates? Take this scenario: - Strategy S uses segment T. - There is an open change request that updates the list of segments for S to T and a new segment U. - In this case, strategy S would show up both in `strategies` _and_ in `changeRequestStrategies`. We have two options: 1. Filter the list of change request strategies, so that they don't contain any duplicates (this is currently how it's implemented) 2. Ignore the duplicates and just send both lists as is. We're doing option 2 for now. --- .../features/segment/segment-controller.ts | 48 +++- src/lib/segments/segment-service-interface.ts | 10 +- src/lib/services/segment-service.ts | 56 ++-- src/test/e2e/api/admin/segment.e2e.test.ts | 263 ++++++++++++++---- 4 files changed, 286 insertions(+), 91 deletions(-) diff --git a/src/lib/features/segment/segment-controller.ts b/src/lib/features/segment/segment-controller.ts index a54caa58ff..58bfc3e6a2 100644 --- a/src/lib/features/segment/segment-controller.ts +++ b/src/lib/features/segment/segment-controller.ts @@ -347,25 +347,47 @@ export class SegmentsController extends Controller { req: IAuthRequest<{ id: number }>, res: Response, ): Promise { - const { id } = req.params; + const id = Number(req.params.id); const { user } = req; const strategies = await this.segmentService.getVisibleStrategies( id, user.id, ); - // Remove unnecessary IFeatureStrategy fields from the response. - const segmentStrategies = strategies.map((strategy) => ({ - id: strategy.id, - projectId: strategy.projectId, - featureName: strategy.featureName, - strategyName: strategy.strategyName, - environment: strategy.environment, - })); + if (this.flagResolver.isEnabled('detectSegmentUsageInChangeRequests')) { + const mapStrategies = (strategy) => ({ + id: strategy.id, + projectId: strategy.projectId, + featureName: strategy.featureName, + strategyName: strategy.strategyName, + environment: strategy.environment, + }); - res.json({ - strategies: segmentStrategies, - }); + const mapChangeRequestStrategies = (strategy) => ({ + ...(strategy.id ? { id: strategy.id } : {}), + projectId: strategy.projectId, + featureName: strategy.featureName, + strategyName: strategy.strategyName, + environment: strategy.environment, + }); + + res.json({ + strategies: strategies.strategies.map(mapStrategies), + changeRequestStrategies: strategies.changeRequestStrategies.map( + mapChangeRequestStrategies, + ), + }); + } else { + const segmentStrategies = strategies.strategies.map((strategy) => ({ + id: strategy.id, + projectId: strategy.projectId, + featureName: strategy.featureName, + strategyName: strategy.strategyName, + environment: strategy.environment, + })); + + res.json({ strategies: segmentStrategies }); + } } async removeSegment( @@ -379,7 +401,7 @@ export class SegmentsController extends Controller { segmentIsInUse = await this.segmentService.isInUse(id); } else { const strategies = await this.segmentService.getAllStrategies(id); - segmentIsInUse = strategies.length > 0; + segmentIsInUse = strategies.strategies.length > 0; } if (segmentIsInUse) { diff --git a/src/lib/segments/segment-service-interface.ts b/src/lib/segments/segment-service-interface.ts index 8740ecbca2..08fc3b0cb8 100644 --- a/src/lib/segments/segment-service-interface.ts +++ b/src/lib/segments/segment-service-interface.ts @@ -1,6 +1,12 @@ +import { ChangeRequestStrategy } from 'lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model'; import { UpsertSegmentSchema } from 'lib/openapi'; import { IClientSegment, IFeatureStrategy, ISegment, IUser } from 'lib/types'; +export type StrategiesUsingSegment = { + strategies: IFeatureStrategy[]; + changeRequestStrategies: ChangeRequestStrategy[]; +}; + export interface ISegmentService { updateStrategySegments: ( strategyId: string, @@ -18,12 +24,12 @@ export interface ISegmentService { * This is NOT considering the private projects * For most use cases, use `getVisibleStrategies` */ - getAllStrategies(id: number): Promise; + getAllStrategies(id: number): Promise; getVisibleStrategies( id: number, userId: number, - ): Promise; + ): Promise; validateName(name: string): Promise; diff --git a/src/lib/services/segment-service.ts b/src/lib/services/segment-service.ts index cf1ab7224a..875196212d 100644 --- a/src/lib/services/segment-service.ts +++ b/src/lib/services/segment-service.ts @@ -18,7 +18,10 @@ import { import User from '../types/user'; import { IFeatureStrategiesStore } from '../features/feature-toggle/types/feature-toggle-strategies-store-type'; import BadDataError from '../error/bad-data-error'; -import { ISegmentService } from '../segments/segment-service-interface'; +import { + ISegmentService, + StrategiesUsingSegment, +} from '../segments/segment-service-interface'; import { PermissionError } from '../error'; import { IChangeRequestAccessReadModel } from '../features/change-request-access-service/change-request-access-read-model'; import { IPrivateProjectChecker } from '../features/private-project/privateProjectCheckerType'; @@ -90,39 +93,50 @@ export class SegmentService implements ISegmentService { async getVisibleStrategies( id: number, userId: number, - ): Promise { - const strategies = await this.getAllStrategies(id); + ): Promise { + const allStrategies = await this.getAllStrategies(id); if (this.flagResolver.isEnabled('privateProjects')) { const accessibleProjects = await this.privateProjectChecker.getUserAccessibleProjects( userId, ); if (accessibleProjects.mode === 'all') { - return strategies; + return allStrategies; } else { - return strategies.filter((strategy) => - accessibleProjects.projects.includes(strategy.projectId), - ); + const filter = (strategy) => + accessibleProjects.projects.includes(strategy.projectId); + return { + strategies: allStrategies.strategies.filter(filter), + changeRequestStrategies: + allStrategies.changeRequestStrategies.filter(filter), + }; } } - return strategies; + return allStrategies; } - async getAllStrategies(id: number): Promise { + async getAllStrategies(id: number): Promise { const strategies = await this.featureStrategiesStore.getStrategiesBySegment(id); - return strategies; + + const strategyIds = new Set(strategies.map((s) => s.id)); + + const changeRequestStrategies = ( + await this.changeRequestSegmentUsageReadModel.getStrategiesUsedInActiveChangeRequests( + id, + ) + ).filter( + (strategy) => !('id' in strategy && strategyIds.has(strategy.id)), + ); + + return { strategies, changeRequestStrategies }; } async isInUse(id: number): Promise { - const strategies = await this.getAllStrategies(id); - if (strategies.length > 0) { - return true; - } + const { strategies, changeRequestStrategies } = + await this.getAllStrategies(id); - return await this.changeRequestSegmentUsageReadModel.isSegmentUsedInActiveChangeRequests( - id, - ); + return strategies.length > 0 || changeRequestStrategies.length > 0; } async create( @@ -300,11 +314,13 @@ export class SegmentService implements ISegmentService { id: number, segment: Omit, ): Promise { - const strategies = - await this.featureStrategiesStore.getStrategiesBySegment(id); + const { strategies, changeRequestStrategies } = + await this.getAllStrategies(id); const projectsUsed = new Set( - strategies.map((strategy) => strategy.projectId), + [strategies, changeRequestStrategies].flatMap((strats) => + strats.map((strategy) => strategy.projectId), + ), ); if ( diff --git a/src/test/e2e/api/admin/segment.e2e.test.ts b/src/test/e2e/api/admin/segment.e2e.test.ts index e4e6cf2236..03ea5280d2 100644 --- a/src/test/e2e/api/admin/segment.e2e.test.ts +++ b/src/test/e2e/api/admin/segment.e2e.test.ts @@ -15,6 +15,7 @@ import { IUnleashTest, setupAppWithCustomConfig, } from '../../helpers/test-helper'; +import { StrategiesUsingSegment } from 'lib/segments/segment-service-interface'; let app: IUnleashTest; let db: ITestDb; @@ -49,11 +50,11 @@ const fetchFeatures = (): Promise => const fetchSegmentStrategies = ( segmentId: number, -): Promise => +): Promise => app.request .get(`${SEGMENTS_BASE_PATH}/${segmentId}/strategies`) .expect(200) - .then((res) => res.body.strategies); + .then((res) => res.body); const createSegment = ( postData: object, @@ -234,57 +235,6 @@ test('should not delete segments used by strategies', async () => { expect((await fetchSegments()).length).toEqual(1); }); -test('should not delete segments used by strategies in CRs', async () => { - await createSegment({ name: 'a', constraints: [] }); - const toggle = mockFeatureToggle(); - await createFeatureToggle(app, toggle); - const [segment] = await fetchSegments(); - - const CR_ID = 54321; - - const user = await db.stores.userStore.insert({ - username: 'test', - }); - - await db.rawDatabase.table('change_requests').insert({ - id: CR_ID, - environment: 'default', - state: 'In review', - project: 'default', - created_by: user.id, - created_at: '2023-01-01 00:00:00', - min_approvals: 1, - title: 'My change request', - }); - - await db.rawDatabase.table('change_request_events').insert({ - feature: toggle.name, - action: 'addStrategy', - payload: { - name: 'flexibleRollout', - title: '', - disabled: false, - segments: [segment.id], - variants: [], - parameters: { - groupId: toggle.name, - rollout: '100', - stickiness: 'default', - }, - constraints: [], - }, - created_at: '2023-01-01 00:01:00', - change_request_id: CR_ID, - created_by: user.id, - }); - - expect((await fetchSegments()).length).toEqual(1); - - await app.request.delete(`${SEGMENTS_BASE_PATH}/${segment.id}`).expect(409); - - expect((await fetchSegments()).length).toEqual(1); -}); - test('should list strategies by segment', async () => { await createSegment({ name: 'S1', constraints: [] }); await createSegment({ name: 'S2', constraints: [] }); @@ -335,15 +285,15 @@ test('should list strategies by segment', async () => { const segmentStrategies2 = await fetchSegmentStrategies(segment2.id); const segmentStrategies3 = await fetchSegmentStrategies(segment3.id); - expect(collectIds(segmentStrategies1)).toEqual( + expect(collectIds(segmentStrategies1.strategies)).toEqual( collectIds(feature1.strategies), ); - expect(collectIds(segmentStrategies2)).toEqual( + expect(collectIds(segmentStrategies2.strategies)).toEqual( collectIds([...feature1.strategies, ...feature2.strategies]), ); - expect(collectIds(segmentStrategies3)).toEqual( + expect(collectIds(segmentStrategies3.strategies)).toEqual( collectIds([ ...feature1.strategies, ...feature2.strategies, @@ -477,3 +427,204 @@ test('Should show usage in features and projects', async () => { const segments = await fetchSegments(); expect(segments).toMatchObject([{ usedInFeatures: 1, usedInProjects: 1 }]); }); + +describe('detect strategy usage in change requests', () => { + const CR_ID = 54321; + let user; + + beforeAll(async () => { + user = await db.stores.userStore.insert({ + username: 'test', + }); + + await db.rawDatabase.table('change_requests').insert({ + id: CR_ID, + environment: 'default', + state: 'In review', + project: 'default', + created_by: user.id, + created_at: '2023-01-01 00:00:00', + min_approvals: 1, + title: 'My change request', + }); + }); + afterAll(async () => { + user = await db.stores.userStore.delete(user.id); + await db.rawDatabase.table('change_requests').delete(); + }); + + afterEach(async () => { + await db.rawDatabase.table('change_request_events').delete(); + }); + + test('should not delete segments used by strategies in CRs', async () => { + await createSegment({ name: 'a', constraints: [] }); + const toggle = mockFeatureToggle(); + await createFeatureToggle(app, toggle); + const [segment] = await fetchSegments(); + + await db.rawDatabase.table('change_request_events').insert({ + feature: toggle.name, + action: 'addStrategy', + payload: { + name: 'flexibleRollout', + title: '', + disabled: false, + segments: [segment.id], + variants: [], + parameters: { + groupId: toggle.name, + rollout: '100', + stickiness: 'default', + }, + constraints: [], + }, + created_at: '2023-01-01 00:01:00', + change_request_id: CR_ID, + created_by: user.id, + }); + + expect((await fetchSegments()).length).toEqual(1); + + await app.request + .delete(`${SEGMENTS_BASE_PATH}/${segment.id}`) + .expect(409); + + expect((await fetchSegments()).length).toEqual(1); + }); + + test('Should show segment usage in addStrategy events', async () => { + await createSegment({ name: 'a', constraints: [] }); + const toggle = mockFeatureToggle(); + await createFeatureToggle(app, toggle); + const [segment] = await fetchSegments(); + + await db.rawDatabase.table('change_request_events').insert({ + feature: toggle.name, + action: 'addStrategy', + payload: { + name: 'flexibleRollout', + title: '', + disabled: false, + segments: [segment.id], + variants: [], + parameters: { + groupId: toggle.name, + rollout: '100', + stickiness: 'default', + }, + constraints: [], + }, + created_at: '2023-01-01 00:01:00', + change_request_id: CR_ID, + created_by: user.id, + }); + + const { strategies, changeRequestStrategies } = + await fetchSegmentStrategies(segment.id); + + expect(changeRequestStrategies).toMatchObject([ + { + environment: 'default', + featureName: toggle.name, + projectId: 'default', + strategyName: 'flexibleRollout', + }, + ]); + expect(strategies).toStrictEqual([]); + }); + + test('Should show segment usage in updateStrategy events', async () => { + await createSegment({ name: 'a', constraints: [] }); + const toggle = mockFeatureToggle(); + await createFeatureToggle(app, toggle); + const [segment] = await fetchSegments(); + + await addStrategyToFeatureEnv( + app, + { ...toggle.strategies[0] }, + 'default', + toggle.name, + ); + + const [feature] = await fetchFeatures(); + + const strategyId = feature.strategies[0].id; + + await db.rawDatabase.table('change_request_events').insert({ + feature: toggle.name, + action: 'updateStrategy', + payload: { + id: strategyId, + name: 'flexibleRollout', + title: '', + disabled: false, + segments: [segment.id], + variants: [], + parameters: { + groupId: toggle.name, + rollout: '100', + stickiness: 'default', + }, + constraints: [], + }, + created_at: '2023-01-01 00:01:00', + change_request_id: CR_ID, + created_by: user.id, + }); + + const { strategies, changeRequestStrategies } = + await fetchSegmentStrategies(segment.id); + + expect(changeRequestStrategies).toMatchObject([{ id: strategyId }]); + expect(strategies).toStrictEqual([]); + }); + + test('If a segment is used in an existing strategy and in a CR for the same strategy, the strategy should only be listed once', async () => { + await createSegment({ name: 'a', constraints: [] }); + const toggle = mockFeatureToggle(); + await createFeatureToggle(app, toggle); + const [segment] = await fetchSegments(); + + await addStrategyToFeatureEnv( + app, + { ...toggle.strategies[0] }, + 'default', + toggle.name, + ); + + const [feature] = await fetchFeatures(); + + const strategyId = feature.strategies[0].id; + await addSegmentsToStrategy([segment.id], strategyId!); + + await db.rawDatabase.table('change_request_events').insert({ + feature: toggle.name, + action: 'updateStrategy', + payload: { + id: strategyId, + name: 'flexibleRollout', + title: '', + disabled: false, + segments: [segment.id], + variants: [], + parameters: { + groupId: toggle.name, + rollout: '100', + stickiness: 'default', + }, + constraints: [], + }, + created_at: '2023-01-01 00:01:00', + change_request_id: CR_ID, + created_by: user.id, + }); + + const { strategies, changeRequestStrategies } = + await fetchSegmentStrategies(segment.id); + + expect(strategies).toMatchObject([{ id: strategyId }]); + + expect(changeRequestStrategies).toStrictEqual([]); + }); +});