From 01318b11ea6362ba67c985142523f1a784d7a11d Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Wed, 24 Jan 2024 12:39:41 +0400 Subject: [PATCH] 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 --- .../strategy-change-diff-calculation.test.ts | 75 ++++++++++++- .../strategy-change-diff-calculation.ts | 104 +++++++++++++----- 2 files changed, 144 insertions(+), 35 deletions(-) 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), );