From b211345a448942c1d2f57ca219fbd226a9e14f05 Mon Sep 17 00:00:00 2001
From: Thomas Heartman <thomas@getunleash.io>
Date: Wed, 22 Nov 2023 14:13:18 +0100
Subject: [PATCH] fix: if a strategy both uses a segment actively and in CRs,
 list it twice (#5390)

This PR changes the behavior of the API a little bit. Instead of
removing any strategies from `changeRequestStrategies` that are also
in `strategies`, we keep them in instead.

The reason for this is that the overview of where a segment is used is
incomplete if it shows only strategies but not CRs. Imagine this:

You want to delete a segment, but you're told it's only used in strategy
S.
So you go and remove it from strategy S, but then you're told it's
suddenly used in CRs A, B, and C. This is now a two-step operation
with a bad surprise. Instead, we could show you immediately that this
segment is used in strategy S and CRs A, B, and C.
---
 src/lib/services/segment-service.ts        | 9 ++-------
 src/test/e2e/api/admin/segment.e2e.test.ts | 4 ++--
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/src/lib/services/segment-service.ts b/src/lib/services/segment-service.ts
index 8cc41a3dcd..753381b5f9 100644
--- a/src/lib/services/segment-service.ts
+++ b/src/lib/services/segment-service.ts
@@ -119,15 +119,10 @@ export class SegmentService implements ISegmentService {
         const strategies =
             await this.featureStrategiesStore.getStrategiesBySegment(id);
 
-        const strategyIds = new Set(strategies.map((s) => s.id));
-
-        const changeRequestStrategies = (
+        const changeRequestStrategies =
             await this.changeRequestSegmentUsageReadModel.getStrategiesUsedInActiveChangeRequests(
                 id,
-            )
-        ).filter(
-            (strategy) => !('id' in strategy && strategyIds.has(strategy.id)),
-        );
+            );
 
         return { strategies, changeRequestStrategies };
     }
diff --git a/src/test/e2e/api/admin/segment.e2e.test.ts b/src/test/e2e/api/admin/segment.e2e.test.ts
index 1824561f6c..cad9c0670d 100644
--- a/src/test/e2e/api/admin/segment.e2e.test.ts
+++ b/src/test/e2e/api/admin/segment.e2e.test.ts
@@ -583,7 +583,7 @@ describe('detect strategy usage in change requests', () => {
         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 () => {
+    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);
@@ -628,6 +628,6 @@ describe('detect strategy usage in change requests', () => {
 
         expect(strategies).toMatchObject([{ id: strategyId }]);
 
-        expect(changeRequestStrategies).toStrictEqual([]);
+        expect(changeRequestStrategies).toMatchObject([{ id: strategyId }]);
     });
 });