Fix/autosave on delete (#5899)

This PR will make FeatureStrategyConstraints use the value coming from the setState function instead of closing over a stale value.
Unleash · Jan 16, 2024 · 9d370ad · 9d370ad
1 parent 6e23472
commit 9d370ad
Show file tree

Hide file tree

Showing 5 changed files with 50 additions and 83 deletions.
diff --git a/...mponent/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEdit.tsx b/...mponent/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEdit.tsx
@@ -134,10 +134,6 @@ export const ConstraintAccordionEdit = ({
         if (onAutoSave) {
             onAutoSave(localConstraint);
         }
-
-        if (onAutoSave && localConstraint.value) {
-            onAutoSave(localConstraint);
-        }
     };
 
     const recordChange = (localConstraint: IConstraint) => {
@@ -233,7 +229,7 @@ export const ConstraintAccordionEdit = ({
 
             setValuesWithRecord(valueCopy);
         },
-        [localConstraint, setValues],
+        [localConstraint, setValuesWithRecord],
     );
 
     const triggerTransition = () => {

diff --git a/...rdion/ConstraintAccordionEdit/ConstraintAccordionEditBody/FreeTextInput/FreeTextInput.tsx b/...rdion/ConstraintAccordionEdit/ConstraintAccordionEditBody/FreeTextInput/FreeTextInput.tsx
@@ -157,6 +157,7 @@ const ConstraintValueChips = ({
                         key={`${value}-${index}`}
                         onDelete={() => removeValue(index)}
                         className={styles.valueChip}
+                        data-testid='CONSTRAINT_VALUES_CHIP'
                     />
                 );
             })}

diff --git a/...t/common/NewConstraintAccordion/NewConstraintAccordionList/NewConstraintAccordionList.tsx b/...t/common/NewConstraintAccordion/NewConstraintAccordionList/NewConstraintAccordionList.tsx
@@ -74,6 +74,7 @@ export const useConstraintAccordionList = (
         | undefined,
     ref: React.RefObject<IConstraintAccordionListRef>,
 ) => {
+    // Constraint metadata: This is a weak map to give a constraint an ID by using the placement in memory.
     const state = useWeakMap<IConstraint, IConstraintAccordionListItemState>();
     const { context } = useUnleashContext();
 
@@ -97,80 +98,6 @@ export const useConstraintAccordionList = (
 
     return { onAdd, state, context };
 };
-
-export const ConstraintAccordionList = forwardRef<
-    IConstraintAccordionListRef | undefined,
-    IConstraintAccordionListProps
->(
-    (
-        { constraints, setConstraints, showCreateButton, showLabel = true },
-        ref,
-    ) => {
-        const { onAdd, state, context } = useConstraintAccordionList(
-            setConstraints,
-            ref as RefObject<IConstraintAccordionListRef>,
-        );
-
-        if (context.length === 0) {
-            return null;
-        }
-
-        return (
-            <StyledContainer id={constraintAccordionListId}>
-                <ConditionallyRender
-                    condition={
-                        constraints && constraints.length > 0 && showLabel
-                    }
-                    show={
-                        <StyledConstraintLabel>
-                            Constraints
-                        </StyledConstraintLabel>
-                    }
-                />
-                <NewConstraintAccordionList
-                    ref={ref}
-                    setConstraints={setConstraints}
-                    constraints={constraints}
-                    state={state}
-                />
-                <ConditionallyRender
-                    condition={Boolean(showCreateButton && onAdd)}
-                    show={
-                        <div>
-                            <StyledAddCustomLabel>
-                                <p>Add any number of constraints</p>
-                                <StyledHelpWrapper
-                                    title='View constraints documentation'
-                                    arrow
-                                >
-                                    <a
-                                        href={
-                                            'https://docs.getunleash.io/reference/strategy-constraints'
-                                        }
-                                        target='_blank'
-                                        rel='noopener noreferrer'
-                                    >
-                                        <StyledHelp />
-                                    </a>
-                                </StyledHelpWrapper>
-                            </StyledAddCustomLabel>
-                            <Button
-                                type='button'
-                                onClick={onAdd}
-                                variant='outlined'
-                                color='primary'
-                                data-testid='ADD_CONSTRAINT_BUTTON'
-                            >
-                                Add constraint
-                            </Button>
-                        </div>
-                    }
-                />
-            </StyledContainer>
-        );
-    },
-);
-
 interface IConstraintList {
     constraints: IConstraint[];
     setConstraints?: React.Dispatch<React.SetStateAction<IConstraint[]>>;

diff --git a/...mponent/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraints.tsx b/...mponent/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraints.tsx
@@ -47,14 +47,15 @@ export const FeatureStrategyConstraints = ({
         };
     }, []);
 
-    const constraints = useMemo(() => {
-        return strategy.constraints ?? [];
-    }, [strategy]);
+    const constraints = strategy.constraints || [];
 
     const setConstraints = (value: React.SetStateAction<IConstraint[]>) => {
         setStrategy((prev) => ({
             ...prev,
-            constraints: value instanceof Function ? value(constraints) : value,
+            constraints:
+                value instanceof Function
+                    ? value(prev.constraints || [])
+                    : value,
         }));
     };
 

diff --git a/...ponent/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.test.tsx b/...ponent/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.test.tsx
@@ -277,6 +277,48 @@ describe('NewFeatureStrategyCreate', () => {
         expect(screen.getByText(values[2])).toBeInTheDocument();
     });
 
+    test('Should update multiple constraints correctly', 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 = await screen.findAllByTestId('CancelIcon');
+        fireEvent.click(deleteBtns[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();