2256 error handling #2260

BCerki · 2024-09-20T18:03:05Z

card: https://github.com/orgs/bcgov/projects/122/views/2?pane=issue&itemId=80555060

This PR:

adds broader error handling to MultiStepBase (previously it was only handling response.error errors)
moved onSubmit error handling from individual form components to MultiStepBase (other error handling, e.g. fetching to populate a dropdown, is still handled in the form components and then the error is passed to MultiStepBase)
move step 1 handling to the MultiStepBase so we don't have to do extra handling in the OperationInformationForm. Step 1 of any workflow will always need to be handled specially because we won't ever have an id before create, but I didn't write a generic case--premature optimization evil and all that created new prop for handling step 1
moves success confirmation to MultiStepBase so that we don't have to do extra error handling in the RegistrationSubmission. Also because a success confirmation is a pretty standard form feature that makes sense in a generic component
moves isSubmitting logic to MultiStepBase
removes baseUrl param from our forms because the wireframes don't show a title in the workflow. It's still in the MultiStep component
Sonarcloud warnings are duplication in test files, I think they're ok to ignore

To test, raise an exception in each endpoint of the registration workflow, and you should see your exception appear on the front end when you hit the endpoint. You can also raise exceptions in /v2/operations/{uuid:operation_id} etc. to test that errors in the form components are passed to the multistepbase correctly

Updates after revisions:

removed showing Submitting... when isSubmitting is true--it was only randomly applied, and a loading spinner will be applied in another ticket
no special step 1 handling in MultiStepBase
success confirmation back to the last step (not necessarily generic to always show a confirmation)
new prop to optionally put the successful response in state to make it available for the parent component if needed (currently used in first and last steps)
removed try/catch from MultiStepBase per dev agreement session (let sentry/error boundary handle it)

BCerki · 2024-09-23T15:23:05Z

bc_obps/registration/api/v2/_operations/_operation_id/_registration/operation.py

@@ -36,41 +32,3 @@ def register_edit_operation_information(
    request: HttpRequest, operation_id: UUID, payload: OperationInformationIn
 ) -> Tuple[Literal[200], Operation]:
    return 200, OperationServiceV2.register_operation_information(get_current_user_guid(request), operation_id, payload)
-
-
-@router.put(


These endpoints were duplicated, cleaning them up in this PR

BCerki · 2024-09-23T17:35:16Z

bciers/apps/registration/app/components/operations/registration/FacilityInformationForm.tsx

@@ -143,12 +138,10 @@ const FacilityInformationForm = ({
    <MultiStepBase
      allowBackNavigation
      baseUrl={`/register-an-operation/${operation}`}
-      baseUrlParams="title=Placeholder+Title"


The wireframes don't show anything besides Register an Operation in the breadcrumbs, so we don't need a title placeholder at the moment

BCerki · 2024-09-23T20:43:43Z

bciers/apps/registration/app/components/operations/registration/OptedInOperationForm.tsx

      cancelUrl="/"
-      error={error}
      formData={formState}
      onChange={handleChange}
      onSubmit={handleSubmit}


Re lines 69-75ish: I was thinking of moving the isSubmitting state logic to the MultiStepBase. However, right now we've only got this submitting schema in a few of the form components, and I'm not sure if that's deliberate and shouldn't be changed. Why are we avoiding showing the read-only schema? Do we specifically need the show this schema, or would any loading component work? Thoughts?

Well I think this is a good idea to handle internally either way so we don't have to do this every time we reuse MultiStepBase. If you look inside FormBase we also handle it there to stop form data reset on submit. Though we may also need to have isSubmitting state in components where we use MultiStepBase for other reasons.

In regards to not showing the read only schema I think the comment could have been more helpful here though I just tested it and I think this is outdated. Before the rich text field titles were added, when you submitted the form it looked very buggy. The titles would disappear and you would just see a list of No momentarily, so we added that to stop that behaviour. At the time it was a nice improvement.

Though I just tried it again and that is fixed. Seeing it shift to the read only form still isn't ideal, though something that may be fixed if we remove the read only schema and the radio buttons will just be disabled.

I was able to move all the isSubmitting logic to MultiStepBase. This revealed some weird behaviour when switching steps that I think must have always been there: https://www.loom.com/share/99f835e99463426fb3a855205c7b43d9. I'll make a bug card

marcellmueller · 2024-09-24T15:29:09Z

bciers/libs/components/src/form/MultiStepBase.tsx

+    try {
+      const response = await onSubmit(data);
+      if (response?.error) {
+        setError(response?.error);
+        return;
+      }
+      // First step
+      if (step === 1) {
+        // The URL below is customized for the registration workflow. It can be generalized later if needed.
+        const nextStepUrl = `/register-an-operation/${response.id}/${step + 1}${
+          baseUrlParams ? `?${baseUrlParams}` : ""
+        }`;
+        router.push(nextStepUrl);
+        return;
+      }
+      // Middle steps
+      if (isNotFinalStep && baseUrl) {
+        const nextStepUrl = `${baseUrl}/${step + 1}${
+          baseUrlParams ? `?${baseUrlParams}` : ""
+        }`;
+        router.push(nextStepUrl);
+        return;
+      }
+      // Last step
+      if (!isNotFinalStep) {
+        setIsSuccessfullySubmitted(true);
+      }


This is supposed to be a general use form base component, we shouldn't have routes such as register-an-operation hardcoded in it. I get that this is trying to reduce duplication but I find this kind of confusing and it makes this component less reusable. I'm definitely pro automating a lot of this though some things make sense to handle in each form component.

Suggest keeping baseUrl prop required and remove most of this refactor except perhaps the last step block.

As discussed, the first step does need to be handled differently, I went with a prop.

marcellmueller

Great work on this PR, there are a lot of solid improvements! I do have some concerns about a couple of additions though and have added some suggestions.

Though I'm mindful that there may have been some issues that I've missed with my suggestions since I haven't been immersed in this as you have. Happy to hop on a call and discuss it! Let me know what you think, thanks :)

marcellmueller · 2024-09-25T16:00:37Z

bciers/libs/components/src/form/MultiStepBase.tsx

+  return isSuccessfullySubmitted ? (
+    successComponent
+  ) : isSubmitting ? (
+    <div>Submitting...</div>
+  ) : (


Why are we doing this in here? It feels like we are trying to solve problems that don't exist, the MultiStepBase component is starting to look overly complicated and hard to follow. Now that we have split up the forms into individual pages we can keep logic specific to those pages in those components.

This also dictates that we have to display a different component on submit (we may want to redirect to a page or do something else).

Now that we have split up the forms into individual pages we can keep logic specific to those pages in those components.

There isn't much logic that actually is specific to individual pages, which is why I've moved some things into MultiStepBase. Error handling and isSubmitting apply to all form pages. Showing a loading message <div>Submitting...</div> while isSubmitting is true also seemed general and like it should apply to all steps (previously it was on about half of them, and not the ones I'd expect like the ones with attachments).

Re successComponent, previously it was conditionally shown in the last step based on the value of isSubmitting. Since isSubmitting is now being managed by MultiStepBase, and showing a success message is a general form submission thing, it makes sense to me to have it in MultiStepBase. It could be a redirect to a different page instead, but this was how it was originally set up so I didn't change it.

Showing Submitting... in place of the entire component is really jarring, and especially noticeable after page 1 with file uploads. I believe we were asked about adding a loading spinner to the Save and continue button.

Looking at the RegistrationSubmissionForm component it is really clean and simple, I don't see the need to further complicate MultiStepBase. We can await the onSubmit response like in the example and setIsSubmitted and do what we need to in that component if reponse.ok or whatever.

marcellmueller · 2024-09-25T16:08:31Z

bciers/apps/registration/app/components/operations/registration/OperationInformationForm.tsx

      onSubmit={handleSubmit}
+      firstStepExtraHandling={(response) => {
+        // Since our form's route includes the operation's id, which doesn't exist until after the first step, we need to pass in a custom function that uses the response to generate a redirect url
+        const nextStepUrl = `/register-an-operation/${response.id}/${step + 1}`;
+        router.push(nextStepUrl);
+      }}


Suggested change

onSubmit={handleSubmit}

firstStepExtraHandling={(response) => {

// Since our form's route includes the operation's id, which doesn't exist until after the first step, we need to pass in a custom function that uses the response to generate a redirect url

const nextStepUrl = `/register-an-operation/${response.id}/${step + 1}`;

router.push(nextStepUrl);

}}

onSubmit={async (data: any) => {

const response = await handleSubmit(data);

const nextStepUrl = `/register-an-operation/${response.id}/${step + 1}`;

router.push(nextStepUrl);

}}

marcellmueller · 2024-09-25T16:11:39Z

bciers/libs/components/src/form/MultiStepBase.tsx

+    // Clear any old errors
+    setError(undefined);
+    try {
+      const response = await onSubmit(data);
+      if (response?.error) {
+        setError(response?.error);
+        return;
+      }
+      // In some cases, for the first step, we need to do something beyond simply redirecting to the baseUrl after successful onSubmit.
+      if (step === 1 && firstStepExtraHandling) {
+        firstStepExtraHandling(response);
+        return;
+      }
+      if (isNotFinalStep && baseUrl) {
+        const nextStepUrl = `${baseUrl}/${step + 1}${
+          baseUrlParams ? `?${baseUrlParams}` : ""
+        }`;
+        router.push(nextStepUrl);
+        return;
+      }
+      // Last step
+      if (!isNotFinalStep) {
+        setIsSuccessfullySubmitted(true);
+      }
+    } catch (err) {
+      // Handle any errors beyond response.error
+      setError("An unexpected error occurred. Please try again.");
+    } finally {


Suggested change

// Clear any old errors

setError(undefined);

try {

const response = await onSubmit(data);

if (response?.error) {

setError(response?.error);

return;

}

// In some cases, for the first step, we need to do something beyond simply redirecting to the baseUrl after successful onSubmit.

if (step === 1 && firstStepExtraHandling) {

firstStepExtraHandling(response);

return;

}

if (isNotFinalStep && baseUrl) {

const nextStepUrl = `${baseUrl}/${step + 1}${

baseUrlParams ? `?${baseUrlParams}` : ""

}`;

router.push(nextStepUrl);

return;

}

// Last step

if (!isNotFinalStep) {

setIsSuccessfullySubmitted(true);

}

} catch (err) {

// Handle any errors beyond response.error

setError("An unexpected error occurred. Please try again.");

} finally {

const submitHandler = async (data: any) => {

// Set isSubmitting to true to disable submit buttons and prevent multiple form submissions

setIsSubmitting(true);

// Clear any old errors

setError(undefined);

try {

const response = await onSubmit(data);

if (response?.error) {

setError(response?.error);

return response;

}

if (baseUrl) {

const nextStepUrl = `${baseUrl}/${step + 1}${

baseUrlParams ? `?${baseUrlParams}` : ""

}`;

router.push(nextStepUrl);

return response;

}

return response;

} catch (err) {

// Handle any errors beyond response.error

setError("An unexpected error occurred. Please try again.");

} finally {

setIsSubmitting(false);

}

};

For your consideration: I think this is getting overly complicated and hard to follow and will be difficult to maintain in the future. I think we should just not handle navigation if the baseUrl doesn't exist, and return the response to the parent component if they need that information.

With this solution I did notice a long Submitting... suspense issue. Though I think we need to remove suspense from this and add a spinner to the submit button for file uploads.

return the response to the parent component if they need that information

What do you mean by this? I've only ever done this with state, and that's not working here (maybe because it takes too long to get the response?) Or do you mean like your suggestion above, which does get me the response, but then we're back to the problem of having to handle errors in individual components:

onSubmit={async (data: any) => { const response = await handleSubmit(data); const nextStepUrl = `/register-an-operation/${response.id}/${step + 1}`; router.push(nextStepUrl); }}

Yeah that's what I meant, in your MultiStepBase changes we just do a plain return. In the suggestion I made we return reponse and can access it in the parent component as shown here.

Sorry I think I misunderstood you, though I think we can pass handleSubmit and to a .then to get the response from in the parent?

BCerki · 2024-09-26T15:38:44Z

bciers/libs/components/src/form/MultiStepBase.tsx

+      return;
+    }
+
+    if (setOnSubmitSuccessfulResponse) setOnSubmitSuccessfulResponse(response);


This is what I ended up with to give the parent component access to the response. It takes a moment for the response to arrive, so we need to use useEffect in the parent component

Why can't we use a useEffect without the extra prop? We should always have access to the response in the parent component.

We should always have access to the response in the parent component.
How do we have access to this without state? We need the response from the MultiStepBase's onSubmit (not the response from the parent's, because that doesn't include error handling)

Well we had it before this refactor, so currently this seems like a step down. There are many reasons why we would want the response in the parent (ie page 1 and submission). I'm working on a POC where we get the response in the parent component, though while I get a successful response I'm not getting the errors for some reason. Will let you know if I figure this out!

Hey! I created a simple POC for the submission page with console logs showing that we can access the response in the MultiStepBase and the parent component. Let me know what you think, perhaps there is something I didn't consider. Would love to get on a call and pair on this later :)

https://github.com/bcgov/cas-registration/pull/2292/files

marcellmueller

Awesome work, thanks for doing this and being receptive to my pestering review suggestions 😅 I enjoyed the back and forth and think this is a solid improvement!

Just a couple clean up comments!

marcellmueller · 2024-09-27T18:33:34Z

bciers/apps/registration/app/components/operations/registration/OperationInformationForm.tsx

-    }${`?title=${response.name}`}`;
-    router.push(nextStepUrl);
+    ).then((resolve) => {
+      console.log("!!!!!resolve", resolve);


marcellmueller · 2024-09-27T18:35:32Z

bciers/apps/registration/app/components/operations/registration/RegistrationSubmissionForm.tsx

@@ -63,19 +63,10 @@ const RegistrationSubmissionForm = ({
        <MultiStepBase
          allowBackNavigation
          baseUrl={`/register-an-operation/${operation}`}


Is baseUrl being used for the final page?

BCerki force-pushed the 2256-error-handling branch 2 times, most recently from 8ac9995 to 5b192bd Compare September 20, 2024 21:55

BCerki commented Sep 23, 2024

View reviewed changes

BCerki force-pushed the 2256-error-handling branch from 15bdb3f to f9b5476 Compare September 23, 2024 15:25

BCerki commented Sep 23, 2024

View reviewed changes

BCerki force-pushed the 2256-error-handling branch 4 times, most recently from 8b04ee2 to 759ced3 Compare September 23, 2024 20:36

BCerki commented Sep 23, 2024

View reviewed changes

BCerki force-pushed the 2256-error-handling branch 2 times, most recently from 0bd06cd to e0e7099 Compare September 23, 2024 21:26

BCerki marked this pull request as ready for review September 23, 2024 21:42

marcellmueller reviewed Sep 24, 2024

View reviewed changes

BCerki force-pushed the 2256-error-handling branch 3 times, most recently from 611b24d to 720a871 Compare September 25, 2024 15:24

marcellmueller reviewed Sep 25, 2024

View reviewed changes

BCerki force-pushed the 2256-error-handling branch 2 times, most recently from 0875924 to 9466fce Compare September 26, 2024 15:27

BCerki commented Sep 26, 2024

View reviewed changes

BCerki force-pushed the 2256-error-handling branch 3 times, most recently from 7633aa7 to a37c090 Compare September 27, 2024 16:36

marcellmueller approved these changes Sep 27, 2024

View reviewed changes

BCerki force-pushed the 2256-error-handling branch 2 times, most recently from a501f7d to 6c2762a Compare September 27, 2024 21:55

BCerki added 3 commits October 1, 2024 08:56

chore: remove duplicated endpoints

6be21d5

chore: error handling in MultiStepBase

b846d6d

test: vitests for error handling

76a4df0

BCerki added 3 commits October 1, 2024 08:56

chore: create prop to handle first step redirecting

53ad5b2

chore: move isSubmitting handling to MultiStepBase

811b0df

test: fix vitest

bf80e70

BCerki force-pushed the 2256-error-handling branch 3 times, most recently from 8ff8257 to 2b3bfda Compare October 1, 2024 18:19

chore: apply suggestions from code review

7c3236c

BCerki force-pushed the 2256-error-handling branch from 2b3bfda to 7c3236c Compare October 1, 2024 18:29

BCerki merged commit e88da4f into develop Oct 1, 2024
46 checks passed

BCerki deleted the 2256-error-handling branch October 1, 2024 18:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2256 error handling #2260

2256 error handling #2260

BCerki commented Sep 20, 2024 •

edited

Loading

BCerki Sep 23, 2024

BCerki Sep 23, 2024 •

edited

Loading

BCerki Sep 23, 2024

marcellmueller Sep 23, 2024

BCerki Sep 24, 2024 •

edited

Loading

marcellmueller Sep 24, 2024

BCerki Sep 24, 2024

marcellmueller left a comment

marcellmueller Sep 25, 2024

BCerki Sep 25, 2024

marcellmueller Sep 25, 2024

marcellmueller Sep 25, 2024

marcellmueller Sep 25, 2024 •

edited

Loading

BCerki Sep 25, 2024 •

edited

Loading

marcellmueller Sep 25, 2024

marcellmueller Sep 25, 2024

BCerki Sep 26, 2024

marcellmueller Sep 26, 2024

BCerki Sep 26, 2024

marcellmueller Sep 26, 2024

marcellmueller Sep 26, 2024

marcellmueller left a comment

marcellmueller Sep 27, 2024

marcellmueller Sep 27, 2024

BCerki Sep 27, 2024

2256 error handling #2260

2256 error handling #2260

Conversation

BCerki commented Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

BCerki Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BCerki Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcellmueller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcellmueller Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

BCerki Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcellmueller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BCerki commented Sep 20, 2024 •

edited

Loading

BCerki Sep 23, 2024 •

edited

Loading

BCerki Sep 24, 2024 •

edited

Loading

marcellmueller Sep 25, 2024 •

edited

Loading

BCerki Sep 25, 2024 •

edited

Loading