feat: add algorithm to detect what strategy changes would be overwritten by applying a CR (#5963)

This change adds an algorithm with tests for detecting what changes
would be overwritten by applying a CR.

Test cases:
- It compares strategies regardless of order of keys in the objects.
This ensures that two strategies with the same content but different
order of keys are compared correctly.
- It treats `undefined` or missing segments in old config as equal to
`[]` in change
- It treats `undefined` or missing strategy variants in old config and
change as equal to `[]`
- It lists changes in a sorted list with the correct values
- It ignores object order on nested objects. Similar to the first
point, this does order-insensitive comparison for nested objects (such
as params and constraints).

frontend/package.json

@@ -2,7 +2,10 @@
   "name": "unleash-frontend-local",
   "version": "0.0.0",
   "private": true,
-  "files": ["index.js", "build"],
+  "files": [
+    "index.js",
+    "build"
+  ],
   "engines": {
     "node": ">=18"
   },
@@ -50,6 +53,7 @@
     "@types/deep-diff": "1.0.5",
     "@types/jest": "29.5.11",
     "@types/lodash.clonedeep": "4.5.9",
+    "@types/lodash.isequal": "^4.5.8",
     "@types/lodash.mapvalues": "^4.6.9",
     "@types/lodash.omit": "4.5.9",
     "@types/node": "18.19.6",
@@ -83,6 +87,7 @@
     "immer": "9.0.21",
     "jsdom": "23.1.0",
     "lodash.clonedeep": "4.5.0",
+    "lodash.isequal": "^4.5.0",
     "lodash.mapvalues": "^4.6.0",
     "lodash.omit": "4.5.0",
     "mermaid": "^9.3.0",
@@ -138,7 +143,11 @@
     }
   },
   "browserslist": {
-    "production": [">0.2%", "not dead", "not op_mini all"],
+    "production": [
+      ">0.2%",
+      "not dead",
+      "not op_mini all"
+    ],
     "development": [
       "last 1 chrome version",
       "last 1 firefox version",

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

@@ -0,0 +1,290 @@
+import { IChangeRequestUpdateStrategy } from 'component/changeRequest/changeRequest.types';
+import { IFeatureStrategy } from 'interfaces/strategy';
+import { getChangesThatWouldBeOverwritten } from './strategy-change-diff-calculation';
+
+describe('Strategy change conflict detection', () => {
+    const existingStrategy: IFeatureStrategy = {
+        name: 'flexibleRollout',
+        constraints: [],
+        variants: [],
+        parameters: {
+            groupId: 'aaa',
+            rollout: '71',
+            stickiness: 'default',
+        },
+        sortOrder: 0,
+        id: '31572930-2db7-461f-813b-3eedc200cb33',
+        title: '',
+        disabled: false,
+        segments: [],
+    };
+
+    const snapshot: IFeatureStrategy = {
+        id: '31572930-2db7-461f-813b-3eedc200cb33',
+        name: 'flexibleRollout',
+        title: '',
+        disabled: false,
+        segments: [],
+        variants: [],
+        sortOrder: 0,
+        parameters: {
+            groupId: 'aaa',
+            rollout: '71',
+            stickiness: 'default',
+        },
+        constraints: [],
+    };
+
+    const change: IChangeRequestUpdateStrategy = {
+        id: 39,
+        action: 'updateStrategy' as const,
+        payload: {
+            id: '31572930-2db7-461f-813b-3eedc200cb33',
+            name: 'flexibleRollout',
+            title: '',
+            disabled: false,
+            segments: [],
+            snapshot,
+            variants: [],
+            parameters: {
+                groupId: 'aaa',
+                rollout: '38',
+                stickiness: 'default',
+            },
+            constraints: [],
+        },
+        createdAt: new Date('2024-01-18T07:58:36.314Z'),
+        createdBy: {
+            id: 1,
+            username: 'admin',
+            imageUrl:
+                'https://gravatar.com/avatar/8c6976e5b5410415bde908bd4dee15dfb167a9c873fc4bb8a81f6f2ab448a918?s=42&d=retro&r=g',
+        },
+    };
+
+    test('It compares strategies regardless of order of keys in the objects', () => {
+        const result = getChangesThatWouldBeOverwritten(
+            existingStrategy,
+            change,
+        );
+
+        expect(result).toBeNull();
+    });
+
+    test('It treats `undefined` or missing segments in old config as equal to `[]` in change', () => {
+        const resultUndefined = getChangesThatWouldBeOverwritten(
+            {
+                ...existingStrategy,
+                segments: undefined,
+            },
+            change,
+        );
+
+        expect(resultUndefined).toBeNull();
+
+        const { segments, ...withoutSegments } = existingStrategy;
+        const resultMissing = getChangesThatWouldBeOverwritten(
+            withoutSegments,
+            change,
+        );
+
+        expect(resultMissing).toBeNull();
+    });
+
+    test('It treats `undefined` or missing strategy variants in old config and change as equal to `[]`', () => {
+        const undefinedVariantsExistingStrategy = {
+            ...existingStrategy,
+            variants: undefined,
+        };
+        const { variants: _variants, ...missingVariantsExistingStrategy } =
+            existingStrategy;
+
+        const { variants: _snapshotVariants, ...snapshot } =
+            change.payload.snapshot!;
+
+        const undefinedVariantsInSnapshot = {
+            ...change,
+            payload: {
+                ...change.payload,
+                snapshot: {
+                    ...snapshot,
+                    variants: undefined,
+                },
+            },
+        };
+
+        const missingVariantsInSnapshot = {
+            ...change,
+            payload: {
+                ...change.payload,
+                snapshot: snapshot,
+            },
+        };
+
+        // for all combinations, check that there are no changes
+        const cases = [
+            undefinedVariantsExistingStrategy,
+            missingVariantsExistingStrategy,
+        ].flatMap((existing) =>
+            [undefinedVariantsInSnapshot, missingVariantsInSnapshot].map(
+                (changeValue) =>
+                    getChangesThatWouldBeOverwritten(existing, changeValue),
+            ),
+        );
+
+        expect(cases.every((result) => result === null)).toBeTruthy();
+    });
+
+    test('It lists changes in a sorted list with the correct values', () => {
+        const withChanges: IFeatureStrategy = {
+            name: 'flexibleRollout',
+            title: 'custom title',
+            constraints: [
+                {
+                    values: ['blah'],
+                    inverted: false,
+                    operator: 'IN' as const,
+                    contextName: 'appName',
+                    caseInsensitive: false,
+                },
+            ],
+            variants: [
+                {
+                    name: 'variant1',
+                    weight: 1000,
+                    payload: {
+                        type: 'string',
+                        value: 'beaty',
+                    },
+                    stickiness: 'userId',
+                    weightType: 'variable' as const,
+                },
+            ],
+            parameters: {
+                groupId: 'aab',
+                rollout: '39',
+                stickiness: 'userId',
+            },
+            sortOrder: 1,
+            id: '31572930-2db7-461f-813b-3eedc200cb33',
+            disabled: true,
+            segments: [3],
+        };
+
+        const result = getChangesThatWouldBeOverwritten(withChanges, change);
+
+        const { id, name, ...changedProperties } = withChanges;
+
+        const expectedOutput = Object.entries(changedProperties).map(
+            ([property, oldValue]) => ({
+                property,
+                oldValue,
+                newValue:
+                    change.payload.snapshot![
+                        property as keyof IFeatureStrategy
+                    ],
+            }),
+        );
+
+        expectedOutput.sort((a, b) => a.property.localeCompare(b.property));
+        expect(result).toStrictEqual(expectedOutput);
+    });
+
+    test('it ignores object order on nested objects', () => {
+        const existingStrategyMod: IFeatureStrategy = {
+            ...existingStrategy,
+            constraints: [
+                {
+                    values: ['blah'],
+                    inverted: false,
+                    operator: 'IN' as const,
+                    contextName: 'appName',
+                    caseInsensitive: false,
+                },
+            ],
+        };
+
+        const constraintChange: IChangeRequestUpdateStrategy = {
+            ...change,
+            payload: {
+                ...change.payload,
+                // @ts-expect-error Some of the properties that may be
+                // undefined, we know exist in the value
+                snapshot: {
+                    ...change.payload.snapshot,
+                    constraints: [
+                        {
+                            caseInsensitive: false,
+                            contextName: 'appName',
+                            inverted: false,
+                            operator: 'IN' as const,
+                            values: ['blah'],
+                        },
+                    ],
+                },
+            },
+        };
+
+        expect(
+            getChangesThatWouldBeOverwritten(
+                existingStrategyMod,
+                constraintChange,
+            ),
+        ).toBeNull();
+    });
+
+    test('Any properties in the existing strategy that do not exist in the snapshot are also detected', () => {
+        const { variants: _snapshotVariants, ...snapshot } =
+            change.payload.snapshot!;
+
+        const existingStrategyWithVariants = {
+            ...existingStrategy,
+            variants: [
+                {
+                    name: 'variant1',
+                    weight: 1000,
+                    payload: {
+                        type: 'string',
+                        value: 'beaty',
+                    },
+                    stickiness: 'userId',
+                    weightType: 'variable' as const,
+                },
+            ],
+        };
+
+        const result = getChangesThatWouldBeOverwritten(
+            existingStrategyWithVariants,
+            {
+                ...change,
+                payload: {
+                    ...change.payload,
+                    snapshot,
+                },
+            },
+        );
+
+        expect(result).toStrictEqual([
+            {
+                property: 'variants',
+                oldValue: existingStrategyWithVariants.variants,
+                newValue: undefined,
+            },
+        ]);
+    });
+
+    test('it returns null if the existing strategy is undefined', () => {
+        const result = getChangesThatWouldBeOverwritten(undefined, change);
+
+        expect(result).toBeNull();
+    });
+    test('it returns null if the snapshot is missing', () => {
+        const { snapshot, ...payload } = change.payload;
+        const result = getChangesThatWouldBeOverwritten(existingStrategy, {
+            ...change,
+            payload,
+        });
+
+        expect(result).toBeNull();
+    });
+});

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

@@ -0,0 +1,79 @@
+import { IChangeRequestUpdateStrategy } from 'component/changeRequest/changeRequest.types';
+import { IFeatureStrategy } from 'interfaces/strategy';
+import isEqual from 'lodash.isequal';
+
+const hasJsonDiff = (object: unknown, objectToCompare: unknown) =>
+    JSON.stringify(object) !== JSON.stringify(objectToCompare);
+
+type DataToOverwrite<Prop extends keyof IFeatureStrategy> = {
+    property: Prop;
+    oldValue: IFeatureStrategy[Prop];
+    newValue: IFeatureStrategy[Prop];
+};
+type ChangesThatWouldBeOverwritten = DataToOverwrite<keyof IFeatureStrategy>[];
+
+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);
+        };
+
+        // 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,
+        )
+            .map(([key, currentValue]: [string, unknown]) => {
+                const snapshotValue = snapshot[key as keyof IFeatureStrategy];
+
+                // 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 ?? [])) {
+                        return {
+                            property: key as keyof IFeatureStrategy,
+                            oldValue: currentValue,
+                            newValue: snapshotValue,
+                        };
+                    }
+                } else if (key === 'variants') {
+                    // strategy variants might not be defined, so use
+                    // fallback values
+                    if (hasJsonDiff(snapshotValue ?? [], currentValue ?? [])) {
+                        return {
+                            property: key as keyof IFeatureStrategy,
+                            oldValue: currentValue,
+                            newValue: snapshotValue,
+                        };
+                    }
+                } else if (hasChanged(snapshotValue, currentValue)) {
+                    return {
+                        property: key as keyof IFeatureStrategy,
+                        oldValue: currentValue,
+                        newValue: snapshotValue,
+                    };
+                }
+            })
+            .filter(
+                (change): change is DataToOverwrite<keyof IFeatureStrategy> =>
+                    Boolean(change),
+            );
+
+        if (changes.length) {
+            // we have changes that would be overwritten
+            changes.sort((a, b) => a.property.localeCompare(b.property));
+            return changes;
+        }
+    }
+
+    return null;
+};

frontend/src/component/changeRequest/changeRequest.types.ts

@@ -228,6 +228,7 @@ export type ChangeRequestAddStrategy = Pick<
 
 export type ChangeRequestEditStrategy = ChangeRequestAddStrategy & {
     id: string;
+    snapshot?: IFeatureStrategy;
 };
 
 type ChangeRequestDeleteStrategy = {

frontend/yarn.lock

@@ -2127,6 +2127,13 @@
   dependencies:
     "@types/lodash" "*"
 
+"@types/lodash.isequal@^4.5.8":
+  version "4.5.8"
+  resolved "https://registry.yarnpkg.com/@types/lodash.isequal/-/lodash.isequal-4.5.8.tgz#b30bb6ff6a5f6c19b3daf389d649ac7f7a250499"
+  integrity sha512-uput6pg4E/tj2LGxCZo9+y27JNyB2OZuuI/T5F+ylVDYuqICLG2/ktjxx0v6GvVntAf8TvEzeQLcV0ffRirXuA==
+  dependencies:
+    "@types/lodash" "*"
+
 "@types/lodash.mapvalues@^4.6.9":
   version "4.6.9"
   resolved "https://registry.yarnpkg.com/@types/lodash.mapvalues/-/lodash.mapvalues-4.6.9.tgz#1edb4b1d299db332166b474221b06058b34030a7"
@@ -5289,6 +5296,11 @@ lodash.isempty@^4.4.0:
   resolved "https://registry.yarnpkg.com/lodash.isempty/-/lodash.isempty-4.4.0.tgz#6f86cbedd8be4ec987be9aaf33c9684db1b31e7e"
   integrity sha512-oKMuF3xEeqDltrGMfDxAPGIVMSSRv8tbRSODbrs4KGsRRLEhrW8N8Rd4DRgB2+621hY8A8XwwrTVhXWpxFvMzg==
 
+lodash.isequal@^4.5.0:
+  version "4.5.0"
+  resolved "https://registry.yarnpkg.com/lodash.isequal/-/lodash.isequal-4.5.0.tgz#415c4478f2bcc30120c22ce10ed3226f7d3e18e0"
+  integrity sha512-pDo3lu8Jhfjqls6GkMgpahsF9kCyayhgykjyLMNFTKWrpVdAQtYyB4muAMWozBB4ig/dtWAmsMxLEI8wuz+DYQ==
+
 lodash.mapvalues@^4.6.0:
   version "4.6.0"
   resolved "https://registry.yarnpkg.com/lodash.mapvalues/-/lodash.mapvalues-4.6.0.tgz#1bafa5005de9dd6f4f26668c30ca37230cc9689c"