Fix and refactor VBBA setup flow #13236

nkuoch · 2022-12-01T14:32:18Z

Details

Refactoring and fixing different issues in the vbba setup flow:

Extract isPlaidDisabled from plaidData Onyx key, as it's not set in the same flow as the rest, and causing issues when we expect plaidData to be rest to empty.
Pass plaidData, reimbursementAccount and reimbursementAccountDraft via required props instead of withOnyx, and call them explicitly instead of via lodashGet:
- to avoid eslint-disable-next-line react/no-unused-prop-types and other engineers thinking they can remove them because unused.
- to avoid having declare default props for those in each view. Instead, it will be centralized in the parent view
Prevent infinite loader in AddPlaidBankAccount view
Add back button on offline blocking view. Handle backPress on parent view so the logic can be shared.
Remove some unused methods
Make sure we reset PlaidData when needed
Add missing reimbursementAccountDraft propTypes
Refactor getDefaultStateForField so engineers don't think reimbursementAccountDraft or reimbursementAccount are not used
Refactor WorkspaceResetBankAccountModal and fix cases were it was stuck displaying
Prevent editing LOCKED bank account
Fix bug that returns to bank login view after logging in successfully

Fixed Issues

$ #13166
$ #12708
$ https://github.com/Expensify/Expensify/issues/245747
$ #11777
$ #12123
$ #13598
$ #13911
$ https://github.com/Expensify/Expensify/issues/231253
$ https://github.com/Expensify/Expensify/issues/236672
$ https://github.com/Expensify/Expensify/issues/217507
$ #14157

Tests

To be tested with Web PR: https://github.com/Expensify/Web-Expensify/pull/35797

Same as in QA Steps

Offline steps

Included in QA Steps

QA Steps

Just complete the VBBA setup flow with random data and doing this:

at the beginning of the flow (when seeing the 2 options: plaid/manual), go offline. Click on Plaid. You should see the offline view. Make sure you can go back to the 2 options. Go back online. Make sure the view remains the same.

Connect via Plaid Chase and select a plaid account without submitting. Go back (clicking on the left arrow from the right panel from the bankAccountStep), click back to a plaid bank and log in again, but picking up other bank accounts than the one you selected before plaid auth side. Make sure the selector shows the new accounts

click on the left arrow from the right panel for each step (to test the "back" behavior)
- BankAccount

Company

Requestor

Onfido

Note: after resubmitting the requestor step, you shouldn't see Onfido again unless you modify your name

Beneficial Owners

exit in the middle of the flow (when seeing the beneficial owners step for example), and make sure you can Continue with Setup and finish the flow

Click on Let's start over and test adding an OPEN VBBA. Make sure you end up with this view:

Make sure you can also add a personal bank account
No console error should appear

PR Author Checklist

Screenshots/Videos

Web

See screenshots from QA Steps

Mobile Web

vbbaIOSSafari.mp4

Desktop

Test start back & back from each step

2023-01-17_11-43-06.mp4

Test start over

remove.mp4

Test changing Plaid bank account

2023-01-17_11-58-50.mp4

Test adding a personal bank account

2023-01-17_12-01-54.mp4

iOS

Test back from each step

TestAllTheWayBackFromOnfidoAndContinue.mp4

TestBackFromBenefStepAndOpen.mp4

Android

Offline Plaid: https://recordit.co/TR5KrMlOto

Backwards works for BankAccountStep: https://recordit.co/0Shseu59kF

Backwards from Requestor Step and Onfido: https://recordit.co/ldlH7Wugkp

Beneficial Owners: https://recordit.co/VnmIixo3QR

melvin-bot · 2022-12-01T14:32:25Z

Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work?

Allow to go back from offline view Fix and refactor startOver modal Refactor getDefaultStateForField so people dont think reimbursementAccountDraft is not used

OSBotify · 2023-01-20T17:56:09Z

✋ 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 · 2023-01-23T18:27:13Z

🚀 Deployed to staging by https://github.com/ctkochan22 in version: 1.2.58-0 🚀

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

Pujan92 · 2023-01-24T06:00:18Z

@nkuoch seems this PR causing the deploy blocker issue as in the BankAccounts.js we are using setBankAccountSubStep but we aren't importing that.

diff --git a/src/libs/actions/BankAccounts.js b/src/libs/actions/BankAccounts.js
index b6aea981e..11fe41607 100644
--- a/src/libs/actions/BankAccounts.js
+++ b/src/libs/actions/BankAccounts.js
@@ -7,6 +7,7 @@ import DateUtils from '../DateUtils';
 import * as PlaidDataProps from '../../pages/ReimbursementAccount/plaidDataPropTypes';
 import Navigation from '../Navigation/Navigation';
 import ROUTES from '../../ROUTES';
+import * as ReimbursementAccount from './ReimbursementAccount';
 
 export {
     goToWithdrawalAccountSetupStep,
@@ -38,7 +39,7 @@ function clearPlaid() {
 }
 
 function openPlaidView() {
-    clearPlaid().then(() => this.setBankAccountSubStep(CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID));
+    clearPlaid().then(() => ReimbursementAccount.setBankAccountSubStep(CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID));
 }

OSBotify · 2023-01-25T02:11:25Z

🚀 Deployed to production by https://github.com/chiragsalian in version: 1.2.58-4 🚀

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

luacmartins · 2023-01-30T23:45:04Z

@nkuoch Are we supposed to be able to edit the company name and tax ID once we've saved them once and advanced to the Personal information step? They don't seem to be disabled anymore although these variables would suggest they should. I just want to confirm the expected behavior before I add a fix to this PR

nkuoch · 2023-01-31T14:45:42Z

I confirm we're not supposed to be able to edit them once we've reached the requestor step.
How did you reproduce the bug? I personally can't reproduce what you're saying.

luacmartins · 2023-01-31T16:57:02Z

Oh weird. I can't reproduce it anymore either 🤔 Anyways, thanks for confirming!

luacmartins · 2023-01-31T17:23:54Z

Nvm, I can still reproduce on main/staging/prod on iOS/android

MariaHCD · 2023-02-16T09:08:01Z

src/pages/ReimbursementAccount/ReimbursementAccountPage.js

+        if (this.props.reimbursementAccount.shouldShowResetModal && Boolean(achData.bankAccountID)) {
+            return (
+                <WorkspaceResetBankAccountModal reimbursementAccount={this.props.reimbursementAccount} />
+            );
+        }


Just as an FYI, this caused #14692

We missed testing for the Start over feature in this PR so we didn't notice that there was a blank screen behind the confirmation modal.

NikkiWines · 2023-03-02T18:35:54Z

👋 Hi, we've identified this PR as 1/2 of the cause of #14972. The logic in getDefaultStateForField() does not work correctly in the event that lodashGet(reimbursementAccountDraft, fieldName) returns false.

 function getDefaultStateForField(reimbursementAccountDraft, reimbursementAccount, fieldName, defaultValue = '') { 
    return lodashGet(reimbursementAccountDraft, fieldName)
        || lodashGet(reimbursementAccount, ['achData', fieldName], defaultValue);
 }

In this case, this could've been avoided by doing some more robust testing to ensure that the default state value matched what we were expecting in a variety of scenarios. We've got a proposed regression test here that should cover this scenario 🙌

Santhosh-Sellavel · 2023-03-27T16:03:26Z

src/pages/AddPersonalBankAccountPage.js

@@ -93,7 +98,10 @@ class AddPersonalBankAccountPage extends React.Component {
                        description={this.props.translate('addPersonalBankAccountPage.successMessage')}
                        shouldShowButton
                        buttonText={this.props.translate('common.continue')}
-                        onButtonPress={() => Navigation.navigate(ROUTES.SETTINGS_PAYMENTS)}
+                        onButtonPress={() => {
+                            BankAccounts.clearPersonalBankAccount();


Clearing this early cause a regression #15202

mananjadhav · 2023-04-27T09:45:54Z

I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check.

We've also updated the item in the checklist with this PR to avoid this issue in the future.

allroundexperts · 2023-07-19T12:00:24Z

src/pages/ReimbursementAccount/ReimbursementAccountPage.js

+            case CONST.BANK_ACCOUNT.STEP.VALIDATION:
+                if (_.contains([BankAccount.STATE.VERIFYING, BankAccount.STATE.SETUP], achData.state)) {
+                    BankAccounts.goToWithdrawalAccountSetupStep(CONST.BANK_ACCOUNT.STEP.ACH_CONTRACT);
+                } else if (achData.state === BankAccount.STATE.PENDING) {


We did not take into account the user being offline while adding this condition. This caused #21837 to come up later.

fedirjh · 2023-12-30T05:41:07Z

src/components/AddPaymentMethodMenu.js

                onSelected: () => {
-                    BankAccounts.clearPlaid();


Coming from #32906, this change caused a regression, after this change the plaid token is not cleared.

nkuoch self-assigned this Dec 1, 2022

nkuoch force-pushed the nat-back branch 9 times, most recently from 945fc23 to 6f042c2 Compare December 8, 2022 14:52

nkuoch force-pushed the nat-back branch 8 times, most recently from b3dca8b to d7b5176 Compare December 9, 2022 15:24

nkuoch changed the title ~~Refactor VBBA setup flow and allow to go back from offline view~~ Fix and refactor VBBA setup flow Dec 13, 2022

nkuoch force-pushed the nat-back branch 7 times, most recently from 95e827b to 52974c0 Compare December 16, 2022 22:15

Refactor VBBA setup flow

687cac6

Allow to go back from offline view Fix and refactor startOver modal Refactor getDefaultStateForField so people dont think reimbursementAccountDraft is not used

nkuoch force-pushed the nat-back branch from 52974c0 to 687cac6 Compare December 16, 2022 22:34

nkuoch marked this pull request as ready for review December 16, 2022 22:40

aimane-chnaif mentioned this pull request Jan 20, 2023

[$1000] Not showing loader on add bank account page #14425

Closed

6 tasks

OSBotify mentioned this pull request Jan 23, 2023

Deploy Checklist: New Expensify 2023-01-23 #14471

Closed

17 tasks

MariaHCD mentioned this pull request Jan 24, 2023

[HOLD for payment 2023-02-02] [$1000] VBA - Connect to plaid button does not respond #14505

Closed

6 tasks

kavimuru mentioned this pull request Jan 24, 2023

mWeb - After redirect to ND again loop starts to connect to plaid #14516

Closed

1 task

situchan mentioned this pull request Jan 25, 2023

[HOLD][$2000] Connect Bank Account- Infinite spinner or blank page for app appears after back online #14362

Closed

4 tasks

MariaHCD reviewed Feb 16, 2023

View reviewed changes

MariaHCD mentioned this pull request Feb 16, 2023

[HOLD for payment 2023-02-22] Blank screen appears tapping Start Over to continue to manual step #14692

Closed

1 task

NikkiWines mentioned this pull request Mar 2, 2023

[HOLD for payment 2023-03-06] [$1000] The Terms of Service checkbox doesn't respond properly while navigating the connect bank account flow #14972

Closed

1 task

Santhosh-Sellavel reviewed Mar 27, 2023

View reviewed changes

Santhosh-Sellavel mentioned this pull request Mar 27, 2023

[HOLD for payment 2023-03-31] After adding a Personal Bank Account as part of paying an IOU or sending money, the Plaid flow is re-triggered unnecessarily #15202

Closed

6 tasks

alexxxwork mentioned this pull request Apr 21, 2023

[HOLD for payment 2023-05-05] [HOLD for payment 2023-04-20] [$1000] Text strings must be rendered within a <Text> component #16683

Closed

parasharrajat mentioned this pull request Jun 26, 2023

[HOLD for payment 2023-06-23] [$2000] IOS - Connect Bank - User cannot fill out forms in the 'Connect BA' flow in Offline #18990

Closed

6 tasks

allroundexperts reviewed Jul 19, 2023

View reviewed changes

allroundexperts mentioned this pull request Jul 19, 2023

[HOLD for payment 2023-07-21] [$1000] Web - Bank - Back icon on last stage of adding bank account page is not clickable in offline mode #21837

Closed

6 tasks

fedirjh reviewed Dec 30, 2023

View reviewed changes

fedirjh mentioned this pull request Dec 30, 2023

[$500] Wallet-Tapping "Enable wallet",add bank account page with select Chase is displayed. #32906

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and refactor VBBA setup flow #13236

Fix and refactor VBBA setup flow #13236

nkuoch commented Dec 1, 2022 •

edited

Loading

melvin-bot bot commented Dec 1, 2022

OSBotify commented Jan 20, 2023

OSBotify commented Jan 23, 2023

Pujan92 commented Jan 24, 2023 •

edited

Loading

OSBotify commented Jan 25, 2023

luacmartins commented Jan 30, 2023

nkuoch commented Jan 31, 2023

luacmartins commented Jan 31, 2023

luacmartins commented Jan 31, 2023 •

edited

Loading

MariaHCD Feb 16, 2023

NikkiWines commented Mar 2, 2023

Santhosh-Sellavel Mar 27, 2023

mananjadhav commented Apr 27, 2023

allroundexperts Jul 19, 2023

fedirjh Dec 30, 2023

Fix and refactor VBBA setup flow #13236

Fix and refactor VBBA setup flow #13236

Conversation

nkuoch commented Dec 1, 2022 • edited Loading

Details

Fixed Issues

Tests

Offline steps

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Dec 1, 2022

OSBotify commented Jan 20, 2023

OSBotify commented Jan 23, 2023

Pujan92 commented Jan 24, 2023 • edited Loading

OSBotify commented Jan 25, 2023

luacmartins commented Jan 30, 2023

nkuoch commented Jan 31, 2023

luacmartins commented Jan 31, 2023

luacmartins commented Jan 31, 2023 • edited Loading

MariaHCD Feb 16, 2023

Choose a reason for hiding this comment

NikkiWines commented Mar 2, 2023

Santhosh-Sellavel Mar 27, 2023

Choose a reason for hiding this comment

mananjadhav commented Apr 27, 2023

allroundexperts Jul 19, 2023

Choose a reason for hiding this comment

fedirjh Dec 30, 2023

Choose a reason for hiding this comment

nkuoch commented Dec 1, 2022 •

edited

Loading

Pujan92 commented Jan 24, 2023 •

edited

Loading

luacmartins commented Jan 31, 2023 •

edited

Loading