From 6efbe2d5450e02ed59d901c76249899f6e5d2705 Mon Sep 17 00:00:00 2001
From: Thomas Heartman <thomas@getunleash.io>
Date: Mon, 12 May 2025 13:35:45 +0200
Subject: [PATCH] Single legal values (#9935)

Adds an input form for single legal values (i.e. for number and semver
operators).

- Uses the `validator` for the constraint to check the list of legal
values and provides a list of "invalid legal values" for values that
don't pass validation.
- Values in the "invalid legal values" set are "disabled" when rendered
in the UI. Additionally, there's an extra bit of text that tells you
that values that aren't valid are disabled.
- Makes the legal values selector more generic and makes it adapt to
multi- or single-value selection based on input props. The external
interface is two separate components (to make it clearer that they are
different things and because their props don't line up perfectly).

Rendered:
<img width="957" alt="image"
src="https://github.com/user-attachments/assets/cd8d2f32-057d-4e31-8fd3-174676eeb65e"
/>


Still todo: testing deleted/invalid legal value detection. I'll do that
as part of the big testathon in a followup.
---
 .../EditableConstraint.tsx                    | 113 ++++++----
 .../LegalValuesSelector.tsx                   | 196 +++++++++++++-----
 .../useEditableConstraint.tsx                 |  55 ++++-
 3 files changed, 260 insertions(+), 104 deletions(-)

diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx
index 6dd89606e0..42eb97b244 100644
--- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx
+++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx
@@ -16,7 +16,10 @@ import { ReactComponent as EqualsIcon } from 'assets/icons/constraint-equals.svg
 import { ReactComponent as NotEqualsIcon } from 'assets/icons/constraint-not-equals.svg';
 import { AddSingleValueWidget } from './AddSingleValueWidget';
 import { ConstraintDateInput } from './ConstraintDateInput';
-import { LegalValuesSelector } from './LegalValuesSelector';
+import {
+    LegalValuesSelector,
+    SingleLegalValueSelector,
+} from './LegalValuesSelector';
 import { useEditableConstraint } from './useEditableConstraint/useEditableConstraint';
 import type { IConstraint } from 'interfaces/strategy';
 import {
@@ -227,9 +230,11 @@ export const EditableConstraint: FC<Props> = ({
         constraint: localConstraint,
         updateConstraint,
         validator,
-        ...constraintMetadata
+        ...legalValueData
     } = useEditableConstraint(constraint, onAutoSave);
 
+    const isLegalValueConstraint = 'legalValues' in legalValueData;
+
     const { context } = useUnleashContext();
     const { contextName, operator } = localConstraint;
     const showCaseSensitiveButton = isStringOperator(operator);
@@ -327,25 +332,37 @@ export const EditableConstraint: FC<Props> = ({
                         values={
                             isMultiValueConstraint(localConstraint)
                                 ? Array.from(localConstraint.values)
-                                : undefined
-                        }
-                        removeValue={(value) =>
-                            updateConstraint({
-                                type: 'remove value from list',
-                                payload: value,
-                            })
+                                : 'legalValues' in legalValueData &&
+                                    localConstraint.value
+                                  ? [localConstraint.value]
+                                  : undefined
                         }
+                        removeValue={(value) => {
+                            if (isMultiValueConstraint(localConstraint)) {
+                                updateConstraint({
+                                    type: 'remove value from list',
+                                    payload: value,
+                                });
+                            } else {
+                                updateConstraint({
+                                    type: 'set value',
+                                    payload: '',
+                                });
+                            }
+                        }}
                         getExternalFocusTarget={() =>
                             addValuesButtonRef.current ??
                             deleteButtonRef.current
                         }
                     >
-                        <TopRowInput
-                            localConstraint={localConstraint}
-                            updateConstraint={updateConstraint}
-                            validator={validator}
-                            addValuesButtonRef={addValuesButtonRef}
-                        />
+                        {isLegalValueConstraint ? null : (
+                            <TopRowInput
+                                localConstraint={localConstraint}
+                                updateConstraint={updateConstraint}
+                                validator={validator}
+                                addValuesButtonRef={addValuesButtonRef}
+                            />
+                        )}
                     </ValueList>
                 </ConstraintDetails>
                 <ButtonPlaceholder />
@@ -361,33 +378,47 @@ export const EditableConstraint: FC<Props> = ({
                     </StyledIconButton>
                 </HtmlTooltip>
             </TopRow>
-            {'legalValues' in constraintMetadata &&
-            isMultiValueConstraint(localConstraint) ? (
+            {'legalValues' in legalValueData ? (
                 <LegalValuesContainer>
-                    <LegalValuesSelector
-                        values={localConstraint.values}
-                        clearAll={() =>
-                            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}
-                    />
+                    {isMultiValueConstraint(localConstraint) ? (
+                        <LegalValuesSelector
+                            values={localConstraint.values}
+                            clearAll={() =>
+                                updateConstraint({
+                                    type: 'clear values',
+                                })
+                            }
+                            addValues={(newValues) =>
+                                updateConstraint({
+                                    type: 'add value(s)',
+                                    payload: newValues,
+                                })
+                            }
+                            removeValue={(value) =>
+                                updateConstraint({
+                                    type: 'remove value from list',
+                                    payload: value,
+                                })
+                            }
+                            {...legalValueData}
+                        />
+                    ) : (
+                        <SingleLegalValueSelector
+                            value={localConstraint.value}
+                            clear={() =>
+                                updateConstraint({
+                                    type: 'clear values',
+                                })
+                            }
+                            addValue={(newValues) =>
+                                updateConstraint({
+                                    type: 'set value',
+                                    payload: newValues,
+                                })
+                            }
+                            {...legalValueData}
+                        />
+                    )}
                 </LegalValuesContainer>
             ) : null}
         </Container>
diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/LegalValuesSelector.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/LegalValuesSelector.tsx
index 20d85224ff..15bc4ef08c 100644
--- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/LegalValuesSelector.tsx
+++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/LegalValuesSelector.tsx
@@ -1,5 +1,5 @@
-import { useState } from 'react';
-import { Alert, Button, Checkbox, styled } from '@mui/material';
+import { type FC, useId, useState } from 'react';
+import { Alert, Button, Checkbox, Radio, styled } from '@mui/material';
 import {
     filterLegalValues,
     LegalValueLabel,
@@ -7,15 +7,6 @@ import {
 import { ConstraintValueSearch } from './ConstraintValueSearch';
 import type { ILegalValue } from 'interfaces/context';
 
-type LegalValuesSelectorProps = {
-    values: Set<string>;
-    addValues: (values: string[]) => void;
-    removeValue: (value: string) => void;
-    clearAll: () => void;
-    deletedLegalValues?: Set<string>;
-    legalValues: ILegalValue[];
-};
-
 const StyledValuesContainer = styled('div')(({ theme }) => ({
     display: 'grid',
     gridTemplateColumns: 'repeat(auto-fit, minmax(120px, 1fr))',
@@ -37,36 +28,39 @@ const LegalValuesSelectorWidget = styled('article')(({ theme }) => ({
     gap: theme.spacing(2),
 }));
 
-export const LegalValuesSelector = ({
+type BaseProps = {
+    legalValues: ILegalValue[];
+    onChange: (value: string) => void;
+    deletedLegalValues?: Set<string>;
+    invalidLegalValues?: Set<string>;
+    isInputSelected: (value: string) => boolean;
+    multiSelect?: {
+        selectAll: () => void;
+        clearAll: () => void;
+        values: Set<string>;
+    };
+};
+
+const BaseLegalValueSelector: FC<BaseProps> = ({
     legalValues,
-    values,
-    addValues,
-    removeValue,
-    clearAll,
+    onChange,
     deletedLegalValues,
-}: LegalValuesSelectorProps) => {
+    invalidLegalValues,
+    isInputSelected: isInputChecked,
+    multiSelect,
+}) => {
     const [filter, setFilter] = useState('');
+    const labelId = useId();
+    const descriptionId = useId();
+    const groupNameId = useId();
+    const alertId = useId();
 
     const filteredValues = filterLegalValues(legalValues, filter);
 
-    const onChange = (legalValue: string) => {
-        if (values.has(legalValue)) {
-            removeValue(legalValue);
-        } else {
-            addValues([legalValue]);
-        }
-    };
-
-    const isAllSelected = legalValues.every((value) => values.has(value.value));
-
-    const onSelectAll = () => {
-        if (isAllSelected) {
-            clearAll();
-            return;
-        } else {
-            addValues(legalValues.map(({ value }) => value));
-        }
-    };
+    const isAllSelected =
+        multiSelect &&
+        legalValues.length ===
+            multiSelect.values.size + (deletedLegalValues?.size ?? 0);
 
     const handleSearchKeyDown = (event: React.KeyboardEvent) => {
         if (event.key === 'Enter') {
@@ -77,11 +71,12 @@ export const LegalValuesSelector = ({
             }
         }
     };
+    const Control = multiSelect ? Checkbox : Radio;
 
     return (
         <LegalValuesSelectorWidget>
             {deletedLegalValues?.size ? (
-                <Alert severity='warning'>
+                <Alert id={alertId} severity='warning'>
                     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:
@@ -92,36 +87,58 @@ export const LegalValuesSelector = ({
                     </ul>
                 </Alert>
             ) : null}
-            <p>Select values from a predefined set</p>
+            <p>
+                <span id={labelId}>Select values from a predefined set</span>
+                {invalidLegalValues?.size ? (
+                    <span id={descriptionId}>
+                        Values that are not valid for your chosen operator have
+                        been disabled.
+                    </span>
+                ) : null}
+            </p>
             <Row>
                 <ConstraintValueSearch
                     onKeyDown={handleSearchKeyDown}
                     filter={filter}
                     setFilter={setFilter}
                 />
-                <Button
-                    sx={{
-                        whiteSpace: 'nowrap',
-                    }}
-                    variant={'text'}
-                    onClick={onSelectAll}
-                >
-                    {isAllSelected ? 'Unselect all' : 'Select all'}
-                </Button>
+                {multiSelect ? (
+                    <Button
+                        sx={{
+                            whiteSpace: 'nowrap',
+                        }}
+                        variant={'text'}
+                        onClick={() => {
+                            if (isAllSelected) {
+                                multiSelect.clearAll();
+                                return;
+                            } else {
+                                multiSelect.selectAll();
+                            }
+                        }}
+                    >
+                        {isAllSelected ? 'Unselect all' : 'Select all'}
+                    </Button>
+                ) : null}
             </Row>
-            <StyledValuesContainer>
+            <StyledValuesContainer
+                aria-labelledby={labelId}
+                aria-describedby={descriptionId}
+                aria-details={alertId}
+                role={multiSelect ? undefined : 'radiogroup'}
+            >
                 {filteredValues.map((match) => (
                     <LegalValueLabel
                         key={match.value}
                         legal={match}
                         filter={filter}
                         control={
-                            <Checkbox
-                                checked={Boolean(values.has(match.value))}
-                                onChange={() => onChange(match.value)}
-                                name='legal-value'
+                            <Control
                                 color='primary'
-                                disabled={deletedLegalValues?.has(match.value)}
+                                name={`legal-value-${groupNameId}`}
+                                checked={isInputChecked(match.value)}
+                                onChange={() => onChange(match.value)}
+                                disabled={invalidLegalValues?.has(match.value)}
                             />
                         }
                     />
@@ -130,3 +147,78 @@ export const LegalValuesSelector = ({
         </LegalValuesSelectorWidget>
     );
 };
+
+type LegalValuesSelectorProps = {
+    values: Set<string>;
+    addValues: (values: string[]) => void;
+    removeValue: (value: string) => void;
+    clearAll: () => void;
+    deletedLegalValues?: Set<string>;
+    invalidLegalValues?: Set<string>;
+    legalValues: ILegalValue[];
+};
+
+export const LegalValuesSelector = ({
+    legalValues,
+    values,
+    addValues,
+    removeValue,
+    clearAll,
+    ...baseProps
+}: LegalValuesSelectorProps) => {
+    const onChange = (legalValue: string) => {
+        if (values.has(legalValue)) {
+            removeValue(legalValue);
+        } else {
+            addValues([legalValue]);
+        }
+    };
+
+    return (
+        <BaseLegalValueSelector
+            legalValues={legalValues}
+            isInputSelected={(inputValue) => values.has(inputValue)}
+            onChange={onChange}
+            multiSelect={{
+                clearAll,
+                selectAll: () => {
+                    addValues(legalValues.map(({ value }) => value));
+                },
+                values,
+            }}
+            {...baseProps}
+        />
+    );
+};
+
+type SingleLegalValueSelectorProps = {
+    value: string;
+    addValue: (value: string) => void;
+    clear: () => void;
+    deletedLegalValues?: Set<string>;
+    legalValues: ILegalValue[];
+    invalidLegalValues?: Set<string>;
+};
+
+export const SingleLegalValueSelector = ({
+    value,
+    addValue,
+    clear,
+    ...baseProps
+}: SingleLegalValueSelectorProps) => {
+    const onChange = (newValue: string) => {
+        if (value === newValue) {
+            clear();
+        } else {
+            addValue(newValue);
+        }
+    };
+
+    return (
+        <BaseLegalValueSelector
+            onChange={onChange}
+            isInputSelected={(inputValue) => inputValue === value}
+            {...baseProps}
+        />
+    );
+};
diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.tsx
index e922e4e142..76c246074d 100644
--- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.tsx
+++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/useEditableConstraint/useEditableConstraint.tsx
@@ -52,6 +52,7 @@ type MultiValueConstraintState = {
 type LegalValueData = {
     legalValues: ILegalValue[];
     deletedLegalValues?: Set<string>;
+    invalidLegalValues?: Set<string>;
 };
 
 type LegalValueConstraintState = {
@@ -80,11 +81,14 @@ export const useEditableConstraint = (
         resolveContextDefinition(context, localConstraint.contextName),
     );
 
+    const validator = constraintValidator(localConstraint);
+
     const deletedLegalValues = useMemo(() => {
         if (
             contextDefinition.legalValues?.length &&
             constraint.values?.length
         ) {
+            // todo: extract and test
             const currentLegalValues = new Set(
                 contextDefinition.legalValues.map(({ value }) => value),
             );
@@ -101,6 +105,24 @@ export const useEditableConstraint = (
         JSON.stringify(constraint.values),
     ]);
 
+    const invalidLegalValues = useMemo(() => {
+        if (
+            contextDefinition.legalValues?.length &&
+            isSingleValueConstraint(localConstraint)
+        ) {
+            // todo: extract and test
+            return new Set(
+                contextDefinition.legalValues
+                    .filter(({ value }) => !validator(value)[0])
+                    .map(({ value }) => value),
+            );
+        }
+        return undefined;
+    }, [
+        JSON.stringify(contextDefinition.legalValues),
+        JSON.stringify(localConstraint.operator),
+    ]);
+
     const updateConstraint = (action: ConstraintUpdateAction) => {
         const nextState = constraintReducer(
             localConstraint,
@@ -120,26 +142,37 @@ export const useEditableConstraint = (
         }
     };
 
+    if (contextDefinition.legalValues?.length) {
+        if (isSingleValueConstraint(localConstraint)) {
+            return {
+                updateConstraint,
+                constraint: localConstraint,
+                validator,
+                legalValues: contextDefinition.legalValues,
+                invalidLegalValues,
+                deletedLegalValues,
+            };
+        }
+        return {
+            updateConstraint,
+            constraint: localConstraint,
+            validator,
+            legalValues: contextDefinition.legalValues,
+            invalidLegalValues,
+            deletedLegalValues,
+        };
+    }
     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,
+            validator,
         };
     }
 
     return {
         updateConstraint,
         constraint: localConstraint,
-        validator: constraintValidator(localConstraint),
+        validator,
     };
 };