-
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
Use transaction distanceUnit to prevent retroactive changes #50001
Changes from 10 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
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,7 +108,6 @@ import type { | |
PayerSettledParams, | ||
PaySomeoneParams, | ||
ReconciliationWorksParams, | ||
ReimbursementRateParams, | ||
RemovedFromApprovalWorkflowParams, | ||
RemovedTheRequestParams, | ||
RemoveMemberPromptParams, | ||
|
@@ -1010,7 +1009,7 @@ const translations = { | |
changed: 'changed', | ||
removed: 'removed', | ||
transactionPending: 'Transaction pending.', | ||
chooseARate: ({unit}: ReimbursementRateParams) => `Select a workspace reimbursement rate per ${unit}`, | ||
chooseARate: 'Select a workspace reimbursement rate per mile or kilometer', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This works better when the selected rate is in a different unit than the current policy distance unit. I think it works fine when the unit is all the same too. |
||
unapprove: 'Unapprove', | ||
unapproveReport: 'Unapprove report', | ||
headsUp: 'Heads up!', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import lodashHas from 'lodash/has'; | ||
import lodashIsEqual from 'lodash/isEqual'; | ||
import lodashSet from 'lodash/set'; | ||
import type {OnyxCollection, OnyxEntry} from 'react-native-onyx'; | ||
import Onyx from 'react-native-onyx'; | ||
import type {ValueOf} from 'type-fest'; | ||
|
@@ -139,6 +140,7 @@ function buildOptimisticTransaction( | |
billable = false, | ||
pendingFields: Partial<{[K in TransactionPendingFieldsKey]: ValueOf<typeof CONST.RED_BRICK_ROAD_PENDING_ACTION>}> | undefined = undefined, | ||
reimbursable = true, | ||
existingTransaction: OnyxEntry<Transaction> | undefined = undefined, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function ( |
||
): Transaction { | ||
// transactionIDs are random, positive, 64-bit numeric strings. | ||
// Because JS can only handle 53-bit numbers, transactionIDs are strings in the front-end (just like reportActionID) | ||
|
@@ -152,6 +154,21 @@ function buildOptimisticTransaction( | |
commentJSON.originalTransactionID = originalTransactionID; | ||
} | ||
|
||
const isDistanceTransaction = (pendingFields?.waypoints ?? '') !== ''; | ||
neil-marcellini marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (isDistanceTransaction) { | ||
// Get the policy of this transaction from the report.policyID | ||
const allReports = ReportConnection.getAllReports(); | ||
const report = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`] ?? null; | ||
const policyID = report?.policyID ?? ''; | ||
const policy = PolicyUtils.getPolicy(policyID); | ||
neil-marcellini marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Set the distance unit, which comes from the policy distance unit or the P2P rate data | ||
const distanceRates = DistanceRequestUtils.getMileageRates(policy, true); | ||
const customUnitRateID = existingTransaction?.comment?.customUnit?.customUnitRateID ?? CONST.CUSTOM_UNITS.FAKE_P2P_ID; | ||
const mileageRate = customUnitRateID === CONST.CUSTOM_UNITS.FAKE_P2P_ID ? DistanceRequestUtils.getRateForP2P(currency) : distanceRates[customUnitRateID] ?? {}; | ||
lodashSet(commentJSON, 'customUnit.distanceUnit', mileageRate.unit ?? CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES); | ||
} | ||
|
||
return { | ||
...(!isEmptyObject(pendingFields) ? {pendingFields} : {}), | ||
transactionID, | ||
|
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 have two sources of truth
mileageRate.unit
andunit
which could be confusing and error-prone. Can we have the returnedmileageRate
already have the correct unit. i.e. write a functionDistanceRequestUtils.getRate
and in that function pass both the policy and the transaction and set theunit
accordingly.Also I think we can remove the
isCustomUnitRateIDForP2P
,getRateForP2P
andgetCustomUnitRateID
(not sure about this one) functions or at least not export them. ThegetRate
function should be enoughThere 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.
That's a pretty good idea, thanks. I'll work on 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.
I ended up adding two new util functions for getting the rate of existing transactions and then getting the distance unit for use when updating a transaction.
I left one use of these other utils you recommended removing, because making the current code work with the new utils would require creating a temporary transaction, setting the customUnitRateID from the transaction changes into it, then using one util for the rate and another for the unit. It's just as messy as the existing code. Not to mention that I would also need to change a lot of the OnyxEntryOrInput types to work with the OnyxEntry types. So I'm going to leave that as is.
App/src/libs/TransactionUtils/index.ts
Lines 807 to 809 in 7982c26
Please let me know what you think in your next review.