From f220f216d6921bc57997bfa3df8eb23980cf8b7e Mon Sep 17 00:00:00 2001 From: Fredrik Strand Oseberg Date: Fri, 25 Aug 2023 10:44:00 +0200 Subject: [PATCH] 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: Skjermbilde 2023-08-25 kl 08 56 02 Also amended this to apply to the single legal value constraints. Skjermbilde 2023-08-25 kl 08 57 40 --- .../ConstraintAccordionEdit.tsx | 2 + .../ResolveInput/ResolveInput.tsx | 24 ++++- .../RestrictiveLegalValues.test.tsx | 94 +++++++++---------- .../RestrictiveLegalValues.tsx | 61 ++++++++++-- .../SingleLegalValue.test.tsx | 27 ++++++ .../SingleLegalValue/SingleLegalValue.tsx | 33 ++++++- .../SegmentTable/SegmentTable.test.tsx | 1 - 7 files changed, 182 insertions(+), 60 deletions(-) create mode 100644 frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/SingleLegalValue/SingleLegalValue.test.tsx diff --git a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEdit.tsx b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEdit.tsx index 67aac897a9..57c50e2ff9 100644 --- a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEdit.tsx +++ b/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} diff --git a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/ResolveInput/ResolveInput.tsx b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/ResolveInput/ResolveInput.tsx index 0ebcd28930..c0d0d45cf8 100644 --- a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/ResolveInput/ResolveInput.tsx +++ b/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>; @@ -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 = ({ <> { - it('should show deleted legal values as disabled', async () => { - const value = 'some-value'; - const values = [{ value }]; - const legalValues = [{ value: 'some-other-value' }]; +test('should show alert when you have illegal legal values', async () => { + const contextDefinitionValues = [{ value: 'value1' }, { value: 'value2' }]; + const fixedValues = ['value1', 'value2']; + const localValues = ['value1', 'value2']; + const deletedLegalValues = [{ value: 'value1' }]; - render( - - ); + render( + {}} + error={''} + setError={() => {}} + /> + ); - const input = await screen.findByDisplayValue('some-value'); - - expect(input).toBeInTheDocument(); - expect(input).toHaveProperty('disabled', true); - - expect( - await screen.findByDisplayValue('some-other-value') - ).toBeInTheDocument(); - }); - - it('should remove deleted legal values when editing values', async () => { - const value = 'some-value'; - const deletedLegalValues = [{ value }]; - const legalValues = [ - { value: 'some-other-value' }, - { value: 'value2' }, - ]; - const setValues = vi.fn(); - render( - - ); - const btn = await screen.findByDisplayValue('some-other-value'); - btn.click(); - - expect(setValues).toHaveBeenCalledWith(['value2', 'some-other-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:' + ); +}); + +test('Should remove illegal legal values from internal value state when mounting', () => { + const contextDefinitionValues = [{ value: 'value1' }, { value: 'value2' }]; + const fixedValues = ['value1', 'value2']; + let localValues = ['value1', 'value2']; + const deletedLegalValues = [{ value: 'value1' }]; + + const setValues = (values: string[]) => { + localValues = values; + }; + + render( + {}} + /> + ); + + expect(localValues).toEqual(['value2']); }); diff --git a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/RestrictiveLegalValues/RestrictiveLegalValues.tsx b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/RestrictiveLegalValues/RestrictiveLegalValues.tsx index ae3ae9e258..572a5af2de 100644 --- a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/RestrictiveLegalValues/RestrictiveLegalValues.tsx +++ b/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( - deletedLegalValues.map(({ value }) => value) - ); + const deletedValuesSet = getLegalValueSet(deletedLegalValues); 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 ( <> + 0)} + show={ + + 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: +
    + {illegalValues?.map(value => ( +
  • {value}
    • + ))} +
+ + } + /> + Select values from a predefined set @@ -92,7 +134,7 @@ export const RestrictiveLegalValues = ({ /> } /> - {filteredValues.concat(deletedLegalValues).map(match => ( + {filteredValues.map(match => ( ))} + {error}

} diff --git a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/SingleLegalValue/SingleLegalValue.test.tsx b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/SingleLegalValue/SingleLegalValue.test.tsx new file mode 100644 index 0000000000..503a110476 --- /dev/null +++ b/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( + {}} + 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.' + ); +}); diff --git a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/SingleLegalValue/SingleLegalValue.tsx b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/SingleLegalValue/SingleLegalValue.tsx index b83db6fa1e..19d8a45e2b 100644 --- a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/SingleLegalValue/SingleLegalValue.tsx +++ b/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>; + 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 ( <> + 0)} + show={ + ({ 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. + + } + /> Add a single {type.toLowerCase()} value diff --git a/frontend/src/component/segments/SegmentTable/SegmentTable.test.tsx b/frontend/src/component/segments/SegmentTable/SegmentTable.test.tsx index fdd3ba0be8..09d3dff4f5 100644 --- a/frontend/src/component/segments/SegmentTable/SegmentTable.test.tsx +++ b/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';