Highlight all errors together in BankAccount step #5041

aldo-expensify · 2021-09-03T00:07:31Z

cc @marcaaron

Brought some of the functionality implemented in this dead PR

Details

Changes:

Added missing inline error for field Account Number
"Save & continue" button is only disabled if there is some field empty or the checkbox is unchecked
Support highlighting multiple errors at the same time
Clear error of field being edited

These changes apply only to BankAccount step in VBA flow.

Fixed Issues

$ #4785

Tests AND QA Steps

Start with a non-public domain account that has not yet set up a workspace or bank account
Create a new workspace by tapping on the green FAB button
Navigate to /bank-account by selecting the workspace and tapping the "Get Started" button
Tap button to add account manually: Connect manually
Expected result: the "Save & continue" button should be disabled.
Enter an invalid Routing number, i.e.: 1 (front-end validation)
Expected result: the "Save & continue" button should remain disabled.
Enter an invalid Account number, i.e.: 1 (front-end validation)
Click "Save & continue"
Expected result: You should get the error modal with the generic message: Please double check any highlighted fields and try again. and see both fields with inline highlighted errors
Focus on any of the inputs, type something more
Expected result: the inline error and highlight should disappear only for that input.
Enter 33333332234 as Routing number and 111 as the Account number
Click "Save & continue"
Expected result: We should see the loading transition (data is sent to back-end), and then we should see the form again with an inline error for Routing number and the error modal with the generic message.

Screen.Recording.2021-09-02.at.5.50.11.PM.mov

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Support highlighting multiple errors Only disable submit button if there are empty fields Clear error of field being edited

aldo-expensify · 2021-09-03T00:33:34Z

Sorry, making this [WIP] again because I didn't consider the case of the error coming from the server.

aldo-expensify · 2021-09-03T00:42:01Z

Update: When the backend returns a validation error for the 'Routing number' we highlight correctly the field again.

marcaaron

This looks great! I just had mostly style things to add + a question about how we should handle the Onyx workaround.

src/libs/actions/BankAccounts.js

marcaaron · 2021-09-03T00:25:41Z

src/libs/actions/BankAccounts.js

@@ -822,7 +832,7 @@ function setupWithdrawalAccount(data) {
 }

 function hideBankAccountErrors() {
-    Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {error: '', existingOwnersList: ''});
+    Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {error: '', existingOwnersList: '', errors: {}});


Probably you'll get conflicts if my other PR is merged, but I noticed that existingOwnersList isn't actually used anywhere but existingOwners is - so let's make sure this bug doesn't get reintroduced here.

Should I change existingOwnersList for existingOwners now? or we handle it once there is conflict?

I didn't know you could do a search like that in Github, cool feature.

I guess it doesn't matter yet, but just pointing out the code you have here does nothing so probably we should update it.

Are we actually clearing the errors if we do errors: {}? This will need to be errors: null I think?

Are we actually clearing the errors if we do errors: {}? This will need to be errors: null I think?

You are right, I was clearing this the wrong way. 🤦 How does it work in the case of arrays? do they get merged (concatenated) or replaced?

The answer: the old array gets replaced by the new array (no merging/concatenating)

Actually, I'm just going to set it to null and the will clear the key. When we get the errors we get them using lodashGet(this.props, ['reimbursementAccount', 'errors'], {}). I feel like setting this to null and then {} is uglier and doesn't make things much better.

src/pages/ReimbursementAccount/BankAccountStep.js

…-account-vba

aldo-expensify · 2021-09-03T05:41:51Z

Update: pushed some changes addressing comments

…-account-vba

aldo-expensify · 2021-09-03T19:25:56Z

Update:

hideBankAccountErrors sets the errors key to null
Added comment for setBankAccountFormValidationErrors

marcaaron

LGTM

marcaaron · 2021-09-03T20:56:11Z

Gonna merge this so we can keep the ball rolling. I'm going to spend some time looking into the Onyx improvement.

OSBotify · 2021-09-03T20:56:59Z

✋ 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 · 2021-09-04T01:01:19Z

🚀 Deployed to staging by @marcaaron in version: 1.0.93-2 🚀

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

OSBotify · 2021-09-08T21:18:02Z

🚀 Deployed to production by @yuwenmemon in version: 1.0.94-1 🚀

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

Highlight all errors together in BankAccount step

736bafe

Support highlighting multiple errors Only disable submit button if there are empty fields Clear error of field being edited

aldo-expensify self-assigned this Sep 3, 2021

aldo-expensify marked this pull request as ready for review September 3, 2021 00:29

aldo-expensify requested a review from a team as a code owner September 3, 2021 00:29

MelvinBot requested review from AndrewGable and removed request for a team September 3, 2021 00:29

aldo-expensify changed the title ~~Highlight all errors together in BankAccount step~~ [WIP] Highlight all errors together in BankAccount step Sep 3, 2021

Update backend routing number validation error

221575c

aldo-expensify changed the title ~~[WIP] Highlight all errors together in BankAccount step~~ Highlight all errors together in BankAccount step Sep 3, 2021

marcaaron requested changes Sep 3, 2021

View reviewed changes

aldo-expensify added 2 commits September 2, 2021 22:36

Document functions and minor refactor

8914e07

Merge branch 'main' of github.com:Expensify/App into aldo_errors-bank…

4799b91

…-account-vba

Merge branch 'main' of github.com:Expensify/App into aldo_errors-bank…

bd3f3e4

…-account-vba

aldo-expensify mentioned this pull request Sep 3, 2021

Routing number error shows a modal with specific message + inline error #5031

Closed

Fix clearing errors, add comment, minor refactor

e84e018

Add documentation for clearErrorAndSetValue

16dcd4c

marcaaron approved these changes Sep 3, 2021

View reviewed changes

marcaaron merged commit 4bb007b into main Sep 3, 2021

marcaaron deleted the aldo_errors-bank-account-vba branch September 3, 2021 20:56

OSBotify mentioned this pull request Sep 3, 2021

Deploy Checklist: New Expensify 2021-09-03 #5086

Closed

14 tasks

aldo-expensify mentioned this pull request Sep 14, 2021

Not all errors are outlined in the VBA flow #4785

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Highlight all errors together in BankAccount step #5041

Highlight all errors together in BankAccount step #5041

aldo-expensify commented Sep 3, 2021 •

edited

Loading

aldo-expensify commented Sep 3, 2021

aldo-expensify commented Sep 3, 2021

marcaaron left a comment

marcaaron Sep 3, 2021

aldo-expensify Sep 3, 2021

marcaaron Sep 3, 2021

marcaaron Sep 3, 2021

aldo-expensify Sep 3, 2021 •

edited

Loading

aldo-expensify Sep 3, 2021

aldo-expensify commented Sep 3, 2021

aldo-expensify commented Sep 3, 2021

marcaaron left a comment

marcaaron commented Sep 3, 2021

OSBotify commented Sep 3, 2021

OSBotify commented Sep 4, 2021

OSBotify commented Sep 8, 2021

Highlight all errors together in BankAccount step #5041

Highlight all errors together in BankAccount step #5041

Conversation

aldo-expensify commented Sep 3, 2021 • edited Loading

Details

Fixed Issues

Tests AND QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

aldo-expensify commented Sep 3, 2021

aldo-expensify commented Sep 3, 2021

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron Sep 3, 2021

Choose a reason for hiding this comment

aldo-expensify Sep 3, 2021

Choose a reason for hiding this comment

marcaaron Sep 3, 2021

Choose a reason for hiding this comment

marcaaron Sep 3, 2021

Choose a reason for hiding this comment

aldo-expensify Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

aldo-expensify Sep 3, 2021

Choose a reason for hiding this comment

aldo-expensify commented Sep 3, 2021

aldo-expensify commented Sep 3, 2021

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron commented Sep 3, 2021

OSBotify commented Sep 3, 2021

OSBotify commented Sep 4, 2021

OSBotify commented Sep 8, 2021

aldo-expensify commented Sep 3, 2021 •

edited

Loading

aldo-expensify Sep 3, 2021 •

edited

Loading