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
Unleash · Jan 17, 2024 · 9d32bf5 · 9d32bf5
1 parent 5f4fe20
commit 9d32bf5
Show file tree

Hide file tree

Showing 4 changed files with 71 additions and 11 deletions.
diff --git a/...ConstraintAccordion/ConstraintAccordionHeaderActions/ConstraintAccordionHeaderActions.tsx b/...ConstraintAccordion/ConstraintAccordionHeaderActions/ConstraintAccordionHeaderActions.tsx
@@ -90,6 +90,7 @@ export const ConstraintAccordionHeaderActions = ({
                             type='button'
                             onClick={onDeleteClick}
                             disabled={disableDelete}
+                            data-testid='DELETE_CONSTRAINT_BUTTON'
                         >
                             <Delete />
                         </IconButton>

diff --git a/...t/common/NewConstraintAccordion/NewConstraintAccordionList/NewConstraintAccordionList.tsx b/...t/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 (
         <StyledContainer id={constraintAccordionListId}>
             {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/...mponent/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraints.tsx b/...mponent/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraints.tsx
@@ -50,13 +50,15 @@ export const FeatureStrategyConstraints = ({
     const constraints = strategy.constraints || [];
 
     const setConstraints = (value: React.SetStateAction<IConstraint[]>) => {
-        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/...ponent/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.test.tsx b/...ponent/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();