Skip to content

Refactor: migrate getRate to use usePersonalPolicy hook (part 5) #95334

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.

Sign up for GitHub

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

Open
shubham1206agra wants to merge 2 commits into
base: main
Choose a base branch
from
+31 −7
Open

Refactor: migrate getRate to use usePersonalPolicy hook (part 5) #95334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90e1475
Refactor: migrate getRate to use usePersonalPolicy hook (part 5)
shubham1206agra Jul 5, 2026
a487870
Fix spell
shubham1206agra Jul 5, 2026
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
3 changes: 3 additions & 0 deletions src/components/AddExistingExpenseFooter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import useLocalize from '@hooks/useLocalize';
import useOnyx from '@hooks/useOnyx';
import usePermissions from '@hooks/usePermissions';
import usePersonalPolicy from '@hooks/usePersonalPolicy';
import useThemeStyles from '@hooks/useThemeStyles';
import useTransactionsByID from '@hooks/useTransactionsByID';

Expand Down Expand Up @@ -49,6 +50,7 @@ function AddExistingExpenseFooter({selectedIds, report, reportToConfirm, reportN
const isASAPSubmitBetaEnabled = isBetaEnabled(CONST.BETAS.ASAP_SUBMIT);
const session = useSession();
const personalDetails = usePersonalDetails();
const personalPolicy = usePersonalPolicy();
const [transactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS);
const [allReports] = useOnyx(ONYXKEYS.COLLECTION.REPORT);
const [policyRecentlyUsedCurrencies] = useOnyx(ONYXKEYS.RECENTLY_USED_CURRENCIES);
Expand Down Expand Up @@ -97,6 +99,7 @@ function AddExistingExpenseFooter({selectedIds, report, reportToConfirm, reportN
transactions,
allTransactionViolation: transactionViolations,
allReports,
personalPolicyOutputCurrency: personalPolicy?.outputCurrency,
});
}
},
Expand Down
1 change: 1 addition & 0 deletions src/hooks/useUndeleteTransactions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ function useUndeleteTransactions() {
transactions,
allTransactionViolation: transactionViolations,
allReports,
personalPolicyOutputCurrency: policy?.outputCurrency,

@ikevin127 ikevin127 Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Redundant personalPolicyOutputCurrency in two call sites (correctness‑neutral, but misleading)

In useUndeleteTransactions.ts and useReportSelectionActions.ts#removeFromReport, the policy passed is already the personal policy, so policy?.outputCurrency resolves first in getRate and personalPolicyOutputCurrency is dead.

// src/hooks/useUndeleteTransactions.ts:16-34
const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${personalPolicyID}`); // <- personal policy
changeTransactionsReport({
    ...
    policy,
    personalPolicyOutputCurrency: policy?.outputCurrency, // redundant: same object, resolved first at getRate:578
});
// useReportSelectionActions.ts:233-238 (removeFromReport)
policy: allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${personalPolicyID}`],
personalPolicyOutputCurrency: allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${personalPolicyID}`]?.outputCurrency, // same source as policy above

Why it matters: It's not a bug, but a future reader will assume personalPolicyOutputCurrency is distinct from policy here and may build on that false assumption. It also duplicates the collection lookup in the second case.

Consider a short comment (// policy is already the personal policy; passed for the getRate fallback contract) or dropping it where genuinely redundant. This is a nit — I would not block on it, but it's worth a one‑line clarification for the next maintainer.

});
};
}
Expand Down
4 changes: 3 additions & 1 deletion src/libs/actions/Transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -839,6 +839,7 @@ type ChangeTransactionsReportProps = {
transactions: Transaction[];
allTransactionViolation?: OnyxCollection<TransactionViolation[]>;
allReports: OnyxCollection<Report>;
personalPolicyOutputCurrency: string | undefined;
};

function changeTransactionsReport({
Expand All @@ -854,6 +855,7 @@ function changeTransactionsReport({
transactions,
allTransactionViolation = {},
allReports: allReportsParam,
personalPolicyOutputCurrency,
}: ChangeTransactionsReportProps) {
const reports = allReportsParam ?? allReports;
const reportID = newReport?.reportID ?? CONST.REPORT.UNREPORTED_REPORT_ID;
Expand Down Expand Up @@ -1231,7 +1233,7 @@ function changeTransactionsReport({
};

if (!isFetchingWaypointsFromServer(transaction)) {
const updatedMileageRate = DistanceRequestUtils.getRate({transaction: updatedTransaction, policy, useTransactionDistanceUnit: false});
const updatedMileageRate = DistanceRequestUtils.getRate({transaction: updatedTransaction, policy, useTransactionDistanceUnit: false, personalPolicyOutputCurrency});
const {unit, rate} = updatedMileageRate;
const distanceInMeters = getDistanceInMeters(updatedTransaction, unit);
const calculatedAmount = DistanceRequestUtils.getDistanceRequestAmount(distanceInMeters, unit, rate ?? 0);
Expand Down
3 changes: 3 additions & 0 deletions src/pages/DynamicNewReportWorkspaceSelectionPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import useLocalize from '@hooks/useLocalize';
import useNetwork from '@hooks/useNetwork';
import useOnyx from '@hooks/useOnyx';
import usePermissions from '@hooks/usePermissions';
import usePersonalPolicy from '@hooks/usePersonalPolicy';
import useResponsiveLayout from '@hooks/useResponsiveLayout';
import useThemeStyles from '@hooks/useThemeStyles';
import useTransactionsByID from '@hooks/useTransactionsByID';
Expand Down Expand Up @@ -84,6 +85,7 @@ function DynamicNewReportWorkspaceSelectionPage({route}: NewReportWorkspaceSelec
const [allTransactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION);
const [transactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS);
const currentUserPersonalDetails = useCurrentUserPersonalDetails();
const personalPolicy = usePersonalPolicy();

const selectedTransactionsKeys = Object.keys(selectedTransactions);
const transactionIDs = selectedTransactionsKeys.length ? selectedTransactionsKeys : selectedTransactionIDs;
Expand Down Expand Up @@ -135,6 +137,7 @@ function DynamicNewReportWorkspaceSelectionPage({route}: NewReportWorkspaceSelec
transactions,
allTransactionViolation: transactionViolations,
allReports,
personalPolicyOutputCurrency: personalPolicy?.outputCurrency,
});

// eslint-disable-next-line rulesdir/no-default-id-values
Expand Down
5 changes: 5 additions & 0 deletions src/pages/Search/SearchTransactionsChangeReport.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import useConditionalCreateEmptyReportConfirmation from '@hooks/useConditionalCr
import useHasPerDiemTransactions from '@hooks/useHasPerDiemTransactions';
import useOnyx from '@hooks/useOnyx';
import usePermissions from '@hooks/usePermissions';
import usePersonalPolicy from '@hooks/usePersonalPolicy';
import usePolicyForMovingExpenses from '@hooks/usePolicyForMovingExpenses';

import {createNewReport} from '@libs/actions/Report';
Expand Down Expand Up @@ -45,6 +46,7 @@ function SearchTransactionsChangeReport() {
const [allPolicies] = useOnyx(ONYXKEYS.COLLECTION.POLICY);
const [allPolicyCategories] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}`);
const [personalPolicyID] = useOnyx(ONYXKEYS.PERSONAL_POLICY_ID);
const personalPolicy = usePersonalPolicy();
const [userBillingGracePeriodEnds] = useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_USER_BILLING_GRACE_PERIOD_END);
const [ownerBillingGracePeriodEnd] = useOnyx(ONYXKEYS.NVP_PRIVATE_OWNER_BILLING_GRACE_PERIOD_END);
const [amountOwed] = useOnyx(ONYXKEYS.NVP_PRIVATE_AMOUNT_OWED);
Expand Down Expand Up @@ -159,6 +161,7 @@ function SearchTransactionsChangeReport() {
transactions,
allTransactionViolation: transactionViolations,
allReports,
personalPolicyOutputCurrency: personalPolicy?.outputCurrency,
});
clearSelectedTransactions();
});
Expand Down Expand Up @@ -237,6 +240,7 @@ function SearchTransactionsChangeReport() {
transactions,
allTransactionViolation: transactionViolations,
allReports,
personalPolicyOutputCurrency: personalPolicy?.outputCurrency,
});
Navigation.goBack(undefined, {afterTransition: clearSelectedTransactions});
};
Expand All @@ -256,6 +260,7 @@ function SearchTransactionsChangeReport() {
transactions,
allTransactionViolation: transactionViolations,
allReports,
personalPolicyOutputCurrency: personalPolicy?.outputCurrency,
});
clearSelectedTransactions();
Navigation.goBack();
Expand Down
4 changes: 4 additions & 0 deletions src/pages/iou/request/step/IOURequestEditReport.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'
import useHasPerDiemTransactions from '@hooks/useHasPerDiemTransactions';
import useOnyx from '@hooks/useOnyx';
import usePermissions from '@hooks/usePermissions';
import usePersonalPolicy from '@hooks/usePersonalPolicy';
import usePolicyForMovingExpenses from '@hooks/usePolicyForMovingExpenses';
import useTransactionsByID from '@hooks/useTransactionsByID';

Expand Down Expand Up @@ -53,6 +54,7 @@ function IOURequestEditReport({route}: IOURequestEditReportProps) {
const {isBetaEnabled} = usePermissions();
const isASAPSubmitBetaEnabled = isBetaEnabled(CONST.BETAS.ASAP_SUBMIT);
const currentUserPersonalDetails = useCurrentUserPersonalDetails();
const personalPolicy = usePersonalPolicy();
const [personalPolicyID] = useOnyx(ONYXKEYS.PERSONAL_POLICY_ID);
const [userBillingGracePeriodEnds] = useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_USER_BILLING_GRACE_PERIOD_END);
const [ownerBillingGracePeriodEnd] = useOnyx(ONYXKEYS.NVP_PRIVATE_OWNER_BILLING_GRACE_PERIOD_END);
Expand Down Expand Up @@ -103,6 +105,7 @@ function IOURequestEditReport({route}: IOURequestEditReportProps) {
transactions,
allTransactionViolation: transactionViolations,
allReports,
personalPolicyOutputCurrency: personalPolicy?.outputCurrency,
});
turnOffMobileSelectionMode();
clearSelectedTransactions(true);
Expand All @@ -126,6 +129,7 @@ function IOURequestEditReport({route}: IOURequestEditReportProps) {
transactions,
allTransactionViolation: transactionViolations,
allReports,
personalPolicyOutputCurrency: personalPolicy?.outputCurrency,
});
if (shouldTurnOffSelectionMode) {
turnOffMobileSelectionMode();
Expand Down
2 changes: 2 additions & 0 deletions src/pages/iou/request/step/IOURequestStepReport/hooks/useReportSelectionActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ function useReportSelectionActions({
transactions: targetTransactions,
allTransactionViolation: transactionViolations,
allReports,
personalPolicyOutputCurrency: allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${personalPolicyID}`]?.outputCurrency,
Comment thread
shubham1206agra marked this conversation as resolved.

@ikevin127 ikevin127 Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Inconsistency: hook vs. raw collection lookup for the same value

(assuming this was missed, and not planned in upcoming PRs)

Most call sites use the usePersonalPolicy() hook (the stated goal of this migration series):

personalPolicyOutputCurrency: personalPolicy?.outputCurrency,

But useReportSelectionActions.ts reaches into the allPolicies prop directly:

personalPolicyOutputCurrency: allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${personalPolicyID}`]?.outputCurrency,

Why it matters: The whole point of the series is to standardize on usePersonalPolicy(). Leaving one site on the raw lookup means the eventual cleanup PR has an inconsistent surface to reason about, and the two paths can diverge if usePersonalPolicy's selector logic ever changes.

Since useReportSelectionActions is a hook, it can call usePersonalPolicy() directly (or receive it as a prop like allPolicies currently is). I'd recommend aligning it, or at minimum note in the PR why this site intentionally differs. Not blocking, but it undercuts the refactor's consistency goal.

});
removeTransaction(transaction.transactionID);
}
Expand All @@ -234,6 +235,7 @@ function useReportSelectionActions({
transactions: targetTransactions,
allTransactionViolation: transactionViolations,
allReports,
personalPolicyOutputCurrency: allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${personalPolicyID}`]?.outputCurrency,
});
removeTransaction(transaction.transactionID);
},
Expand Down
2 changes: 2 additions & 0 deletions src/pages/iou/request/step/IOURequestStepUpgrade.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,8 @@ function IOURequestStepUpgrade({
transactions,
allTransactionViolation: transactionViolations,
allReports,
// Expenses move to the upgraded workspace (newPolicy), whose currency drives any distance calculation, so the personal-policy currency is never read here.

@ikevin127 ikevin127 Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 IOURequestStepUpgrade comment is slightly overconfident

The comment says the personal currency is "never read here," but that only holds if newPolicy?.outputCurrency is defined. newPolicy is an optimistically‑referenced, freshly‑upgraded workspace (allPolicies?.[...policyID]). If outputCurrency hasn't populated yet at that instant, getRate falls through personalPolicyOutputCurrency (undefined) → getPersonalPolicy() → USD.

Why it matters: Behavior is still safe (it degrades to the same fallback that existed pre‑refactor), so there's no regression. But the comment overstates the guarantee. Suggest softening to:

// newPolicy.outputCurrency normally drives the calc; undefined here just defers to getRate's existing fallback

Documentation accuracy only — no code change required.

personalPolicyOutputCurrency: undefined,
});

clearSelectedTransactions();
Expand Down
7 changes: 4 additions & 3 deletions tests/actions/TransactionTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,15 @@ import getOnyxValue from '../utils/getOnyxValue';
import {getGlobalFetchMock, getOnyxData} from '../utils/TestHelper';
import waitForBatchedUpdates from '../utils/waitForBatchedUpdates';

@ikevin127 ikevin127 Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Test coverage gap: the new parameter's behavior is not asserted

Both test wrappers were updated to accept and forward the param, but they make it optional and every existing test leaves it undefined:

// tests/unit/TransactionTest.ts:39-45
personalPolicyOutputCurrency?: string;
function changeTransactionsReport({..., personalPolicyOutputCurrency, ...rest}) {
    changeTransactionsReportAction({..., personalPolicyOutputCurrency, ...rest});
}

I grepped both suites: there is no test that passes a non‑undefined personalPolicyOutputCurrency and asserts the resulting rate/currency. So the branch that this entire PR exists to enable — policy?.outputCurrency == null and personalPolicyOutputCurrency supplied — is exercised at 0%. Existing tests only cover the policy currency path and the undefined fallback (which still routes to getPersonalPolicy()).

Why it matters: The migration's whole risk is "does passing the currency explicitly produce the same rate as the old module read?" Without an assertion on that path, a future change to the fallback order in getRate:578 could silently break distance amounts on unreported/self‑DM expenses and no test would catch it.

Recommendation: add one focused case — a distance transaction with policy having no outputCurrency, personalPolicyOutputCurrency: 'EUR', asserting the updated transaction's currency/modifiedAmount reflect EUR. This is a high‑value follow‑up in the PR.

type LegacyChangeTransactionsReportProps = Omit<Parameters<typeof changeTransactionsReportAction>[0], 'transactions' | 'allTransactionViolation'> & {
type LegacyChangeTransactionsReportProps = Omit<Parameters<typeof changeTransactionsReportAction>[0], 'transactions' | 'allTransactionViolation' | 'personalPolicyOutputCurrency'> & {
allTransactions: OnyxCollection<Transaction>;
transactionViolations: Parameters<typeof changeTransactionsReportAction>[0]['allTransactionViolation'];
personalPolicyOutputCurrency?: string;
};

function changeTransactionsReport({allTransactions, transactionIDs, transactionViolations, ...rest}: LegacyChangeTransactionsReportProps) {
function changeTransactionsReport({allTransactions, transactionIDs, transactionViolations, personalPolicyOutputCurrency, ...rest}: LegacyChangeTransactionsReportProps) {
const transactions = transactionIDs.map((id) => allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${id}`]).filter((transaction): transaction is Transaction => !!transaction);
changeTransactionsReportAction({transactionIDs, transactions, allTransactionViolation: transactionViolations, ...rest});
changeTransactionsReportAction({transactionIDs, transactions, allTransactionViolation: transactionViolations, personalPolicyOutputCurrency, ...rest});
}

const topMostReportID = '23423423';
Expand Down
7 changes: 4 additions & 3 deletions tests/unit/TransactionTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,16 @@ import getOnyxValue from '../utils/getOnyxValue';
import * as TestHelper from '../utils/TestHelper';
import waitForBatchedUpdates from '../utils/waitForBatchedUpdates';

type LegacyChangeTransactionsReportProps = Omit<Parameters<typeof changeTransactionsReportAction>[0], 'transactions' | 'allTransactionViolation'> & {
type LegacyChangeTransactionsReportProps = Omit<Parameters<typeof changeTransactionsReportAction>[0], 'transactions' | 'allTransactionViolation' | 'personalPolicyOutputCurrency'> & {
allTransactions: OnyxCollection<Transaction>;
transactionViolations?: OnyxCollection<TransactionViolation[]>;
personalPolicyOutputCurrency?: string;
};

// Wrapper mirroring the pre-refactor signature so existing test call sites compile unchanged.
function changeTransactionsReport({allTransactions, transactionIDs, transactionViolations = {}, ...rest}: LegacyChangeTransactionsReportProps) {
function changeTransactionsReport({allTransactions, transactionIDs, transactionViolations = {}, personalPolicyOutputCurrency, ...rest}: LegacyChangeTransactionsReportProps) {
const transactions = transactionIDs.map((id) => allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${id}`]).filter((transaction): transaction is Transaction => !!transaction);
changeTransactionsReportAction({transactionIDs, transactions, allTransactionViolation: transactionViolations, ...rest});
changeTransactionsReportAction({transactionIDs, transactions, allTransactionViolation: transactionViolations, personalPolicyOutputCurrency, ...rest});
}

function generateTransaction(values: Partial<Transaction> = {}): Transaction {
Expand Down
Loading