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

Update Plaid Link Component #6362

Merged
merged 8 commits into from
Nov 22, 2021
Merged

Update Plaid Link Component #6362

merged 8 commits into from
Nov 22, 2021

Conversation

TomatoToaster
Copy link
Contributor

@TomatoToaster TomatoToaster commented Nov 18, 2021

Details

Refactors this component as functional so we can use the Plaid Hooks.

Fixed Issues

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

Tests

Same as QA done locally and android

QA Steps

This should be tested on iOS

  1. Create a new user and a workspace in NewDot.
  2. Go to workspace settings and Connect a Bank
  3. Follow the screenshots to go through the plaid oauth setup for PlatypusOAuth bank. You can enter empty fields for Purple OAuth forms like in the screenshots.
  4. Reach the final screenshot where you back to the expensify app

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Screenshots take a lot of the screen so click here to expand/collapse them!

iOS

image

image

image

image

image

image

image

image

(scroll to the bottom)
image

image

image

image

@TomatoToaster TomatoToaster self-assigned this Nov 18, 2021
@TomatoToaster TomatoToaster marked this pull request as ready for review November 20, 2021 00:01
@TomatoToaster TomatoToaster requested a review from a team as a code owner November 20, 2021 00:01
@MelvinBot MelvinBot requested review from jasperhuangg and removed request for a team November 20, 2021 00:01
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

Gonna test this now, but LGTM. There's a small improvement that could be made in a follow up. Just going to give this a test and then merge if everything looks good.

const emitter = new NativeEventEmitter(nativeModule);
this.listener = emitter.addListener('onEvent', this.onEvent.bind(this));
listener = emitter.addListener('onEvent', (event) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it will work, but I noticed there's also this in the docs

https://github.com/plaid/react-native-plaid-link-sdk#to-receive-onevent-callbacks

@marcaaron
Copy link
Contributor

Works great on iOS. Testing Android next.

@TomatoToaster
Copy link
Contributor Author

I think we should CP this to Staging so it goes live sooner. But feel free to remove the label if you think otherwise.

@github-actions
Copy link
Contributor

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@jasperhuangg jasperhuangg removed their request for review November 22, 2021 17:13
@marcaaron
Copy link
Contributor

Having some trouble testing Android. I'm not able to create workspace policies for some reason. I think probably there is some issue with my dev environment - working through it now.

@marcaaron
Copy link
Contributor

Ok fixed the dev environment issue, but not able to complete the link handoff on Android. I'm just getting this spinner after going through the OAuth flow and it fails with endless spinner when trying to put me back into the app.

2021-11-22_07-27-38

What should we try next @TomatoToaster ?

@marcaaron
Copy link
Contributor

marcaaron commented Nov 22, 2021

I think this is probably not working because we are not yet passing the Android package version name?

@TomatoToaster
Copy link
Contributor Author

Hmm which android package do you think we should update?

@TomatoToaster
Copy link
Contributor Author

Updated steps to test this OAuth flow on iOS only for now.

@marcaaron
Copy link
Contributor

Sorry, to clarify, we need to specify an android_package name when calling Web-Secure which is com.expensify.chat. And then pass that when calling Plaid_GetLinkToken. But I'm not sure if this has been added on production yet - maybe @Dal-Papa knows?

@marcaaron
Copy link
Contributor

FWIW added this and it works on Android for me but we can do this in a follow up.

diff --git a/src/libs/API.js b/src/libs/API.js
index d7d8c6d6b..1d5017601 100644
--- a/src/libs/API.js
+++ b/src/libs/API.js
@@ -883,7 +883,7 @@ function Wallet_GetOnfidoSDKToken() {
  * @returns {Promise}
  */
 function Plaid_GetLinkToken() {
-    return Network.post('Plaid_GetLinkToken', {}, CONST.NETWORK.METHOD.POST, true);
+    return Network.post('Plaid_GetLinkToken', {android_package: 'com.expensify.chat'}, CONST.NETWORK.METHOD.POST, true);
 }

marcaaron
marcaaron previously approved these changes Nov 22, 2021
@TomatoToaster
Copy link
Contributor Author

@marcaaron I also updated the podfile/ios package information. I had the podfile and workspace file change when I ran pod install and it wouldn't work on the emulator without these changes. Let me know if I shouldn't include them.

I was able to test this on iOS and it worked with the plaid event emitter changes.

@marcaaron
Copy link
Contributor

@marcaaron I also updated the podfile/ios package information. I had the podfile and workspace file change when I ran pod install and it wouldn't work on the emulator without these changes. Let me know if I shouldn't include them.

Hmm seems fine to include or not? I don't know much about podfile tbh but unsure why main should end up different from a local build - maybe the changes were not committed from a previous PR.

@marcaaron
Copy link
Contributor

I guess one other thing is that I'm a little confused how this is working and wonder if it will actually work on staging.

If you read this you'll see it mentions linking to a "blank page" -> https://plaid.com/docs/link/oauth/#ios-sdk-react-native-ios

We added this here:

"/partners/plaid/oauth_ios"

But are not passing a redirect_uri here

So why does this still work?

I think (but I'm not sure) it's because we don't need the redirect to actually deliver us to that page - just drop us back into the app (which has already handled the success response). Only thing is... I don't see where we registered /partners/plaid/oauth_ios?

@marcaaron
Copy link
Contributor

Also interesting as this blurb says we cannot test "app to app" flows in the sandbox -> https://plaid.com/docs/link/oauth/#app-to-app-authentication

But the Platypus OAuth option seems to work fine 🤔

@marcaaron
Copy link
Contributor

I'm good to merge this because I don't think it will break anything, but I think to actually test whether this works we need to try a connection that uses OAuth (and I have a good feeling it won't work).

When using the "Platypus OAuth" test we don't ever really leave the app on iOS so we don't need a redirect_uri - but for other cases we will need to send the universal link as a param which is why we added it to the apple-app-site-association.json.

@marcaaron marcaaron merged commit 1a8b4fa into main Nov 22, 2021
@marcaaron marcaaron deleted the amal-plaid-link-update branch November 22, 2021 20:04
github-actions bot pushed a commit that referenced this pull request Nov 22, 2021
Update Plaid Link Component

(cherry picked from commit 1a8b4fa)
@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by @marcaaron in version: 1.1.15-21 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @marcaaron in version: 1.1.16-0 🚀

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

@isagoico
Copy link

@marcaaron @TomatoToaster Is this QAble by us? I don't think we have access to the Platypus bank account credentials yet. Let us know if this is internal or not 😬 Thanks in advance!

@marcaaron
Copy link
Contributor

Ah can we please test that Chase bank credentials are still working for now?

Also, there are no credentials for Platypus bank account just enter anything and it should succeed.

@isagoico
Copy link

isagoico commented Nov 24, 2021

Ohh ok! Tested it and it works on iOS for the Platypus Bank! 🎉 Also chase bank account credentials are still working

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.16-10 🚀

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

1 similar comment
@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.16-10 🚀

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.

4 participants