Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.34k stars 2.77k forks source link

[HOLD #46753 #43588] [$250][P2P Distance] Rate: incorrect currency for p2p request (beta) #46844

Open m-natarajan opened 1 month ago

m-natarajan commented 1 month ago

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.16-5 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @paultsimura Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1722875886722249

Action Performed:

Pre-requisites: Have 2 accounts with different personal currencies. For this, on User A:

  1. Log in to OD
  2. Go to Settings > Workspaces > Individual
  3. Select your personal workspace
  4. Select Reports from the left menu
  5. Change the report currency to GBP For user B, keep the default currency (let's say, it's USD). Steps:
  6. Login as User A to ND
  7. Submit a Distance request to User B
  8. Open the created request details
  9. Verify it's amount and rate are displayed in GBP
  10. Login as User B to ND
  11. Open the said Distance request details

Expected Result:

The amount and rate should be in the original currency

Actual Result:

The amount is in the original currency (GBP), while the rate shows User B's currency (0.45zl instead of 0.45 GBP)

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

https://github.com/user-attachments/assets/fa7da7ec-8d9e-4f56-b936-891c69ce54ae

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01fb1bf8d2bf2fb7c7
  • Upwork Job ID: 1821378352686653817
  • Last Price Increase: 2024-08-15
  • Automatic offers:
    • paultsimura | Contributor | 103443192
Issue OwnerCurrent Issue Owner: @thesahindia
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @stephanieelliott (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.

nyomanjyotisa commented 1 month ago

Proposal

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

incorrect currency for p2p request

What is the root cause of that problem?

We get the currency from the personal policy output currency here https://github.com/Expensify/App/blob/0a178210f6ce87f30d8e819a08053d76c5cee632/src/components/ReportActionItem/MoneyRequestView.tsx#L231

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

Get the currency from the transaction data first

const currency = transaction?.currency ?? policy?.outputCurrency ?? PolicyUtils.getPersonalPolicy()?.outputCurrency ?? CONST.CURRENCY.USD;

What alternative solutions did you explore? (Optional)

Or just this:

const currency = transaction?.currency;

Or:

const currency = transaction?.currency ?? CONST.CURRENCY.USD;
paultsimura commented 1 month ago

I'd love to be C+ here as I've reported the bug and have been working tightly with the Distance expenses recently.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

📣 @paultsimura 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

stephanieelliott commented 1 month ago

Sure, all yours @paultsimura -- I've assigned you as C+!

thesahindia commented 1 month ago

Just a note: C+ can't request to be assigned because they have reported the issue (which was discussed). This is for fairness.

paultsimura commented 1 month ago

Sorry @thesahindia I was unaware of that discussion. Please take this issue back, it would be fair👍

thesahindia commented 1 month ago

No problem! I don't have any issue here. Feel free to take it.

I was just adding a note for awareness.

paultsimura commented 1 month ago

This issue should cover the mileage issue as well. What should we do when user A uses kilometers, and user B – miles?

a: Convert kilometers to miles and show the corresponding value in each user's mileage preference b: Show the original distance & mileage unit for both users

@stephanieelliott @neil-marcellini

neil-marcellini commented 1 month ago

Wow this is an interesting one. I think ideally each user will see it based on their own preferences. So if A is in EUR/km and B is in USD/mi, then both alway sees the values in their units.

However, I'm not sure if our system is going to support that very well right now. It would be helpful to know what happens with IOUs if two users create manual expenses with different currencies. We will probably want to follow that pattern.

@paultsimura would you please do that investigation and then make a post asking about the UX for this situation in #expensify-open-source? I will then share it with our internal team and we can all agree on what's best. It feels somewhat important and something we haven't considered much so I want to get a good amount of feedback.

paultsimura commented 1 month ago

The discussion is still in progress: https://expensify.slack.com/archives/C01GTK53T8Q/p1723152837593809

neil-marcellini commented 1 month ago

Assigning myself to help move that discussion along

melvin-bot[bot] commented 1 month ago

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

paultsimura commented 1 month ago

To sum up the discussion:

FitseTLT commented 1 month ago

Proposal

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

P2P Distance] Rate: incorrect currency for p2p request

What is the root cause of that problem?

We were displaying based on currency setting of the user's personal policy https://github.com/Expensify/App/blob/d4d5a2586910ff46147219ee3e98bb3e936f8037/src/components/ReportActionItem/MoneyRequestView.tsx#L240-L241

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

We should change it to

    const currency = policy ? policy.outputCurrency : transactionCurrency ?? CONST.CURRENCY.USD;

or

    const currency = policy ? policy.outputCurrency : transactionCurrency ?? PolicyUtils.getPersonalPolicy()?.outputCurrency ?? CONST.CURRENCY.USD;

What alternative solutions did you explore? (Optional)

paultsimura commented 1 month ago

Unfortunately, so far none of the proposals is good enough. Let's hold this on both https://github.com/Expensify/App/issues/46753 and https://github.com/Expensify/App/issues/43588 - there are pending BE changes.

melvin-bot[bot] commented 4 weeks ago

@paultsimura @neil-marcellini @stephanieelliott 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!

melvin-bot[bot] commented 4 weeks ago

@paultsimura, @neil-marcellini, @stephanieelliott Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

stephanieelliott commented 3 weeks ago

Sounds good @paultsimura -- adjusted title and labels for the hold

stephanieelliott commented 2 weeks ago

Still held for https://github.com/Expensify/App/issues/46753 and https://github.com/Expensify/App/issues/43588

neil-marcellini commented 1 week ago

FYI the conclusion from our last discussion is here. I think we should specifically focus this issue on the currencies displayed for the amount and rate. The unit can be handled with this issue https://github.com/Expensify/App/issues/43588#top. Also holding on it is still a good idea as the approach we use there can inform this one.

stephanieelliott commented 18 hours ago

Still held on https://github.com/Expensify/App/issues/43588