Refactor PLAID_DATA and fix plaid bankAccounts selector by nkuoch · Pull Request #11750 · Expensify/App

nkuoch · 2022-10-12T08:51:12Z

Details

Some views where unnecessarily re-rendering because too many "independent" things where stored in PLAID_DATA.
There was a placeholder to plaid bank account selectors, which value (undefined) was somehow passed when selecting another item, due to multiple simultaneous re-rendering.
So, now I made sure only useful data where stored, in appropriate places

Fixed Issues

$ #11554
$ #11769

Tests

Test adding a personal bank account
Test adding a VBBA
Verify that no errors appear in the JS console

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

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

QA Steps

Test adding a personal bank account
Test adding a VBBA
Verify that no errors appear in the JS console

Screenshots

Web

Add a personal bank account via Plaid

2022-10-12_21-35-54.mp4

Add a VBBA via Plaid. Make sure no error occurs

vbba.mp4

Mobile Web - Chrome

Screen.Recording.2022-10-12.at.20.27.03.mov

Mobile Web - Safari

Kapture.2022-10-12.at.16.19.05.mp4

Desktop

Kapture.2022-10-12.at.16.24.13.mp4

iOS

Kapture.2022-10-12.at.17.02.47.mp4

Android

Kapture.2022-10-12.at.17.47.30.mp4

…cessarily when other attributes of PLAID_DATA change

MariaHCD

I think the proposals being discussed here are probably related and would conflict with this PR.

MariaHCD · 2022-10-12T10:36:44Z

     */
    selectAccount(plaidAccountID) {
        const selectedPlaidBankAccount = _.findWhere(this.getPlaidBankAccounts(), {plaidAccountID});
+        if (!selectedPlaidBankAccount) {


Do we know why the selectedPlaidBankAccount can be undefined/null value here? When testing locally (on main) I started running into this error:

Yep, it was a mix of several things that I fixed in this PR. Not it can't be undefined anymore.

github-actions · 2022-10-12T20:09:57Z

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

…at-isPlaidDisabled

marcochavezf · 2022-10-12T21:21:47Z

Review in progress:

Screenshots

Web

Add a PBA (Plaid)

Screen.Recording.2022-10-12.at.16.28.12.mov

Add a VBA (Plaid)

Mobile Web - Chrome

Screen.Recording.2022-10-12.at.20.27.03.mov

Mobile Web - Safari

Desktop

iOS

Screen.Recording.2022-10-12.at.20.38.05.mov

Android

Screen.Recording.2022-10-12.at.20.19.54.mov

…at-isPlaidDisabled

yuwenmemon · 2022-10-12T22:35:50Z

@marcochavezf Updated!

marcochavezf · 2022-10-12T23:08:52Z

I checked that screenshots or videos are included for tests on all platforms

@yuwenmemon I think we (author and reviewer) now should add the screenshots for all platforms

yuwenmemon · 2022-10-12T23:10:03Z

I'll add screenshots

yuwenmemon · 2022-10-13T00:48:17Z

Added!

cead22 · 2022-10-13T01:43:45Z

    /** Contains plaid data */
    plaidData: plaidDataPropTypes,

+    selectedPlaidAccountID: PropTypes.string,


The reviewer checklist says that all props are defined accurately and each prop has a /** comment above it */, although it's under _ If a new component is created I verified that:_ so not sure if this should be a blocker or not

Good catch, we should probably still do that.

cead22 · 2022-10-13T01:44:49Z

        error: PropTypes.string,
        shouldShowSuccess: PropTypes.bool,
        isLoading: PropTypes.bool,
+        plaidAccountID: PropTypes.string,


Maybe all these need to be documented too

cead22 · 2022-10-13T01:46:18Z

@@ -76,7 +78,11 @@ class AddPersonalBankAccountPage extends React.Component {
     * @param {Object} values - form input values passed by the Form component


NAB since this wasn't modified in this PR and I'm not sure if all these are mandatory, the JSDocs style guide says to Document object parameters with separate lines e.g. @PARAM {Object} parameters followed by @PARAM {String} parameters.field

marcochavezf · 2022-10-13T02:13:40Z

Tested both flows (PBA and VBA) on every platform, added the screenshots and videos in this comment.

marcochavezf · 2022-10-13T02:23:18Z

I checked that screenshots or videos are included for tests on all platforms

There's a missing screenshot for the Mobile Web - Chrome platform, but I'm checking off this point ^ since I tested it locally and looks good.

luacmartins

LGTM. I left a clean up comment and I think the test steps could be more descriptive.

luacmartins · 2022-10-13T03:45:02Z

-        });
+            plaidAccessToken: this.props.plaidData.plaidAccessToken,
+        };
+        ReimbursementAccount.updateReimbursementAccountDraft(bankAccountData);


NAB we can clean this up later, but I don't think that we need this since Form automatically updates the draft data if we pass shouldSaveDraft to each input

Suggested change

ReimbursementAccount.updateReimbursementAccountDraft(bankAccountData);

ReimbursementAccount.updateReimbursementAccountDraft(bankAccountData);

Okay, cc @nkuoch and @MariaHCD so you're aware. You're saying that we could just remove this as long as we pass shouldSaveDraft to each form component below, yeah?

Refactor PLAID_DATA and fix plaid bankAccounts selector (cherry picked from commit 6f0d806)

…-11750 🍒 Cherry pick PR #11750 to staging 🍒

OSBotify · 2022-10-13T05:12:31Z

🚀 Cherry-picked to staging by @yuwenmemon in version: 1.2.13-5 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

OSBotify · 2022-10-13T05:54:31Z

🚀 Deployed to production by @yuwenmemon in version: 1.2.13-5 🚀

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

Refactor PLAID_DATA and fix plaid bankAccounts selector

OSBotify · 2023-06-15T01:44:16Z

🚀 Cherry-picked to staging by https://github.com/AndrewGable in version: 1.3.28-2 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

nkuoch self-assigned this Oct 12, 2022

nkuoch changed the title ~~Move isPlaidDisabled to a separate Onyx key to avoid rerendering unne…~~ Move isPlaidDisabled to a separate Onyx key to avoid rerendering unnecessarily when other attributes of PLAID_DATA change Oct 12, 2022

nkuoch force-pushed the nat-isPlaidDisabled branch 2 times, most recently from 418f4f0 to b34d8c6 Compare October 12, 2022 09:39

Move isPlaidDisabled to a separate Onyx key to avoid rerendering unne…

ee52b44

…cessarily when other attributes of PLAID_DATA change

nkuoch force-pushed the nat-isPlaidDisabled branch from b34d8c6 to ee52b44 Compare October 12, 2022 10:02

nkuoch changed the title ~~Move isPlaidDisabled to a separate Onyx key to avoid rerendering unnecessarily when other attributes of PLAID_DATA change~~ Refactor PLAID_DATA Oct 12, 2022

MariaHCD reviewed Oct 12, 2022

View reviewed changes

Trying to fix selectAccount issue

c2ad155

sketchydroide mentioned this pull request Oct 12, 2022

[$250] Investigate: 💥 Crash 💥 value #11554

Closed

nkuoch changed the title ~~Refactor PLAID_DATA~~ Refactor PLAID_DATA and fix plaid bankAccounts selector Oct 12, 2022

nkuoch force-pushed the nat-isPlaidDisabled branch from a291186 to 4a298ca Compare October 12, 2022 19:05

nkuoch marked this pull request as ready for review October 12, 2022 19:17

nkuoch requested a review from a team as a code owner October 12, 2022 19:17

nkuoch requested review from MariaHCD and removed request for a team October 12, 2022 19:17

melvin-bot Bot requested a review from marcochavezf October 12, 2022 19:17

nkuoch force-pushed the nat-isPlaidDisabled branch 2 times, most recently from 4f1aa9e to 5f0722f Compare October 12, 2022 19:48

Fix Plaid selectAccount

b6e47e8

nkuoch force-pushed the nat-isPlaidDisabled branch from 5f0722f to b6e47e8 Compare October 12, 2022 20:03

nkuoch added the CP Staging label Oct 12, 2022

yuwenmemon self-assigned this Oct 12, 2022

yuwenmemon and others added 4 commits October 12, 2022 13:13

Merge branch 'main' of github.com:Expensify/App into nat-isPlaidDisabled

2b6acc4

Revert useless isPlaidDisabled change

2f70daf

Merge branch 'nat-isPlaidDisabled' of github.com:Expensify/App into n…

5296864

…at-isPlaidDisabled

Revert useless changes

494f242

make sure we get account number from the right spot

f4526e3

yuwenmemon added 2 commits October 12, 2022 15:19

Merge branch 'nat-isPlaidDisabled' of github.com:Expensify/App into n…

128f886

…at-isPlaidDisabled

get accountNumber from accountData and use lodash get

e1ade35

cead22 reviewed Oct 13, 2022

View reviewed changes

Add docs

56bb1d9

luacmartins approved these changes Oct 13, 2022

View reviewed changes

yuwenmemon merged commit 6f0d806 into main Oct 13, 2022

yuwenmemon deleted the nat-isPlaidDisabled branch October 13, 2022 03:50

OSBotify pushed a commit that referenced this pull request Oct 13, 2022

Merge pull request #11750 from Expensify/nat-isPlaidDisabled

4800bfa

Refactor PLAID_DATA and fix plaid bankAccounts selector (cherry picked from commit 6f0d806)

OSBotify mentioned this pull request Oct 13, 2022

🍒 Cherry pick PR #11750 to staging 🍒 #11789

Merged

OSBotify added a commit that referenced this pull request Oct 13, 2022

Merge pull request #11789 from Expensify/OSBotify-cherry-pick-staging…

1a54e1a

…-11750 🍒 Cherry pick PR #11750 to staging 🍒

OSBotify mentioned this pull request Oct 13, 2022

Deploy Checklist: New Expensify 2022-10-11 #11741

Closed

48 tasks

OSBotify mentioned this pull request Oct 13, 2022

Deploy Checklist: New Expensify 2022-10-13 #11793

Closed

9 tasks

smrutiparida pushed a commit to autosave-app/App that referenced this pull request Oct 13, 2022

Merge pull request Expensify#11750 from Expensify/nat-isPlaidDisabled

709d0f4

Refactor PLAID_DATA and fix plaid bankAccounts selector

OSBotify mentioned this pull request Oct 14, 2022

Deploy Checklist: New Expensify 2022-10-14 #11873

Closed

53 tasks

sobitneupane mentioned this pull request Oct 15, 2022

[HOLD for payment 2022-11-04] [$250] BUG: Enter password field shows on add bank account page while trying to add bank accounts consecutively reported by @adeel0202 #11816

Closed

OSBotify mentioned this pull request May 2, 2023

Deploy Checklist: New Expensify 2023-05-02 #18315

Closed

OSBotify mentioned this pull request Jun 15, 2023

Deploy Checklist: New Expensify 2023-06-14 #20782

Closed

		@@ -76,7 +78,11 @@ class AddPersonalBankAccountPage extends React.Component {
		* @param {Object} values - form input values passed by the Form component

	ReimbursementAccount.updateReimbursementAccountDraft(bankAccountData);
	ReimbursementAccount.updateReimbursementAccountDraft(bankAccountData);

Conversation

nkuoch commented Oct 12, 2022 • edited by yuwenmemon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Uh oh!

MariaHCD left a comment

Choose a reason for hiding this comment

Uh oh!

MariaHCD Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkuoch Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 12, 2022

Uh oh!

marcochavezf commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Uh oh!

yuwenmemon commented Oct 12, 2022

Uh oh!

marcochavezf commented Oct 12, 2022

Uh oh!

yuwenmemon commented Oct 12, 2022

Uh oh!

yuwenmemon commented Oct 13, 2022

Uh oh!

cead22 Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

yuwenmemon Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

cead22 Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

cead22 Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

marcochavezf commented Oct 13, 2022

Uh oh!

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

Uh oh!

Uh oh!

luacmartins left a comment

Choose a reason for hiding this comment

Uh oh!

luacmartins Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

yuwenmemon Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Oct 13, 2022

Uh oh!

OSBotify commented Oct 13, 2022

Uh oh!

OSBotify commented Jun 15, 2023

Uh oh!

nkuoch commented Oct 12, 2022 •

edited by yuwenmemon

Loading

MariaHCD Oct 12, 2022 •

edited

Loading

marcochavezf commented Oct 12, 2022 •

edited

Loading

marcochavezf commented Oct 13, 2022 •

edited

Loading