[HOLD for payment 2024-12-16] [HOLD for payment 2024-12-11] [$250] [Held requests] Partial paying a report causes the wrong amounts to show in the LHN

[m-natarajan]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.30-15
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/p1725649428907579

Action Performed:

1. On a Collect policy, have user A submit several expenses to User B
2. As User B, hold at least two expenses, approve the rest

Expected Result:

The report amount in the LHN is correct.

Actual Result:

The report amount in the LHN only shows the amount of one expense

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

Recording.522.mp4

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021833144594407206391
• Upwork Job ID: 1833144594407206391
• Last Price Increase: 2024-09-09

Issue Owner

Current Issue Owner: @robertjchen

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

[bernhardoj]

Proposal

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

Approving partly a report with held transaction will show the incorrect amount for the new report that contains all the held transaction.

What is the root cause of that problem?

There are 2 problems. First, when we approve the report, we create the new expense report optimistically that contains all the held transactions, but, we only use the first amount as the report total amount.

App/src/libs/actions/IOU.ts

Lines 6390 to 6394 in 03ec468

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

Also, we don't optimistically update the approved report total by subtracting the current total with the new expense report total

App/src/libs/actions/IOU.ts

Lines 6943 to 6949 in 03ec468

	const optimisticIOUReportData: OnyxUpdate = {
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.REPORT}${expenseReport?.reportID}`,
	value: {
	...expenseReport,
	lastMessageText: ReportActionsUtils.getReportActionText(optimisticApprovedReportAction),
	lastMessageHtml: ReportActionsUtils.getReportActionHtml(optimisticApprovedReportAction),

Second, even though the optimistic data is incorrect, we receive the correct data from the BE response. However, the LHN last message isn't updated immediately. The LHN shows the total from the expense report and we get the report inside the function instead of connecting the LHN component with the report.

App/src/libs/OptionsListUtils.ts

Lines 625 to 626 in 03ec468

	} else if (ReportActionUtils.isReportPreviewAction(lastReportAction)) {
	const iouReport = ReportUtils.getReportOrDraftReport(ReportActionUtils.getIOUReportIDFromReportActionPreview(lastReportAction));

And even though we connect all reports inside lhn list,

App/src/components/LHNOptionsList/LHNOptionsList.tsx

Line 35 in 25cdaa9

const [reports] = useOnyx(ONYXKEYS.COLLECTION.REPORT);

the OptionRowLHNData is memoized, so it won't re-render if the props are not changed.

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

Update the optimistic data to set the correct amount for the new expense report.

App/src/libs/actions/IOU.ts

Line 6394 in 03ec468

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

We can calculate it from the report total and unheldTotal,
(iouReport.total - iouReport.unheldTotal) * -1,

or sum the amount from the individual transaction amount.
holdTransactions.reduce((acc, transaction) => acc + transaction.amount, 0)

Then, we need to update the current expense report total too.

App/src/libs/actions/IOU.ts

Lines 6918 to 6922 in 03ec468

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

App/src/libs/actions/IOU.ts

Lines 6943 to 6949 in 03ec468

	const optimisticIOUReportData: OnyxUpdate = {
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.REPORT}${expenseReport?.reportID}`,
	value: {
	...expenseReport,
	lastMessageText: ReportActionsUtils.getReportActionText(optimisticApprovedReportAction),
	lastMessageHtml: ReportActionsUtils.getReportActionHtml(optimisticApprovedReportAction),

...expenseReport,
total,
...,

Solving the optimistic data is currently enough for the LHN to show the correct amount. But if we also want to fix the 2nd issue, then we can connect the expense report in OptionRowLHNData.

const [iouReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${fullReport?.iouReportID}`);

Then, pass it to SidebarUtils.getOptionData > OptionsListUtils.getLastMessageTextForReport and replace the iouReport here with the new iouReport params.

App/src/libs/OptionsListUtils.ts

Lines 625 to 626 in 03ec468

	} else if (ReportActionUtils.isReportPreviewAction(lastReportAction)) {
	const iouReport = ReportUtils.getReportOrDraftReport(ReportActionUtils.getIOUReportIDFromReportActionPreview(lastReportAction));

However, there is a BE issue, specifically the ApproveMoneyRequest API where the response returns the previous iouReportID instead of the new one. For example, let's say the current iouReportID is 1 which has 2 held transactions. After approving it, we optimistically move the 2 held transactions to a new report with an ID of 2 then update iouReportID to 2. But the ApproveMoneyRequest response revert the iouReportID back to 1. If we refresh/reopen the chat report, the OpenReport will return the correct iouReportID, 2. So, we need to fix the BE if we want to apply this solution.

If we don't want to rely on iouReportID, then we can follow the same logic here by taking the last action and extract the iouReportID.

App/src/libs/OptionsListUtils.ts

Line 626 in 03ec468

const iouReport = ReportUtils.getReportOrDraftReport(ReportActionUtils.getIOUReportIDFromReportActionPreview(lastReportAction));

Fortunately, we already get the iouReportID here,

App/src/components/LHNOptionsList/LHNOptionsList.tsx

Line 131 in 25cdaa9

const iouReportIDOfLastAction = OptionsListUtils.getIOUReportIDOfLastAction(itemFullReport);

so we just need to retrieve the report and pass it down to OptionRowLHNData.

const itemIouReport = reports?.[`${ONYXKEYS.COLLECTION.REPORT}${iouReportIDOfLastAction}`];
...

iouReport={itemIouReport}

[melvin-bot]

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

[melvin-bot]

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

[trjExpensify]

Adding Robert for the secondary internal review of the proposals for hold. @s77rt can you review the above from Bernhard? Thanks!

[trjExpensify]

Just a request that we make sure we test proposed solutions with both iouReports and expenseReports, because they are different report types.

[s77rt]

@bernhardoj Thanks for the proposal. Can you explain how this is related to optimistic data? Per the OP video, the incorrect data is on User A and not User B. (The optimistic data you referred to are for User B)

Seems like a pusher related issue

[bernhardoj]

Oh, I didn't notice the OP video since it's different from the Actions performed. Can you test using the Actions performed instead of the OP video?

[s77rt]

@bernhardoj Ah I see, we have multiple similar bugs. The RCA and the solution for the bug you mentioned make sense to me. But let's not update the total, as that total is meant for the total that was approved and not for what's left as total to be paid. Fixing getReportFromHoldRequestsOnyxData is sufficient.

🎀 👀 🎀 C+ reviewed
Link to proposal

[melvin-bot]

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

[s77rt]

@robertjchen Please note that we have two bugs reported here. The bug reported in the "Action Performed" section is FE and @bernhardoj's solution above will fix it. However the bug showcased in the OP video is a little different and seems related to pusher events. After the payer approved some expenses, the payee got wrong new amount that the WS still owes (it should be non-zero since some expenses are still held)

[kevinksullivan]

This one's moving forward. Looping in another BZ member as I'll be off till September 24

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

• Fix wrong total amount of hold expenses when approve/pay partially #49971

If no regressions arise, payment will be issued on 2024-12-11. 🎊

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

• @s77rt requires payment through NewDot Manual Requests
• @bernhardoj requires payment through NewDot Manual Requests

[melvin-bot]

@s77rt @RachCHopkins @s77rt The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[RachCHopkins]

Hey @bernhardoj have you already been paid here? I assume no, since there is no additional comment from the payer, but I just want to be 100% sure.

[bernhardoj]

Yes, I've been paid before, but then we realize there is a case we don't handle correctly. But I think it would be fair if the price is increased here since we tackle multiple other issues too on the 2nd PR.

cc: @s77rt

[s77rt]

True that multiple issues have been handled but also some regressions (1, 2) has been introduced. I think both would cancel each other and issue no raise and no penalties either. Keeping this at a flat $250.

[bernhardoj]

The first one isn't a regression. Before the PR, we just took 1 transaction amount from the expense, and then we calculated it all on the 1st PR, but we didn't handle the case when there was a different currency. The 2nd one as explained the root here, it was done as an attempt to fix other issues too.

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

• Fix paying partially expense report with non-reimbursable results in error #53622

If no regressions arise, payment will be issued on 2024-12-16. 🎊

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

• @s77rt requires payment through NewDot Manual Requests
• @bernhardoj requires payment through NewDot Manual Requests

[melvin-bot]

@s77rt @RachCHopkins @s77rt The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[s77rt]

@bernhardoj Right, the first one was already broken but the second is something that I expected to be handled in the PR. But I agree that the work turned out more complex than originally expected. @RachCHopkins Can you please bump this to $500?

As for the checklist, it has been already provided in #48760 (comment) but now we need to add a regression test:

Regression Test Proposal

Precondition:

• Has a workspace (admin)
• Workspace has approval workflow

Test:

1. Open the workspace chat
2. Submit 2 expenses with different currencies (e.g. $10 and €5)
3. Track 1 expense (e.g. $20)
4. Hold the 2 submitted expenses
5. Approve the report partially
6. Verify the workspace chat LHN row shows the correct owed amount that is the total of the hold expenses (e.g. $15.28)

[RachCHopkins]

@robertjchen do you agree that this is worth $500 due to the extra work involved?

And @bernhardoj and @s77rt who has been paid what so far? As far as I'm aware, @bernhardoj has been paid $250 and @s77rt has not put any payment request in yet. Is that correct?

[bernhardoj]

Yes, I've been paid $250 before.