Fix/legal values deletion (#4564)

This PR fixes a bug reported from a customer where deleting a legal
value that was used in a strategy constraint would make it impossible to
edit the constraint.

[The bug was introduced
here](https://github.com/Unleash/unleash/pull/4473)

The core of the problem introduced was that the values used to calculate
illegal values was based on changing state. On the first render it would
display correct state as it would match the legal values coming from the
context definition with the legal values currently used in the
constraint as values. However, when you triggered the onClick method for
the checkboxes the state would be changed because we would remove the
illegal values from the valueset and only insert current legal values in
the state. This would trigger a re-render of the component, and now the
data used to identify the illegal values would no longer be correct,
because the bad values had been cleaned from the state. This would cause
the UI for constraints to display incorrectly.

Changed the flow to now give you a warning if you have illegal values,
and that if you make changes and save the strategy these values will be
removed from the constraint:

<img width="726" alt="Skjermbilde 2023-08-25 kl 08 56 02"
src="https://github.com/Unleash/unleash/assets/16081982/78e9875d-d864-4e21-bfb7-a530247a07eb">

Also amended this to apply to the single legal value constraints.

<img width="721" alt="Skjermbilde 2023-08-25 kl 08 57 40"
src="https://github.com/Unleash/unleash/assets/16081982/237a11d0-5c05-445c-9e99-b79cab0bff94">

frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEdit.tsx

@@ -252,6 +252,8 @@ export const ConstraintAccordionEdit = ({
                             setValue={setValue}
                             setError={setError}
                             localConstraint={localConstraint}
+                            constraintValues={constraint?.values || []}
+                            constraintValue={constraint?.value || ''}
                             input={input}
                             error={error}
                             contextDefinition={contextDefinition}

frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/ResolveInput/ResolveInput.tsx

@@ -22,6 +22,8 @@ import React from 'react';
 interface IResolveInputProps {
     contextDefinition: IUnleashContextDefinition;
     localConstraint: IConstraint;
+    constraintValues: string[];
+    constraintValue: string;
     setValue: (value: string) => void;
     setValues: (values: string[]) => void;
     setError: React.Dispatch<React.SetStateAction<string>>;
@@ -34,6 +36,13 @@ const resolveLegalValues = (
     values: IConstraint['values'],
     legalValues: IUnleashContextDefinition['legalValues']
 ): { legalValues: ILegalValue[]; deletedLegalValues: ILegalValue[] } => {
+    if (legalValues?.length === 0) {
+        return {
+            legalValues: [],
+            deletedLegalValues: [],
+        };
+    }
+
     const deletedLegalValues = (values || [])
         .filter(
             value =>
@@ -52,6 +61,8 @@ const resolveLegalValues = (
 export const ResolveInput = ({
     input,
     contextDefinition,
+    constraintValues,
+    constraintValue,
     localConstraint,
     setValue,
     setValues,
@@ -67,9 +78,10 @@ export const ResolveInput = ({
                     <>
                         <RestrictiveLegalValues
                             data={resolveLegalValues(
-                                localConstraint.values,
+                                constraintValues,
                                 contextDefinition.legalValues
                             )}
+                            constraintValues={constraintValues}
                             values={localConstraint.values || []}
                             setValues={setValues}
                             error={error}
@@ -81,8 +93,13 @@ export const ResolveInput = ({
                 return (
                     <>
                         <SingleLegalValue
+                            data={resolveLegalValues(
+                                [constraintValue],
+                                contextDefinition.legalValues
+                            )}
                             setValue={setValue}
                             value={localConstraint.value}
+                            constraintValue={constraintValue}
                             type="number"
                             legalValues={
                                 contextDefinition.legalValues?.filter(
@@ -98,8 +115,13 @@ export const ResolveInput = ({
                 return (
                     <>
                         <SingleLegalValue
+                            data={resolveLegalValues(
+                                [constraintValue],
+                                contextDefinition.legalValues
+                            )}
                             setValue={setValue}
                             value={localConstraint.value}
+                            constraintValue={constraintValue}
                             type="semver"
                             legalValues={contextDefinition.legalValues || []}
                             error={error}

frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/RestrictiveLegalValues/RestrictiveLegalValues.test.tsx

@@ -1,54 +1,52 @@
 import { render } from 'utils/testRenderer';
-import { RestrictiveLegalValues } from './RestrictiveLegalValues';
 import { screen } from '@testing-library/react';
-import { vi } from 'vitest';
+import { RestrictiveLegalValues } from './RestrictiveLegalValues';
 
-describe('RestictiveLegalValues', () => {
+test('should show alert when you have illegal legal values', async () => {
-    it('should show deleted legal values as disabled', async () => {
+    const contextDefinitionValues = [{ value: 'value1' }, { value: 'value2' }];
-        const value = 'some-value';
+    const fixedValues = ['value1', 'value2'];
-        const values = [{ value }];
+    const localValues = ['value1', 'value2'];
-        const legalValues = [{ value: 'some-other-value' }];
+    const deletedLegalValues = [{ value: 'value1' }];
 
-        render(
+    render(
-            <RestrictiveLegalValues
+        <RestrictiveLegalValues
-                data={{ legalValues, deletedLegalValues: values }}
+            data={{ legalValues: contextDefinitionValues, deletedLegalValues }}
-                values={[value]}
+            constraintValues={fixedValues}
-                setValues={vi.fn()}
+            values={localValues}
-                error={''}
+            setValues={() => {}}
-                setError={vi.fn()}
+            error={''}
-            />
+            setError={() => {}}
-        );
+        />
+    );
 
-        const input = await screen.findByDisplayValue('some-value');
+    await screen.findByText(
-
+        'This constraint is using legal values that have been deleted as valid options. If you save changes on this constraint and then save the strategy the following values will be removed:'
-        expect(input).toBeInTheDocument();
+    );
-        expect(input).toHaveProperty('disabled', true);
+});
-
+
-        expect(
+test('Should remove illegal legal values from internal value state when mounting', () => {
-            await screen.findByDisplayValue('some-other-value')
+    const contextDefinitionValues = [{ value: 'value1' }, { value: 'value2' }];
-        ).toBeInTheDocument();
+    const fixedValues = ['value1', 'value2'];
-    });
+    let localValues = ['value1', 'value2'];
-
+    const deletedLegalValues = [{ value: 'value1' }];
-    it('should remove deleted legal values when editing values', async () => {
+
-        const value = 'some-value';
+    const setValues = (values: string[]) => {
-        const deletedLegalValues = [{ value }];
+        localValues = values;
-        const legalValues = [
+    };
-            { value: 'some-other-value' },
+
-            { value: 'value2' },
+    render(
-        ];
+        <RestrictiveLegalValues
-        const setValues = vi.fn();
+            data={{
-        render(
+                legalValues: contextDefinitionValues,
-            <RestrictiveLegalValues
+                deletedLegalValues,
-                data={{ legalValues, deletedLegalValues }}
+            }}
-                values={[value, 'value2']}
+            constraintValues={fixedValues}
-                setValues={setValues}
+            values={localValues}
-                error={''}
+            setValues={setValues}
-                setError={vi.fn()}
+            error={''}
-            />
+            setError={() => {}}
-        );
+        />
-        const btn = await screen.findByDisplayValue('some-other-value');
+    );
-        btn.click();
+
-
+    expect(localValues).toEqual(['value2']);
-        expect(setValues).toHaveBeenCalledWith(['value2', 'some-other-value']);
-    });
 });

frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/RestrictiveLegalValues/RestrictiveLegalValues.tsx

@@ -1,6 +1,6 @@
 import { useEffect, useState } from 'react';
 import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender';
-import { Checkbox } from '@mui/material';
+import { Alert, Checkbox } from '@mui/material';
 import { useThemeStyles } from 'themes/themeStyles';
 import { ConstraintValueSearch } from 'component/common/ConstraintAccordion/ConstraintValueSearch/ConstraintValueSearch';
 import { ConstraintFormHeader } from '../ConstraintFormHeader/ConstraintFormHeader';
@@ -15,6 +15,7 @@ interface IRestrictiveLegalValuesProps {
         legalValues: ILegalValue[];
         deletedLegalValues: ILegalValue[];
     };
+    constraintValues: string[];
     values: string[];
     setValues: (values: string[]) => void;
     beforeValues?: JSX.Element;
@@ -35,35 +36,59 @@ const createValuesMap = (values: string[]): IValuesMap => {
     }, {});
 };
 
+export const getLegalValueSet = (values: ILegalValue[]) => {
+    return new Set(values.map(({ value }) => value));
+};
+
+export const getIllegalValues = (
+    constraintValues: string[],
+    deletedLegalValues: ILegalValue[]
+) => {
+    const deletedValuesSet = getLegalValueSet(deletedLegalValues);
+
+    return constraintValues.filter(value => deletedValuesSet.has(value));
+};
+
 export const RestrictiveLegalValues = ({
     data,
     values,
     setValues,
     error,
     setError,
+    constraintValues,
 }: IRestrictiveLegalValuesProps) => {
     const [filter, setFilter] = useState('');
     const { legalValues, deletedLegalValues } = data;
+
     const filteredValues = filterLegalValues(legalValues, filter);
 
     // Lazily initialise the values because there might be a lot of them.
     const [valuesMap, setValuesMap] = useState(() => createValuesMap(values));
     const { classes: styles } = useThemeStyles();
 
-    useEffect(() => {
-        setValuesMap(createValuesMap(values));
-    }, [values, setValuesMap]);
-
     const cleanDeletedLegalValues = (constraintValues: string[]): string[] => {
-        const deletedValuesSet = new Set(
+        const deletedValuesSet = getLegalValueSet(deletedLegalValues);
-            deletedLegalValues.map(({ value }) => value)
-        );
         return (
             constraintValues?.filter(value => !deletedValuesSet.has(value)) ||
             []
         );
     };
 
+    const illegalValues = getIllegalValues(
+        constraintValues,
+        deletedLegalValues
+    );
+
+    useEffect(() => {
+        setValuesMap(createValuesMap(values));
+    }, [values, setValuesMap, createValuesMap]);
+
+    useEffect(() => {
+        if (illegalValues.length > 0) {
+            setValues(cleanDeletedLegalValues(values));
+        }
+    }, []);
+
     const onChange = (legalValue: string) => {
         setError('');
 
@@ -80,6 +105,23 @@ export const RestrictiveLegalValues = ({
 
     return (
         <>
+            <ConditionallyRender
+                condition={Boolean(illegalValues && illegalValues.length > 0)}
+                show={
+                    <Alert severity="warning">
+                        This constraint is using legal values that have been
+                        deleted as valid options. If you save changes on this
+                        constraint and then save the strategy the following
+                        values will be removed:
+                        <ul>
+                            {illegalValues?.map(value => (
+                                <li key={value}>{value}</li>
+                            ))}
+                        </ul>
+                    </Alert>
+                }
+            />
+
             <ConstraintFormHeader>
                 Select values from a predefined set
             </ConstraintFormHeader>
@@ -92,7 +134,7 @@ export const RestrictiveLegalValues = ({
                     />
                 }
             />
-            {filteredValues.concat(deletedLegalValues).map(match => (
+            {filteredValues.map(match => (
                 <LegalValueLabel
                     key={match.value}
                     legal={match}
@@ -109,6 +151,7 @@ export const RestrictiveLegalValues = ({
                     }
                 />
             ))}
+
             <ConditionallyRender
                 condition={Boolean(error)}
                 show={<p className={styles.error}>{error}</p>}

frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/SingleLegalValue/SingleLegalValue.test.tsx

@@ -0,0 +1,27 @@
+import { render } from 'utils/testRenderer';
+import { screen } from '@testing-library/react';
+import { SingleLegalValue } from './SingleLegalValue';
+
+test('should show alert when you have illegal legal values', async () => {
+    const contextDefinitionValues = [{ value: 'value1' }, { value: 'value2' }];
+    const fixedValue = 'value1';
+    const localValue = 'value1';
+    const deletedLegalValues = [{ value: 'value1' }];
+
+    render(
+        <SingleLegalValue
+            data={{ legalValues: contextDefinitionValues, deletedLegalValues }}
+            constraintValue={fixedValue}
+            value={localValue}
+            setValue={() => {}}
+            type="number"
+            legalValues={contextDefinitionValues}
+            error={''}
+            setError={() => {}}
+        />
+    );
+
+    await screen.findByText(
+        'This constraint is using legal values that have been deleted as a valid option. Please select a new value from the remaining predefined legal values. The constraint will be updated with the new value when you save the strategy.'
+    );
+});

frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/SingleLegalValue/SingleLegalValue.tsx

@@ -1,6 +1,6 @@
 import React, { useState } from 'react';
 import { ConstraintFormHeader } from '../ConstraintFormHeader/ConstraintFormHeader';
-import { FormControl, RadioGroup, Radio } from '@mui/material';
+import { FormControl, RadioGroup, Radio, Alert } from '@mui/material';
 import { ConstraintValueSearch } from 'component/common/ConstraintAccordion/ConstraintValueSearch/ConstraintValueSearch';
 import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender';
 import { useThemeStyles } from 'themes/themeStyles';
@@ -9,6 +9,7 @@ import {
     LegalValueLabel,
     filterLegalValues,
 } from '../LegalValueLabel/LegalValueLabel';
+import { getIllegalValues } from '../RestrictiveLegalValues/RestrictiveLegalValues';
 
 interface ISingleLegalValueProps {
     setValue: (value: string) => void;
@@ -17,6 +18,11 @@ interface ISingleLegalValueProps {
     legalValues: ILegalValue[];
     error: string;
     setError: React.Dispatch<React.SetStateAction<string>>;
+    data: {
+        legalValues: ILegalValue[];
+        deletedLegalValues: ILegalValue[];
+    };
+    constraintValue: string;
 }
 
 export const SingleLegalValue = ({
@@ -26,13 +32,38 @@ export const SingleLegalValue = ({
     legalValues,
     error,
     setError,
+    data,
+    constraintValue,
 }: ISingleLegalValueProps) => {
     const [filter, setFilter] = useState('');
     const { classes: styles } = useThemeStyles();
     const filteredValues = filterLegalValues(legalValues, filter);
 
+    const { deletedLegalValues } = data;
+
+    const illegalValues = getIllegalValues(
+        [constraintValue],
+        deletedLegalValues
+    );
+
     return (
         <>
+            <ConditionallyRender
+                condition={Boolean(illegalValues && illegalValues.length > 0)}
+                show={
+                    <Alert
+                        severity="warning"
+                        sx={theme => ({ marginTop: theme.spacing(1) })}
+                    >
+                        {' '}
+                        This constraint is using legal values that have been
+                        deleted as a valid option. Please select a new value
+                        from the remaining predefined legal values. The
+                        constraint will be updated with the new value when you
+                        save the strategy.
+                    </Alert>
+                }
+            />
             <ConstraintFormHeader>
                 Add a single {type.toLowerCase()} value
             </ConstraintFormHeader>

frontend/src/component/segments/SegmentTable/SegmentTable.test.tsx

@@ -1,6 +1,5 @@
 import { render } from '../../../utils/testRenderer';
 import { screen } from '@testing-library/react';
-import React from 'react';
 import { SegmentTable } from './SegmentTable';
 import { testServerRoute, testServerSetup } from '../../../utils/testServer';
 import { UIProviderContainer } from '../../providers/UIProvider/UIProviderContainer';