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

Error appears after enter test credentials on Plaid modal #5075

Closed
isagoico opened this issue Sep 3, 2021 · 24 comments
Closed

Error appears after enter test credentials on Plaid modal #5075

isagoico opened this issue Sep 3, 2021 · 24 comments
Assignees

Comments

@isagoico
Copy link

isagoico commented Sep 3, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Issue is failing PRs #4962 and #5029 (CC @marcaaron)

Action Performed:

  1. With the app instlalled on the device navigate to staging.new.expensify.com/bank-account/new
  2. Select the option to log in to bank account
  3. Enter the test credentials.

Expected Result:

User should be able to use the test credentials and redirected to the bank account selector.

Actual Result:

An internal server error occurred when using the test credentials.

Workaround:

Unknown.

Platform:

Where is this issue occurring?

  • Android
  • iOS

Version Number: 1.0.93-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

image

Bug5222363_Bank_Chase2.mp4

Expensify/Expensify Issue URL:

View all open jobs on GitHub

Assigned @marcaaron since he's the author of both PRs.

@isagoico isagoico added DeployBlockerCash This issue or pull request should block deployment Engineering Daily KSv2 labels Sep 3, 2021
@github-actions github-actions bot added Hourly KSv2 and removed Daily KSv2 labels Sep 3, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Sep 3, 2021

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@marcaaron
Copy link
Contributor

Is this only happening on Android? It was working fine for me and now suddenly isn't on staging. Since no customers are using this yet we can either revert this or merge it anyway. I'll leave this up to deployers to decide as it's Friday and I don't know if there are plans to deploy a release build or not.

@isagoico
Copy link
Author

isagoico commented Sep 3, 2021

I was only able to check on Android app since iOS bank-account/new link is redirecting to mWeb (mentioned here #5029 (comment)) I asked a tester to check on iOS with the new link and will let you know the results.

On the deploy release I think @roryabraham can answer that :)

@marcaaron marcaaron removed their assignment Sep 3, 2021
@isagoico
Copy link
Author

isagoico commented Sep 3, 2021

Same error is reproduced in iOS

@roryabraham
Copy link
Contributor

Given that:

  1. This is not yet a user-facing feature
  2. Before the offending PRs, the modal just wasn't rendering at all (so it's not really broken any worse than it was before)

I think we can demote this from deploy-blocker to regular bug.

@roryabraham roryabraham added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Sep 3, 2021
@marcaaron
Copy link
Contributor

So... I'm stumped on this one. But here's what I can say about it:

  • I re-tested on dev build + also with staging env variables and it works... totally fine
  • Staging web works.. well, fine
  • Built a release build for Android and it works... fine
  • Went back to try the Play Store beta again and... Internal error occurred

And that's my cue... have a good weekend everyone 😃

@marcaaron
Copy link
Contributor

Took another look today and I have verified that:

  1. Non-sandbox bank connections should work fine on production
  2. Probably there's some issue with our token endpoint and we are getting rate limited as described here

@kevinksullivan @MitchExpensify do you guys think this is worth prioritizing right now? It basically means that we can't test Plaid using the sandbox credentials on staging versions of the app until resolved.

@marcaaron
Copy link
Contributor

Did some more testing + looking at logs and noticed that we are actually using the production.plaid.com API endpoint (logs) while using the staging version of the app.

However, the staging version of the app should be pointing to staging-secure.expensify.com which should be using the sandbox.plaid.com endpoint. On the web version of the app the correct endpoint is used. However, it looks like that's not happening for the native app judging from the logs. Pretty confused about why this is happening. 😕

@marcaaron
Copy link
Contributor

Alright, now I just think maybe this has never worked and we haven't tested it on native. I'm pretty sure we are not using the "staging" environment variables for these builds. @AndrewGable or @Jag96 do either of you guys know?

@AndrewGable
Copy link
Contributor

Yes, I agree. I think TestFlight and Google Play Beta use production end points because we do not build another version, we just move them from TestFlight to production, changing the end points would require a new build

@marcaaron
Copy link
Contributor

Yes, I agree. I think TestFlight and Google Play Beta use production end points because we do not build another version, we just move them from TestFlight to production, changing the end points would require a new build

Ah ok that makes so much sense thanks for verifying. Mystery solved! I thought I had broken this 😄

It does seem like we're able to detect that we are in the "staging" environment (judging from the STG header badge) so maybe we can have secure calls switch to the staging server in these cases. Will spin something up for that.

@marcaaron marcaaron self-assigned this Sep 7, 2021
@Jag96
Copy link
Contributor

Jag96 commented Sep 7, 2021

It does seem like we're able to detect that we are in the "staging" environment (judging from the STG header badge) so maybe we can have secure calls switch to the staging server in these cases.

Yeah, you can check getEnvironment and betaChecker to see how we handle the badges, I think using the same logic would be the best way to handle that using existing code.

@marcaaron
Copy link
Contributor

marcaaron commented Sep 7, 2021

I guess thinking more on this though... is the intention of having a beta version of the app that things like setting up a bank account would work as they do in production? I'm not sure who has access to the beta/test flight versions of the app - but if I was a random public beta tester (or even an internal one) I might be confused about why the app isn't letting me set up a bank account (and the answer would be because we are using a sandbox).

@AndrewGable
Copy link
Contributor

is the intention of having a beta version of the app that things like setting up a bank account would work as they do in production?

Yes, I believe that is the intention.

@marcaaron
Copy link
Contributor

Got it. Maybe we can add a toggle in settings to switch to the staging secure server? I know there was some discussion about always having staging use the production endpoints. But it makes testing these flows with test credentials impossible. Just not sure how desperate we are to get this working. Will wait for @MitchExpensify @kevinksullivan to chime in before doing anything else.

@AndrewGable
Copy link
Contributor

How do we test this on old.expensify?

@marcaaron
Copy link
Contributor

On the staging website we just use the staging secure server.

On Mobile-Expensify we hit this page and would be staging or prod based on this which ultimately depends on this which we can toggle with a switch here.

@marcaaron
Copy link
Contributor

Just following up here. Ended up implementing something similar to what we have in Mobile-Expensify and hid it in the user preferences here:

2021-09-07_13-01-18

It should only show on the Google Play / Test Flight versions of the app as it's using the environment checker + basically just forces the API to use staging secure then saves it as a local preference.

@shawnborton
Copy link
Contributor

Here for the design eyes - can we get some more padding below the language selector and above the test preferences section? Thanks!

@shawnborton
Copy link
Contributor

Also I think the switch and the label to the left should be vertically centered.

@botify botify closed this as completed Sep 8, 2021
@botify
Copy link

botify commented Sep 8, 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.

@marcaaron
Copy link
Contributor

marcaaron commented Sep 9, 2021

@shawnborton ah shucks I'll create a follow up for those comments as this got merged before I could update. FWIW it's only going to be visible for beta users but I agree with the padding and centering comments 100%.

@botify
Copy link

botify commented Sep 9, 2021

🚀 Deployed to staging by @ctkochan22 in version: 1.0.95-2 🚀

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

@botify
Copy link

botify commented Sep 10, 2021

🚀 Deployed to production by @roryabraham in version: 1.0.96-0 🚀

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

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

No branches or pull requests

8 participants