[$250] Search-Invoice with +2 expense submitted in OD is displayed with a negative total amount in ND

[IuliiaHerets]

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.66-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5256232
Issue reported by: Applause Internal Team

Action Performed:

1. Login with a account that is an admin of a workspace
2. Login with the same account in OD
3. In OD - Submit a invoice to anyone with at least 2 expenses
4. Back in ND - Navigate to the Search page
5. Click on invoices
6. Click on the Outstanding filter
7. Verify the expenses are now divided per invoice report
8. Verify the invoice submitted in OD has the 2 expenses grouped

Expected Result:

The invoice with at least 2 expenses submitted in OD is displayed with a positive total amount in ND.

Actual Result:

The invoice with at least 2 expenses submitted in OD is displayed with a negative total amount in ND.

Workaround:

Unknown

Platforms:

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6674480_1732394559495.total_negative_amount.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021861216736472283553
• Upwork Job ID: 1861216736472283553
• Last Price Increase: 2024-12-17

Issue Owner

Current Issue Owner: @ikevin127

[melvin-bot]

Triggered auto assignment to @mallenexpensify (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]

Edited by proposal-police: This proposal was edited at 2024-11-28 02:32:31 UTC.

Proposal

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

The total invoice report amount is shown in negative.

What is the root cause of that problem?

The total amount of the invoice report is in negative, the same as expense report. However, when we show the total amount, we only negate the expense report.

App/src/components/SelectionList/Search/ReportListItem.tsx

Lines 39 to 44 in f4f8da7

	let total = reportItem?.total ?? 0;
	// Only invert non-zero values otherwise we'll end up with -0.00
	if (total) {
	total *= reportItem?.type === CONST.REPORT.TYPE.EXPENSE ? -1 : 1;
	}

So, for invoice report, the negative amount is shown.

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

We need to check for invoice type too and if it's true, then we negate it too (multiply by -1)

total *= (reportItem?.type === CONST.REPORT.TYPE.EXPENSE || reportItem?.type === CONST.REPORT.TYPE.INVOICE) ? -1 : 1;

OR

total *= (isExpenseReport(reportItem) || isInvoiceReport(reportItem)) ? -1 : 1;

(or we can use Math.abs if it's not an expense report)

There are multiple places where we check for expense report type to multiply it by -1 where we can probably apply the same fix, but looks like it's currently not possible to view an invoice report, so not sure how to test the other cases. To handle the other cases too, we can create a new util function in IOUUtils called getNormalizedAmount.

function getNormalizedAmount(amount: number, report: Report) {
    // Only invert non-zero values otherwise we'll end up with -0.00
    if (!amount) {
        return amount;
    }

    // Expense and invoice report case:
    // The amounts are stored using an opposite sign and negative values can be set,
    // we need to return an opposite sign
    if (isExpenseReport(report) || isInvoiceReport(report)) {
        return -amount;
    }

    // IOU requests cannot have negative values, but they can be stored as negative values, let's return absolute value
    return Math.abs(amount);
}

And then we can use it like this

App/src/components/SelectionList/Search/ReportListItem.tsx

Lines 39 to 44 in 10a7be8

	let total = reportItem?.total ?? 0;
	// Only invert non-zero values otherwise we'll end up with -0.00
	if (total) {
	total *= reportItem?.type === CONST.REPORT.TYPE.EXPENSE ? -1 : 1;
	}

const total = IOUUtils.getNormalizedAmount(reportItem?.total ?? 0, reportItem);

[melvin-bot]

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

[melvin-bot]

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

[mallenexpensify]

There are multiple places where we check for expense report type to multiply it by -1 where we can probably apply the same fix,

Music to my ears @bernhardoj :)

Was able to reproduce

When I click on View I get an error, is that also related here?

Only error I see in console

[bernhardoj]

The BE returns not found error when viewing it.

[ikevin127]

Let's go with @bernhardoj's proposal since the RCA makes sense and the proposed solution fixes the issue.

Regarding the solution, since the component already imports ReportUtils, I like this version as it's 14 characters shorter:

total *= ReportUtils.isExpenseReport(reportItem) || ReportUtils.isInvoiceReport(reportItem) ? -1 : 1;

🎀👀🎀 C+ reviewed

When I click on View I get an error, is that also related here ?

I don't see it related to the negative amount issue in any way which would justify extending the scope of this issue given the context shared in #53045 (comment) - I think it should be treated as a separate issue.

[melvin-bot]

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

[iwiznia]

Anyone knows why we invert the amount for expense (and now invoice) but not other types?

Solution sounds good, but it seems like we should centralize this logic in one function so we don't have the same bug in many places and try to find all places where this logic exists... I see for example we have it also here

Can you update the proposal to include that please?

[bernhardoj]

It's explained here (invoice not included)

App/src/libs/TransactionUtils/index.ts

Lines 428 to 449 in 4c47abe

	function getAmount(transaction: OnyxInputOrEntry<Transaction>, isFromExpenseReport = false, isFromTrackedExpense = false): number {
	// IOU requests cannot have negative values, but they can be stored as negative values, let's return absolute value
	if (!isFromExpenseReport || isFromTrackedExpense) {
	const amount = transaction?.modifiedAmount ?? 0;
	if (amount) {
	return Math.abs(amount);
	}
	return Math.abs(transaction?.amount ?? 0);
	}
	// 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;
	}
	// To avoid -0 being shown, lets only change the sign if the value is other than 0.
	amount = transaction?.amount ?? 0;
	return amount ? -amount : 0;
	}

I think we can create a util function in IOUUtils called getNormalizedAmount, but do we want to follow the logic above, that is multiply by -1 for the expense report only and use abs for the invoice?

[iwiznia]

Ah nice, let's ensure we have that explanation in the new method and that we use it there too as it seems that code is also wrong

[iwiznia]

I think we can create a util function in IOUUtils called getNormalizedAmount, but do we want to follow the logic above, that is multiply by -1 for the expense report only and use abs for the invoice?

I don't think so, invoices can be positive or negative, same as expense reports. IOUs are the only ones that are different

[bernhardoj]

Updated my proposal to include the util function.

as it seems that code is also wrong

Which part of TransactionUtils.getAmount is wrong?

[iwiznia]

Actually not sure if it is wrong, because I now realize I have no idea what isFromTrackedExpense means (let's add a comment on the param as part of this change once we figure it out). I was thinking it is not checking invoices there, so it might be using the abs version instead of the negated version.

[bernhardoj]

My first thought that it should be true for self DM track, but I just realized we can track expense in workspace too. It was added in #38709, specifically this commit, but looks like we never make use of the isFromTrackedExpense param in that PR, meaning it's always false.

The only code that uses it is this:

App/src/pages/iou/request/step/IOURequestStepTaxRatePage.tsx

Line 65 in 64eaf2f

const taxAmount = getTaxAmount(policy, currentTransaction, taxes.code, TransactionUtils.getAmount(currentTransaction, false, true));

It's from this PR: #40443

Maybe @ishpaul777 can help us clarify the purpose of that param.

[iwiznia]

ccing @thienlnam too since he was reviewer of #38709

[melvin-bot]

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

[iwiznia]

Waiting on input from the people pinged above

[ikevin127]

Waiting on input from the people pinged above

[ishpaul777]

From the blame history, I recall initially being confused about how tracked expenses should be moved to the workspace chat. In self-DM, tracked expenses were stored as negative values. When moving them to the workspace, I converted them to +ive.

At that time, I added this check. However, after i was corrected by Jack, I removed the parameter locally. Unfortunately, I believe I missed removing the parameter from the function during cleanup, which was an oversight.

As for why the param is used later, I don't have much context, but it appears to be related to this commit: 228c47705cc4a06d8d0a343be222e8e952523039.

[thienlnam]

Could someone summarize what the confusion is regarding?

If it's about the isFromTrackedExpense param, it's been a while but my guess is from when you convert a tracked expense to a reimbursable one since we flip the amount (negative to positive). If it's not used anymore, let's just remove it

[bernhardoj]

Yes, it's the isFromTrackedExpense param, but we have one usage of it on the IOU tax rate page.

App/src/pages/iou/request/step/IOURequestStepTaxRatePage.tsx

Line 65 in 64eaf2f

const taxAmount = getTaxAmount(policy, currentTransaction, taxes.code, TransactionUtils.getAmount(currentTransaction, false, true));

From this commit, previously we get the transaction amount by transaction.amount to calculate the tax amount. That PR updates it to use TransactionUtils.getAmount most likely to cover the case when the amount is updated, thus we should get from transaction.modifiedAmount instead (which what TransactionUtils.getAmount does).

My guess with why we pass isFromTrackedExpense as true and isFromExpenseReport as false is so that the amount returned is always in positive.

App/src/libs/TransactionUtils/index.ts

Lines 423 to 430 in 3baf965

	// IOU requests cannot have negative values, but they can be stored as negative values, let's return absolute value
	if (!isFromExpenseReport || isFromTrackedExpense) {
	const amount = transaction?.modifiedAmount ?? 0;
	if (amount) {
	return Math.abs(amount);
	}
	return Math.abs(transaction?.amount ?? 0);
	}

I think we can safely remove isFromTrackedExpense. @iwiznia What do you think?

[ikevin127]

Not overdue - still discussing.

[melvin-bot]

@iwiznia @mallenexpensify @ikevin127 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!

[ikevin127]

Not overdue - still discussing.

[melvin-bot]

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

[melvin-bot]

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

[ikevin127]

Not overdue - still discussing.

[melvin-bot]

@iwiznia, @mallenexpensify, @ikevin127 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[ikevin127]

Awaiting feedback from @iwiznia regarding #53045 (comment) so we can decide whether we can move forward with the selected proposal or should the solution be adjusted.

[melvin-bot]

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

[iwiznia]

I have no idea if we can safely remove it because I don't get what it does and don't know where it is used. Did you check the usages to ensure we don't need it?

[bernhardoj]

I believe yes since we only have 1 usage as mentioned in my previous comment. Also, if we remove the isFromTrackedExpense, it won't change anything because isFromExpenseReport is passed as false (as shown in my previous comment too).

[iwiznia]

Ah ok, I was confused because I saw more usages, but now I see you meant no usages passing true. I agree on removing it then

[bernhardoj]

Okay great, waiting for an assignment then.

[melvin-bot]

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

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[ikevin127]

Issue not reproducible during KI retests. (First week)

@bernhardoj Can you confirm whether the issue is still reproducible on your side ? If it is, feel free to tag iwiznia for assignment based on my review from #53045 (comment).

[bernhardoj]

I can't repro anymore. It's been fixed by #52556

[melvin-bot]

@iwiznia @mallenexpensify @ikevin127 this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!