[HOLD on #378314][$500] [Held requests] Request on hold becomes unheld when approving only unheld request

[lanitochka17]

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.58-4
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Issue found when executing PR #33124

Action Performed:

Precondition:

• Admin and employee are in the Collect workspace

1. [Employee] Request money from the workspace chat
2. [Employee] Create two money requests
3. [Employee] Hold one of the requests and leave the other one unheld
4. [Admin] Go to expense report
5. [Admin] Click Approve button
6. [Admin] Approve the unheld request

Expected Result:

The request on hold will be moved to a new report

Actual Result:

Request on hold becomes unheld when approving only unheld request

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

Bug6433343_1711893426393.20240331_215154.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d6a3e193d64c10a4
• Upwork Job ID: 1775018433509507072
• Last Price Increase: 2024-04-09

[melvin-bot]

Triggered auto assignment to @NikkiWines (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[lanitochka17]

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

[lanitochka17]

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

[NikkiWines]

This definitely looks like a regression / unhandled case from #33124. I've been trying to reproduce locally but the behavior is pretty buggy on main right now (crashing when holding an expense, approving the full total and not the held amount, paying doesn't work because the "The requested amount has changed") so it's hard to see what's causing this particular staging behavior

Pinged @robertjchen since he's got context on the original PR and might be able to lend some insight here

[NikkiWines]

Looks like - at least on dev - when approving the report, it's the optimisticIOUReportData from here that is overwriting the amount with the full (held + non-held) amount, even though the amount is correctly being set in optimisticApprovedReportAction here.

[robertjchen]

Thanks for the report! Definitely related to the changes there: #33124 and we'll be addressing it. Not a deploy blocker per the sentiments here

[NikkiWines]

Sounds good, marking this as an external bug then so it can get worked on 👍

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[nkdengineer]

Proposal

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

Request on hold becomes unheld when approving only unheld request

What is the root cause of that problem?

When we approve only unheld request, we don't create new expense report and report preview for the hold request. And BE also doesn't implement this like the expected here

The request on hold will be moved to a new report

App/src/libs/actions/IOU.ts

Line 4623 in a4d01a8

function approveMoneyRequest(expenseReport: OnyxTypes.Report | EmptyObject, full?: boolean) {

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

Add a new variable like shouldCreateNewPreview and update it to true in this case.

App/src/libs/actions/IOU.ts

Lines 4626 to 4627 in a4d01a8

	if (ReportUtils.hasHeldExpenses(expenseReport.reportID) && !full && !!expenseReport.unheldTotal) {
	total = expenseReport.unheldTotal;

If it's true, we will move all hold request to new report. The main steps:

1. Create an optimistic expense report
2. Move all IOU actions hold transactions from old expense report to new expense report
3. Update the total of new expense report as the expenseReport.total - expenseReport.unheldTotal
4. Update the total of the old expense report as expenseReport.unheldTotal
5. Create an optimistic report preview for new expense report, the child request count is the number of hold requests, other fields can be updated accordingly base on hold requests
6. Update the old report preview base on upheld requests

And we also need to implement this in back-end as well.

What alternative solutions did you explore? (Optional)

NA

[melvin-bot]

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

[NikkiWines]

@alitoshmatov please review the proposals when you have a moment 🙇

[trjExpensify]

Hm, interesting! But @marcochavezf only has the currency issue remaining, no? I guess I don't know where this is being fixed, or if his earlier PRs shoul dhave.

[melvin-bot]

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

[trjExpensify]

Same, Melv!

[melvin-bot]

@trjExpensify, @NikkiWines, @alitoshmatov Whoops! This issue is 2 days overdue. Let's get this updated quick!

[trjExpensify]

@marcochavezf can you confirm if this issue is resolved with the PRs that have come for #378314?

[marcochavezf]

Yup, this issue was fixed in this PR

[trjExpensify]

Cool, once deployed we close this out then. :)

[melvin-bot]

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

[NikkiWines]

Not overdue; waiting on #40065, which is currently on staging.

[trjExpensify]

Hit prod two days ago, closing!

[nkdengineer]

@NikkiWines @trjExpensify We have this bug #41652 here this is the part of this issue since we don't implement the optimistic data when we approve partial. Can you please take a look and re-open this issue.

[NikkiWines]

@NikkiWines @trjExpensify We have this bug #41652 here this is the part of this issue since we don't implement the optimistic data when we approve partial. Can you please take a look and re-open this issue.

Been focused on some other urgent issues but I will look into this during the week. From a quick glance it looks like there's some activity in the linked issue for resolving the issue as well. Re-opening for now so it doesn't fall off my radar.

[trjExpensify]

I think we can use the other issue for this follow-up. @nkdengineer I've assigned you for the C+ review on that.

[NikkiWines]

Sounds good, re-closing this then