[$250] Held expense-Negative amount in transaction thread for held expense after paying unheld expense

[IuliiaHerets]

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.40-1
Reproducible in staging?: Y
Reproducible in production?: Y
Issue was found when executing this PR: #49463
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. [User A] Submit two expenses to User B.
3. [User A] Hold one of the expenses.
4. [User B] Go offline.
5. [User B] Pay the unheld expense.
6. [User B] Click on the expense preview of the held expense in the main chat.

Expected Result:

The amount in the transaction thread will not be negative.

Actual Result:

The amount in the transaction thread is negative.

Workaround:

Unknown

Platforms:

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6615322_1727299431457.20240926_051632.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021841126678085530276
• Upwork Job ID: 1841126678085530276
• Last Price Increase: 2024-10-15
• Automatic offers:
• Nodebrute | Contributor | 104483850

Issue Owner

Current Issue Owner: @hungvu193

[melvin-bot]

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

[IuliiaHerets]

We think that this bug might be related to #wave-collect - Release 1

[IuliiaHerets]

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

[Nodebrute]

Edited by proposal-police: This proposal was edited at 2024-10-09 18:11:01 UTC.

Proposal

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

Negative amount in transaction thread for held expense after paying unheld expense

What is the root cause of that problem?

For expense report case we add - before amount because the amounts are stored using an opposite sign

App/src/libs/TransactionUtils/index.ts

Lines 345 to 351 in b655040

	// Expense report case:
	// The amounts are stored using an opposite sign and negative values can be set,
	// we need to return an opposite sign than is saved in the transaction object
	let amount = transaction?.modifiedAmount ?? 0;
	if (amount) {
	return -amount;
	}

However, when we handle the pay money request and use getReportFromHoldRequestsOnyxData, we are actually creating an ExpenseReport. For 1:1 direct messages, it should instead be an IOUReport.

App/src/libs/actions/IOU.ts

Lines 6451 to 6459 in b655040

	const optimisticExpenseReport = ReportUtils.buildOptimisticExpenseReport(
	chatReport.reportID,
	chatReport.policyID ?? iouReport?.policyID ?? '',
	recipient.accountID ?? 1,
	holdTransactions.reduce((acc, transaction) => acc + transaction.amount, 0) * (ReportUtils.isIOUReport(iouReport) ? 1 : -1),
	getCurrency(firstHoldTransaction),
	false,
	newParentReportActionID,
	);

This is also not consistent with the BE response in case of 1:1 DMs the backend will return an IOUReport

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

We should add a check here isPolicyExpenseChat. The idea is that if it's a policy expense chat, we should build an optimistic expense report using buildOptimisticExpenseReport. If it's not a policy expense chat, then we should build an optimistic IOU report using buildOptimisticIOUReport. This is the same logic we are using here Pseudocode

 const optimisticExpenseReport = isPolicyExpenseChat
        ? ReportUtils.buildOptimisticExpenseReport(
              chatReport.reportID,
              chatReport.policyID ?? iouReport?.policyID ?? '',
              recipient.accountID ?? 1,
              totalAmount,
              getCurrency(firstHoldTransaction),
              false,
              newParentReportActionID,
          )
        : ReportUtils.buildOptimisticIOUReport(recipient.accountID ?? 1, iouReport?.managerID ?? 1, totalAmount, chatReport.reportID, getCurrency(firstHoldTransaction), false);

After this change, this line will evaluate to false, and we'll move to this block, where we'll apply -amount, which will fix the bug

App/src/libs/TransactionUtils/index.ts

Line 337 in b655040

if (!isFromExpenseReport || isFromTrackedExpense) {

Note: This is pseudocode only. We need to test the implementation with multiple test cases, which can be done in the pull request (PR).

What alternative solutions did you explore? (Optional)

We can use buildOptimisticIOUReport here instead of buildOptimisticExpenseReport

[OfstadC]

Missed this yesterday - reviewing now

[melvin-bot]

@OfstadC Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[OfstadC]

I had issues reproducing this on Friday - forgot to add a comment here.

[m-natarajan]

Able to reproduce

20241001_220541.mp4

[melvin-bot]

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

[melvin-bot]

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

[hungvu193]

On my list this weekend.

[hungvu193]

Reviewing shortly

[hungvu193]

hey @Nodebrute, I think your proposal makes sense, can you please provide a little more detailed about your main solution? TIA.

[Nodebrute]

Thank You @hungvu193, I'll update in few hours.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[hungvu193]

Waiting for contributor information, no action required

[melvin-bot]

@OfstadC @hungvu193 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[Nodebrute]

@hungvu193 I've updated my main proposal. I tried my best to explain it as clearly as possible, but I'm unsure how to add more detail. Please let me know if you have any questions, and I'll do my best to answer them.

[hungvu193]

Thanks I'll review it today

[hungvu193]

It worked but I still need to review it carefully.

[hungvu193]

@Nodebrute Ok, can you confirm if this diff is matching with your idea?

diff --git a/src/libs/actions/IOU.ts b/src/libs/actions/IOU.ts
index de896e6f72f..c5d6fad1211 100644
--- a/src/libs/actions/IOU.ts
+++ b/src/libs/actions/IOU.ts
@@ -6471,16 +6471,19 @@ function getReportFromHoldRequestsOnyxData(
     const {holdReportActions, holdTransactions} = getHoldReportActionsAndTransactions(iouReport?.reportID ?? '');
     const firstHoldTransaction = holdTransactions.at(0);
     const newParentReportActionID = rand64();
-
-    const optimisticExpenseReport = ReportUtils.buildOptimisticExpenseReport(
-        chatReport.reportID,
-        chatReport.policyID ?? iouReport?.policyID ?? '',
-        recipient.accountID ?? 1,
-        holdTransactions.reduce((acc, transaction) => acc + transaction.amount, 0) * (ReportUtils.isIOUReport(iouReport) ? 1 : -1),
-        getCurrency(firstHoldTransaction),
-        false,
-        newParentReportActionID,
-    );
+    const isPolicyExpenseChat = ReportUtils.isPolicyExpenseChat(chatReport);
+    const totalHoldAmount = holdTransactions.reduce((acc, transaction) => acc + transaction.amount, 0) * (ReportUtils.isIOUReport(iouReport) ? 1 : -1);
+    const optimisticExpenseReport = isPolicyExpenseChat
+        ? ReportUtils.buildOptimisticExpenseReport(
+              chatReport.reportID,
+              chatReport.policyID ?? iouReport?.policyID ?? '',
+              recipient.accountID ?? 1,
+              totalHoldAmount,
+              getCurrency(firstHoldTransaction),
+              false,
+              newParentReportActionID,
+          )
+        : ReportUtils.buildOptimisticIOUReport(recipient.accountID ?? 1, iouReport?.managerID ?? 1, totalHoldAmount, chatReport.reportID, getCurrency(firstHoldTransaction), false);
     const optimisticExpenseReportPreview = ReportUtils.buildOptimisticReportPreview(
         chatReport,
         optimisticExpenseReport,

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[Nodebrute]

@hungvu193 thank you for your patience. I'll update this in few hours.

[Nodebrute]

@hungvu193 This is the code.

    const isPolicyExpenseChat = ReportUtils.isPolicyExpenseChat(chatReport);
    const holdTransactionAmount = holdTransactions.reduce((acc, transaction) => acc + TransactionUtils.getAmount(transaction), 0);
    const optimisticExpenseReport = isPolicyExpenseChat
        ? ReportUtils.buildOptimisticExpenseReport(
              chatReport.reportID,
              chatReport.policyID ?? iouReport?.policyID ?? '',
              recipient.accountID ?? 1,
              holdTransactions.reduce((acc, transaction) => acc + TransactionUtils.getAmount(transaction), 0),
              getCurrency(firstHoldTransaction),
              false,
              newParentReportActionID,
          )
        : ReportUtils.buildOptimisticIOUReport(recipient.accountID ?? 1, iouReport?.managerID ?? 1, holdTransactionAmount, chatReport.reportID, getCurrency(firstHoldTransaction), false);

The only change here from your code diff is

 const holdTransactionAmount = holdTransactions.reduce((acc, transaction) => acc + TransactionUtils.getAmount(transaction), 0);

This is because we only want to add the amount. There's no need to add a - sign here; we can pass the amount, just like we do in other places. You can test this code snippet—it’s working for me.

[hungvu193]

I see/ i'll check it in a while

[hungvu193]

Thanks for your details. @Nodebrute 's proposal here looks good to me!

[hungvu193]

🎀 👀 🎀

[melvin-bot]

Triggered auto assignment to @blimpich, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

📣 @Nodebrute 🎉 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 📖

[melvin-bot]

@OfstadC, @hungvu193, @blimpich, @Nodebrute Whoops! This issue is 2 days overdue. Let's get this updated quick!

[hungvu193]

Not overdue. PR is inprogress