Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.58k stars 2.92k forks source link

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

Open lanitochka17 opened 3 months ago

lanitochka17 commented 3 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com 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:

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?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/ee48cf85-86b5-410f-a14d-a12c851cc1b4

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 OwnerCurrent Issue Owner: @teneeto
melvin-bot[bot] commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

jjcoffee commented 3 months ago

Waiting for proposals!

melvin-bot[bot] commented 3 months ago

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

jjcoffee commented 3 months ago

Bumped on Slack to hopefully get some proposals in!

melvin-bot[bot] commented 3 months ago

@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 commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

jjcoffee commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

miljakljajic commented 3 months ago

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

jjcoffee commented 3 months ago

@miljakljajic I think the issue is that Pay with Expensify should not appear for the employee at all, but it does appear once the admin disconnects the bank account. @lanitochka17 can you confirm?

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 2 months ago

@jjcoffee @miljakljajic this issue is now 4 weeks old, please consider:

Thanks!

melvin-bot[bot] commented 2 months ago

@jjcoffee, @miljakljajic Eep! 4 days overdue now. Issues have feelings too...

jjcoffee commented 2 months ago

@miljakljajic Should we bump the price to get more eyes on this?

teneeto commented 2 months ago

Hey! I'm Eto Olei from Callstack. I will like to look into this issue. Please assign it to me. Thanks!

miljakljajic commented 2 months ago

Thank you @teneeto! I will

melvin-bot[bot] commented 2 months ago

📣 @jjcoffee 🎉 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

teneeto commented 2 months ago

Hi @miljakljajic, FYI I have an unexpected review that needs my attention. Will come back to this in no time.

melvin-bot[bot] commented 2 months ago

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

jjcoffee commented 2 months ago

Not overdue - just waiting for @teneeto to pick this back up.

teneeto commented 2 months ago

Hi @miljakljajic, I'm back to looking into this.

miljakljajic commented 2 months ago

Thank you!

melvin-bot[bot] commented 2 months ago

@teneeto, @jjcoffee, @miljakljajic Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

jjcoffee commented 2 months ago

Just waiting for @teneeto! :pray:

teneeto commented 2 months ago

Hey hey! I'm still investigating and have to dig in a little.

melvin-bot[bot] commented 2 months ago

@teneeto, @jjcoffee, @miljakljajic Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

teneeto commented 2 months ago

Hya, I' still on this and just a tiny bit more to check and I'll post findings.

jjcoffee commented 2 months ago

Sweet, thanks for the update!

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @twisterdotcom (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.

miljakljajic commented 2 months ago

Assigning this over to another BZ member as I'm heading out on parental leave.

@twisterdotcom - we are waiting for an update from @teneeto at Call stack before moving forward. Thank you!

teneeto commented 2 months ago

O yea, still checking and will share updates by EOD.

melvin-bot[bot] commented 2 months ago

@twisterdotcom, @teneeto, @jjcoffee Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

twisterdotcom commented 2 months ago

Any updates @teneeto?

teneeto commented 2 months ago

@twisterdotcom, I suspect this to be BE - all checks on FE passes so far. I'm just looking at one more thing. I'm not done. Once I can be sure I can share my thoughts, My apologies for not posting updates as promised earlier. Thanks.

teneeto commented 2 months ago

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[bot] commented 1 month ago

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

teneeto commented 1 month ago

@twisterdotcom @jjcoffee a PR is raised here: https://github.com/Expensify/App/pull/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.


https://github.com/user-attachments/assets/b05a9257-143d-4ebd-bb4c-149be7526b78

jjcoffee commented 1 month ago

@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.

https://github.com/Expensify/App/blob/dcf1a0cac762ef658ebfa8f692331f04828d6716/src/libs/ReportUtils.ts#L1641-L1647

teneeto commented 1 month ago

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: https://github.com/Expensify/App/issues/46738#issuecomment-2378746230. 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 commented 1 month ago

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.

:ribbon::eyes::ribbon: C+ reviewed

melvin-bot[bot] commented 1 month ago

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

srikarparsi commented 1 month ago

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 commented 1 month ago

I think it comes from here actually https://github.com/Expensify/App/pull/38253, but I am not sure what we were fixing, maybe @luacmartins remembers