Refactor ReimbursementAccountPage's fetchFreePlanVerifiedBankAccount #11751

ctkochan22 · 2022-10-12T08:52:44Z

@nkuoch @MariaHCD @youssef-lr

Details

This replaces the use of fetchFreePlanVerifiedBankAccount

Fixed Issues

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

Tests / QA

Test a bank account in SETUP

Start the VBBA process. Select plaid (use user_good and pass_good to get past plaid)
Add any fields to the Company Step, the key is to make it past this step
After submission, you should be on the RequestorStep "Personal Information"
Close out of the flow. Back all the way to a chat window
Navigate back to the VBA flow (Settings -> Workspace settings -> Connect Bank Account -> Continue with setup)
Verify that you are shown back to the Requestor Step "Personal information"
Now log out to clear your cache
Log back in
Navigate back to the VBA flow (Settings -> Workspace settings -> Connect Bank Account)
Verify that you are shown back to the Requestor Step "Personal information"

Test Requestor Step without completing Onfido

With the same VBBA setup, fill out the requestor step (Do not use Alberta Charleston from the SO, use a random name)
It should take you to Onfido flow
Don't complete onfido. Exit the flow
Navigate back to the VBA flow (Settings -> Workspace settings -> Connect Bank Account -> Continue with setup)
Verify that you are brought back to the Onfido Step
Log out and log back in
Navigate back to the VBA flow
Verify that you are brought back to the Requestor Step ("Personal information"). And everything is pre-filled
When you click submit, you still have to go through Onfido

Test Beneficial Owners Step

Once onfido is complete, you should be moved to the ACHContract Step or Beneficial Owners Step or "Additional information"
Exit flow
Navigate back to the VBA flow (Settings -> Workspace settings -> Connect Bank Account -> Continue with setup)
Verify that you are still on that step you left off
Log out and log back in
Navigate back to the VBA flow
Verify that you are still on that step

Test VERIFYING state

So long as you did not add Alberta Charleston as the name, the VBBA should be in a VERIFYING state
Verify that you are asked to provide more information:
Click "Yes lets chat"
Navigate back to the VBA flow (Settings -> Workspace settings -> Connect Bank Account -> Continue with setup)
Verify that you are still on that step/need to provide more info
Log out and log back in.
Navigate back to the VBA flow
Verify that you are still on that step

Test PENDING state

Follow the SO instructions to add a PENDING VBBA (link).
Get to the Validation Step
Navigate away
Navigate back to the VBBA flow
Verify that you see the "Continue Setup"/"Start-Over".
Click continue
Verify that you see Validation Step "Test Transactions"

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

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

https://recordit.co/fseVdmCoMz

iOS

https://recordit.co/ygknviefmP

Android

src/libs/actions/BankAccounts.js

ctkochan22 · 2022-10-18T07:38:35Z

Still need to figure out QA and screenshots from every platform, but this can begin initial review I think and testing

src/pages/ReimbursementAccount/ReimbursementAccountPage.js

youssef-lr · 2022-10-20T21:22:13Z

I don't know what's up with my VM, I'm getting a CORS error when trying to open the VBBA page. Trying to figure out what's causing this so I can test the PR.

MariaHCD

The code LGTM so far!

I believe we also need to refactor the usage of fetchFreePlanVerifiedBankAccount in the WorkspacePageWithSections component:

App/src/pages/workspace/WorkspacePageWithSections.js

Lines 83 to 86 in 3a5d99b

    
           fetchData() { 
        
               const achState = lodashGet(this.props.reimbursementAccount, 'achData.state', ''); 
        
               BankAccounts.fetchFreePlanVerifiedBankAccount('', achState); 
        
           }

MariaHCD · 2022-10-21T11:58:29Z

I don't know what's up with my VM, I'm getting a CORS error when trying to open the VBBA page. Trying to figure out what's causing this so I can test the PR.

@youssef-lr Possibly you need to upgrade to PHP8.1?

youssef-lr · 2022-10-21T12:16:09Z

@MariaHCD I've already did. I think I'll point App to staging to test this while I figure it out.

MariaHCD

Tested this locally:

Test a bank account in SETUP: worked as expected
Test Requestor Step without completing Onfido: worked as expected, except for logging out and logging back in. The personal information form did not have the form data pre-filled

New.Expensify.2.webm

Test Beneficial Owners Step: worked as expected
Test PENDING state: haven't tested this yet. We can remove At the moment, the SO instructions to add a PENDING VBBA is broken since that bug has been fixed.

ctkochan22 · 2022-11-02T07:06:38Z

This should be ready to go @MariaHCD @nkuoch

The only issue to note is, as noted in this bug https://github.com/Expensify/Expensify/issues/238317 . When testing and you sign in for the first time, there will be no VBBA and we will not fetch it at any point until we click "Continue Setup". This means, we won't see the option to continue previous setup or start over

nkuoch · 2022-11-02T22:47:13Z

src/libs/actions/ReimbursementAccount/setupWithdrawalAccount.js

@@ -58,7 +58,7 @@ function getNextStep(updatedACHData) {
        return currentStep;
    }

-    if (currentStep === CONST.BANK_ACCOUNT.STEP.VALIDATION && updatedACHData.bankAccountInReview) {


Did you make sure no other places used bankAccountInReview?

Oh I need to undo this. We actually removed bankAccountInReview completely from the web/auth side.

And looking at this page, I don't think this method is used anymore? Its used in setupWithdrawalAccount(), but that is no longer being used I don't think.

I'm going to undo this change

setupWithdrawalAccount() isn't being used anymore, created an issue to remove this file completely: https://github.com/Expensify/Expensify/issues/240702

MariaHCD

Could you also update the Jest unit tests? We're currently using fetchFreePlanVerifiedBankAccount in ReimbursementAccountTest.js

Can we also get rid of tests/unit/fetchFreePlanVerifiedBankAccountTest.js?

ctkochan22 · 2022-11-08T06:05:25Z

Could you also update the Jest unit tests? We're currently using fetchFreePlanVerifiedBankAccount in ReimbursementAccountTest.js

I'm not super familiar with Jest tests. But it seems like it doesn't actually hit an API? If that is the case, is there much to test? We handle so much of it on the php side.

MariaHCD · 2022-11-08T12:37:28Z

I'm not super familiar with Jest tests. But it seems like it doesn't actually hit an API? If that is the case, is there much to test? We handle so much of it on the php side.

Ah, good point. We're just mocking the API responses in the Jest tests. And since most of the logic is now in the API, we shouldn't need these tests anymore, correct?

MariaHCD · 2022-11-08T13:20:14Z

@ctkochan22 Could you re-fill the PR author checklist? Perhaps it's outdated which is why the test is failing.

MariaHCD · 2022-11-08T13:50:59Z

Noticed when testing this that if you log out and then back in again, the forms that you completed before are no longer populated with the data:

Screen.Recording.2022-11-08.at.5.46.25.PM.mov

ctkochan22 · 2022-11-08T18:43:19Z

Woah, this is happening because the path here is incorrect.

App/src/libs/ReimbursementAccountUtils.js

Line 26 in 4d20ee5

    
           || lodashGet(props, ['reimbursementAccount', 'achData', fieldName], defaultValue);

Was that changed recently?

ctkochan22 · 2022-11-09T08:41:34Z

@youssef-lr Can you test this today please? 🙇

youssef-lr · 2022-11-09T12:41:01Z

@ckscanlon testing now!

MariaHCD · 2022-11-09T13:29:52Z

Heads up @youssef-lr, if you run into this error: #11751 (comment)

You can test with the Web-E PR: https://github.com/Expensify/Web-Expensify/pull/35506 and it will work

youssef-lr · 2022-11-09T14:07:04Z

@ckscanlon after running the query in the last step, this is what I'm seeing, am I missing something? All other tests worked as expected. cc @MariaHCD I think Chris is offline.

ctkochan22 · 2022-11-09T15:09:34Z

@youssef-lr Is the bank account ending in 9378 the new bank account in the validate state?

ctkochan22 · 2022-11-09T17:40:25Z

@youssef-lr my bad, the test steps were outdated. Please try the last step when you can! And this PR should be ready to go!

ctkochan22 · 2022-11-10T04:59:20Z

I'm not sure why this PR didn't have a C+ assigned

youssef-lr

Tested well!

youssef-lr · 2022-11-10T14:05:57Z

PR Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

OSBotify · 2022-11-10T14:12:07Z

✋ 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 · 2022-11-11T03:39:30Z

🚀 Deployed to staging by @youssef-lr in version: 1.2.27-0 🚀

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

Luke9389 · 2022-11-11T22:25:24Z

src/libs/actions/BankAccounts.js

+                key: ONYXKEYS.REIMBURSEMENT_ACCOUNT,
+                value: {
+                    errors: null,
+                    isLoading: true,


is it this @ctkochan22 ?

Are we stuck in the isLoading state?

but upon any sort of completion it should be set to false

OSBotify · 2022-11-15T21:10:14Z

🚀 Deployed to production by @roryabraham in version: 1.2.27-4 🚀

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

OSBotify · 2022-11-16T00:18:07Z

🚀 Deployed to production by @roryabraham in version: 1.2.27-4 🚀

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

Kosuke Tseng added 6 commits October 4, 2022 11:05

add new command

6397999

Merge branch 'main' into ckt_fetchFreePlan_reimbursementPage_2

4e3f948

get local items

36d1277

Merge branch 'main' into ckt_fetchFreePlan_reimbursementPage_2

06d3a45

pass in params

20da5bf

remove old usage

24ec6ea

ctkochan22 self-assigned this Oct 12, 2022

ctkochan22 requested a review from a team as a code owner October 12, 2022 08:52

melvin-bot bot requested review from youssef-lr and removed request for a team October 12, 2022 08:53

ctkochan22 changed the base branch from master to main October 12, 2022 08:58

youssef-lr reviewed Oct 12, 2022

View reviewed changes

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

youssef-lr reviewed Oct 12, 2022

View reviewed changes

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

Kosuke Tseng added 2 commits October 12, 2022 09:24

update param to isBankAccountINReview

79958f7

Merge branch 'main' into ckt_fetchFreePlan_reimbursementPage_2

0f99df9

ctkochan22 requested review from nkuoch and MariaHCD October 18, 2022 07:31

ctkochan22 changed the title ~~[WIP] Refactor ReimbursementAccountPage's fetchFreePlanVerifiedBankAccount~~ Refactor ReimbursementAccountPage's fetchFreePlanVerifiedBankAccount Oct 18, 2022

optimistic data

68f70e7

nkuoch reviewed Oct 18, 2022

View reviewed changes

src/pages/ReimbursementAccount/ReimbursementAccountPage.js Outdated Show resolved Hide resolved

Kosuke Tseng added 3 commits October 18, 2022 18:31

cleanup

34f6a52

Merge branch 'main' into ckt_fetchFreePlan_reimbursementPage_2

7f48071

Merge branch 'main' into ckt_fetchFreePlan_reimbursementPage_2

3a5d99b

MariaHCD reviewed Oct 21, 2022

View reviewed changes

nkuoch previously approved these changes Oct 21, 2022

View reviewed changes

nkuoch reviewed Nov 2, 2022

View reviewed changes

undo unnecessary change

206f310

nkuoch approved these changes Nov 4, 2022

View reviewed changes

MariaHCD approved these changes Nov 7, 2022

View reviewed changes

MariaHCD requested changes Nov 7, 2022

View reviewed changes

MariaHCD approved these changes Nov 8, 2022

View reviewed changes

youssef-lr approved these changes Nov 10, 2022

View reviewed changes

youssef-lr merged commit 0c65dbe into main Nov 10, 2022

youssef-lr deleted the ckt_fetchFreePlan_reimbursementPage_2 branch November 10, 2022 14:07

OSBotify mentioned this pull request Nov 11, 2022

Deploy Checklist: New Expensify 2022-11-11 #12662

Closed

27 tasks

ctkochan22 mentioned this pull request Nov 11, 2022

Connect Bank Account - Endless loading is shown when navigate to Connect Bank Account #12670

Closed

Luke9389 reviewed Nov 11, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ReimbursementAccountPage's fetchFreePlanVerifiedBankAccount #11751

Refactor ReimbursementAccountPage's fetchFreePlanVerifiedBankAccount #11751

ctkochan22 commented Oct 12, 2022 •

edited

Loading

ctkochan22 commented Oct 18, 2022

youssef-lr commented Oct 20, 2022

MariaHCD left a comment

MariaHCD commented Oct 21, 2022

youssef-lr commented Oct 21, 2022 •

edited

Loading

MariaHCD left a comment

ctkochan22 commented Nov 2, 2022

nkuoch Nov 2, 2022

ctkochan22 Nov 3, 2022

ctkochan22 Nov 3, 2022

MariaHCD Nov 4, 2022

MariaHCD left a comment •

edited

Loading

ctkochan22 commented Nov 8, 2022

MariaHCD commented Nov 8, 2022

MariaHCD commented Nov 8, 2022

MariaHCD commented Nov 8, 2022

ctkochan22 commented Nov 8, 2022

ctkochan22 commented Nov 9, 2022

youssef-lr commented Nov 9, 2022

MariaHCD commented Nov 9, 2022

youssef-lr commented Nov 9, 2022 •

edited

Loading

ctkochan22 commented Nov 9, 2022

ctkochan22 commented Nov 9, 2022

ctkochan22 commented Nov 10, 2022

youssef-lr left a comment

youssef-lr commented Nov 10, 2022

PR Reviewer Checklist

OSBotify commented Nov 10, 2022

OSBotify commented Nov 11, 2022

Luke9389 Nov 11, 2022

ctkochan22 Nov 11, 2022

OSBotify commented Nov 15, 2022

OSBotify commented Nov 16, 2022

	fetchData() {
	const achState = lodashGet(this.props.reimbursementAccount, 'achData.state', '');
	BankAccounts.fetchFreePlanVerifiedBankAccount('', achState);
	}

Refactor ReimbursementAccountPage's fetchFreePlanVerifiedBankAccount #11751

Refactor ReimbursementAccountPage's fetchFreePlanVerifiedBankAccount #11751

Conversation

ctkochan22 commented Oct 12, 2022 • edited Loading

Details

Fixed Issues

Tests / QA

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

ctkochan22 commented Oct 18, 2022

youssef-lr commented Oct 20, 2022

MariaHCD left a comment

Choose a reason for hiding this comment

MariaHCD commented Oct 21, 2022

youssef-lr commented Oct 21, 2022 • edited Loading

MariaHCD left a comment

Choose a reason for hiding this comment

ctkochan22 commented Nov 2, 2022

nkuoch Nov 2, 2022

Choose a reason for hiding this comment

ctkochan22 Nov 3, 2022

Choose a reason for hiding this comment

ctkochan22 Nov 3, 2022

Choose a reason for hiding this comment

MariaHCD Nov 4, 2022

Choose a reason for hiding this comment

MariaHCD left a comment • edited Loading

Choose a reason for hiding this comment

ctkochan22 commented Nov 8, 2022

MariaHCD commented Nov 8, 2022

MariaHCD commented Nov 8, 2022

MariaHCD commented Nov 8, 2022

ctkochan22 commented Nov 8, 2022

ctkochan22 commented Nov 9, 2022

youssef-lr commented Nov 9, 2022

MariaHCD commented Nov 9, 2022

youssef-lr commented Nov 9, 2022 • edited Loading

ctkochan22 commented Nov 9, 2022

ctkochan22 commented Nov 9, 2022

ctkochan22 commented Nov 10, 2022

youssef-lr left a comment

Choose a reason for hiding this comment

youssef-lr commented Nov 10, 2022

PR Reviewer Checklist

OSBotify commented Nov 10, 2022

OSBotify commented Nov 11, 2022

Luke9389 Nov 11, 2022

Choose a reason for hiding this comment

ctkochan22 Nov 11, 2022

Choose a reason for hiding this comment

OSBotify commented Nov 15, 2022

OSBotify commented Nov 16, 2022

ctkochan22 commented Oct 12, 2022 •

edited

Loading

youssef-lr commented Oct 21, 2022 •

edited

Loading

MariaHCD left a comment •

edited

Loading

youssef-lr commented Nov 9, 2022 •

edited

Loading