Highlight all errors in VBA flow - Personal information and Additional information steps

[aldo-expensify]

Details

Changes:

• isValidIdentity became validateIdentity, which returns an object with the errors.
• Show all highlighted errors together in Personal information Step (/bank-account/personal-information)
• Show all highlighted errors together in Additional information Step (/bank-account/contract)
• Updated errors in reimbursementAccountPropTypes to accommodate beneficial owners errors in "Additional information Step (/bank-account/contract)"

Fixed Issues

$ #4785

We are working in this issue step by step. This PR updates the error handling specifically for the company information step in the VBA flow.

Tests AND QA Steps

Setup

1. Create account and verify it
2. Login in NewDot, create workspace and start the flow to add VBA
3. Do the first step "Add bank account" using any option (Log into your bank / Connect manually). Use data from step (2) or (3) of this SO.
4. Fill Company Step form with the available information from the SO, missing fields can be filled with anything that passes validations, then, click "Save & Continue"
5. You should be in the "Personal Information" step (/bank-account/personal-information)

Personal Information Step

1. Click "Save & continue" before filling anything, all fields are mandatory and all of them should have inline errors:

2. Start filling the different fields. Each field's inline errors should disappear as soon as they are changed. Use Alberta as first name and Charleson as last name.

3. Choose as "Date of birth" a recent date, example:2021-10-10

4. Click "Save & continue". The "Date of birth" should have the error "Requestors must be over 18 years old"

5. Choose as "Date of birth" a date older than 18 years, example: 2000-10-10

6. Click "Save & continue". If all fields pass the validation, we should get to the "Additional information" step (/bank-account/contract)

Additional information Step (beneficial owners)

1. Click "Save & continue", you should see the following errors:

2. Clicking any of the mandatory checkbox should remove the inline error of the clicked checkbox.
3. Check the checkbox next to "Somebody else owns more than 25%..."
4. Click the link below the new form with the text "Add another individual who owns..."
5. You should have two nested forms asking for information on the "Additional beneficial owner":
6. Scroll down and click "Save & continue", all mandatory fields should have inline errors:
7. Filling information on fields should clear the inline errors on them. Verify that filling a field in one "Additional beneficial owner" removes only the error for that nested form and not for any other "Additional beneficial owner" form.
8. Fill all mandatory fields with valid information and click "Save & continue".
9. We should be on the next step: Validate

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

[aldo-expensify]

Pending:

• Update ACHContract step
• Date of birth field has two different validations which end up in two different errors. With the newer way of handling errors, how do we handle this case?

App/src/libs/ValidationUtils.js

Lines 103 to 113 in 0f4abe1

	if (!isValidDate(identity.dob)) {
	showBankAccountFormValidationError(translateLocal('bankAccount.error.dob'));
	showBankAccountErrorModal();
	return false;
	}
	if (!isValidAge(identity.dob)) {
	showBankAccountFormValidationError(translateLocal('bankAccount.error.age'));
	showBankAccountErrorModal();
	return false;
	}

Maybe the errors object should have the translated error message instead of true as value. For example:

errors.dob = translateLocal('bankAccount.error.age');

[aldo-expensify]

@marcaaron , before continuing further, can I have your thoughts on this:

• Store string values in reimbursementAccount.errors instead of booleans. Why? because that would add support for fields that can have more than one error message. If you look at the ValidationUtils.js here, dob has two possible different errors

[marcaaron]

Store string values in reimbursementAccount.errors instead of booleans. Why? because that would add support for fields that can have more than one error message. If you look at the ValidationUtils.js here, dob has two possible different errors

That feels kind of like we are moving backwards and will have to change a lot to make it all consistent. It might be something we could consider for the design doc we are planning, but not something I'd probably not implement in this PR.

Rather than store the error text could we have the input handle multiple error keys somehow?

[aldo-expensify]

Rather than store the error text could we have the input handle multiple error keys somehow?

Yes, I would say is possible without too much effort and uglyness, it is just one field that has this problem.

I'll change them to boolean again and handle dob errors with different keys.

[marcaaron]

I know this is still in draft but just ran through to leave some comments since I took a look.

[aldo-expensify]

The error for missing State uses two lines, taking up too much space:

I think we should use a shorter text

[marcaaron]

Nice catch, since the feature is not being broadly promoted yet, I maybe would suggest creating a new issue for this and continuing to focus on solving the multiple errors issue.

[aldo-expensify]

@flodnv 's PR removed the error message "Requestors must be over 18 years old"

I'm introducing it again because of this slack comment, but changing the "requestor" wording.

[luacmartins]

LGTM and tests well. Great work!

[marcaaron]

Changes look good. There is just one place that needs to be addressed. The rest of my notes are NAB.

[marcaaron]

NAB, I think this is OK for now. But we're going to have to figure out a better way to handle individual fields with multiple possible errors in the future.

[marcaaron]

NAB, I think this logic could benefit from some brief documentation explaining that dob is unique from other fields as there are two kinds of validation happening.

Probably eventually we could have something like dob_invalid and dob_minimumAge where the front part indicates the fieldName and the next part indicates which error. Or move to a structure like

errors: {
    identity: {
        dob: {
            invalid: true,
            minimumAge: true,
        }
    }
}

[aldo-expensify]

I'm leaving the same comment on each part we do something with 'dob' related to multiple errors/validations

[aldo-expensify]

I like this structure 😬

errors: {
    identity: {
        dob: {
            invalid: true,
            minimumAge: true,
        }
    }
}

[nickmurray47]

+1 this looks like a good way of handling the multiple errors

[marcaaron]

Testing now and everything looks good except "First name" "Last name" and "Personal address" fields on the beneficial owners step look to be not required when a form is submitted without any info?

I'm not sure that's right. I think we need this information? But it would be good to have someone from ops verify.

[marcaaron]

I think we should just go ahead and add a isRequiredFulfilled for each of those checks since I can't think of why they would not be required. 🤔

[marcaaron]

We can also do this in a follow up.

[marcaaron]

Another weird thing is that even though I've entered no date of birth I see this:

Which implies that a valid date has been entered in the first place.

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[aldo-expensify]

Another weird thing is that even though I've entered no date of birth I see this:

Which implies that a valid date has been entered in the first place.

I'll look into it

[aldo-expensify]

Testing now and everything looks good except "First name" "Last name" and "Personal address" fields on the beneficial owners step look to be not required when a form is submitted without any info?

I'm not sure that's right. I think we need this information? But it would be good to have someone from ops verify.

Asked in #ops, Katie also thinks this should be required information.

https://expensify.slack.com/archives/C6PHQ660K/p1633368748485700?thread_ts=1633368702.484800&cid=C6PHQ660K

[luacmartins]

LGTM and tests well! Thanks @aldo-expensify!

[OSBotify]

🚀 Deployed to staging by @marcaaron in version: 1.1.4-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[OSBotify]

🚀 Deployed to production by @chiragsalian in version: 1.1.5-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅