-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure Requestor Info validation shows error growl #4879
Conversation
I am currently unable to test mobile web for either Android or Web (slack discussion). So please verify that platform if possible. |
src/libs/actions/BankAccounts.js
Outdated
*/ | ||
function showBankAccountFormValidationError(error, isServerError) { | ||
function showBankAccountFormValidationError(error, isUnhandledError) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB, maybe something like shouldGrowl
be better here since we aren't making assumptions about the kind of error?
src/libs/actions/BankAccounts.js
Outdated
*/ | ||
function showBankAccountFormValidationError(error, isServerError) { | ||
function showBankAccountFormValidationError(error, isUnhandledError) { | ||
Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {error}).then(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we want to clean this up now, but I think maybe we should. IIRC we should not be waiting for Onyx.merge()
promises to resolve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and tests well!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @marcaaron in version: 1.0.88-3 🚀
|
@Julesssss We will test this via the |
Hi @isagoico, that makes sense. Is this how you have tested all other Bank account PRs up to now? |
Yes! we have used this method for those PRs. |
Thanks, then the plan sounds fine. I'll just tag @MitchExpensify and @kevinksullivan to ensure we test internally in addition. |
🚀 Deployed to production by @roryabraham in version: 1.0.90-2 🚀
|
Details
Follow up to this PR, which was accidentally merged early.
Adds the Growl for non-server errors on the Requestor Info VBA step. These errors are not yet displayed in the error Modal, so we need to continue to show with the Growl.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/175331
Tests
0) Test Setup
1) Verify that Modal validation errors DO NOT show as a Growl
https://
2) Verify that server errors DO show as a Growl
12345678
2) Verify that unhandled validation errors DO show as a Growl
QA Steps
Run above tests
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android