From 7d827442ee030489988fbfa520894873506ac0cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Mon, 11 Mar 2024 08:18:36 +0000 Subject: [PATCH] fix: add actions validation (#6481) https://linear.app/unleash/issue/2-2022/improve-actions-validation Improves our current actions form validation. Empty actions are now ignored on the payload and we get errors in actions where any of the required fields are empty. Also refactored our current actions into a constant map that can be shared across frontend and backend. --- .../ProjectActionsForm/ProjectActionsForm.tsx | 7 ++ .../ProjectActionsActionItem.tsx | 68 +++++++++++-------- .../ProjectActionsFormStepActions.tsx | 6 ++ .../ProjectActionsFormStepSource.tsx | 3 + .../useProjectActionsForm.ts | 39 +++++++++-- .../ProjectActionsModal.tsx | 16 +++-- .../useServiceAccountAccessMatrix.ts | 11 ++- src/lib/util/constants/actions.ts | 24 +++++++ 8 files changed, 130 insertions(+), 44 deletions(-) create mode 100644 src/lib/util/constants/actions.ts diff --git a/frontend/src/component/project/Project/ProjectSettings/ProjectActions/ProjectActionsTable/ProjectActionsModal/ProjectActionsForm/ProjectActionsForm.tsx b/frontend/src/component/project/Project/ProjectSettings/ProjectActions/ProjectActionsTable/ProjectActionsModal/ProjectActionsForm/ProjectActionsForm.tsx index fe173ab102..7bc2e81c3e 100644 --- a/frontend/src/component/project/Project/ProjectSettings/ProjectActions/ProjectActionsTable/ProjectActionsModal/ProjectActionsForm/ProjectActionsForm.tsx +++ b/frontend/src/component/project/Project/ProjectSettings/ProjectActions/ProjectActionsTable/ProjectActionsModal/ProjectActionsForm/ProjectActionsForm.tsx @@ -57,6 +57,8 @@ interface IProjectActionsFormProps { setActions: React.Dispatch>; errors: ProjectActionsFormErrors; validateName: (name: string) => boolean; + validateSourceId: (sourceId: number) => boolean; + validateActorId: (actorId: number) => boolean; validated: boolean; } @@ -77,6 +79,8 @@ export const ProjectActionsForm = ({ setActions, errors, validateName, + validateSourceId, + validateActorId, validated, }: IProjectActionsFormProps) => { const { serviceAccounts, loading: serviceAccountsLoading } = @@ -142,6 +146,7 @@ export const ProjectActionsForm = ({ setSourceId={setSourceId} filters={filters} setFilters={setFilters} + validateSourceId={validateSourceId} /> ({ display: 'flex', @@ -28,25 +29,13 @@ const StyledFieldContainer = styled('div')({ flex: 1, }); -const options = [ - { - label: 'Enable flag', - key: 'TOGGLE_FEATURE_ON', - permissions: ['UPDATE_FEATURE_ENVIRONMENT'], - }, - { - label: 'Disable flag', - key: 'TOGGLE_FEATURE_OFF', - permissions: ['UPDATE_FEATURE_ENVIRONMENT'], - }, -]; - interface IProjectActionsItemProps { action: ActionsActionState; index: number; stateChanged: (action: ActionsActionState) => void; actorId: number; onDelete: () => void; + validated: boolean; } export const ProjectActionsActionItem = ({ @@ -55,22 +44,23 @@ export const ProjectActionsActionItem = ({ stateChanged, actorId, onDelete, + validated, }: IProjectActionsItemProps) => { - const { action: actionName } = action; + const { action: actionName, executionParams, error } = action; const projectId = useRequiredPathParam('projectId'); const { project } = useProjectOverview(projectId); const { permissions } = useServiceAccountAccessMatrix( actorId, projectId, - action.executionParams.environment as string, + executionParams.environment as string, ); - const hasPermission = useMemo(() => { - const requiredPermissions = options.find( - ({ key }) => key === actionName, - )?.permissions; + const actionDefinition = ACTIONS.get(actionName); - const { environment: actionEnvironment } = action.executionParams; + const hasPermission = useMemo(() => { + const requiredPermissions = actionDefinition?.permissions; + + const { environment: actionEnvironment } = executionParams; if ( permissions.length === 0 || @@ -87,7 +77,24 @@ export const ProjectActionsActionItem = ({ environment === actionEnvironment, ), ); - }, [actionName, permissions]); + }, [actionDefinition, permissions]); + + useEffect(() => { + stateChanged({ + ...action, + error: undefined, + }); + if ( + actionDefinition?.required.some( + (required) => !executionParams[required], + ) + ) { + stateChanged({ + ...action, + error: 'Please fill all required fields.', + }); + } + }, [actionDefinition, executionParams]); const environments = project.environments.map( ({ environment }) => environment, @@ -116,7 +123,10 @@ export const ProjectActionsActionItem = ({ ({ + key, + label, + }))} value={actionName} onChange={(selected) => stateChanged({ @@ -135,12 +145,12 @@ export const ProjectActionsActionItem = ({ label: environment, key: environment, }))} - value={action.executionParams.environment as string} + value={executionParams.environment as string} onChange={(selected) => stateChanged({ ...action, executionParams: { - ...action.executionParams, + ...executionParams, environment: selected, }, }) @@ -156,12 +166,12 @@ export const ProjectActionsActionItem = ({ label: feature.name, key: feature.name, }))} - value={action.executionParams.featureName as string} + value={executionParams.featureName as string} onChange={(selected) => stateChanged({ ...action, executionParams: { - ...action.executionParams, + ...executionParams, featureName: selected, }, }) @@ -170,6 +180,10 @@ export const ProjectActionsActionItem = ({ /> + {error}} + /> >; actorId: number; setActorId: React.Dispatch>; + validateActorId: (actorId: number) => boolean; + validated: boolean; } export const ProjectActionsFormStepActions = ({ @@ -38,6 +40,8 @@ export const ProjectActionsFormStepActions = ({ setActions, actorId, setActorId, + validateActorId, + validated, }: IProjectActionsFormStepActionsProps) => { const projectId = useRequiredPathParam('projectId'); @@ -91,6 +95,7 @@ export const ProjectActionsFormStepActions = ({ options={serviceAccountOptions} value={`${actorId}`} onChange={(v) => { + validateActorId(Number(v)); setActorId(parseInt(v)); }} /> @@ -107,6 +112,7 @@ export const ProjectActionsFormStepActions = ({ actions.filter((a) => a.id !== action.id), ) } + validated={validated} /> ))} diff --git a/frontend/src/component/project/Project/ProjectSettings/ProjectActions/ProjectActionsTable/ProjectActionsModal/ProjectActionsForm/ProjectActionsFormStep/ProjectActionsFormStepSource/ProjectActionsFormStepSource.tsx b/frontend/src/component/project/Project/ProjectSettings/ProjectActions/ProjectActionsTable/ProjectActionsModal/ProjectActionsForm/ProjectActionsFormStep/ProjectActionsFormStepSource/ProjectActionsFormStepSource.tsx index 90e3875efe..a5c04bb113 100644 --- a/frontend/src/component/project/Project/ProjectSettings/ProjectActions/ProjectActionsTable/ProjectActionsModal/ProjectActionsForm/ProjectActionsFormStep/ProjectActionsFormStepSource/ProjectActionsFormStepSource.tsx +++ b/frontend/src/component/project/Project/ProjectSettings/ProjectActions/ProjectActionsTable/ProjectActionsModal/ProjectActionsForm/ProjectActionsFormStep/ProjectActionsFormStepSource/ProjectActionsFormStepSource.tsx @@ -37,6 +37,7 @@ interface IProjectActionsFormStepSourceProps { setSourceId: React.Dispatch>; filters: ActionsFilterState[]; setFilters: React.Dispatch>; + validateSourceId: (sourceId: number) => boolean; } export const ProjectActionsFormStepSource = ({ @@ -44,6 +45,7 @@ export const ProjectActionsFormStepSource = ({ setSourceId, filters, setFilters, + validateSourceId, }: IProjectActionsFormStepSourceProps) => { const { signalEndpoints, loading: signalEndpointsLoading } = useSignalEndpoints(); @@ -102,6 +104,7 @@ export const ProjectActionsFormStepSource = ({ options={signalEndpointOptions} value={`${sourceId}`} onChange={(v) => { + validateSourceId(Number(v)); setSourceId(parseInt(v)); }} /> diff --git a/frontend/src/component/project/Project/ProjectSettings/ProjectActions/ProjectActionsTable/ProjectActionsModal/ProjectActionsForm/useProjectActionsForm.ts b/frontend/src/component/project/Project/ProjectSettings/ProjectActions/ProjectActionsTable/ProjectActionsModal/ProjectActionsForm/useProjectActionsForm.ts index 6b0a75ac30..ad738e21eb 100644 --- a/frontend/src/component/project/Project/ProjectSettings/ProjectActions/ProjectActionsTable/ProjectActionsModal/ProjectActionsForm/useProjectActionsForm.ts +++ b/frontend/src/component/project/Project/ProjectSettings/ProjectActions/ProjectActionsTable/ProjectActionsModal/ProjectActionsForm/useProjectActionsForm.ts @@ -23,6 +23,7 @@ export type ActionsActionState = Omit< 'id' | 'createdAt' | 'createdByUserId' > & { id: string; + error?: string; }; const DEFAULT_PROJECT_ACTIONS_FORM_ERRORS = { @@ -99,12 +100,13 @@ export const useProjectActionsForm = (action?: IActionSet) => { }; useEffect(() => { - clearError(ErrorField.FILTERS); - if (filters.some(({ error }) => error)) { - setError(ErrorField.FILTERS, 'One or more filters have errors.'); - } + validateFilters(filters); }, [filters]); + useEffect(() => { + validateActions(actions); + }, [actions]); + const isEmpty = (value: string) => !value.length; const isNameNotUnique = (value: string) => @@ -137,6 +139,16 @@ export const useProjectActionsForm = (action?: IActionSet) => { return true; }; + const validateFilters = (filters: ActionsFilterState[]) => { + if (filters.some(({ error }) => error)) { + setError(ErrorField.FILTERS, 'One or more filters have errors.'); + return false; + } + + clearError(ErrorField.FILTERS); + return true; + }; + const validateActorId = (sourceId: number) => { if (isIdEmpty(sourceId)) { setError(ErrorField.ACTOR, 'Service account is required.'); @@ -148,11 +160,17 @@ export const useProjectActionsForm = (action?: IActionSet) => { }; const validateActions = (actions: ActionsActionState[]) => { - if (actions.length === 0) { + if (actions.filter(({ action }) => Boolean(action)).length === 0) { setError(ErrorField.ACTIONS, 'At least one action is required.'); return false; } + clearError(ErrorField.ACTIONS); + if (actions.some(({ error }) => error)) { + setError(ErrorField.ACTIONS, 'One or more actions have errors.'); + return false; + } + clearError(ErrorField.ACTIONS); return true; }; @@ -160,12 +178,19 @@ export const useProjectActionsForm = (action?: IActionSet) => { const validate = () => { const validName = validateName(name); const validSourceId = validateSourceId(sourceId); + const validFilters = validateFilters(filters); const validActorId = validateActorId(actorId); const validActions = validateActions(actions); setValidated(true); - return validName && validSourceId && validActorId && validActions; + return ( + validName && + validSourceId && + validFilters && + validActorId && + validActions + ); }; return { @@ -187,6 +212,8 @@ export const useProjectActionsForm = (action?: IActionSet) => { setErrors, validated, validateName, + validateSourceId, + validateActorId, validate, reloadForm, }; diff --git a/frontend/src/component/project/Project/ProjectSettings/ProjectActions/ProjectActionsTable/ProjectActionsModal/ProjectActionsModal.tsx b/frontend/src/component/project/Project/ProjectSettings/ProjectActions/ProjectActionsTable/ProjectActionsModal/ProjectActionsModal.tsx index 11ba571b96..371390ce82 100644 --- a/frontend/src/component/project/Project/ProjectSettings/ProjectActions/ProjectActionsTable/ProjectActionsModal/ProjectActionsModal.tsx +++ b/frontend/src/component/project/Project/ProjectSettings/ProjectActions/ProjectActionsTable/ProjectActionsModal/ProjectActionsModal.tsx @@ -81,6 +81,8 @@ export const ProjectActionsModal = ({ setActions, errors, validateName, + validateSourceId, + validateActorId, validate, validated, reloadForm, @@ -127,11 +129,13 @@ export const ProjectActionsModal = ({ ), }, actorId, - actions: actions.map(({ action, sortOrder, executionParams }) => ({ - action, - sortOrder, - executionParams, - })), + actions: actions + .filter(({ action }) => Boolean(action)) + .map(({ action, sortOrder, executionParams }) => ({ + action, + sortOrder, + executionParams, + })), }; const formatApiCode = () => `curl --location --request ${ @@ -206,6 +210,8 @@ export const ProjectActionsModal = ({ setActions={setActions} errors={errors} validateName={validateName} + validateSourceId={validateSourceId} + validateActorId={validateActorId} validated={validated} /> diff --git a/frontend/src/hooks/api/getters/useServiceAccountAccessMatrix/useServiceAccountAccessMatrix.ts b/frontend/src/hooks/api/getters/useServiceAccountAccessMatrix/useServiceAccountAccessMatrix.ts index ad05d8776a..5b8d79d6eb 100644 --- a/frontend/src/hooks/api/getters/useServiceAccountAccessMatrix/useServiceAccountAccessMatrix.ts +++ b/frontend/src/hooks/api/getters/useServiceAccountAccessMatrix/useServiceAccountAccessMatrix.ts @@ -1,11 +1,11 @@ import { useMemo } from 'react'; import { formatApiPath } from 'utils/formatPath'; import handleErrorResponses from '../httpErrorResponseHandler'; -import useSWR from 'swr'; import { IRole } from 'interfaces/role'; import { IServiceAccount } from 'interfaces/service-account'; import { IMatrixPermission } from 'interfaces/permissions'; import { IPermission } from 'interfaces/user'; +import { useConditionalSWR } from '../useConditionalSWR/useConditionalSWR'; interface IServiceAccountAccessMatrix { root: IMatrixPermission[]; @@ -30,7 +30,7 @@ interface IServiceAccountAccessMatrixOutput } export const useServiceAccountAccessMatrix = ( - id: number, + id?: number, project?: string, environment?: string, ): IServiceAccountAccessMatrixOutput => { @@ -39,10 +39,9 @@ export const useServiceAccountAccessMatrix = ( }`; const url = `api/admin/service-account/${id}/permissions${queryParams}`; - const { data, error, mutate } = useSWR( - formatApiPath(url), - fetcher, - ); + const { data, error, mutate } = useConditionalSWR< + IServiceAccountAccessMatrixResponse | undefined + >(Boolean(id), undefined, formatApiPath(url), fetcher); return useMemo( () => ({ diff --git a/src/lib/util/constants/actions.ts b/src/lib/util/constants/actions.ts new file mode 100644 index 0000000000..6222700ea6 --- /dev/null +++ b/src/lib/util/constants/actions.ts @@ -0,0 +1,24 @@ +type ActionDefinition = { + label: string; + permissions: string[]; + required: string[]; +}; + +export const ACTIONS = new Map([ + [ + 'TOGGLE_FEATURE_ON', + { + label: 'Enable flag', + permissions: ['UPDATE_FEATURE_ENVIRONMENT'], + required: ['project', 'environment', 'featureName'], + }, + ], + [ + 'TOGGLE_FEATURE_OFF', + { + label: 'Disable flag', + permissions: ['UPDATE_FEATURE_ENVIRONMENT'], + required: ['project', 'environment', 'featureName'], + }, + ], +]);