Skip to content

Fixed negative amount turns to positive amount after sorting duplicates #57033

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
iwiznia merged 3 commits into from
Feb 21, 2025
Merged

Fixed negative amount turns to positive amount after sorting duplicates #57033

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f3a1219
Fixed negative amount turns to positive amount after sorting duplicates
shubham1206agra Feb 18, 2025
acb1dc1
Fix sign of amount
shubham1206agra Feb 18, 2025
b658bb4
Merge branch 'Expensify:main' into fix-merge-transaction-amount
shubham1206agra Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libs/TransactionUtils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1380,7 +1380,7 @@ function buildNewTransactionAfterReviewingDuplicates(reviewDuplicateTransaction:

function buildTransactionsMergeParams(reviewDuplicates: OnyxEntry<ReviewDuplicates>, originalTransaction: Partial<Transaction>): TransactionMergeParams {
return {
amount: -getAmount(originalTransaction as OnyxEntry<Transaction>, false),
amount: -getAmount(originalTransaction as OnyxEntry<Transaction>, true),
Copy link
Contributor

@iwiznia iwiznia Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite get this method:

  1. Why are there 2 params isFromExpenseReport and isFromTrackedExpense that do the same thing?
  2. What does isFromTrackedExpense mean?
  3. Why are we passing a hardcoded true/false here, shouldn't it be looking at what report the transaction is in and pass true/false based on that?

Copy link
Contributor Author

@shubham1206agra shubham1206agra Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we passing a hardcoded true/false here, shouldn't it be looking at what report the transaction is in and pass true/false based on that?

Since duplicate resolution happens only in expense reports, not IOU reports.

Copy link
Contributor Author

@shubham1206agra shubham1206agra Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are there 2 params isFromExpenseReport and isFromTrackedExpense that do the same thing?

This is not the same. Since we can track expense in workspace too and it uses a different convention than expense report, we use isFromTrackedExpense to force a convention irrespective of where the track expense is present.

Copy link
Contributor

@iwiznia iwiznia Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since duplicate resolution happens only in expense reports, not IOU reports.

Not following, is buildTransactionsMergeParams only used for expense reports? And if so, where are we validating that we don't pass an IOU?

This is not the same. Since we can track expense in workspace too and it uses a different convention than expense report, we use isFromTrackedExpense to force a convention irrespective of where the track expense is present.

Sorry, I don't understand what you mean. They both do exactly the same thing in code. If you pass one or the other to true, the exact same logic applies, so to me one of them should be removed.

Copy link
Contributor Author

@shubham1206agra shubham1206agra Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you pass one or the other to true, the exact same logic applies, so to me one of them should be removed.

Nope, one is inverted and works in a different way. We can merge this in future, but we should not do this here.

Copy link
Contributor Author

@shubham1206agra shubham1206agra Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not following, is buildTransactionsMergeParams only used for expense reports?

Yes

And if so, where are we validating that we don't pass an IOU?

No need since duplicate violation occurs in expense reports.

Copy link
Contributor

@iwiznia iwiznia Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, one is inverted

Oh, true! 👨‍🦯

reportID: originalTransaction?.reportID,
receiptID: originalTransaction?.receipt?.receiptID ?? CONST.DEFAULT_NUMBER_ID,
currency: getCurrency(originalTransaction as OnyxEntry<Transaction>),
Expand Down