[HOLD for payment 2024-08-14] [$250] Hold - Missing "Unhold" on the report header of held expense after it is moved to new report offline

[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: 9.0.13-3
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

Issue found when executing PR #42573

Action Performed:

Precondition:

• Workspace has an approver (can be workspace owner)

1. Go to staging.new.expensify.com
2. Go to workspace chat
3. Submit three expenses to the workspace chat
4. Hold two of the expenses
5. Click Submit button
6. Go offline
7. Click Approve
8. Approve partially (click the green approve button)
9. Go back to the main workspace chat
10. Click on the new report generated which contains the held expenses
11. Click on any of the held expense preview to go to transaction thread
12. Click on the report header

Expected Result:

Unhold option should be present on the report details of the held expense after it is moved to a new report offline

Actual Result:

hold option is missing on the report details of the held expense after it is moved to a new report offline

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

Bug6554728_1722089982378.bandicam_2024-07-27_22-12-39-358.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0154caef32a9b3ac64
• Upwork Job ID: 1817961052655862549
• Last Price Increase: 2024-07-29
• Automatic offers:
• ikevin127 | Reviewer | 103351156
• cretadn22 | Contributor | 103351157

Issue Owner

Current Issue Owner: @alexpensify

[melvin-bot]

Triggered auto assignment to @alexpensify (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]

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

[cretadn22]

Proposal

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

Missing "Unhold" on the report header of held expense after it is moved to new report offline

What is the root cause of that problem?

App/src/pages/home/report/ContextMenu/ContextMenuActions.tsx

Lines 266 to 267 in 08bb00b

	shouldShow: (type, reportAction) =>
	type === CONST.CONTEXT_MENU_TYPES.REPORT_ACTION && ReportUtils.canEditReportAction(reportAction) && ReportUtils.canHoldUnholdReportAction(reportAction).canUnholdRequest,

Because canUnholdRequest is false, the unHold option doesn't display

More detail in ReportUtils.canHoldUnholdReportAction function
moneyRequestReport.parentReportActionID is undefined -> parentReportAction is undefined -> isDeletedParentAction is true -> canHoldOrUnholdRequest is false -> canUnholdRequest is false

The underlying issue is that when we approve the request, we don't to include the parentReportActionID in the new iouReport.

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

App/src/libs/actions/IOU.ts

Lines 6246 to 6254 in 08bb00b

	const optimisticExpenseReport = ReportUtils.buildOptimisticExpenseReport(
	chatReport.reportID,
	chatReport.policyID ?? iouReport.policyID ?? '',
	recipient.accountID ?? 1,
	(firstHoldTransaction?.amount ?? 0) * -1,
	getCurrency(firstHoldTransaction),
	false,
	);
	const optimisticExpenseReportPreview = ReportUtils.buildOptimisticReportPreview(chatReport, optimisticExpenseReport, '', firstHoldTransaction, optimisticExpenseReport.reportID);

optimisticExpenseReport is new iouReport
optimisticExpenseReportPreview is new reportAction in chatReport

Thus, optimisticExpenseReport.parentReportActionID should equal optimisticExpenseReportPreview.reportActionID

+const newParentReportActionID = rand64();
const optimisticExpenseReport = ReportUtils.buildOptimisticExpenseReport(
        chatReport.reportID,
        chatReport.policyID ?? iouReport.policyID ?? '',
        recipient.accountID ?? 1,
        (firstHoldTransaction?.amount ?? 0) * -1,
        getCurrency(firstHoldTransaction),
        false,
+      newParentReportActionID
    );
+const optimisticExpenseReportPreview = ReportUtils.buildOptimisticReportPreview(chatReport, optimisticExpenseReport, '', firstHoldTransaction, optimisticExpenseReport.reportID, newParentReportActionID);

In the buildOptimisticExpenseReport and buildOptimisticReportPreview functions, we need to update them to incorporate the new parameter.

An alternative approach is to use optimisticExpenseReport.parentReportActionID directly in buildOptimisticReportPreview instead of adding a new parameter. However, I prefer the previous approach because it is safer.

---

Another Bug

While addressing the original bug, I discovered an additional issue. We can observe this new bug in the bug video at timestamp 0:52.

The total amount in the report preview (and in iouReport) isn’t updated when a partial approval occurs

Causes and solutions

We’re not updating the total amount in the optimistic data. To resolve this issue, we need to adjust the total amount for iouReport and reportPreview using the unheldTotal field, similar to what we did previously

App/src/libs/actions/IOU.ts

Lines 6715 to 6719 in 08bb00b

	let total = expenseReport?.total ?? 0;
	const hasHeldExpenses = ReportUtils.hasHeldExpenses(expenseReport?.reportID);
	if (hasHeldExpenses && !full && !!expenseReport?.unheldTotal) {
	total = expenseReport?.unheldTotal;
	}

[nkdengineer]

Proposal

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

hold option is missing on the report details of the held expense after it is moved to a new report offline

What is the root cause of that problem?

The held requests are moved to a new expense report and a new report preview is also created. Still, we haven't updated the parentReportActionID of optimisticExpenseReport to the new report preview action.

App/src/libs/actions/IOU.ts

Line 6246 in 98d8a5a

const optimisticExpenseReport = ReportUtils.buildOptimisticExpenseReport(

App/src/libs/actions/IOU.ts

Line 6254 in 98d8a5a

const optimisticExpenseReportPreview = ReportUtils.buildOptimisticReportPreview(chatReport, optimisticExpenseReport, '', firstHoldTransaction, optimisticExpenseReport.reportID);

Then parentReportAction is undefined here and the hold or unhold option doesn't appear

App/src/pages/ReportDetailsPage.tsx

Line 122 in 98d8a5a

const parentReportAction = ReportActionsUtils.getReportAction(report?.parentReportID ?? '', report?.parentReportActionID ?? '');

Another bug here is we updated the total of the new expense report as the amount of the first hold transaction and it's wrong. I think maybe the reason is each hold transaction can have a different currency.

App/src/libs/actions/IOU.ts

Line 6250 in 98d8a5a

(firstHoldTransaction?.amount ?? 0) * -1,

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

1. After creating the optimistic expense report and report preview action, we should update the parentReportActionID of the expense report to the reportActionID of the new report preview action

optimisticExpenseReport.parentReportActionID = optimisticExpenseReportPreview.reportActionID;

App/src/libs/actions/IOU.ts

Line 6254 in 98d8a5a

const optimisticExpenseReportPreview = ReportUtils.buildOptimisticReportPreview(chatReport, optimisticExpenseReport, '', firstHoldTransaction, optimisticExpenseReport.reportID);

2. We should update the total of the new expense report as the total amount of all hold transactions instead of first transaction if all transactions have the same currency.

What alternative solutions did you explore? (Optional)

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0154caef32a9b3ac64

[melvin-bot]

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

[robertjchen]

We should update the total of the new expense report as the total amount of all hold transactions instead of first transaction if all transactions have the same currency.

There was some discussion on the report amounts here and why some of the behavior is intentional 🤔

[robertjchen]

Also, a PR is in progress around context menus- hopefully there's no overlap there as well?

I'm seeing if the fix here may also address this? #41655

[ikevin127]

♻️ Will review proposals today, check on possible overlap issues and see if it solves the other mentioned issue.

[ikevin127]

There was some discussion on the report amounts #46310 (comment) and why some of the behavior is intentional 🤔

Indeed, based on that comment I would not go out of scope in this issue - keep it contained to what this issue is about strictly.

Also, a PR is in progress around #45151 hopefully there's no overlap there as well?

I'm seeing if the fix here may also address this? #41655

Regarding #45151, took a quick-look and I don't think there are any conflicts between the work done there and this issue. @cdOut should be able to confirm whether or not my assumption is right.

Regarding #41655 - I don't think there's any overlap here.

@cretadn22's proposal LGTM. The RCA was identified and the solution solves the issue.

We should only fix the OP issue via the main solution. Keep the other found bug separate as it's not within the scope of this issue.

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @robertjchen is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[ikevin127]

@nkdengineer In the future, please be mindful of the contributor guidelines as repeated failure in following the rules can lead to warnings and even removal from the contributor program.

Note: Before submitting a proposal on an issue, be sure to read any other existing proposals. ALL NEW PROPOSALS MUST BE DIFFERENT FROM EXISTING PROPOSALS. The difference should be important, meaningful or considerable.

[nkdengineer]

@ikevin127 Thanks for your feedback. I think my proposal is different from the selected proposal. The main ideal is the same but the implementation is different

Here is the alternative implementation from the selected proposal. It mentioned to use optimisticExpenseReport.parentReportActionID directly in buildOptimisticReportPreview

An alternative approach is to use optimisticExpenseReport.parentReportActionID directly in buildOptimisticReportPreview instead of adding a new parameter. However, I prefer the previous approach because it is safer.

And here is my implementation

After creating the optimistic expense report and report preview action, we should update the parentReportActionID of the expense report to the reportActionID of the new report preview action

That is why I posted a new proposal.

[cdOut]

Regarding #45151, took a quick-look and I don't think there are any conflicts between the work done there and this issue. @cdOut should be able to confirm whether or not my assumption is right.

Just wanted to confirm that there is indeed no overlap.

[alexpensify]

@robertjchen we need your review here? Thanks!

[robertjchen]

Let's do it 👍

[melvin-bot]

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

[melvin-bot]

📣 @cretadn22 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[ikevin127]

@cretadn22 Please let us know when we can expect a PR to be ready for review🧑‍💻

[cretadn22]

The PR is up today

[alexpensify]

Awesome, waiting for automation to kick in here.

[Christinadobrzyn]

hey there! #46896 was created while testing the PR linked to this issue.

Should we treat #46896 separately or fix it as part of this?

[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.17-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Add parentReportActionID to new expense report on the approve flow #46721

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

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

• @ikevin127 requires payment automatic offer (Reviewer)
• @cretadn22 requires payment automatic offer (Contributor)

[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:

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR:
• [@ikevin127] 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:
• [@ikevin127] 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:
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@ikevin127] 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.
• [@alexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[ikevin127]

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: Create optimistic report when approving/paying for report with hold expenses #42573.
• [@ikevin127] 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: Create optimistic report when approving/paying for report with hold expenses #42573 (comment).
• [@ikevin127] 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: N/A.
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@ikevin127] 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.

Regression Test Proposal

Precondition: Workspace has an approver (can be workspace owner).

1. Go to workspace chat.
2. Submit three expenses to the workspace chat.
3. Hold two of the expenses.
4. Go offline.
5. Go back to the main workspace chat.
6. Click Approve.
7. Approve partially (click the green approve button for only compliant expenses).
8. The two held expenses should show underneath as optimistic.
9. Click on the new report generated which contains the two held expenses.
10. Click on any of the held expense preview to go to the transaction thread.
11. Click on the report header.
12. Ensure that the Unhold option is present.

Do we agree 👍 or 👎.

[ikevin127]

cc @alexpensify

[alexpensify]

Payouts due: 2024-08-14

• Contributor: $250 @cretadn22
• Contributor+: $250 @ikevin127

Upwork job is here.

[alexpensify]

I've completed the payment process via Upwork and will close this issue for now. Later this week, I'll create the regression test request.