From 01a38becb376bb4936fab1d0954da51366c156c1 Mon Sep 17 00:00:00 2001
From: Thomas Heartman <thomas@getunleash.io>
Date: Fri, 19 Jan 2024 18:56:46 +0400
Subject: [PATCH] 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.
---
 .../strategy-change-diff-calculation.test.ts  | 56 +++++++++++++++++++
 .../strategy-change-diff-calculation.ts       | 10 ++++
 .../changeRequest/changeRequest.types.ts      |  2 +-
 3 files changed, 67 insertions(+), 1 deletion(-)

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 a39c6fd5c0..c6f448d0e4 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,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();
+    });
 });
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 225566be1a..1cfa553d55 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
@@ -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,
diff --git a/frontend/src/component/changeRequest/changeRequest.types.ts b/frontend/src/component/changeRequest/changeRequest.types.ts
index e003158a57..79472f965c 100644
--- a/frontend/src/component/changeRequest/changeRequest.types.ts
+++ b/frontend/src/component/changeRequest/changeRequest.types.ts
@@ -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 = {