[$500] Bank account - Ongoing BA setup shows Connect online instead of Continue setup in offline mode

[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.3.96-5
Reproducible in staging?: Y
Reproducible in production?: N/A, cannot check bank account on production
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. Navigate to staging.new.expensify.com
2. Go to Settings > Workspaces > any workspace
3. Go to Bank account > Connect manually
4. Proceed until the final step (Test transactions)
5. Close the setup
6. Go offline
7. Go to Settings > Workspaces > any workspace > Go to Bank account

Expected Result:

The Bank account page shows Continue with setup and Start over options

Actual Result:

The Bank account page shows Connect online with Plaid and Connect manually

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

Bug6267925_1699388183173.20231108_012714.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018f1012ebf2275e7d
• Upwork Job ID: 1721991376582500352
• Last Price Increase: 2023-12-01

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~018f1012ebf2275e7d

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

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

[dukenv0307]

Proposal

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

In offline mode, bank account page show connect online with plaid and connect manually

What is the root cause of that problem?

We clear reimbursementAccount whenever this page is unmounted.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Lines 188 to 190 in c2e4042

	componentWillUnmount() {
	BankAccounts.clearReimbursementAccount();
	}

When online, the page will get reimbursementAccount from BE, and the continue step displays correctly. But when offline, we cannot get reimbursementAccount from BE and it was cleared from Onyx before so it displays connect online with Plaid and connect manually

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

We cleared reimbursementAccount to prevent this page is blinked when revisiting this page, so I think we should show loading page in offline mode or display not found page with a description like We should enable the internet to connect bank account

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Lines 188 to 190 in c2e4042

	componentWillUnmount() {
	BankAccounts.clearReimbursementAccount();
	}

What alternative solutions did you explore? (Optional)

If we want to display this page in offline mode, we should not clear reimbutrsementAccount whenever the page is unmounted and we will always display loading page until the API is complete when the page is mounted if we are online. If we offline, display the current step with local reimbutrsementAccount data.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Lines 188 to 190 in c2e4042

	componentWillUnmount() {
	BankAccounts.clearReimbursementAccount();
	}

[situchan]

Dupe of #26631

[adelekennedy]

Closing this

[bernhardoj]

Proposal

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

(Offline) The bank account page shows the connect option page instead of the continue setup page even though we have an ongoing bank account setup.

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 188 to 190 in b33ac36

	componentWillUnmount() {
	BankAccounts.clearReimbursementAccount();
	}

So, when we reopen the bank account page, the page doesn't know we have an ongoing 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.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Lines 189 to 191 in f168423

	componentWillUnmount() {
	BankAccounts.clearReimbursementAccount();
	}

Second, the second condition below always returns false.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Lines 113 to 114 in f168423

	const hasACHDataBeenLoaded =
	this.props.reimbursementAccount !== ReimbursementAccountProps.reimbursementAccountDefaultProps && _.has(this.props.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 on 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 121 to 123 in f168423

	componentDidMount() {
	this.fetchData();
	}

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Lines 311 to 313 in f168423

	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

Lines 401 to 402 in f168423

	const isLoading =
	(this.props.isLoadingReportData || this.props.account.isLoading || this.props.reimbursementAccount.isLoading) &&

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 isLoading that has an initial value of true. All usages of this.props.reimbursementAccount.isLoading will be replaced with this.state.isLoading.

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

componentDidUpdate(prevProps) {
    if (prevProps.reimbursementAccount.isLoading !== this.props.reimbursementAccount.isLoading) {
        this.setState({isLoading: this.props.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((!this.state.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 414 in 5d5af8f

if ((!this.state.hasACHDataBeenLoaded || isLoading) && shouldShowOfflineLoader && (shouldReopenOnfido || !this.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)
) {

[laurenreidexpensify]

@adelekennedy @abdulrahuman5196 after discussion in #26631 let's reopen this and keep these as two separate issues

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[adelekennedy]

issue reopened - @abdulrahuman5196 do you mind checking these proposals again (including the one added after the issue was closed)?

[abdulrahuman5196]

Got it. Will check the proposals in my morning.

[melvin-bot]

@abdulrahuman5196 @adelekennedy this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot]

Current assignee @abdulrahuman5196 is eligible for the Internal assigner, not assigning anyone new.

[abdulrahuman5196]

Reviewing now

[abdulrahuman5196]

I gone through the proposals at a glance and the linked issue. I would need more time to analyse. Will update here once done.

[adelekennedy]

@abdulrahuman5196 thans for the update - do you have an eta on when you'll be able to review?

[abdulrahuman5196]

I will provide an update by tomorrow.

[adelekennedy]

@abdulrahuman5196 Should we close this? This isn't directly linked to any of our wave or vip projects at the moment and this is an annoying but ultimately small issue imo

[abdulrahuman5196]

@adelekennedy I am not sure about priority, might be small issue with little complex fix as per current proposals.

Could you kindly check internal if we should proceed on this?

[adelekennedy]

closing this one

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.