[$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 contributors@expensify.com 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: @s77rt

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

[JmillsExpensify]

$250 approved for @bernhardoj

[bernhardoj]

@s77rt Oh, I didn't notice that. I think we should use whatever the currency of the current IOU report, so it would be,

iouReport.currency,

What do you think?

[s77rt]

@bernhardoj It could work if the calculated unheldTotal is also in the iou currency

[bernhardoj]

Yes, the unheld total is also in the IOU currency (the green button).

https://github.com/user-attachments/assets/e313b04a-138b-44ad-a062-869eaea0a793

The approve confirmation modal uses unheldTotal and total. Since both total and unheldTotal are in the IOU currency, I think we can safely use them.

But I honestly still don't understand about this one

[s77rt]

@bernhardoj Regarding https://github.com/Expensify/App/pull/49279#issuecomment-2353428039, I think what I reproduced was an edge-case. Basically what I was trying to say is that we have 2 totals, total and nonReimbursableTotal and the total that was being calculated was not clear which one of those (or if it's the sum of the two). After looking into the code I can't find much about non-reimbursable transactions and I think it's not implemented in ND yet. I have asked in Slack.

In OD however you can create non-reimbursable transactions and after testing I got a broken behaviour already, green button showing to approve 0.00 which is not correct and the total is negative and also not correct. Assuming those are not regressions from the previous PR, we can probably proceed with your suggested solution.

https://github.com/user-attachments/assets/863d43d3-c1f0-4deb-81a2-68a816674fda

[bernhardoj]

total and nonReimbursableTotal and the total that was being calculated was not clear which one of those (or if it's the sum of the two).

If we see this code, we can infer that nonReimbursableTotal is part of total. https://github.com/Expensify/App/blob/d18766580c7e35e439ff933f3097366e36c6ceca/src/libs/ReportUtils.ts#L2719-L2729

green button showing to approve 0.00

Since the green button shows the amount from unheldTotal, this means the unheldTotal is 0.

the total is negative and also not correct

Expense report total is negative.

[robertjchen]

To clarify, this incorrect total calculation is only happening for the optimistic/offline action, is that correct?

[bernhardoj]

@robertjchen I'm guessing you are talking about this one. The response from PayMoneyRequest doesn't return the IOU/expense report total, so I can't say if the BE calculation is correct or not, but yes, the total calculation currently is incorrect optimistically.

[s77rt]

@bernhardoj For now can you please raise a PR to use the unheldTotal to calculate the new total to fix the regression.

[bernhardoj]

@s77rt PR is ready

[melvin-bot[bot]]

This issue has not been updated in over 15 days. @robertjchen, @s77rt, @RachCHopkins, @bernhardoj eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[s77rt]

Still working on it. We are having problems determining the right values for total and unheldTotal for IOU reports when the owner changes

[RachCHopkins]

Active discussion ongoing.