Add Pattern B to creating Personal Bank Account by ctkochan22 · Pull Request #10953 · Expensify/App

ctkochan22 · 2022-09-13T05:26:22Z

cc @MariaHCD @nkuoch

Details

We are adding pattern b to creating a Personal Bank Account.

Hold https://github.com/Expensify/Web-Expensify/pull/34855

Features:

We now show pending bank accounts in ONYX data after they successfully log into plaid
If there is an error with the PBA, we will show the red brick road

Fixed Issues

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

Tests

Same as QA

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Basic flow

Go to Payments -> Add Payment Method -> Bank Account
Add Plaid login credentials user_good, pass_good, credential_good (if they ask for a code)
Agree to any Plaid Sandbox to pass
Select an account and input an incorrect password
Verify that you see an error below the password field
Click the "x" on the error, verify that it goes away.
Now input the correct password
Verify that the personal bank account was added

Test Offline Behavior

Pick up where you left off last section
Type in an incorrect password
Before submitting, go offline.
Submit. Verify that everything is greyed out
Go back online and verify that the error still comes up.
Go back offline and submit again.
Now navigate away from the modal (or close it).
Then go back online, and verify that you see a red brick indicator come up.
Avatar:

Payments menu item

Red brick near the pending bank account
Now click on the pending bank account and then verify that you see the error on the form
Now go back offline.
Type in the correct password
Go offline and then submit.
Navigate away. Then go back online
Go to payments page and verify that the new bank account has been added.

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions · 2022-09-13T05:26:39Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

ctkochan22 · 2022-09-13T05:27:37Z

To do:

Need to fix the opacity issue with the red brick indicator
Fix merge conflicts

nkuoch · 2022-09-29T08:12:45Z

-        const selectedPlaidBankAccount = lodashGet(this.props, 'plaidData.selectedPlaidBankAccount', {});
        const {icon, iconSize} = getBankIcon();

+        // Show selected plaid bank account. If not set, check the props to see if we have one pre-selected


when does that happen? Also, did you mean state instead of props?

Well the state of AddPersonalBankAccountPage, but it's being passed into here as a prop.

Sorry I'm still trying to wrap my mind around the merge with your PR.

So when you select an account here, we set the state in AddPersonalBankAccount. I wanted to keep the bank account we selected when the user navigate back to the form.

Then let's just pass selectedPlaidAccountID via props (add add it to the propTypes)

Oh I just realized there is a bug (minor edge case) where this all gets messed up if someone is setting up a VBA (using BankAccounts.updatePlaidData) and also setting up a PBA.

Try to create a PBA.

Login into plaid, then selecting the test account that ends with 000. Add an incorrect password and submit (creates a pending PBA)

Now go to your workspace settings. Choose "Connect bank account" to setup VBA. Log into plaid

Selected the bank account 1111.

Navigate bank to the PBA, it will be set to 1111.

Click 0000, go to next tab, the state forced it back to 1111. We just need to run update here too

We can just continue to use updatePlaidData here too?

ctkochan22 · 2022-09-30T05:33:18Z

            setDefaultFailure: 'Something went wrong. Please chat with Concierge for further assistance.',
        },
-        addBankAccountFailure: 'And unexpected error occurred while trying to add your bank account. Please try again.',
+        addBankAccountFailure: 'An unexpected error occurred while trying to add your bank account. Please try again.',


ctkochan22 · 2022-09-30T06:16:42Z

Updated

ctkochan22 · 2022-10-03T17:07:51Z

Updated please review again! 🙇

MariaHCD

If a user submits the form when offline, they are shown the below which isn't ideal UX because it is an infinite spinner:

After submitting, can we navigate to this page instead:

MariaHCD · 2022-10-04T08:08:05Z

+        return {};
+    }
+
+    // Get errors


Why are we reformatting the errors here?

This is how we use to. How do we pass back errors now? Atm we don't seem to pass back any errors within the item itself.

We pass errors back in timestamp => errorMessage format, but the item object takes an array of error messages

Sorry, I don't think I'm following the logic in this function. So we're basically fetching the personalBankAccount currently being setup, taking the errors and adding it to pendingBankAccount (retrieved from the Plaid data). And we're doing this because we need some additional properties like the account number, address name, etc. in formatPaymentMethods. Is that right?

Perhaps an alternative here would be to take the properties we need from pendingBankAccount and add them to personalBankAccount instead. So we won't have to do any reformatting of the errors. Not a major win but maybe would make this more understandable?

Ah, so we have bank account info in plaidData. Stuff like unmasked account numbers and such. Data we need to pass to PHP when trying to add a bank account.

Currently, when you try to add a bank account, we let the data sit in plaidData and just save selectePlaidAccountID in the onyxkey personalBankAccount. We currently use that to find the bank account they selected in plaidData. And we then format that data so that it would appear in the payment list.

You are suggesting that upon submission, we save data we need, formatted already, into personalBankAccount. So when we show the payment methods, we don't have to do anything here.

I'm good with that. My argument against that is, we would be doing some sort of "massaging" somewhere in the client side, because its client side data. So instead of doing all the formatting and such here, we'd do it in BankAccounts.addPersonalBankAccount. It may make this method cleaner, but still somewhat convoluted? And I think it'll make it harder to follow.

Thanks for clarifying! I like your suggestion:

So instead of doing all the formatting and such here, we'd do it in `BankAccounts.addPersonalBankAccount`

I think perhaps the flow would be easier to understand if we did the above. Would something like this work:

The user submits the add bank account form

We fetch the all we need from plaidData and store it in the personalBankAccount key

We can display the pending bank account in the payment methods list without needing any more formatting in formatPaymentMethods(...)

MariaHCD · 2022-10-04T08:10:52Z

+    // See if we need to show a pending bank account in the payment methods list
+    if (!_.isEmpty(pendingBankAccount)) {


Is this data massaging something we can do in OpenPaymentsPage instead?

I don't think so. This pending bank account is all local data. None of this is stored on the server-side. Just like plaid data

Is this data massaging? Something can be empty or undefined.

Is this data massaging?

Ah, sorry, I wasn't clear. I meant the data massaging on lines 43 - 54.

But if this data is all local data then I don't see how we can avoid this 🤔

ctkochan22 · 2022-10-04T22:06:50Z

After submitting, can we navigate to this page instead:

You mean when they submit offline. We could, although I feel like we common practice is to keep them there? We give them the option to navigate away, but what if they only lost connection for a second? It would be annoying to be navigated away when you are a second away from getting feedback

MariaHCD · 2022-10-05T08:22:10Z

+    // See if we need to show a pending bank account in the payment methods list
+    if (!_.isEmpty(pendingBankAccount)) {


Is this data massaging?

Ah, sorry, I wasn't clear. I meant the data massaging on lines 43 - 54.

But if this data is all local data then I don't see how we can avoid this 🤔

MariaHCD · 2022-10-05T08:52:08Z

+        return {};
+    }
+
+    // Get errors


Sorry, I don't think I'm following the logic in this function. So we're basically fetching the personalBankAccount currently being setup, taking the errors and adding it to pendingBankAccount (retrieved from the Plaid data). And we're doing this because we need some additional properties like the account number, address name, etc. in formatPaymentMethods. Is that right?

Perhaps an alternative here would be to take the properties we need from pendingBankAccount and add them to personalBankAccount instead. So we won't have to do any reformatting of the errors. Not a major win but maybe would make this more understandable?

MariaHCD · 2022-10-05T08:56:56Z

although I feel like we common practice is to keep them there? We give them the option to navigate away, but what if they only lost connection for a second? It would be annoying to be navigated away when you are a second away from getting feedback

That's an interesting point. Maybe just my perspective but if I was offline and saw the whole form go blank, it wouldn't be clear what was happening.

If I was redirected to the payment methods page showing the bank account greyed out, it would be clear that it's pending creation.

MariaHCD · 2022-10-05T08:58:53Z

Also, not sure if related to this PR but the loading indicator isn't centered anymore

ctkochan22 · 2022-10-06T06:41:15Z

Okay, we are close. I can feel it!

Issue: We want to avoid all the massaging here.

We currently have all the unformatted data saved in the onyxkey plaidData
When the user hits the submit button(addPersonalBankAccount()), we will add the key pendingBankAccount to the onyxkey personalBankAccount. Which will store all the formatted data we need to display it on the payment method list.
- accountData, accountType, description, etc
When they click on the pending bank account from the payment method list, they will go back to the add personal bank account form. We skip plaid and the form will be filled according to plaidData.selectedPlaidAccount

This is the solution @MariaHCD and I agreed on. Can you take a look and let us know your thoughts so we are on the same page?

MariaHCD · 2022-10-06T06:48:32Z

That approach makes sense to me.

we will add the key pendingBankAccount to the onyxkey personalBankAccount

Perhaps we can have ^ set in the optimisticData here.

Alternate proposal for pattern B of AddPersonalBankAccount

ctkochan22 · 2022-10-07T18:22:24Z

https://github.com/Expensify/Expensify/issues/214739#issuecomment-1271918173

Kosuke Tseng added 15 commits August 30, 2022 16:10

Add offlinefeedback

d683905

Add pending and error handling

a96a4d9

add onclose

1481056

pass plaid index

c3ac6be

fix up onyxdata

a500ed6

fix persisting plaid data

c440439

dry up PM navigation

b4c8291

fix dependency cycle

6456f3b

Fix lint issues

31f5848

if bank account is pending, naviagte to add-bank-account

9a72191

Add pending key

7a4eccc

dry up format numebr method

4208af0

add errors

819aee1

add brick indicator

b1f3c6a

Add pending style

5c2be25

ctkochan22 requested a review from a team as a code owner September 13, 2022 05:26

ctkochan22 self-assigned this Sep 13, 2022

ctkochan22 changed the base branch from master to main September 13, 2022 05:26

melvin-bot Bot requested review from danieldoglas and removed request for a team September 13, 2022 05:26

MariaHCD self-requested a review September 13, 2022 08:49

Kosuke Tseng added 5 commits September 13, 2022 13:24

Fix opacity

30b498b

move button to bottom

c4d7c30

wrap button

ec40de5

merge main

1adc86f

style

39bd57a

ctkochan22 requested a review from nkuoch September 14, 2022 20:00

ctkochan22 changed the title ~~[WIP] Add Pattern B to creating Personal Bank Account~~ Add Pattern B to creating Personal Bank Account Sep 14, 2022

nkuoch reviewed Sep 29, 2022

View reviewed changes

Comment thread src/components/personalBankAccountPropTypes.js

nkuoch reviewed Sep 29, 2022

View reviewed changes

Comment thread src/components/personalBankAccountPropTypes.js Outdated

nkuoch reviewed Sep 29, 2022

View reviewed changes

Comment thread src/components/personalBankAccountPropTypes.js Outdated

fix error handling

d26d1de

ctkochan22 commented Sep 30, 2022

View reviewed changes

merge main and fix conflicts

93e39d8

Kosuke Tseng added 3 commits October 3, 2022 09:46

Add method to collect selected bank object"

b4f80b2

update to handle pendingBankAccount

4cb1d6b

update proptypes

8704adb

MariaHCD reviewed Oct 4, 2022

View reviewed changes

danieldoglas removed their request for review October 4, 2022 14:15

Kosuke Tseng added 2 commits October 4, 2022 14:51

clean up

caf1f08

don't show button spinner if action is offline

147e723

MariaHCD reviewed Oct 5, 2022

View reviewed changes

fix comment

0ea6bc6

nkuoch mentioned this pull request Oct 6, 2022

Alternate proposal for pattern B of AddPersonalBankAccount #11630

Merged

91 tasks

nkuoch and others added 3 commits October 6, 2022 16:08

Alternate proposal for pattern B of AddPersonalBankAccount

cfcdf71

Apply Pattern B only when offline

11da5c2

Merge pull request #11630 from Expensify/nat-ckt

bc1ce2a

Alternate proposal for pattern B of AddPersonalBankAccount

ctkochan22 closed this Oct 7, 2022

coleaeason deleted the ckt_refactor_PBA_patternb branch May 19, 2026 19:33

		// See if we need to show a pending bank account in the payment methods list
		if (!_.isEmpty(pendingBankAccount)) {

Uh oh!

Conversation

ctkochan22 commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

github-actions Bot commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ctkochan22 commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctkochan22 commented Sep 30, 2022

Uh oh!

ctkochan22 commented Oct 3, 2022

Uh oh!

MariaHCD left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

ctkochan22 commented Oct 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ctkochan22 commented Sep 13, 2022 •

edited

Loading

github-actions Bot commented Sep 13, 2022 •

edited

Loading

ctkochan22 commented Sep 13, 2022 •

edited

Loading