diff --git a/frontend/package.json b/frontend/package.json index 228373e190..f72a9ada45 100644 --- a/frontend/package.json +++ b/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", diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/strategy-change-diff-calculation.test.ts b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/strategy-change-diff-calculation.test.ts new file mode 100644 index 0000000000..a39c6fd5c0 --- /dev/null +++ b/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(); + }); +}); diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/strategy-change-diff-calculation.ts b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/strategy-change-diff-calculation.ts new file mode 100644 index 0000000000..225566be1a --- /dev/null +++ b/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 = { + property: Prop; + oldValue: IFeatureStrategy[Prop]; + newValue: IFeatureStrategy[Prop]; +}; +type ChangesThatWouldBeOverwritten = DataToOverwrite[]; + +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 => + 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; +}; diff --git a/frontend/src/component/changeRequest/changeRequest.types.ts b/frontend/src/component/changeRequest/changeRequest.types.ts index 75015cad06..e003158a57 100644 --- a/frontend/src/component/changeRequest/changeRequest.types.ts +++ b/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 = { diff --git a/frontend/yarn.lock b/frontend/yarn.lock index 6163e0305f..ab90f1ddea 100644 --- a/frontend/yarn.lock +++ b/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"