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

[No QA] Catch http errors that don't trigger .then() #4400

Merged
merged 1 commit into from
Aug 5, 2021

Conversation

stitesExpensify
Copy link
Contributor

@stitesExpensify stitesExpensify commented Aug 3, 2021

Details

We were not catching errors that hit .catch() as opposed to .then when adding user's bank accounts. This led to endless spinners which is a bad user experience. This adds .catch and throws an error when this happens.

Fixed Issues

$ #4321

Tests

  1. Update your .env file to cause 404s on secure calls e.g. EXPENSIFY_URL_SECURE=https://secuasdfre.expensify.com.dev/
  2. Go to http://localhost:8080/bank-account
  3. Click "Manual"
  4. Disconnect from wifi
  5. Fill out the forms and click continue
  6. Make sure you see an error and are sent back to the main page

QA Steps

Ping me to test this as you need a special account set up

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

2021-08-03_11-23-13.mp4

@stitesExpensify stitesExpensify requested a review from a team as a code owner August 3, 2021 22:54
@stitesExpensify stitesExpensify self-assigned this Aug 3, 2021
@MelvinBot MelvinBot requested review from aldo-expensify and removed request for a team August 3, 2021 22:55
@aldo-expensify
Copy link
Contributor

If we use 'Log Into Your Bank' disconnecting wifi it spins forever too, is that out of the scope of the fix? I noticed also that when I use 'Log Into Your Bank' I have a back button on top, which can save me from the spinning forever problem. Is it intended that the back button becomes hidden when we submit the form in the 'Connect Manually' flow?

Screen Shot 2021-08-04 at 10 14 40 AM

I'm still trying to get this message consistently in my local dev environment... it works sometimes and sometimes it seems like it keeps retrying forever to make the connection (disabling wifi / chrome tools option) without getting to the catch. Did you setting yourself offline using chrome dev tools?

Screen Shot 2021-08-04 at 10 28 50 AM

@aldo-expensify
Copy link
Contributor

I'm getting the error message after I connect again to the internet. While it is disconnected it looks like it keeps retrying and retrying, shouldn't we give up at some point and notify the user that there is no internet connection?

Screen.Recording.2021-08-04.at.11.04.31.AM.mov

The first time I tried it, it worked like you show in your video, immediately returning and showing the error message, but now it is behaving like this. Could this be due to some environment configuration that I have? I'm using ngrok and EXPENSIFY_URL_COM=https://www.expensify.com.dev/ (in case that info is of any use)

@stitesExpensify
Copy link
Contributor Author

Hmm. To be honest with you I wrote those test steps without doing them. I was causing the error by using ngrok and just clicking manual. I assumed disconnecting from the internet would do the same thing, but it looks like a different issue.

To clarify, it is testing correctly if you are just connected to the internet and using ngrok?

@aldo-expensify
Copy link
Contributor

Hmm. To be honest with you I wrote those test steps without doing them. I was causing the error by using ngrok and just clicking manual. I assumed disconnecting from the internet would do the same thing, but it looks like a different issue.

To clarify, it is testing correctly if you are just connected to the internet and using ngrok?

Connected to the internet and using ngrok I get the following:

Screen.Recording.2021-08-04.at.8.11.22.PM.mov

I get an error because the transit number is bad, but it doesn't block the interface.

@stitesExpensify
Copy link
Contributor Author

Interesting haha. I when I had ngrok on I was getting a 404 error which was causing the catch to get hit.

I updated with better test steps, basically you just need to go into your .env and mess up your secure url like EXPENSIFY_URL_SECURE=https://secuasdfre.expensify.com.dev/.

I'm going to mark this noQA since we can't do that on staging

@stitesExpensify stitesExpensify changed the title Catch http errors that don't trigger .then() [No QA] Catch http errors that don't trigger .then() Aug 5, 2021
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

Tested this w/ the updated steps and it works for me, I didn't have to disconnect my wifi, just changing EXPENSIFY_URL_SECURE (or SECURE_NGROK_URL w/ ngrok running) resulted in the error showing.

@stitesExpensify stitesExpensify merged commit f117c99 into main Aug 5, 2021
@stitesExpensify stitesExpensify deleted the stites-catchAPIFailures branch August 5, 2021 19:22
@OSBotify
Copy link
Contributor

OSBotify commented Aug 5, 2021

✋ 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

OSBotify commented Aug 6, 2021

🚀 Deployed to staging in version: 1.0.82-8🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 failure ❌
🍎 iOS 🍎 success ✅
🕸 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