From 23ea27703eef07885f1936fd0e6077b4d99f8db8 Mon Sep 17 00:00:00 2001 From: Tyler Ohlsen Date: Tue, 26 Mar 2024 17:39:35 -0700 Subject: [PATCH] Enforce fine-grained form and flow validation (#114) Signed-off-by: Tyler Ohlsen (cherry picked from commit 5231f094ba16c6f06862e6e01a63b3628101bf41) --- common/utils.ts | 1 + .../component_details/component_details.tsx | 6 +- .../component_details/component_inputs.tsx | 6 +- .../component_details/input_field_list.tsx | 3 + .../input_fields/select_field.tsx | 3 +- .../input_fields/text_field.tsx | 7 + .../workflow_detail/components/header.tsx | 18 +- public/pages/workflow_detail/utils/utils.ts | 37 +--- .../pages/workflow_detail/workflow_detail.tsx | 16 +- .../workspace/resizable_workspace.tsx | 166 ++++++++++++++++-- .../workflow_detail/workspace/workspace.tsx | 15 +- public/pages/workflows/workflows.test.tsx | 4 +- public/pages/workflows/workflows.tsx | 12 +- 13 files changed, 195 insertions(+), 99 deletions(-) diff --git a/common/utils.ts b/common/utils.ts index aaf0d788..014e29e7 100644 --- a/common/utils.ts +++ b/common/utils.ts @@ -76,6 +76,7 @@ export function toWorkspaceFlow( /** * Validates the UI workflow state. * Note we don't have to validate connections since that is done via input/output handlers. + * But we need to validate there are no open connections */ export function validateWorkspaceFlow( workspaceFlow: WorkspaceFlowState diff --git a/public/pages/workflow_detail/component_details/component_details.tsx b/public/pages/workflow_detail/component_details/component_details.tsx index d106092f..7ecaed6e 100644 --- a/public/pages/workflow_detail/component_details/component_details.tsx +++ b/public/pages/workflow_detail/component_details/component_details.tsx @@ -13,6 +13,7 @@ import { EmptyComponentInputs } from './empty_component_inputs'; import '../workspace/workspace-styles.scss'; interface ComponentDetailsProps { + onFormChange: () => void; selectedComponent?: ReactFlowComponent; } @@ -31,7 +32,10 @@ export function ComponentDetails(props: ComponentDetailsProps) { {props.selectedComponent ? ( - + ) : ( )} diff --git a/public/pages/workflow_detail/component_details/component_inputs.tsx b/public/pages/workflow_detail/component_details/component_inputs.tsx index bed3ad72..cae794df 100644 --- a/public/pages/workflow_detail/component_details/component_inputs.tsx +++ b/public/pages/workflow_detail/component_details/component_inputs.tsx @@ -10,6 +10,7 @@ import { ReactFlowComponent } from '../../../../common'; interface ComponentInputsProps { selectedComponent: ReactFlowComponent; + onFormChange: () => void; } export function ComponentInputs(props: ComponentInputsProps) { @@ -19,7 +20,10 @@ export function ComponentInputs(props: ComponentInputsProps) {

{props.selectedComponent.data.label || ''}

- + ); } diff --git a/public/pages/workflow_detail/component_details/input_field_list.tsx b/public/pages/workflow_detail/component_details/input_field_list.tsx index ced08c14..f7aed9d8 100644 --- a/public/pages/workflow_detail/component_details/input_field_list.tsx +++ b/public/pages/workflow_detail/component_details/input_field_list.tsx @@ -15,6 +15,7 @@ import { ReactFlowComponent } from '../../../../common'; interface InputFieldListProps { selectedComponent: ReactFlowComponent; + onFormChange: () => void; } export function InputFieldList(props: InputFieldListProps) { @@ -30,6 +31,7 @@ export function InputFieldList(props: InputFieldListProps) { @@ -42,6 +44,7 @@ export function InputFieldList(props: InputFieldListProps) { ); diff --git a/public/pages/workflow_detail/component_details/input_fields/select_field.tsx b/public/pages/workflow_detail/component_details/input_fields/select_field.tsx index 95d83772..f76567a8 100644 --- a/public/pages/workflow_detail/component_details/input_fields/select_field.tsx +++ b/public/pages/workflow_detail/component_details/input_fields/select_field.tsx @@ -36,6 +36,7 @@ const existingIndices = [ interface SelectFieldProps { field: IComponentField; componentId: string; + onFormChange: () => void; } /** @@ -56,8 +57,8 @@ export function SelectField(props: SelectFieldProps) { options={options} valueOfSelected={field.value || getInitialValue(props.field.type)} onChange={(option) => { - field.onChange(option); form.setFieldValue(formField, option); + props.onFormChange(); }} isInvalid={isFieldInvalid( props.componentId, diff --git a/public/pages/workflow_detail/component_details/input_fields/text_field.tsx b/public/pages/workflow_detail/component_details/input_fields/text_field.tsx index fec071ac..d3866a7e 100644 --- a/public/pages/workflow_detail/component_details/input_fields/text_field.tsx +++ b/public/pages/workflow_detail/component_details/input_fields/text_field.tsx @@ -17,6 +17,7 @@ import { interface TextFieldProps { field: IComponentField; componentId: string; + onFormChange: () => void; } /** @@ -46,6 +47,12 @@ export function TextField(props: TextFieldProps) { placeholder={props.field.placeholder || ''} compressed={false} value={field.value || getInitialValue(props.field.type)} + onChange={(e) => form.setFieldValue(formField, e.target.value)} + // This is a design decision to only trigger form updates onBlur() instead + // of onChange(). This is to rate limit the number of updates & re-renders made, as users + // typically rapidly type things into a text box, which would consequently trigger + // onChange() much more often. + onBlur={() => props.onFormChange()} /> ); diff --git a/public/pages/workflow_detail/components/header.tsx b/public/pages/workflow_detail/components/header.tsx index 2b99e57e..94b63195 100644 --- a/public/pages/workflow_detail/components/header.tsx +++ b/public/pages/workflow_detail/components/header.tsx @@ -33,21 +33,9 @@ export function WorkflowDetailHeader(props: WorkflowDetailHeaderProps) { ) } rightSideItems={[ - // TODO: add launch logic - {}}> - Launch - , - { - // @ts-ignore - saveWorkflow(props.workflow, reactFlowInstance); - dispatch(removeDirty()); - }} - > - Save + // TODO: finalize if this is needed + {}}> + Delete , ]} tabs={props.tabs} diff --git a/public/pages/workflow_detail/utils/utils.ts b/public/pages/workflow_detail/utils/utils.ts index f536ab25..b3037a1c 100644 --- a/public/pages/workflow_detail/utils/utils.ts +++ b/public/pages/workflow_detail/utils/utils.ts @@ -3,41 +3,20 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { - WorkspaceFlowState, - Workflow, - ReactFlowComponent, - toTemplateFlows, - validateWorkspaceFlow, -} from '../../../../common'; +import { Workflow, ReactFlowComponent } from '../../../../common'; -export function saveWorkflow(workflow: Workflow, rfInstance: any): void { - let curFlowState = rfInstance.toObject(); - - curFlowState = { - ...curFlowState, - nodes: processNodes(curFlowState.nodes), - }; - - const isValid = validateWorkspaceFlow(curFlowState); - if (isValid) { - const updatedWorkflow = { - ...workflow, - workspaceFlowState: curFlowState, - workflows: toTemplateFlows(curFlowState), - } as Workflow; - if (workflow.id) { - // TODO: implement connection to update workflow API - } else { - // TODO: implement connection to create workflow API - } +export function saveWorkflow(workflow?: Workflow): void { + if (workflow && workflow.id) { + // TODO: implement connection to update workflow API } else { - return; + // TODO: implement connection to create workflow API } } // Process the raw ReactFlow nodes to only persist the fields we need -function processNodes(nodes: ReactFlowComponent[]): ReactFlowComponent[] { +export function processNodes( + nodes: ReactFlowComponent[] +): ReactFlowComponent[] { return nodes .map((node: ReactFlowComponent) => { return Object.fromEntries( diff --git a/public/pages/workflow_detail/workflow_detail.tsx b/public/pages/workflow_detail/workflow_detail.tsx index 38e15f2e..40b11474 100644 --- a/public/pages/workflow_detail/workflow_detail.tsx +++ b/public/pages/workflow_detail/workflow_detail.tsx @@ -98,22 +98,11 @@ export function WorkflowDetail(props: WorkflowDetailProps) { // On initial load: // - fetch workflow, if there is an existing workflow ID - // - add a window listener to warn users if they exit/refresh - // without saving latest changes useEffect(() => { if (!isNewWorkflow) { // TODO: can optimize to only fetch a single workflow dispatch(searchWorkflows({ query: { match_all: {} } })); } - - // TODO: below has the following issue: - // 1. user starts to create new unsaved workflow changes - // 2. user navigates to other parts of the plugin without refreshing - no warning happens - // 3. user refreshes at any later time: if isDirty is still true, shows browser warning - // tune to only handle the check if still on the workflow details page, or consider adding a check / warning - // if navigating away from the details page without refreshing (where it is currently not being triggered) - // window.onbeforeunload = (e) => - // isDirty || isNewWorkflow ? true : undefined; }, []); const tabs = [ @@ -156,7 +145,10 @@ export function WorkflowDetail(props: WorkflowDetailProps) { tabs={tabs} /> {selectedTabId === WORKFLOW_DETAILS_TAB.EDITOR && ( - + )} {selectedTabId === WORKFLOW_DETAILS_TAB.LAUNCHES && } {selectedTabId === WORKFLOW_DETAILS_TAB.PROTOTYPE && ( diff --git a/public/pages/workflow_detail/workspace/resizable_workspace.tsx b/public/pages/workflow_detail/workspace/resizable_workspace.tsx index 983e36f0..b827fdc9 100644 --- a/public/pages/workflow_detail/workspace/resizable_workspace.tsx +++ b/public/pages/workflow_detail/workspace/resizable_workspace.tsx @@ -4,11 +4,17 @@ */ import React, { useRef, useState, useEffect, useContext } from 'react'; +import { useDispatch, useSelector } from 'react-redux'; import { useOnSelectionChange } from 'reactflow'; import { Form, Formik } from 'formik'; import * as yup from 'yup'; import { cloneDeep } from 'lodash'; -import { EuiButton, EuiResizableContainer } from '@elastic/eui'; +import { + EuiButton, + EuiCallOut, + EuiPageHeader, + EuiResizableContainer, +} from '@elastic/eui'; import { Workflow, WorkspaceFormValues, @@ -17,12 +23,18 @@ import { WorkspaceSchemaObj, componentDataToFormik, getComponentSchema, + toWorkspaceFlow, + validateWorkspaceFlow, + WorkspaceFlowState, + toTemplateFlows, } from '../../../../common'; -import { rfContext } from '../../../store'; +import { AppState, removeDirty, setDirty, rfContext } from '../../../store'; import { Workspace } from './workspace'; import { ComponentDetails } from '../component_details'; +import { processNodes, saveWorkflow } from '../utils'; interface ResizableWorkspaceProps { + isNewWorkflow: boolean; workflow?: Workflow; } @@ -33,6 +45,27 @@ const COMPONENT_DETAILS_PANEL_ID = 'component_details_panel_id'; * panels - the ReactFlow workspace panel and the selected component details panel. */ export function ResizableWorkspace(props: ResizableWorkspaceProps) { + const dispatch = useDispatch(); + + // Overall workspace state + const isDirty = useSelector((state: AppState) => state.workspace.isDirty); + const [isFirstSave, setIsFirstSave] = useState(props.isNewWorkflow); + const isSaveable = isFirstSave ? true : isDirty; + + // Workflow state + const [workflow, setWorkflow] = useState( + props.workflow + ); + + // Formik form state + const [formValues, setFormValues] = useState({}); + const [formSchema, setFormSchema] = useState(yup.object({})); + + // Validation states. Maintain separate state for form vs. overall flow so + // we can have fine-grained errors and action items for users + const [formValidOnSubmit, setFormValidOnSubmit] = useState(true); + const [flowValidOnSubmit, setFlowValidOnSubmit] = useState(true); + // Component details side panel state const [isDetailsPanelOpen, setisDetailsPanelOpen] = useState(true); const collapseFn = useRef( @@ -68,6 +101,25 @@ export function ResizableWorkspace(props: ResizableWorkspaceProps) { }, }); + // Hook to update the workflow's flow state, if applicable. It may not exist if + // it is a backend-only-created workflow, or a new, unsaved workflow. If so, + // generate a default one based on the 'workflows' JSON field. + useEffect(() => { + const workflowCopy = { ...props.workflow } as Workflow; + if (workflowCopy) { + if (!workflowCopy.workspaceFlowState) { + workflowCopy.workspaceFlowState = toWorkspaceFlow( + workflowCopy.workflows + ); + console.debug( + `There is no saved UI flow for workflow: ${workflowCopy.name}. Generating a default one.` + ); + } + setWorkflow(workflowCopy); + } + }, [props.workflow]); + + // Hook to updated the selected ReactFlow component useEffect(() => { reactFlowInstance?.setNodes((nodes: ReactFlowComponent[]) => nodes.map((node) => { @@ -80,16 +132,12 @@ export function ResizableWorkspace(props: ResizableWorkspaceProps) { ); }, [selectedComponent]); - // Formik form state - const [formValues, setFormValues] = useState({}); - const [formSchema, setFormSchema] = useState(yup.object({})); - // Initialize the form state to an existing workflow, if applicable. useEffect(() => { - if (props.workflow?.workspaceFlowState) { + if (workflow?.workspaceFlowState) { const initFormValues = {} as WorkspaceFormValues; const initSchemaObj = {} as WorkspaceSchemaObj; - props.workflow.workspaceFlowState.nodes.forEach((node) => { + workflow.workspaceFlowState.nodes.forEach((node) => { initFormValues[node.id] = componentDataToFormik(node.data); initSchemaObj[node.id] = getComponentSchema(node.data); }); @@ -97,7 +145,7 @@ export function ResizableWorkspace(props: ResizableWorkspaceProps) { setFormValues(initFormValues); setFormSchema(initFormSchema); } - }, [props.workflow]); + }, [workflow]); // Update the form values and validation schema when a node is added // or removed from the workspace. @@ -129,6 +177,16 @@ export function ResizableWorkspace(props: ResizableWorkspaceProps) { setFormSchema(updatedSchema); } + /** + * Function to pass down to the Formik
components as a listener to propagate + * form changes to this parent component to re-enable save button, etc. + */ + function onFormChange() { + if (!isDirty) { + dispatch(setDirty()); + } + } + return ( {(formikProps) => ( + {!formValidOnSubmit && ( + + Please address the highlighted fields and try saving again. + + )} + {!flowValidOnSubmit && ( + + Please ensure there are no open connections between the + components. + + )} + {}}> + Launch + , + { + dispatch(removeDirty()); + if (isFirstSave) { + setIsFirstSave(false); + } + // Submit the form to bubble up any errors. + // Ideally we handle Promise accept/rejects with submitForm(), but there is + // open issues for that - see https://github.com/jaredpalmer/formik/issues/2057 + // The workaround is to additionally execute validateForm() which will return any errors found. + formikProps.submitForm(); + formikProps.validateForm().then((validationResults: {}) => { + if (Object.keys(validationResults).length > 0) { + setFormValidOnSubmit(false); + } else { + setFormValidOnSubmit(true); + // @ts-ignore + let curFlowState = reactFlowInstance.toObject() as WorkspaceFlowState; + curFlowState = { + ...curFlowState, + nodes: processNodes(curFlowState.nodes), + }; + if (validateWorkspaceFlow(curFlowState)) { + setFlowValidOnSubmit(true); + const updatedWorkflow = { + ...workflow, + workspaceFlowState: curFlowState, + workflows: toTemplateFlows(curFlowState), + } as Workflow; + saveWorkflow(updatedWorkflow); + } else { + setFlowValidOnSubmit(false); + } + } + }); + }} + > + Save + , + ]} + bottomBorder={false} + /> {(EuiResizablePanel, EuiResizableButton, { togglePanel }) => { if (togglePanel) { @@ -153,17 +286,18 @@ export function ResizableWorkspace(props: ResizableWorkspaceProps) { <> onToggleChange()} > - + ); }} - formikProps.handleSubmit()}> - Submit - )}
diff --git a/public/pages/workflow_detail/workspace/workspace.tsx b/public/pages/workflow_detail/workspace/workspace.tsx index 6fb9f5b4..a8ee3c8d 100644 --- a/public/pages/workflow_detail/workspace/workspace.tsx +++ b/public/pages/workflow_detail/workspace/workspace.tsx @@ -21,7 +21,6 @@ import { IComponentData, ReactFlowComponent, Workflow, - toWorkspaceFlow, } from '../../../../common'; import { generateId, initComponentData } from '../../../utils'; import { WorkspaceComponent } from '../workspace_component'; @@ -116,20 +115,10 @@ export function Workspace(props: WorkspaceProps) { [reactFlowInstance] ); - // Initialization. Set the nodes and edges to an existing workflow, - // if applicable. + // Initialization. Set the nodes and edges to an existing workflow state, useEffect(() => { const workflow = { ...props.workflow }; - if (workflow) { - if (!workflow.workspaceFlowState) { - // No existing workspace state. This could be due to it being a backend-only-created - // workflow, or a new, unsaved workflow - // @ts-ignore - workflow.workspaceFlowState = toWorkspaceFlow(workflow.workflows); - console.debug( - `There is no saved UI flow for workflow: ${workflow.name}. Generating a default one.` - ); - } + if (workflow && workflow.workspaceFlowState) { setNodes(workflow.workspaceFlowState.nodes); setEdges(workflow.workspaceFlowState.edges); } diff --git a/public/pages/workflows/workflows.test.tsx b/public/pages/workflows/workflows.test.tsx index 3c12244c..cdf14c57 100644 --- a/public/pages/workflows/workflows.test.tsx +++ b/public/pages/workflows/workflows.test.tsx @@ -40,7 +40,7 @@ const renderWithRouter = () => ({ describe('Workflows', () => { test('renders the page', () => { - const { getByText } = renderWithRouter(); - expect(getByText('Workflows')).not.toBeNull(); + const { getAllByText } = renderWithRouter(); + expect(getAllByText('Workflows').length).toBeGreaterThan(0); }); }); diff --git a/public/pages/workflows/workflows.tsx b/public/pages/workflows/workflows.tsx index 64f5f3f2..b745d888 100644 --- a/public/pages/workflows/workflows.tsx +++ b/public/pages/workflows/workflows.tsx @@ -58,20 +58,14 @@ export function Workflows(props: WorkflowsProps) { ] as WORKFLOWS_TAB; const [selectedTabId, setSelectedTabId] = useState(tabFromUrl); - // If there is no selected tab or invalid tab, default to a tab depending - // on if user has existing created workflows or not. + // If there is no selected tab or invalid tab, default to manage tab useEffect(() => { if ( !selectedTabId || !Object.values(WORKFLOWS_TAB).includes(selectedTabId) ) { - if (Object.keys(workflows).length > 0) { - setSelectedTabId(WORKFLOWS_TAB.MANAGE); - replaceActiveTab(WORKFLOWS_TAB.MANAGE, props); - } else { - setSelectedTabId(WORKFLOWS_TAB.CREATE); - replaceActiveTab(WORKFLOWS_TAB.CREATE, props); - } + setSelectedTabId(WORKFLOWS_TAB.MANAGE); + replaceActiveTab(WORKFLOWS_TAB.MANAGE, props); } }, [selectedTabId, workflows]);