Skip to content

Commit

Permalink
fix: handle title diffing correctly in strategy change diffs (#5971)
Browse files Browse the repository at this point in the history
A strategy title can be either an empty string or undefined on the
type we use in the frontend. In the snapshot it can be an empty
string, null (presumably), and undefined.

This change updates the diffing logic to handle the various title diff
cases correctly. It also updates the type used for the snapshot to
reflect this.
  • Loading branch information
thomasheartman committed Jan 19, 2024
1 parent 5253482 commit 01a38be
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 1 deletion.
@@ -1,5 +1,6 @@
import { IChangeRequestUpdateStrategy } from 'component/changeRequest/changeRequest.types';
import { IFeatureStrategy } from 'interfaces/strategy';
import omit from 'lodash.omit';
import { getChangesThatWouldBeOverwritten } from './strategy-change-diff-calculation';

describe('Strategy change conflict detection', () => {
Expand Down Expand Up @@ -287,4 +288,59 @@ describe('Strategy change conflict detection', () => {

expect(result).toBeNull();
});

test('It treats `null` and `""` for `title` in the change as equal to `""` in the existing strategy', () => {
const emptyTitleExistingStrategy = {
...existingStrategy,
title: '',
};
const undefinedTitleExistingStrategy = omit(existingStrategy, 'title');

const { title: _snapshotTitle, ...snapshot } = change.payload.snapshot!;

const nullTitleInSnapshot = {
...change,
payload: {
...change.payload,
snapshot: {
...snapshot,
title: null,
},
},
};

const emptyTitleInSnapshot = {
...change,
payload: {
...change.payload,
snapshot: {
...snapshot,
title: '',
},
},
};

const missingTitleInSnapshot = {
...change,
payload: {
...change.payload,
snapshot,
},
};

const cases = [
undefinedTitleExistingStrategy,
emptyTitleExistingStrategy,
].flatMap((existing) =>
[
nullTitleInSnapshot,
emptyTitleInSnapshot,
missingTitleInSnapshot,
].map((changeValue) =>
getChangesThatWouldBeOverwritten(existing, changeValue),
),
);

expect(cases.every((result) => result === null)).toBeTruthy();
});
});
Expand Up @@ -55,6 +55,16 @@ export const getChangesThatWouldBeOverwritten = (
newValue: snapshotValue,
};
}
} else if (key === 'title') {
// the title can be defined as `null` or
// `undefined`, so we fallback to an empty string
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,
Expand Down
Expand Up @@ -228,7 +228,7 @@ export type ChangeRequestAddStrategy = Pick<

export type ChangeRequestEditStrategy = ChangeRequestAddStrategy & {
id: string;
snapshot?: IFeatureStrategy;
snapshot?: Omit<IFeatureStrategy, 'title'> & { title?: string | null };
};

type ChangeRequestDeleteStrategy = {
Expand Down

0 comments on commit 01a38be

Please sign in to comment.