Repository navigation
Fix distance preview mismatch with expense details header - #101068
nabi-ebrahimi wants to merge 47 commits into
Conversation
|
@marufsharifi Please 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 526afbe6da
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
marufsharifi
left a comment
There was a problem hiding this comment.
No regression test added for the normalization this PR introduces.
The diff touches only these 2 source files, yet the author checklist marks "I added unit tests." tests/ui/TransactionPreviewContentTest.tsx already exists and is the natural home for:
- a distance expense w/ commuter exclusion, once removed from its policy-expense chat (shown in self-DM), renders full-route amount/distance in the preview and matches the details header; and
- a distance expense still in its policy-expense chat keeps the commuter-adjusted values (normalization must NOT apply).
For a subtle amount-normalization fix, this assertion is what stops the bug from silently returning. Test coverage of the diff is currently 0%.
| const transactionDetails = useMemo<Partial<TransactionDetails>>( | ||
| () => getTransactionDetails(transaction, undefined, policy, isParentPolicyExpenseChat) ?? {}, | ||
| [transaction, policy, isParentPolicyExpenseChat], | ||
| () => getTransactionDetails(displayTransaction, undefined, policy, isParentPolicyExpenseChat) ?? {}, |
There was a problem hiding this comment.
❓ Same allowNegativeAmount concern as index.tsx:90 (pre-existing here).
getTransactionDetails(displayTransaction, undefined, policy, isParentPolicyExpenseChat) passes the policy-expense-chat flag into the allowNegativeAmount slot (ReportUtils.ts:5010). Only transaction → displayTransaction changed on this line, so the questionable arg is pre-existing — but since you're already touching it, either switch to a real derived allowNegative (like MoneyRequestView.tsx:338) or add a clarifying comment so it isn't "fixed" into a regression later.
There was a problem hiding this comment.
This argument is pre-existing and was intentionally added in eeaa81f to support negative transaction preview values. This PR only changes the input from transaction to displayTransaction. I’m leaving the existing behavior unchanged to avoid introducing an unrelated regression; deriving allowNegativeAmount through shouldEnableNegative() can be handled separately with dedicated coverage.
|
@nabi-ebrahimi, Gentle bump. thanks. |
|
Hi,@marufsharifi ,Thank you for taking the time to review this. I’ve addressed all your feedback and thoroughly tested the changes to confirm that everything works as expected. Could you please review the latest changes again when you have a chance? |
|
Hi @Gonals , while testing the issue, I found another related bug that also reproduces on main. With distance rates enabled and a commuter exclusion configured: Move/remove a distance expense from a workspace to Self DM. For example, with a 2-mile commuter exclusion, 19.34 mi / TJS 19.34 becomes 17.34 mi / TJS 17.34. I also tested this with different distance-rate values, and the same behavior occurs. Should this be fixed as part of the current issue/PR, or would you prefer to track it in a separate issue? Screencast.From.2026-09-25.11-47-38.mp4 |
|
@Gonals, could you please take a look at #101068 (comment) when you get a chance. thanks. |
|
@Gonals, Gentle bump #101068 (comment). thanks. |
Let's create a different issue for it |
|
@marufsharifi, gentle bump. Thank you! |
|
Hi @marufsharifi, I’ve addressed all of your feedback. Could you please review the latest changes when you have a chance? Thanks! |
|
Hi @marufsharifi, during testing, I found and addressed two issues: changing the workspace distance rate could affect an existing expense’s amount in Self DM, and switching the app language could also change that amount. The fixes preserve the saved rate and keep the restored amount consistent, with regression tests added. Could you please review the latest changes? Thanks. |
|
Hi @marufsharifi, I have addressed all of your feedback and added the requested test coverage. Could you please review the latest changes? Thank you for your guidance. |
|
Hi @marufsharifi, during testing, I found that changing the workspace distance rate could cause an expense in Self DM to display its commuter-reduced amount again. I addressed this by validating both saved amounts against the historical rate and added regression tests. Could you please review the latest changes? Thank you. |
BUGSteps to reproduce: Open the workspace distance expense created at $1.00/mile. Expected result: The workspace’s one-mile commuter deduction applies consistently to both distance and amount: 13.53 − 1.00 = 12.53 miles × $2.00 = $25.06 The expense header, report row, and Amount field should reflect $25.06. Actual result: The Distance field shows 12.53 miles and “Removed 1.00 commuter mile”, but the Amount field, expense header, and report row show $27.06. The amount is calculated using the full 13.53 miles, despite the displayed deduction. RF-1.mov |
Thanks for the feedback, @marufsharifi . I’ve addressed it and added regression tests for the offline move and fractional rates. Could you please review it again? Click to show videoScreencast.From.2026-10-23.12-18-33.mp4 |
Explanation of Change
This PR fixes the distance expense preview mismatch after a distance expense with commuter exclusion is removed from a report and shown in the self-DM.
The transaction preview now uses the same display normalization logic as the expense details/header flow. For distance requests, it resolves the original policy for the selected distance rate when needed, then passes a display-safe transaction into
getTransactionDetails().This prevents the preview card from showing commuter-exclusion-adjusted amount/distance values when the expense is no longer in a policy expense chat, while preserving the existing commuter exclusion behavior for real workspace expenses.
Fixed Issues
$#100188
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screencast.From.2026-09-14.14-00-21.mp4