Not all errors are outlined in the VBA flow

[kevinksullivan]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Navigate to /bank-account/company or any other step in the VBA flow
2. Enter incorrect information in all fields
3. Press Enter key

Expected Result:

Every field that has an error should have an error message and red outline associated with it. This is supposed to occur in addition to the generic modal that will instruct users to look for the highlighted issues (PR) (CC @chiragsalian)

Actual Result:

Only the final incorrect entry is outlined with an error messages, and other fields that either failed a check or are in the incorrect format are not outlined at all.

Platform:

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Notes/Photos/Videos:
(note that not all fields are not filled out in this video, but that is being fixed by #4748)

129963638-5cbcbc94-e725-47c2-a15b-cab3d9aefd6f.mov

@MitchExpensify should we take this internal to get it done quickly?

[marcaaron]

I have some time and can grab this.

[marcaaron]

Every field that has an error should have an error message and red outline associated with it. This is supposed to occur in addition to the generic modal that will instruct users to look for the highlighted issues

At the moment there is a growl that shows a single message. When there are multiple messages what is it supposed to say?

[aldo-expensify]

Maybe if there is more than one error the growl could change to a generic message like: 'There are errors in the form'

[marcaaron]

Thanks @aldo-expensify I hashed this out with @kevinksullivan 1:1 and forgot to update here.

We are going to get rid of the growls since they don't add anything that is not achieved by the inline error message + confirm modal.

[aldo-expensify]

makes sense! 👍

[marcaaron]

@kevinksullivan, this issue is made somewhat complicated by the fact that some of these errors are also coming from the server and they aren't translated at all (which we should probably fix). I'm not sure if we want to prioritize those or not. But shouldn't be too bad to come up with a solution.

[marcaaron]

Another question about this...

Presently, since there is only one error we only need to worry about clearing a single error when that field is "touched" (a.k.a. value is changed).

Now that we will show multiple errors - what should we do when just one of them is "touched"? I'm not sure if the original intention when we added the inline errors was to have them clear on touch or not (but we did that).

So, what should we do here? Clear only the error on the touched field? Clear all errors when one field is touched?

[marcaaron]

Errors in question:

https://github.com/Expensify/Web-Secure/blob/0396dc03542de0db071b0db2ba7c2e7c732de5c1/lib/BankAccountAPI.php#L994-L1000

[aldo-expensify]

The issue#4730 came back to life and there is a comment requesting the same changes that this issue is addressing: #4730 (comment)

IMH, that issue#4730 which is a DeployBlockerCash shouldn't include those changes because it is not a regression. I understand that the app was showing the error one by one before.

[marcaaron]

Ah yes, good eye @aldo-expensify thanks! I agree not a deploy blocker since we know this behavior should not work yet.

[marcaaron]

Alright, chatted 1:1 with @jasperhuangg who did the 1.0 version of this for moving error messages from growls to inline and he pointed out that the growl component was originally left in so that users who have scrolled past an error would be able to see it. That makes sense to me, but I think we can solve this problem by just prompting with a confirm modal when there's an error - and also do this anywhere we were doing a Growl.

We also talked about the server errors which are getting thrown and consumed directly by the front end - which reminds me that we should probably also pop open the "confirm" modal when those get thrown. Ultimately, whatever strategy we settle on for form error handling we should try to use everywhere. It feels like we're still figuring that out on the design side, but this should give us some lessons to lean on.

Also chatted a bit about whether it would be a good time to look into a form library and decided that we can wait for now as there are probably a lot of opinions on how it should work + well want to develop (or use) something that works for our design, Onyx architecture, etc. At the same time, we don't want to leave too many inconsistent patterns around or else we'll be reinventing the wheel each time we create a new form - but given the urgency of N6 seems like an acceptable trade-off.

So, I think this issue can be broken into a few steps:

Web-Secure changes

• Update the errors that are being directly consumed by the front end so that they have a unique jsonCode (which will require handling that code in both Old and New Dot) and also add extra commentary in Web-Secure that they are used in New Expensify so that we don't alter them accidentally.

New Expensify

• Create constants in New Expensify for these so that we can serve the proper translation for the error e.g. CONST.BANK_ACCOUNT.ERROR.INVALID_PHONE
• Take the various calls to Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {error: ''}) and change them instead to a unique Onyx key for that form with an object value where each key corresponds to an individual form element ID. So something like CompanyStep will start using a ONYXKEYS.COMPANY_STEP_ERRORS key and store any errors related to an individual form element.
• When a front-end error is thrown the validate() method will note which fields have failed and the merge into Onyx an object with keys of form element id and boolean values. When a back-end error is thrown we will also set this object instead of an error string. This means that the API handling in BankAccounts.js will have a more direct knowledge of which form elements they are interacting with vs. setting random error strings and having the component figure out what to do with this.
• We'll then need to update all the things looking at this.props.reimbursementAccount.error and change it so we start looking at this.props.companyStepErrors instead and can easily check for an individual form field error in the CompanyStep component.
• When a form field is touched (changes) we will clear a single field by updating the Onyx object.
• Since each time we would use a Growl we want instead to show a ConfirmModal the ConfirmModal component should be moved to ReimbursementAccountPage so that we don't need to add a confirm modal to each individual view.

Follow Up:

• Let's consider a form library. Either of our own design or 3rd party.

Alternative idea:

• Another idea to individual Onyx keys for form errors is to turn this.props.reimbursementAccount.error into this.props.reimbursementAccount.errors. I thought about this for a bit, but it will require an Onyx change if we want to update sub keys e.g. removing only reimbursementaAccount.errors.something without modifying the underlying object is not really supported. We can merge objects, but not remove deeply nested keys. I also don't love how this would leave form error states coupled with the rest of the bank account information. When we come up with a more universal solution to handling form state + their errors then we will probably need to redo this anyway so I don't think we should overthink it.

[MitchExpensify]

Looks good @marcaaron!

Just want to confirm to be 100% that this issue will capture these changes:

1. Removed all growls from VBA flow (We're using center modals for all instead - GH - on production)
2. All fields returning an error get highlighted.

[marcaaron]

Yep, that's the plan!

[marcaaron]

Thought I'd get this done in time but turned out to be a fairly complex issue + requires a careful refactor.

I'm getting called in to do allocations so I'm going to unassign myself for now - although I'd like to manage this issue from a distance if it gets picked up before I'm done.

I'll update the previous post with the breakdown of how we should handle server errors. But we discussed the Web-Secure error handling a bit in this Slack thread: https://expensify.slack.com/archives/C03TQ48KC/p1629840632329800

[aldo-expensify]

Hey @marcaaron , I would be interested in continuing with this task. Just one thing to clarify: when the request hits the back end, we will still send a single error, we won't try to change this to return multiple errors from the backend.

[MitchExpensify]

we won't try to change this to return multiple errors from the backend.

Does this mean we wouldn't be able to highlight more than one field?

That'd be great if you could grab this @aldo-expensify ! thanks a million

[aldo-expensify]

we won't try to change this to return multiple errors from the backend.

Does this mean we wouldn't be able to highlight more than one field?

That'd be great if you could grab this @aldo-expensify ! thanks a million

There are some validations that are done in the front and some in the back (after the request is sent). I understand we will show together the errors of all failed validations done in the front, but for validations done in the back, we would still be getting one at a time. Is this correct? I'm assuming that from the back we will still get one at a time because we have to maintain compatibility with the error handling of Web-Expensify front, right?

[marcaaron]

for validations done in the back, we would still be getting one at a time. Is this correct?

Yes, because the back end can only return a single error and not multiple errors.

[aldo-expensify]

I'm dropping this so someone with more experience can get it done quicker!

[jasperhuangg]

I'm interested in picking this up, but I'm busy with figuring out my schedule with school, allocations, and a few ongoing PRs. If things clear up in a few days I'll get to work on this!

[MitchExpensify]

Sounds good @jasperhuangg! Let us know when you can.

[kevinksullivan]

Perhaps we can split these issues out:

1. Remove the growl (blocker for N6)
2. Make sure all errors have an outline (not a blocker - since the flow still works fine and will continue to highlight one error when one or more exist)

@marcaaron I assume is a quick fix that we can get done, whereas 2 is what makes this more challenging. Is that correct?

[marcaaron]

I assume is a quick fix that we can get done, whereas 2 is what makes this more challenging. Is that correct?

The "challenging" part is that we have 3 different feedback mechanisms for handling errors and errors being set in random places with a very inconsistent pattern + probably more errors in the back end that are not handled at all. We should clean that stuff up regardless of how this work is split up.

This issue feels complicated because we keep pivoting. We didn't plan out the error handling UI initially then decided it should be different... then we decided it should be different again...

Maybe we can take a step back and think about the various errors that can be thrown by the API or validated in the front end and then map out how they should be presented to the user what happens to them when the user interacts with a field, etc. That will help inform the technical design so we don't have to keep hacking changes in...

While that happens, here's a rough idea on how we could re-prioritize this...

PR 1:
Replace the growls with a single confirm modal moved to ReimbursementAccount (there is one generic one in CompanyStep and also the existing owners one - and I think we should just have one central "confirm modal" in ReimbursementAccount that can handle all of these errors). When we set a single "error" string in Onyx then we can show the inline error and confirm modal. I'm not totally sure how this will work for things that don't have inline errors. Should the confirm modal show more specific text in those cases?

PR 2:
I have issues with how the inline errors are working at the moment since we are using string comparison to determine whether they should show + there is a lot of duplication in the code. We should address this next, but it must be cleaned up before we get to the "show multiple errors" part of this.

PR 3:
Handle multiple errors inline and show the generic confirm modal when there is at least one error.

[kevinksullivan]

This issue feels complicated because we keep pivoting. We didn't plan out the error handling UI initially then decided it should be different... then we decided it should be different again...

Yeah, this is a great call. We really didn't discuss error handling during the HL phase, and in hindsight that would've helped avoid a lot of this.

I'm not totally sure how this will work for things that don't have inline errors. Should the confirm modal show more specific text in those cases?

I believe there will always be inline errors, since even if a certain API check fails we will point the user to correcting information relevant to said check.

So PR 1 feels the most like a blocker, since we're throwing a lot of errors at once currently. PR 2 and PR 3 will really clean up what we threw together over the recent days, which is necessary. With that said I think we should consider those fast followers to N6, and make sure we spend the time needed to get them right. Would you agree @marcaaron ?

[MelvinBot]

@marcaaron, @aldo-expensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[aldo-expensify]

On n6-hold, should we change the priority to weekly? so it doesn't go overdue all the time while we are not working on it.

[chiragsalian]

Yes i updated it. Once its off n6-hold feel free to bring it back to daily if needed.

[pecanoro]

Should we remove the hold on this one since it's part of polishing n6?

[marcaaron]

@aldo-expensify any update on this one?

[aldo-expensify]

@aldo-expensify any update on this one?

I just updated the PR with the main branch resolving a bunch of conflicts. I would like your opinion on this before continuing further 😬

[aldo-expensify]

PR ready for review for all errors outlined together in steps:

• Personal information
• Additional information

[kevinksullivan]

@aldo-expensify just checking here, is this done?

[aldo-expensify]

This PR covering the "Personal information" and "Additional information" (Beneficial owners) step was just merged. @marcaaron will address some final details in a new PR.

Details:
#5259 (comment)
#5259 (comment)

After that, I think we should be done about "Showing all errors together" in all VBA steps.

[marcaaron]

I think we're done here this is on production now. There are some smaller issues getting wrapped up but the VBA flow errors should be 👌