Skip to content
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

Remove Growl from VBA flow, for non-server errors #4875

Merged
merged 3 commits into from
Aug 27, 2021

Conversation

Julesssss
Copy link
Contributor

@Julesssss Julesssss commented Aug 27, 2021

Details

Removes the Growl for non-server errors, as these are displayed in the error Modal. Server 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

  • Create a new user
  • From the global create menu, tap 'New Workspace', then the 'Get Started' button
  • Tap 'Connect Manually' and use the data from this StackOverflow post to fill the Routing and Account number

1) Verify that Modal validation errors DO NOT show as a Growl

  • Using the stack overflow post, fill each field except 'Company Website', leave that as https://
  • Hit 'Save & Continue'
  • You should see the Company Website field highlighted red with an error message
  • You should see a Modal popup with an error message
  • You should NOT see a Growl with the same message

2) Verify that server errors DO show as a Growl

  • Continue from the end of test 1
  • Enter a valid website
  • Set the 'Phone Number' field to 12345678
  • Hit 'Save & Continue'
  • You should see a Growl with an error message
  • You should NOT see the field highlighted red
  • You should NOT see a Modal popup

2) Verify that unhandled validation errors DO show as a Growl

  • Continue from the end of test 2
  • Enter valid information in every field
  • Hit 'Save & Continue'
  • At the 'Requestor Information' step, enter first and second name only
  • Hit 'Save & Continue'
  • Fix each error one by one, ensure a Growl message shows for each
  • These will be changed to Modal validation in a separate issue

QA Steps

Run above tests

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screenshot 2021-08-27 at 14 33 51

Mobile Web

Desktop

Screenshot 2021-08-27 at 17 04 51

iOS

Simulator Screen Shot - iPhone 8 - 2021-08-27 at 17 07 29

Android

Screenshot_1630080877

@Julesssss Julesssss self-assigned this Aug 27, 2021
@Julesssss Julesssss requested a review from a team as a code owner August 27, 2021 13:45
@MelvinBot MelvinBot requested review from nkuoch and removed request for a team August 27, 2021 13:46
@nkuoch nkuoch merged commit 887778c into main Aug 27, 2021
@nkuoch nkuoch deleted the jules-removeGrowlFromVBAFlow branch August 27, 2021 15:02
@OSBotify
Copy link
Contributor

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

@Julesssss
Copy link
Contributor Author

I didn't assign PullerBear... 😕

I have another commit that I was testing, will raise a new PR for it now.

* @param {Boolean} isServerError
*/
function showBankAccountFormValidationError(error, isServerError) {
Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {error}).then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB, I'm not sure it's necessary to wait for this promise. IIRC we don't want to do it as a convention or something to avoid unnecessary synchronous code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @nkuoch in version: 1.0.88-3 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Sep 1, 2021

🚀 Deployed to production by @roryabraham in version: 1.0.90-2 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants