test(1-3734): test constraint reducer (#9966)

Adds a fairly comprehensive test suite for the constraint reducer. I put
in all cases I thought were relevant.

As part of this, I discovered one bug, and changed two actions.

The bug was toggling on the wrong property when you tried to invert case
sensitivity.

The action changes are:
- rename "remove value from list" to "remove value"
- remove "set value" in favor of instead letting "add value(s)" work in
single-value constraints too.

frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx

@@ -156,7 +156,7 @@ const TopRowInput: FC<{
             <ConstraintDateInput
                 setValue={(value: string) =>
                     updateConstraint({
-                        type: 'set value',
+                        type: 'add value(s)',
                         payload: value,
                     })
                 }
@@ -171,7 +171,7 @@ const TopRowInput: FC<{
                 validator={validator}
                 onAddValue={(newValue) => {
                     updateConstraint({
-                        type: 'set value',
+                        type: 'add value(s)',
                         payload: newValue,
                     });
                 }}
@@ -188,7 +188,7 @@ const TopRowInput: FC<{
                 validator={validator}
                 onAddValue={(newValue) => {
                     updateConstraint({
-                        type: 'set value',
+                        type: 'add value(s)',
                         payload: newValue,
                     });
                 }}
@@ -338,17 +338,10 @@ export const EditableConstraint: FC<Props> = ({
                                   : undefined
                         }
                         removeValue={(value) => {
-                            if (isMultiValueConstraint(localConstraint)) {
+                            updateConstraint({
-                                updateConstraint({
+                                type: 'remove value',
-                                    type: 'remove value from list',
+                                payload: value,
-                                    payload: value,
+                            });
-                                });
-                            } else {
-                                updateConstraint({
-                                    type: 'set value',
-                                    payload: '',
-                                });
-                            }
                         }}
                         getExternalFocusTarget={() =>
                             addValuesButtonRef.current ??
@@ -396,7 +389,7 @@ export const EditableConstraint: FC<Props> = ({
                             }
                             removeValue={(value) =>
                                 updateConstraint({
-                                    type: 'remove value from list',
+                                    type: 'remove value',
                                     payload: value,
                                 })
                             }
@@ -412,7 +405,7 @@ export const EditableConstraint: FC<Props> = ({
                             }
                             addValue={(newValues) =>
                                 updateConstraint({
-                                    type: 'set value',
+                                    type: 'add value(s)',
                                     payload: newValues,
                                 })
                             }

frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.test.ts

@@ -0,0 +1,440 @@
+import {
+    allOperators,
+    IN,
+    isDateOperator,
+    isMultiValueOperator,
+    isSingleValueOperator,
+    NOT_IN,
+    NUM_EQ,
+} from 'constants/operators';
+import type {
+    EditableConstraint,
+    EditableDateConstraint,
+    EditableMultiValueConstraint,
+    EditableSingleValueConstraint,
+} from './editable-constraint-type';
+import { DATE_AFTER, DATE_BEFORE } from '@server/util/constants';
+import { constraintReducer } from './constraint-reducer';
+import { CURRENT_TIME_CONTEXT_FIELD } from 'utils/operatorsForContext';
+
+const extraConstraintFields: Partial<EditableConstraint> = {
+    inverted: true,
+    caseInsensitive: true,
+};
+
+const multiValueConstraint = (
+    contextField: string = 'multi-value-context-field',
+): EditableMultiValueConstraint => ({
+    ...extraConstraintFields,
+    contextName: contextField,
+    operator: NOT_IN,
+    values: new Set(['A', 'B']),
+});
+
+const singleValueConstraint = (
+    contextField: string = 'single-value-context-field',
+): EditableSingleValueConstraint => ({
+    ...extraConstraintFields,
+    contextName: contextField,
+    operator: NUM_EQ,
+    value: '5',
+});
+
+const dateConstraint = (
+    contextField: string = CURRENT_TIME_CONTEXT_FIELD,
+): EditableDateConstraint => ({
+    ...extraConstraintFields,
+    contextName: contextField,
+    operator: DATE_AFTER,
+    value: '2024-05-05T00:00:00Z',
+});
+
+const getConstraintForOperator = (
+    operator: string,
+    contextField?: string,
+): EditableConstraint => {
+    if (isDateOperator(operator)) {
+        return { ...dateConstraint(contextField), operator };
+    }
+    if (isSingleValueOperator(operator)) {
+        return { ...singleValueConstraint(contextField), operator };
+    }
+    if (isMultiValueOperator(operator)) {
+        return { ...multiValueConstraint(contextField), operator };
+    }
+    return { ...multiValueConstraint(contextField), operator: IN };
+};
+
+// helper type to allow deconstruction to { value, values, ...rest }
+type Extractable = EditableConstraint & {
+    value?: string;
+    values?: Set<string>;
+};
+
+describe('changing context field', () => {
+    test.each([
+        ['multi-value', multiValueConstraint],
+        ['single-value', singleValueConstraint],
+        ['date', dateConstraint],
+    ])(
+        'changing context field to the same field is a no-op for %s constraints',
+        (_, constraint) => {
+            const input = constraint();
+            expect(
+                constraintReducer(input, {
+                    type: 'set context field',
+                    payload: input.contextName,
+                }),
+            ).toStrictEqual(input);
+        },
+    );
+    test('changing context field for a single-value constraint clears the `value` prop', () => {
+        const input = singleValueConstraint('field-a');
+        const result = constraintReducer(input, {
+            type: 'set context field',
+            payload: 'field-b',
+        });
+        expect(result).toStrictEqual({
+            ...input,
+            contextName: 'field-b',
+            value: '',
+        });
+    });
+
+    test('changing context field for a multi-value constraint clears the `values` prop', () => {
+        const input = multiValueConstraint('field-a');
+        const result = constraintReducer(input, {
+            type: 'set context field',
+            payload: 'field-b',
+        });
+        expect(result).toStrictEqual({
+            ...input,
+            contextName: 'field-b',
+            values: new Set(),
+        });
+    });
+
+    test.each([
+        ['multi-value', multiValueConstraint],
+        ['single-value', singleValueConstraint],
+    ])(
+        '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 { value, ...result } = constraintReducer(input, {
+                type: 'set context field',
+                payload: 'currentTime',
+            }) as Extractable;
+            const {
+                values: _vs,
+                value: _v,
+                ...inputBody
+            } = input as Extractable;
+            expect(result).toStrictEqual({
+                ...inputBody,
+                contextName: 'currentTime',
+                operator: DATE_AFTER,
+            });
+            expect(new Date(value as string).getTime()).toBeGreaterThanOrEqual(
+                now.getTime(),
+            );
+        },
+    );
+    test('changing context field from currentTime to something else sets a default operator', () => {
+        const input = dateConstraint();
+        const result = constraintReducer(input, {
+            type: 'set context field',
+            payload: 'somethingElse',
+        });
+        const { value: _, ...inputBody } = input;
+        expect(result).toStrictEqual({
+            ...inputBody,
+            contextName: 'somethingElse',
+            operator: IN,
+            values: new Set(),
+        });
+    });
+});
+describe('changing operator', () => {
+    test.each(allOperators)(
+        'changing operator to the same operator (%s) is a no-op',
+        (operator) => {
+            const constraint = getConstraintForOperator(operator);
+            expect(
+                constraintReducer(constraint, {
+                    type: 'set operator',
+                    payload: operator,
+                }),
+            ).toStrictEqual(constraint);
+        },
+    );
+
+    const nonDateOperators = allOperators.filter((op) => !isDateOperator(op));
+    const allCombinations = nonDateOperators
+        .flatMap((a) => nonDateOperators.map((b) => [a, b]))
+        .filter(([a, b]) => a !== b);
+
+    test.each(allCombinations)(
+        "changing the operator to anything that isn't date based clears the value: %s -> %s",
+        (operatorA, operatorB) => {
+            const constraint = getConstraintForOperator(operatorA);
+            const { value, values, ...result } = constraintReducer(constraint, {
+                type: 'set operator',
+                payload: operatorB,
+            }) as Extractable;
+            const {
+                value: _v,
+                values: _values,
+                ...inputConstraint
+            } = constraint as Extractable;
+            expect(result).toStrictEqual({
+                ...inputConstraint,
+                operator: operatorB,
+            });
+
+            if (isMultiValueOperator(operatorB)) {
+                expect(values).toStrictEqual(new Set());
+            } else if (isSingleValueOperator(operatorB)) {
+                expect(value).toBe('');
+            }
+        },
+    );
+
+    const dateTransititons = [
+        [DATE_BEFORE, DATE_AFTER],
+        [DATE_AFTER, DATE_BEFORE],
+    ] as const;
+    test.each(dateTransititons)(
+        'changing the operator from one date operator to another date operator leaves the value untouched: %s -> %s',
+        (operatorA, operatorB) => {
+            const input: EditableDateConstraint = {
+                ...dateConstraint(),
+                operator: operatorA,
+            };
+            const output = constraintReducer(input, {
+                type: 'set operator',
+                payload: operatorB,
+            });
+            expect(input.value).toBe(
+                (output as EditableSingleValueConstraint).value,
+            );
+        },
+    );
+});
+describe('adding values', () => {
+    describe('single-value constraints', () => {
+        test('adding a value replaces the existing value', () => {
+            const input = singleValueConstraint();
+            const output = constraintReducer(input, {
+                type: 'add value(s)',
+                payload: 'new-value',
+            });
+            expect(output).toStrictEqual({
+                ...input,
+                value: 'new-value',
+            });
+        });
+        test('adding a list replaces the existing value with the first value of the list', () => {
+            const input = singleValueConstraint();
+            const output = constraintReducer(input, {
+                type: 'add value(s)',
+                payload: ['list-value'],
+            });
+            expect(output).toStrictEqual({
+                ...input,
+                value: 'list-value',
+            });
+        });
+        test('adding an empty list effectively clears the value', () => {
+            const input = singleValueConstraint();
+            const output = constraintReducer(input, {
+                type: 'add value(s)',
+                payload: [],
+            });
+            expect(output).toStrictEqual({
+                ...input,
+                value: '',
+            });
+        });
+
+        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']),
+            );
+            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]),
+            );
+            expect(output).toStrictEqual({
+                ...input,
+                value: '',
+            });
+        });
+    });
+    describe('multi-value constraints', () => {
+        test('adding a single value to a multi-value constraint adds it to the set', () => {
+            const input = multiValueConstraint();
+            const output = constraintReducer(input, {
+                type: 'add value(s)',
+                payload: 'new-value',
+            });
+            expect(output).toStrictEqual({
+                ...input,
+                values: new Set([...input.values, 'new-value']),
+            });
+        });
+        test('adding a list to a multi-value constraint adds all new elements to the set', () => {
+            const input = multiValueConstraint();
+            const output = constraintReducer(input, {
+                type: 'add value(s)',
+                payload: ['X', 'Y'],
+            });
+            expect(output).toStrictEqual({
+                ...input,
+                values: new Set([...input.values, 'X', 'Y']),
+            });
+        });
+        test('adding an empty list to a multi-value constraint has no effect', () => {
+            const input = multiValueConstraint();
+            const output = constraintReducer(input, {
+                type: 'add value(s)',
+                payload: [],
+            });
+            expect(output).toStrictEqual(input);
+        });
+
+        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: 'add value(s)',
+                    payload: ['deleted-new', 'B'],
+                },
+                new Set(['deleted-old', 'deleted-new']),
+            );
+            expect(output).toStrictEqual({
+                ...input,
+                values: new Set(['A', 'B']),
+            });
+        });
+    });
+});
+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 noChange = constraintReducer(input, {
+                type: 'remove value',
+                payload: '55422b90-9bc4-4847-8a61-17fc928069ff',
+            });
+            expect(noChange).toStrictEqual(input);
+
+            const removed = constraintReducer(input, {
+                type: 'remove value',
+                payload: input.value,
+            });
+
+            expect(removed).toStrictEqual({ ...input, value: '' });
+        });
+        test('clearing a value removes the existing value', () => {
+            const input = singleValueConstraint('context-field');
+            const cleared = constraintReducer(input, {
+                type: 'clear values',
+            });
+
+            expect(cleared).toStrictEqual({ ...input, value: '' });
+        });
+    });
+    describe('multi-value constraints', () => {
+        test('removing a value removes it from the set if it exists', () => {
+            const values = ['A', 'B', 'C'];
+            const input = {
+                ...multiValueConstraint(),
+                values: new Set(values),
+            };
+            const noChange = constraintReducer(input, {
+                type: 'remove value',
+                payload: '55422b90-9bc4-4847-8a61-17fc928069ff',
+            });
+            expect(noChange).toStrictEqual(input);
+
+            const removed = constraintReducer(input, {
+                type: 'remove value',
+                payload: 'B',
+            });
+
+            expect(removed).toStrictEqual({
+                ...input,
+                values: new Set(['A', 'C']),
+            });
+        });
+        test('clearing values removes all values from the set', () => {
+            const input = multiValueConstraint();
+            const cleared = constraintReducer(input, {
+                type: 'clear values',
+            });
+
+            expect(cleared).toStrictEqual({
+                ...input,
+                values: new Set(),
+            });
+        });
+    });
+});
+describe('toggle options', () => {
+    const stateTransitions = [
+        [undefined, true],
+        [true, false],
+        [false, true],
+    ];
+    test.each(stateTransitions)(
+        'toggle case sensitivity: %s -> %s',
+        (from, to) => {
+            const input = {
+                ...multiValueConstraint(),
+                caseInsensitive: from,
+            };
+            expect(
+                constraintReducer(input, {
+                    type: 'toggle case sensitivity',
+                }),
+            ).toStrictEqual({
+                ...input,
+                caseInsensitive: to,
+            });
+        },
+    );
+    test.each(stateTransitions)('match inversion: %s -> %s', (from, to) => {
+        const input = {
+            ...multiValueConstraint(),
+            inverted: from,
+        };
+        expect(
+            constraintReducer(input, {
+                type: 'toggle inverted operator',
+            }),
+        ).toStrictEqual({
+            ...input,
+            inverted: to,
+        });
+    });
+});

frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.ts

@@ -3,53 +3,46 @@ import {
     IN,
     type Operator,
     isDateOperator,
+    isMultiValueOperator,
     isSingleValueOperator,
 } from 'constants/operators';
 import { CURRENT_TIME_CONTEXT_FIELD } from 'utils/operatorsForContext';
 import {
+    type EditableDateConstraint,
     isDateConstraint,
-    isMultiValueConstraint,
     isSingleValueConstraint,
     type EditableConstraint,
+    type EditableMultiValueConstraint,
+    type EditableSingleValueConstraint,
 } from './editable-constraint-type';
 import { difference, union } from './set-functions';
 
 export type ConstraintUpdateAction =
-    | { type: 'add value(s)'; payload: string[] }
+    | { type: 'add value(s)'; payload: string | string[] }
-    | { type: 'set value'; payload: string }
     | { type: 'clear values' }
-    | { type: 'remove value from list'; payload: string }
+    | { type: 'remove value'; payload: string }
     | { type: 'set context field'; payload: string }
     | { type: 'set operator'; payload: Operator }
     | { type: 'toggle case sensitivity' }
     | { type: 'toggle inverted operator' };
 
-const resetValues = (state: EditableConstraint): EditableConstraint => {
+const withValue = <
-    if (isSingleValueConstraint(state)) {
+    T extends EditableConstraint & { value?: string; values?: Set<string> },
-        if ('values' in state) {
+>(
-            const { values, ...rest } = state;
+    newValue: string | null,
-            return {
+    constraint: T,
-                ...rest,
+): EditableConstraint => {
-                value: '',
+    const { value, values, ...rest } = constraint;
-            };
+    if (isMultiValueOperator(constraint.operator)) {
-        }
-        return {
-            ...state,
-            value: '',
-        };
-    }
-
-    if ('value' in state) {
-        const { value, ...rest } = state;
         return {
             ...rest,
-            values: new Set(),
+            values: new Set([newValue].filter(Boolean)),
-        };
+        } as EditableConstraint;
     }
     return {
-        ...state,
+        ...rest,
-        values: new Set(),
+        value: newValue ?? '',
-    };
+    } as EditableConstraint;
 };
 
 export const constraintReducer = (
@@ -59,33 +52,37 @@ export const constraintReducer = (
 ): EditableConstraint => {
     switch (action.type) {
         case 'set context field':
+            if (action.payload === state.contextName) {
+                return state;
+            }
             if (
                 action.payload === CURRENT_TIME_CONTEXT_FIELD &&
                 !isDateOperator(state.operator)
             ) {
-                return {
+                return withValue(new Date().toISOString(), {
                     ...state,
                     contextName: action.payload,
                     operator: DATE_AFTER,
-                    value: new Date().toISOString(),
+                } as EditableDateConstraint);
-                };
             } else if (
                 action.payload !== CURRENT_TIME_CONTEXT_FIELD &&
                 isDateOperator(state.operator)
             ) {
-                return {
+                return withValue(null, {
                     ...state,
                     operator: IN,
                     contextName: action.payload,
-                    values: new Set(),
+                } as EditableMultiValueConstraint);
-                };
             }
 
-            return resetValues({
+            return withValue(null, {
                 ...state,
                 contextName: action.payload,
             });
         case 'set operator':
+            if (action.payload === state.operator) {
+                return state;
+            }
             if (isDateConstraint(state) && isDateOperator(action.payload)) {
                 return {
                     ...state,
@@ -94,23 +91,40 @@ export const constraintReducer = (
             }
 
             if (isSingleValueOperator(action.payload)) {
-                return resetValues({
+                return withValue(null, {
                     ...state,
-                    value: '',
                     operator: action.payload,
-                });
+                } as EditableSingleValueConstraint);
             }
-            return resetValues({
+            return withValue(null, {
                 ...state,
-                values: new Set(),
                 operator: action.payload,
-            });
+            } as EditableMultiValueConstraint);
         case 'add value(s)': {
-            if (!('values' in state)) {
+            if (isSingleValueConstraint(state)) {
-                return 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(action.payload);
+            const newValues = new Set(
+                Array.isArray(action.payload)
+                    ? action.payload
+                    : [action.payload],
+            );
             const combinedValues = union(state.values, newValues);
             const filteredValues = deletedLegalValues
                 ? difference(combinedValues, deletedLegalValues)
@@ -120,18 +134,20 @@ export const constraintReducer = (
                 values: filteredValues,
             };
         }
-        case 'set value':
-            if (isMultiValueConstraint(state)) {
-                return state;
-            }
-            return { ...state, value: action.payload };
         case 'toggle inverted operator':
             return { ...state, inverted: !state.inverted };
         case 'toggle case sensitivity':
-            return { ...state, caseInsensitive: !state.inverted };
+            return { ...state, caseInsensitive: !state.caseInsensitive };
-        case 'remove value from list':
+        case 'remove value':
             if (isSingleValueConstraint(state)) {
-                return state;
+                if (state.value === action.payload) {
+                    return {
+                        ...state,
+                        value: '',
+                    };
+                } else {
+                    return state;
+                }
             }
             state.values.delete(action.payload);
             return {
@@ -139,6 +155,6 @@ export const constraintReducer = (
                 values: state.values ?? new Set(),
             };
         case 'clear values':
-            return resetValues(state);
+            return withValue(null, state);
     }
 };