fix: [GPS]Transactions - Always use iouRequestType to check for request type#88289
Conversation
|
@mananjadhav 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: d944b76967
ℹ️ 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".
|
@codex review |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3688075a47
ℹ️ 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".
…nsactions-Always-use-iouRequestType-to-check-for-request-type
|
PR doesn’t need product input as a code clean-up PR. Unassigning and unsubscribing myself. |
…nsactions-Always-use-iouRequestType-to-check-for-request-type # Conflicts: # src/components/ReportActionItem/MoneyRequestView.tsx # src/components/ReportActionItem/TripRoomPreview.tsx # src/components/Search/SearchList/ListItem/ExpenseReportListItemRow.tsx # src/libs/TransactionUtils/index.ts # src/pages/settings/InitialSettingsPage.tsx # src/pages/settings/Wallet/ExpensifyCardPage/index.tsx # src/pages/workspace/rules/SpendRules/SpendRulePageBase.tsx # src/pages/workspace/rules/SpendRules/SpendRulesSection.tsx
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 402ae74ee1
ℹ️ 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".
…nsactions-Always-use-iouRequestType-to-check-for-request-type
…nsactions-Always-use-iouRequestType-to-check-for-request-type
Sure, I wanted to receive feedbacks on code first, before recording for all, but Android Native is already recorded. Adding the rest now. |
…nsactions-Always-use-iouRequestType-to-check-for-request-type
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb05eca08c
ℹ️ 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".
Added demo for desktop and android native and screenshot for the rest of platforms. We can continue. |
|
Thanks @TaduJR. I'll take a look tomorrow. |
| currentUserAccountID: params.currentUserAccountIDParam, | ||
| existingTransaction: { | ||
| ...(params.transactionParams ?? {}), | ||
| iouRequestType: getDuplicateRequestType(transaction), |
There was a problem hiding this comment.
I can see this being duplicated quite a few times.
| }, | ||
| }); | ||
|
|
||
| splitTransaction.iouRequestType = receipt?.source ? CONST.IOU.REQUEST_TYPE.SCAN : CONST.IOU.REQUEST_TYPE.MANUAL; |
There was a problem hiding this comment.
We're building splitTransaction just before this line. Any reason why this isn't added there?
mananjadhav
left a comment
There was a problem hiding this comment.
One comment on repetitive object creation. Starting to test, can be done in parallel.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-iou-request-type.movandroid-iou-request.movAndroid: mWeb Chromemweb-chrome-iourequesttype.moviOS: HybridAppiOS: mWeb Safarimweb-safari-iourequestype.movMacOS: Chrome / Safariweb-iou-request-type.mov |
mananjadhav
left a comment
There was a problem hiding this comment.
Issues with my iOS build. But added one final comment.
|
@codex review @mananjadhav can you confirm all the 25 testing steps were taken? |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Yes I did. Generally we test all the steps fully and then a few others. But uploading a subset of the steps. Do you want me to upload them again? |
|
🚧 @NicolasBonet has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/NicolasBonet in version: 9.3.89-1 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Help site review: no docs updates required. This PR is an internal refactor + bug fix — every user-facing behavior it touches is preserved, not changed, so no What the PR changes vs. what's documentedThe PR's changes are code-level only:
No new UI, no new buttons/tabs/settings, no flow changes:
The PR's own Tests section (A–F) is a regression suite confirming the existing documented behavior still works after the refactor — it does not describe new functionality. @TaduJR, since there's nothing to publish, no draft docs PR was created. If you spot a user-visible change I missed, reply with |
Explanation of Change
is{Type}Request()utilities had two detection paths: checkiouRequestTypedirectly (during creation), or infer the type from structural fields (comment.type,customUnit.name, waypoints,receipt.source) after the server save. The fallback existed becauseiouRequestTypewasn't server-returned, and it forced a GPS-vs-Map hack tagging waypoints withkeyForList: 'gps_start'/'gps_stop'.Auth#19725 now sends
iouRequestTypeon every transaction, so the fallback is redundant.Changes:
is{Type}Requestis now a one-liner checkingiouRequestType(isDistanceRequestchecks theDISTANCEparent + its sub-types). RemovedhasDistanceCustomUnit,hasGPSWaypoints,isManualRequest(dead), theisUpdatedMergeTransactionbypass, and simplifiedgetRequestType.MergeTransaction.tswas writingiouRequestType: nullduring distance merges, breaking type detection under the simplified logic. Now falls back to the target transaction's type.isScanRequestcallers (MoneyRequestBuilder.ts,TrackExpense.ts) passedPartial<Transaction>shapes that would silently returnfalse. Instead of inline!!receipt?.source && amount === 0heuristics (CONSISTENCY-3),RequestMoneyInformation/CreateTrackExpenseParamsnow carry an explicitexistingTransaction, threaded through every submission path —createTransaction, the distance-track flow (handleMoneyRequestStepDistanceNavigation),useExpenseSubmission,SubmitDetailsPage,IOURequestStepAmount, and the duplicate flow. Resolved a known TIME duplicate trade-off as a side effect.getDuplicateRequestType()to coerce SCAN→MANUAL (since the duplicate flow strips the receipt). Wired into every duplicate call site —duplicateExpenseTransaction(workspace and unreported-track paths),createExpenseByType, andduplicateReport.requestMoney/trackExpensewere JSON-stringifying the entireexistingTransactioninto receipt-errorretryParamspersisted in Onyx. Replaced with a minimal shim (onlyiouRequestType+ placeholders), keeping retries small while preserving type recovery.startSplitBillsetsiouRequestTypeon optimistic split transactions;buildMergedTransactionDatapropagatesiouRequestTypecross-subtype.isScanRequesttoOnyxEntry<Transaction>(compile-time guard against partial shapes); dropped stale!!mergeTransactionIDarg inMoneyRequestView; updated JSDoc onTransaction.iouRequestType.Fixed Issues
$ #83028
PROPOSAL: #83028 (comment)
Tests
Before you start, set up:
A. Creating each kind of expense shows the correct icon and details
For each expense below, after you submit it, check three places: the expense's row inside the report, the expense preview shown in the chat, and the expense details page (tap the expense to open it).
Verify: the details page shows the amount and a Merchant field you can tap to edit; the expense row shows a cash icon.
Verify: the details page shows the receipt photo at the top and the scanned amount; the expense row shows a receipt thumbnail.
Verify: the details page shows a map with the route; the expense row shows a car icon.
Verify: the amount equals distance × rate; the expense row shows a car icon; the details page shows Distance and Rate fields.
Verify: the details page shows the two odometer photos, and the distance equals end minus start.
Verify: the details page shows the GPS route on a map; the expense row shows a car icon.
Verify: the expense row shows a calendar icon; the details page shows the destination, date range, and sub-rate breakdown, and the thumbnail shows a calendar icon (not a receipt photo, a blank box, or a cash icon).
Verify: the expense row shows a clock icon; the details page shows hours × rate.
B. The right fields show, hide, or can be edited on each kind of expense
Verify: the Merchant field is not shown (Distance and Rate are shown instead); the Tax field is either not shown, or shown but cannot be edited.
Verify: the Amount and Distance fields cannot be tapped to change them; the Rate field can still be changed.
Verify: the Tax field is not shown.
Verify: the Tax field is not shown.
Verify: on both, the Merchant field is shown and can be edited.
C. GPS and Map distance expenses look different from each other
Verify: the GPS expense shows its GPS route with start and end locations; the Map expense shows its waypoints/route; the two look clearly different.
Verify: both expenses still look correct — the GPS one still shows as GPS, the Map one still shows as Map.
D. Merging keeps the expense kind
Verify: before confirming, the preview still shows a map/route (not a blank box or a cash icon).
Verify: it is still a distance expense — the map is shown and the Merchant field is hidden.
Verify: the merged expense still shows the car icon and map.
E. Duplicating keeps the expense kind (a scan becomes a manual expense)
Verify: the duplicate has the car icon and map preview.
Verify: the duplicate has the calendar icon and per-diem breakdown.
Verify: the duplicate has the clock icon and hours breakdown.
Verify: the duplicate is created as a Manual expense (cash icon, no receipt photo). A duplicated scan does not copy the receipt, so it becomes a regular manual expense.
Verify: in the duplicated report, the scan expense is now a Manual expense, and the distance expense is still a Distance expense.
F. Retrying a failed receipt upload keeps it as a scan
Verify: the expense shows a red receipt-upload error.
Verify: the retried expense is created as a Scan expense (a receipt thumbnail appears) and appears in the report — it must not turn into a plain cash/manual expense.
Offline tests
Same as 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 stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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-Native.mp4
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Mac-Chrome.mp4