From 30ade4ddf8e6bfafd206b6efbc89e7463e9234c5 Mon Sep 17 00:00:00 2001 From: David Watrous <509299+dpwatrous@users.noreply.github.com> Date: Mon, 27 Mar 2023 11:39:03 -0400 Subject: [PATCH] Improve action logging & error handling - Friendly names to actions - Action error logging - ActionForm onError callback --- .../ui-common/action/__tests__/action.spec.ts | 5 + .../common/src/ui-common/action/action.ts | 106 +++++++++++-- .../common/src/ui-common/form/parameter.ts | 4 +- .../demo/form/action-form-demo.tsx | 5 +- .../ui-react/account/create-account-action.ts | 2 + .../form/__tests__/action-form.spec.tsx | 28 +++- .../ui-react/components/form/action-form.tsx | 139 ++++++++++++++---- 7 files changed, 244 insertions(+), 45 deletions(-) diff --git a/packages/common/src/ui-common/action/__tests__/action.spec.ts b/packages/common/src/ui-common/action/__tests__/action.spec.ts index 9a7722d414..1312b2f193 100644 --- a/packages/common/src/ui-common/action/__tests__/action.spec.ts +++ b/packages/common/src/ui-common/action/__tests__/action.spec.ts @@ -12,6 +12,9 @@ describe("Action tests", () => { subject: "planet", }); + expect(action.actionName).toEqual("Hello"); + expect(action.logger.context.area).toEqual("Action-Hello"); + // Test that we can call waitFor() functions even while execute/initialize // aren't running, and they return immediately await action.waitForInitialization(); @@ -132,6 +135,8 @@ describe("Action tests", () => { }; class HelloAction extends AbstractAction { + actionName = "Hello"; + message = "Hola world"; onValidateCallCount = 0; subjectOnValidateCallCount = 0; diff --git a/packages/common/src/ui-common/action/action.ts b/packages/common/src/ui-common/action/action.ts index 4f3935ab94..34bdba0de1 100644 --- a/packages/common/src/ui-common/action/action.ts +++ b/packages/common/src/ui-common/action/action.ts @@ -1,15 +1,75 @@ import { Form, FormValues, ValidationStatus } from "../form"; -import { getLogger } from "../logging"; +import { getLogger, Logger } from "../logging"; import { Deferred } from "../util"; export interface Action { - form: Form; - isInitialized: boolean; + /** + * A globally unique friendly name for this action. Used for logging, etc. + */ + readonly actionName: string; + + /** + * A logger which is pre-configured with the name of the action + */ + readonly logger: Logger; + + /** + * The form associated with this action. + */ + readonly form: Form; + + /** + * True if initialization has finished. + */ + readonly isInitialized: boolean; + + /** + * Load data needed for the action and perform any other initialization. + * Calls the onInitialize() callback. + */ initialize(): Promise; + + /** + * A callback which performs action initialization. + */ onInitialize(): Promise; + + /** + * Constructs the form for the action. + * + * @param initialValues Initial values to populate the form with. + */ + buildForm(initialValues: V): Form; + + /** + * Execute the action and return a result (which may be success or failure) + * @param formValues The form values to use to execute the action. + */ execute(formValues: V): Promise; + + /** + * A callback which performs validation synchronously. Note that both + * onValidateSync and onValidateAsync will be called for every validation. + * The onValidateSync callback will simply return more quickly. + * + * @param formValues The form values to validate + */ onValidateSync?(formValues: V): ValidationStatus; + + /** + * A callback which performs validation asynchronously. Note that both + * onValidateSync and onValidateAsync will be called for every validation. + * The onValidateSync callback will simply return more quickly. + * + * @param formValues The form values to validate + */ onValidateAsync?(formValues: V): Promise; + + /** + * A callback which performs action execution. + * + * @param formValues The form values to use + */ onExecute(formValues: V): Promise; /** @@ -36,6 +96,22 @@ export type ActionExecutionResult = { export abstract class AbstractAction implements Action { + abstract actionName: string; + + private _logger: Logger | undefined; + + /** + * Get a logger for the action + */ + get logger(): Logger { + // In practice this should never be undefined + const actionName = this.actionName ?? "UnknownAction"; + if (!this._logger) { + this._logger = getLogger(`Action-${actionName}`); + } + return this._logger; + } + protected _form?: Form; private _isInitialized: boolean = false; @@ -79,10 +155,11 @@ export abstract class AbstractAction }; this._isInitialized = true; - this._initializationDeferred.resolve(); - } catch (e) { - this._initializationDeferred.reject(e); } finally { + // Always resolve rather than reject. Waiting for initialization + // isn't the right place to handle errors. Instead, handle rejections + // from initialize() itself. + this._initializationDeferred.resolve(); this._isInitializing = false; } } @@ -157,20 +234,19 @@ export abstract class AbstractAction executionResult.success = true; } catch (e) { executionResult.error = e; - getLogger("AbstractAction").warn( - "Action failed to execute:", - e - ); + this.logger.warn("Action failed to execute:", e); } + } finally { + this._isExecuting = false; + // Always resolve rather than reject. Waiting for execution + // isn't the right place to handle errors. Instead, handle rejections + // from execute() itself. this._executionDeferred.resolve(); - } catch (e) { - this._executionDeferred.reject(e); - } finally { + // Create a new deferred execution object since execution // can happen again and again this._executionDeferred = new Deferred(); - this._isExecuting = false; } return executionResult; } @@ -181,7 +257,7 @@ export abstract class AbstractAction abstract onExecute(formValues: V): Promise; - abstract onValidateSync(values: V): ValidationStatus; + onValidateSync?(values: V): ValidationStatus; onValidateAsync?(values: V): Promise; } diff --git a/packages/common/src/ui-common/form/parameter.ts b/packages/common/src/ui-common/form/parameter.ts index e995f63fc3..b9c2173898 100644 --- a/packages/common/src/ui-common/form/parameter.ts +++ b/packages/common/src/ui-common/form/parameter.ts @@ -210,7 +210,7 @@ export abstract class AbstractParameter< D extends ParameterDependencies = ParameterDependencies > implements Parameter { - private _logger: Logger = getLogger("FormParameter"); + private _logger: Logger; readonly parentForm: Form; readonly parentSection?: Section; @@ -280,6 +280,8 @@ export abstract class AbstractParameter< this.name = name; + this._logger = getLogger(`FormParameter-${name}`); + this._label = init?.label; this.description = init?.description; this.disabled = init?.disabled; diff --git a/packages/playground/src/ui-playground/demo/form/action-form-demo.tsx b/packages/playground/src/ui-playground/demo/form/action-form-demo.tsx index e418951e1e..df0aa6bc83 100644 --- a/packages/playground/src/ui-playground/demo/form/action-form-demo.tsx +++ b/packages/playground/src/ui-playground/demo/form/action-form-demo.tsx @@ -1,4 +1,3 @@ -import { getLogger } from "@batch/ui-common"; import { AbstractAction, Action } from "@batch/ui-common/lib/action"; import { Form, @@ -29,7 +28,7 @@ type CarFormValues = { }; class CreateOrUpdateCarAction extends AbstractAction { - private _logger = getLogger("CreateOrUpdateCarAction"); + actionName = "CreateOrUpdateCar"; async onInitialize(): Promise { return { @@ -120,7 +119,7 @@ class CreateOrUpdateCarAction extends AbstractAction { } async onExecute(formValues: CarFormValues): Promise { - this._logger.info("Execute called with values:" + formValues); + this.logger.info("Execute called with values:" + formValues); } } diff --git a/packages/react/src/ui-react/account/create-account-action.ts b/packages/react/src/ui-react/account/create-account-action.ts index 74dbe4f42a..e6299f908d 100644 --- a/packages/react/src/ui-react/account/create-account-action.ts +++ b/packages/react/src/ui-react/account/create-account-action.ts @@ -29,6 +29,8 @@ export type CreateAccountFormValues = { }; export class CreateAccountAction extends AbstractAction { + actionName = "CreateAccount"; + private _initialValues: CreateAccountFormValues = {}; async onInitialize(): Promise { diff --git a/packages/react/src/ui-react/components/form/__tests__/action-form.spec.tsx b/packages/react/src/ui-react/components/form/__tests__/action-form.spec.tsx index ecf27d1e0f..85d804908e 100644 --- a/packages/react/src/ui-react/components/form/__tests__/action-form.spec.tsx +++ b/packages/react/src/ui-react/components/form/__tests__/action-form.spec.tsx @@ -7,7 +7,7 @@ import { StringParameter, ValidationStatus, } from "@batch/ui-common/lib/form"; -import { act, render, screen } from "@testing-library/react"; +import { act, render, screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import * as React from "react"; import { initMockBrowserEnvironment } from "../../../environment"; @@ -28,6 +28,26 @@ describe("Action form tests", () => { expect(await runAxe(container)).toHaveNoViolations(); }); + test("Can handle initialization errors", async () => { + const action = new PetDogAction({ + throwInitializationError: true, + }); + + let err: string | undefined; + render( + { + err = (e as Error).message; + }} + /> + ); + + await waitFor(() => { + expect(err).toEqual("Fake initialization error"); + }); + }); + test("Can submit and reset using the built-in buttons", async () => { userEvent.setup(); @@ -78,9 +98,12 @@ type PetDogFormValues = { dogName?: string; numberOfPets?: number; giveTreat?: boolean; + throwInitializationError?: boolean; }; class PetDogAction extends AbstractAction { + actionName = "PetDog"; + onPet?: (count: number) => void; private _initialValues: PetDogFormValues = {}; @@ -96,6 +119,9 @@ class PetDogAction extends AbstractAction { } async onInitialize(): Promise { + if (this._initialValues.throwInitializationError) { + throw new Error("Fake initialization error"); + } return this._initialValues; } diff --git a/packages/react/src/ui-react/components/form/action-form.tsx b/packages/react/src/ui-react/components/form/action-form.tsx index d2183ec45c..780c5e8a67 100644 --- a/packages/react/src/ui-react/components/form/action-form.tsx +++ b/packages/react/src/ui-react/components/form/action-form.tsx @@ -4,21 +4,84 @@ import { FormValues, ValidationSnapshot } from "@batch/ui-common/lib/form"; import { FormLayoutType } from "./form-layout"; import { FormButton, FormContainer } from "./form-container"; import { translate } from "@batch/ui-common"; -import { useLogger } from "../../hooks/use-logger"; export interface ActionFormProps { + /** + * The action associated with the form + */ action: Action; + + /** + * Controls how the form is rendered. The exact implementation depends on + * the configured environment. + */ layout?: FormLayoutType; + + /** + * If true, don't show the default reset button + */ hideResetButton?: boolean; + + /** + * If true, don't show the default submit button + */ hideSubmitButton?: boolean; + + /** + * The visible label of the form's reset button + */ resetButtonLabel?: string; + + /** + * The visible label of the form's submit button + */ submitButtonLabel?: string; + + /** + * Additional form buttons to add (beside the defaults) + */ buttons?: FormButton[]; + + /** + * Callback which is run after successful initialization + */ onActionInitialized?: () => void; + + /** + * Callback which is run when there is an unhandled error (either during + * initialization or action execution) + * + * @param error The object which was thrown (generally an error object) + */ + onError?: (error: unknown) => void; + + /** + * Callback which is run after successful action execution + */ + onExecuteSucceeded?: () => void; + + /** + * Callback which is run when action execution fails + * + * @param error The object which was thrown (generally an error object) + */ + onExecuteFailed?: (error: unknown) => void; + + /** + * Callback which is run on any form value change + * + * @param newValues The current values + * @param oldValues The previous values + */ onFormChange?: (newValues: V, oldValues: V) => void; + + /** + * Callback which is run whenever the action's form is validated + * + * @param snapshot Validation information, including the values of the + * form as they were when validate() was called. + */ onValidate?: (snapshot?: ValidationSnapshot) => void; - onSuccess?: () => unknown; - onFailure?: (error: unknown) => unknown; } export const ActionForm = ( @@ -28,14 +91,17 @@ export const ActionForm = ( action, layout, onFormChange, - onValidate, onActionInitialized, + onError, + onExecuteFailed, + onExecuteSucceeded, + onValidate, buttons, hideSubmitButton, hideResetButton, + submitButtonLabel, } = props; - const logger = useLogger("ActionForm"); const [loading, setLoading] = React.useState(true); const [submitting, setSubmitting] = React.useState(false); @@ -43,18 +109,34 @@ export const ActionForm = ( let isMounted = true; setLoading(true); - action.initialize().then(() => { - if (isMounted) { - setLoading(false); - if (onActionInitialized) { - onActionInitialized(); + action + .initialize() + .then(() => { + // Fulfilled + if (isMounted) { + setLoading(false); + if (onActionInitialized) { + onActionInitialized(); + } } - } - }); + }) + .catch((e) => { + // Rejected + action.logger.error( + `Failed to initialize action: ${ + e instanceof Error ? e.message : String(e) + }` + ); + if (onError) { + onError(e); + } + }); return () => { isMounted = false; }; + // Purposly don't include onError so the caller doesn't have to use useCallback() + // eslint-disable-next-line react-hooks/exhaustive-deps }, [onActionInitialized, action]); if (!action.isInitialized || loading) { @@ -78,23 +160,31 @@ export const ActionForm = ( ); if (success) { - if (props.onSuccess) { - props.onSuccess(); + if (onExecuteSucceeded) { + onExecuteSucceeded(); } } if (error) { - if (props.onFailure) { - props.onFailure(error); - } else { - logger.error( - "Error executing action: " + - (error instanceof Error - ? error.message - : String(error)) - ); + action.logger.error( + "Action execution failed: " + + (error instanceof Error + ? error.message + : String(error)) + ); + if (onExecuteFailed) { + onExecuteFailed(error); } } + } catch (e) { + action.logger.error( + `Unhandled error executing action: ${ + e instanceof Error ? e.message : String(e) + }` + ); + if (onError) { + onError(e); + } } finally { setSubmitting(false); } @@ -105,8 +195,7 @@ export const ActionForm = ( // Default reset (discard) button allButtons.push({ label: - props.submitButtonLabel ?? - translate("form.buttons.discardChanges"), + submitButtonLabel ?? translate("form.buttons.discardChanges"), disabled: submitting === true, onClick: () => { action.form.reset();