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