Skip to content
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

Merged
merged 34 commits into from
Oct 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
b9d9f6b
Use transaction distanceUnit to prevent retroactive changes
neil-marcellini Oct 1, 2024
3cedd46
Display rate with transaction's distance unit
neil-marcellini Oct 1, 2024
d6c88de
Prompt with both units for when transaction unit doesn't match policy
neil-marcellini Oct 1, 2024
778dd23
WIP optimistically set distanceUnit
neil-marcellini Oct 1, 2024
540f11a
Pass existing transaction in all distance cases
neil-marcellini Oct 2, 2024
3961d4c
Optimistically update to current distance unit when editing
neil-marcellini Oct 2, 2024
4520fe3
Prettify
neil-marcellini Oct 2, 2024
7579b73
Merge branch 'main' into neil-distanceUnit
neil-marcellini Oct 7, 2024
9936790
Remove unused types
neil-marcellini Oct 7, 2024
56c96fb
Missed a spot
neil-marcellini Oct 7, 2024
ebff76e
Merge branch 'main' into neil-distanceUnit
neil-marcellini Oct 9, 2024
9a06cfe
WIP use util to get transaction distance rate
neil-marcellini Oct 9, 2024
7982c26
Util to get updated distance unit
neil-marcellini Oct 9, 2024
5c43231
Use get rate util in another spot
neil-marcellini Oct 9, 2024
e07a837
Simplify using lodashSet
neil-marcellini Oct 9, 2024
18657fd
WIP convert distance to new rate unit
neil-marcellini Oct 9, 2024
1703edb
Refactor and fix getting updated distance unit
neil-marcellini Oct 9, 2024
466990b
Fix distance conversion with parenthesis
neil-marcellini Oct 9, 2024
da47219
Set distance pending when converted to new rate unit
neil-marcellini Oct 9, 2024
8f10f77
Clear pendingFields that aren't part of transactionChanges
neil-marcellini Oct 9, 2024
5bd5c3a
Fix lint
neil-marcellini Oct 9, 2024
a307c8d
Fix getting mileageRate.rate
neil-marcellini Oct 9, 2024
99e9170
Merge branch 'main' into neil-distanceUnit
neil-marcellini Oct 11, 2024
aded5c2
Ensure currency is always set
neil-marcellini Oct 11, 2024
7a9d0ea
Use stored P2P rate for existing transactions
neil-marcellini Oct 11, 2024
1e87ca3
Explain why the update distance unit should come from the rate
neil-marcellini Oct 11, 2024
ce80ef1
Apply feedback to clean things up
neil-marcellini Oct 11, 2024
817266b
Merge branch 'main' into neil-distanceUnit
neil-marcellini Oct 15, 2024
a2f0074
Fix some ts errors
neil-marcellini Oct 15, 2024
00eeb4a
Fix if somehow mileageRate is undefined
neil-marcellini Oct 15, 2024
5b12592
Merge branch 'main' into neil-distanceUnit
neil-marcellini Oct 16, 2024
01e652a
Ensure policy is defined for optimistic distance unit
neil-marcellini Oct 16, 2024
611ad8b
Fix ts, missing param
neil-marcellini Oct 16, 2024
f0ec762
Merge branch 'main' into neil-distanceUnit
neil-marcellini Oct 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 4 additions & 10 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,11 @@ import Config from 'react-native-config';
import * as KeyCommand from 'react-native-key-command';
import type {ValueOf} from 'type-fest';
import type {Video} from './libs/actions/Report';
import type {MileageRate} from './libs/DistanceRequestUtils';
import BankAccount from './libs/models/BankAccount';
import * as Url from './libs/Url';
import SCREENS from './SCREENS';
import type PlaidBankAccount from './types/onyx/PlaidBankAccount';
import type {Unit} from './types/onyx/Policy';

type RateAndUnit = {
unit: Unit;
rate: number;
currency: string;
};
type CurrencyDefaultMileageRate = Record<string, RateAndUnit>;

// Creating a default array and object this way because objects ({}) and arrays ([]) are not stable types.
// Freezing the array ensures that it cannot be unintentionally modified.
Expand Down Expand Up @@ -2466,6 +2459,8 @@ const CONST = {
DEFAULT_RATE: 'Default Rate',
RATE_DECIMALS: 3,
FAKE_P2P_ID: '_FAKE_P2P_ID_',
MILES_TO_KILOMETERS: 1.609344,
KILOMETERS_TO_MILES: 0.621371,
},

TERMS: {
Expand Down Expand Up @@ -5525,7 +5520,7 @@ const CONST = {
"rate": 2377,
"unit": "km"
}
}`) as CurrencyDefaultMileageRate,
}`) as Record<string, MileageRate>,

EXIT_SURVEY: {
REASONS: {
Expand Down Expand Up @@ -5919,7 +5914,6 @@ export type {
Country,
IOUAction,
IOUType,
RateAndUnit,
OnboardingPurposeType,
OnboardingCompanySizeType,
IOURequestType,
Expand Down
27 changes: 5 additions & 22 deletions src/components/MoneyRequestConfirmationList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {useFocusEffect, useIsFocused} from '@react-navigation/native';
import lodashIsEqual from 'lodash/isEqual';
import React, {memo, useCallback, useEffect, useMemo, useRef, useState} from 'react';
import {InteractionManager, View} from 'react-native';
import {useOnyx, withOnyx} from 'react-native-onyx';
import {withOnyx} from 'react-native-onyx';
import type {OnyxEntry} from 'react-native-onyx';
import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails';
import useDebouncedState from '@hooks/useDebouncedState';
Expand Down Expand Up @@ -37,7 +37,6 @@ import type * as OnyxTypes from '@src/types/onyx';
import type {Participant} from '@src/types/onyx/IOU';
import type {PaymentMethodType} from '@src/types/onyx/OriginalMessage';
import type {SplitShares} from '@src/types/onyx/Transaction';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import ButtonWithDropdownMenu from './ButtonWithDropdownMenu';
import type {DropdownOption} from './ButtonWithDropdownMenu/types';
import FormHelpMessage from './FormHelpMessage';
Expand Down Expand Up @@ -66,9 +65,6 @@ type MoneyRequestConfirmationListOnyxProps = {
/** The draft policy of the report */
policyDraft: OnyxEntry<OnyxTypes.Policy>;

/** Unit and rate used for if the expense is a distance expense */
mileageRates: OnyxEntry<Record<string, MileageRate>>;

/** Mileage rate default for the policy */
defaultMileageRate: OnyxEntry<MileageRate>;

Expand Down Expand Up @@ -181,7 +177,6 @@ function MoneyRequestConfirmationList({
iouAmount,
policyCategories: policyCategoriesReal,
policyCategoriesDraft,
mileageRates: mileageRatesReal,
isDistanceRequest = false,
policy: policyReal,
policyDraft,
Expand Down Expand Up @@ -216,10 +211,6 @@ function MoneyRequestConfirmationList({
}: MoneyRequestConfirmationListProps) {
const policy = policyReal ?? policyDraft;
const policyCategories = policyCategoriesReal ?? policyCategoriesDraft;
const [mileageRatesDraft] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_DRAFTS}${policyID || '-1'}`, {
selector: (selectedPolicy: OnyxEntry<OnyxTypes.Policy>) => DistanceRequestUtils.getMileageRates(selectedPolicy),
});
const mileageRates = isEmptyObject(mileageRatesReal) ? mileageRatesDraft : mileageRatesReal;

const styles = useThemeStyles();
const {translate, toLocaleDigit} = useLocalize();
Expand Down Expand Up @@ -247,15 +238,11 @@ function MoneyRequestConfirmationList({
IOU.setCustomUnitRateID(transactionID, rateID);
}, [defaultMileageRate, customUnitRateID, lastSelectedDistanceRates, policy?.id, canUseP2PDistanceRequests, transactionID, isDistanceRequest]);

const policyCurrency = policy?.outputCurrency ?? PolicyUtils.getPersonalPolicy()?.outputCurrency ?? CONST.CURRENCY.USD;

const mileageRate = TransactionUtils.isCustomUnitRateIDForP2P(transaction) ? DistanceRequestUtils.getRateForP2P(policyCurrency) : mileageRates?.[customUnitRateID] ?? defaultMileageRate;

const {unit, rate} = mileageRate ?? {};

const mileageRate = DistanceRequestUtils.getRate({transaction, policy, policyDraft});
const rate = mileageRate.rate;
const prevRate = usePrevious(rate);

const currency = (mileageRate as MileageRate)?.currency ?? policyCurrency;
const unit = mileageRate.unit;
const currency = mileageRate.currency ?? CONST.CURRENCY.USD;
neil-marcellini marked this conversation as resolved.
Show resolved Hide resolved
const prevCurrency = usePrevious(currency);

// A flag for showing the categories field
Expand Down Expand Up @@ -990,10 +977,6 @@ export default withOnyx<MoneyRequestConfirmationListProps, MoneyRequestConfirmat
key: ({policyID}) => `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
selector: DistanceRequestUtils.getDefaultMileageRate,
},
mileageRates: {
key: ({policyID}) => `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
selector: (policy: OnyxEntry<OnyxTypes.Policy>) => DistanceRequestUtils.getMileageRates(policy),
},
policy: {
key: ({policyID}) => `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
},
Expand Down
12 changes: 1 addition & 11 deletions src/components/ReportActionItem/MoneyRequestView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,6 @@ function MoneyRequestView({report, shouldShowAnimatedBackground, readonly = fals
const [parentReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${parentReportID}`, {
canEvict: false,
});
const [distanceRates = {}] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, {
selector: () => DistanceRequestUtils.getMileageRates(policy, true),
});
const [transactionViolations] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${getTransactionID(report, parentReportActions)}`);

const parentReportAction = parentReportActions?.[report?.parentReportActionID ?? '-1'];
Expand Down Expand Up @@ -202,14 +199,7 @@ function MoneyRequestView({report, shouldShowAnimatedBackground, readonly = fals
let amountDescription = `${translate('iou.amount')}`;

const hasRoute = TransactionUtils.hasRoute(transactionBackup ?? transaction, isDistanceRequest);
const rateID = TransactionUtils.getRateID(transaction) ?? '-1';

const currency = transactionCurrency ?? CONST.CURRENCY.USD;

const mileageRate = TransactionUtils.isCustomUnitRateIDForP2P(transaction) ? DistanceRequestUtils.getRateForP2P(currency) : distanceRates[rateID] ?? {};
const {unit} = mileageRate;
const rate = transaction?.comment?.customUnit?.defaultP2PRate ?? mileageRate.rate;
neil-marcellini marked this conversation as resolved.
Show resolved Hide resolved

const {unit, rate, currency} = DistanceRequestUtils.getRate({transaction, policy});
const distance = TransactionUtils.getDistanceInMeters(transactionBackup ?? transaction, unit);
const rateToDisplay = DistanceRequestUtils.getRateForDisplay(unit, rate, currency, translate, toLocaleDigit, isOffline);
const distanceToDisplay = DistanceRequestUtils.getDistanceForDisplay(hasRoute, distance, unit, rate, translate);
Expand Down
72 changes: 67 additions & 5 deletions src/libs/DistanceRequestUtils.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
import type {OnyxEntry} from 'react-native-onyx';
import Onyx from 'react-native-onyx';
import type {LocaleContextProps} from '@components/LocaleContextProvider';
import type {RateAndUnit} from '@src/CONST';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {LastSelectedDistanceRates, OnyxInputOrEntry} from '@src/types/onyx';
import type {LastSelectedDistanceRates, OnyxInputOrEntry, Transaction} from '@src/types/onyx';
import type {Unit} from '@src/types/onyx/Policy';
import type Policy from '@src/types/onyx/Policy';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import * as CurrencyUtils from './CurrencyUtils';
import * as PolicyUtils from './PolicyUtils';
import * as ReportConnection from './ReportConnection';
import * as ReportUtils from './ReportUtils';
import * as TransactionUtils from './TransactionUtils';

type MileageRate = {
customUnitRateID?: string;
Expand Down Expand Up @@ -219,17 +219,30 @@ function getDistanceMerchant(
return `${distanceInUnits} @ ${ratePerUnit}`;
}

function ensureRateDefined(rate: number | undefined): asserts rate is number {
if (rate !== undefined) {
return;
}
throw new Error('All default P2P rates should have a rate defined');
}

/**
* Retrieves the rate and unit for a P2P distance expense for a given currency.
*
* @param currency
* @returns The rate and unit in RateAndUnit object.
* @returns The rate and unit in MileageRate object.
*/
function getRateForP2P(currency: string): RateAndUnit {
function getRateForP2P(currency: string, transaction: OnyxEntry<Transaction>): MileageRate {
const currencyWithExistingRate = CONST.CURRENCY_TO_DEFAULT_MILEAGE_RATE[currency] ? currency : CONST.CURRENCY.USD;
const mileageRate = CONST.CURRENCY_TO_DEFAULT_MILEAGE_RATE[currencyWithExistingRate];
ensureRateDefined(mileageRate.rate);

// Ensure the rate is updated when the currency changes, otherwise use the stored rate
const rate = TransactionUtils.getCurrency(transaction) === currency ? transaction?.comment?.customUnit?.defaultP2PRate ?? mileageRate.rate : mileageRate.rate;
return {
...CONST.CURRENCY_TO_DEFAULT_MILEAGE_RATE[currencyWithExistingRate],
...mileageRate,
currency: currencyWithExistingRate,
rate,
};
}

Expand Down Expand Up @@ -301,6 +314,52 @@ function getTaxableAmount(policy: OnyxEntry<Policy>, customUnitRateID: string, d
return amount * taxClaimablePercentage;
}

function getDistanceUnit(transaction: OnyxEntry<Transaction>, mileageRate: OnyxEntry<MileageRate>): Unit {
return transaction?.comment?.customUnit?.distanceUnit ?? mileageRate?.unit ?? CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES;
}

/**
* Get the selected rate for a transaction, from the policy or P2P default rate.
* Use the distanceUnit stored on the transaction by default to prevent policy changes modifying existing transactions. Otherwise, get the unit from the rate.
*/
function getRate({
transaction,
policy,
policyDraft,
useTransactionDistanceUnit = true,
}: {
transaction: OnyxEntry<Transaction>;
policy: OnyxEntry<Policy>;
policyDraft?: OnyxEntry<Policy>;
useTransactionDistanceUnit?: boolean;
}): MileageRate {
let mileageRates = getMileageRates(policy, true, transaction?.comment?.customUnit?.customUnitRateID);
if (isEmptyObject(mileageRates) && policyDraft) {
mileageRates = getMileageRates(policyDraft, true, transaction?.comment?.customUnit?.customUnitRateID);
}
const policyCurrency = policy?.outputCurrency ?? PolicyUtils.getPersonalPolicy()?.outputCurrency ?? CONST.CURRENCY.USD;
const defaultMileageRate = getDefaultMileageRate(policy);
const customUnitRateID = TransactionUtils.getRateID(transaction) ?? '';
const mileageRate = TransactionUtils.isCustomUnitRateIDForP2P(transaction) ? getRateForP2P(policyCurrency, transaction) : mileageRates?.[customUnitRateID] ?? defaultMileageRate;
const unit = getDistanceUnit(useTransactionDistanceUnit ? transaction : undefined, mileageRate);
return {
...mileageRate,
unit,
currency: mileageRate?.currency ?? policyCurrency,
};
}

/**
* Get the updated distance unit from the selected rate instead of the distanceUnit stored on the transaction.
* Useful for updating the transaction distance unit when the distance or rate changes.
*
* For example, if an expense is '10 mi @ $1.00 / mi' and the rate is updated to '$1.00 / km',
* then the updated distance unit should be 'km' from the updated rate, not 'mi' from the currently stored transaction distance unit.
*/
function getUpdatedDistanceUnit({transaction, policy, policyDraft}: {transaction: OnyxEntry<Transaction>; policy: OnyxEntry<Policy>; policyDraft?: OnyxEntry<Policy>}) {
return getRate({transaction, policy, policyDraft, useTransactionDistanceUnit: false}).unit;
}
neil-marcellini marked this conversation as resolved.
Show resolved Hide resolved

export default {
getDefaultMileageRate,
getDistanceMerchant,
Expand All @@ -312,6 +371,9 @@ export default {
getCustomUnitRateID,
convertToDistanceInMeters,
getTaxableAmount,
getDistanceUnit,
getUpdatedDistanceUnit,
getRate,
};

export type {MileageRate};
38 changes: 29 additions & 9 deletions src/libs/TransactionUtils/index.ts
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';
Expand Down Expand Up @@ -139,6 +140,8 @@ 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,
Copy link
Contributor

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 different existingTransactionID and existingTransaction; 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 transaction

policy: OnyxEntry<Policy> = undefined,
): 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)
Expand All @@ -152,6 +155,12 @@ function buildOptimisticTransaction(
commentJSON.originalTransactionID = originalTransactionID;
}

const isDistanceTransaction = !!pendingFields?.waypoints;
if (isDistanceTransaction) {
// Set the distance unit, which comes from the policy distance unit or the P2P rate data
lodashSet(commentJSON, 'customUnit.distanceUnit', DistanceRequestUtils.getUpdatedDistanceUnit({transaction: existingTransaction, policy}));
}

return {
...(!isEmptyObject(pendingFields) ? {pendingFields} : {}),
transactionID,
Expand Down Expand Up @@ -261,15 +270,26 @@ function getUpdatedTransaction(transaction: Transaction, transactionChanges: Tra
}

if (Object.hasOwn(transactionChanges, 'customUnitRateID')) {
updatedTransaction.comment = {
...(updatedTransaction?.comment ?? {}),
customUnit: {
...updatedTransaction?.comment?.customUnit,
customUnitRateID: transactionChanges.customUnitRateID,
defaultP2PRate: null,
},
};
lodashSet(updatedTransaction, 'comment.customUnit.customUnitRateID', transactionChanges.customUnitRateID);
lodashSet(updatedTransaction, 'comment.customUnit.defaultP2PRate', null);
shouldStopSmartscan = true;

const existingDistanceUnit = transaction?.comment?.customUnit?.distanceUnit;
const allReports = ReportConnection.getAllReports();
const report = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${transaction.reportID}`] ?? null;
const policyID = report?.policyID ?? '';
const policy = PolicyUtils.getPolicy(policyID);
neil-marcellini marked this conversation as resolved.
Show resolved Hide resolved

// Get the new distance unit from the rate's unit
const newDistanceUnit = DistanceRequestUtils.getUpdatedDistanceUnit({transaction: updatedTransaction, policy});

// If the distanceUnit is set and the rate is changed to one that has a different unit, convert the distance to the new unit
if (existingDistanceUnit && newDistanceUnit !== existingDistanceUnit) {
const conversionFactor = existingDistanceUnit === CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES ? CONST.CUSTOM_UNITS.MILES_TO_KILOMETERS : CONST.CUSTOM_UNITS.KILOMETERS_TO_MILES;
const distance = NumberUtils.roundToTwoDecimalPlaces((transaction?.comment?.customUnit?.quantity ?? 0) * conversionFactor);
lodashSet(updatedTransaction, 'comment.customUnit.quantity', distance);
lodashSet(updatedTransaction, 'pendingFields.waypoints', CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE);
}
}

if (Object.hasOwn(transactionChanges, 'taxAmount') && typeof transactionChanges.taxAmount === 'number') {
Expand Down Expand Up @@ -823,7 +843,7 @@ function calculateAmountForUpdatedWaypointOrRate(
const mileageRates = DistanceRequestUtils.getMileageRates(policy, true);
const policyCurrency = policy?.outputCurrency ?? PolicyUtils.getPersonalPolicy()?.outputCurrency ?? CONST.CURRENCY.USD;
const mileageRate = isCustomUnitRateIDForP2P(transaction)
? DistanceRequestUtils.getRateForP2P(policyCurrency)
? DistanceRequestUtils.getRateForP2P(policyCurrency, transaction ?? undefined)
: mileageRates?.[customUnitRateID] ?? DistanceRequestUtils.getDefaultMileageRate(policy);
const {unit, rate, currency} = mileageRate;

Expand Down
Loading
Loading