diff --git a/src/lib/features/segment/segment-controller.ts b/src/lib/features/segment/segment-controller.ts index 8ecf1b5e8045..7c96513395dc 100644 --- a/src/lib/features/segment/segment-controller.ts +++ b/src/lib/features/segment/segment-controller.ts @@ -354,39 +354,30 @@ export class SegmentsController extends Controller { user.id, ); - if (this.flagResolver.isEnabled('detectSegmentUsageInChangeRequests')) { - const mapStrategies = (strategy) => ({ - id: strategy.id, - projectId: strategy.projectId, - featureName: strategy.featureName, - strategyName: strategy.strategyName, - environment: strategy.environment, - }); + const segmentStrategies = strategies.strategies.map((strategy) => ({ + id: strategy.id, + projectId: strategy.projectId, + featureName: strategy.featureName, + strategyName: strategy.strategyName, + environment: strategy.environment, + })); - const mapChangeRequestStrategies = (strategy) => ({ - ...(strategy.id ? { id: strategy.id } : {}), - projectId: strategy.projectId, - featureName: strategy.featureName, - strategyName: strategy.strategyName, - environment: strategy.environment, - changeRequest: strategy.changeRequest, - }); + if (this.flagResolver.isEnabled('detectSegmentUsageInChangeRequests')) { + const changeRequestStrategies = + strategies.changeRequestStrategies.map((strategy) => ({ + ...('id' in strategy ? { id: strategy.id } : {}), + projectId: strategy.projectId, + featureName: strategy.featureName, + strategyName: strategy.strategyName, + environment: strategy.environment, + changeRequest: strategy.changeRequest, + })); res.json({ - strategies: strategies.strategies.map(mapStrategies), - changeRequestStrategies: strategies.changeRequestStrategies.map( - mapChangeRequestStrategies, - ), + strategies: segmentStrategies, + changeRequestStrategies, }); } 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 }); } } diff --git a/src/lib/services/segment-service.ts b/src/lib/services/segment-service.ts index f42609e97263..7565a29ab239 100644 --- a/src/lib/services/segment-service.ts +++ b/src/lib/services/segment-service.ts @@ -119,12 +119,19 @@ export class SegmentService implements ISegmentService { const strategies = await this.featureStrategiesStore.getStrategiesBySegment(id); - const changeRequestStrategies = - await this.changeRequestSegmentUsageReadModel.getStrategiesUsedInActiveChangeRequests( - id, - ); + if ( + this.flagResolver.isEnabled('detectSegmentUsageInChangeRequests') && + this.config.isEnterprise + ) { + const changeRequestStrategies = + await this.changeRequestSegmentUsageReadModel.getStrategiesUsedInActiveChangeRequests( + id, + ); + + return { strategies, changeRequestStrategies }; + } - return { strategies, changeRequestStrategies }; + return { strategies, changeRequestStrategies: [] }; } async isInUse(id: number): Promise { diff --git a/src/test/e2e/api/admin/segment.e2e.test.ts b/src/test/e2e/api/admin/segment.e2e.test.ts index 438e1efe4646..8dd98d1c2549 100644 --- a/src/test/e2e/api/admin/segment.e2e.test.ts +++ b/src/test/e2e/api/admin/segment.e2e.test.ts @@ -435,7 +435,43 @@ describe('detect strategy usage in change requests', () => { const CR_ID = 54321; let user; + // Change request data is only counted for enterprise + // instances, so we'll instantiate our own version of the app + // for that. + let enterpriseApp; + + // likewise, we want to fetch from the right app to make sure + // we get the right data + const enterpriseFetchSegments = () => + enterpriseApp.request + .get(SEGMENTS_BASE_PATH) + .expect(200) + .then((res) => res.body.segments); + + const enterpriseFetchSegmentStrategies = ( + segmentId: number, + ): Promise => + enterpriseApp.request + .get(`${SEGMENTS_BASE_PATH}/${segmentId}/strategies`) + .expect(200) + .then((res) => res.body); + beforeAll(async () => { + enterpriseApp = await setupAppWithCustomConfig( + db.stores, + { + enterpriseVersion: '5.3.0', + ui: { environment: 'Enterprise' }, + isEnterprise: true, + experimental: { + flags: { + detectSegmentUsageInChangeRequests: true, + }, + }, + }, + db.rawDatabase, + ); + user = await db.stores.userStore.insert({ username: 'test', }); @@ -463,8 +499,8 @@ describe('detect strategy usage in change requests', () => { 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 createFeatureToggle(enterpriseApp, toggle); + const [segment] = await enterpriseFetchSegments(); await db.rawDatabase.table('change_request_events').insert({ feature: toggle.name, @@ -487,20 +523,25 @@ describe('detect strategy usage in change requests', () => { created_by: user.id, }); - expect((await fetchSegments()).length).toEqual(1); + expect((await enterpriseFetchSegments()).length).toEqual(1); - await app.request + await enterpriseApp.request .delete(`${SEGMENTS_BASE_PATH}/${segment.id}`) .expect(409); - expect((await fetchSegments()).length).toEqual(1); + expect((await enterpriseFetchSegments()).length).toEqual(1); + + // check that it can be deleted in OSS + await app.request + .delete(`${SEGMENTS_BASE_PATH}/${segment.id}`) + .expect(204); }); 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 createFeatureToggle(enterpriseApp, toggle); + const [segment] = await enterpriseFetchSegments(); await db.rawDatabase.table('change_request_events').insert({ feature: toggle.name, @@ -524,7 +565,7 @@ describe('detect strategy usage in change requests', () => { }); const { strategies, changeRequestStrategies } = - await fetchSegmentStrategies(segment.id); + await enterpriseFetchSegmentStrategies(segment.id); expect(changeRequestStrategies).toMatchObject([ { @@ -536,16 +577,21 @@ describe('detect strategy usage in change requests', () => { }, ]); expect(strategies).toStrictEqual([]); + + // check that OSS gets no CR strategies + const ossResult = await fetchSegmentStrategies(segment.id); + expect(ossResult.strategies).toStrictEqual([]); + expect(ossResult.changeRequestStrategies ?? []).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 createFeatureToggle(enterpriseApp, toggle); + const [segment] = await enterpriseFetchSegments(); await addStrategyToFeatureEnv( - app, + enterpriseApp, { ...toggle.strategies[0] }, 'default', toggle.name, @@ -578,7 +624,7 @@ describe('detect strategy usage in change requests', () => { }); const { strategies, changeRequestStrategies } = - await fetchSegmentStrategies(segment.id); + await enterpriseFetchSegmentStrategies(segment.id); expect(changeRequestStrategies).toMatchObject([ { @@ -587,16 +633,21 @@ describe('detect strategy usage in change requests', () => { }, ]); expect(strategies).toStrictEqual([]); + + // check that OSS gets no CR strategies + const ossResult = await fetchSegmentStrategies(segment.id); + expect(ossResult.strategies).toStrictEqual([]); + expect(ossResult.changeRequestStrategies ?? []).toStrictEqual([]); }); test('If a segment is used in an existing strategy and in a CR for the same strategy, the strategy should be listed both places', async () => { await createSegment({ name: 'a', constraints: [] }); const toggle = mockFeatureToggle(); - await createFeatureToggle(app, toggle); - const [segment] = await fetchSegments(); + await createFeatureToggle(enterpriseApp, toggle); + const [segment] = await enterpriseFetchSegments(); await addStrategyToFeatureEnv( - app, + enterpriseApp, { ...toggle.strategies[0] }, 'default', toggle.name, @@ -630,39 +681,19 @@ describe('detect strategy usage in change requests', () => { }); const { strategies, changeRequestStrategies } = - await fetchSegmentStrategies(segment.id); + await enterpriseFetchSegmentStrategies(segment.id); expect(strategies).toMatchObject([{ id: strategyId }]); expect(changeRequestStrategies).toMatchObject([{ id: strategyId }]); + + // check that OSS gets no CR strategies + const ossResult = await fetchSegmentStrategies(segment.id); + expect(ossResult.strategies).toMatchObject([{ id: strategyId }]); + expect(ossResult.changeRequestStrategies ?? []).toStrictEqual([]); }); test('Should show usage in features and projects in CRs', async () => { - // Change request data is only counted for enterprise - // instances, so we'll instantiate our own version of the app - // for that. - const enterpriseApp = await setupAppWithCustomConfig( - db.stores, - { - enterpriseVersion: '5.3.0', - ui: { environment: 'Enterprise' }, - experimental: { - flags: { - detectSegmentUsageInChangeRequests: true, - }, - }, - }, - db.rawDatabase, - ); - - // likewise, we want to fetch from the right app to make sure - // we get the right data - const enterpriseFetchSegments = () => - enterpriseApp.request - .get(SEGMENTS_BASE_PATH) - .expect(200) - .then((res) => res.body.segments); - // because they use the same db, we can use the regular app // (through `createSegment` and `createFeatureToggle`) to // create the segment and the flag @@ -698,5 +729,11 @@ describe('detect strategy usage in change requests', () => { expect(segments).toMatchObject([ { usedInFeatures: 1, usedInProjects: 1 }, ]); + + // check that OSS gets no CR usage + const ossSegments = await fetchSegments(); + expect(ossSegments).toMatchObject([ + { usedInFeatures: 0, usedInProjects: 0 }, + ]); }); });