-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[P2P Distance] Rate currency doesn't match expense currency #50142
Conversation
Hi @paultsimura I can't create a p2p distance request to test this changes, have override the
Recording: -1-New-Expensify.27.mp4Any thoughts on this? |
@nyomanjyotisa this is weird 🤔 |
Reviewer Checklist
Screenshots/VideosAndroid: Native2024-10-03.-.19.11.-.android.mp4Android: mWeb Chrome2024-10-03.-.19.11.-.chrome.mp4iOS: Native2024-10-03.-.19.11.-.Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-10-03.at.19.00.31.mp4iOS: mWeb Safari2024-10-03.-.19.11.-.Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-10-03.at.19.01.19.mp4MacOS: Desktop2024-10-03.-.19.11.-.Screen.Recording.2024-10-03.at.18.57.38.mp4 |
@paultsimura Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@paultsimura ready for review, need to test it between 2 completely fresh accounts |
@nyomanjyotisa could you please change the testing steps in the description to these? Pre-requisites:
Test:
Note: Also, please remove the "Verify that no errors appear in the JS console" at the end – it's redundant here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ✔️
@neil-marcellini all yours. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use TransactionUtils.getCurrency instead since it accounts for modifiedCurrency.
Could we please also include a test step to click into the rate field and make sure the currencies show there properly? For example if I create a request with the policy report currency as USD and on a policy with several rates, then change the report currency to EUR, then go to edit the distance rate I think it will show the currently selected rate in EUR, when it should be USD, and all other rates should show as EUR. We need to get the rate currency for the currently selected rate from the transaction vs the policy.
You can check out this draft PR where I'm doing something similar for the distance unit.
This is exactly why I suggested using App/src/components/ReportActionItem/MoneyRequestView.tsx Lines 114 to 126 in 5e39861
Line 2958 in 60afa5e
|
@neil-marcellini, regarding your testing scenario – I think it already works as expected (please check the recording). The only thing is that when you click the "rate" field to modify it, one of them is highlighted (because the mapping is done by ID, which doesn't change) when it already shows a different currency in the selection list. However, I don't think it's quite related to this GH 🤔 2024-10-04.-.22.07.-.Screen.Recording.2024-10-04.at.22.06.36.mp4 |
@paultsimura Yes, so in the video you showed above the problem is that the currency of the rate shows in EUR on the rate editor page, but it shows in USD on the transaction view. Let's fix that by getting the rate currency from the transaction for the currently selected rate. I suppose it is a little confusing given that a rate in that currency doesn't exist on the policy, but I think that's less confusing than having a discrepancy between the transaction and the selected rate. The main goal is that existing distance expenses are not edited retroactively when the policy distance unit changes. If users complain or get confused then maybe we can include some informational text or re-think the pattern. Additionally, I think we should change the prompt text on the rate editor to be unit agnostic, like I did here. |
@nyomanjyotisa would you be able to handle this change request? |
I have created a commit to handle this change request, could you please review? @paultsimura -1-New-Expensify.28.mp4Also, I got this lint error |
Yes, this is a new requirement: https://expensify.slack.com/archives/C01GTK53T8Q/p1726169039084589 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes 🙏
@nyomanjyotisa please add another test to the PR description. |
PR description and test updated @paultsimura |
@nyomanjyotisa another bug:
Expected: the selected rate currency is changed to EUR
Expected: the request amount shows currency: EUR 2024-10-08.-.17.08.-.Screen.Recording.2024-10-08.at.16.57.44.mp4 |
@nyomanjyotisa regarding this – my guess is we should update the draft transaction's currency when the rate currency changes during the creation flow. |
@nyomanjyotisa a potential fix is to change this line by adding
const shouldCalculateDistanceAmount = isDistanceRequest && (iouAmount === 0 || prevRate !== rate || prevDistance !== distance || prevCurrency !== currency); |
@paultsimura Thanks, I'll try and test this out today |
Let's also hold for #50532 as we'll be changing |
The code looks good and tests well. We should hold for #50532 for the CI. However, it hasn't been C+ reviewed for five days now. WDYT @neil-marcellini? |
It would be fine for me to implement the change, @paultsimura. Should we implement it directly in this PR? |
I think we should wait for @neil-marcellini decision on #50142 (comment). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code looks fine and since C+ says it tests well we can go ahead and merge. We don't need to wait for the MoneyRequestConfirmationList refactor since that's a separate thing. I'll bump that one along too.
@nyomanjyotisa we need to fix merge conflicts though. Please message me on Slack when it's updated so I can merge quickly.
Conflict resolved @neil-marcellini |
Changed files check is being addressed with [$250] Migrate MoneyRequestConfirmationList to useOnyx and other issues. |
@neil-marcellini looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.0.50-0 🚀
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.0.50-8 🚀
|
Details
Fixed Issues
$ #46844
PROPOSAL: #46844 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Pre-requisites:
Have 2 accounts with different personal currencies.
For this:
Test 1:
Test 2:
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Test 1
User A
User B
Test 2
Android-Native.mp4
Android: mWeb Chrome
Test 1
User A
User B
Test 2
Android-mWeb.Chrome.mp4
iOS: Native
Test 1User A
User B
Test 2
iOS-Native.mp4
iOS: mWeb Safari
Test 1
User A
User B
Test 2
iOS-mWeb.Safari.mp4
MacOS: Chrome / Safari
Test 1
User A
User B
Test 2
MacOS-Chrome.mp4
MacOS: Desktop
Test 1
User A
User B
Test 2
MacOS-Desktop.mp4