1
0
mirror of https://github.com/Unleash/unleash.git synced 2024-12-22 19:07:54 +01:00

fix: handle title diffing correctly in strategy change diffs (#5971)

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.
This commit is contained in:
Thomas Heartman 2024-01-19 18:56:46 +04:00 committed by GitHub
parent 5253482f61
commit 01a38becb3
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 67 additions and 1 deletions

View File

@ -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', () => {
@ -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();
});
});

View File

@ -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,

View File

@ -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 = {