Mirrors/unleash.unleash
1
0
Fork 0
mirror of https://github.com/Unleash/unleash.git synced 2025-06-27 01:19:00 +02:00
Code Issues Projects Releases Wiki Activity

Fix: deleted legal values not being cleared when you select new ones (#9986)

Browse Source 
Updates how we handle deleted legal values for the constraint reducer.
The previous iteration used useState and took the deleted legal values
as a third argument. This isn't possible anymore because a reducer can
take only two args. The simplest way forward for this was to move the
deleted legal values into the state itself, so that it's available in
the reducer. Because deleted legal values can be updated whenever (when
we get a response for that specific context field), we'll update it via
`useEffect`.

I'm not crazy about this approach, so if you have better suggestions,
I'm listening.

I've changed the signature of the reducer, so I've also updated the
tests. In doing so, I thought it now makes more sense to have the base
objects be objects instead of functions, so the changes there are
primarily updating that.
This commit is contained in:
Thomas Heartman 2025-05-15 15:06:08 +02:00 committed by GitHub
parent e09b839ac0
commit ffdf85c8b8
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 137 additions and 283 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
frontend/src/component/common/VariantsSplitPreview/VariantsSplitPreview.tsx
View File

@ -22,7 +22,10 @@ const StyledVariantItem = styled(Box)<{ selected?: boolean }>(
}),
);
const StyledVariantItemTrack = styled(Box)<{
const StyledVariantItemTrack = styled(Box, {
shouldForwardProp: (prop) =>
!['index', 'hasError', 'isFirst', 'isLast'].includes(prop as string),
})<{
index: number;
hasError?: boolean;
isFirst?: boolean;

192
frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/constraint-reducer.test.ts
View File

@ -22,47 +22,38 @@ const extraConstraintFields: Partial<EditableConstraint> = {
caseInsensitive: true,
};
const multiValueConstraint = (
contextField: string = 'multi-value-context-field',
): EditableMultiValueConstraint => ({
const multiValueConstraint: EditableMultiValueConstraint = {
...extraConstraintFields,
contextName: contextField,
contextName: 'multi-value-context-field',
operator: NOT_IN,
values: new Set(['A', 'B']),
});
};
const singleValueConstraint = (
contextField: string = 'single-value-context-field',
): EditableSingleValueConstraint => ({
const singleValueConstraint: EditableSingleValueConstraint = {
...extraConstraintFields,
contextName: contextField,
contextName: 'single-value-context-field',
operator: NUM_EQ,
value: '5',
});
};
const dateConstraint = (
contextField: string = CURRENT_TIME_CONTEXT_FIELD,
): EditableDateConstraint => ({
const dateConstraint: EditableDateConstraint = {
...extraConstraintFields,
contextName: contextField,
contextName: CURRENT_TIME_CONTEXT_FIELD,
operator: DATE_AFTER,
value: '2024-05-05T00:00:00Z',
});
};
const getConstraintForOperator = (
operator: string,
contextField?: string,
): EditableConstraint => {
const getConstraintForOperator = (operator: string): EditableConstraint => {
if (isDateOperator(operator)) {
return { ...dateConstraint(contextField), operator };
return { ...dateConstraint, operator };
}
if (isSingleValueOperator(operator)) {
return { ...singleValueConstraint(contextField), operator };
return { ...singleValueConstraint, operator };
}
if (isMultiValueOperator(operator)) {
return { ...multiValueConstraint(contextField), operator };
return { ...multiValueConstraint, operator };
}
return { ...multiValueConstraint(contextField), operator: IN };
return { ...multiValueConstraint, operator: IN };
};
// helper type to allow deconstruction to { value, values, ...rest }
@ -79,7 +70,7 @@ describe('changing context field', () => {
])(
'changing context field to the same field is a no-op for %s constraints',
(_, constraint) => {
const input = constraint();
const input = constraint;
expect(
constraintReducer(input, {
type: 'set context field',
@ -89,7 +80,7 @@ describe('changing context field', () => {
},
);
test('changing context field for a single-value constraint clears the `value` prop', () => {
const input = singleValueConstraint('field-a');
const input = { ...singleValueConstraint, contextName: 'field-a' };
const result = constraintReducer(input, {
type: 'set context field',
payload: 'field-b',
@ -102,7 +93,7 @@ describe('changing context field', () => {
});
test('changing context field for a multi-value constraint clears the `values` prop', () => {
const input = multiValueConstraint('field-a');
const input = { ...multiValueConstraint, contextName: 'field-a' };
const result = constraintReducer(input, {
type: 'set context field',
payload: 'field-b',
@ -121,7 +112,7 @@ describe('changing context field', () => {
'changing context field to currentTime from a %s constraint sets the current time as the value',
(_, constraint) => {
const now = new Date();
const input = constraint('field-a');
const input = { ...constraint, contextName: 'field-a' };
const { value, ...result } = constraintReducer(input, {
type: 'set context field',
payload: 'currentTime',
@ -142,7 +133,7 @@ describe('changing context field', () => {
},
);
test('changing context field from currentTime to something else sets a default operator', () => {
const input = dateConstraint();
const input = dateConstraint;
const result = constraintReducer(input, {
type: 'set context field',
payload: 'somethingElse',
@ -209,7 +200,7 @@ describe('changing operator', () => {
'changing the operator from one date operator to another date operator leaves the value untouched: %s -> %s',
(operatorA, operatorB) => {
const input: EditableDateConstraint = {
...dateConstraint(),
...dateConstraint,
operator: operatorA,
};
const output = constraintReducer(input, {
@ -225,7 +216,7 @@ describe('changing operator', () => {
describe('adding values', () => {
describe('single-value constraints', () => {
test('adding a value replaces the existing value', () => {
const input = singleValueConstraint();
const input = singleValueConstraint;
const output = constraintReducer(input, {
type: 'add value(s)',
payload: 'new-value',
@ -236,7 +227,7 @@ describe('adding values', () => {
});
});
test('adding a list replaces the existing value with the first value of the list', () => {
const input = singleValueConstraint();
const input = singleValueConstraint;
const output = constraintReducer(input, {
type: 'add value(s)',
payload: ['list-value'],
@ -247,7 +238,7 @@ describe('adding values', () => {
});
});
test('adding an empty list effectively clears the value', () => {
const input = singleValueConstraint();
const input = singleValueConstraint;
const output = constraintReducer(input, {
type: 'add value(s)',
payload: [],
@ -259,27 +250,29 @@ describe('adding values', () => {
});
test('trying to add a deleted legal value results in no change', () => {
const input = singleValueConstraint();
const output = constraintReducer(
input,
{
type: 'add value(s)',
payload: 'deleted',
},
new Set(['deleted']),
);
const input = {
...singleValueConstraint,
deletedLegalValues: new Set(['deleted']),
};
const output = constraintReducer(input, {
type: 'add value(s)',
payload: '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: 'add value(s)',
payload: 'deleted',
},
new Set(['deleted', input.value]),
);
const input = {
...singleValueConstraint,
deletedLegalValues: new Set([
'deleted',
singleValueConstraint.value,
]),
};
const output = constraintReducer(input, {
type: 'add value(s)',
payload: 'deleted',
});
expect(output).toStrictEqual({
...input,
value: '',
@ -288,7 +281,7 @@ describe('adding values', () => {
});
describe('multi-value constraints', () => {
test('adding a single value to a multi-value constraint adds it to the set', () => {
const input = multiValueConstraint();
const input = multiValueConstraint;
const output = constraintReducer(input, {
type: 'add value(s)',
payload: 'new-value',
@ -299,7 +292,7 @@ describe('adding values', () => {
});
});
test('adding a list to a multi-value constraint adds all new elements to the set', () => {
const input = multiValueConstraint();
const input = multiValueConstraint;
const output = constraintReducer(input, {
type: 'add value(s)',
payload: ['X', 'Y'],
@ -310,7 +303,7 @@ describe('adding values', () => {
});
});
test('adding an empty list to a multi-value constraint has no effect', () => {
const input = multiValueConstraint();
const input = multiValueConstraint;
const output = constraintReducer(input, {
type: 'add value(s)',
payload: [],
@ -320,17 +313,14 @@ describe('adding values', () => {
test('deleted legal values are removed from the set before saving new values', () => {
const input = {
...multiValueConstraint(),
...multiValueConstraint,
values: new Set(['deleted-old', 'A']),
deletedLegalValues: new Set(['deleted-old', 'deleted-new']),
};
const output = constraintReducer(
input,
{
type: 'add value(s)',
payload: ['deleted-new', 'B'],
},
new Set(['deleted-old', 'deleted-new']),
);
const output = constraintReducer(input, {
type: 'add value(s)',
payload: ['deleted-new', 'B'],
});
expect(output).toStrictEqual({
...input,
values: new Set(['A', 'B']),
@ -342,7 +332,7 @@ 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 input = singleValueConstraint;
const output = constraintReducer(input, {
type: 'toggle value',
payload: input.value,
@ -353,7 +343,7 @@ describe('toggling values', () => {
});
});
test('if the toggle value is different from the existing value: replaces it', () => {
const input = singleValueConstraint();
const input = singleValueConstraint;
const output = constraintReducer(input, {
type: 'toggle value',
payload: 'updated value',
@ -365,28 +355,29 @@ describe('toggling values', () => {
});
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']),
);
const input = {
...singleValueConstraint,
deletedLegalValues: new Set(['deleted']),
};
const output = constraintReducer(input, {
type: 'toggle value',
payload: '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]),
);
const input = {
...singleValueConstraint,
deletedLegalValues: new Set([
'deleted',
singleValueConstraint.value,
]),
};
const output = constraintReducer(input, {
type: 'toggle value',
payload: 'deleted',
});
expect(output).toStrictEqual({
...input,
value: '',
@ -395,7 +386,7 @@ describe('toggling values', () => {
});
describe('multi-value constraints', () => {
test('if not present, it will be added', () => {
const input = multiValueConstraint();
const input = multiValueConstraint;
const output = constraintReducer(input, {
type: 'toggle value',
payload: 'new-value',
@ -406,7 +397,7 @@ describe('toggling values', () => {
});
});
test('if it is present, it will be removed', () => {
const input = multiValueConstraint();
const input = multiValueConstraint;
const output = constraintReducer(input, {
type: 'toggle value',
payload: 'B',
@ -419,17 +410,14 @@ describe('toggling values', () => {
test('deleted legal values are removed from the set before saving new values', () => {
const input = {
...multiValueConstraint(),
...multiValueConstraint,
values: new Set(['deleted-old', 'A']),
deletedLegalValues: new Set(['deleted-old', 'deleted-new']),
};
const output = constraintReducer(
input,
{
type: 'toggle value',
payload: 'deleted-new',
},
new Set(['deleted-old', 'deleted-new']),
);
const output = constraintReducer(input, {
type: 'toggle value',
payload: 'deleted-new',
});
expect(output).toStrictEqual({
...input,
values: new Set(['A']),
@ -441,7 +429,10 @@ describe('toggling values', () => {
describe('removing / clearing values', () => {
describe('single-value constraints', () => {
test('removing a value removes the existing value if it matches', () => {
const input = singleValueConstraint('context-field');
const input = {
...singleValueConstraint,
contextName: 'context-field',
};
const noChange = constraintReducer(input, {
type: 'remove value',
payload: '55422b90-9bc4-4847-8a61-17fc928069ff',
@ -456,7 +447,10 @@ describe('removing / clearing values', () => {
expect(removed).toStrictEqual({ ...input, value: '' });
});
test('clearing a value removes the existing value', () => {
const input = singleValueConstraint('context-field');
const input = {
...singleValueConstraint,
contextName: 'context-field',
};
const cleared = constraintReducer(input, {
type: 'clear values',
});
@ -468,7 +462,7 @@ describe('removing / clearing values', () => {
test('removing a value removes it from the set if it exists', () => {
const values = ['A', 'B', 'C'];
const input = {
...multiValueConstraint(),
...multiValueConstraint,
values: new Set(values),
};
const noChange = constraintReducer(input, {
@ -488,7 +482,7 @@ describe('removing / clearing values', () => {
});
});
test('clearing values removes all values from the set', () => {
const input = multiValueConstraint();
const input = multiValueConstraint;
const cleared = constraintReducer(input, {
type: 'clear values',
});
@ -510,7 +504,7 @@ describe('toggle options', () => {
'toggle case sensitivity: %s -> %s',
(from, to) => {
const input = {
...multiValueConstraint(),
...multiValueConstraint,
caseInsensitive: from,
};
expect(
@ -525,7 +519,7 @@ describe('toggle options', () => {
);
test.each(stateTransitions)('match inversion: %s -> %s', (from, to) => {
const input = {
...multiValueConstraint(),
...multiValueConstraint,
inverted: from,
};
expect(

30
frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/constraint-reducer.ts
View File

@ -18,6 +18,10 @@ import {
} from './editable-constraint-type.js';
import { difference, union } from './set-functions.js';
type EditableConstraintWithDeletedLegalValues = EditableConstraint & {
deletedLegalValues?: Set<string>;
};
export type ConstraintUpdateAction =
| { type: 'add value(s)'; payload: string | string[] }
| { type: 'clear values' }
@ -26,14 +30,18 @@ export type ConstraintUpdateAction =
| { type: 'set operator'; payload: Operator }
| { type: 'toggle case sensitivity' }
| { type: 'toggle inverted operator' }
| { type: 'deleted legal values update'; payload?: Set<string> }
| { type: 'toggle value'; payload: string };
const withValue = <
T extends EditableConstraint & { value?: string; values?: Set<string> },
T extends EditableConstraintWithDeletedLegalValues & {
value?: string;
values?: Set<string>;
},
>(
newValue: string | null,
constraint: T,
): EditableConstraint => {
): EditableConstraintWithDeletedLegalValues => {
const { value, values, ...rest } = constraint;
if (isMultiValueOperator(constraint.operator)) {
return {
@ -48,10 +56,9 @@ const withValue = <
};
export const constraintReducer = (
state: EditableConstraint,
state: EditableConstraintWithDeletedLegalValues,
action: ConstraintUpdateAction,
deletedLegalValues?: Set<string>,
): EditableConstraint => {
): EditableConstraintWithDeletedLegalValues => {
const removeValue = (value: string) => {
if (isSingleValueConstraint(state)) {
if (state.value === value) {
@ -73,8 +80,8 @@ export const constraintReducer = (
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)) {
if (state.deletedLegalValues?.has(newValue)) {
if (state.deletedLegalValues?.has(state.value)) {
return {
...state,
value: '',
@ -90,8 +97,8 @@ export const constraintReducer = (
const newValues = new Set(Array.isArray(value) ? value : [value]);
const combinedValues = union(state.values, newValues);
const filteredValues = deletedLegalValues
? difference(combinedValues, deletedLegalValues)
const filteredValues = state.deletedLegalValues
? difference(combinedValues, state.deletedLegalValues)
: combinedValues;
return {
...state,
@ -170,5 +177,10 @@ export const constraintReducer = (
return removeValue(action.payload);
}
return addValue(action.payload);
case 'deleted legal values update':
return {
...state,
deletedLegalValues: action.payload,
};
}
};

15
frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/useEditableConstraint.tsx
View File

@ -60,13 +60,14 @@ export const useEditableConstraint = (
constraint: IConstraint,
onUpdate: (constraint: IConstraint) => void,
): EditableConstraintState => {
const [localConstraint, updateConstraint] = useReducer(
const [constraintState, updateConstraint] = useReducer(
constraintReducer,
fromIConstraint(constraint),
);
const { deletedLegalValues, ...localConstraint } = constraintState;
useEffect(() => {
onUpdate(toIConstraint(localConstraint));
}, [localConstraint]);
}, [constraintState]);
const { context } = useUnleashContext();
@ -77,17 +78,21 @@ export const useEditableConstraint = (
const validator = constraintValidator(localConstraint);
const deletedLegalValues = useMemo(() => {
useEffect(() => {
if (
contextDefinition.legalValues?.length &&
constraint.values?.length
) {
return getDeletedLegalValues(
const deletedLegalValues = getDeletedLegalValues(
contextDefinition.legalValues,
constraint.values,
);
updateConstraint({
type: 'deleted legal values update',
payload: deletedLegalValues,
});
}
return undefined;
}, [
JSON.stringify(contextDefinition.legalValues),
JSON.stringify(constraint.values),

178
frontend/src/component/feature/FeatureStrategy/FeatureStrategyCreate/FeatureStrategyCreate.test.tsx
View File

@ -17,6 +17,7 @@ import {
setupUiConfigEndpoint,
setupContextEndpoint,
} from './featureStrategyFormTestSetup.ts';
import userEvent from '@testing-library/user-event';
const featureName = 'my-new-feature';
@ -143,11 +144,15 @@ describe('NewFeatureStrategyCreate', () => {
const targetingEl = screen.getByText('Targeting');
fireEvent.click(targetingEl);
const addConstraintEl = await screen.findByText('Add constraint');
fireEvent.click(addConstraintEl);
const addConstraintEl = await screen.findByRole('button', {
name: 'Add constraint',
});
await userEvent.click(addConstraintEl);
const addValueEl = screen.getByText('Add values');
fireEvent.click(addValueEl);
const addValueEl = await screen.findByRole('button', {
name: 'Add values',
});
await userEvent.click(addValueEl);
const inputElement = screen.getByPlaceholderText('Enter value');
fireEvent.change(inputElement, {
@ -257,171 +262,6 @@ describe('NewFeatureStrategyCreate', () => {
expect(variants2.length).toBe(0);
});
test('Should autosave constraint settings when navigating between tabs', async () => {
const { expectedMultipleValues } = 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);
const addValueEl = screen.getByText('Add values');
fireEvent.click(addValueEl);
const inputElement = screen.getByPlaceholderText('Enter value');
fireEvent.change(inputElement, {
target: { value: expectedMultipleValues },
});
const doneEl = screen.getByText('Add');
fireEvent.click(doneEl);
const variantsEl = screen.getByText('Variants');
fireEvent.click(variantsEl);
fireEvent.click(targetingEl);
const values = expectedMultipleValues.split(',');
expect(screen.getByText(values[0])).toBeInTheDocument();
expect(screen.getByText(values[1])).toBeInTheDocument();
expect(screen.getByText(values[2])).toBeInTheDocument();
});
test.skip('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 addValueEls = await screen.findAllByText('Add values');
// first constraint
fireEvent.click(addValueEls[0]);
await waitFor(() => {
const inputElement = screen.getByPlaceholderText('Enter value');
expect(inputElement).toBeInTheDocument();
});
const firstEnterElement = screen.getByPlaceholderText('Enter value');
fireEvent.change(firstEnterElement, {
target: { value: '123' },
});
const firstAddElement = screen.getByText('Add');
fireEvent.click(firstAddElement);
// second constraint
fireEvent.click(addValueEls[1]);
await waitFor(() => {
const inputElement = screen.getByPlaceholderText('Enter value');
expect(inputElement).toBeInTheDocument();
});
// const secondEnterElement = screen.getByPlaceholderText('Enter value');
const secondEnterElement = screen.getByPlaceholderText('Enter value');
fireEvent.change(secondEnterElement, {
target: { value: '456' },
});
const secondDoneElement = screen.getByText('Add');
fireEvent.click(secondDoneElement);
// third constraint
fireEvent.click(addValueEls[2]);
const thirdEnterElement = screen.getByPlaceholderText('Enter value');
fireEvent.change(thirdEnterElement, {
target: { value: '789' },
});
const thirdDoneElement = screen.getByText('Add');
fireEvent.click(thirdDoneElement);
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.skip('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 addValueEls = await screen.findAllByText('Add values');
// first constraint
fireEvent.click(addValueEls[0]);
const firstEnterElement = screen.getByPlaceholderText('Enter value');
fireEvent.change(firstEnterElement, {
target: { value: '123' },
});
const firstAddElement = screen.getByText('Add');
fireEvent.click(firstAddElement);
// second constraint
fireEvent.click(addValueEls[1]);
screen.debug(undefined, 200000);
const secondEnterElement =
await screen.findByPlaceholderText('Enter value');
fireEvent.change(secondEnterElement, {
target: { value: '456' },
});
const secondDoneElement = screen.getByText('Add');
fireEvent.click(secondDoneElement);
// third constraint
fireEvent.click(addValueEls[2]);
const thirdEnterElement = screen.getByPlaceholderText('Enter value');
fireEvent.change(thirdEnterElement, {
target: { value: '789' },
});
const thirdDoneElement = screen.getByText('Add');
fireEvent.click(thirdDoneElement);
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 remove constraint when no valid values are set and moving between tabs', async () => {
setupComponent();