-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[HOLD for payment 2024-04-25] [LOW] [Splits] [$500] Split bill - Currency for split bill via Scan is USD instead of local currency #29709
Comments
Triggered auto assignment to @anmurali ( |
Job added to Upwork: https://www.upwork.com/jobs/~014b7a910d5416d07a |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif ( |
ProposalPlease 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: Lines 1294 to 1336 in 22874c7
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:
And then on the money request confirmation page, pass down the users local currency like so:
So that it reflects the local currency like in other areas |
ProposalPlease 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). Lines 1308 to 1319 in be44c93
In addition, the back end also sends us the currency as USD. What changes do you think we should make in order to solve the problem?I propose two solutions.
First, add a function startSplitBill(participants, currentUserLogin, currentUserAccountID, comment, receipt, existingSplitChatReportID = '', currency) { Next, pass the IOU.startSplitBill(
selectedParticipants,
props.currentUserPersonalDetails.login,
props.currentUserPersonalDetails.accountID,
trimmedComment,
receipt,
existingSplitChatReportID,
props.iou.currency // <- Added
); Finally, replace 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 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 USDexpensify-split-bill-changing-currency.movAnother 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 offlineexpensify-split-bill-solution-tested-offline.movOnce you go back online, the currency is reverted back to USD. Video of back end changing the currency back to USD when coming back onlineexpensify-split-bill-solution-coming-back-online.movWhat 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. |
@anmurali, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
@anmurali, @aimane-chnaif 6 days overdue. This is scarier than being forced to listen to Vogon poetry! |
Current assignee @aimane-chnaif is eligible for the External assigner, not assigning anyone new. |
@aimane-chnaif can you review proposals and pick one so we can proceed? |
As backend also sends the currency as USD for scan transaction, first wanna confirm if it's intentional. |
Triggered auto assignment to @marcochavezf, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@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! |
@marcochavezf, @anmurali, @aimane-chnaif Huh... This is 4 days overdue. Who can take care of this? |
Checking it out today |
Yes, @anmurali could you handle payment for @sicarius97 and @Victor-Nyagudi? since the solution used part of their proposals. Also, payment for @aimane-chnaif and @Santhosh-Sellavel for C+ review |
Bump @anmurali for payment so we can close this out. |
@marcochavezf, @anmurali, @Santhosh-Sellavel, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Payment summary
|
@marcochavezf, @anmurali, @Santhosh-Sellavel, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
@marcochavezf, @anmurali, @Santhosh-Sellavel, @aimane-chnaif 6 days overdue. This is scarier than being forced to listen to Vogon poetry! |
Not overdue.Still waiting on responses from @sicarius97 and @aimane-chnaif #29709 (comment) |
@marcochavezf, @anmurali, @Santhosh-Sellavel, @aimane-chnaif Huh... This is 4 days overdue. Who can take care of this? |
I accepted offer |
@marcochavezf, @anmurali, @Santhosh-Sellavel, @aimane-chnaif 6 days overdue. This is scarier than being forced to listen to Vogon poetry! |
Requested on ND |
@aimane-chnaif is paid |
@marcochavezf, @anmurali, @Santhosh-Sellavel, @aimane-chnaif Huh... This is 4 days overdue. Who can take care of this? |
@marcochavezf, @anmurali, @Santhosh-Sellavel, @aimane-chnaif 6 days overdue. This is scarier than being forced to listen to Vogon poetry! |
I think we can close this for now. @sicarius97 can ping here or slack when they're available. |
@marcochavezf, @anmurali, @Santhosh-Sellavel, @aimane-chnaif Still overdue 6 days?! Let's take care of this! |
$250 approved for @Santhosh-Sellavel |
@marcochavezf, @anmurali, @Santhosh-Sellavel, @aimane-chnaif 10 days overdue. I'm getting more depressed than Marvin. |
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: 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 #29664
Action Performed:
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
Bug6239337_1697475398336.29664_.mp4
MacOS: Desktop
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @anmuraliIssue Owner
Current Issue Owner: @marcochavezfThe text was updated successfully, but these errors were encountered: