From a7118e0c18808bebd24d6aa9706255da0903ad74 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 6 May 2025 15:21:33 +0200 Subject: [PATCH] chore(1-3639): constraint validation (#9909) Implements client-side validation of constraint values before you can add them to a constraint. I've removed the extra server-side validation that used to happen for each specific constraint, because the surrounding form itself uses server side validation to check every constraint every time there's a change. This is what controls disabling the submit button etc. I wanna make the next PR a bit of a followup cleanup now that it's clearer what properties we do and don't need. image image --- .../useConstraintInput/useConstraintInput.tsx | 3 + .../AddSingleValueWidget.tsx | 26 ++++++- .../AddValuesWidget.tsx | 15 +++- .../ConstraintDateInput.tsx | 22 ++++-- .../EditableConstraint.tsx | 27 ++++--- .../EditableConstraintWrapper.tsx | 17 ----- .../constraint-validator.ts | 75 +++++++++++++++++++ .../NavigationSidebar/NavigationSidebar.tsx | 39 +++++----- 8 files changed, 164 insertions(+), 60 deletions(-) create mode 100644 frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/constraint-validator.ts diff --git a/frontend/src/component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/useConstraintInput.tsx b/frontend/src/component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/useConstraintInput.tsx index 3597817482..6313ad1c72 100644 --- a/frontend/src/component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/useConstraintInput.tsx +++ b/frontend/src/component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/useConstraintInput.tsx @@ -64,6 +64,9 @@ type Validator = | 'STRING_ARRAY_VALIDATOR' | 'DATE_VALIDATOR'; +/** + * @deprecated; remove with `addEditStrategy` flag. This component requires a lot of state and mixes many components. Better off using dedicated pieces where you need them. + */ export const useConstraintInput = ({ contextDefinition, localConstraint, diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddSingleValueWidget.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddSingleValueWidget.tsx index 1ce574eabe..d81feb5e3a 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddSingleValueWidget.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddSingleValueWidget.tsx @@ -3,6 +3,7 @@ import { styled } from '@mui/material'; import { forwardRef, useImperativeHandle, useRef, useState } from 'react'; import { ValueChip } from './ValueList'; import { AddValuesPopover, type OnAddActions } from './AddValuesPopover'; +import type { ConstraintValidatorOutput } from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/constraintValidators'; const StyledChip = styled(ValueChip, { shouldForwardProp: (prop) => prop !== 'hasValue', @@ -23,10 +24,21 @@ type Props = { currentValue?: string; helpText?: string; inputType: 'text' | 'number'; + validator: (value: string) => ConstraintValidatorOutput; }; export const AddSingleValueWidget = forwardRef( - ({ currentValue, onAddValue, removeValue, helpText, inputType }, ref) => { + ( + { + currentValue, + onAddValue, + removeValue, + helpText, + inputType, + validator, + }, + ref, + ) => { const [open, setOpen] = useState(false); const positioningRef = useRef(null); useImperativeHandle( @@ -40,9 +52,15 @@ export const AddSingleValueWidget = forwardRef( return; } - onAddValue(newValue); - setError(''); - setOpen(false); + const [isValid, errorMessage] = validator(newValue); + if (isValid) { + onAddValue(newValue); + setError(''); + setOpen(false); + } else { + setError(errorMessage); + return; + } }; return ( diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddValuesWidget.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddValuesWidget.tsx index 33b154cd65..a320c6a24c 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddValuesWidget.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/AddValuesWidget.tsx @@ -4,6 +4,7 @@ import { forwardRef, useImperativeHandle, useRef, useState } from 'react'; import { parseParameterStrings } from 'utils/parseParameter'; import { baseChipStyles } from './ValueList'; import { AddValuesPopover, type OnAddActions } from './AddValuesPopover'; +import type { ConstraintValidatorOutput } from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/constraintValidators'; // todo: MUI v6 / v7 upgrade: consider changing this to a Chip to align with the rest of the values and the single value selector. There was a fix introduced in v6 that makes you not lose focus on pressing esc: https://mui.com/material-ui/migration/upgrade-to-v6/#chip talk to Thomas for more info. const AddValuesButton = styled('button')(({ theme }) => ({ @@ -29,10 +30,11 @@ const AddValuesButton = styled('button')(({ theme }) => ({ interface AddValuesProps { onAddValues: (newValues: string[]) => void; helpText?: string; + validator: (...values: string[]) => ConstraintValidatorOutput; } export const AddValuesWidget = forwardRef( - ({ onAddValues, helpText }, ref) => { + ({ onAddValues, helpText, validator }, ref) => { const [open, setOpen] = useState(false); const positioningRef = useRef(null); useImperativeHandle( @@ -56,9 +58,14 @@ export const AddValuesWidget = forwardRef( return; } - onAddValues(newValues); - clearInput(); - setError(''); + const [isValid, errorMessage] = validator(...newValues); + if (isValid) { + onAddValues(newValues); + clearInput(); + setError(''); + } else { + setError(errorMessage); + } }; return ( diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/ConstraintDateInput.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/ConstraintDateInput.tsx index d9bb930718..3059bfe27f 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/ConstraintDateInput.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/ConstraintDateInput.tsx @@ -6,12 +6,12 @@ import { styled } from '@mui/material'; import TimezoneCountries from 'countries-and-timezones'; import { ScreenReaderOnly } from 'component/common/ScreenReaderOnly/ScreenReaderOnly'; import { HelpIcon } from 'component/common/HelpIcon/HelpIcon'; +import type { ConstraintValidatorOutput } from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/constraintValidators'; interface IDateSingleValueProps { setValue: (value: string) => void; value?: string; - error: string; - setError: React.Dispatch>; + validator: (value: string) => ConstraintValidatorOutput; } const StyledInput = styled(Input)({ @@ -31,9 +31,9 @@ const Container = styled('div')(({ theme }) => ({ export const ConstraintDateInput = ({ setValue, value, - error, - setError, + validator, }: IDateSingleValueProps) => { + const [error, setError] = useState(''); const timezones = Object.values( TimezoneCountries.getAllTimezones({ deprecated: false }) as { [timezone: string]: { name: string; utcOffsetStr: string }; @@ -79,8 +79,18 @@ export const ConstraintDateInput = ({ setError(''); const parsedDate = parseValidDate(e.target.value); const dateString = parsedDate?.toISOString(); - dateString && setPickedDate(dateString); - dateString && setValue(dateString); + if (!dateString) { + setError('Invalid date format'); + } else { + setPickedDate(dateString); + + const [isValid, errorMessage] = validator(dateString); + if (isValid) { + dateString && setValue(dateString); + } else { + setError(errorMessage); + } + } }} InputLabelProps={{ shrink: true, diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx index 04af6fef83..25955cd10c 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint.tsx @@ -1,6 +1,9 @@ import { IconButton, styled } from '@mui/material'; import GeneralSelect from 'component/common/GeneralSelect/GeneralSelect'; -import type { Input } from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/useConstraintInput'; +import { + useConstraintInput, + type Input, +} from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/useConstraintInput'; import { DATE_AFTER, dateOperators, @@ -11,7 +14,7 @@ import { import useUnleashContext from 'hooks/api/getters/useUnleashContext/useUnleashContext'; import type { IUnleashContextDefinition } from 'interfaces/context'; import type { IConstraint } from 'interfaces/strategy'; -import { useEffect, useRef, useState, type FC } from 'react'; +import { useEffect, useMemo, useRef, useState, type FC } from 'react'; import { oneOf } from 'utils/oneOf'; import { CURRENT_TIME_CONTEXT_FIELD, @@ -32,6 +35,7 @@ import { AddSingleValueWidget } from './AddSingleValueWidget'; import { ConstraintDateInput } from './ConstraintDateInput'; import { LegalValuesSelector } from './LegalValuesSelector'; import { resolveLegalValues } from './resolve-legal-values'; +import { constraintValidator } from './constraint-validator'; const Container = styled('article')(({ theme }) => ({ '--padding': theme.spacing(2), @@ -177,7 +181,6 @@ type Props = { setContextName: (contextName: string) => void; setOperator: (operator: Operator) => void; setLocalConstraint: React.Dispatch>; - action: string; onDelete?: () => void; toggleInvertedOperator: () => void; toggleCaseSensitivity: () => void; @@ -189,11 +192,9 @@ type Props = { setValue: (value: string) => void; setValues: (values: string[]) => void; setValuesWithRecord: (values: string[]) => void; - setError: React.Dispatch>; removeValue: (index: number) => void; - input: Input; - error: string; }; + export const EditableConstraint: FC = ({ constraintChanges, constraint, @@ -205,17 +206,19 @@ export const EditableConstraint: FC = ({ onUndo, toggleInvertedOperator, toggleCaseSensitivity, - input, contextDefinition, constraintValues, constraintValue, setValue, setValues, setValuesWithRecord, - setError, removeValue, - error, }) => { + const { input } = useConstraintInput({ + contextDefinition, + localConstraint, + }); + const { context } = useUnleashContext(); const { contextName, operator } = localConstraint; const [showCaseSensitiveButton, setShowCaseSensitiveButton] = @@ -279,6 +282,7 @@ export const EditableConstraint: FC = ({ } }; + const validator = useMemo(() => constraintValidator(input), [input]); const TopRowInput = () => { switch (inputType.input) { case 'date': @@ -286,13 +290,13 @@ export const EditableConstraint: FC = ({ ); case 'single value': return ( { setValue(newValue); }} @@ -311,6 +315,7 @@ export const EditableConstraint: FC = ({ case 'multiple values': return ( { diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraintWrapper.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraintWrapper.tsx index 63267001a0..da3abe52e2 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraintWrapper.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraintWrapper.tsx @@ -1,12 +1,10 @@ import { useCallback, useEffect, useState } from 'react'; import type { IConstraint } from 'interfaces/strategy'; import { cleanConstraint } from 'utils/cleanConstraint'; -import useFeatureApi from 'hooks/api/actions/useFeatureApi/useFeatureApi'; 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'; -import { useConstraintInput } from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/useConstraintInput'; interface IConstraintAccordionEditProps { constraint: IConstraint; @@ -55,13 +53,6 @@ export const EditableConstraintWrapper = ({ const [contextDefinition, setContextDefinition] = useState( resolveContextDefinition(context, localConstraint.contextName), ); - const { validateConstraint } = useFeatureApi(); - const [action, setAction] = useState(''); - - const { input, validator, setError, error } = useConstraintInput({ - contextDefinition, - localConstraint, - }); useEffect(() => { setContextDefinition( @@ -69,10 +60,6 @@ export const EditableConstraintWrapper = ({ ); }, [localConstraint.contextName, context]); - useEffect(() => { - setError(''); - }, [setError]); - const onUndo = () => { if (constraintChanges.length < 2) return; const previousChange = constraintChanges[constraintChanges.length - 2]; @@ -190,7 +177,6 @@ export const EditableConstraintWrapper = ({ setLocalConstraint={setLocalConstraint} setContextName={setContextName} setOperator={setOperator} - action={action} toggleInvertedOperator={setInvertedOperator} toggleCaseSensitivity={setCaseInsensitive} onDelete={onDelete} @@ -199,11 +185,8 @@ export const EditableConstraintWrapper = ({ setValues={setValues} setValuesWithRecord={setValuesWithRecord} setValue={setValue} - setError={setError} constraintValues={constraint?.values || []} constraintValue={constraint?.value || ''} - input={input} - error={error} contextDefinition={contextDefinition} removeValue={removeValue} constraint={constraint} diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/constraint-validator.ts b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/constraint-validator.ts new file mode 100644 index 0000000000..9f001519c6 --- /dev/null +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/constraint-validator.ts @@ -0,0 +1,75 @@ +// 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'; + +export type ConstraintValidationResult = [boolean, string]; + +const numberValidator = (value: string): ConstraintValidationResult => { + const converted = Number(value); + + if (typeof converted !== 'number' || Number.isNaN(converted)) { + if (typeof value === 'string' && value.includes(',')) { + return [ + false, + 'Comma (",") is not valid as a decimal separator. Use a period (".") instead.', + ]; + } + return [false, 'Value must be a number']; + } + + return [true, '']; +}; + +const stringListValidator = ( + ...values: string[] +): ConstraintValidationResult => { + const error: ConstraintValidationResult = [ + false, + 'Values must be a list of strings', + ]; + if (!Array.isArray(values)) { + return error; + } + + if (!values.every((value) => typeof value === 'string')) { + return error; + } + + return [true, '']; +}; + +const semVerValidator = (value: string): ConstraintValidationResult => { + const isCleanValue = semver.clean(value) === value; + + if (!semver.valid(value) || !isCleanValue) { + return [false, 'Value is not a valid semver. For example 1.2.4']; + } + + return [true, '']; +}; + +const dateValidator = (value: string): ConstraintValidationResult => { + if (!isValid(parseISO(value))) { + return [false, 'Value must be a valid date matching RFC3339']; + } + 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; + } +}; diff --git a/frontend/src/component/layout/MainLayout/NavigationSidebar/NavigationSidebar.tsx b/frontend/src/component/layout/MainLayout/NavigationSidebar/NavigationSidebar.tsx index 127590f834..c3a4e0bdf5 100644 --- a/frontend/src/component/layout/MainLayout/NavigationSidebar/NavigationSidebar.tsx +++ b/frontend/src/component/layout/MainLayout/NavigationSidebar/NavigationSidebar.tsx @@ -53,23 +53,24 @@ export const MobileNavigationSidebar: FC<{ ); }; -export const StretchContainer = styled(Box)<{ mode: string; admin: boolean }>( - ({ theme, mode, admin }) => ({ - backgroundColor: admin - ? theme.palette.background.application - : theme.palette.background.paper, - borderRight: admin ? `2px solid ${theme.palette.divider}` : 'none', - padding: theme.spacing(2), - alignSelf: 'stretch', - display: 'flex', - flexDirection: 'column', - gap: theme.spacing(2), - zIndex: 1, - overflowAnchor: 'none', - minWidth: mode === 'full' ? theme.spacing(32) : 'auto', - width: mode === 'full' ? theme.spacing(32) : 'auto', - }), -); +export const StretchContainer = styled(Box, { + shouldForwardProp: (propName) => + propName !== 'mode' && propName !== 'admin', +})<{ mode: string; admin: boolean }>(({ theme, mode, admin }) => ({ + backgroundColor: admin + ? theme.palette.background.application + : theme.palette.background.paper, + borderRight: admin ? `2px solid ${theme.palette.divider}` : 'none', + padding: theme.spacing(2), + alignSelf: 'stretch', + display: 'flex', + flexDirection: 'column', + gap: theme.spacing(2), + zIndex: 1, + overflowAnchor: 'none', + minWidth: mode === 'full' ? theme.spacing(32) : 'auto', + width: mode === 'full' ? theme.spacing(32) : 'auto', +})); const StyledLink = styled(Link)(({ theme }) => focusable(theme)); @@ -92,7 +93,9 @@ const StyledUnleashLogoOnlyWhite = styled(LogoOnlyWhite)(({ theme }) => ({ })); // This component is needed when the sticky item could overlap with nav items. You can replicate it on a short screen. -const StickyContainer = styled(Box)<{ admin: boolean }>(({ theme, admin }) => ({ +const StickyContainer = styled(Box, { + shouldForwardProp: (prop) => prop !== 'admin', +})<{ admin: boolean }>(({ theme, admin }) => ({ position: 'sticky', paddingBottom: theme.spacing(1.5), paddingTop: theme.spacing(1),