[HOLD VBBA refactor] [$500] Web – Add bank account modal is displayed briefly with 2 options if bank account has been added

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.29-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Log in
3. Create a Workspace and Add bank account
4. Navigate to Settings / Workspace / Cards / Workspace / Bank account

Expected Result:

Directed to a page with the header Connect bank account and the option to disconnect your bank account

Actual Result:

Add bank account modal is displayed briefly with 2 options if bank account has been added. Then directed to a page with the header Connect bank account and the option to disconnect your bank account

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6350916_1705949472773.Add_bank_account_modal.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0182f628dafbfb1237
• Upwork Job ID: 1749519924811567104
• Last Price Increase: 2024-01-22

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0182f628dafbfb1237

[melvin-bot]

Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (External)

[allgandalf]

Proposal

Please re-state the problem that we are trying to solve in this issue.

For some time, we see the bank account modal with 2 options even when the bank was added

What is the root cause of that problem?

In ReimbursementAccountPage, we set the currentStep variable to the default value CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT, if achData is empty.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Line 252 in b1c49db

const currentStep = achData.currentStep || CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT;

Until we update the value of currentStep, we see the default option to select the 2 options to add bank account modal.

when the currentStep will be set to CONST.BANK_ACCOUNT.STEP.ENABLE, at that time correct route will be set to ROUTE_NAMES.ENABLE and then we will see the EnableStep component contents.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Lines 533 to 539 in f3111a4

	if (currentStep === CONST.BANK_ACCOUNT.STEP.ENABLE) {
	return (
	<EnableStep
	reimbursementAccount={reimbursementAccount}
	policyName={policyName}
	onBackButtonPress={goBack}
	/>

What changes do you think we should make in order to solve the problem?

Simply remove the default assignment to CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT and wait for the achData to be set to the latest value

What alternative solutions did you explore? (Optional)

N/A

[DylanDylann]

Proposal

Please re-state the problem that we are trying to solve in this issue.

• Web – Add bank account modal is displayed briefly with 2 options if bank account has been added

What is the root cause of that problem?

• In ReimbursementAccountPage, we use the currentStep to know what we should display.
• This bug appears when the user visit the WorkspaceCardPage before we open the ReimbursementAccountPage. In this case, the ReimbursementAccountPage has the initial value of the reimbursementAccount is:

{
    "isLoading": false,
    "achData": {
        "state": ""
    }
}

• So, hasACHDataBeenLoaded is set to true:
  

  App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

  Line 299 in de3ad34

  setHasACHDataBeenLoaded(true);

• So, the ReimbursementAccountLoadingIndicator component is not displayed:
  

  App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

  Lines 408 to 411 in de3ad34

  	if ((!hasACHDataBeenLoaded || isLoading) && shouldShowOfflineLoader && (shouldReopenOnfido || !requestorStepRef.current)) {
  	const isSubmittingVerificationsData = _.contains([CONST.BANK_ACCOUNT.STEP.COMPANY, CONST.BANK_ACCOUNT.STEP.REQUESTOR, CONST.BANK_ACCOUNT.STEP.ACH_CONTRACT], currentStep);
  	return (
  	<ReimbursementAccountLoadingIndicator

• Instead, it displays the rest content:

What changes do you think we should make in order to solve the problem?

• We need to clear the ONYXKEYS.REIMBURSEMENT_ACCOUNT when WorkspaceCardPage unmounted by adding the logic:

    useEffect(
        () => {
            return () => {
                BankAccounts.clearReimbursementAccount();
            };
        },
        // eslint-disable-next-line react-hooks/exhaustive-deps
        [],
    ); // The empty dependency array ensures this runs only once after the component mounts.

• Or we can call BankAccounts.clearReimbursementAccount() before navigating to ReimbursementAccountPage

What alternative solutions did you explore? (Optional)

• Also, we can update the logic that is responsible for setting the hasACHDataBeenLoaded:
  

  App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

  Lines 297 to 300 in de3ad34

  	if (reimbursementAccount !== ReimbursementAccountProps.reimbursementAccountDefaultProps && reimbursementAccount.isLoading === false) {
  	setShouldShowContinueSetupButton(getShouldShowContinueSetupButtonInitialValue());
  	setHasACHDataBeenLoaded(true);
  	}

[bernhardoj]

This has the same root cause as #31030, so taking my proposal from there.

Proposal

Please re-state the problem that we are trying to solve in this issue.

The bank account page shows the connect option page instead of the all set page.

What is the root cause of that problem?

It's a regression from #29260 where we always clear the bank account data when we close the bank account page.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Lines 278 to 287 in 616d75f

	useEffect(
	() => {
	fetchData();
	return () => {
	BankAccounts.clearReimbursementAccount();
	};
	},
	// eslint-disable-next-line react-hooks/exhaustive-deps
	[],
	); // The empty dependency array ensures this runs only once after the component mounts.

So, when we reopen the bank account page, the page doesn't know we have a finished setup and shows the connect option page.

The purpose of that PR is to solve #27901 (blinking issue). The issue is that when we open the BA page, it will first show the connect option page, a full-screen loader, and then the connect option page.

However, the PR itself is full of flaws.

First, we lost our ongoing BA setup whenever we closed the page.

Second, the second condition below always returns false.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Line 247 in 616d75f

reimbursementAccount !== ReimbursementAccountProps.reimbursementAccountDefaultProps && _.has(reimbursementAccount, 'achData.currentStep'),

_.has(this.props.reimbursementAccount, 'achData.currentStep');

Why? Undercore has can't accept a multi-level object property.

What changes do you think we should make in order to solve the problem?

Undo the changes from #29260 and then we should have a new fix for the blinking issue #27901.

To fix the blinking issue, we should understand the root cause first which is explained in the blinking issue proposal. To put it short, we set the loading state of the BA onyx on mount,

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Lines 278 to 280 in 616d75f

	useEffect(
	() => {
	fetchData();

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Lines 266 to 268 in 616d75f

	function fetchData(ignoreLocalCurrentStep) {
	// Show loader right away, as optimisticData might be set only later in case multiple calls are in the queue
	BankAccounts.setReimbursementAccountLoading(true);

but it takes time, so we initially see the connect option page. After the onyx sends us the new data, a full-screen loading is shown, and after the loading is done, we see the connect option again.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Line 402 in 616d75f

const isLoading = (isLoadingApp || account.isLoading || reimbursementAccount.isLoading) && (!plaidCurrentEvent || plaidCurrentEvent === CONST.BANK_ACCOUNT.PLAID.EVENTS_NAME.EXIT);

From what I understand, we want to immediately show the loading indicator on the component mount. To achieve this, we should have a loading state/props that has an initial value of true. There are 2 ways I can think of to do that:

The first one is to have a new reimbursement account onyx connection with an undefined initial value and only takes the isLoading property. Here is what it looks like.

isReimbursementAccountLoading: {
    key: ONYXKEYS.REIMBURSEMENT_ACCOUNT,
    initWithStoredValues: false,
    selector: (reimbursementAccount) => lodashGet(reimbursementAccount, 'isLoading'),
},

We set initWithStoredValues: false, so the onyx value is always undefined when the component is mounted. Because it's always undefined on mounted, we can set a default isReimbursementAccountLoading props value to true and then replaces all usages of this.props.reimbursementAccount.isLoading with this.props.isReimbursementAccountLoading.

However, initWithStoredValues is currently broken after the onyx cache PR. So, to verify this solution, you can revert the PR locally, specifically this line of code, or see the onyx fix section below.

What alternative solutions did you explore? (Optional)

The second one is to have a new local state called isReimbursementAccountLoading that has an initial value of true. All usages of reimbursementAccount.isLoading will be replaced with isReimbursementAccountLoading.

We will update isLoading state every time reimbursementAccount.isLoading is updated.

const prevIsReimbursementAccountLoading = usePrevious(reimbursementAccount.isLoading);

useEffect(() => {
    if (reimbursementAccount.isLoading === prevIsReimbursementAccountLoading) return
    setIsLoading(reimbursementAccount.isLoading);
}, [prevIsReimbursementAccountLoading, reimbursementAccount.isLoading]);

Another alternative:
Both solutions above assume we want to keep the current behavior, that is by showing the loading indicator on component mount. This solution takes a different approach that is to show the loading indicator only if we never load the data yet.

Below is the solution

if((!hasACHDataBeenLoaded && isLoading)

Currently, we show the loading indicator if it's loading OR we don't have the data yet.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Line 410 in 616d75f

if ((!hasACHDataBeenLoaded || isLoading) && shouldShowOfflineLoader && (shouldReopenOnfido || !requestorStepRef.current)) {

Onyx fix

The root cause of the broken initWithStoredValues is that we initialize all the onyx value from the cached value, even if we set initWithStoredValues as false.

To fix it, we should initialize the onyx key only if initWithStoredValues is true.

-if (
+if (mapping.initWithStoredValues &&
    ((value !== undefined
        && !Onyx.hasPendingMergeForKey(key))
    || mapping.allowStaleData)
) {

Btw, I guess we want to wait for bank account refactor @mountiny

[adelekennedy]

@Santhosh-Sellavel a few proposals to review!

[mountiny]

On hold

[Santhosh-Sellavel]

Thanks for clearing that up!

[Santhosh-Sellavel]

I'm unavailable next week, Please assign a new C+ Issue here if required while I am away, thanks!

cc: @adelekennedy

[Christinadobrzyn]

Hey @Santhosh-Sellavel do you think GH issue will be resolved with this job? (can we consolidate #36376 into this one?)

Other option is to put #36376 on hold until this is fixed to see if it resolves the issue.

[Santhosh-Sellavel]

@Christinadobrzyn Lets put that on hold too.

[adelekennedy]

still on hold

[bernhardoj]

I think we can unhold this?

[nkdengineer]

Coming from #39319 which has the same root cause.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Add bank account modal is displayed briefly with 2 options if bank account has been added. Then directed to a page with the header Connect bank account and the option to disconnect your bank account

What is the root cause of that problem?

This PR #29260 leads to the reimbursement account being cleared whenever ReimbursementAccountPage unmounted, leading to the reimbursement account data being loss and it shows as if there was no reimbursement account (briefly) in other pages like Travel page.

What changes do you think we should make in order to solve the problem?

1. Change this to

BankAccounts.clearPlaid();

We should not clear the reimbursement account fully, we can just clear the Plaid data that is generated when we attempt the connection. This will make sure that the isLoading here could be true, otherwise it will always be false because the plaidCurrentEvent value after the BA connection will be HANDOFF.

Without this step, there won't be any loading indicator when coming to the ReimbursementAccountPage again after BA connection, in web.

2. Use another approach to make sure we always show loading screen when coming to the ReimbursementAccountPage (which is what the offending PR intended to do).

We can set a local isReimbursementAccountLoading state, which defaults to true, only if the reimbursementAccount.isLoading turns from true to false (which means the reimbursement account has been loaded), we'll set isReimbursementAccountLoading to false

const [isReimbursementAccountLoading, setIsReimbursementAccountLoading] = useState(true);

const prevIsReimbursementAccountLoading = usePrevious(reimbursementAccount.isLoading);
useEffect(() => {
    if (prevIsReimbursementAccountLoading && !reimbursementAccount.isLoading) {
        setIsReimbursementAccountLoading(false);
    }
}, [prevIsReimbursementAccountLoading, reimbursementAccount.isLoading]);

Then in here, use isReimbursementAccountLoading instead of reimbursementAccount.isLoading

What alternative solutions did you explore? (Optional)

In addition to the 1st solution part, we can potentially revert the whole offending PR.

An alternative to step 2 is to still use reimbursementAccount.isLoading, but set initWithStoredValues to false, and set the default value of isLoading to true, so that it will always be true initially. But we'll also need to fix the existing Onyx bug where initWithStoredValues is not working, by adding to here the mapping.initWithStoredValues && condition.

Moreover in 1, we can make the condition stricter by only clearing Plaid data on unmount, if the bank account was successfully connected.

[adelekennedy]

I actually think we should close this one given our new standards - this is a bug but isn't very disruptive