Add Personal Bank Account via Plaid flow #2746

marcaaron · 2021-05-07T23:59:56Z

Details

This issue is sort of holding on https://github.com/Expensify/Expensify/issues/163233 where we'll add the Android app package name to the Plaid dashboard. I don't have the access level to do this so until that is done Android cannot be tested on production - but should be testable on dev and staging which are both using the sandbox. This is done now so we can test on release builds.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/161736
Fixes https://github.com/Expensify/Expensify/issues/149395

Notes on testing this on native platforms

This flow is only accessible via deep link at the moment so we'll need to follow instructions here

If you are using ngrok on iOS simulator make sure you also have certs on your simulator device. I wasted some time trying to figure out why iOS couldn't call secure.

Notes on testing on Android

In order to access local web-secure on Android it's necessary to create a separate endpoint for web-secure. I created a whole new one by following instructions here but a temporary one should work too.

After setting up a new "secure" ngrok url to use I ran this and everything worked fine.

ngrok http -hostname=secure-expensify-marc.ngrok.io --host-header rewrite secure.expensify.com.dev:443

Tests

Navigate to /add-bank-account
Enter "Chase" for the bank
Enter the test credentials user_good/pass_good
Select "Plaid Checking"
Enter Password
Tap Continue
Verify that the alert pops up and says the account has been added
Verify in expensify.com.dev that the account has been added

QA Steps (Internal)

QA is largely the same for staging. Making it internal because it may be tricky to get the deep links to work.
I can also do the QA for this (or help out) if whoever is assigned asks.

Tested On

Web
Mobile Web
Desktop / Not Accessible via Desktop
iOS
Android

Screenshots

Web

Mobile Web

Desktop

N/A

iOS

Android

marcaaron · 2021-05-10T20:13:41Z

android/gradle.properties

@@ -10,7 +10,7 @@
 # Specifies the JVM arguments used for the daemon process.
 # The setting is particularly useful for tweaking memory settings.
 # Default value: -Xmx10248m -XX:MaxPermSize=256m
-# org.gradle.jvmargs=-Xmx2048m -XX:MaxPermSize=512m -XX:+HeapDumpOnOutOfMemoryError -Dfile.encoding=UTF-8
+org.gradle.jvmargs=-Xmx2048m -XX:MaxPermSize=512m -XX:+HeapDumpOnOutOfMemoryError -Dfile.encoding=UTF-8


This seems necessary after adding the Plaid SDK there's some more information here about this:

https://medium.com/google-developers/faster-android-studio-builds-with-dex-in-process-5988ed8aa37e

TL;DR is that without this enabled the android app wouldn't build at all.

What version of the JDK do we target? MaxPermSize was removed in JDK 8 so that param might not do anything

JDK 8 I believe, I don't know much about Java though I just uncommented this line out based on the advice in that blog post.

add the add bank account page Add PlaidLink on web and ability to add bank account add react plaid link library derp revert plist changes Add Android configs Add secure NGROK option add note about dependency add secure ngrok option temp disable flipper add nativeModules files Fix up propTypes Use React.Fragment add translation stuff add comments add doc

marcaaron · 2021-05-11T00:44:47Z

@francoisl this PR is kind of huge so going to add a couple more reviewers to it 😄

marcaaron · 2021-05-11T00:52:09Z

ios/Podfile

  post_install do |installer|
-    flipper_post_install(installer)
+    # flipper_post_install(installer)


Oops forgot to remove this guy. It needed so that iOS will build on dev for now. 🙃

marcaaron · 2021-05-11T01:00:58Z

android/build.gradle

@@ -3,7 +3,7 @@
 buildscript {
    ext {
        buildToolsVersion = "29.0.2"
-        minSdkVersion = 16
+        minSdkVersion = 21


This appears to be a requirement in order to use the Plaid SDK on Android

https://github.com/plaid/react-native-plaid-link-sdk#3-configure-gradle

I'm hoping it's OK ? @Jag96

We talked about doing this a while back but this is definitely fine, the issue is here: https://github.com/Expensify/Expensify/issues/149395

marcaaron · 2021-05-11T01:04:29Z

src/components/PlaidLink/index.js

+        props.onSuccess({publicToken, metadata});
+    }, []);
+
+    const {open, ready, error} = usePlaidLink({


We normally should not need to use hooks (and kind of discourage it), but the Plaid library we're using for React web here really only offers this option to launch the flow programmatically.

marcaaron · 2021-05-11T01:08:22Z

src/libs/actions/BankAccounts.js

+            currency: 'USD',
+            fieldsType: 'local',
+            plaidAccessToken,
+            isForStripeConnect: false,


@ctkochan22 wondering if you can help us figure out whether we need all of this stuff. I loosely based these parameters on what we send when adding a PBA on e.com.

Don't need isForStripeConnect. The rest are necessary I think. But when it comes to additionalData, more info is always good I think. What you have now is good.

francoisl

Looks good, a lot of it is over my head though. Still trying to test but I think my .env file is messed up and causing me errors, going to look into that.

francoisl · 2021-05-11T20:43:02Z

android/gradle.properties

@@ -10,7 +10,7 @@
 # Specifies the JVM arguments used for the daemon process.
 # The setting is particularly useful for tweaking memory settings.
 # Default value: -Xmx10248m -XX:MaxPermSize=256m
-# org.gradle.jvmargs=-Xmx2048m -XX:MaxPermSize=512m -XX:+HeapDumpOnOutOfMemoryError -Dfile.encoding=UTF-8
+org.gradle.jvmargs=-Xmx2048m -XX:MaxPermSize=512m -XX:+HeapDumpOnOutOfMemoryError -Dfile.encoding=UTF-8


What version of the JDK do we target? MaxPermSize was removed in JDK 8 so that param might not do anything

francoisl · 2021-05-11T21:10:17Z

src/pages/AddBankAccountPage.js

+    }
+
+    /**
+     * Get list of bank accounts


Curious why this method gets a doc but not the one below?

Only because it has a return value. I think at one point we were enforcing method docs but now only making them if there is a param or return that needs to be documented or some additional comments.

francoisl · 2021-05-11T21:26:08Z

Conflicts

marcaaron · 2021-05-12T16:20:30Z

Updated and fixed the conflicts.

marcaaron · 2021-05-14T16:44:12Z

@shawnborton I was thinking we could do styling stuff in a follow up with a contributor if that works for ya. Mostly just trying to scaffold some stuff out here.

shawnborton · 2021-05-14T16:48:55Z

Cool, that works for me. Though it is a bit strange that our default component wouldn't have the default font/color baked into it? But yeah, we can investigate that elsewhere.

Jag96 · 2021-05-14T17:23:25Z

src/CONFIG.js

+
+// Throw errors on dev if config variables are not set correctly
+if (ENVIRONMENT === CONST.ENVIRONMENT.DEV) {
+    if (!useNgrok && expensifyURL.endsWith('dev') && !expensifyURLSecure.endsWith('dev')) {


In my .env file, I have a trailing slash for both the EXPENSIFY_URL_COM and the EXPENSIFY_URL_SECURE values, so this logic doesn't throw the error if I have EXPENSIFY_URL_COM=https://www.expensify.com.dev/ and EXPENSIFY_URL_SECURE=https://secure.expensify.com/

🤦 fixed this up and changed it to an String.includes()

ctkochan22

Looks great

francoisl

There's a conflict

OSBotify · 2021-05-17T21:13:41Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

isagoico · 2021-05-18T20:21:43Z

@Jag96 Heyo! Will this be internally tested? Let me know so we can check it of the list. 🙇

marcaaron · 2021-05-18T20:40:20Z

This should be QA testable. With the exception of desktop since there are no deep links.

marcaaron · 2021-05-18T20:40:58Z

I'm happy to takeover the testing for this. I think I intended it to be Internal just because of how weird it is.

marcaaron · 2021-05-18T20:49:18Z

Web is working fine 🎉
Android doesn't work 🤔
Will test iOS next...

I think no need to block on this as long as the rest of the app works. Nothing is using the flow yet.

marcaaron · 2021-05-18T20:57:03Z

Oh hmm I think judging from logs the native Android app is not using the staging server...
I assumed that it would, but that seems wrong since there is no badge in the sidebar header.

cc @roryabraham to confirm.

marcaaron · 2021-05-18T21:03:00Z

Probably we will need to test this on production anyway to be sure it works - I think it will, but I don't have an account to test with as my bank is not an option in Plaid.

roryabraham · 2021-05-18T21:51:16Z

@marcaaron Are you asking if the E.cash Android beta (staging) app communicates with the staging API? If so, the answer is no – E.cash staging only communicates with the production API, by design. There was a fair amount of discussion around this already, but basically the reason is that if E.cash staging can't talk to our staging API, then there's no chance we deploy code that breaks E.cash production because it is dependent upon a staging API which has not yet been deployed.

marcaaron · 2021-05-18T22:13:55Z

Let me ask another way... 😅

https://staging.expensify.cash appears to use the .env.staging variables but native apps do not?

roryabraham · 2021-05-18T22:46:10Z

Ah, yes that's true. The reason for that is because when we run an iOS/Android deploy we just take an existing beta build and release it for production.

marcaaron · 2021-05-18T22:50:04Z

Got it! Thanks for the context. I think this will need to be Production QA in that case and someone with a bank account can help us test it eventually. Anyways, @isagoico no need to block on this one.

shubham1206agra · 2024-03-27T11:46:09Z

src/components/PlaidLink/index.js

+
+    useEffect(() => {
+        if (error) {
+            props.onError(error);


This usage leads to #38069. This happened due to a change in onError on each re-render as it wasn't memoized.

marcaaron self-assigned this May 7, 2021

marcaaron commented May 10, 2021

View reviewed changes

marcaaron force-pushed the marcaaron-addBankAccount branch from c4ec957 to 99ded8c Compare May 10, 2021 23:23

marcaaron changed the title ~~[WIP] Add Personal Bank Account flow~~ [HOLD] Add Personal Bank Account flow May 10, 2021

marcaaron changed the title ~~[HOLD] Add Personal Bank Account flow~~ [HOLD] Add Personal Bank Account via Plaid flow May 11, 2021

use better button

faefc3f

marcaaron marked this pull request as ready for review May 11, 2021 00:41

marcaaron requested a review from a team as a code owner May 11, 2021 00:41

marcaaron changed the title ~~[HOLD] Add Personal Bank Account via Plaid flow~~ Add Personal Bank Account via Plaid flow May 11, 2021

MelvinBot requested review from francoisl and removed request for a team May 11, 2021 00:41

marcaaron added the InternalQA label May 11, 2021

marcaaron requested review from ctkochan22 and Jag96 May 11, 2021 00:45

undo some unnecessary changes

23a841a

marcaaron commented May 11, 2021

View reviewed changes

fix podfile

de31ab9

marcaaron commented May 11, 2021

View reviewed changes

fix incorrect modal stack

31effbf

marcaaron commented May 11, 2021

View reviewed changes

fix conflicts

1a7de12

stitesExpensify mentioned this pull request May 11, 2021

Profile - GMT time is not displayed #2097 #2756

Closed

5 tasks

francoisl reviewed May 11, 2021

View reviewed changes

marcaaron added 2 commits May 11, 2021 17:16

fix conflicts

047af51

use isEmpty

091281f

Jag96 suggested changes May 14, 2021

View reviewed changes

remove isForStripeConnect

d08d0c7

Jag96 previously approved these changes May 14, 2021

View reviewed changes

ctkochan22 previously approved these changes May 17, 2021

View reviewed changes

francoisl previously approved these changes May 17, 2021

View reviewed changes

fix conflict

4d18304

marcaaron dismissed stale reviews from francoisl, ctkochan22, and Jag96 via 4d18304 May 17, 2021 20:42

Jag96 approved these changes May 17, 2021

View reviewed changes

ctkochan22 approved these changes May 17, 2021

View reviewed changes

francoisl approved these changes May 17, 2021

View reviewed changes

Jag96 merged commit c100011 into main May 17, 2021

Jag96 deleted the marcaaron-addBankAccount branch May 17, 2021 21:12

roryabraham mentioned this pull request May 18, 2021

Deploy Checklist: Expensify.cash 2021-05-14 #2948

Closed

26 tasks

shubham1206agra reviewed Mar 27, 2024

View reviewed changes

shubham1206agra mentioned this pull request Mar 27, 2024

[HOLD for payment 2024-03-26] [$500] BA - Not able to add Regions BA, modal redirects to initial Bank account page #38069

Closed

6 tasks

Add Personal Bank Account via Plaid flow #2746

Add Personal Bank Account via Plaid flow #2746

Conversation

marcaaron commented May 7, 2021 • edited

Details

Fixed Issues

Notes on testing this on native platforms

Tests

QA Steps (Internal)

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented May 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron May 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francoisl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francoisl commented May 11, 2021

marcaaron commented May 12, 2021

marcaaron commented May 14, 2021

shawnborton commented May 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctkochan22 left a comment

Choose a reason for hiding this comment

francoisl left a comment

Choose a reason for hiding this comment

OSBotify commented May 17, 2021

isagoico commented May 18, 2021

marcaaron commented May 18, 2021

marcaaron commented May 18, 2021

marcaaron commented May 18, 2021

marcaaron commented May 18, 2021

marcaaron commented May 18, 2021

roryabraham commented May 18, 2021

marcaaron commented May 18, 2021

roryabraham commented May 18, 2021

marcaaron commented May 18, 2021

Choose a reason for hiding this comment

marcaaron commented May 7, 2021 •

edited

marcaaron May 11, 2021 •

edited