[$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 [email protected] 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:

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

Screenshots/Videos

https://github.com/user-attachments/assets/65bb2063-5079-4993-baf8-85d3fec1d880

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-03

Issue Owner

Current Issue Owner: @ikevin127

[melvin-bot[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. https://github.com/Expensify/App/blob/f4f8da74c100f2c5ddb666ff4314038010104cbf/src/components/SelectionList/Search/ReportListItem.tsx#L39-L44

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 https://github.com/Expensify/App/blob/10a7be853c9e449f5c680cedaeb298dfae95afae/src/components/SelectionList/Search/ReportListItem.tsx#L39-L44

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

[melvin-bot[bot]]

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

[melvin-bot[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 https://github.com/Expensify/App/issues/53045#issuecomment-2499391353 - I think it should be treated as a separate issue.

[melvin-bot[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) https://github.com/Expensify/App/blob/4c47abedddbbdfb163faad81afcfbbcd84766f6e/src/libs/TransactionUtils/index.ts#L428-L449

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 https://github.com/Expensify/App/pull/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: https://github.com/Expensify/App/blob/64eaf2fdd357bd06008adfb9c0c049e09cd375b9/src/pages/iou/request/step/IOURequestStepTaxRatePage.tsx#L65

It's from this PR: https://github.com/Expensify/App/pull/40443

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

[iwiznia]

ccing @thienlnam too since he was reviewer of https://github.com/Expensify/App/pull/38709

[melvin-bot[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.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[ikevin127]

Still working on a decision regarding the direction of this issue and its fix.

[mallenexpensify]

@thienlnam 👀 above plz cuz you were the reviewier on the PR below.

we can track expense in workspace too. It was added in https://github.com/Expensify/App/pull/38709,

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

https://github.com/Expensify/App/blob/64eaf2fdd357bd06008adfb9c0c049e09cd375b9/src/pages/iou/request/step/IOURequestStepTaxRatePage.tsx#L65

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.

https://github.com/Expensify/App/blob/3baf965618ced48845ddd7534b0fad4d4110b5de/src/libs/TransactionUtils/index.ts#L423-L430

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

[ikevin127]

Not overdue - still discussing.

[melvin-bot[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[bot]]

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