Add Validation to BankAccount + CompanyStep + better handling for Plaid

[marcaaron]

cc @roryabraham

Details

Adds validation to forms in the BankAccount / CompanyStep

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/167761

Tests

1. Try to break the manual account form + company information step in various ways
2. Verify helpful error messages appear in the form of growls.

QA Steps

No QA

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

[roryabraham]

One NABish thing I'm noticing is that hitting Enter doesn't submit the form.

[roryabraham]

Not sure if this is intentional, but if the password is wrong and everything else is "correct", we just display an "Unknown error" growl. Might be a better UX to just say that the password is wrong.

[roryabraham]

Great work as always! I had a few suggestions, but no blockers (I guess except removing the TODO). Up to you if you want to act on any of the other suggestions.

[nickmurray47]

reviewing in a minute! sorry, have been on a long candidate call this morning

[marcaaron]

Thanks for the review @roryabraham I'm looking into these two now

• The password is wrong and everything else is "correct", we just display an "Unknown error" growl. Might be a better UX to just say that the password is wrong.
• Enter doesn't submit the form.

[marcaaron]

@roryabraham can you clarify how you got to the "Unknown error"? I'm seeing this ->

So I think maybe you are running into an unknown error 😅

[marcaaron]

Ready for another review.

[nickmurray47]

noticed a few things from the old e.com bank account setup:

Doesn't look like we have phone number validation for the CompanyStep right now - is that by choice?
I also like the placeholder that we had for the date format and the dropdown for the industry classification code (although I'm guessing that will prolly be a later polish step).

[marcaaron]

Doesn't look like we have phone number validation for the CompanyStep right now - is that by choice?

@nickmurray47 Mainly because they are also caught and thrown in PHP e.g. the phone is checked here

I also like the placeholder that we had for the date format

The placeholder is a great idea since it's not obvious what format we are preferring for that one so I'll add it now! Thanks!

and the dropdown for the industry classification code (although I'm guessing that will prolly be a later polish step)

The industry code selector is not in scope for this project, but there are plans to eventually migrate that over.

[nickmurray47]

sounds good!

also, was running into the same error that @roryabraham pointed out. Not sure if you or Rory's already seen this but I think it's because we throw an error from Web-S if a user has too many errors in 24 hours, throwIfThrottled. I noticed my account has the throttle NVP set and I can't progress further. Not sure if this is worthy of addressing now.

[marcaaron]

also, was running into the same error that @roryabraham pointed out. Not sure if you or Rory's already seen this but I think it's because we throw an error from secure if a user has too many errors in 24 hours, throwIfThrottled. I noticed my account has the throttle NVP set and I can't progress further. Not sure if this is worthy of addressing now.

Hmm I'd actually expect us to be handling that here...

https://github.com/Expensify/Expensify.cash/blob/c1536a83ef628f27dcae5d0320bfba16d7bff58e/src/pages/ReimbursementAccount/ReimbursementAccountPage.js#L122-L136

I think we should maybe not block on this, but I'll create a follow up to verify that's all working correctly.

[marcaaron]

Alright so I tried to look into this comment here -> #3686 (comment)

And set that NVP + saw an error message. If that's your issue @nickmurray47 then you should be seeing this now:

Can you verify with me whether you see that the next time you open the modal?

[nickmurray47]

hmmm it seems like I'm still getting this generic error message. Perhaps I'm holding something wrong.

[marcaaron]

Ok, so there's not a ton for me to go off of here... but I can suggest we try to debug (or just Log::info() something for) these lines maybe we can figure out which error is getting hit and why?

Are y'all using the recommended test credentials for setting up a bank account?

@roryabraham any thoughts on this? This isn't happening for me so I'm inclined to move forward with this.

[nickmurray47]

Are y'all using the recommended test credentials for setting up a bank account?

Using the test credentials for the OPEN bank account. I also agree we can move forward with the PR and circle back to this error.

[marcaaron]

Using the test credentials for the OPEN bank account.

Ok thanks, I just ran through again with that flow and didn't see any errors.

[roryabraham]

Don't think we need to block on this – clearly it's an edge case and we don't need perfection right now.

[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.

[OSBotify]

🚀 Deployed to staging in version: 1.0.73-4🚀

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

[OSBotify]

🚀 Deployed to production in version: 1.0.74-0🚀

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