[No QA] Add getRateForPolicyChange to select a distance rate when the policy changes - #101080
Conversation
…changes Co-authored-by: truph01 <truph01@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
🤖 Web test steps verification results: ✅ pass App loaded and was signed in with no visible errors. The full distance-expense flow (FAB → Track distance → Start/Stop waypoints → Confirm details) worked end-to-end and rendered a correct distance rate row ($0.76/mile) with no failed network requests observed.
Scope note: the view run · no recording available |
|
🤖 Fixed and pushed in Cause: the Fix: ran
DetailsThe failing job's output was: The reformat: -function getRateForPolicyChange({
- transaction,
- policy,
- currentRate,
-}: {
- transaction: OnyxEntry<Transaction>;
- policy: OnyxEntry<Policy>;
- currentRate?: MileageRate;
-}): MileageRate | undefined {
+function getRateForPolicyChange({transaction, policy, currentRate}: {transaction: OnyxEntry<Transaction>; policy: OnyxEntry<Policy>; currentRate?: MileageRate}): MileageRate | undefined {Re-running Verification before pushing:
Two notes on the local run: One correction to the PR description. The |
|
🤖 Android test steps verification results: ✅ pass The
Scope note: the Session note: the pinned Android session started signed-out — CI's automated sign-in replay failed at the email-fill step. Re-probing the same selectors on a fresh snapshot resolved them immediately, so sign-in was completed by hand and the run proceeded. This looks like a one-off flake in the prestart replay rather than selector drift, so no flow fix was published. view run · no recording available |
|
@MelvinBot Merge main and mark this PR as ready |
…hange Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
Merged
AI tests re-run after the merge
Changed files are unchanged by the merge: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1d53e96ed
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function getRateForPolicyChange({transaction, policy, currentRate}: {transaction: OnyxEntry<Transaction>; policy: OnyxEntry<Policy>; currentRate?: MileageRate}): MileageRate | undefined { | ||
| const expenseDate = getFormattedCreated(transaction); | ||
| const mileageRates = getMileageRates(policy); | ||
| const rateToMatch = currentRate ?? (isCustomUnitRateIDForP2P(transaction) ? getRateForP2P(getCurrency(transaction), transaction) : undefined); |
There was a problem hiding this comment.
When an existing P2P expense's stored distanceUnit differs from the currently cached default P2P unit—for example, after the personal-policy currency/default changes—this derives rateToMatch through getRateForP2P(). That function takes the rate from transaction.comment.customUnit.defaultP2PRate but, whenever a default is loaded, takes its unit from the current global default rather than the transaction. Consequently, an equivalent destination rate using the expense's actual unit is rejected and the fallback rate can reprice the expense; construct the P2P match from the transaction's saved rate, currency, and distanceUnit instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a21b300 — this is the same issue truph01 raised, so replying here too.
The unit is now read off the transaction via getDistanceUnit(transaction, p2pRate), the way getRate does it: src/libs/DistanceRequestUtils.ts:663-666.
One correction to the suggestion: the saved rate and currency were already coming from the transaction. getRateForP2P is called with getCurrency(transaction), so rate resolves to transaction.comment.customUnit.defaultP2PRate and currency to the transaction's currency in both branches. Only the unit was taken from the global default.
truph01
left a comment
truph01
left a comment
There was a problem hiding this comment.
@MelvinBot Please check these comments
| function getRateForPolicyChange({transaction, policy, currentRate}: {transaction: OnyxEntry<Transaction>; policy: OnyxEntry<Policy>; currentRate?: MileageRate}): MileageRate | undefined { | ||
| const expenseDate = getFormattedCreated(transaction); | ||
| const mileageRates = getMileageRates(policy); | ||
| const rateToMatch = currentRate ?? (isCustomUnitRateIDForP2P(transaction) ? getRateForP2P(getCurrency(transaction), transaction) : undefined); |
There was a problem hiding this comment.
P2P unit comes from the global default, not the expense
getRateForP2P returns unit: p2pRate.unit whenever the stored default exists, so a P2P expense recorded in km matches against mi and gets repriced — could we wrap it in getDistanceUnit(transaction, ...) the way getRate does?
There was a problem hiding this comment.
Good catch — fixed in a21b300.
getRateForP2P returns unit: defaultRate ? p2pRate.unit : fallbackUnit, so once the global default loads the unit comes from Onyx, not the expense. A P2P expense saved in km then matched as mi, every destination rate was disqualified, and the fallback repriced it.
I wrapped it the way getRate does — src/libs/DistanceRequestUtils.ts:663-666:
const p2pRate = isCustomUnitRateIDForP2P(transaction) ? getRateForP2P(getCurrency(transaction), transaction) : undefined;
const rateToMatch = currentRate ?? (p2pRate ? {...p2pRate, unit: getDistanceUnit(transaction, p2pRate)} : undefined);rate and currency already come from the transaction (getRateForP2P is called with getCurrency(transaction), so both branches resolve to the expense's own values) — the unit was the only field taken from the global default.
Added two tests that both fail without this change: one where the destination policy is in km and the match must still be found, and one where it is in mi and the match must be rejected.
| return bScore - aScore; | ||
| } | ||
|
|
||
| if (aScore === 2 && bScore === 2) { |
There was a problem hiding this comment.
bScore === 2 is unreachable
The aScore !== bScore check above already returned, so aScore === 2 implies bScore === 2 — drop the second half?
There was a problem hiding this comment.
Right — dropped in a21b300. The aScore !== bScore early return above guarantees the scores are equal, so bScore === 2 was dead. Left a one-line comment saying why aScore === 2 is enough to know both rates are fully bounded: src/libs/DistanceRequestUtils.ts:586-587.
|
|
||
| return Object.values(mileageRates) | ||
| .filter( | ||
| (rate) => |
There was a problem hiding this comment.
Duplicate eligibility + sort logic
getRateMatchingCurrentRate re-implements the enabled/isRateEligibleForDate/sort chain from getBestEligibleRate — could we filter by value/currency/unit first and then hand the result to getBestEligibleRate?
There was a problem hiding this comment.
Done in a21b300 — filter by value/currency/unit, then hand the subset to getBestEligibleRate, exactly as you suggested. src/libs/DistanceRequestUtils.ts:639-643:
const equivalentRates = Object.fromEntries(
Object.entries(mileageRates).filter(([, rate]) => rate.rate === currentRate.rate && rate.currency === currentRate.currency && rate.unit === currentRate.unit),
);
return getBestEligibleRate(equivalentRates, expenseDate);The enabled/isRateEligibleForDate/sort chain now lives in one place, so the "narrowest and most recent date range" rule can't drift between the two. This also resolves your separate comment about the redundant enabled filter.
|
|
||
| return Object.values(mileageRates) | ||
| .filter( | ||
| (rate) => |
There was a problem hiding this comment.
enabled filter is already applied
getMileageRates(policy) drops disabled rates by default, so rate.enabled !== false here never fires — is it worth keeping, given the "does not match a disabled rate" test passes without it?
There was a problem hiding this comment.
Agreed, and it's gone as of a21b300. getMileageRates(policy) defaults includeDisabledRates to false and drops disabled rates at src/libs/DistanceRequestUtils.ts:56, so the check never fired here.
It dropped out naturally with the refactor from your other comment: the filter is now value/currency/unit only, and the remaining enabled !== false guard lives in getBestEligibleRate, which is exported and can be called with rate sets that do include disabled rates.
…etBestEligibleRate Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
@truph01 @neil-marcellini One of you needs to 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] |
|
Addressed all four review comments in
The codex bot flagged the same P2P unit problem, so that thread is answered too. The P2P unit one was a real bug, not just cleanup: a P2P expense saved in AI tests
The |
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
| /** | ||
| * Selects the distance rate to use for an expense that is moving to a different workspace. | ||
| * | ||
| * Selection order: | ||
| * 1. A rate on the destination policy that is equivalent to the expense's current rate. | ||
| * 2. The destination policy's best rate for the expense date, falling back to its default rate. | ||
| * 3. Nothing, when the destination policy has no usable rate. Callers keep the `customUnitOutOfPolicy` violation in that case. | ||
| * | ||
| * `currentRate` resolves against the source policy, which this module cannot look up, so callers that have it pass it in. It is | ||
| * optional only because a P2P expense carries its rate value on the transaction itself. | ||
| * | ||
| * Let's ensure this logic is consistent with the logic in the backend (Auth), which is authoritative here. The app computes the | ||
| * same answer only to build optimistic data. | ||
| */ |
There was a problem hiding this comment.
Shortened in 40500a2 — 14 lines down to 9. Collapsed the numbered selection order into one sentence and dropped the "only to build optimistic data" clause, which the getRate cross-reference already implies.
/**
* Selects the distance rate for an expense moving to a different workspace: an equivalent rate on the destination policy, else its
* best rate for the expense date, else its default rate, else nothing so the caller keeps the `customUnitOutOfPolicy` violation.
*
* `currentRate` resolves against the source policy, which this module cannot look up, so callers pass it in. It is optional because
* a P2P expense carries its rate on the transaction.
*
* Let's ensure this logic is consistent with the logic in the backend (Auth), which is authoritative here.
*/Comment-only change. typecheck, lint, oxfmt, spell-changed, and DistanceRequestUtilsTest (63 passed) all pass.
|
@MelvinBot Update PR title to "[No QA] Add getRateForPolicyChange to select a distance rate when the policy changes" |
Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
MelvinBot
changed the title
|
Title updated to The PR body's |
garrettmknight
left a comment
garrettmknight
left a comment
There was a problem hiding this comment.
Looks good from a product perspective.
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
Great work, thank you!
|
🚧 neil-marcellini has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.4.82-0 🚀
|
|
🤖 No help site changes are required for this PR. This PR adds The section that will need updating — but not yet docs/articles/new-expensify/reports-and-expenses/Distance-Expenses.md:168-172 currently documents the pre-project behavior:
Once auto-select actually ships, that becomes wrong — the expense gets a valid rate automatically and Concierge posts a message on the report. The right PRs to pair a docs update with are F3 and F4 in [Tracking] Auto-select distance rate on workspace change (frontend), both still open. F3 is the one that closes Expense-Rate violation disappears after moving expense to another report and makes the move-expense flow auto-select, so that is the earliest point the help site is measurably stale. I did not create a draft PR, so there is no linked help site PR to mark @truph01, please confirm you agree this PR needs no docs update. If you'd rather land the docs change now and let it sit ahead of F3/F4, say so and I'll open the draft PR against that section. |
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.4.86-0 🚀
|
|
No help site changes are required for this PR, so I did not create a draft docs PR. This PR adds
The article that will need updating is already written, and it is still correct.
That is accurate right now. It becomes wrong only once F3 and F4 wire the helper up, because the app will then auto-select a rate on the destination workspace — an equivalent rate first, otherwise the destination's best rate for the expense date, otherwise its default rate — and fall back to the violation only when the destination has no usable rate at all. My recommendation: update that section in the PR that adds the first caller, not here. Publishing the new behavior while it is unwired would describe something the product does not do yet. How Expensify selects a distance rate based on the expense date in Tracking issue for the follow-up work: [Tracking] Auto-select distance rate on workspace change (frontend) If you would rather I open the draft docs PR now so the copy is ready to merge alongside F3, say the word and I will write it against There is no linked help site PR to review, so the usual "confirm it reflects the current behavior, then mark it |
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.4.89-0 🚀
|
|
Help site changes are required — draft PR: Docs updates for E/A#101080 clarify distance rate behavior when moving expenses between workspaces. I said twice before that no docs change was needed. That was wrong, and I want to be precise about why: I checked whether this PR changed behavior (it doesn't — Two inaccuracies in 1. "Moved to a different Workspace" was wrong for expense-level moves. The article claims a moved expense always "keeps its original unit and rate." True for report moves, false for expense moves — 2. The article contradicted itself on rate editing. One FAQ says "It is not possible to adjust the distance rate or unit at the expense level"; the next tells users to select a valid rate on the expense. The rate is editable ( Two things to know before you review:
The PR documents behavior shipping today. When F3 and F4 of [Tracking] Auto-select distance rate on workspace change (frontend) wire up the new helper, the expense-level paragraph needs another pass, since selection becomes date-aware and prefers an equivalent rate over the default. Checks run and one unrelated finding
Unrelated finding worth a separate look: I could not push to your fork, so the branch lives upstream. Instructions to take ownership are in a comment on the docs PR. @truph01, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR view run · no recording available |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.89-0 🚀
Bundle Size Analysis (Sentry): |
5 participants
Explanation of Change
This is F2 of the frontend tracking issue: the shared helper that picks a distance rate when an expense moves to a different workspace.
Today the only App-side helper for this is
getDefaultMileageRate, which sorts the enabled rates byindexand takes the first. It ignores the expense date, and it never checks whether the destination workspace already has a rate equivalent to the one the expense is using — so a move reprices the expense even when an identical rate exists on the destination.This PR adds
DistanceRequestUtils.getRateForPolicyChange({transaction, policy, currentRate}), the app-side mirror of Auth's selection rule:getBestEligibleRateOrPolicyDefault, which is the destination policy's best rate for the expense date, falling back to its default rate.customUnitOutOfPolicyviolation.Two supporting details:
getBestEligibleRateis extracted intocompareRatesByDateSpecificityand reused for the matching step, so the "narrowest and most recent date range" rule stays in one place.getBestEligibleRatebehavior is unchanged.undefinedwhen the selected rate has nocustomUnitRateID.getDefaultMileageRatereturns a fully shaped rate object with an undefinedcustomUnitRateIDwhen the policy has no enabled rates, which a caller doingif (rate)would read as a successful selection.Note on the signature. The issue lists
{transaction, policy}. A workspace distance expense does not store its rate value — onlycomment.customUnit.customUnitRateID, which resolves against the source policy — andDistanceRequestUtilsholds no Onyx connections, so it cannot look that policy up itself.currentRateis therefore an optional third field: F3 and F4 pass the rate they have already resolved, and for a P2P expense the helper derives it fromcomment.customUnit.defaultP2PRateon the transaction so the documented two-argument call still works.The server is authoritative — the app computes the same answer only to build optimistic data — so the helper carries the cross-reference comment that
getRatealready uses. The matching comment on the Auth side is not in this PR, since it lives in a different repository.No callers change in this PR, so it can land at any time. F3 and F4 wire it up.
AI Tests
npm test -- tests/unit/DistanceRequestUtilsTest.tsnpm test -- tests/unit/DistanceRateTest.ts tests/unit/useDistanceRateOriginalPolicyTest.ts tests/unit/useTransactionViolationsDistanceRateTest.ts tests/unit/PolicyDistanceRatesUtilsTest.tsnpm run typechecknpm run lint-changednpm run spell-changednpm run lint(full repo)lint-changedruns the same linter over both changed files and passed.npm run prettierprettierscript exists inpackage.jsonat this commit.npm run storybook -- --smoke-test --ciFixed Issues
$ #100557
PROPOSAL: #100557 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review".
// Note: this PR adds a helper with no callers, so there is no user-facing behavior to test manually. The unit tests in
tests/unit/DistanceRequestUtilsTest.tscover the selection rules.Offline tests
Not applicable — this PR adds a pure helper function with no callers and no network behavior.
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review", or add "[No QA]" to the PR title.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
This PR adds a pure helper function with no callers, so there is no UI change to capture.
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari