From e4ead3bd67fa91a936d15ef3e6d5ad39d42036a1 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 9 May 2025 11:47:22 +0200 Subject: [PATCH] Refactor: get rid of editable constraint wrapper (#9921) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This (admittedly pretty big) PR removes a component layer, moves all logic for updating constraint values into a single module, and dumbs down other components. The main changes are: - EditableConstraintWrapper is gone. All the logic in there has been moved into the new `useEditableConstraint` hook. Previously it was split between the wrapper, editableConstraint itself, the legalValues component. - the `useEditableConstraint` hook accepts a constraint and a save function and returns an editable version of that constraint, the validator for input values, a function that accepts update commands, and, when relevant, existing and deleted legal values. - All the logic for updating a constraint now exists in the `constraint-reducer` file. As a pure function, it'll be easy to unit test pretty thoroughly to make sure all commands work as they should (tests will come later) - The legal values selector has been dumbed down consiberably as it no longer needs to create its own internal weak map. The internal representation of selected values is now a set, so any kind of lookup is now constant time, which should remove the need for the extra layer of abstraction. ## Discussion points I know the reducer pattern isn't one we use a *lot* in Unleash, but I found a couple examples of it in the front end and it's also quite similar to how we handle state updates to change request states. I'd be happy to find a different way to represent it if we can keep it in a single, testable interface. Semi-relatedly: I've exposed the actions to submit for the updates at the moment, but we could map these to functions instead. It'd make invocations a little easier (you wouldn't need to specify the action yourself; only use the payload as a function arg if there is one), but we'd end up doing more mapping to create them. I'm not sure it's worth it, but I also don't mind if we do 💁🏼 --- .../ResolveInput/ResolveInput.tsx | 16 +- .../EditableConstraintsList.tsx | 43 +-- .../NewConstraintAccordionList.tsx | 17 +- .../EditableConstraint.tsx | 330 ++++++++---------- .../EditableConstraintWrapper.tsx | 195 ----------- ...FeatureStrategyConstraintAccordionList.tsx | 33 +- .../LegalValuesSelector.tsx | 146 ++------ .../FeatureStrategyConstraints/ValueList.tsx | 7 +- .../constraint-reducer.ts | 144 ++++++++ .../constraint-validator.ts | 34 +- .../editable-constraint-type.ts | 98 ++++++ .../useEditableConstraint/set-functions.ts | 25 ++ .../useEditableConstraint.tsx | 145 ++++++++ .../component/segments/SegmentFormStepTwo.tsx | 44 +-- 14 files changed, 664 insertions(+), 613 deletions(-) delete mode 100644 frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraintWrapper.tsx create mode 100644 frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.ts rename frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/{ => useEditableConstraint}/constraint-validator.ts (63%) create mode 100644 frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/editable-constraint-type.ts create mode 100644 frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/set-functions.ts create mode 100644 frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.tsx diff --git a/frontend/src/component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/ResolveInput/ResolveInput.tsx b/frontend/src/component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/ResolveInput/ResolveInput.tsx index 2f50ec24f1..c22309a957 100644 --- a/frontend/src/component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/ResolveInput/ResolveInput.tsx +++ b/frontend/src/component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/ResolveInput/ResolveInput.tsx @@ -21,8 +21,6 @@ import { type Input, } from '../useConstraintInput/useConstraintInput'; import type React from 'react'; -import { useUiFlag } from 'hooks/useUiFlag'; -import { LegalValuesSelector } from 'component/feature/FeatureStrategy/FeatureStrategyConstraints/LegalValuesSelector'; interface IResolveInputProps { contextDefinition: Pick; @@ -84,23 +82,11 @@ export const ResolveInput = ({ removeValue, error, }: IResolveInputProps) => { - const useNewLegalValueInput = useUiFlag('addEditStrategy'); const resolveInput = () => { switch (input) { case IN_OPERATORS_LEGAL_VALUES: case STRING_OPERATORS_LEGAL_VALUES: - return useNewLegalValueInput ? ( - - ) : ( + return ( void; } export interface IEditableConstraintsListProps { constraints: IConstraint[]; - setConstraints?: React.Dispatch>; + setConstraints: React.Dispatch>; } const StyledContainer = styled('div')({ @@ -42,29 +41,16 @@ export const EditableConstraintsList = forwardRef< }, })); - const onRemove = - setConstraints && - ((index: number) => { - setConstraints( - produce((draft) => { - draft.splice(index, 1); - }), - ); - }); - - const onSave = - setConstraints && - ((index: number, constraint: IConstraint) => { - setConstraints( - produce((draft) => { - draft[index] = constraint; - }), - ); - }); + const onDelete = (index: number) => { + setConstraints( + produce((draft) => { + draft.splice(index, 1); + }), + ); + }; const onAutoSave = - setConstraints && - ((id: string | undefined) => (constraint: IConstraint) => { + (id: string | undefined) => (constraint: IConstraint) => { setConstraints( produce((draft) => { return draft.map((oldConstraint) => { @@ -75,7 +61,7 @@ export const EditableConstraintsList = forwardRef< }); }), ); - }); + }; if (context.length === 0) { return null; @@ -85,12 +71,11 @@ export const EditableConstraintsList = forwardRef< {constraints.map((constraint, index) => ( - onDelete(index)} + onAutoSave={onAutoSave(constraint[constraintId])} /> ))} diff --git a/frontend/src/component/common/NewConstraintAccordion/NewConstraintAccordionList/NewConstraintAccordionList.tsx b/frontend/src/component/common/NewConstraintAccordion/NewConstraintAccordionList/NewConstraintAccordionList.tsx index 6357ed6a33..e4dc791993 100644 --- a/frontend/src/component/common/NewConstraintAccordion/NewConstraintAccordionList/NewConstraintAccordionList.tsx +++ b/frontend/src/component/common/NewConstraintAccordion/NewConstraintAccordionList/NewConstraintAccordionList.tsx @@ -12,8 +12,8 @@ import { import { NewConstraintAccordion } from 'component/common/NewConstraintAccordion/NewConstraintAccordion'; import { ConstraintsList } from 'component/common/ConstraintsList/ConstraintsList'; import { useUiFlag } from 'hooks/useUiFlag'; -import { EditableConstraintWrapper } from 'component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraintWrapper'; import { ConstraintAccordionView } from 'component/common/NewConstraintAccordion/ConstraintAccordionView/ConstraintAccordionView'; +import { EditableConstraint } from 'component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint'; export interface IConstraintAccordionListProps { constraints: IConstraint[]; @@ -147,16 +147,15 @@ export const NewConstraintAccordionList = forwardRef< {constraints.map((constraint, index) => addEditStrategy ? ( - state.get(constraint)?.editing ? ( - onRemove(index)} + // @ts-ignore + onAutoSave={onAutoSave(constraintId)} /> ) : ( ({ '--padding': theme.spacing(2), @@ -150,111 +140,29 @@ const StyledCaseSensitiveIcon = styled(CaseSensitiveIcon)(({ theme }) => ({ fill: 'currentcolor', })); -type InputType = - | { input: 'legal values' } - | { input: 'date' } - | { input: 'single value'; type: 'number' | 'semver' } - | { input: 'multiple values' }; - -const getInputType = (input: Input): InputType => { - switch (input) { - case 'IN_OPERATORS_LEGAL_VALUES': - case 'STRING_OPERATORS_LEGAL_VALUES': - case 'NUM_OPERATORS_LEGAL_VALUES': - case 'SEMVER_OPERATORS_LEGAL_VALUES': - return { input: 'legal values' }; - case 'DATE_OPERATORS_SINGLE_VALUE': - return { input: 'date' }; - case 'NUM_OPERATORS_SINGLE_VALUE': - return { input: 'single value', type: 'number' }; - case 'SEMVER_OPERATORS_SINGLE_VALUE': - return { input: 'single value', type: 'semver' }; - case 'IN_OPERATORS_FREETEXT': - case 'STRING_OPERATORS_FREETEXT': - return { input: 'multiple values' }; - } -}; - type Props = { constraint: IConstraint; - localConstraint: IConstraint; - setContextName: (contextName: string) => void; - setOperator: (operator: Operator) => void; - setLocalConstraint: React.Dispatch>; - onDelete?: () => void; - toggleInvertedOperator: () => void; - toggleCaseSensitivity: () => void; - onUndo: () => void; - constraintChanges: IConstraint[]; - contextDefinition: Pick; - constraintValues: string[]; - constraintValue: string; - setValue: (value: string) => void; - setValues: (values: string[]) => void; - setValuesWithRecord: (values: string[]) => void; - removeValue: (index: number) => void; + onDelete: () => void; + onAutoSave: (constraint: IConstraint) => void; }; export const EditableConstraint: FC = ({ - constraintChanges, - constraint, - localConstraint, - setLocalConstraint, - setContextName, - setOperator, onDelete, - onUndo, - toggleInvertedOperator, - toggleCaseSensitivity, - contextDefinition, - constraintValues, - constraintValue, - setValue, - setValues, - setValuesWithRecord, - removeValue, + constraint, + onAutoSave, }) => { - const { input } = useConstraintInput({ - contextDefinition, - localConstraint, - }); + const { + constraint: localConstraint, + updateConstraint, + validator, + ...constraintMetadata + } = useEditableConstraint(constraint, onAutoSave); const { context } = useUnleashContext(); const { contextName, operator } = localConstraint; - const [showCaseSensitiveButton, setShowCaseSensitiveButton] = - useState(false); + const showCaseSensitiveButton = isStringOperator(operator); const deleteButtonRef = useRef(null); const addValuesButtonRef = useRef(null); - const inputType = getInputType(input); - - /* We need a special case to handle the currentTime context field. Since - this field will be the only one to allow DATE_BEFORE and DATE_AFTER operators - this will check if the context field is the current time context field AND check - if it is not already using one of the date operators (to not overwrite if there is existing - data). */ - useEffect(() => { - if ( - contextName === CURRENT_TIME_CONTEXT_FIELD && - !oneOf(dateOperators, operator) - ) { - setLocalConstraint((prev) => ({ - ...prev, - operator: DATE_AFTER, - value: new Date().toISOString(), - })); - } else if ( - contextName !== CURRENT_TIME_CONTEXT_FIELD && - oneOf(dateOperators, operator) - ) { - setOperator(IN); - } - - if (oneOf(stringOperators, operator)) { - setShowCaseSensitiveButton(true); - } else { - setShowCaseSensitiveButton(false); - } - }, [contextName, setOperator, operator, setLocalConstraint]); if (!context) { return null; @@ -265,72 +173,76 @@ export const EditableConstraint: FC = ({ }); const onOperatorChange = (operator: Operator) => { - if (oneOf(stringOperators, operator)) { - setShowCaseSensitiveButton(true); - } else { - setShowCaseSensitiveButton(false); - } - - if (oneOf(dateOperators, operator)) { - setLocalConstraint((prev) => ({ - ...prev, - operator: operator, - value: new Date().toISOString(), - })); - } else { - setOperator(operator); - } + updateConstraint({ type: 'set operator', payload: operator }); }; - const validator = useMemo(() => constraintValidator(input), [input]); const TopRowInput = () => { - switch (inputType.input) { - case 'date': - return ( - - ); - case 'single value': - return ( - { - setValue(newValue); - }} - removeValue={() => setValue('')} - currentValue={localConstraint.value} - helpText={ - inputType.type === 'number' - ? 'Add a single number' - : 'A semver value should be of the format X.Y.Z' - } - inputType={ - inputType.type === 'number' ? 'number' : 'text' - } - /> - ); - case 'multiple values': - return ( - { - // todo (`addEditStrategy`): move deduplication logic higher up in the context handling - const combinedValues = new Set([ - ...(localConstraint.values || []), - ...newValues, - ]); - setValuesWithRecord(Array.from(combinedValues)); - }} - /> - ); - default: - return null; + if (isDateConstraint(localConstraint)) { + return ( + + updateConstraint({ + type: 'set value', + payload: value, + }) + } + value={localConstraint.value} + validator={validator} + /> + ); } + if (isSemVerConstraint(localConstraint)) { + return ( + { + updateConstraint({ + type: 'set value', + payload: newValue, + }); + }} + removeValue={() => + updateConstraint({ type: 'clear values' }) + } + currentValue={localConstraint.value} + helpText={'A semver value should be of the format X.Y.Z'} + inputType={'text'} + /> + ); + } + if (isNumberConstraint(localConstraint)) { + return ( + { + updateConstraint({ + type: 'set value', + payload: newValue, + }); + }} + removeValue={() => + updateConstraint({ type: 'clear values' }) + } + currentValue={localConstraint.value} + helpText={'Add a single number'} + inputType={'number'} + /> + ); + } + + return ( + { + updateConstraint({ + type: 'add value(s)', + payload: newValues, + }); + }} + /> + ); }; return ( @@ -346,14 +258,23 @@ export const EditableConstraint: FC = ({ autoFocus options={constraintNameOptions} value={contextName || ''} - onChange={setContextName} + onChange={(contextField) => + updateConstraint({ + type: 'set context field', + payload: contextField, + }) + } variant='standard' /> + updateConstraint({ + type: 'toggle inverted operator', + }) + } > {localConstraint.inverted ? ( @@ -378,7 +299,11 @@ export const EditableConstraint: FC = ({ {showCaseSensitiveButton ? ( + updateConstraint({ + type: 'toggle case sensitivity', + }) + } > {localConstraint.caseInsensitive ? ( @@ -397,9 +322,17 @@ export const EditableConstraint: FC = ({ + updateConstraint({ + type: 'remove value from list', + payload: value, + }) + } getExternalFocusTarget={() => addValuesButtonRef.current ?? deleteButtonRef.current @@ -421,17 +354,32 @@ export const EditableConstraint: FC = ({ - {inputType.input === 'legal values' ? ( + {'legalValues' in constraintMetadata && + isMultiValueConstraint(localConstraint) ? ( + updateConstraint({ + type: 'clear values', + }) + } + addValues={(newValues) => + updateConstraint({ + type: 'add value(s)', + payload: newValues, + }) + } + removeValue={(value) => + updateConstraint({ + type: 'remove value from list', + payload: value, + }) + } + deletedLegalValues={ + constraintMetadata.deletedLegalValues + } + legalValues={constraintMetadata.legalValues} /> ) : null} diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraintWrapper.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraintWrapper.tsx deleted file mode 100644 index da3abe52e2..0000000000 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraintWrapper.tsx +++ /dev/null @@ -1,195 +0,0 @@ -import { useCallback, useEffect, useState } from 'react'; -import type { IConstraint } from 'interfaces/strategy'; -import { cleanConstraint } from 'utils/cleanConstraint'; -import useUnleashContext from 'hooks/api/getters/useUnleashContext/useUnleashContext'; -import type { IUnleashContextDefinition } from 'interfaces/context'; -import type { Operator } from 'constants/operators'; -import { EditableConstraint } from 'component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint'; - -interface IConstraintAccordionEditProps { - constraint: IConstraint; - onCancel?: () => void; - onSave: (constraint: IConstraint) => void; - onDelete?: () => void; - onAutoSave?: (constraint: IConstraint) => void; -} - -export const CANCEL = 'cancel'; -export const SAVE = 'save'; - -const resolveContextDefinition = ( - context: IUnleashContextDefinition[], - contextName: string, -): IUnleashContextDefinition => { - const definition = context.find( - (contextDef) => contextDef.name === contextName, - ); - - return ( - definition || { - name: '', - description: '', - createdAt: '', - sortOrder: 1, - stickiness: false, - } - ); -}; - -export const EditableConstraintWrapper = ({ - constraint, - onSave, - onDelete, - onAutoSave, -}: IConstraintAccordionEditProps) => { - const [localConstraint, setLocalConstraint] = useState( - cleanConstraint(constraint), - ); - const [constraintChanges, setConstraintChanges] = useState([ - cleanConstraint(constraint), - ]); - - const { context } = useUnleashContext(); - const [contextDefinition, setContextDefinition] = useState( - resolveContextDefinition(context, localConstraint.contextName), - ); - - useEffect(() => { - setContextDefinition( - resolveContextDefinition(context, localConstraint.contextName), - ); - }, [localConstraint.contextName, context]); - - const onUndo = () => { - if (constraintChanges.length < 2) return; - const previousChange = constraintChanges[constraintChanges.length - 2]; - - setLocalConstraint(previousChange); - setConstraintChanges((prev) => prev.slice(0, prev.length - 1)); - autoSave(previousChange); - }; - - const autoSave = (localConstraint: IConstraint) => { - if (onAutoSave) { - onAutoSave(localConstraint); - } - }; - - const recordChange = (localConstraint: IConstraint) => { - setConstraintChanges((prev) => [...prev, localConstraint]); - autoSave(localConstraint); - }; - - const setContextName = useCallback((contextName: string) => { - setLocalConstraint((prev) => { - const localConstraint = cleanConstraint({ - ...prev, - contextName, - values: [], - value: '', - }); - - recordChange(localConstraint); - - return localConstraint; - }); - }, []); - - const setOperator = useCallback((operator: Operator) => { - setLocalConstraint((prev) => { - const localConstraint = cleanConstraint({ - ...prev, - operator, - values: [], - value: '', - }); - - recordChange(localConstraint); - - return localConstraint; - }); - }, []); - - const setValuesWithRecord = useCallback((values: string[]) => { - setLocalConstraint((prev) => { - const localConstraint = { ...prev, values }; - - recordChange(localConstraint); - - return localConstraint; - }); - }, []); - - const setValues = useCallback((values: string[]) => { - setLocalConstraint((prev) => { - const localConstraint = { ...prev, values }; - - return localConstraint; - }); - }, []); - - const setValue = useCallback((value: string) => { - setLocalConstraint((prev) => { - const localConstraint = { ...prev, value }; - - recordChange(localConstraint); - - return localConstraint; - }); - }, []); - - const setInvertedOperator = () => { - setLocalConstraint((prev) => { - const localConstraint = { ...prev, inverted: !prev.inverted }; - - recordChange(localConstraint); - - return localConstraint; - }); - }; - - const setCaseInsensitive = useCallback(() => { - setLocalConstraint((prev) => { - const localConstraint = { - ...prev, - caseInsensitive: !prev.caseInsensitive, - }; - - recordChange(localConstraint); - - return localConstraint; - }); - }, []); - - const removeValue = useCallback( - (index: number) => { - const valueCopy = [...localConstraint.values!]; - valueCopy.splice(index, 1); - - setValuesWithRecord(valueCopy); - }, - [localConstraint, setValuesWithRecord], - ); - - return ( - - ); -}; diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraintAccordionList/FeatureStrategyConstraintAccordionList.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraintAccordionList/FeatureStrategyConstraintAccordionList.tsx index f6f6730f08..5cd8fb71df 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraintAccordionList/FeatureStrategyConstraintAccordionList.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/FeatureStrategyConstraintAccordionList/FeatureStrategyConstraintAccordionList.tsx @@ -93,25 +93,20 @@ export const FeatureStrategyConstraintAccordionList = forwardRef< } /> - - } - elseShow={ - - } - /> - + {addEditStrategy && setConstraints ? ( + + ) : ( + + )} ({ marginTop: theme.spacing(2), diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/LegalValuesSelector.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/LegalValuesSelector.tsx index 13a9580743..20d85224ff 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/LegalValuesSelector.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/LegalValuesSelector.tsx @@ -1,51 +1,19 @@ -import { useEffect, useState } from 'react'; -import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; +import { useState } from 'react'; import { Alert, Button, Checkbox, styled } from '@mui/material'; -import type { ILegalValue } from 'interfaces/context'; import { filterLegalValues, LegalValueLabel, } from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/LegalValueLabel/LegalValueLabel'; import { ConstraintValueSearch } from './ConstraintValueSearch'; +import type { ILegalValue } from 'interfaces/context'; -interface IRestrictiveLegalValuesProps { - data: { - legalValues: ILegalValue[]; - deletedLegalValues: ILegalValue[]; - }; - constraintValues: string[]; - values: string[]; - setValues: (values: string[]) => void; - setValuesWithRecord: (values: string[]) => void; - beforeValues?: JSX.Element; -} - -interface IValuesMap { - [key: string]: boolean; -} - -const createValuesMap = (values: string[]): IValuesMap => { - return values.reduce((result: IValuesMap, currentValue: string) => { - if (!result[currentValue]) { - result[currentValue] = true; - } - return result; - }, {}); -}; - -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) => value && deletedValuesSet.has(value), - ); +type LegalValuesSelectorProps = { + values: Set; + addValues: (values: string[]) => void; + removeValue: (value: string) => void; + clearAll: () => void; + deletedLegalValues?: Set; + legalValues: ILegalValue[]; }; const StyledValuesContainer = styled('div')(({ theme }) => ({ @@ -70,66 +38,34 @@ const LegalValuesSelectorWidget = styled('article')(({ theme }) => ({ })); export const LegalValuesSelector = ({ - data, + legalValues, values, - setValues, - setValuesWithRecord, - constraintValues, -}: IRestrictiveLegalValuesProps) => { + addValues, + removeValue, + clearAll, + deletedLegalValues, +}: LegalValuesSelectorProps) => { 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 cleanDeletedLegalValues = (constraintValues: string[]): string[] => { - 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) => { - if (valuesMap[legalValue]) { - const index = values.findIndex((value) => value === legalValue); - const newValues = [...values]; - newValues.splice(index, 1); - setValuesWithRecord(newValues); - return; + if (values.has(legalValue)) { + removeValue(legalValue); + } else { + addValues([legalValue]); } - - setValuesWithRecord([...cleanDeletedLegalValues(values), legalValue]); }; - const isAllSelected = legalValues.every((value) => - values.includes(value.value), - ); + const isAllSelected = legalValues.every((value) => values.has(value.value)); const onSelectAll = () => { if (isAllSelected) { - return setValuesWithRecord([]); + clearAll(); + return; + } else { + addValues(legalValues.map(({ value }) => value)); } - setValuesWithRecord([ - ...legalValues.map((legalValue) => legalValue.value), - ]); }; const handleSearchKeyDown = (event: React.KeyboardEvent) => { @@ -144,22 +80,18 @@ export const LegalValuesSelector = ({ 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}
    • - ))} -
- - } - /> + {deletedLegalValues?.size ? ( + + 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: +
    + {[...deletedLegalValues].map((value) => ( +
  • {value}
    • + ))} +
+ + ) : null}

Select values from a predefined set

onChange(match.value)} name='legal-value' color='primary' - disabled={deletedLegalValues - .map(({ value }) => value) - .includes(match.value)} + disabled={deletedLegalValues?.has(match.value)} /> } /> diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/ValueList.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/ValueList.tsx index a2754810c7..ec48a6c006 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/ValueList.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/ValueList.tsx @@ -54,9 +54,8 @@ export const ValueChip = styled( })); type Props = { - values: string[] | undefined; - removeValue: (index: number) => void; - setValues: (values: string[]) => void; + values?: string[]; + removeValue: (value: string) => void; // the element that should receive focus when all value chips are deleted getExternalFocusTarget: () => HTMLElement | null; }; @@ -102,7 +101,7 @@ export const ValueList: FC> = ({ label={value} onDelete={() => { nextFocusTarget(index)?.focus(); - removeValue(index); + removeValue(value); }} /> diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.ts b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.ts new file mode 100644 index 0000000000..9e81b15005 --- /dev/null +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-reducer.ts @@ -0,0 +1,144 @@ +import { + DATE_AFTER, + IN, + type Operator, + isDateOperator, + isSingleValueOperator, +} from 'constants/operators'; +import { CURRENT_TIME_CONTEXT_FIELD } from 'utils/operatorsForContext'; +import { + isDateConstraint, + isMultiValueConstraint, + isSingleValueConstraint, + type EditableConstraint, +} 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: 'clear values' } + | { type: 'remove value from list'; 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; + return { + ...rest, + values: new Set(), + }; + } + return { + ...state, + values: new Set(), + }; +}; + +export const constraintReducer = ( + state: EditableConstraint, + action: ConstraintUpdateAction, + deletedLegalValues?: Set, +): EditableConstraint => { + switch (action.type) { + case 'set context field': + if ( + action.payload === CURRENT_TIME_CONTEXT_FIELD && + !isDateOperator(state.operator) + ) { + return { + ...state, + contextName: action.payload, + operator: DATE_AFTER, + value: new Date().toISOString(), + }; + } else if ( + action.payload !== CURRENT_TIME_CONTEXT_FIELD && + isDateOperator(state.operator) + ) { + return { + ...state, + operator: IN, + contextName: action.payload, + values: new Set(), + }; + } + + return resetValues({ + ...state, + contextName: action.payload, + }); + case 'set operator': + if (isDateConstraint(state) && isDateOperator(action.payload)) { + return { + ...state, + operator: action.payload, + }; + } + + if (isSingleValueOperator(action.payload)) { + return resetValues({ + ...state, + value: '', + operator: action.payload, + }); + } + return resetValues({ + ...state, + values: new Set(), + operator: action.payload, + }); + case 'add value(s)': { + if (!('values' in state)) { + return state; + } + + const newValues = new Set(action.payload); + const combinedValues = union(state.values, newValues); + const filteredValues = deletedLegalValues + ? difference(combinedValues, deletedLegalValues) + : combinedValues; + return { + ...state, + 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': + if (isSingleValueConstraint(state)) { + return state; + } + state.values.delete(action.payload); + return { + ...state, + values: state.values ?? new Set(), + }; + case 'clear values': + return resetValues(state); + } +}; diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/constraint-validator.ts b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-validator.ts similarity index 63% rename from frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/constraint-validator.ts rename to frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-validator.ts index 9f001519c6..f0b4f3db17 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/constraint-validator.ts +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/constraint-validator.ts @@ -1,8 +1,11 @@ -// todo: (flag: `addEditStrategy`) see if this type is better duplicated or extracted to somewhere else -import type { Input } from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/useConstraintInput'; import { isValid, parseISO } from 'date-fns'; import semver from 'semver'; - +import { + type EditableConstraint, + isDateConstraint, + isNumberConstraint, + isSemVerConstraint, +} from './editable-constraint-type'; export type ConstraintValidationResult = [boolean, string]; const numberValidator = (value: string): ConstraintValidationResult => { @@ -56,20 +59,15 @@ const dateValidator = (value: string): ConstraintValidationResult => { return [true, '']; }; -export const constraintValidator = (input: Input) => { - switch (input) { - case 'IN_OPERATORS_LEGAL_VALUES': - case 'STRING_OPERATORS_LEGAL_VALUES': - case 'NUM_OPERATORS_LEGAL_VALUES': - case 'SEMVER_OPERATORS_LEGAL_VALUES': - case 'IN_OPERATORS_FREETEXT': - case 'STRING_OPERATORS_FREETEXT': - return stringListValidator; - case 'DATE_OPERATORS_SINGLE_VALUE': - return dateValidator; - case 'NUM_OPERATORS_SINGLE_VALUE': - return numberValidator; - case 'SEMVER_OPERATORS_SINGLE_VALUE': - return semVerValidator; +export const constraintValidator = (constraint: EditableConstraint) => { + if (isDateConstraint(constraint)) { + return dateValidator; } + if (isSemVerConstraint(constraint)) { + return semVerValidator; + } + if (isNumberConstraint(constraint)) { + return numberValidator; + } + return stringListValidator; }; diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/editable-constraint-type.ts b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/editable-constraint-type.ts new file mode 100644 index 0000000000..c55c546cfb --- /dev/null +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/editable-constraint-type.ts @@ -0,0 +1,98 @@ +import { + type DateOperator, + isDateOperator, + isMultiValueOperator, + isSingleValueOperator, + type NumOperator, + type SemVerOperator, + type MultiValueOperator, + isNumOperator, + isSemVerOperator, +} from 'constants/operators'; +import type { IConstraint } from 'interfaces/strategy'; + +type EditableConstraintBase = Omit< + IConstraint, + 'operator' | 'values' | 'value' +>; + +export type EditableNumberConstraint = EditableConstraintBase & { + operator: NumOperator; + value: string; +}; +export type EditableDateConstraint = EditableConstraintBase & { + operator: DateOperator; + value: string; +}; +export type EditableSemVerConstraint = EditableConstraintBase & { + operator: SemVerOperator; + value: string; +}; + +export type EditableMultiValueConstraint = EditableConstraintBase & { + operator: MultiValueOperator; + values: Set; +}; + +export type EditableSingleValueConstraint = + | EditableNumberConstraint + | EditableDateConstraint + | EditableSemVerConstraint; + +export type EditableConstraint = + | EditableSingleValueConstraint + | EditableMultiValueConstraint; + +export const isMultiValueConstraint = ( + constraint: EditableConstraint, +): constraint is EditableMultiValueConstraint => + isMultiValueOperator(constraint.operator); + +export const isSingleValueConstraint = ( + constraint: EditableConstraint, +): constraint is EditableSingleValueConstraint => + !isMultiValueConstraint(constraint); + +export const isDateConstraint = ( + constraint: EditableConstraint, +): constraint is EditableDateConstraint => isDateOperator(constraint.operator); + +export const isNumberConstraint = ( + constraint: EditableConstraint, +): constraint is EditableNumberConstraint => isNumOperator(constraint.operator); + +export const isSemVerConstraint = ( + constraint: EditableConstraint, +): constraint is EditableSemVerConstraint => + isSemVerOperator(constraint.operator); + +export const fromIConstraint = ( + constraint: IConstraint, +): EditableConstraint => { + const { value, values, operator, ...rest } = constraint; + if (isSingleValueOperator(operator)) { + return { + ...rest, + operator, + value: value ?? '', + }; + } else { + return { + ...rest, + operator, + values: new Set(values), + }; + } +}; + +export const toIConstraint = (constraint: EditableConstraint): IConstraint => { + if ('value' in constraint) { + return constraint; + } else { + const { values, ...rest } = constraint; + return { + ...rest, + values: Array.from(values), + }; + } +}; diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/set-functions.ts b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/set-functions.ts new file mode 100644 index 0000000000..6731854459 --- /dev/null +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/set-functions.ts @@ -0,0 +1,25 @@ +// because Set.prototype.union and difference are baseline available 2024, but +// not available in GH actions yet. Caniuse also reports coverage at 87%. we can +// likely remove these in favor of the native implementations the next time we +// touch this code. +// +// todo: replace the use of this methods with set.union and set.difference when +// it's available. + +export const union = (a: Iterable, b: Set) => { + const result = new Set(a); + for (const element of b) { + result.add(element); + } + return result; +}; + +export const difference = (a: Iterable, b: Set) => { + const result = new Set(a); + for (const element of a) { + if (!b.has(element)) { + result.add(element); + } + } + return result; +}; diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.tsx new file mode 100644 index 0000000000..e922e4e142 --- /dev/null +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.tsx @@ -0,0 +1,145 @@ +import { useMemo, useState } from 'react'; +import useUnleashContext from 'hooks/api/getters/useUnleashContext/useUnleashContext'; +import type { IConstraint } from 'interfaces/strategy'; +import { + type EditableMultiValueConstraint, + type EditableSingleValueConstraint, + fromIConstraint, + isSingleValueConstraint, + toIConstraint, +} from './editable-constraint-type'; +import type { + ILegalValue, + IUnleashContextDefinition, +} from 'interfaces/context'; +import { + constraintReducer, + type ConstraintUpdateAction, +} from './constraint-reducer'; +import { + type ConstraintValidationResult, + constraintValidator, +} from './constraint-validator'; +import { difference } from './set-functions'; + +const resolveContextDefinition = ( + context: IUnleashContextDefinition[], + contextName: string, +): IUnleashContextDefinition => { + const definition = context.find( + (contextDef) => contextDef.name === contextName, + ); + + return ( + definition || { + name: '', + description: '', + createdAt: '', + sortOrder: 1, + stickiness: false, + } + ); +}; + +type SingleValueConstraintState = { + constraint: EditableSingleValueConstraint; +}; + +type MultiValueConstraintState = { + constraint: EditableMultiValueConstraint; +}; + +type LegalValueData = { + legalValues: ILegalValue[]; + deletedLegalValues?: Set; +}; + +type LegalValueConstraintState = { + constraint: EditableMultiValueConstraint; +} & LegalValueData; + +type EditableConstraintState = { + updateConstraint: (action: ConstraintUpdateAction) => void; + validator: (...values: string[]) => ConstraintValidationResult; +} & ( + | SingleValueConstraintState + | MultiValueConstraintState + | LegalValueConstraintState +); + +export const useEditableConstraint = ( + constraint: IConstraint, + onAutoSave: (constraint: IConstraint) => void, +): EditableConstraintState => { + const [localConstraint, setLocalConstraint] = useState(() => { + return fromIConstraint(constraint); + }); + + const { context } = useUnleashContext(); + const [contextDefinition, setContextDefinition] = useState( + resolveContextDefinition(context, localConstraint.contextName), + ); + + const deletedLegalValues = useMemo(() => { + if ( + contextDefinition.legalValues?.length && + constraint.values?.length + ) { + const currentLegalValues = new Set( + contextDefinition.legalValues.map(({ value }) => value), + ); + const deletedValues = difference( + constraint.values, + currentLegalValues, + ); + + return deletedValues; + } + return undefined; + }, [ + JSON.stringify(contextDefinition.legalValues), + JSON.stringify(constraint.values), + ]); + + const updateConstraint = (action: ConstraintUpdateAction) => { + const nextState = constraintReducer( + localConstraint, + action, + deletedLegalValues, + ); + const contextFieldHasChanged = + localConstraint.contextName !== nextState.contextName; + + setLocalConstraint(nextState); + onAutoSave(toIConstraint(nextState)); + + if (contextFieldHasChanged) { + setContextDefinition( + resolveContextDefinition(context, nextState.contextName), + ); + } + }; + + if (isSingleValueConstraint(localConstraint)) { + return { + updateConstraint, + constraint: localConstraint, + validator: constraintValidator(localConstraint), + }; + } + if (contextDefinition.legalValues?.length) { + return { + updateConstraint, + constraint: localConstraint, + validator: constraintValidator(localConstraint), + legalValues: contextDefinition.legalValues, + deletedLegalValues, + }; + } + + return { + updateConstraint, + constraint: localConstraint, + validator: constraintValidator(localConstraint), + }; +}; diff --git a/frontend/src/component/segments/SegmentFormStepTwo.tsx b/frontend/src/component/segments/SegmentFormStepTwo.tsx index bffc021212..560b6a6888 100644 --- a/frontend/src/component/segments/SegmentFormStepTwo.tsx +++ b/frontend/src/component/segments/SegmentFormStepTwo.tsx @@ -202,31 +202,25 @@ export const SegmentFormStepTwo: React.FC = ({ } /> - - } - elseShow={ - - } - /> + {addEditStrategy && + hasAccess(modePermission, project) && + setConstraints ? ( + + ) : ( + + )}