Refactor BankAccountSetup using nextStepValues to force Chase manual account numbers by nkuoch · Pull Request #6851 · Expensify/App

nkuoch · 2021-12-21T17:06:10Z

HOLD on https://github.com/Expensify/Web-Secure/pull/2121

Details

Allow to show manual account numbers form after choosing Plaid Oauth Chase accounts, while refactoring using nextStepValues as all the logic moved php side in this PR: https://github.com/Expensify/Web-Secure/pull/2121/files

Fixed Issues

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

Tests

Made sure I could add a business bank account via Plaid OAuth Chase, entering account numbers manually.
To test it, pull the web-secure branch nat-chaseoauth and then follow https://docs.google.com/document/d/1_E2pxPGzp2UjVOPHqVHUj09dvWyzSYWbBOy8pbsMORc/edit#

QA Steps

Add a business bank account using Plaid Chase. Make sure that after you connect to Chase, you're still asked to add your account numbers manually. Make sure you can complete the rest of the flow.

Tested On

Screenshots

Web

If you enter a wrong account number (not ending with the mask), you'll see an error

Make sure you can finish the flow.

joelbettner

This LGTM

joelbettner · 2021-12-21T19:11:58Z

+            const nextStep = getNextStep(updatedACHData);
+
+            // Some steps require several substeps based on some data.
+            // This logic is done php side: when we need to display a specific step with specific data, php add them in the response in nextStepValues


joelbettner · 2021-12-21T19:13:30Z

+            // Some steps require several substeps based on some data.
+            // This logic is done php side: when we need to display a specific step with specific data, php add them in the response in nextStepValues
+            // Example 1: When forcing manual step after adding Chase bank account via Plaid, so we can ask for the real numbers instead of the plaid substitutes
+            // Example 2: When on the requestor step, showing Onfido view after submitting the identity and retrieving the sdkToken


Has the information about whether or not a user needs to do onFido always been determined/processed/returned by php?

no, but now it will be since https://github.com/Expensify/Web-Secure/pull/2121

marcaaron

LGTM just left a clarifying question and still need to run the tests.

marcaaron · 2021-12-22T19:24:52Z

-    );
-
-    return differentiatorQuestion ? [differentiatorQuestion] : [];
-}


Oh wow, amazing we don't need any of this stuff anymore.

marcaaron · 2021-12-22T19:29:28Z

+                navigation.goToWithdrawalAccountSetupStep(_.get(responseACHData.nextStepValues, 'currentStep') || nextStep, {
+                    ...updatedACHData,
+                    ...responseACHData,
+                    ...responseACHData.nextStepValues,


I haven't checked out the other PR yet, but is the intention to have the nextStepValues overwrite the achData (because nextStepValues are similar to achData)? And if so should we remove the nextStepValues after they are copied here? Or is that not necessary?

Yes, that's the intention. Removing nextStepValues then is not necessary, but might be cleaner, so I can do it :)

marcaaron · 2021-12-22T19:30:34Z

        const isFromPlaid = this.props.achData.setupType === CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID;
        const shouldDisableInputs = Boolean(this.props.achData.bankAccountID) || isFromPlaid;
-        const shouldReinitializePlaidLink = this.props.plaidLinkOAuthToken && this.props.receivedRedirectURI;
+        const shouldReinitializePlaidLink = this.props.plaidLinkOAuthToken && this.props.receivedRedirectURI && this.props.achData.subStep !== 'manual';


Suggested change

const shouldReinitializePlaidLink = this.props.plaidLinkOAuthToken && this.props.receivedRedirectURI && this.props.achData.subStep !== 'manual';

const shouldReinitializePlaidLink = this.props.plaidLinkOAuthToken && this.props.receivedRedirectURI && this.props.achData.subStep !== CONST.BANK_ACCOUNT.SUBSTEP.MANUAL;

nkuoch · 2021-12-23T10:09:07Z

updated

marcaaron

Code LGTM ✅

I don't have a Chase account - but did a run through the VBA flow using the "Platypus OAuth" account and everything seems to be working still.

marcaaron · 2021-12-23T21:05:47Z


            // Go to next step
-            navigation.goToWithdrawalAccountSetupStep(getNextStep(updatedACHData), responseACHData);
+            // Note: php sometimes returns code 200 with no achData, just to indicate that we should keep whatever the JS values are.


Contributors who work on this code might not connect that when we refer to "php" we mean the API (though it's not hard to figure out from the context).

NAB, this note seems important, but I had some trouble understanding when it could happen. Is it important to know not just that it can happen but when?

Anyway, not sure if this is really valuable but maybe we can log something instead of leave this comment

if (_.isEmpty(responseACHData)) { Log.info('[SetupWithdrawalAccount] No achData in response. Navigating to next step based on currently stored values.'); }

Some steps don't need to verify or save anything (like here), and we early return, just to say that the JS needs to go to the next step with whatever data they already have.

nkuoch · 2021-12-24T10:09:23Z

updated

OSBotify · 2021-12-28T17:12:52Z

✋ 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-12-29T00:50:19Z

🚀 Deployed to staging by @nkuoch in version: 1.1.23-2 🚀

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

nkuoch · 2021-12-29T07:45:01Z

Tested on https://staging.new.expensify.com/bank-account/, all good!

OSBotify · 2022-01-04T01:34:19Z

🚀 Deployed to production by @francoisl in version: 1.1.24-8 🚀

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

nkuoch requested a review from a team as a code owner December 21, 2021 17:06

MelvinBot requested review from joelbettner and removed request for a team December 21, 2021 17:06

nkuoch force-pushed the nat-nextStepValues branch from 16be3ed to 1e8f545 Compare December 21, 2021 17:24

Refactor BankAccountSetup using nextStepValues

0190208

nkuoch force-pushed the nat-nextStepValues branch from 1e8f545 to 0190208 Compare December 21, 2021 17:32

nkuoch self-assigned this Dec 21, 2021

joelbettner reviewed Dec 21, 2021

View reviewed changes

nkuoch changed the title ~~Refactor BankAccountSetup using nextStepValues so force Chase manual account numbers~~ [WIP] Refactor BankAccountSetup using nextStepValues so force Chase manual account numbers Dec 21, 2021

fix comment typo

f9f8614

nkuoch changed the title ~~[WIP] Refactor BankAccountSetup using nextStepValues so force Chase manual account numbers~~ [WIP] Refactor BankAccountSetup using nextStepValues to force Chase manual account numbers Dec 21, 2021

nkuoch added 4 commits December 21, 2021 23:07

Fix goto manual subStep

a72348a

Save mask

0701d98

Fix nextStep when error

859b5c4

Fix up to date achData

1e5cd2a

nkuoch requested a review from marcaaron December 22, 2021 13:03

nkuoch changed the title ~~[WIP] Refactor BankAccountSetup using nextStepValues to force Chase manual account numbers~~ Refactor BankAccountSetup using nextStepValues to force Chase manual account numbers Dec 22, 2021

nkuoch changed the title ~~Refactor BankAccountSetup using nextStepValues to force Chase manual account numbers~~ [HOLD] Refactor BankAccountSetup using nextStepValues to force Chase manual account numbers Dec 22, 2021

marcaaron reviewed Dec 22, 2021

View reviewed changes

Comment thread src/libs/actions/ReimbursementAccount/setupWithdrawalAccount.js

Fixes from Marc review

63daa8a

marcaaron previously approved these changes Dec 23, 2021

View reviewed changes

Only use local achData when not returned from response

6d0baa3

nkuoch dismissed marcaaron’s stale review via 6d0baa3 December 24, 2021 08:56

marcaaron approved these changes Dec 24, 2021

View reviewed changes

nkuoch changed the title ~~[HOLD] Refactor BankAccountSetup using nextStepValues to force Chase manual account numbers~~ Refactor BankAccountSetup using nextStepValues to force Chase manual account numbers Dec 28, 2021

nkuoch merged commit 9a55e51 into main Dec 28, 2021

nkuoch deleted the nat-nextStepValues branch December 28, 2021 17:12

OSBotify mentioned this pull request Dec 29, 2021

Deploy Checklist: New Expensify 2021-12-29 #6920

Closed

44 tasks

aldo-expensify mentioned this pull request Dec 29, 2021

Staging/Dev: Adding VBA using plaid bounces the user to manual step of adding Routing/Account number #6940

Closed

	const shouldReinitializePlaidLink = this.props.plaidLinkOAuthToken && this.props.receivedRedirectURI && this.props.achData.subStep !== 'manual';
	const shouldReinitializePlaidLink = this.props.plaidLinkOAuthToken && this.props.receivedRedirectURI && this.props.achData.subStep !== CONST.BANK_ACCOUNT.SUBSTEP.MANUAL;

Conversation

nkuoch commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Uh oh!

joelbettner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nkuoch commented Dec 23, 2021

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkuoch commented Dec 24, 2021

Uh oh!

OSBotify commented Dec 28, 2021

Uh oh!

OSBotify commented Dec 29, 2021

Uh oh!

nkuoch commented Dec 29, 2021

Uh oh!

OSBotify commented Jan 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nkuoch commented Dec 21, 2021 •

edited

Loading