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 index c6f448d0e4..8467acd54c 100644 --- 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 @@ -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![ - property as keyof IFeatureStrategy - ], + change.payload[property as keyof ChangeRequestEditStrategy], }), ); @@ -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(); + }); }); 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 index 1cfa553d55..e0312627d9 100644 --- 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 @@ -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) => - JSON.stringify(object) !== JSON.stringify(objectToCompare); - -type DataToOverwrite = { - property: Prop; - oldValue: IFeatureStrategy[Prop]; - newValue: IFeatureStrategy[Prop]; +type JsonDiffProps = { + snapshotValue: unknown; + currentValue: unknown; + changeValue: unknown; + fallback?: unknown; }; -type ChangesThatWouldBeOverwritten = DataToOverwrite[]; +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 = { + 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 => + ( + change, + ): change is DataToOverwrite => Boolean(change), );