Migrate BankAccount_SetupWithdrawalAccount API #3507

marcaaron · 2021-06-10T02:40:22Z

Details

I tried to keep things familiar here while migrating stuff over from Web-Secure. That means for now, I'm sticking with a single page ReimbursementAccountPage + then conditionally showing views based on the achData.currentStep so basically it's very similar to what we have in Web-Secure.

There are some changes that I am proposing that feel more inline with how we are doing things in Expensify.cash

Got rid of the "magic" methods used in Web-Secure used to move between steps, call the withdrawal account API, validate forms, etc. Mostly I found that pattern to be pretty difficult to reason about and the convenience isn't worth the confusion (that's just my opinion, but we don't have any patterns like this in E.cash so far).
Instead, every view will handle it's own validation, form state, submit action, and navigation via the header in some cases so nextStep() and previousStep() are no longer triggered in the parent view (addReimbursementPage.jsx in Web-Secure) and instead implemented by each "step". This might be a little more verbose, but I think it's better to be explicit.
The country selector in the BankAccountStep is not included since all the accounts added for the free plan should be US.
Instead of having a this.achData to reference we are referencing + storing the data for an account in setup in the Onyx cache / localStorage. Most of the logic is left intact, but everywhere we refer to this.achData is now will refer to Onyx data. This is ideal because it eliminates the need for something like ReimbursementAccountPage.refreshView(). Instead we can navigate to the next step simply by setting the Onyx data and letting the view which is subscribing to this data update itself in response.

Fixed Issues (Part 2)

https://github.com/Expensify/Expensify/issues/166041

Tests

Navigate to /add-verified-bank-account
Verify the "Add Bank Account" view appears
Select "Connect Manually"
Enter the test credentials:

Routing Number: 011401533
Account Number: 1111222233331111

Press Save & Continue button
Verify you land on the "Company Information" step
Check the Chrome dev tools Network tab and verify that BankAccount_SetupWithdrawalAccount request was made (Web Only)
Press the "<" button in the top left of the header
Verify you are back on the selection screen
Select "Log Into Your Bank"
Enter the Plaid test credentials

Login: user_good
Password: pass_good

Follow prompts until you reach the account select screen
Select the savings account
Tap Save & Continue button
Verify you land in the "Company Information" step and the API request was successful

QA Steps

No QA necessary until the e2e flow is ready for testing

Tested On

Screenshots

Web

Not including other screenshot because no UI is being added here. The web flow is provided as a reference.

…e of it all

…ly respond to Onyx setting the achData

marcaaron · 2021-06-11T21:23:05Z

Trying to get through all of my @TODO notes but so far this is going well and I've got it so we can move from the BankAccountStep > CompanyStep (which is blank for now) but the BankAccount_SetupWithdrawalAccount API is working.

marcaaron · 2021-06-11T22:46:53Z

Ready for review, but I'm going to run through the rest of the platform testing just to make sure there is nothing unexpected.

NikkiWines

Couple of minor comments, overall looking good. Not sure about the TODOs for some of the later account setup steps - esp. the stuff about open bank accounts. I'd like the err on the side of not including it so we don't end up with code we don't need though

NikkiWines · 2021-06-14T21:25:06Z

src/libs/actions/BankAccounts.js

+        }
+    }
+
+    // When going from CompanyStep to BankAccountStep, show the manual form instead of Plaid


NAB: When going back to the BankAccountStep from the Company Step just for clarity in the order of the steps.

Nice. Also this is possibly some logic that can be removed.

src/libs/models/BankAccount.js

NikkiWines · 2021-06-14T21:30:41Z

src/libs/models/BankAccount.js

+    }
+
+    /**
+     * @deprecated Use !isPending instead.


If it's deprecated should we even include it?

I don't really know. Pretty much just copying stuff over from Web-Secure in this case - but probably we can remove any methods that are not used. Might save this for a follow up.

src/pages/ReimbursementAccount/BankAccountStep.js

src/pages/ReimbursementAccount/CompanyStep.js

marcaaron · 2021-06-14T22:36:51Z

I'd like the err on the side of not including it so we don't end up with code we don't need though

I definitely agree with this. However, we have infinite time to follow up and remove things we don't need and not much time to get this done. So, I'm thinking that we can create a follow up issue to address some of these odd @todos and question marks and anything that just generally no longer makes sense.

marcaaron · 2021-06-14T22:38:15Z

Also kind of separate but related - once this flow is nailed down I think it will be a little easier to tell what is needed and what is not so I was a bit hesitant to remove things right away.

src/CONST.js

src/languages/es.js

roryabraham · 2021-06-12T01:16:35Z

src/libs/API.js

+    // Only keep allowed parameters in the additionalData object
+    const additionalData = _.pick(parameters, allowedParameters);
+
+    requireParameters(['currentStep'], parameters, commandName);


This is another thing we could create a follow-up for, but are there different sets of required parameters for each step? Might be more comprehensible to have something like:

const requiredParameters = [ CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT: [ // the required parameters for this step ], CONST.BANK_ACCOUNT.STEP.COMPANY: [ // the required parameters for this step ], ... ... ];

And that could help us do better client-side validation 🤷

Sure. I think something like that would clean this up maybe. They are also not used anywhere else atm.

src/libs/actions/BankAccounts.js

roryabraham · 2021-06-14T22:53:07Z

src/libs/actions/BankAccounts.js

+                        );
+                        const sdkToken = lodashGet(onfidoRes, CONST.BANK_ACCOUNT.ONFIDO_RESPONSE.SDK_TOKEN);
+                        if (sdkToken && !previousACHData.isOnfidoSetupComplete
+                                && onfidoRes.status !== CONST.BANK_ACCOUNT.ONFIDO_RESPONSE.PASS


Suggested change

&& onfidoRes.status !== CONST.BANK_ACCOUNT.ONFIDO_RESPONSE.PASS

&& onfidoRes.status !== CONST.BANK_ACCOUNT.ONFIDO_RESPONSE.PASS)

Also, should this really be using || instead of && ?

if (sdkToken || !previousACHData.isOnfidoSetupComplete || onfidoResponse.status !== CONST.BANK_ACCOUNT.ONFIDO_RESPONSE.PASS) { }

Not sure. Why should it?

src/libs/actions/BankAccounts.js

src/pages/ReimbursementAccount/BankAccountStep.js

marcaaron

Thanks for the review @roryabraham!

src/libs/actions/BankAccounts.js

NikkiWines

Looks good! One tiny comment.

~~I also noticed that I wasn't getting routed to the Company Step ever, I just get re-routed to the beginning of the add VBA flow.~~

Edit: NVM turns out I set up a VBA with those values already which is why it was re-routing incorrectly 🙃 I retested on a different account and everything works as expected

src/libs/actions/BankAccounts.js

marcaaron · 2021-06-15T18:13:40Z

@NikkiWines I think it's because there are errors - going to swap out the console.error for a growl instead so it's more obvious something went wrong.

NikkiWines

Looks good! The growl is definitely way clearer 👍

NikkiWines · 2021-06-15T18:19:36Z

src/libs/actions/BankAccounts.js

+function goToWithdrawalAccountSetupStep(stepID, achData) {
+    const newACHData = {...previousACHData};
+
+    // If we go back to Requestor Step, reset any validation and previously answered questions from expectID.


NAB: When going to the Requestor Step, reset any validation and previously answered questions from expectID.

shawnborton · 2021-06-15T18:28:25Z

Looks like the icon colors are off in this screenshot:

I think they should be in our standard colors.icon which is like a gray

marcaaron · 2021-06-15T18:46:30Z

Will do a follow up for the icon colors @shawnborton

Spoke with @ctkochan22 who gave this a once over and mentioned not blocking on him so I'm going to merge this to keep things moving.

OSBotify · 2021-06-15T18:47:16Z

✋ 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 · 2021-06-16T18:18:03Z

🚀 Deployed to staging in version: 1.0.69-1🚀

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

OSBotify · 2021-06-25T18:32:06Z

🚀 Deployed to production in version: 1.0.73-3🚀

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

add models

c219c9e

marcaaron self-assigned this Jun 10, 2021

marcaaron added 12 commits June 10, 2021 09:47

add BankAccount model

6eac7d4

update some consts

cc78e01

attempt to migrate stuff to a controller page for now while make sens…

c810c56

…e of it all

sketch out plan

4f96379

move setupWithdrawalAccount into an action so that our views can pure…

99e0f81

…ly respond to Onyx setting the achData

move everything into actions clean up temp view

3319c16

fix conflicts

ca91a0c

fix conflicts and rename some things

d61a525

reorganize things a bit and fix incorrect API usage

e966390

dont pass achData

a7eaefe

fix Permissions issue. rename goToStepID to goToWithdrawalStepID

4bebfbb

use unmasked accountNumber

76b67ce

marcaaron requested a review from ctkochan22 June 11, 2021 21:23

marcaaron added 5 commits June 11, 2021 11:49

clean up some TODOs

c3b8007

fix up some CONSTs

e31e101

fix propTypes and error messages

36e5a97

change BusinessBankAccountNewPage to BankAccountStep

b96c297

Fix comments style etc

661ee34

marcaaron changed the title ~~[WIP] Migrate BankAccount_SetupWithdrawalAccount API~~ Migrate BankAccount_SetupWithdrawalAccount API Jun 11, 2021

marcaaron marked this pull request as ready for review June 11, 2021 22:46

marcaaron requested a review from a team as a code owner June 11, 2021 22:46

marcaaron requested a review from NikkiWines June 11, 2021 22:47

MelvinBot requested review from jasperhuangg and Gonals and removed request for a team June 11, 2021 22:47

marcaaron removed the request for review from Gonals June 11, 2021 22:47

fix new workspace modal

e85f587

NikkiWines requested changes Jun 14, 2021

View reviewed changes

make some requested changes

825d108

roryabraham requested changes Jun 14, 2021

View reviewed changes

marcaaron added 2 commits June 14, 2021 13:23

Make requested changes

c900d39

Make some requested changes

1b1e210

marcaaron commented Jun 15, 2021

View reviewed changes

roryabraham requested changes Jun 15, 2021

View reviewed changes

src/libs/actions/BankAccounts.js Outdated Show resolved Hide resolved

jasperhuangg previously approved these changes Jun 15, 2021

View reviewed changes

use promiseAllSettled

a48ddb5

marcaaron dismissed jasperhuangg’s stale review via a48ddb5 June 15, 2021 18:04

NikkiWines reviewed Jun 15, 2021

View reviewed changes

src/libs/actions/BankAccounts.js Outdated Show resolved Hide resolved

add growl for errors

635bebf

fix comment

b408663

marcaaron requested review from roryabraham, NikkiWines and jasperhuangg June 15, 2021 18:14

NikkiWines approved these changes Jun 15, 2021

View reviewed changes

roryabraham approved these changes Jun 15, 2021

View reviewed changes

marcaaron merged commit c1b4bd4 into main Jun 15, 2021

marcaaron deleted the marcaaron-setupWithdrawlAccount branch June 15, 2021 18:46

OSBotify mentioned this pull request Jun 17, 2021

Deploy Checklist: Expensify.cash 2021-06-15 #3593

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate BankAccount_SetupWithdrawalAccount API #3507

Migrate BankAccount_SetupWithdrawalAccount API #3507

marcaaron commented Jun 10, 2021 •

edited

Loading

marcaaron commented Jun 11, 2021

marcaaron commented Jun 11, 2021

NikkiWines left a comment

NikkiWines Jun 14, 2021

marcaaron Jun 14, 2021

NikkiWines Jun 14, 2021

marcaaron Jun 14, 2021

marcaaron commented Jun 14, 2021

marcaaron commented Jun 14, 2021

roryabraham Jun 12, 2021

marcaaron Jun 14, 2021

roryabraham Jun 14, 2021

roryabraham Jun 14, 2021

marcaaron Jun 15, 2021

marcaaron left a comment

NikkiWines left a comment •

edited

Loading

marcaaron commented Jun 15, 2021

NikkiWines left a comment

NikkiWines Jun 15, 2021

shawnborton commented Jun 15, 2021

marcaaron commented Jun 15, 2021

OSBotify commented Jun 15, 2021

OSBotify commented Jun 16, 2021

OSBotify commented Jun 25, 2021

	&& onfidoRes.status !== CONST.BANK_ACCOUNT.ONFIDO_RESPONSE.PASS
	&& onfidoRes.status !== CONST.BANK_ACCOUNT.ONFIDO_RESPONSE.PASS)

Migrate BankAccount_SetupWithdrawalAccount API #3507

Migrate BankAccount_SetupWithdrawalAccount API #3507

Conversation

marcaaron commented Jun 10, 2021 • edited Loading

Details

Fixed Issues (Part 2)

Tests

QA Steps

Tested On

Screenshots

Web

marcaaron commented Jun 11, 2021

marcaaron commented Jun 11, 2021

NikkiWines 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

Choose a reason for hiding this comment

marcaaron commented Jun 14, 2021

marcaaron commented Jun 14, 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

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

NikkiWines left a comment • edited Loading

Choose a reason for hiding this comment

marcaaron commented Jun 15, 2021

NikkiWines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnborton commented Jun 15, 2021

marcaaron commented Jun 15, 2021

OSBotify commented Jun 15, 2021

OSBotify commented Jun 16, 2021

OSBotify commented Jun 25, 2021

marcaaron commented Jun 10, 2021 •

edited

Loading

NikkiWines left a comment •

edited

Loading