Skip to content
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.

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

[$500] Feature Request: show individual expense amounts in both the original currency and the policy currency #38970

Closed
m-natarajan opened this issue Mar 26, 2024 · 43 comments
Assignees
Labels
Internal Requires API changes or must be handled by Expensify staff NewFeature Something to build that is a new item. Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Mar 26, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Problem:

If an expense is in a currency other than the policy currency, not showing what the converted amount is. We show the report total in the policy currency, but there is no indication on the individual expenses what the converted amount is.

Solution:

Ideally we'd show it on both the transaction preview on the report and in the transaction itself.

Should have the same design patterns as card transaction, where the converted amount is shown as the primary amount, with the original amount shown after the dot separator.

CleanShot 2024-03-25 at 09 58 15@2x (1)

Context/Examples/Screenshots/Notes:

image (26)

image (25)

Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @puneetlath
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1711377096912369

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~014de0b2f3db834923
  • Upwork Job ID: 1772587978054914048
  • Last Price Increase: 2024-03-26
@m-natarajan m-natarajan added Weekly KSv2 NewFeature Something to build that is a new item. labels Mar 26, 2024
Copy link

melvin-bot bot commented Mar 26, 2024

Copy link

melvin-bot bot commented Mar 26, 2024

⚠️ It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time ⚠️

@garrettmknight
Copy link
Contributor

Since we're reusing existing patterns, I'm going to open to External and see if we can handle with contributors.

@garrettmknight garrettmknight added the External Added to denote the issue can be worked on by a contributor label Mar 26, 2024
@melvin-bot melvin-bot bot changed the title Feature Request: show individual expense amounts in both the original currency and the policy currency [$500] Feature Request: show individual expense amounts in both the original currency and the policy currency Mar 26, 2024
Copy link

melvin-bot bot commented Mar 26, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 26, 2024
Copy link

melvin-bot bot commented Mar 26, 2024

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 26, 2024
@FitseTLT
Copy link
Contributor

FitseTLT commented Mar 26, 2024

Proposal

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

Feature Request: show individual expense amounts in both the original currency and the policy currency

What is the root cause of that problem?

New Feature

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

In the BE we need to be returning the amount converted in the policy currency in transaction as we don't have that info now (also in action.originalMessage for request preview) May be same as for card transaction populate amount currency with the policy currency and originalAmount originalCurrency with the expense policy,
For MoneyRequestView:
In which case we can use the already existing logic for card transaction to concatenate the description with original amount here

amountDescription += ` • ${translate('iou.original')} ${formattedOriginalAmount}`;
}

For MoneyRequestPreview:
We are now displaying it in the original expense currency so we should add same as card transaction we need to display the amount in policy currency as the main amount and after the dot the amount in the expenses currency
in money request preview
After action.originalMessage is populated in BE with the policy currency converted values we can build the text here

by concatenating • ${translate('iou.original')} and then orignalAmount
Of course We can tweek the styling of the texts as the design teams' recommendation

What alternative solutions did you explore? (Optional)

@nkdengineer
Copy link
Contributor

nkdengineer commented Mar 26, 2024

Proposal

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

show individual expense amounts in both the original currency and the policy currency

What is the root cause of that problem?

We're only add the description for formattedOriginalAmount if the transaction is the card transaction

if (formattedOriginalAmount) {
amountDescription += ` • ${translate('iou.original')} ${formattedOriginalAmount}`;
}

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

  1. Update the condition here to
if (formattedOriginalAmount) {
    amountDescription += ` • ${translate('iou.original')} ${formattedOriginalAmount}`;
} else if (!isDistanceRequest) {
    amountDescription += ` • ${translate('iou.cash')}`;
}

  1. For MoneyRequestPreview, we should get the formattedOriginalAmount and add amountDescription for original amount by the same way we do in MoneyRequestView and then we will display it next to the Cash text.


3. BE need to return the converted amount of the transaction that isn't card transaction as well.

What alternative solutions did you explore? (Optional)

NA

@FitseTLT
Copy link
Contributor

Updated to add some detail

@allgandalf
Copy link
Contributor

Proposal

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

Show individual expense amounts in both the original currency and the policy currency

What is the root cause of that problem?

Feature Request

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

  1. Firstly, We need to update the BE to return originalAmount and originalCurrency on Every Transaction.

    App/src/libs/ReportUtils.ts

    Lines 2229 to 2231 in 9dd66cf

    originalAmount: TransactionUtils.getOriginalAmount(transaction),
    originalCurrency: TransactionUtils.getOriginalCurrency(transaction),
    };

This is because onMoneyRequestPreviewContent we use formattedOriginalAmount to check if we have got the original amount, and formattedOriginalAmount is calculated by:

const formattedOriginalAmount = transactionOriginalAmount && transactionOriginalCurrency && CurrencyUtils.convertToDisplayString(transactionOriginalAmount, transactionOriginalCurrency);

transactionOriginalAmount and transactionOriginalCurrency is taken from:

originalAmount: transactionOriginalAmount,
originalCurrency: transactionOriginalCurrency,
cardID: transactionCardID,
} = ReportUtils.getTransactionDetails(transaction) ?? {};

  1. Then, we need to move out the following out of the isCardTransaction block:
    if (formattedOriginalAmount) {
    amountDescription += ` • ${translate('iou.original')} ${formattedOriginalAmount}`;
    }

We want to show the details on every transaction and not only cards

Note we should put the original currency and amount at the end :

} else if (report.isWaitingOnBankAccount) {
amountDescription += ` • ${translate('iou.pending')}`;
}
}

This is better by design ;)

  1. For MoneyRequestPreviewContent too we can follow the same steps to get transaction details and add the original amount at the end after before we return the message:

} else if (isOnHold) {
message += ` • ${translate('iou.hold')}`;
}
return message;

What alternative solutions did you explore? (Optional)

N/A

Note

Currently the BE doesn't return any value for originalAmount and originalCurrency for any transaction other then the card one:

image

@abdulrahuman5196
Copy link
Contributor

@puneetlath I doubt if we return the original currency and amount in this case. Same was pointed out by multiple contributors.
Shouldn't we make BE return first before working on this?

Tagging you since you had reported. Do tag others if required.

@garrettmknight
Copy link
Contributor

I wasn't sure if this could be worked on by External first so I set the label. I'll update to internal for the moment and we can flip back to External when we're ready to complete the front end.

@garrettmknight garrettmknight added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Mar 26, 2024
Copy link

melvin-bot bot commented Mar 26, 2024

Current assignee @abdulrahuman5196 is eligible for the Internal assigner, not assigning anyone new.

@melvin-bot melvin-bot bot removed the Overdue label Aug 5, 2024
@melvin-bot melvin-bot bot added the Overdue label Aug 14, 2024
@garrettmknight
Copy link
Contributor

Still awaiting a volunteer

@melvin-bot melvin-bot bot removed the Overdue label Aug 14, 2024
@melvin-bot melvin-bot bot added the Overdue label Aug 23, 2024
@garrettmknight
Copy link
Contributor

Still awaiting a volunteer

@garrettmknight
Copy link
Contributor

Still awaiting a volunteer.

@marcaaron marcaaron self-assigned this Sep 6, 2024
@marcaaron
Copy link
Contributor

Looking into the backend change now.

@marcaaron
Copy link
Contributor

FWIW, this is what I'm seeing on a USD policy with a GBP expense. For a 100 GBP expense we're showing Pay $100.00 elsewhere and $136 (which I'd maybe assume is the correct conversion).

2024-09-05_17-49-43

But then see this...

2024-09-05_17-52-23

I'm seeing us show USD (policy currency) with a possible conversion to USD from GBP happening. Then I see us use the policy currency with the unconverted amount in other places. And a "Company Spend" breakdown that seems like it shouldn't even be there at all (or maybe I'm not understanding why that's showing up in this case).

I can't reproduce this with a manual request. It looks broken or possibly unimplemented. Are we following a Design Doc for this one?

@marcaaron
Copy link
Contributor

AFAICT we are returning everything we would need in the transaction object. Can we try to figure this out by using the transaction object instead of the reportAction?

@FitseTLT
Copy link
Contributor

FitseTLT commented Sep 6, 2024

AFAICT we are returning everything we would need in the transaction object. Can we try to figure this out by using the transaction object instead of the reportAction?

For multiple transactions created with different currencies the iou report will return the total in workspace currency but the transaction doesn't have the amount in the policy currency. The reason you were confused in ^ case is because you only created one transaction in an iou report you are seeing the converted total amount from the iou report, which in this case is equal to the transaction amount as there is only one transaction.

@marcaaron
Copy link
Contributor

The reason you were confused in ^ case is because you only created one transaction in an iou report you are seeing the converted total amount from the iou report, which in this case is equal to the transaction amount as there is only one transaction.

Ok, but the amount shows with a $ next to it? It's confusing on several levels 😄

@melvin-bot melvin-bot bot added the Overdue label Sep 16, 2024
@garrettmknight
Copy link
Contributor

@marcaaron you gonna take this one on?

@melvin-bot melvin-bot bot removed the Overdue label Sep 16, 2024
@marcaaron
Copy link
Contributor

Actually, sorry, I forgot I assigned myself and got busy with other issues. Gonna remove myself so I won't be a blocker here. I'm admittedly having some difficulty in understanding the scope of what this issue is asking for.

@marcaaron marcaaron removed their assignment Sep 17, 2024
@garrettmknight
Copy link
Contributor

Dang, scared him off. 👹

@garrettmknight
Copy link
Contributor

Still awaiting a volunteer.

@melvin-bot melvin-bot bot added the Overdue label Oct 1, 2024
@garrettmknight
Copy link
Contributor

Still awaiting a volunteer.

@melvin-bot melvin-bot bot removed the Overdue label Oct 2, 2024
@garrettmknight
Copy link
Contributor

Still awaiting a volunteer.

1 similar comment
@garrettmknight
Copy link
Contributor

Still awaiting a volunteer.

@garrettmknight
Copy link
Contributor

Still awaiting a volunteer.

I'm going to close this until we have a request from a migration customer.

@github-project-automation github-project-automation bot moved this from Hot Picks to Done in [#whatsnext] #expense Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Requires API changes or must be handled by Expensify staff NewFeature Something to build that is a new item. Weekly KSv2
Projects
Status: Done
Development

No branches or pull requests

7 participants