diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx index 35334c8e48..826f2f57ab 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx @@ -2,7 +2,7 @@ import { IconButton, styled } from '@mui/material'; import GeneralSelect from 'component/common/GeneralSelect/GeneralSelect'; import { isStringOperator, type Operator } from 'constants/operators'; import useUnleashContext from 'hooks/api/getters/useUnleashContext/useUnleashContext'; -import { useRef, type FC } from 'react'; +import { useCallback, useRef, type FC } from 'react'; import { operatorsForContext } from 'utils/operatorsForContext'; import { ConstraintOperatorSelect } from './ConstraintOperatorSelect'; import { HtmlTooltip } from 'component/common/HtmlTooltip/HtmlTooltip'; @@ -29,7 +29,6 @@ import { isNumberConstraint, isSemVerConstraint, } from './useEditableConstraint/editable-constraint-type'; -import type { ConstraintUpdateAction } from './useEditableConstraint/constraint-reducer'; import type { ConstraintValidationResult } from './useEditableConstraint/constraint-validator'; const Container = styled('article')(({ theme }) => ({ @@ -146,20 +145,22 @@ const StyledCaseSensitiveIcon = styled(CaseSensitiveIcon)(({ theme }) => ({ })); const TopRowInput: FC<{ + addValues: (value: string | string[]) => void; + clearValues: () => void; localConstraint: EditableConstraintType; - updateConstraint: (action: ConstraintUpdateAction) => void; validator: (value: string) => ConstraintValidationResult; addValuesButtonRef: React.RefObject; -}> = ({ localConstraint, updateConstraint, validator, addValuesButtonRef }) => { +}> = ({ + addValues, + clearValues, + localConstraint, + validator, + addValuesButtonRef, +}) => { if (isDateConstraint(localConstraint)) { return ( - updateConstraint({ - type: 'add value(s)', - payload: value, - }) - } + setValue={addValues} value={localConstraint.value} validator={validator} /> @@ -169,13 +170,8 @@ const TopRowInput: FC<{ return ( { - updateConstraint({ - type: 'add value(s)', - payload: newValue, - }); - }} - removeValue={() => updateConstraint({ type: 'clear values' })} + onAddValue={addValues} + removeValue={clearValues} currentValue={localConstraint.value} helpText={'A semver value should be of the format X.Y.Z'} inputType={'text'} @@ -186,13 +182,8 @@ const TopRowInput: FC<{ return ( { - updateConstraint({ - type: 'add value(s)', - payload: newValue, - }); - }} - removeValue={() => updateConstraint({ type: 'clear values' })} + onAddValue={addValues} + removeValue={clearValues} currentValue={localConstraint.value} helpText={'Add a single number'} inputType={'number'} @@ -205,12 +196,7 @@ const TopRowInput: FC<{ validator={validator} helpText='Maximum 100 char length per value' ref={addValuesButtonRef} - onAddValues={(newValues) => { - updateConstraint({ - type: 'add value(s)', - payload: newValues, - }); - }} + onAddValues={addValues} /> ); }; @@ -232,6 +218,31 @@ export const EditableConstraint: FC = ({ validator, legalValueData, } = useEditableConstraint(constraint, onUpdate); + const addValues = useCallback( + (value: string | string[]) => + updateConstraint({ type: 'add value(s)', payload: value }), + [updateConstraint], + ); + const removeValue = useCallback( + (value: string) => + updateConstraint({ type: 'remove value', payload: value }), + [updateConstraint], + ); + const clearAll = useCallback( + () => updateConstraint({ type: 'clear values' }), + [updateConstraint], + ); + const toggleValue = useCallback( + (value: string) => + updateConstraint({ type: 'toggle value', payload: value }), + [updateConstraint], + ); + const onOperatorChange = useCallback( + (operator: Operator) => { + updateConstraint({ type: 'set operator', payload: operator }); + }, + [updateConstraint], + ); const { context } = useUnleashContext(); const { contextName, operator } = localConstraint; @@ -247,10 +258,6 @@ export const EditableConstraint: FC = ({ return { key: context.name, label: context.name }; }); - const onOperatorChange = (operator: Operator) => { - updateConstraint({ type: 'set operator', payload: operator }); - }; - return ( @@ -334,12 +341,7 @@ export const EditableConstraint: FC = ({ ? [localConstraint.value] : undefined } - removeValue={(value) => { - updateConstraint({ - type: 'remove value', - payload: value, - }); - }} + removeValue={removeValue} getExternalFocusTarget={() => addValuesButtonRef.current ?? deleteButtonRef.current @@ -348,7 +350,8 @@ export const EditableConstraint: FC = ({ {legalValueData ? null : ( @@ -373,39 +376,15 @@ export const EditableConstraint: FC = ({ {isMultiValueConstraint(localConstraint) ? ( - updateConstraint({ - type: 'clear values', - }) - } - addValues={(newValues) => - updateConstraint({ - type: 'add value(s)', - payload: newValues, - }) - } - removeValue={(value) => - updateConstraint({ - type: 'remove value', - payload: value, - }) - } + toggleValue={toggleValue} + clearAll={clearAll} + addValues={addValues} {...legalValueData} /> ) : ( - updateConstraint({ - type: 'clear values', - }) - } - addValue={(newValues) => - updateConstraint({ - type: 'add value(s)', - payload: newValues, - }) - } {...legalValueData} /> )} diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/LegalValuesSelector.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/LegalValuesSelector.tsx index 9bdc15972b..df0c324ba1 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/LegalValuesSelector.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/LegalValuesSelector.tsx @@ -1,11 +1,19 @@ import { type FC, useId, useState } from 'react'; -import { Alert, Button, Checkbox, Radio, styled } from '@mui/material'; +import { + Alert, + Button, + Checkbox, + type CheckboxProps, + Radio, + styled, +} from '@mui/material'; import { filterLegalValues, LegalValueLabel, } from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/LegalValueLabel/LegalValueLabel'; import { ConstraintValueSearch } from './ConstraintValueSearch'; import type { ILegalValue } from 'interfaces/context'; +import React from 'react'; const StyledValuesContainer = styled('div')(({ theme }) => ({ display: 'grid', @@ -129,19 +137,15 @@ const BaseLegalValueSelector: FC = ({ role={multiSelect ? undefined : 'radiogroup'} > {filteredValues.map((match) => ( - onChange(match.value)} - disabled={invalidLegalValues?.has(match.value)} - /> - } + Control={Control} + onChange={onChange} + groupNameId={groupNameId} + checked={isInputChecked(match.value)} + disabled={invalidLegalValues?.has(match.value)} /> ))} @@ -149,10 +153,44 @@ const BaseLegalValueSelector: FC = ({ ); }; +const MemoLabel = React.memo(function MemoLabel({ + legal, + filter, + Control, + onChange, + groupNameId, + checked, + disabled, +}: { + legal: ILegalValue; + filter: string; + Control: (props: CheckboxProps) => JSX.Element; + onChange: (value: string) => void; + checked: boolean; + groupNameId: string; + disabled?: boolean; +}) { + return ( + onChange(legal.value)} + disabled={disabled} + /> + } + /> + ); +}); + type LegalValuesSelectorProps = { values: Set; - addValues: (values: string[]) => void; - removeValue: (value: string) => void; + addValues: (value: string | string[]) => void; + toggleValue: (value: string) => void; clearAll: () => void; deletedLegalValues?: Set; invalidLegalValues?: Set; @@ -163,23 +201,15 @@ export const LegalValuesSelector = ({ legalValues, values, addValues, - removeValue, + toggleValue, clearAll, ...baseProps }: LegalValuesSelectorProps) => { - const onChange = (legalValue: string) => { - if (values.has(legalValue)) { - removeValue(legalValue); - } else { - addValues([legalValue]); - } - }; - return ( values.has(inputValue)} - onChange={onChange} + onChange={toggleValue} multiSelect={{ clearAll, selectAll: () => { @@ -194,30 +224,20 @@ export const LegalValuesSelector = ({ type SingleLegalValueSelectorProps = { value: string; - addValue: (value: string) => void; - clear: () => void; deletedLegalValues?: Set; legalValues: ILegalValue[]; + toggleValue: (value: string) => void; invalidLegalValues?: Set; }; export const SingleLegalValueSelector = ({ value, - addValue, - clear, + toggleValue, ...baseProps }: SingleLegalValueSelectorProps) => { - const onChange = (newValue: string) => { - if (value === newValue) { - clear(); - } else { - addValue(newValue); - } - }; - return ( inputValue === value} {...baseProps} /> diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.test.ts b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.test.ts index 80198261b8..3ce1678b09 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.test.ts +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.test.ts @@ -338,6 +338,106 @@ describe('adding values', () => { }); }); }); + +describe('toggling values', () => { + describe('single-value constraints', () => { + test('if the toggle value is the same as the existing value: clears the value', () => { + const input = singleValueConstraint(); + const output = constraintReducer(input, { + type: 'toggle value', + payload: input.value, + }); + expect(output).toStrictEqual({ + ...input, + value: '', + }); + }); + test('if the toggle value is different from the existing value: replaces it', () => { + const input = singleValueConstraint(); + const output = constraintReducer(input, { + type: 'toggle value', + payload: 'updated value', + }); + expect(output).toStrictEqual({ + ...input, + value: 'updated value', + }); + }); + + test('trying to add a deleted legal value results in no change', () => { + const input = singleValueConstraint(); + const output = constraintReducer( + input, + { + type: 'toggle value', + payload: 'deleted', + }, + new Set(['deleted']), + ); + expect(output).toStrictEqual(input); + }); + + test('if both the new value and the old value are deleted legal values, it clears the field', () => { + const input = singleValueConstraint(); + const output = constraintReducer( + input, + { + type: 'toggle value', + payload: 'deleted', + }, + new Set(['deleted', input.value]), + ); + expect(output).toStrictEqual({ + ...input, + value: '', + }); + }); + }); + describe('multi-value constraints', () => { + test('if not present, it will be added', () => { + const input = multiValueConstraint(); + const output = constraintReducer(input, { + type: 'toggle value', + payload: 'new-value', + }); + expect(output).toStrictEqual({ + ...input, + values: new Set([...input.values, 'new-value']), + }); + }); + test('if it is present, it will be removed', () => { + const input = multiValueConstraint(); + const output = constraintReducer(input, { + type: 'toggle value', + payload: 'B', + }); + expect(output).toStrictEqual({ + ...input, + values: new Set(['A']), + }); + }); + + test('deleted legal values are removed from the set before saving new values', () => { + const input = { + ...multiValueConstraint(), + values: new Set(['deleted-old', 'A']), + }; + const output = constraintReducer( + input, + { + type: 'toggle value', + payload: 'deleted-new', + }, + new Set(['deleted-old', 'deleted-new']), + ); + expect(output).toStrictEqual({ + ...input, + values: new Set(['A']), + }); + }); + }); +}); + describe('removing / clearing values', () => { describe('single-value constraints', () => { test('removing a value removes the existing value if it matches', () => { diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.ts b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.ts index 034e96f063..1149ae49cf 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.ts +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.ts @@ -14,6 +14,7 @@ import { type EditableConstraint, type EditableMultiValueConstraint, type EditableSingleValueConstraint, + isMultiValueConstraint, } from './editable-constraint-type'; import { difference, union } from './set-functions'; @@ -24,7 +25,8 @@ export type ConstraintUpdateAction = | { type: 'set context field'; payload: string } | { type: 'set operator'; payload: Operator } | { type: 'toggle case sensitivity' } - | { type: 'toggle inverted operator' }; + | { type: 'toggle inverted operator' } + | { type: 'toggle value'; payload: string }; const withValue = < T extends EditableConstraint & { value?: string; values?: Set }, @@ -50,6 +52,53 @@ export const constraintReducer = ( action: ConstraintUpdateAction, deletedLegalValues?: Set, ): EditableConstraint => { + const removeValue = (value: string) => { + if (isSingleValueConstraint(state)) { + if (state.value === value) { + return { + ...state, + value: '', + }; + } else { + return state; + } + } + const updatedValues = difference(state.values, new Set([value])); + return { + ...state, + values: updatedValues ?? new Set(), + }; + }; + + const addValue = (value: string | string[]) => { + if (isSingleValueConstraint(state)) { + const newValue = Array.isArray(value) ? value[0] : value; + if (deletedLegalValues?.has(newValue)) { + if (deletedLegalValues?.has(state.value)) { + return { + ...state, + value: '', + }; + } + return state; + } + return { + ...state, + value: newValue ?? '', + }; + } + + const newValues = new Set(Array.isArray(value) ? value : [value]); + const combinedValues = union(state.values, newValues); + const filteredValues = deletedLegalValues + ? difference(combinedValues, deletedLegalValues) + : combinedValues; + return { + ...state, + values: filteredValues, + }; + }; + switch (action.type) { case 'set context field': if (action.payload === state.contextName) { @@ -101,60 +150,25 @@ export const constraintReducer = ( operator: action.payload, } as EditableMultiValueConstraint); case 'add value(s)': { - if (isSingleValueConstraint(state)) { - const newValue = Array.isArray(action.payload) - ? action.payload[0] - : action.payload; - if (deletedLegalValues?.has(newValue)) { - if (deletedLegalValues?.has(state.value)) { - return { - ...state, - value: '', - }; - } - return state; - } - return { - ...state, - value: newValue ?? '', - }; - } - - const newValues = new Set( - Array.isArray(action.payload) - ? action.payload - : [action.payload], - ); - const combinedValues = union(state.values, newValues); - const filteredValues = deletedLegalValues - ? difference(combinedValues, deletedLegalValues) - : combinedValues; - return { - ...state, - values: filteredValues, - }; + return addValue(action.payload); } case 'toggle inverted operator': return { ...state, inverted: !state.inverted }; case 'toggle case sensitivity': return { ...state, caseInsensitive: !state.caseInsensitive }; case 'remove value': - if (isSingleValueConstraint(state)) { - if (state.value === action.payload) { - return { - ...state, - value: '', - }; - } else { - return state; - } - } - state.values.delete(action.payload); - return { - ...state, - values: state.values ?? new Set(), - }; + return removeValue(action.payload); case 'clear values': return withValue(null, state); + case 'toggle value': + if ( + (isSingleValueConstraint(state) && + state.value === action.payload) || + (isMultiValueConstraint(state) && + state.values.has(action.payload)) + ) { + return removeValue(action.payload); + } + return addValue(action.payload); } }; diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.test.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.test.tsx index 717dcdcf7b..280b7516cb 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.test.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.test.tsx @@ -20,7 +20,7 @@ const setupApi = (contextField?: ContextFieldSchema) => { testServerRoute(server, '/api/admin/context', [contextField]); }; -test('calls onUpdate with new state', () => { +test('calls onUpdate with new state', async () => { const initial: IConstraint = { contextName: 'context-field', operator: NOT_IN, @@ -32,15 +32,19 @@ test('calls onUpdate with new state', () => { const { result } = renderHook(() => useEditableConstraint(initial, onUpdate), ); + result.current.updateConstraint({ type: 'set operator', payload: IN, }); - expect(onUpdate).toHaveBeenCalledWith({ - contextName: 'context-field', - operator: IN, - values: [], + // gets called by useEffect, so we need to wait for the next render. + await waitFor(() => { + expect(onUpdate).toHaveBeenCalledWith({ + contextName: 'context-field', + operator: IN, + values: [], + }); }); }); diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.tsx index c126bb9b2f..460c1af624 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.tsx @@ -1,4 +1,4 @@ -import { useMemo, useState } from 'react'; +import { useEffect, useMemo, useReducer } from 'react'; import useUnleashContext from 'hooks/api/getters/useUnleashContext/useUnleashContext'; import type { IConstraint } from 'interfaces/strategy'; import { @@ -60,9 +60,13 @@ export const useEditableConstraint = ( constraint: IConstraint, onUpdate: (constraint: IConstraint) => void, ): EditableConstraintState => { - const [localConstraint, setLocalConstraint] = useState(() => { - return fromIConstraint(constraint); - }); + const [localConstraint, updateConstraint] = useReducer( + constraintReducer, + fromIConstraint(constraint), + ); + useEffect(() => { + onUpdate(toIConstraint(localConstraint)); + }, [localConstraint]); const { context } = useUnleashContext(); @@ -105,17 +109,6 @@ export const useEditableConstraint = ( JSON.stringify(localConstraint.operator), ]); - const updateConstraint = (action: ConstraintUpdateAction) => { - const nextState = constraintReducer( - localConstraint, - action, - deletedLegalValues, - ); - - setLocalConstraint(nextState); - onUpdate(toIConstraint(nextState)); - }; - const legalValueData = contextDefinition.legalValues?.length ? { legalValues: contextDefinition.legalValues,