[Payment due @marufsharifi] No proactive transaction thread creation for CreateDistanceRequest and CreatePerDiemRequest - #96492
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-07-21.at.12.12.32.AM.movScreen.Recording.2026-07-21.at.12.14.56.AM.mov |
| // The backend creates the transaction thread itself and syncs it back via Onyx updates, so the client doesn't build it optimistically or pass its reportID to the API. | ||
| shouldGenerateTransactionThreadReport: false, |
There was a problem hiding this comment.
Confirm before merge — per diem is a different backend command than distance.
This disables proactive thread creation for per diem, but per diem goes through CreatePerDiemRequest, not CreateDistanceRequest (src/libs/API/types.ts:232 vs :242). The PR title and description only mention CreateDistanceRequest.
If Auth PR #23059 only makes CreateDistanceRequest build the thread server-side, per-diem expenses will end up with no transaction thread at all — the client stops creating it optimistically and the old backend won't create it either. Users then can't open or comment on the per-diem transaction thread, and it won't surface as a JS console error (so the current Tests section won't catch it).
Please confirm #23059 also covers CreatePerDiemRequest, and update the title/description to include per diem — or split per diem into its own PR until the backend is ready.
There was a problem hiding this comment.
This disables proactive thread creation for per diem, but per diem goes through CreatePerDiemRequest, not CreateDistanceRequest (src/libs/API/types.ts:232 vs :242). The PR title and description only mention CreateDistanceRequest.
Oh yea, I initially started only with CreateDistanceRequest but then realized CreatePerDiemRequest was also affected.
If Auth PR #23059 only makes CreateDistanceRequest build the thread server-side, per-diem expenses will end up with no transaction thread at all — the client stops creating it optimistically and the old backend won't create it either. Users then can't open or comment on the per-diem transaction thread, and it won't surface as a JS console error (so the current Tests section won't catch it).
It changes both APIs and to clarify the thread is created lazily on first access (OpenReport on open, EditMoneyRequest on edit, comments via get-or-create). So users can open/comment fine.
Updated the title.
| odometerStart, | ||
| odometerEnd, | ||
| }, | ||
| shouldGenerateTransactionThreadReport: false, |
There was a problem hiding this comment.
Unconditional false — deploy ordering is the only safety net.
There's no beta/flag here, so this takes effect the instant it ships. If App deploys before Auth #23059 is live, every distance (and per-diem) expense created in that window has no transaction thread until something backfills it.
This matches the existing pattern (useExpenseSubmission.ts:205 and IOUAmountSubmission.ts:365 also hard-code false), so a beta isn't strictly required — but those already had backend support. Since this one is a co-deploy, please gate on ordering: land/deploy this only after #23059 is confirmed live on the same environment. The description's "doesn't need to block" is the risk worth double-checking.
There was a problem hiding this comment.
No, really everything is fine. Both PRs are needed for the thread to not be created, but:
- App ships first (Auth still old): App omits the ID, then old Auth (shouldCreate=true) creates the thread server-side.
- Auth ships first (App still old): App still sends the optimistic ID, then new Auth uses it.
|
|
||
| // STEP 5: Build Onyx Data | ||
| const {optimisticData, successData, failureData} = buildOnyxDataForMoneyRequest({ | ||
| shouldGenerateTransactionThreadReport: false, |
There was a problem hiding this comment.
No regression test for the changed optimistic data (diff coverage = 0%).
This change alters the shape of the optimistic Onyx data (whether a transaction thread report + its CREATED action are written) — exactly the kind of thing a unit test catches cheaply and manual QA misses.
Suggested test alongside the existing IOU builder specs:
it('per diem / distance creation does not optimistically create a transaction thread', () => {
const {transactionThreadReportID, createdReportActionIDForThread, onyxData} = /* invoke builder */;
expect(transactionThreadReportID).toBeUndefined();
expect(createdReportActionIDForThread).toBeUndefined();
// and: no optimistic REPORT / REPORT_ACTIONS write for a transaction thread
});This applies equally to the createDistanceRequest path (Split.ts:2135).
iwiznia
changed the title
melvin-bot
Bot
changed the title
|
🎯 @marufsharifi, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
…ivethread # Conflicts: # tests/actions/IOUTest/SplitTest.ts
|
🚧 Valforte 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/Valforte in version: 9.4.42-0 🚀
|
|
🤖 Help site review: no docs changes required. I reviewed the changes in this PR and they do not require any updates to the help site articles under Why: This PR only changes who creates the transaction thread for The relevant help articles (e.g. Since no help site content needs to change, I did not create a draft docs PR (there is no linked help site PR to review). If you believe there's a user-facing behavior change I missed, let me know and I'll take another look. |
|
Hi @iwiznia. Any QA steps here? |
|
No, regression tests should cover this |
|
🤖 Payment issue created: #96788 |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.4.42-1 🚀
Bundle Size Analysis (Sentry): |
7 participants
Explanation of Change
This goes in hand with https://github.com/Expensify/Auth/pull/23059 we don't need to block, but both need to be deployed for this to work
Fixed Issues
$https://github.com/Expensify/Expensify/issues/428920
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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