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.56k stars 2.9k forks source link

[HOLD for payment 2024-04-25] [LOW] [Splits] [$500] Split bill - Currency for split bill via Scan is USD instead of local currency #29709

Closed lanitochka17 closed 5 months ago

lanitochka17 commented 1 year 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: 1.3.84-7 Reproducible in staging?: Yes Reproducible in production?: Yes 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: Applause - Internal Team Slack conversation:

Issue found when executing PR https://github.com/Expensify/App/pull/29664

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to group chat > + > Split bill > Manual
  3. Note that the currency is the local currency
  4. Go to Scan > Upload a photo
  5. Select Split with a few users and split the bill
  6. Open the split preview and click Amount
  7. Create a Scan request (1:1)
  8. Go to Scan details page > Amount
  9. Note that the currency is also local currency

Expected Result:

In Step 6, when editing the amount for split bill, the currency should be in the local currency

Actual Result:

In Step 6, when editing the amount for split bill, the currency is USD. While the currency in Manual split bill (Step 2) and 1:1 Scan request (Step 8) follows the local currency

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari https://github.com/Expensify/App/assets/78819774/8193c984-2317-4ba0-ba79-e8f1218dc1c8
MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01eec1820774133ef4
  • Upwork Job ID: 1716906523802828800
  • Last Price Increase: 2023-10-24
Issue OwnerCurrent Issue Owner: @anmurali
Issue OwnerCurrent Issue Owner: @marcochavezf
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 1 year ago

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

sicarius97 commented 1 year ago

Proposal

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

USD instead of local currency shown when starting a split on a receipt

What is the root cause of that problem?

It looks to me like in the code below the currency is set to the constant USD on line 1310 & 1326 rather than the local currency being passed down in the startSplitBill function:

https://github.com/Expensify/App/blob/22874c719767a23aa795ae09a4030c4890c542e7/src/libs/actions/IOU.js#L1294-L1336

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

Instead of setting the currency to be a constant we could add a variable to the startSplitBill function like so:

function startSplitBill(participants, currentUserLogin, currentUserAccountID, comment, receipt, existingSplitChatReportID = '', currency = CONST.CURRENCY.USD) {

And then on the money request confirmation page, pass down the users local currency like so:

IOU.startSplitBill(
                        selectedParticipants,
                        props.currentUserPersonalDetails.login,
                        props.currentUserPersonalDetails.accountID,
                        trimmedComment,
                        receipt,
                        existingSplitChatReportID,
                        props.iou.currency
                    );

So that it reflects the local currency like in other areas

Victor-Nyagudi commented 1 year ago

Proposal

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

Split bill - Currency for split bill via Scan is USD instead of local currency

What is the root cause of that problem?

When splitting a bill, the optimistic transaction's currency is set to USD (line 1310).

https://github.com/Expensify/App/blob/be44c93037fe165d6e3aec7445044c371bcbc921/src/libs/actions/IOU.js#L1308-L1319

In addition, the back end also sends us the currency as USD.

Images of network request and split bill transaction ![expensify-RHP-split-bill-bug](https://github.com/Expensify/App/assets/79470910/d2cd7371-e646-455e-9704-33dc29d3163e) ![expensify-split-bill-bug-network-request](https://github.com/Expensify/App/assets/79470910/e12f02e5-94a9-4ee2-b81a-393fe8345fc8)

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

I propose two solutions.

First, add a currency parameter at the end of the startSplitBill() function.

function startSplitBill(participants, currentUserLogin, currentUserAccountID, comment, receipt, existingSplitChatReportID = '', currency) {

Next, pass the props.iou.currency to startSplitBill() method call in MoneyRequestConfirmPage, similar to what happens in a regular money request.

IOU.startSplitBill(
    selectedParticipants,
    props.currentUserPersonalDetails.login,
    props.currentUserPersonalDetails.accountID,
    trimmedComment,
    receipt,
    existingSplitChatReportID,
    props.iou.currency // <- Added
);

Finally, replace CONST.CURRENCY.USD with this currency in splitTransaction.

const splitTransaction = TransactionUtils.buildOptimisticTransaction(
    0,
    currency, // <- currency replaced
    CONST.REPORT.SPLIT_REPORTID,
    comment,
    '',
    '',
    '',
    CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT,
    receiptObject,
    filename,
);

This solution improves consistency with what happens in a regular money request.

Since we already have the current user's personal details, we can leverage this and replace CONST.CURRENCY.USD with currentUserPersonalDetails.localCurrencyCode.

const splitTransaction = TransactionUtils.buildOptimisticTransaction(
    0,
    currentUserPersonalDetails.localCurrencyCode, // <- currency replaced
    CONST.REPORT.SPLIT_REPORTID,
    comment,
    '',
    '',
    '',
    CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT,
    receiptObject,
    filename,
);

This second option has lesser changes.

Either of these solutions addresses the front end issue, but as you can see from the following video, the back end changes the currency back to USD, so a change will need to happen over there too.

Video of back end changing the currency back to USD https://github.com/Expensify/App/assets/79470910/2238cbe7-a79a-475f-8612-b9c4a77ba3a2

Another indicator of a back end issue is if you test either of my proposed solutions offline, the currency stays at the local one.

Video of solution tested while offline https://github.com/Expensify/App/assets/79470910/a18d2c6b-1f63-433e-a2e7-ba3a88052dcc

Once you go back online, the currency is reverted back to USD.

Video of back end changing the currency back to USD when coming back online https://github.com/Expensify/App/assets/79470910/91a3f68d-be1a-4640-8ba6-512adfc518e9

What alternative solutions did you explore? (Optional)

I thought about making one of my two proposed solutions an alternative, but I figured I'd propose them together since the difference between them isn't that great.

melvin-bot[bot] commented 1 year ago

@anmurali, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 1 year ago

@anmurali, @aimane-chnaif 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

melvin-bot[bot] commented 1 year ago

Current assignee @aimane-chnaif is eligible for the External assigner, not assigning anyone new.

anmurali commented 1 year ago

@aimane-chnaif can you review proposals and pick one so we can proceed?

aimane-chnaif commented 1 year ago

As backend also sends the currency as USD for scan transaction, first wanna confirm if it's intentional. 🎀 👀 🎀

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

@marcochavezf @anmurali @aimane-chnaif 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 1 year ago

@marcochavezf, @anmurali, @aimane-chnaif Huh... This is 4 days overdue. Who can take care of this?

marcochavezf commented 1 year ago

Checking it out today

marcochavezf commented 1 year ago

Oops I've been swamped with some higher priorities.

melvin-bot[bot] commented 1 year ago

@marcochavezf @anmurali @aimane-chnaif this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 1 year ago

@marcochavezf, @anmurali, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

marcochavezf commented 1 year ago

No update

marcochavezf commented 1 year ago

No update, focused on other higher priority items atm

melvin-bot[bot] commented 1 year ago

@marcochavezf @anmurali @aimane-chnaif this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 1 year ago

Current assignee @aimane-chnaif is eligible for the Internal assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

@marcochavezf, @anmurali, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 year ago

@marcochavezf @anmurali @aimane-chnaif this issue is now 4 weeks old and preventing us from maintaining WAQ. This should now be your highest priority. Please post below what your plan is to get a PR in review ASAP. Thanks!

melvin-bot[bot] commented 1 year ago

@marcochavezf, @anmurali, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 1 year ago

@marcochavezf @anmurali @aimane-chnaif this issue is now 4 weeks old and preventing us from maintaining WAQ. This should now be your highest priority. Please post below what your plan is to get a PR in review ASAP. Thanks!

marcochavezf commented 1 year ago

No update

marcochavezf commented 1 year ago

No update, still addressing other daily items

marcochavezf commented 1 year ago

No update

marcochavezf commented 12 months ago

No update

marcochavezf commented 12 months ago

Moving this to weekly to focus on wave7

marcochavezf commented 11 months ago

No update, still focused on critical items for wave7

marcochavezf commented 11 months ago

No update

marcochavezf commented 11 months ago

No update

marcochavezf commented 10 months ago

No update

marcochavezf commented 10 months ago

Focused on wave issues, no update

marcochavezf commented 10 months ago

No update, probably this GH can probably be part of VIP project

marcochavezf commented 9 months ago

No update, focused on wave work

marcochavezf commented 9 months ago

No update

anmurali commented 9 months ago

Adding BZ backup but this is still lower priority than waves so makes sense

melvin-bot[bot] commented 9 months ago

Triggered auto assignment to @laurenreidexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

marcochavezf commented 9 months ago

Still focused on wave work. I'm going to move it to weekly meanwhile

marcochavezf commented 8 months ago

No update, focused on critical and high-priority wave tasks

marcochavezf commented 8 months ago

No update

marcochavezf commented 8 months ago

No update, focused on higher priority tasks/bugs

marcochavezf commented 7 months ago

I checked it out, and yes, we need a backend fix for this first. I created a few PRs to update the currency and quickly created a frontend PR to test the changes. The frontend PR includes part of the proposals from @sicarius97 and @Victor-Nyagudi, so we can split the compensation after the PR is deployed to production.

Santhosh-Sellavel commented 7 months ago

@marcochavezf please Assign me this issue for tracking payment

melvin-bot[bot] commented 7 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 7 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.62-17 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-04-25. :confetti_ball:

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 7 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed: