feat: show segment conflicts in crs (#6138)

This PR updates the diff calculation to work with both strategy changes
and segment changes. It also adds the corresponding segment change
conflict overview to segment updates.

<img width="1225" alt="image"
src="https://github.com/Unleash/unleash/assets/17786332/688a57a5-5cd7-4b0a-bd1e-df63189594d8">

frontend/src/component/changeRequest/ChangeRequest/Changes/Change/SegmentChangeDetails.tsx

@@ -7,6 +7,7 @@ import {
 import { useSegment } from 'hooks/api/getters/useSegment/useSegment';
 import { SegmentDiff, SegmentTooltipLink } from '../../SegmentTooltipLink';
 import { ConstraintAccordionList } from 'component/common/ConstraintAccordion/ConstraintAccordionList/ConstraintAccordionList';
+import { SegmentChangesToOverwrite } from './StrategyChangeOverwriteWarning';
 
 const ChangeItemCreateEditWrapper = styled(Box)(({ theme }) => ({
     display: 'grid',
@@ -77,6 +78,10 @@ export const SegmentChangeDetails: VFC<{
             )}
             {change.action === 'updateSegment' && (
                 <>
+                    <SegmentChangesToOverwrite
+                        currentSegment={currentSegment}
+                        change={change}
+                    />
                     <ChangeItemCreateEditWrapper>
                         <ChangeItemInfo>
                             <Typography>Editing segment:</Typography>

frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx

@@ -17,7 +17,7 @@ import { Badge } from 'component/common/Badge/Badge';
 import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender';
 import { flexRow } from 'themes/themeStyles';
 import { EnvironmentVariantsTable } from 'component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/EnvironmentVariantsCard/EnvironmentVariantsTable/EnvironmentVariantsTable';
-import { ChangesToOverwrite } from './StrategyChangeOverwriteWarning';
+import { StrategyChangesToOverwrite } from './StrategyChangeOverwriteWarning';
 
 export const ChangeItemWrapper = styled(Box)({
     display: 'flex',
@@ -210,7 +210,7 @@ export const StrategyChange: VFC<{
             )}
             {change.action === 'updateStrategy' && (
                 <>
-                    <ChangesToOverwrite
+                    <StrategyChangesToOverwrite
                         currentStrategy={currentStrategy}
                         change={change}
                     />

frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChangeOverwriteWarning.tsx

@@ -1,12 +1,20 @@
 import { Box, styled } from '@mui/material';
-import { IChangeRequestUpdateStrategy } from 'component/changeRequest/changeRequest.types';
 import { useChangeRequestPlausibleContext } from 'component/changeRequest/ChangeRequestContext';
+import {
+    IChangeRequestUpdateSegment,
+    IChangeRequestUpdateStrategy,
+} from 'component/changeRequest/changeRequest.types';
 import { useUiFlag } from 'hooks/useUiFlag';
+import { ISegment } from 'interfaces/segment';
 import { IFeatureStrategy } from 'interfaces/strategy';
-import { getChangesThatWouldBeOverwritten } from './strategy-change-diff-calculation';
+import {
+    ChangesThatWouldBeOverwritten,
+    getStrategyChangesThatWouldBeOverwritten,
+    getSegmentChangesThatWouldBeOverwritten,
+} from './strategy-change-diff-calculation';
 import { useEffect } from 'react';
 
-const ChangesToOverwriteWarning = styled(Box)(({ theme }) => ({
+const ChangesToOverwriteContainer = styled(Box)(({ theme }) => ({
     color: theme.palette.warning.dark,
     backgroundColor: theme.palette.warning.light,
     fontSize: theme.fontSizes.smallBody,
@@ -70,13 +78,113 @@ const OverwriteTable = styled('table')(({ theme }) => ({
     },
 }));
 
-export const ChangesToOverwrite: React.FC<{
+const DetailsTable: React.FC<{
+    changesThatWouldBeOverwritten: ChangesThatWouldBeOverwritten;
+}> = ({ changesThatWouldBeOverwritten }) => {
+    return (
+        <OverwriteTable>
+            <thead>
+                <tr>
+                    <th>Property</th>
+                    <th>Current value</th>
+                    <th>Value after change</th>
+                </tr>
+            </thead>
+
+            <tbody>
+                {changesThatWouldBeOverwritten.map(
+                    ({ property, oldValue, newValue }) => (
+                        <tr key={property}>
+                            <td data-column='Property'>{property}</td>
+                            <td data-column='Current value'>
+                                <pre>
+                                    <del>
+                                        {JSON.stringify(oldValue, null, 2)
+                                            .split('\n')
+                                            .map((line, index) => (
+                                                <code
+                                                    key={`${property}${line}${index}`}
+                                                >
+                                                    {`${line}\n`}
+                                                </code>
+                                            ))}
+                                    </del>
+                                </pre>
+                            </td>
+                            <td data-column='Value after change'>
+                                <pre>
+                                    <ins>
+                                        {JSON.stringify(newValue, null, 2)
+                                            .split('\n')
+                                            .map((line, index) => (
+                                                <code
+                                                    key={`${property}${line}${index}`}
+                                                >
+                                                    {`${line}\n`}
+                                                </code>
+                                            ))}
+                                    </ins>
+                                </pre>
+                            </td>
+                        </tr>
+                    ),
+                )}
+            </tbody>
+        </OverwriteTable>
+    );
+};
+
+const OverwriteWarning: React.FC<{
+    changeType: 'segment' | 'strategy';
+    changesThatWouldBeOverwritten: ChangesThatWouldBeOverwritten;
+}> = ({ changeType, changesThatWouldBeOverwritten }) => {
+    return (
+        <ChangesToOverwriteContainer>
+            <p>
+                <strong>Heads up!</strong> The ${changeType} has been updated
+                since you made your changes. Applying this change now would
+                overwrite the configuration that is currently live.
+            </p>
+            <details>
+                <summary>Changes that would be overwritten</summary>
+                <DetailsTable
+                    changesThatWouldBeOverwritten={
+                        changesThatWouldBeOverwritten
+                    }
+                />
+            </details>
+        </ChangesToOverwriteContainer>
+    );
+};
+
+export const SegmentChangesToOverwrite: React.FC<{
+    currentSegment?: ISegment;
+    change: IChangeRequestUpdateSegment;
+}> = ({ change, currentSegment }) => {
+    const checkForChanges = useUiFlag('changeRequestConflictHandling');
+    const changesThatWouldBeOverwritten = checkForChanges
+        ? getSegmentChangesThatWouldBeOverwritten(currentSegment, change)
+        : null;
+
+    if (!changesThatWouldBeOverwritten) {
+        return null;
+    }
+
+    return (
+        <OverwriteWarning
+            changeType='segment'
+            changesThatWouldBeOverwritten={changesThatWouldBeOverwritten}
+        />
+    );
+};
+
+export const StrategyChangesToOverwrite: React.FC<{
     currentStrategy?: IFeatureStrategy;
     change: IChangeRequestUpdateStrategy;
 }> = ({ change, currentStrategy }) => {
     const checkForChanges = useUiFlag('changeRequestConflictHandling');
     const changesThatWouldBeOverwritten = checkForChanges
-        ? getChangesThatWouldBeOverwritten(currentStrategy, change)
+        ? getStrategyChangesThatWouldBeOverwritten(currentStrategy, change)
         : null;
     const { registerWillOverwriteStrategyChanges } =
         useChangeRequestPlausibleContext();
@@ -92,73 +200,9 @@ export const ChangesToOverwrite: React.FC<{
     }
 
     return (
-        <ChangesToOverwriteWarning>
+        <OverwriteWarning
-            <p>
+            changeType='strategy'
-                <strong>Heads up!</strong> The strategy has been updated since
+            changesThatWouldBeOverwritten={changesThatWouldBeOverwritten}
-                you made your changes. Applying this change now would overwrite
+        />
-                the configuration that is currently live.
-            </p>
-            <details>
-                <summary>Changes that would be overwritten</summary>
-
-                <OverwriteTable>
-                    <thead>
-                        <tr>
-                            <th>Property</th>
-                            <th>Current value</th>
-                            <th>Value after change</th>
-                        </tr>
-                    </thead>
-
-                    <tbody>
-                        {changesThatWouldBeOverwritten.map(
-                            ({ property, oldValue, newValue }) => (
-                                <tr key={property}>
-                                    <td data-column='Property'>{property}</td>
-                                    <td data-column='Current value'>
-                                        <pre>
-                                            <del>
-                                                {JSON.stringify(
-                                                    oldValue,
-                                                    null,
-                                                    2,
-                                                )
-                                                    .split('\n')
-                                                    .map((line, index) => (
-                                                        <code
-                                                            key={`${property}${line}${index}`}
-                                                        >
-                                                            {`${line}\n`}
-                                                        </code>
-                                                    ))}
-                                            </del>
-                                        </pre>
-                                    </td>
-                                    <td data-column='Value after change'>
-                                        <pre>
-                                            <ins>
-                                                {JSON.stringify(
-                                                    newValue,
-                                                    null,
-                                                    2,
-                                                )
-                                                    .split('\n')
-                                                    .map((line, index) => (
-                                                        <code
-                                                            key={`${property}${line}${index}`}
-                                                        >
-                                                            {`${line}\n`}
-                                                        </code>
-                                                    ))}
-                                            </ins>
-                                        </pre>
-                                    </td>
-                                </tr>
-                            ),
-                        )}
-                    </tbody>
-                </OverwriteTable>
-            </details>
-        </ChangesToOverwriteWarning>
     );
 };

frontend/src/component/changeRequest/ChangeRequest/Changes/Change/strategy-change-diff-calculation.test.ts

@@ -1,10 +1,14 @@
 import {
     ChangeRequestEditStrategy,
+    IChangeRequestUpdateSegment,
     IChangeRequestUpdateStrategy,
 } from 'component/changeRequest/changeRequest.types';
 import { IFeatureStrategy } from 'interfaces/strategy';
 import omit from 'lodash.omit';
-import { getChangesThatWouldBeOverwritten } from './strategy-change-diff-calculation';
+import {
+    getSegmentChangesThatWouldBeOverwritten,
+    getStrategyChangesThatWouldBeOverwritten,
+} from './strategy-change-diff-calculation';
 
 describe('Strategy change conflict detection', () => {
     const existingStrategy: IFeatureStrategy = {
@@ -67,7 +71,7 @@ describe('Strategy change conflict detection', () => {
     };
 
     test('It compares strategies regardless of order of keys in the objects', () => {
-        const result = getChangesThatWouldBeOverwritten(
+        const result = getStrategyChangesThatWouldBeOverwritten(
             existingStrategy,
             change,
         );
@@ -76,7 +80,7 @@ describe('Strategy change conflict detection', () => {
     });
 
     test('It treats `undefined` or missing segments in old config as equal to `[]` in change', () => {
-        const resultUndefined = getChangesThatWouldBeOverwritten(
+        const resultUndefined = getStrategyChangesThatWouldBeOverwritten(
             {
                 ...existingStrategy,
                 segments: undefined,
@@ -87,7 +91,7 @@ describe('Strategy change conflict detection', () => {
         expect(resultUndefined).toBeNull();
 
         const { segments, ...withoutSegments } = existingStrategy;
-        const resultMissing = getChangesThatWouldBeOverwritten(
+        const resultMissing = getStrategyChangesThatWouldBeOverwritten(
             withoutSegments,
             change,
         );
@@ -132,7 +136,10 @@ describe('Strategy change conflict detection', () => {
         ].flatMap((existing) =>
             [undefinedVariantsInSnapshot, missingVariantsInSnapshot].map(
                 (changeValue) =>
-                    getChangesThatWouldBeOverwritten(existing, changeValue),
+                    getStrategyChangesThatWouldBeOverwritten(
+                        existing,
+                        changeValue,
+                    ),
             ),
         );
 
@@ -175,7 +182,10 @@ describe('Strategy change conflict detection', () => {
             segments: [3],
         };
 
-        const result = getChangesThatWouldBeOverwritten(withChanges, change);
+        const result = getStrategyChangesThatWouldBeOverwritten(
+            withChanges,
+            change,
+        );
 
         const { id, name, ...changedProperties } = withChanges;
 
@@ -228,7 +238,7 @@ describe('Strategy change conflict detection', () => {
         };
 
         expect(
-            getChangesThatWouldBeOverwritten(
+            getStrategyChangesThatWouldBeOverwritten(
                 existingStrategyMod,
                 constraintChange,
             ),
@@ -255,7 +265,7 @@ describe('Strategy change conflict detection', () => {
             ],
         };
 
-        const result = getChangesThatWouldBeOverwritten(
+        const result = getStrategyChangesThatWouldBeOverwritten(
             existingStrategyWithVariants,
             {
                 ...change,
@@ -276,16 +286,22 @@ describe('Strategy change conflict detection', () => {
     });
 
     test('it returns null if the existing strategy is undefined', () => {
-        const result = getChangesThatWouldBeOverwritten(undefined, change);
+        const result = getStrategyChangesThatWouldBeOverwritten(
+            undefined,
+            change,
+        );
 
         expect(result).toBeNull();
     });
     test('it returns null if the snapshot is missing', () => {
         const { snapshot, ...payload } = change.payload;
-        const result = getChangesThatWouldBeOverwritten(existingStrategy, {
+        const result = getStrategyChangesThatWouldBeOverwritten(
-            ...change,
+            existingStrategy,
-            payload,
+            {
-        });
+                ...change,
+                payload,
+            },
+        );
 
         expect(result).toBeNull();
     });
@@ -338,7 +354,7 @@ describe('Strategy change conflict detection', () => {
                 emptyTitleInSnapshot,
                 missingTitleInSnapshot,
             ].map((changeValue) =>
-                getChangesThatWouldBeOverwritten(existing, changeValue),
+                getStrategyChangesThatWouldBeOverwritten(existing, changeValue),
             ),
         );
 
@@ -358,7 +374,7 @@ describe('Strategy change conflict detection', () => {
                 title: 'other-new-title',
             },
         };
-        const result = getChangesThatWouldBeOverwritten(
+        const result = getStrategyChangesThatWouldBeOverwritten(
             liveVersion,
             changedVersion,
         );
@@ -385,7 +401,7 @@ describe('Strategy change conflict detection', () => {
                 title: liveVersion.title,
             },
         };
-        const result = getChangesThatWouldBeOverwritten(
+        const result = getStrategyChangesThatWouldBeOverwritten(
             liveVersion,
             changedVersion,
         );
@@ -401,7 +417,7 @@ describe('Strategy change conflict detection', () => {
                 title: 'new-title',
             },
         };
-        const result = getChangesThatWouldBeOverwritten(
+        const result = getStrategyChangesThatWouldBeOverwritten(
             existingStrategy,
             changedVersion,
         );
@@ -409,3 +425,82 @@ describe('Strategy change conflict detection', () => {
         expect(result).toBeNull();
     });
 });
+
+describe('Segment change conflict detection', () => {
+    const snapshot = {
+        id: 12,
+        name: 'Original name',
+        project: 'change-request-conflict-handling',
+        createdAt: '2024-02-06T09:11:23.782Z',
+        createdBy: 'admin',
+        constraints: [],
+        description: '',
+    };
+
+    const change: IChangeRequestUpdateSegment = {
+        id: 39,
+        action: 'updateSegment' as const,
+        name: 'what?a',
+        payload: {
+            id: 12,
+            name: 'Original name',
+            project: 'change-request-conflict-handling',
+            constraints: [],
+            snapshot,
+        },
+    };
+
+    test('it registers any change in constraints as everything will be overwritten', () => {
+        const segmentWithConstraints = {
+            ...snapshot,
+            constraints: [
+                {
+                    values: ['blah'],
+                    inverted: false,
+                    operator: 'IN' as const,
+                    contextName: 'appName',
+                    caseInsensitive: false,
+                },
+            ],
+        };
+
+        const changeWithConstraints = {
+            ...change,
+            payload: {
+                ...change.payload,
+                constraints: [
+                    ...segmentWithConstraints.constraints,
+                    {
+                        values: ['bluh'],
+                        inverted: false,
+                        operator: 'IN' as const,
+                        contextName: 'appName',
+                        caseInsensitive: false,
+                    },
+                ],
+            },
+        };
+
+        const result = getSegmentChangesThatWouldBeOverwritten(
+            segmentWithConstraints,
+            changeWithConstraints,
+        );
+
+        expect(result).toStrictEqual([
+            {
+                property: 'constraints',
+                oldValue: segmentWithConstraints.constraints,
+                newValue: changeWithConstraints.payload.constraints,
+            },
+        ]);
+    });
+
+    test('It treats missing description in change as equal to an empty description in snapshot', () => {
+        const result = getSegmentChangesThatWouldBeOverwritten(
+            snapshot,
+            change,
+        );
+
+        expect(result).toBeNull();
+    });
+});

frontend/src/component/changeRequest/ChangeRequest/Changes/Change/strategy-change-diff-calculation.ts

@@ -1,7 +1,8 @@
 import {
-    ChangeRequestEditStrategy,
+    IChangeRequestUpdateSegment,
     IChangeRequestUpdateStrategy,
 } from 'component/changeRequest/changeRequest.types';
+import { ISegment } from 'interfaces/segment';
 import { IFeatureStrategy } from 'interfaces/strategy';
 import isEqual from 'lodash.isequal';
 import omit from 'lodash.omit';
@@ -33,65 +34,69 @@ const hasChanged = (
     return hasJsonDiff()(snapshotValue, currentValue, changeValue);
 };
 
-type DataToOverwrite<Prop extends keyof ChangeRequestEditStrategy> = {
+type DataToOverwrite = {
-    property: Prop;
+    property: string;
-    oldValue: ChangeRequestEditStrategy[Prop];
+    oldValue: unknown;
-    newValue: ChangeRequestEditStrategy[Prop];
+    newValue: unknown;
 };
 
-type ChangesThatWouldBeOverwritten = DataToOverwrite<
+export type ChangesThatWouldBeOverwritten = DataToOverwrite[];
-    keyof ChangeRequestEditStrategy
->[];
 
-const removeEmptyEntries = (
+function isNotUndefined<T>(value: T | undefined): value is T {
-    change: unknown,
+    return value !== undefined;
-): change is DataToOverwrite<keyof ChangeRequestEditStrategy> =>
+}
-    Boolean(change);
 
-const getChangedProperty = (
+const getChangedPropertyWithFallbacks =
-    key: keyof ChangeRequestEditStrategy,
+    (fallbacks: { [key: string]: unknown }) =>
-    currentValue: unknown,
+    (
-    snapshotValue: unknown,
+        key: string,
-    changeValue: unknown,
+        currentValue: unknown,
-) => {
+        snapshotValue: unknown,
-    const fallbacks = { segments: [], variants: [], title: '' };
+        changeValue: unknown,
-    const fallback = fallbacks[key as keyof typeof fallbacks] ?? undefined;
+    ) => {
-    const diffCheck = key in fallbacks ? hasJsonDiff(fallback) : hasChanged;
+        const fallback = fallbacks[key as keyof typeof fallbacks] ?? undefined;
+        const diffCheck = key in fallbacks ? hasJsonDiff(fallback) : hasChanged;
 
-    const changeInfo = {
+        const changeInfo = {
-        property: key as keyof ChangeRequestEditStrategy,
+            property: key,
-        oldValue: currentValue,
+            oldValue: currentValue,
-        newValue: changeValue,
+            newValue: changeValue,
+        };
+
+        return diffCheck(snapshotValue, currentValue, changeValue)
+            ? changeInfo
+            : undefined;
     };
 
-    return diffCheck(snapshotValue, currentValue, changeValue)
+type Change<T> = {
-        ? changeInfo
+    payload: Partial<T> & {
-        : undefined;
+        snapshot?: { [Key in keyof T]: unknown };
+    };
 };
 
-export const getChangesThatWouldBeOverwritten = (
+function getChangesThatWouldBeOverwritten<T>(
-    currentStrategyConfig: IFeatureStrategy | undefined,
+    currentConfig: T | undefined,
-    change: IChangeRequestUpdateStrategy,
+    change: Change<T>,
-): ChangesThatWouldBeOverwritten | null => {
+    fallbacks: Partial<T>,
+): ChangesThatWouldBeOverwritten | null {
     const { snapshot } = change.payload;
-    if (!snapshot || !currentStrategyConfig) return null;
+    if (!snapshot || !currentConfig) return null;
 
-    const changes: ChangesThatWouldBeOverwritten = Object.entries(
+    const getChangedProperty = getChangedPropertyWithFallbacks(fallbacks);
-        omit(currentStrategyConfig, 'strategyName'),
+
-    )
+    const changes: ChangesThatWouldBeOverwritten = Object.entries(currentConfig)
         .map(([key, currentValue]: [string, unknown]) => {
-            const snapshotValue = snapshot[key as keyof IFeatureStrategy];
+            const snapshotValue = snapshot[key as keyof T];
-            const changeValue =
+            const changeValue = change.payload[key as keyof T];
-                change.payload[key as keyof ChangeRequestEditStrategy];
 
             return getChangedProperty(
-                key as keyof ChangeRequestEditStrategy,
+                key,
                 currentValue,
                 snapshotValue,
                 changeValue,
             );
         })
-        .filter(removeEmptyEntries);
+        .filter(isNotUndefined);
 
     if (changes.length) {
         changes.sort((a, b) => a.property.localeCompare(b.property));
@@ -99,4 +104,28 @@ export const getChangesThatWouldBeOverwritten = (
     }
 
     return null;
-};
+}
+
+export function getSegmentChangesThatWouldBeOverwritten(
+    currentSegmentConfig: ISegment | undefined,
+    change: IChangeRequestUpdateSegment,
+): ChangesThatWouldBeOverwritten | null {
+    const fallbacks = { description: '' };
+    return getChangesThatWouldBeOverwritten(
+        omit(currentSegmentConfig, 'createdAt', 'createdBy'),
+        change,
+        fallbacks,
+    );
+}
+
+export function getStrategyChangesThatWouldBeOverwritten(
+    currentStrategyConfig: IFeatureStrategy | undefined,
+    change: IChangeRequestUpdateStrategy,
+): ChangesThatWouldBeOverwritten | null {
+    const fallbacks = { segments: [], variants: [], title: '' };
+    return getChangesThatWouldBeOverwritten(
+        omit(currentStrategyConfig, 'strategyName'),
+        change,
+        fallbacks,
+    );
+}

frontend/src/component/changeRequest/changeRequest.types.ts

@@ -1,4 +1,5 @@
 import { IFeatureVariant } from 'interfaces/featureToggle';
+import { ISegment } from 'interfaces/segment';
 import { IFeatureStrategy } from '../../interfaces/strategy';
 import { IUser } from '../../interfaces/user';
 import { SetStrategySortOrderSchema } from '../../openapi';
@@ -183,6 +184,7 @@ export interface IChangeRequestUpdateSegment {
         description?: string;
         project?: string;
         constraints: IFeatureStrategy['constraints'];
+        snapshot?: ISegment;
     };
 }