fix: show the updated value instead of the snapshot value (#5989)

This PR fixes a bug in the displayed value of the conflict list so that
it shows the value it would update to instead of the snapshot value.

In doing so, it updates the logic of the algorithm to:

1. if the snapshot value and the current value are the same, it's not a
conflict (it's an intended change)
2. If the snapshot value differs from the current value, it is a
conflict if and only if the value in the change differs from the current
value. Otherwise, it's not a conflict.

The new test cases are:
- it shows a diff for a property if the snapshot and live version differ
for that property and the changed value is different from the live
version
- it does not show a diff for a property if the live version and the
change have the same value, even if the snapshot differs from the live
version
- it does not show a diff for a property if the snapshot and the live
version are the same

frontend/src/component/changeRequest/ChangeRequest/Changes/Change/strategy-change-diff-calculation.test.ts

@@ -1,4 +1,7 @@
-import { IChangeRequestUpdateStrategy } from 'component/changeRequest/changeRequest.types';
+import {
+    ChangeRequestEditStrategy,
+    IChangeRequestUpdateStrategy,
+} from 'component/changeRequest/changeRequest.types';
 import { IFeatureStrategy } from 'interfaces/strategy';
 import omit from 'lodash.omit';
 import { getChangesThatWouldBeOverwritten } from './strategy-change-diff-calculation';
@@ -181,9 +184,7 @@ describe('Strategy change conflict detection', () => {
                 property,
                 oldValue,
                 newValue:
-                    change.payload.snapshot![
+                    change.payload[property as keyof ChangeRequestEditStrategy],
-                        property as keyof IFeatureStrategy
-                    ],
             }),
         );
 
@@ -269,7 +270,7 @@ describe('Strategy change conflict detection', () => {
             {
                 property: 'variants',
                 oldValue: existingStrategyWithVariants.variants,
-                newValue: undefined,
+                newValue: [],
             },
         ]);
     });
@@ -343,4 +344,68 @@ describe('Strategy change conflict detection', () => {
 
         expect(cases.every((result) => result === null)).toBeTruthy();
     });
+
+    test('it shows a diff for a property if the snapshot and live version differ for that property and the changed value is different from the live version', () => {
+        const liveVersion = {
+            ...existingStrategy,
+            title: 'new-title',
+        };
+
+        const changedVersion = {
+            ...change,
+            payload: {
+                ...change.payload,
+                title: 'other-new-title',
+            },
+        };
+        const result = getChangesThatWouldBeOverwritten(
+            liveVersion,
+            changedVersion,
+        );
+
+        expect(result).toStrictEqual([
+            {
+                property: 'title',
+                oldValue: liveVersion.title,
+                newValue: changedVersion.payload.title,
+            },
+        ]);
+    });
+
+    test('it does not show a diff for a property if the live version and the change have the same value, even if the snapshot differs from the live version', () => {
+        const liveVersion = {
+            ...existingStrategy,
+            title: 'new-title',
+        };
+
+        const changedVersion = {
+            ...change,
+            payload: {
+                ...change.payload,
+                title: liveVersion.title,
+            },
+        };
+        const result = getChangesThatWouldBeOverwritten(
+            liveVersion,
+            changedVersion,
+        );
+
+        expect(result).toBeNull();
+    });
+
+    test('it does not show a diff for a property if the snapshot and the live version are the same', () => {
+        const changedVersion = {
+            ...change,
+            payload: {
+                ...change.payload,
+                title: 'new-title',
+            },
+        };
+        const result = getChangesThatWouldBeOverwritten(
+            existingStrategy,
+            changedVersion,
+        );
+
+        expect(result).toBeNull();
+    });
 });

frontend/src/component/changeRequest/ChangeRequest/Changes/Change/strategy-change-diff-calculation.ts

@@ -1,80 +1,124 @@
-import { IChangeRequestUpdateStrategy } from 'component/changeRequest/changeRequest.types';
+import {
+    ChangeRequestEditStrategy,
+    IChangeRequestUpdateStrategy,
+} from 'component/changeRequest/changeRequest.types';
 import { IFeatureStrategy } from 'interfaces/strategy';
 import isEqual from 'lodash.isequal';
+import omit from 'lodash.omit';
 
-const hasJsonDiff = (object: unknown, objectToCompare: unknown) =>
+type JsonDiffProps = {
-    JSON.stringify(object) !== JSON.stringify(objectToCompare);
+    snapshotValue: unknown;
-
+    currentValue: unknown;
-type DataToOverwrite<Prop extends keyof IFeatureStrategy> = {
+    changeValue: unknown;
-    property: Prop;
+    fallback?: unknown;
-    oldValue: IFeatureStrategy[Prop];
-    newValue: IFeatureStrategy[Prop];
 };
-type ChangesThatWouldBeOverwritten = DataToOverwrite<keyof IFeatureStrategy>[];
+const hasJsonDiff = ({
+    snapshotValue,
+    currentValue,
+    changeValue,
+    fallback,
+}: JsonDiffProps) => {
+    const currentJson = JSON.stringify(currentValue ?? fallback);
+    return (
+        JSON.stringify(snapshotValue ?? fallback) !== currentJson &&
+        JSON.stringify(changeValue ?? fallback) !== currentJson
+    );
+};
+
+const hasChanged = (
+    snapshotValue: unknown,
+    currentValue: unknown,
+    changeValue: unknown,
+) => {
+    if (typeof snapshotValue === 'object') {
+        return (
+            !isEqual(snapshotValue, currentValue) &&
+            !isEqual(currentValue, changeValue)
+        );
+    }
+    return hasJsonDiff({ snapshotValue, currentValue, changeValue });
+};
+
+type DataToOverwrite<Prop extends keyof ChangeRequestEditStrategy> = {
+    property: Prop;
+    oldValue: ChangeRequestEditStrategy[Prop];
+    newValue: ChangeRequestEditStrategy[Prop];
+};
+type ChangesThatWouldBeOverwritten = DataToOverwrite<
+    keyof ChangeRequestEditStrategy
+>[];
 
 export const getChangesThatWouldBeOverwritten = (
     currentStrategyConfig: IFeatureStrategy | undefined,
     change: IChangeRequestUpdateStrategy,
 ): ChangesThatWouldBeOverwritten | null => {
     const { snapshot } = change.payload;
-    if (snapshot && currentStrategyConfig) {
-        const hasChanged = (a: unknown, b: unknown) => {
-            if (typeof a === 'object') {
-                return !isEqual(a, b);
-            }
-            return hasJsonDiff(a, b);
-        };
 
+    if (snapshot && currentStrategyConfig) {
         // compare each property in the snapshot. The property order
         // might differ, so using JSON.stringify to compare them
         // doesn't work.
         const changes: ChangesThatWouldBeOverwritten = Object.entries(
-            currentStrategyConfig,
+            omit(currentStrategyConfig, 'strategyName'),
         )
             .map(([key, currentValue]: [string, unknown]) => {
                 const snapshotValue = snapshot[key as keyof IFeatureStrategy];
+                const changeValue =
+                    change.payload[key as keyof ChangeRequestEditStrategy];
+
+                const hasJsonDiffWithFallback = (fallback: unknown) =>
+                    hasJsonDiff({
+                        snapshotValue,
+                        currentValue,
+                        changeValue,
+                        fallback,
+                    });
 
                 // compare, assuming that order never changes
                 if (key === 'segments') {
                     // segments can be undefined on the original
                     // object, but that doesn't mean it has changed
-                    if (hasJsonDiff(snapshotValue, currentValue ?? [])) {
+                    if (hasJsonDiffWithFallback([])) {
                         return {
-                            property: key as keyof IFeatureStrategy,
+                            property: key as keyof ChangeRequestEditStrategy,
                             oldValue: currentValue,
-                            newValue: snapshotValue,
+                            newValue: changeValue,
                         };
                     }
                 } else if (key === 'variants') {
                     // strategy variants might not be defined, so use
                     // fallback values
-                    if (hasJsonDiff(snapshotValue ?? [], currentValue ?? [])) {
+                    if (hasJsonDiffWithFallback([])) {
                         return {
-                            property: key as keyof IFeatureStrategy,
+                            property: key as keyof ChangeRequestEditStrategy,
                             oldValue: currentValue,
-                            newValue: snapshotValue,
+                            newValue: changeValue,
                         };
                     }
                 } else if (key === 'title') {
                     // the title can be defined as `null` or
                     // `undefined`, so we fallback to an empty string
-                    if (hasJsonDiff(snapshotValue ?? '', currentValue ?? '')) {
+                    if (hasJsonDiffWithFallback('')) {
                         return {
-                            property: key as keyof IFeatureStrategy,
+                            property: key as keyof ChangeRequestEditStrategy,
                             oldValue: currentValue,
-                            newValue: snapshotValue,
+                            newValue: changeValue,
                         };
                     }
-                } else if (hasChanged(snapshotValue, currentValue)) {
+                } else if (
+                    hasChanged(snapshotValue, currentValue, changeValue)
+                ) {
                     return {
-                        property: key as keyof IFeatureStrategy,
+                        property: key as keyof ChangeRequestEditStrategy,
                         oldValue: currentValue,
-                        newValue: snapshotValue,
+                        newValue: changeValue,
                     };
                 }
             })
             .filter(
-                (change): change is DataToOverwrite<keyof IFeatureStrategy> =>
+                (
+                    change,
+                ): change is DataToOverwrite<keyof ChangeRequestEditStrategy> =>
                     Boolean(change),
             );