[$500] Workspace -Incorrect currency message flashes for second in the workspace where the Bank account is added

[m-natarajan]

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.40-0
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:

Precondition:
Fully verified bank account has been successfully added
Steps:

1. Navigate to Workspace Settings in which fully verified bank account is added
2. Click on Bank Account
3. click on Chats at the botton
4. Click on settings

Expected Result:

"The default currency can't be changed because this Workspace is linked to USD bank account" message should be present under Currency section

Actual Result:

"All expenses on this workspace will be converted to this currency" flashes for a moment when Bank account is added

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

Bug6377233_1707771199654.Recording__2240.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~015575d1ff4376d09e
• Upwork Job ID: 1757148556204711936
• Last Price Increase: 2024-02-12
• Automatic offers:
• fedirjh | Reviewer | 0
• bernhardoj | Contributor | 0

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~015575d1ff4376d09e

[melvin-bot]

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

[melvin-bot]

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

[allgandalf]

Proposal

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

All expenses will be converted to this currency appear when Bank account is added

What is the root cause of that problem?

We currently check if the currency page has VBA:

App/src/pages/workspace/WorkspaceProfilePage.js

Line 131 in 5f483a4

{hasVBA ? translate('workspace.editor.currencyInputDisabledText') : translate('workspace.editor.currencyInputHelpText')}

This check is not performed on initial page load but is done when we change between chat and settings

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

Update the code to perform this check on the first load itself

We need to define the following check:

const achState = lodashGet(reimbursementAccount, 'achData.state', '');
    const hasVBA = achState === BankAccount.STATE.OPEN;

What alternative solutions did you explore? (Optional)

N/A

[Christinadobrzyn]

Hi @m-natarajan so sorry, I don't understand the reproduction steps in the OP.

Is the issue that the currency messaging on the workspace Bank Account page doesn't match the Profile page? Asking QA for some guidance - https://expensify.slack.com/archives/C9YU7BX5M/p1707775399734399

2024-02-12_14-59-21.mp4

[bernhardoj]

Should have the same root cause as #34919

[Christinadobrzyn]

oh nice -thanks @bernhardoj! Reached out in #34919 to see if we can consolidate or if we should hold this until #34919 is fixed.

[Christinadobrzyn]

Okay, we're going to put this on hold for #34919 (comment)

[Christinadobrzyn]

hold for #34919 (comment)

[Christinadobrzyn]

hold for #34919 (comment)

[Christinadobrzyn]

Hold for - #34919 (comment)

[Christinadobrzyn]

hold for - #34919

[melvin-bot]

📣 @fedirjh 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[bernhardoj]

PR is ready

cc: @fedirjh

[nkdengineer]

@yuwenmemon There's discussion ongoing on a regression by the selected proposal, could we pause assignment here for a moment while we sort it out? Thanks!

[nkdengineer]

It would be helpful if you could explain how my solution leads to the regression.

@bernhardoj It happens after applying your change so I believe it is.

Do you agree it only starts happening after your change?

[bernhardoj]

I can't follow your repro steps as I can't seem to add a bank account from the new dot, so I add it from the old dot. You are saying that to solve the regression from my proposal, we should clear the plaid when unmounting.

BankAccounts.clearPlaid();

So, please explain which part of my proposal caused the regression that requires us to apply the code above instead of just saying that my proposal caused the regression.

[bernhardoj]

I think I finally understand the issue you mean. It's specifically this change.

When the plaid event is not cleared, isLoading variable is false. But we have another condition to show the loading, that is if hasACHDataBeenLoaded is false.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Lines 436 to 438 in f150229

	if ((!hasACHDataBeenLoaded || isLoading) && shouldShowOfflineLoader && (shouldReopenOnfido || !requestorStepRef.current)) {
	return <ReimbursementAccountLoadingIndicator onBackButtonPress={goBack} />;
	}

On main, hasACHDataBeenLoaded value is always false initially because of the bug as explained in my proposal, but then we fix it because the data is indeed loaded when we already have a connected bank account.

With or without my solution, the plaid event is never cleared. So, yeah, it's still an existing issue on the main where we don't clear the plaid event.

[fedirjh]

Isn't the Plaid data cleaned when the request is successfully executed ?

[bernhardoj]

@fedirjh Oh, I didn't know OpenReimbursementAccountPage clears the plaid data, but it's ONYXKEYS.PLAID_CURRENT_EVENT specifically that still exists.

[nkdengineer]

@fedirjh Oh, I didn't know OpenReimbursementAccountPage clears the plaid data, but it's ONYXKEYS.PLAID_CURRENT_EVENT specifically that still exists.

Yes @fedirjh, ONYXKEYS.PLAID_CURRENT_EVENT isn't cleared and it causes the issue I mentioned, the BankAccounts.clearPlaid(); part of my proposal was meant to fix it.

On main, hasACHDataBeenLoaded value is always false initially because of the bug as explained in my proposal, but then we fix it because the data is indeed loaded when we already have a connected bank account.

@bernhardoj Your proposal reveals the issue (still a regression I think, although it doesn't introduce the root cause), so I still think we need to fix it here. The UX issue wasn't reproducible in main. Otherwise it might later be flagged as a regression.

@fedirjh What do you think? (I'd be happy to collaborate with @bernhardoj if we decide to fix this)

[bernhardoj]

My proposal reveals the issue specifically to your steps, but the loading issue already happens even without my change.

You can see here even on main, the loading page never shows when switching between steps (1 -> 2 -> 3) after connecting to plaid.

Screen.Recording.2024-04-09.at.22.43.15.mov

Screen.Recording.2024-04-09.at.22.51.43.mov

Compared to when the plaid event is empty.

Screen.Recording.2024-04-09.at.22.45.11.mov

If we really want to clear the plaid event, then I would prefer to clear it when mounted just like I did here since unmount logic won't be called if you close the app. Even better is setting initWithStoredValues: false to the plaid event onyx. But then we may need to clear it too once we connected to plaid and maybe any other uncovered cases (or even potentially remove checking the plaid event to decide the loading state since idk why we do that). So, I suggest handling it on a separate issue to carefully fix the loading issue.

[melvin-bot]

This issue has not been updated in over 15 days. @yuwenmemon, @fedirjh, @bernhardoj, @Christinadobrzyn eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[Christinadobrzyn]

Hi @bernhardoj @fedirjh - can you provide an update on this?

[bernhardoj]

@Christinadobrzyn hi, the PR is already deployed to prod a week ago

[Christinadobrzyn]

yikes! let's pay this out! I need to step away for an appt but I'll write up a payment summary so we can pay this out.

[Christinadobrzyn]

Payouts due:

• Contributor: $500 @bernhardoj (paid in Upwork - https://www.upwork.com/nx/wm/offer/101753880)
• Contributor+: $500 @fedirjh (paid in Upwork - https://www.upwork.com/nx/wm/offer/101753879)

Upwork job is here.

@bernhardoj or @fedirjh do we need a regression test?

[Christinadobrzyn]

nudge @bernhardoj or @fedirjh do we need a regression test?

[bernhardoj]

I think it's good to have a regression test. We can take the test step from the PR

Prerequisite: have a connected workspace bank account

1. Open the workspace that has the connected bank account
2. Open the workspace profile page
3. Verify the message below Default currency is "The default currency can't be changed..."
4. Open the bank account page
5. Verify you see a loading animation and there is no blinking
6. Close the bank account page
7. Open the workspace profile page again
8. Verify the message below Default currency is still the same as in step 3

[Christinadobrzyn]

Regression test - https://github.com/Expensify/Expensify/issues/394581

Closing this out, let me know if I'm missing anything!