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

Prevent infinite loading in validateBankAccount #4165

Merged
merged 2 commits into from
Jul 21, 2021
Merged

Conversation

luacmartins
Copy link
Contributor

cc @chiragsalian

Details

Removes return statement that kept API calls in an infinite loading state.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/171371

Tests

  1. Create a new Workspace
  2. Follow these steps to add a PENDING account.
  3. When prompted for the 3 codes, follow these steps.
  4. Verify that the button text reappears.

QA Steps

Internal QA.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

web.mov

Mobile Web

Desktop

iOS

Android

@luacmartins luacmartins requested a review from a team July 21, 2021 17:46
@luacmartins luacmartins self-assigned this Jul 21, 2021
@MelvinBot MelvinBot requested review from marcaaron and removed request for a team July 21, 2021 17:46
@@ -553,7 +553,6 @@ function validateBankAccount(bankAccountID, validateCode) {
if (response.jsonCode === 200) {
Growl.show('Bank Account successfully validated!', CONST.GROWL.SUCCESS, 3000);
Navigation.dismissModal();
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

But I think the intention to early return here is correct as the non 200 case would be error handling logic.

So we just need to set loading to false and error to empty string maybe ''?

    Navigation.dismissModal();
    Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {loading: false, error: ''});
    return;
}

@luacmartins luacmartins requested a review from a team as a code owner July 21, 2021 18:37
@MelvinBot MelvinBot requested review from marcaaron and removed request for a team July 21, 2021 18:37
@luacmartins
Copy link
Contributor Author

Updated!

@luacmartins luacmartins changed the title remove return Prevent infinite loading in validateBankAccount Jul 21, 2021
@marcaaron marcaaron merged commit e275e26 into main Jul 21, 2021
@marcaaron marcaaron deleted the cmartins-finish-setup branch July 21, 2021 22:19
@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.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.79-5🚀

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

@MitchExpensify
Copy link
Contributor

MitchExpensify commented Jul 26, 2021

Prevent.infinite.loading.in.validateBankAccount.mov

It was weird to see "Get Started" as the button in Expensify Card when it should read "Finish Setup". That screen did not auto update after entering the validation amounts. I would have expected to see "Manage Cards" at that point. Also "Get Started" brought me to a blank modal before the hard refresh

@luacmartins
Copy link
Contributor Author

Thanks for the input @Mitch! I put up this PR to update the button text.

@isagoico
Copy link

@luacmartins @MitchExpensify Should this issue #4165 (comment) be treated as a deploy blocker or can we check off this PR from the list?

@luacmartins
Copy link
Contributor Author

I think it's fine to check it off the list since it solves the issue at hand and the remaining steps are only copy that needs to be updated. I put up another PR to update the button text.

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.80-2🚀

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

@kevinksullivan
Copy link
Contributor

@luacmartins I ran into the same experience after the Company Information step, where now I am stuck on a loading spinner.

image

So I haven't been able to get to the ValidateBankAccount step, but it looks like we may have similar problems at each step of the process... Should I open a separate GH? And would you want to pick it up since you just knocked this one out?

@kevinksullivan
Copy link
Contributor

Is there a reason this didn't closed if the PR is deployed to production?

@luacmartins
Copy link
Contributor Author

Not sure, for some reason I can't manually close it either...

@MitchExpensify
Copy link
Contributor

Weird, neither can I.

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.

6 participants