From 9d32bf53eb4fa993d1b613a6d792e8caf3fe69c7 Mon Sep 17 00:00:00 2001 From: Fredrik Strand Oseberg Date: Wed, 17 Jan 2024 08:51:54 +0100 Subject: [PATCH] fix: refactor autosave to use the id to resolve the constraint (#5917) Use id to resolve autosave constraints now that we have consistent id references --- .../ConstraintAccordionHeaderActions.tsx | 1 + .../NewConstraintAccordionList.tsx | 12 +++-- .../FeatureStrategyConstraints.tsx | 16 +++--- .../NewFeatureStrategyCreate.test.tsx | 53 +++++++++++++++++++ 4 files changed, 71 insertions(+), 11 deletions(-) diff --git a/frontend/src/component/common/NewConstraintAccordion/ConstraintAccordionHeaderActions/ConstraintAccordionHeaderActions.tsx b/frontend/src/component/common/NewConstraintAccordion/ConstraintAccordionHeaderActions/ConstraintAccordionHeaderActions.tsx index 33b99f3f33..cc5e83a1d7 100644 --- a/frontend/src/component/common/NewConstraintAccordion/ConstraintAccordionHeaderActions/ConstraintAccordionHeaderActions.tsx +++ b/frontend/src/component/common/NewConstraintAccordion/ConstraintAccordionHeaderActions/ConstraintAccordionHeaderActions.tsx @@ -90,6 +90,7 @@ export const ConstraintAccordionHeaderActions = ({ type='button' onClick={onDeleteClick} disabled={disableDelete} + data-testid='DELETE_CONSTRAINT_BUTTON' > diff --git a/frontend/src/component/common/NewConstraintAccordion/NewConstraintAccordionList/NewConstraintAccordionList.tsx b/frontend/src/component/common/NewConstraintAccordion/NewConstraintAccordionList/NewConstraintAccordionList.tsx index dc38f9b180..bb7ec74e06 100644 --- a/frontend/src/component/common/NewConstraintAccordion/NewConstraintAccordionList/NewConstraintAccordionList.tsx +++ b/frontend/src/component/common/NewConstraintAccordion/NewConstraintAccordionList/NewConstraintAccordionList.tsx @@ -139,11 +139,16 @@ export const NewConstraintAccordionList = forwardRef< const onAutoSave = setConstraints && - ((index: number, constraint: IConstraint) => { + ((id: string | undefined) => (constraint: IConstraint) => { state.set(constraint, { editing: true }); setConstraints( produce((draft) => { - draft[index] = constraint; + return draft.map((oldConstraint) => { + if (oldConstraint[constraintId] === id) { + return constraint; + } + return oldConstraint; + }); }), ); }); @@ -161,7 +166,6 @@ export const NewConstraintAccordionList = forwardRef< return ( {constraints.map((constraint, index) => { - // biome-ignore lint: reason=objectId would change every time values change - this is no different than using index const id = constraint[constraintId]; return ( @@ -177,7 +181,7 @@ export const NewConstraintAccordionList = forwardRef< onCancel={onCancel.bind(null, index)} onDelete={onRemove?.bind(null, index)} onSave={onSave?.bind(null, index)} - onAutoSave={onAutoSave?.bind(null, index)} + onAutoSave={onAutoSave?.(id)} editing={Boolean(state.get(constraint)?.editing)} compact /> diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraints.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraints.tsx index 79b9cd6ea2..4ba1791463 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraints.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraints.tsx @@ -50,13 +50,15 @@ export const FeatureStrategyConstraints = ({ const constraints = strategy.constraints || []; const setConstraints = (value: React.SetStateAction) => { - setStrategy((prev) => ({ - ...prev, - constraints: - value instanceof Function - ? value(prev.constraints || []) - : value, - })); + setStrategy((prev) => { + return { + ...prev, + constraints: + value instanceof Function + ? value(prev.constraints || []) + : value, + }; + }); }; const showCreateButton = useHasProjectEnvironmentAccess( diff --git a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.test.tsx b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.test.tsx index 54c0fc2970..615b798829 100644 --- a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.test.tsx +++ b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.test.tsx @@ -319,6 +319,59 @@ describe('NewFeatureStrategyCreate', () => { expect(screen.queryByText('789')).toBeInTheDocument(); }); + test('Should update multiple constraints with the correct react key', async () => { + setupComponent(); + + const titleEl = await screen.findByText('Gradual rollout'); + expect(titleEl).toBeInTheDocument(); + + const targetingEl = screen.getByText('Targeting'); + fireEvent.click(targetingEl); + + const addConstraintEl = await screen.findByText('Add constraint'); + fireEvent.click(addConstraintEl); + fireEvent.click(addConstraintEl); + fireEvent.click(addConstraintEl); + + const inputElements = screen.getAllByPlaceholderText( + 'value1, value2, value3...', + ); + + fireEvent.change(inputElements[0], { + target: { value: '123' }, + }); + fireEvent.change(inputElements[1], { + target: { value: '456' }, + }); + fireEvent.change(inputElements[2], { + target: { value: '789' }, + }); + + const addValueEls = await screen.findAllByText('Add values'); + fireEvent.click(addValueEls[0]); + fireEvent.click(addValueEls[1]); + fireEvent.click(addValueEls[2]); + + expect(screen.queryByText('123')).toBeInTheDocument(); + + const deleteBtns = screen.getAllByTestId('DELETE_CONSTRAINT_BUTTON'); + fireEvent.click(deleteBtns[0]); + + const inputElements2 = screen.getAllByPlaceholderText( + 'value1, value2, value3...', + ); + + fireEvent.change(inputElements2[0], { + target: { value: '666' }, + }); + const addValueEls2 = screen.getAllByText('Add values'); + fireEvent.click(addValueEls2[0]); + + expect(screen.queryByText('123')).not.toBeInTheDocument(); + expect(screen.queryByText('456')).toBeInTheDocument(); + expect(screen.queryByText('789')).toBeInTheDocument(); + }); + test('Should undo changes made to constraints', async () => { setupComponent();