Action execution can now display validation errors

packages/bonito-core/src/action/__tests__/action.spec.ts

@@ -2,7 +2,7 @@ import { initMockEnvironment } from "../../environment";
 import { createForm, Form, ValidationStatus } from "../../form";
 import { StringParameter } from "../../form/string-parameter";
 import { delay, mergeDeep } from "../../util";
-import { AbstractAction } from "../action";
+import { AbstractAction, ActionExecutionResult } from "../action";
 
 describe("Action tests", () => {
     beforeEach(() => initMockEnvironment());
@@ -44,7 +44,7 @@ describe("Action tests", () => {
 
         // Executing adds the sender
         const result = await action.execute();
-        expect(result.formValidationStatus.level).toEqual("ok");
+        expect(result.validationStatus.level).toEqual("ok");
         expect(action.message).toEqual("Hello planet from Contoso");
 
         // Can change form values and execute again
@@ -125,8 +125,26 @@ describe("Action tests", () => {
         expect(action.form.validationStatus?.level).toEqual("ok");
 
         // Execute action using form
-        await action.execute();
+        const successResult = await action.execute();
         expect(action.message).toEqual("Hello universe");
+        expect(successResult.success).toBe(true);
+        expect(successResult.error).toBeUndefined();
+        expect(successResult.validationStatus.level).toBe("ok");
+
+        // Execute again, but this time the execution will return a validation error
+        action.form.updateValue("subject", "invalid");
+        const failResult = await action.execute();
+        expect(failResult.success).toBe(false);
+        expect(failResult.error).toBeUndefined();
+        expect(failResult.validationStatus.level).toBe("error");
+
+        // Can fix the error and submit again
+        action.form.updateValue("subject", "valid subject");
+        const successResult2 = await action.execute();
+        expect(action.message).toEqual("Hello valid subject");
+        expect(successResult2.success).toBe(true);
+        expect(successResult2.error).toBeUndefined();
+        expect(successResult2.validationStatus.level).toBe("ok");
     });
 
     type HelloFormValues = {
@@ -203,14 +221,30 @@ describe("Action tests", () => {
             return new ValidationStatus("ok");
         }
 
-        async onExecute(formValues: HelloFormValues): Promise<void> {
-            return new Promise<void>((resolve) => {
+        async onExecute(
+            formValues: HelloFormValues
+        ): Promise<ActionExecutionResult> {
+            return new Promise<ActionExecutionResult>((resolve) => {
                 setTimeout(() => {
+                    const result: ActionExecutionResult = {
+                        success: true,
+                        validationStatus: new ValidationStatus("ok"),
+                    };
+
                     this.message = `Hello ${formValues.subject}`;
                     if (formValues.sender) {
                         this.message += ` from ${formValues.sender}`;
                     }
-                    resolve();
+
+                    if (formValues.subject === "invalid") {
+                        result.success = false;
+                        result.validationStatus = new ValidationStatus(
+                            "error",
+                            "Invalid subject"
+                        );
+                    }
+
+                    resolve(result);
                 }, 0);
             });
         }

packages/bonito-core/src/action/action.ts

@@ -23,6 +23,12 @@ export interface Action<V extends FormValues = FormValues> {
      */
     readonly isInitialized: boolean;
 
+    /**
+     * The result the last time the action was executed (or undefined if
+     * the action hasn't been executed yet)
+     */
+    readonly lastExecutionResult?: ActionExecutionResult;
+
     /**
      * Load data needed for the action and perform any other initialization.
      * Calls the onInitialize() callback.
@@ -70,14 +76,14 @@ export interface Action<V extends FormValues = FormValues> {
      *
      * @param formValues The form values to use
      */
-    onExecute(formValues: V): Promise<void>;
+    onExecute(formValues: V): Promise<ActionExecutionResult | void>;
 
     /**
      * Returns a promise that resolves when action execution
      * has finished. If there is no execution currently in-progress,
      * the promise will resolve immediately.
      */
-    waitForExecution(): Promise<void>;
+    waitForExecution(): Promise<ActionExecutionResult | void>;
 
     /**
      * Returns a promise that resolves when action initialization
@@ -90,7 +96,7 @@ export interface Action<V extends FormValues = FormValues> {
 export type ActionExecutionResult = {
     success: boolean;
     error?: unknown;
-    formValidationStatus: ValidationStatus;
+    validationStatus: ValidationStatus;
 };
 
 export abstract class AbstractAction<V extends FormValues>
@@ -112,12 +118,18 @@ export abstract class AbstractAction<V extends FormValues>
         return this._logger;
     }
 
+    private _lastExecutionResult?: ActionExecutionResult;
+
+    get lastExecutionResult(): ActionExecutionResult | undefined {
+        return this._lastExecutionResult;
+    }
+
     protected _form?: Form<V>;
 
     private _isInitialized: boolean = false;
 
     private _isExecuting = false;
-    private _executionDeferred = new Deferred();
+    private _executionDeferred = new Deferred<ActionExecutionResult>();
 
     private _isInitializing = false;
     private _initializationDeferred = new Deferred();
@@ -172,9 +184,13 @@ export abstract class AbstractAction<V extends FormValues>
         }
     }
 
-    waitForExecution(): Promise<void> {
+    waitForExecution(): Promise<ActionExecutionResult | undefined> {
         if (!this._isExecuting || this._executionDeferred.done) {
-            return Promise.resolve();
+            if (this._executionDeferred.done) {
+                return Promise.resolve(this._executionDeferred.resolvedTo);
+            } else {
+                return Promise.resolve(undefined);
+            }
         } else {
             return this._executionDeferred.promise;
         }
@@ -195,10 +211,10 @@ export abstract class AbstractAction<V extends FormValues>
     }
 
     async execute(): Promise<ActionExecutionResult> {
-        const executionResult: ActionExecutionResult = {
+        let executionResult: ActionExecutionResult = {
             success: false,
             // Default to error status in case an exception is thrown
-            formValidationStatus: new ValidationStatus(
+            validationStatus: new ValidationStatus(
                 "error",
                 "Failed to execute action"
             ),
@@ -223,17 +239,27 @@ export abstract class AbstractAction<V extends FormValues>
                 );
             }
 
-            executionResult.formValidationStatus = validationStatus;
+            executionResult.validationStatus = validationStatus;
             if (validationStatus.level === "error") {
                 // Validation failed - early out
+                this._lastExecutionResult = executionResult;
                 return executionResult;
             }
 
             try {
-                await this.onExecute(formValues);
-                executionResult.success = true;
+                const result = await this.onExecute(formValues);
+                if (result) {
+                    executionResult = result;
+                } else {
+                    executionResult.success = true;
+                }
             } catch (e) {
-                executionResult.error = e;
+                executionResult.error =
+                    e ?? "Failed to execute action " + this.actionName;
+                executionResult.validationStatus = new ValidationStatus(
+                    "error",
+                    `${String(e)}`
+                );
                 this.logger.warn("Action failed to execute:", e);
             }
         } finally {
@@ -242,12 +268,21 @@ export abstract class AbstractAction<V extends FormValues>
             // 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();
+            this._executionDeferred.resolve(executionResult);
 
             // Create a new deferred execution object since execution
             // can happen again and again
-            this._executionDeferred = new Deferred();
+            this._executionDeferred = new Deferred<ActionExecutionResult>();
         }
+
+        this._lastExecutionResult = executionResult;
+
+        if (!executionResult.success) {
+            this.form.forceValidationStatus(
+                this._lastExecutionResult.validationStatus
+            );
+        }
+
         return executionResult;
     }
 
@@ -255,7 +290,7 @@ export abstract class AbstractAction<V extends FormValues>
 
     abstract buildForm(initialValues: V): Form<V>;
 
-    abstract onExecute(formValues: V): Promise<void>;
+    abstract onExecute(formValues: V): Promise<ActionExecutionResult | void>;
 
     onValidateSync?(values: V): ValidationStatus;
 

packages/bonito-core/src/form/__tests__/form.spec.ts

@@ -425,6 +425,18 @@ describe("Form tests", () => {
         expect(form.validationStatus?.level).toEqual("ok");
         expect(form.validationStatus?.message).toBeUndefined();
 
+        // Can force a validation status externally
+        form.forceValidationStatus(
+            new ValidationStatus("error", "This is a forced error")
+        );
+        expect(form.validationStatus?.level).toBe("error");
+        expect(form.validationStatus?.message).toBe("This is a forced error");
+
+        // Running validation again clears the forced status
+        await form.validate();
+        expect(form.validationStatus?.level).toEqual("ok");
+        expect(form.validationStatus?.message).toBeUndefined();
+
         // When calling validate() multiple times in rapid succession,
         // the last call always wins
         form.updateValue("squareMiles", -1);

packages/bonito-core/src/form/form.ts

@@ -179,6 +179,14 @@ export interface Form<V extends FormValues> {
         handler: FormEventMap<V>[E]
     ): void;
 
+    /**
+     * Externally sets the validation status of the form. Note that the next
+     * time validation runs, this status will be cleared.
+     *
+     * @param status The validation to set.
+     */
+    forceValidationStatus(status: ValidationStatus): void;
+
     /**
      * Evaluate dynamic properties
      *

packages/bonito-core/src/form/internal/form-impl.ts

@@ -48,7 +48,11 @@ export class FormImpl<V extends FormValues> implements Form<V> {
     }
 
     get validationStatus(): ValidationStatus | undefined {
-        return this._validationSnapshot?.overallStatus;
+        // Use the forced status if it is defined
+        return (
+            this._forcedValidationStatus ??
+            this._validationSnapshot?.overallStatus
+        );
     }
 
     get entryValidationStatus(): {
@@ -65,6 +69,8 @@ export class FormImpl<V extends FormValues> implements Form<V> {
         true
     );
 
+    private _forcedValidationStatus?: ValidationStatus;
+
     _emitter = new EventEmitter() as TypedEventEmitter<{
         change: (newValues: V, oldValues: V) => void;
         validate: (snapshot: ValidationSnapshot<V>) => void;
@@ -278,6 +284,11 @@ export class FormImpl<V extends FormValues> implements Form<V> {
         if (!opts) {
             opts = defaultValidationOpts;
         }
+
+        // Clear any previously forced validation status each time validation
+        // runs
+        this._forcedValidationStatus = undefined;
+
         const snapshot = new ValidationSnapshot(this.values);
         const previousValidationInProgress =
             this._validationSnapshot.validationCompleteDeferred.done !== true;
@@ -333,8 +344,7 @@ export class FormImpl<V extends FormValues> implements Form<V> {
         snapshot: ValidationSnapshot<V>,
         opts: ValidationOpts
     ): ValidationSnapshot<V> {
-        // Call validateAsync() for each entry but don't block
-        // on completion
+        // Call validateSync() for each entry
         for (const entry of this.allEntries()) {
             if (entry instanceof AbstractParameter) {
                 const entryName = entry.name as ParameterName<V>;
@@ -436,6 +446,16 @@ export class FormImpl<V extends FormValues> implements Form<V> {
         return snapshot;
     }
 
+    /**
+     * Externally sets the validation status of the form. Note that the next
+     * time validation runs, this status will be cleared.
+     *
+     * @param status The validation to set.
+     */
+    forceValidationStatus(status: ValidationStatus): void {
+        this._forcedValidationStatus = status;
+    }
+
     /**
      * Checks to make sure the current validation snapshot is the most recent.
      * If not, sets the validation status to error with a message saying

packages/bonito-core/src/form/subform.ts

@@ -230,6 +230,10 @@ export class SubForm<
         return this.form.validateAsync(snapshot, opts);
     }
 
+    forceValidationStatus(status: ValidationStatus): void {
+        this.form.forceValidationStatus(status);
+    }
+
     async waitForValidation(): Promise<ValidationStatus | undefined> {
         return this.form.waitForValidation();
     }

packages/bonito-ui/src/components/form/__tests__/action-form.spec.tsx

@@ -1,5 +1,8 @@
 import { createForm } from "@azure/bonito-core";
-import { AbstractAction } from "@azure/bonito-core/lib/action";
+import {
+    AbstractAction,
+    ActionExecutionResult,
+} from "@azure/bonito-core/lib/action";
 import {
     BooleanParameter,
     Form,
@@ -92,6 +95,72 @@ describe("Action form tests", () => {
             numberOfPets: 1,
         });
     });
+
+    test("Validation errors are shown properly", async () => {
+        userEvent.setup();
+
+        const action = new PetDogAction({
+            dogName: "Baron Maximilian Augustus von Woofington",
+        });
+
+        render(<ActionForm action={action} />);
+
+        const submitButton = await screen.findByRole("button", {
+            name: "Apply",
+        });
+        expect(submitButton).toBeDefined();
+
+        await act(async () => {
+            await userEvent.click(submitButton);
+            await action.waitForExecution();
+        });
+        expect(action.form.validationStatus?.level).toBe("error");
+        expect(action.lastExecutionResult?.validationStatus.level).toBe(
+            "error"
+        );
+
+        const alerts = await screen.findAllByRole("alert");
+        expect(alerts.length).toBe(2);
+        // Validation error on input
+        expect(alerts[0].textContent).toBe(
+            "Dog names cannot be greater than 10 characters"
+        );
+        // Validation error on summary
+        expect(alerts[1].textContent).toBe(
+            "Dog names cannot be greater than 10 characters"
+        );
+
+        await act(async () => {
+            action.form.updateValue("dogName", "f1d0");
+            await userEvent.click(submitButton);
+            await action.waitForExecution();
+        });
+        // Action execution failure cascades to the form as a validation error
+        expect(action.form.validationStatus?.level).toBe("error");
+        expect(action.form.validationStatus?.message).toBe(
+            "Dog names cannot contain numbers"
+        );
+        expect(action.lastExecutionResult?.validationStatus.level).toBe(
+            "error"
+        );
+        expect(action.lastExecutionResult?.validationStatus.message).toBe(
+            "Dog names cannot contain numbers"
+        );
+
+        const alert = await screen.findByRole("alert");
+        // Only one validation error this time because the error isn't associated
+        // with a form entry (it came from the action execution)
+        expect(alert.textContent).toBe("Dog names cannot contain numbers");
+
+        // Reset back to initial values
+        const resetButton = await screen.findByRole("button", {
+            name: "Discard changes",
+        });
+        await userEvent.click(resetButton);
+        expect(action.form.values).toStrictEqual({
+            dogName: "Baron Maximilian Augustus von Woofington",
+        });
+    });
 });
 
 type PetDogFormValues = {
@@ -99,6 +168,7 @@ type PetDogFormValues = {
     numberOfPets?: number;
     giveTreat?: boolean;
     throwInitializationError?: boolean;
+    throwExecutionError?: boolean;
 };
 
 class PetDogAction extends AbstractAction<PetDogFormValues> {
@@ -132,6 +202,15 @@ class PetDogAction extends AbstractAction<PetDogFormValues> {
 
         form.param("dogName", StringParameter, {
             label: "Dog name",
+            onValidateSync: (value) => {
+                if (value && value.length > 10) {
+                    return new ValidationStatus(
+                        "error",
+                        "Dog names cannot be greater than 10 characters"
+                    );
+                }
+                return new ValidationStatus("ok");
+            },
         });
 
         form.param("numberOfPets", NumberParameter, {
@@ -169,9 +248,25 @@ class PetDogAction extends AbstractAction<PetDogFormValues> {
         return new ValidationStatus("ok");
     }
 
-    async onExecute(formValues: PetDogFormValues): Promise<void> {
+    async onExecute(
+        formValues: PetDogFormValues
+    ): Promise<ActionExecutionResult> {
+        if (formValues.dogName?.match(/[0-9]/)) {
+            return {
+                success: false,
+                validationStatus: new ValidationStatus(
+                    "error",
+                    "Dog names cannot contain numbers"
+                ),
+            };
+        }
+
         if (this.onPet && formValues.numberOfPets != undefined) {
             this.onPet(formValues.numberOfPets);
         }
+        return {
+            success: true,
+            validationStatus: new ValidationStatus("ok"),
+        };
     }
 }

packages/bonito-ui/src/components/form/list-form-layout.tsx

@@ -87,7 +87,10 @@ const ListForm = <V extends FormValues>(props: ListFormProps<V>) => {
                 </Stack>
 
                 {messageBarType && (
-                    <div style={{ paddingTop: 8 }}>
+                    <div
+                        data-testid="form-validation-message"
+                        style={{ paddingTop: 8 }}
+                    >
                         <MessageBar messageBarType={messageBarType}>
                             {form.validationStatus?.message}
                         </MessageBar>