diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx index 42eb97b244..5a4904f08a 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx @@ -156,7 +156,7 @@ const TopRowInput: FC<{ 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 = ({ : undefined } removeValue={(value) => { - if (isMultiValueConstraint(localConstraint)) { - updateConstraint({ - type: 'remove value from list', - payload: value, - }); - } else { - updateConstraint({ - type: 'set value', - payload: '', - }); - } + updateConstraint({ + type: 'remove value', + payload: value, + }); }} getExternalFocusTarget={() => addValuesButtonRef.current ?? @@ -396,7 +389,7 @@ export const EditableConstraint: FC = ({ } removeValue={(value) => updateConstraint({ - type: 'remove value from list', + type: 'remove value', payload: value, }) } @@ -412,7 +405,7 @@ export const EditableConstraint: FC = ({ } addValue={(newValues) => updateConstraint({ - type: 'set value', + type: 'add value(s)', payload: newValues, }) } 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 new file mode 100644 index 0000000000..80198261b8 --- /dev/null +++ b/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 = { + 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; +}; + +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, + }); + }); +}); 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 9e81b15005..034e96f063 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.ts +++ b/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: 'set value'; payload: string } + | { type: 'add value(s)'; payload: string | 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 => { - if (isSingleValueConstraint(state)) { - if ('values' in state) { - const { values, ...rest } = state; - return { - ...rest, - value: '', - }; - } - return { - ...state, - value: '', - }; - } - - if ('value' in state) { - const { value, ...rest } = state; +const withValue = < + T extends EditableConstraint & { value?: string; values?: Set }, +>( + newValue: string | null, + constraint: T, +): EditableConstraint => { + const { value, values, ...rest } = constraint; + if (isMultiValueOperator(constraint.operator)) { return { ...rest, - values: new Set(), - }; + values: new Set([newValue].filter(Boolean)), + } as EditableConstraint; } return { - ...state, - values: new Set(), - }; + ...rest, + 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)) { - return state; + 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(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 }; - case 'remove value from list': + return { ...state, caseInsensitive: !state.caseInsensitive }; + 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); } };