Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Use transaction distanceUnit to prevent retroactive changes #50001
Use transaction distanceUnit to prevent retroactive changes #50001
Changes from all commits
b9d9f6b
3cedd46
d6c88de
778dd23
540f11a
3961d4c
4520fe3
7579b73
9936790
56c96fb
ebff76e
9a06cfe
7982c26
5c43231
e07a837
18657fd
1703edb
466990b
da47219
8f10f77
5bd5c3a
a307c8d
99e9170
aded5c2
7a9d0ea
1e87ca3
ce80ef1
817266b
a2f0074
00eeb4a
5b12592
01e652a
611ad8b
f0ec762
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Seems like this change re-introduced a bug we fixed in #50142.
Any objections to reverting to using this line instead of
currency
fromDistanceRequestUtils.getRate({transaction, policy})
?@neil-marcellini @s77rt
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.
Bug:
Expected: distance rate on the submitted request shows the original currency.
Actual: distance rate shows the updated currency even though the amount is calculated in the original currency.
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 we need to move that logic to
DistanceRequestUtils.getRate
.Is there any case where we don't want to use the transaction currency and instead use the rate's currency? I think there is only one case, that is if we change the rate to a rate that uses a diff currency (same with unit
useTransactionDistanceUnit
)Do you know why the currency is displayed correctly on the rate change page?
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.
We display the transaction's currency for the selected rate:
App/src/pages/iou/request/step/IOURequestStepDistanceRate.tsx
Line 69 in ce01614
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.
Moving it into
DistanceRequestUtils.getRate
will complicate the function with an extra parameter and an extra if-else check. If you think it's worth it, I'll add auseTransactionCurrency
param.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.
Using
transactionCurrency
directly is fine. Having too many sources of truth is confusing and will always lead to such bugs. We probably need to cleanup this at a later time (or at least centralize it)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.
This function (
buildOptimisticTransaction
) now have two parameters that are similar in name but differentexistingTransactionID
andexistingTransaction
; existingTransaction could exist and at the same time existingTransactionID does not which is a little confusing because you'd think the later id is the coming from that transaction but it's not necessary the case as we can pass the draft transactionThere 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.
Coming from #51285 {BZ checklist}, setting pending action on waypoint caused the map preview to be blank when selecting distance rate with different unit. There more to RC see detailed Analysis in this Proposal #51285 (comment)