Open IuliiaHerets opened 3 weeks ago
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.
We think that this bug might be related to #wave-collect - Release 1
@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
Edited by proposal-police: This proposal was edited at 2024-10-09 18:11:01 UTC.
Negative amount in transaction thread for held expense after paying unheld expense
For expense report case we add -
before amount because the amounts are stored using an opposite sign
https://github.com/Expensify/App/blob/b655040afb3d5a482fd0c295dda4a6d8802c3df6/src/libs/TransactionUtils/index.ts#L345-L351
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
.https://github.com/Expensify/App/blob/b655040afb3d5a482fd0c295dda4a6d8802c3df6/src/libs/actions/IOU.ts#L6451-L6459
This is also not consistent with the BE response in case of 1:1 DMs the backend will return an IOUReport
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
https://github.com/Expensify/App/blob/b655040afb3d5a482fd0c295dda4a6d8802c3df6/src/libs/TransactionUtils/index.ts#L337
Note: This is pseudocode only. We need to test the implementation with multiple test cases, which can be done in the pull request (PR).
We can use buildOptimisticIOUReport
here instead of buildOptimisticExpenseReport
Missed this yesterday - reviewing now
@OfstadC Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
I had issues reproducing this on Friday - forgot to add a comment here.
Job added to Upwork: https://www.upwork.com/jobs/~021841126678085530276
Triggered auto assignment to Contributor-plus team member for initial proposal review - @hungvu193 (External
)
On my list this weekend.
Reviewing shortly
hey @Nodebrute, I think your proposal makes sense, can you please provide a little more detailed about your main solution? TIA.
Thank You @hungvu193, I'll update in few hours.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
Waiting for contributor information, no action required
@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!
@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.
Thanks I'll review it today
It worked but I still need to review it carefully.
@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,
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@hungvu193 thank you for your patience. I'll update this in few hours.
@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.
I see/ i'll check it in a while
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.40-1 Reproducible in staging?: Y Reproducible in production?: Y Issue was found when executing this PR: https://github.com/Expensify/App/pull/49463 Email or phone of affected tester (no customers): applausetester+kh010901@applause.expensifail.com Issue reported by: Applause Internal Team
Action Performed:
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:
Screenshots/Videos
https://github.com/user-attachments/assets/e26b1db0-7ac1-4245-a8f4-a053402cd943
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @hungvu193