From 0e7f1aab508698beec0c1c9f53e7af2dc391c586 Mon Sep 17 00:00:00 2001
From: Thomas Heartman <thomas@getunleash.io>
Date: Wed, 29 May 2024 12:46:51 +0200
Subject: [PATCH] fix: sort segments before comparing in cr diff calculations
 (#7202)

This change fixes a bug where we would show the list of segments as
changed (causing a conflict) if their order wasn't the same in the
change as in the original.

By sorting the segments before comparing them, we can avoid this issue.

To avoid modifying the objects that are passed in (in case that has
knock-on effects anywhere), we copy the objects. And because `toSorted`
"only" has about 89% coverage now, I chose to use `sort` and spread the
arrays instead.
---
 .../strategy-change-diff-calculation.test.ts  | 18 +++++++++++++
 .../strategy-change-diff-calculation.ts       | 26 +++++++++++++++++--
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/strategy-change-diff-calculation.test.ts b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/strategy-change-diff-calculation.test.ts
index 9cd1b26709..b15400e129 100644
--- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/strategy-change-diff-calculation.test.ts
+++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/strategy-change-diff-calculation.test.ts
@@ -99,6 +99,24 @@ describe('Strategy change conflict detection', () => {
         expect(resultMissing).toBeNull();
     });
 
+    test('It sorts segments before comparing (because their order is irrelevant)', () => {
+        const result = getStrategyChangesThatWouldBeOverwritten(
+            {
+                ...existingStrategy,
+                segments: [25, 26, 1],
+            },
+            {
+                ...change,
+                payload: {
+                    ...change.payload,
+                    segments: [26, 1, 25],
+                },
+            },
+        );
+
+        expect(result).toBeNull();
+    });
+
     test('It treats `undefined` or missing strategy variants in old config and change as equal to `[]`', () => {
         const undefinedVariantsExistingStrategy = {
             ...existingStrategy,
diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/strategy-change-diff-calculation.ts b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/strategy-change-diff-calculation.ts
index 1e8c2d9b27..3e5472928b 100644
--- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/strategy-change-diff-calculation.ts
+++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/strategy-change-diff-calculation.ts
@@ -142,9 +142,31 @@ export function getStrategyChangesThatWouldBeOverwritten(
     change: IChangeRequestUpdateStrategy,
 ): ChangesThatWouldBeOverwritten | null {
     const fallbacks = { segments: [], variants: [], title: '' };
+
+    const withSortedSegments = (() => {
+        if (!(change.payload.segments && currentStrategyConfig?.segments)) {
+            return { current: currentStrategyConfig, change: change };
+        }
+
+        const changeCopy = {
+            ...change,
+            payload: {
+                ...change.payload,
+                segments: [...change.payload.segments].sort(),
+            },
+        };
+
+        const currentCopy = {
+            ...currentStrategyConfig,
+            segments: [...currentStrategyConfig.segments].sort(),
+        };
+
+        return { current: currentCopy, change: changeCopy };
+    })();
+
     return getChangesThatWouldBeOverwritten(
-        omit(currentStrategyConfig, 'strategyName'),
-        change,
+        omit(withSortedSegments.current, 'strategyName'),
+        withSortedSegments.change,
         fallbacks,
     );
 }