[Held requests] When partial approving report, long delay creating new report for held expense(s)

[m-natarajan]

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-2
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/p1714761240737509

Action Performed:

1. Navigate to a report containing several expenses from someone else
2. Click in the three dot menu in the expense
3. Choose Hold, leave comment
4. Confirm that the expense is on hold (e.g. Hold in the header)
5. Navigate to the report, click Approve
6. Choose Approve only…

Expected Result:

A new report with the held expense is created immediately in the front-end

Actual Result:

A new report with the held expense is created 10+ seconds later

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

partial.approving.mp4

Add any screenshot/video evidence

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @kevinksullivan

[melvin-bot]

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

[JmillsExpensify]

After speaking internally, we landed on the following solution:

• Optimistically create the report
• Use the "pending" pattern as long as the user is offline
• Set the report total/currency optimistically to the first expense on the report
• Message in subsequent expense previews to correctly reflect that the report total will be updated when the user comes online
• Update X expenses on the report preview to correctly reflect the number of expenses on the report

We already employ this solution for IOU reports but we need to update expense reports to bring them in line and do the same thing.

[JmillsExpensify]

Screenshots for clarity

Report created optimistically with multiple currencies

Report preview appropriately showing 2 expenses

[JmillsExpensify]

Based on a Slack convo, I think that @war-in might be interested in this issue?

[nkdengineer]

@JmillsExpensify This bug is the same as #39338. I think we should re-open the original issue.

[war-in]

Based on a Slack convo, I think that @war-in might be interested in this issue?

Yes, I would like to take this one

[trjExpensify]

@war-in are you good to prioritise putting up a PR for this one?

[trjExpensify]

@nkdengineer @robertjchen I'm going to assign you both to ultimately review the PR. 👍

[war-in]

Yes, however, I work part-time and will be available on Friday. Is that okay with you?

[melvin-bot]

@robertjchen, @kevinksullivan, @war-in, @nkdengineer Eep! 4 days overdue now. Issues have feelings too...

[robertjchen]

In progress/Discussions on Slack

[war-in]

Hi @robertjchen 👋
Contacting you there because I was removed from the #expensify-bugs channel 😢
My current progress looks like that

Screen.Recording.2024-05-17.at.16.33.25.mov

It's not finished yet but would be nice to get an endpoint which accepts the reportID parameter and does not create a new one. Do you have an ETA for it? Or maybe such an endpoint already exists?

[melvin-bot]

@robertjchen @kevinksullivan @war-in @nkdengineer this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[robertjchen]

Looks great! 🤩 I believe the API call involved with this flow is ApproveMoneyRequest. Could you add a new optional string parameter for an optimisticHoldReportID and pass the reportID for the optimistic report?

We will need to build out the backend portion to use it, but we can add the parameter for now! 👍

[robertjchen]

Backend Auth PR now under review!

[robertjchen]

Backend PR was deployed, @war-in let me know if you need anything else to complete the App PR, thanks! 🙇

[war-in]

Frontend draft PR is going through an internal (SWM) review. Will mark it as ready for review ASAP 🚀

[war-in]

PR marked as ready for review

[robertjchen]

PR was merged and on the way out!

[ZhenjaHorbach]

@robertjchen
@kevinksullivan
Could you please assign me to this issue since I reviewed this PR ?

[trjExpensify]

Assigned!

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[robertjchen]

^ The above is not a blocker. Explanation

[robertjchen]

We can close this out once: #42573 hits prod, and track any successor issues on the main/parent issue: https://github.com/Expensify/Expensify/issues/274076

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.13-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Create optimistic report when approving/paying for report with hold expenses #42573

If no regressions arise, payment will be issued on 2024-08-05. 🎊

For reference, here are some details about the assignees on this issue:

• @war-in does not require payment (Contractor)
• @ZhenjaHorbach requires payment (Needs manual offer from BZ)
• @nkdengineer requires payment (Needs manual offer from BZ)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ZhenjaHorbach] The PR that introduced the bug has been identified. Link to the PR:
• [@ZhenjaHorbach] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@ZhenjaHorbach] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@ZhenjaHorbach] Determine if we should create a regression test for this bug.
• [@ZhenjaHorbach] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@kevinksullivan] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: