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

[PlaidLink OAuth] Add web redirect_uri to /bank-accounts page #6259

Merged
merged 73 commits into from
Dec 3, 2021

Conversation

nickmurray47
Copy link
Contributor

@nickmurray47 nickmurray47 commented Nov 7, 2021

cc @Dal-Papa

Details

This PR updates our Plaid integration to use OAuth for participating banks. The OAuth flow redirects the user away from new.e to their banking institution login then returns them to us once they are credentialed and we reinitialize the PlaidLink on our end.

The new param redirect_uri returns us to the bank-account route where the user can finish setting up their account. We reinitialize the PlaidLink by passing it the existing PlaidLinkToken and the receivedRedirectURI which includes the OAuth URI and the stateID.

Fixed Issues

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

Tests

Pull latest Web-S
Replace redirect_uri in src/libs/getPlaidLinkTokenParameters/index.js with:

{redirect_uri: `http://localhost:8080/${bankAccountRoute}`};
  1. Add a bank account to a workspace using Plaid.
  2. Search for "OAuth" and use the Plaid OAuth Sandbox bank account, Platypus OAuth (since not all banks in sandbox mode have OAuth setup).
  3. Click past all the forms without filling anything out but make sure to check off any necessary checkbox, i.e. the bank account and the Terms and Conditions at the end.
  4. Verify after signing off the Platypus OAuth sign-in that you're redirected back to the new.e page with the "Success" message. Click "Continue".
  5. Verify you're returned to new.e and prompted to select a specific account from your Platypus bank.
  6. Select one of the accounts to add and verify you are taken to the first step of the business bank account setup. (Warning: if you attempt to add a bank account too many times from one account you'll be locked out for a bit).
  7. Attempt to add another bank account using Plaid, but this time exit early and verify you're taken back to new.e's Bank Account Setup page.
  8. (Regression testing) Add a Chase business bank account using user_good and pass_good and make sure it gets added.

QA Steps

Same as the above, except using a participating OAuth bank like Chase.

Tested On

  • Web:
    • Dev
    • Chase
    • WF
    • CapOne
    • BofA
  • Mobile Web:
    • Chase
    • WF
    • CapOne
    • BofA
  • Desktop:
    • Dev
    • Chase
    • WF
    • CapOne
    • BofA
  • iOS:
    • Dev
    • Chase
    • WF
    • CapOne
    • BofA
  • Android:
    • Dev
    • Chase
    • WF
    • CapOne
    • BofA

Screenshots

PlaidLinkRedirect

Web

prod screenshots Screen Shot 2021-11-09 at 2 04 21 PM
screenshots Screen Shot 2021-11-09 at 2 04 21 PM Screen Shot 2021-11-09 at 2 04 49 PM Screen Shot 2021-11-09 at 2 35 55 PM Screen Shot 2021-11-09 at 2 36 06 PM Screen Shot 2021-11-10 at 2 13 35 PM

Mobile Web

prod screenshots Screen Shot 2021-11-09 at 2 04 21 PM

Desktop

prod screenshots
screenshots Screen Shot 2021-11-29 at 11 12 30 AM Screen Shot 2021-11-29 at 11 12 52 AM Screen Shot 2021-11-29 at 11 19 08 AM Screen Shot 2021-11-29 at 11 19 20 AM

iOS

prod screenshots

Android

prod screenshots

@nickmurray47 nickmurray47 self-assigned this Nov 7, 2021
@nickmurray47 nickmurray47 requested a review from a team as a code owner November 7, 2021 21:34
@MelvinBot MelvinBot requested review from MonilBhavsar and removed request for a team November 7, 2021 21:34
@nickmurray47 nickmurray47 changed the title [PlaidLink OAuth] Add web redirect_uri [WIP] [PlaidLink OAuth] Add web redirect_uri Nov 7, 2021
@nickmurray47
Copy link
Contributor Author

Doing a bit more testing and clean-up on Web then this should be ready for review soon

@marcaaron
Copy link
Contributor

I tried connecting oauth bank account and simulated error, upon cancelling the process, it went to connect bank account step but without any options to connect

Nice catch @MonilBhavsar. If we don't want to tackle handling the errors now we can do a follow up issue. As long as most users can restart the flow somehow if there's an unexpected error. Although, ideally we can handle this situation correctly..

@nickmurray47
Copy link
Contributor Author

As long as most users can restart the flow somehow if there's an unexpected error.

Yeah, believe most users can restart the flow just by clicking away.

If we don't want to tackle handling the errors now we can do a follow up issue.

Yeah, I think a follow-up issue would make sense and I have a decent idea of how we would catch the error using one of the PlaidLink's emitted events.

@nickmurray47
Copy link
Contributor Author

Just tested with Chase prod credentials on web and the flow worked super smooth! I'm going to add a checklist for each institution across all platforms to the PR description so we can keep track of progress.

@nickmurray47
Copy link
Contributor Author

nickmurray47 commented Dec 1, 2021

Quick report on status of Prod testing for this PR:

  • mWeb for Chase was also a success
  • @thienlnam tested out WF and CapOne for us but we weren't able to get past the loading screen, which I think is most likely related to Web-S API calls failing to get the plaidLinkToken. I think we can try again tomorrow with updating Web-S and tailing logs to make sure that token is populated. might just need to update Web-S to latest main
  • Ran into issues testing Chase on iOS too where we never fully redirect after signing into my bank account and I'm thinking that the native PlaidLink PR Update Plaid Link Component #6362 might have only worked in the sandbox environment. If I switch back to the main branch and go through the prod testing flow then I run into the same issue.

MonilBhavsar
MonilBhavsar previously approved these changes Dec 1, 2021
Dal-Papa
Dal-Papa previously approved these changes Dec 1, 2021
Copy link

@Dal-Papa Dal-Papa left a comment

Choose a reason for hiding this comment

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

One NAB

src/libs/getPlaidOAuthReceivedRedirectURI/index.js Outdated Show resolved Hide resolved
@bondydaa
Copy link
Contributor

bondydaa commented Dec 2, 2021

i checked off wells fargo for Web (web-e/web-s/mobile-e code bases were all tested and worked) and Mobile Web (new dot).

I left notes here for what happened on new dot ios https://expensify.slack.com/archives/C03TQ48KC/p1638394223326600?thread_ts=1637784112.158700&cid=C03TQ48KC

didn't yet try android, i'll do that tomorrow.

@nickmurray47
Copy link
Contributor Author

The last web prod bank passes QA!

@marcaaron
Copy link
Contributor

@nickmurray47 Tested Wells Fargo with @bondydaa's creds after the changes. Works great on both iOS and Android so I think we are good. And I think we can merge after the lint errors are fixed?

Also think if we do this we should create a follow up issue to ensure the testing is completed for the other platforms.

@nickmurray47
Copy link
Contributor Author

nickmurray47 commented Dec 2, 2021

Also think if we do this we should create a follow up issue to ensure the testing is completed for the other platforms.

Sounds good! Still testing a couple of flows for Chase and will tag you for final review once that is all done.

Edit: new testing tracking issue here https://github.com/Expensify/Expensify/issues/187280

@nickmurray47
Copy link
Contributor Author

Going to self-merge to move things along and so that things don't break on Web for the OAuth institutions (if they haven't broken already since we're past the deadline)

@nickmurray47 nickmurray47 merged commit 85799c2 into main Dec 3, 2021
@nickmurray47 nickmurray47 deleted the nmurray-plaidlink-oauth-update branch December 3, 2021 01:30
github-actions bot pushed a commit that referenced this pull request Dec 3, 2021
[PlaidLink OAuth] Add web redirect_uri to `/bank-accounts` page

(cherry picked from commit 85799c2)
marcaaron added a commit that referenced this pull request Dec 3, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Dec 7, 2021

🚀 Deployed to staging by @nickmurray47 in version: 1.1.17-8 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Dec 8, 2021

🚀 Deployed to production by @roryabraham in version: 1.1.18-3 🚀

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants