diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/SegmentChangeDetails.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/SegmentChangeDetails.tsx index 2ef33e3674..5a3281d1c7 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/SegmentChangeDetails.tsx +++ b/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' && ( <> + Editing segment: diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx index c7b71a4c9e..5fbfb1ffcc 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx +++ b/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' && ( <> - diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChangeOverwriteWarning.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChangeOverwriteWarning.tsx index 8c6bf2ae42..ccae24d905 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChangeOverwriteWarning.tsx +++ b/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 ( + + + + Property + Current value + Value after change + + + + + {changesThatWouldBeOverwritten.map( + ({ property, oldValue, newValue }) => ( + + {property} + +
+                                    
+                                        {JSON.stringify(oldValue, null, 2)
+                                            .split('\n')
+                                            .map((line, index) => (
+                                                
+                                                    {`${line}\n`}
+                                                
+                                            ))}
+                                    
+                                
+ + +
+                                    
+                                        {JSON.stringify(newValue, null, 2)
+                                            .split('\n')
+                                            .map((line, index) => (
+                                                
+                                                    {`${line}\n`}
+                                                
+                                            ))}
+                                    
+                                
+ + + ), + )} + +
+ ); +}; + +const OverwriteWarning: React.FC<{ + changeType: 'segment' | 'strategy'; + changesThatWouldBeOverwritten: ChangesThatWouldBeOverwritten; +}> = ({ changeType, changesThatWouldBeOverwritten }) => { + return ( + +

+ Heads up! The ${changeType} has been updated + since you made your changes. Applying this change now would + overwrite the configuration that is currently live. +

+
+ Changes that would be overwritten + +
+
+ ); +}; + +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 ( + + ); +}; + +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 ( - -

- Heads up! The strategy has been updated since - you made your changes. Applying this change now would overwrite - the configuration that is currently live. -

-
- Changes that would be overwritten - - - - - Property - Current value - Value after change - - - - - {changesThatWouldBeOverwritten.map( - ({ property, oldValue, newValue }) => ( - - {property} - -
-                                            
-                                                {JSON.stringify(
-                                                    oldValue,
-                                                    null,
-                                                    2,
-                                                )
-                                                    .split('\n')
-                                                    .map((line, index) => (
-                                                        
-                                                            {`${line}\n`}
-                                                        
-                                                    ))}
-                                            
-                                        
- - -
-                                            
-                                                {JSON.stringify(
-                                                    newValue,
-                                                    null,
-                                                    2,
-                                                )
-                                                    .split('\n')
-                                                    .map((line, index) => (
-                                                        
-                                                            {`${line}\n`}
-                                                        
-                                                    ))}
-                                            
-                                        
- - - ), - )} - -
-
-
+ ); }; 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 8467acd54c..1c0b660fe9 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,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, { - ...change, - payload, - }); + const result = getStrategyChangesThatWouldBeOverwritten( + existingStrategy, + { + ...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(); + }); +}); 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 24426c2dc2..97bafca54e 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,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 = { - property: Prop; - oldValue: ChangeRequestEditStrategy[Prop]; - newValue: ChangeRequestEditStrategy[Prop]; +type DataToOverwrite = { + property: string; + oldValue: unknown; + newValue: unknown; }; -type ChangesThatWouldBeOverwritten = DataToOverwrite< - keyof ChangeRequestEditStrategy ->[]; +export type ChangesThatWouldBeOverwritten = DataToOverwrite[]; -const removeEmptyEntries = ( - change: unknown, -): change is DataToOverwrite => - Boolean(change); +function isNotUndefined(value: T | undefined): value is T { + return value !== undefined; +} -const getChangedProperty = ( - key: keyof ChangeRequestEditStrategy, - currentValue: unknown, - snapshotValue: unknown, - changeValue: unknown, -) => { - const fallbacks = { segments: [], variants: [], title: '' }; - const fallback = fallbacks[key as keyof typeof fallbacks] ?? undefined; - const diffCheck = key in fallbacks ? hasJsonDiff(fallback) : hasChanged; +const getChangedPropertyWithFallbacks = + (fallbacks: { [key: string]: unknown }) => + ( + key: string, + currentValue: unknown, + snapshotValue: unknown, + changeValue: unknown, + ) => { + const fallback = fallbacks[key as keyof typeof fallbacks] ?? undefined; + const diffCheck = key in fallbacks ? hasJsonDiff(fallback) : hasChanged; - const changeInfo = { - property: key as keyof ChangeRequestEditStrategy, - oldValue: currentValue, - newValue: changeValue, + const changeInfo = { + property: key, + oldValue: currentValue, + newValue: changeValue, + }; + + return diffCheck(snapshotValue, currentValue, changeValue) + ? changeInfo + : undefined; }; - return diffCheck(snapshotValue, currentValue, changeValue) - ? changeInfo - : undefined; +type Change = { + payload: Partial & { + snapshot?: { [Key in keyof T]: unknown }; + }; }; -export const getChangesThatWouldBeOverwritten = ( - currentStrategyConfig: IFeatureStrategy | undefined, - change: IChangeRequestUpdateStrategy, -): ChangesThatWouldBeOverwritten | null => { +function getChangesThatWouldBeOverwritten( + currentConfig: T | undefined, + change: Change, + fallbacks: Partial, +): ChangesThatWouldBeOverwritten | null { const { snapshot } = change.payload; - if (!snapshot || !currentStrategyConfig) return null; + if (!snapshot || !currentConfig) return null; - const changes: ChangesThatWouldBeOverwritten = Object.entries( - omit(currentStrategyConfig, 'strategyName'), - ) + const getChangedProperty = getChangedPropertyWithFallbacks(fallbacks); + + const changes: ChangesThatWouldBeOverwritten = Object.entries(currentConfig) .map(([key, currentValue]: [string, unknown]) => { - const snapshotValue = snapshot[key as keyof IFeatureStrategy]; - const changeValue = - change.payload[key as keyof ChangeRequestEditStrategy]; + const snapshotValue = snapshot[key as keyof T]; + const changeValue = change.payload[key as keyof T]; 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, + ); +} diff --git a/frontend/src/component/changeRequest/changeRequest.types.ts b/frontend/src/component/changeRequest/changeRequest.types.ts index 6c28f828a5..33dbe168ec 100644 --- a/frontend/src/component/changeRequest/changeRequest.types.ts +++ b/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; }; }