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
2.99k stars 2.5k forks source link

[$250] [Held requests] Incorrectly showing the held confirmation modal for one expense report #41613

Open m-natarajan opened 2 weeks ago

m-natarajan commented 2 weeks 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: 1.4.70-1 Reproducible in staging?: Yes Reproducible in production?: Yes 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: @JmillsExpensify Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1714590485812579

Action Performed:

Prerequisite: Create a Collect workspace as user A, invite user B and configure to submit to user A

  1. As user B create an expense
  2. As user A, hold user B’s expense
  3. Approve the report

Expected Result:

Hold confirmation modal should not be displayed in the case of only one expense

Actual Result:

Confirmation modal displayed

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

CleanShot 2024-05-01 at 13 04 44@2x Screen Shot 2024-05-03 at 3 32 12 PM (2)

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0147c167b8b6e9da09
  • Upwork Job ID: 1787747404592803840
  • Last Price Increase: 2024-05-14
Issue OwnerCurrent Issue Owner: @JmillsExpensify
melvin-bot[bot] commented 2 weeks ago

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

bernhardoj commented 2 weeks ago

Proposal

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

The approve confirmation shows even all the request is on hold.

What is the root cause of that problem?

I don't know how to hold a single transaction, but I can reproduce it when hold all transactions in an expense report. I guess the expectation is that the confirmation modal is not needed when all transactions are on hold.

When we press the approve button, it checks whether the report has at least one on hold transaction. https://github.com/Expensify/App/blob/2f5173c9c0f041967acfbdf9581ce52327623fa6/src/components/MoneyReportHeader.tsx#L138-L145

So, when we have all transactions on hold, the condition is still true and the confirmation modal is shown instead of immediately approving it.

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

We can update the condition to

if (ReportUtils.hasHeldExpenses(moneyRequestReport.reportID) && !ReportUtils.hasOnlyHeldExpenses(moneyRequestReport.reportID)) {

So, it will only show the confirmation modal if the report contains an on hold transaction and a non hold transaction.

We can do the same for pay here https://github.com/Expensify/App/blob/2f5173c9c0f041967acfbdf9581ce52327623fa6/src/components/MoneyReportHeader.tsx#L125-L136

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

Job added to Upwork: https://www.upwork.com/jobs/~0147c167b8b6e9da09

melvin-bot[bot] commented 1 week ago

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

trjExpensify commented 1 week ago

@JmillsExpensify do we need to consider the expected results here? Should it have different modal copy to confirm you're okay with taking the expense off-hold and approving it?

melvin-bot[bot] commented 1 week ago

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

laurenreidexpensify commented 5 days ago

@mananjadhav bump for review thanks

mananjadhav commented 5 days ago

@JmillsExpensify do we need to consider the expected results here? Should it have different modal copy to confirm you're okay with taking the expense off-hold and approving it?

I was waiting for the confirmation on this comment. @trjExpensify @JmillsExpensify any updates on this one?

trjExpensify commented 5 days ago

I've asked Jason to take a look.

melvin-bot[bot] commented 4 days ago

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

laurenreidexpensify commented 2 days ago

@JmillsExpensify adding you to take a look, pls unassign yourself when you've had a chance to look thanks

JmillsExpensify commented 2 days ago

I don't really feel like there should be an extra confirmation modal. There is a hold pattern in the header, right? If there isn't I think that's another bug.

Generally though, I don't think another modal is the solution here. If you see something is on hold, including via the comments, we shouldn't ask you yet again if you're sure.

trjExpensify commented 2 days ago

By "hold pattern" do you mean the banner copy yeah? If so, yeah, that should be there though you can't hold a one expense report right now until that gets fixed.

Even still, I think it would be consistent that in all cases of either one or 20 expenses on the report, if something is held on the report and you go to take an action to approve/pay you would get a confirmation modal. Kinda' even more so in the one-expense report case, as it's an unusual action to take to approve or pay an expense that is held.

Not super passionate, but I think we're giving people a lot of credit with the below sentiment. #nobodyreads, haha.

If you see something is on hold, including via the comments, we shouldn't ask you yet again if you're sure.

laurenreidexpensify commented 1 day ago

@Expensify/design any thoughts?

shawnborton commented 1 day ago

Hmm I don't feel too strongly, happy to leave the decision up to the other designers + Jason/Tom :)

dannymcclain commented 1 day ago

Even still, I think it would be consistent that in all cases of either one or 20 expenses on the report, if something is held on the report and you go to take an action to approve/pay you would get a confirmation modal. Kinda' even more so in the one-expense report case, as it's an unusual action to take to approve or pay an expense that is held.

I think I'm with Tom on this one. And I tend to agree with this:

Not super passionate, but I think we're giving people a lot of credit with the below sentiment. #nobodyreads, haha.

If you see something is on hold, including via the comments, we shouldn't ask you yet again if you're sure.

So I think I'm in favor of leaving the confirmation modal in all cases.

melvin-bot[bot] commented 1 day ago

@JmillsExpensify @mananjadhav @laurenreidexpensify 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!