[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
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/d0f83087-4e76-4ac9-b13f-1c07f397d79f

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: @RachCHopkins

[melvin-bot[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. https://github.com/Expensify/App/blob/03ec468bfb14265d8953a8cfad7a0a875991fd8f/src/libs/actions/IOU.ts#L6390-L6394

Also, we don't optimistically update the approved report total by subtracting the current total with the new expense report total https://github.com/Expensify/App/blob/03ec468bfb14265d8953a8cfad7a0a875991fd8f/src/libs/actions/IOU.ts#L6943-L6949

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. https://github.com/Expensify/App/blob/03ec468bfb14265d8953a8cfad7a0a875991fd8f/src/libs/OptionsListUtils.ts#L625-L626

And even though we connect all reports inside lhn list, https://github.com/Expensify/App/blob/25cdaa9be8520b6b633e2515c665a12234dcf970/src/components/LHNOptionsList/LHNOptionsList.tsx#L35

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. https://github.com/Expensify/App/blob/03ec468bfb14265d8953a8cfad7a0a875991fd8f/src/libs/actions/IOU.ts#L6394

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. https://github.com/Expensify/App/blob/03ec468bfb14265d8953a8cfad7a0a875991fd8f/src/libs/actions/IOU.ts#L6918-L6922 https://github.com/Expensify/App/blob/03ec468bfb14265d8953a8cfad7a0a875991fd8f/src/libs/actions/IOU.ts#L6943-L6949

...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. https://github.com/Expensify/App/blob/03ec468bfb14265d8953a8cfad7a0a875991fd8f/src/libs/OptionsListUtils.ts#L625-L626

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. https://github.com/Expensify/App/blob/03ec468bfb14265d8953a8cfad7a0a875991fd8f/src/libs/OptionsListUtils.ts#L626

Fortunately, we already get the iouReportID here, https://github.com/Expensify/App/blob/25cdaa9be8520b6b633e2515c665a12234dcf970/src/components/LHNOptionsList/LHNOptionsList.tsx#L131

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[bot]]

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

[melvin-bot[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[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[bot]]

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

[trjExpensify]

@robertjchen are you happy with this proposal then? If so, can we assign @bernhardoj to move forward with it?

[robertjchen]

Yep, let's do it! 👍

[trjExpensify]

Great, @bernhardoj please proceed with a PR ASAP. Thanks!

[bernhardoj]

PR is ready

cc: @s77rt

[robertjchen]

PR reviewed and merged 👍 Looking into BE

[robertjchen]

Potential BE fix in review ^

[robertjchen]

BE fix merged and being deployed today

[robertjchen]

I think we're all set here! The only remaining issue involves seeing a 0.00 amount in the LHN, but we'll be addressing it in a separate issue here: https://github.com/Expensify/App/issues/48093

[robertjchen]

cc: @RachCHopkins for next steps here!

[RachCHopkins]

Let me confirm the payments since the automation is borked.

• Contributor: @bernhardoj to be paid $250 via newdot manual request
• Contributor+: @s77rt to be paid $250 via newdot manual request

Upwork job here

Is that correct @robertjchen?

[RachCHopkins]

And do we need @s77rt to complete the checklist?

The following checklist (instructions) will need to be completed before the issue can be closed:

• [ ] The PR that introduced the bug has been identified. Link to the PR:
• [ ] 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:
• [ ] 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:
• [ ] 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.
• [ ] Link the GH issue for creating/updating the regression test once above steps have been agreed upon

[bernhardoj]

Requested in ND.

[s77rt]

• The PR that introduced the bug has been identified: https://github.com/Expensify/App/pull/42573
• The offending PR has been commented on: https://github.com/Expensify/App/pull/42573#discussion_r1777020766
• A discussion in #expensify-bugs has been started : Not needed
• Determine if we should create a regression test for this bug: Not yet. I just found this comment that may change the expected hehaviour

[s77rt]

@bernhardoj I just found out we sum up the hold transactions values without taking into account their currency. This causes wrong value calculation if we hold multiple expenses with different currencies

https://github.com/user-attachments/assets/866ed0fd-6df2-43b0-bbc0-4cf0db182782

cc @robertjchen since you are working on fixing a similar bug on BE, I think you may face the same issue

[bernhardoj]

Oh, that's bad. That means we can't use the solution to iterate over the transaction amount. I think we need to revisit the approach of subtracting total - unheldTotal.

[s77rt]

@bernhardoj Will that approach be enough to make it work? I see this line that gets the currency based on the first hold transaction https://github.com/Expensify/App/blob/b5a0a2967635d117506e3dc4c1b78139d0d17b44/src/libs/actions/IOU.ts#L6457

If the first transaction currency is different than the others then the problem will still occur

[melvin-bot[bot]]

@robertjchen, @s77rt, @kevinksullivan, @RachCHopkins, @bernhardoj Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!