[$250] Expense - Pay with Expensify button appears for Employee after admin disconnects bank account

[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: 9.0.16-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: N/A
Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

• Delayed submission is enabled in Workflows
• Workspace has an approver (admin)
• Workspace has set up with bank account
• Employee does not have bank account

1. Go to staging.new.expensify.com
2. [Employee] Go to workspace chat
3. [Employee] Submit an expense over $100
4. [Employee] Click on the submit button and stay on the main chat
5. [Admin] Click Approve button on the expense preview
6. [Admin] Go to workspace settings > Workflows
7. [Admin] Click on the bank account
8. [Admin] Disconnect bank account
9. [Employee] Note that "Pay with Expensify" button shows up on expense preview on employee end after admin has disconnected bank account

Expected Result:

"Pay with Expensify" button will not appear on expense preview on employee end after admin has disconnected bank account

Actual Result:

"Pay with Expensify" button shows up on expense preview on employee end after admin has disconnected bank account.
When clicking on the expense preview and returning to main chat, the button disappears

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

Bug6560213_1722605179858.20240802_211830.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e26750961886bca4
• Upwork Job ID: 1821153286159845456
• Last Price Increase: 2024-08-28
• Automatic offers:
• jjcoffee | Reviewer | 103796028

Issue Owner

Current Issue Owner: @teneeto

[melvin-bot]

Triggered auto assignment to @miljakljajic (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

@miljakljajic FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #wave-collect - Release 1

[melvin-bot]

@miljakljajic Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01e26750961886bca4

[melvin-bot]

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

[melvin-bot]

@jjcoffee, @miljakljajic Huh... This is 4 days overdue. Who can take care of this?

[jjcoffee]

Waiting for proposals!

[melvin-bot]

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

[jjcoffee]

Bumped on Slack to hopefully get some proposals in!

[melvin-bot]

@jjcoffee @miljakljajic this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[jjcoffee]

@miljakljajic Maybe we should try bumping the price to get more eyes on this?

[melvin-bot]

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

[jjcoffee]

This isn't getting any attention, let's bump the price @miljakljajic

[melvin-bot]

@jjcoffee, @miljakljajic Huh... This is 4 days overdue. Who can take care of this?

[miljakljajic]

reconfirming the expected outcome here - I'm not clear on why the employee would see the Pay with Expensify option

[teneeto]

I managed to track the issue all the way down to this condition here: https://github.com/teneeto/Expensify/blob/dcf1a0cac762ef658ebfa8f692331f04828d6716/src/libs/ReportUtils.ts#L1640-L1649

Each time a workspace bank account is removed, !policy?.achAccount?.reimburser is opimistically updated which sets that condition to true and then causing the settlement button to show. This shouldn't even be a problem. The main issue there is, the conditions there don't fall through as it should at least for this operation.

I'm still looking at how best this could be fixed - which i don't expect, should take any longer before i raise a PR.

[melvin-bot]

@twisterdotcom, @teneeto, @jjcoffee Whoops! This issue is 2 days overdue. Let's get this updated quick!

[teneeto]

@twisterdotcom @jjcoffee a PR is raised here: #49969

Here are my thoughts on my solution and possible alternatives (if needed).

Problem

On these lines:
https://github.com/teneeto/Expensify/blob/dcf1a0cac762ef658ebfa8f692331f04828d6716/src/libs/ReportUtils.ts#L1641 and https://github.com/teneeto/Expensify/blob/dcf1a0cac762ef658ebfa8f692331f04828d6716/src/libs/ReportUtils.ts#L1645, we are doing condition to check for payer and depending on whether or not a bank account is connected, those conditions are triggered. The issue is when a bank account is disconnected, the second condition is not immediately triggered because policy?.reimbursementChoice is not immediately or optimistically updated when the bank account is disconnected - except you force a page reload or the endpoint is re-triggered.

Soution

I found another field policy?.reimbursement?.choice?.reimbursementChoice which updates as expected. policy?.reimbursementChoice is now used as a fallback.
const reimbursementChoice = policy?.reimbursement?.choice?.reimbursementChoice ?? policy?.reimbursementChoice;

alternate solution

We can find out why policy?.reimbursementChoice doesn't optimistically update, fix it and then use it again.

Screenshot

The video below shows a log of policy objects and the variation in the reimbursement choice fields after the bank account is disconnected.

---

Screen.Recording.2024-10-01.at.08.20.42.mov

[jjcoffee]

@teneeto Thanks for the investigation and for the detailed write-up! I just need to clarify a few points before proceeding.

when a bank account is disconnected, the second condition is not immediately triggered

Are you saying that once the bank account is disconnected, the reimbursementChoice should be CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_MANUAL?

I think the main confusion for me here is that the employee shouldn't be seeing the pay button at all as they are not the "reimburser" (in either condition). So it seems like once the bank account is disconnected, the employee is treated as the reimburser for some reason.

App/src/libs/ReportUtils.ts

Lines 1641 to 1647 in dcf1a0c

	if (policy?.reimbursementChoice === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES) {
	const isReimburser = session?.email === policy?.achAccount?.reimburser;
	return (!policy?.achAccount?.reimburser || isReimburser) && (isApproved || isManager);
	}
	if (policy?.reimbursementChoice === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_MANUAL) {
	return isAdmin && (isApproved || isManager);
	}

[teneeto]

Yes, reimbursementChoice should be CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_MANUAL. For the second point, I can't say I'm 100% sure of what the flow should be but yes, that contributed to the issue as well, I also stated it here: #46738 (comment). Either way, it involves the values being updated from BE.

Overall, the conditions with reimbursementChoice are the parent issue. If that is solved properly then it handles the confusing part as well.

[jjcoffee]

That makes sense, thanks! I guess policy?.achAccount?.reimburser is getting removed along with the bank account which is another part of the issue.

I'm a bit unsure on why there are two fields for reimbursementChoice that you mention in your solution, so I'm gonna get an engineer assigned to make sure it's okay to switch to.

🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @srikarparsi, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[srikarparsi]

The RCA here makes sense to me. I do think we should go with the alternate solution listed though since we already store reimbursementChoice and don't think we need a new variable for that data.

Also, I don't think we should ever show the pay button to regular employees (not admins, managers...). @rlinoz, do you know why we show the settlement button in this line if reimburser is empty (and the report is approved)?

[rlinoz]

I think it comes from here actually #38253, but I am not sure what we were fixing, maybe @luacmartins remembers

[luacmartins]

Form the PR above, that covers this case when reimbursements are enabled but there's no VBBA account, hence no achAccount data in the policy

Reimbursement enabled, indirect (no VBBA) -> show Pay elsewhere and Pay with Expensify

[teneeto]

How do we proceed with this, are we looking at a different route?

[jjcoffee]

Sounds like this maybe needs to be internal if we plan to fix policy?.reimbursementChoice as @srikarparsi suggests?

[teneeto]

I'm still keeping up with it. Ping me if you need me to make any changes regarding this issue.

[teneeto]

@twisterdotcom @jjcoffee @srikarparsi do we proceed with this as a BE issue and internal? what should we do with this PR. Just curious to know how we can proceed with this issue based on our previous discussions

[teneeto]

Ping! Ping!! @twisterdotcom @jjcoffee @srikarparsi, I'm just knocking to check the next steps on this issue and the PR that has already been raised. Do you have any ideas or suggestions on what I can do next?

[twisterdotcom]

Ping ping indeed. Really need @srikarparsi to chime in, but it does sound like we need this to be Internal.

[srikarparsi]

Hey, yes this does look backend but it is a little intersting. Sorry for the delay in responding.

In the backend, whenever we set reimbursementChoice, we nest it under policy.reimbursement.choice.

But in the frontend, we directly access it as policy.reimbursementChoice.

This is because we send the onyx update directly under policy instead of under policy.reimbursement.choice. But, it looks like we missed doing that in one place which is why policy.reimbursement.choice.reimbursementChoice is updating but policy.reimbursementChoice is not.

So two approaches:

1. Change the frontend: I do think it's cleaner to keep things 1:1 with the db and it might be better to access the reimbursementChoice in the frontend as policy.reimbursement.choice.reimbursementChoice since that's what we do in the backend. But I'm not sure if it's worth refactoring all this code even if we want to do it. cc @puneetlath for thoughts since you worked on SetPolicyReimbursement recently.
2. Queue the right Onyx update in the backend: Somewhere, we're queuing an update to policy.reimbursement.choice.reimbursementChoice instead of policy.reimbursement.choice. If we change this, this should fix the bug as @teneeto pointed out.

I'm going to make this internal since I think 2 is the better approach for now.

[puneetlath]

I do think generally that it's better to keep the front-end and back-end aligned if we can. It just makes things simpler and less confusing. Is it a big refactor to do so?

[srikarparsi]

Cool, yeah. It's not crazy big, there's 51 occurrences in App of reimbursementChoice. I did not count how many instances we queue onyx updates to reimbursementChoice in the backend but it doesn't look like it's too too many. I agree that it'll probably be better for the long run but just wasn't sure in terms of priorities right now. But if you think now would be a good time to refactor it, I can start working on that.

[puneetlath]

I think it's as good a time as any. Thanks so much for handling!