Fix: distance expense to new user lands on Workspace with 'previously existing chat' error (new manual expense flow)#94755
Conversation
Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
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.
|
|
@MelvinBot Please replace all [ ] characters in PR description by [x]. Then mark this PR as ready for review |
|
@thelullabyy 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] |
melvin-bot
Bot
requested review from
flaviadefaria
and removed request for
a team
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariNew flow: Screen.Recording.2026-07-02.at.02.57.14.movOld flow: Screen.Recording.2026-07-02.at.02.58.35.mov |
|
@MelvinBot Your fix doesn't work. Please revert all changes and apply my patch --- a/src/pages/iou/request/step/IOURequestStepConfirmation.tsx
+++ b/src/pages/iou/request/step/IOURequestStepConfirmation.tsx
@@ -36,6 +36,7 @@ import {setMoneyRequestBillable, setMoneyRequestReimbursable} from '@libs/action
import {setTransactionReport} from '@libs/actions/Transaction';
import {isMobileSafari} from '@libs/Browser';
import {canUseTouchScreen} from '@libs/DeviceCapabilities';
+import DistanceRequestUtils from '@libs/DistanceRequestUtils';
import getIsNarrowLayout from '@libs/getIsNarrowLayout';
import getNonEmptyStringOnyxID from '@libs/getNonEmptyStringOnyxID';
import {
@@ -54,7 +55,7 @@ import {submitWithDismissFirst} from '@libs/Navigation/helpers/submitWithDismiss
import Navigation, {navigationRef} from '@libs/Navigation/Navigation';
import type {MoneyRequestNavigatorParamList} from '@libs/Navigation/types';
import {getParticipantsOption, getReportOption} from '@libs/OptionsListUtils';
-import {findSelfDMReportID, getReportOrDraftReport, isMoneyRequestReport, isPolicyExpenseChat as isPolicyExpenseChatUtils} from '@libs/ReportUtils';
+import {findSelfDMReportID, generateReportID, getReportOrDraftReport, isMoneyRequestReport, isPolicyExpenseChat as isPolicyExpenseChatUtils} from '@libs/ReportUtils';
import {buildCannedSearchQuery, getCurrentSearchQueryJSON} from '@libs/SearchQueryUtils';
import {cancelTracking, getPendingSubmitFollowUpAction, isTracking} from '@libs/telemetry/submitFollowUpAction';
import type {SkeletonSpanReasonAttributes} from '@libs/telemetry/useSkeletonSpan';
@@ -66,7 +67,7 @@ import {
isOdometerDistanceRequest as isOdometerDistanceRequestTransactionUtils,
isScanRequest,
} from '@libs/TransactionUtils';
-import {getIOURequestPolicyID, setMoneyRequestParticipants, setMoneyRequestParticipantsFromReport} from '@userActions/IOU/MoneyRequest';
+import {getIOURequestPolicyID, setCustomUnitRateID, setMoneyRequestCategory, setMoneyRequestParticipants, setMoneyRequestParticipantsFromReport} from '@userActions/IOU/MoneyRequest';
import {setMoneyRequestReceipt} from '@userActions/IOU/Receipt';
import {removeDraftTransaction, replaceDefaultDraftTransaction} from '@userActions/TransactionEdit';
import CONST from '@src/CONST';
@@ -219,6 +220,7 @@ function IOURequestStepConfirmation({
const isOdometerDistanceRequest = isOdometerDistanceRequestTransactionUtils(transaction);
const isTimeRequest = requestType === CONST.IOU.REQUEST_TYPE.TIME;
const [lastLocationPermissionPrompt] = useOnyx(ONYXKEYS.NVP_LAST_LOCATION_PERMISSION_PROMPT);
+ const [lastSelectedDistanceRates] = useOnyx(ONYXKEYS.NVP_LAST_SELECTED_DISTANCE_RATES);
const privateIsArchivedMap = usePrivateIsArchivedMap();
const receiptFilename = transaction?.receipt?.filename;
@@ -323,15 +325,54 @@ function IOURequestStepConfirmation({
}
setMoneyRequestParticipants(activeTransactionID, participantsList);
const firstParticipant = participantsList.at(0);
- if (iouType !== CONST.IOU.TYPE.SPLIT) {
- setTransactionReport(activeTransactionID, {reportID: firstParticipant?.reportID ?? reportID}, true);
+ if (iouType !== CONST.IOU.TYPE.SPLIT && firstParticipant) {
+ const isPolicyExpenseChatParticipant = !!firstParticipant.isPolicyExpenseChat;
+
+ // A brand-new recipient picked by email has no chat yet (no reportID). Reusing the route's `reportID`
+ // (which points at the flow's origin report - e.g. the default workspace chat this distance flow was
+ // seeded with) leaves the expense bound to that workspace, so the backend rejects it with
+ // "There is a previously existing chat between these users." Generate a fresh optimistic reportID for
+ // P2P recipients, mirroring the legacy participants-step flow (useParticipantSubmission).
+ // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
+ const participantReportID = firstParticipant.reportID || (isPolicyExpenseChatParticipant ? reportID : generateReportID());
+ setTransactionReport(activeTransactionID, {reportID: participantReportID}, true);
+
+ // When switching from the auto-assigned default workspace to a P2P recipient we must also undo the
+ // workspace-specific defaults the distance step applied: reset the mileage rate to the P2P rate and
+ // clear the workspace's default category (and the category-derived tax). Otherwise the confirmation
+ // keeps the workspace "Default Rate"/category and the expense stays bound to that workspace. This
+ // mirrors what the legacy addParticipant/goToNextStep path does when a P2P recipient is selected.
+ if (!isPolicyExpenseChatParticipant) {
+ if (isDistanceRequest) {
+ const p2pRateID = DistanceRequestUtils.getCustomUnitRateID({
+ reportID: firstParticipant.reportID,
+ isPolicyExpenseChat: false,
+ policy: undefined,
+ lastSelectedDistanceRates,
+ expenseDate: transaction?.created,
+ });
+ setCustomUnitRateID(activeTransactionID, p2pRateID, transaction, undefined);
+ }
+ setMoneyRequestCategory(activeTransactionID, '', undefined);
+ }
}
}
if (participantsList.length > 0) {
closeParticipantPicker();
}
},
- [activeTransactionID, closeParticipantPicker, currentUserPersonalDetails.accountID, navigation, selfDMReport, iouType, reportID],
+ [
+ activeTransactionID,
+ closeParticipantPicker,
+ currentUserPersonalDetails.accountID,
+ navigation,
+ selfDMReport,
+ iouType,
+ reportID,
+ isDistanceRequest,
+ lastSelectedDistanceRates,
+ transaction,
+ ],
);
useEffect(() => {--- a/src/libs/Violations/ViolationsUtils.ts
+++ b/src/libs/Violations/ViolationsUtils.ts
@@ -499,16 +499,25 @@ const ViolationsUtils = {
const customUnitRateID = updatedTransaction?.comment?.customUnit?.customUnitRateID;
if (customUnitRateID && customUnitRateID.length > 0 && !isSelfDM) {
- const isPerDiem = TransactionUtils.isPerDiemRequest(updatedTransaction);
- const customRate = isPerDiem ? getPerDiemRateCustomUnitRate(policy, customUnitRateID) : getDistanceRateCustomUnitRate(policy, customUnitRateID);
- if (customRate && customRate.enabled !== false) {
+ // A P2P distance rate (FAKE_P2P_ID) isn't tied to any workspace, so it can never be "out of policy".
+ // Clear any stale violation instead of flagging it. This prevents a spurious "Rate not valid for this
+ // workspace" from appearing optimistically when a P2P expense is created while a workspace policy is
+ // still in context (e.g. the new manual expense flow switching the recipient from the default workspace
+ // to a person before the API distance response arrives).
+ if (TransactionUtils.isCustomUnitRateIDForP2P(updatedTransaction)) {
newTransactionViolations = reject(newTransactionViolations, {name: CONST.VIOLATIONS.CUSTOM_UNIT_OUT_OF_POLICY});
} else {
- newTransactionViolations.push({
- name: CONST.VIOLATIONS.CUSTOM_UNIT_OUT_OF_POLICY,
- type: CONST.VIOLATION_TYPES.VIOLATION,
- showInReview: true,
- });
+ const isPerDiem = TransactionUtils.isPerDiemRequest(updatedTransaction);
+ const customRate = isPerDiem ? getPerDiemRateCustomUnitRate(policy, customUnitRateID) : getDistanceRateCustomUnitRate(policy, customUnitRateID);
+ if (customRate && customRate.enabled !== false) {
+ newTransactionViolations = reject(newTransactionViolations, {name: CONST.VIOLATIONS.CUSTOM_UNIT_OUT_OF_POLICY});
+ } else {
+ newTransactionViolations.push({
+ name: CONST.VIOLATIONS.CUSTOM_UNIT_OUT_OF_POLICY,
+ type: CONST.VIOLATION_TYPES.VIOLATION,
+ showInReview: true,
+ });
+ }
}
}
|
…h + skip P2P out-of-policy violation Replaces the previous reportID-only fix. On the confirmation participant picker, when switching to a P2P recipient the mileage rate is reset to the P2P rate and the workspace default category is cleared. ViolationsUtils no longer flags a P2P (FAKE_P2P_ID) distance rate as CUSTOM_UNIT_OUT_OF_POLICY. Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
@puneetlath 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] |
|
🤖 Done — reverted the previous fix and applied your patch in What changed
Verification
Notes on how the patch was appliedYour patch's context was written against a newer
Give the failing flow another test when you get a chance and let me know if anything still isn't right. |
|
@codex review |
|
@MelvinBot Please merge main & resolve conflicts |
![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: d0b57482d9
ℹ️ 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".
| if (TransactionUtils.isCustomUnitRateIDForP2P(updatedTransaction)) { | ||
| newTransactionViolations = reject(newTransactionViolations, {name: CONST.VIOLATIONS.CUSTOM_UNIT_OUT_OF_POLICY}); |
There was a problem hiding this comment.
When a tracked distance expense is moved to a workspace, the existing flow intentionally preserves FAKE_P2P_ID so the user can manually choose a workspace rate, and DistanceRequestController also skips auto-updating that exact case (isMovingTransactionFromTrackExpense && customUnitRateID === FAKE_P2P_ID). This branch now treats every FAKE_P2P_ID as valid P2P and removes CUSTOM_UNIT_OUT_OF_POLICY even when the selected recipient/report is a policy expense chat, so the confirmation screen can submit a workspace distance expense with a personal mileage rate without showing the “Rate not valid for this workspace” violation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — this is a real regression, fixed in d5ad17e.
You're right that isCustomUnitRateIDForP2P keys only on the rate being FAKE_P2P_ID, so the branch cleared CUSTOM_UNIT_OUT_OF_POLICY even when the transaction was bound to a policy expense chat. That breaks the intentional track→workspace flow where FAKE_P2P_ID is preserved (and DistanceRequestController skips auto-updating it) precisely so the violation prompts the user to pick a valid workspace rate.
The fix gates the P2P clear on the transaction not being on a policy expense chat, so it only suppresses the violation for genuine P2P/self-DM recipients and lets the workspace case fall through to the existing rate check:
const isTransactionOnPolicyExpenseChat = updatedTransaction.participants?.some((participant) => participant?.isPolicyExpenseChat);
if (TransactionUtils.isCustomUnitRateIDForP2P(updatedTransaction) && !isTransactionOnPolicyExpenseChat) {
newTransactionViolations = reject(newTransactionViolations, {name: CONST.VIOLATIONS.CUSTOM_UNIT_OUT_OF_POLICY});
} else {
// ...existing workspace rate-matching logic (pushes the violation for FAKE_P2P_ID on a workspace)...
}src/libs/Violations/ViolationsUtils.ts:603
I used the transaction's participants rather than isSelfDM/iouReport because those aren't passed at the optimistic new-manual-flow call site (getViolationsOnyxData from buildOnyxDataForMoneyRequest), and isSelfDM alone can't distinguish a P2P person chat from a workspace (both are non-self-DM). The participants[].isPolicyExpenseChat flag travels with the transaction at every call site and mirrors the check already used in IOURequestStepConfirmation.tsx. Result: genuine P2P person + self-DM → cleared; workspace → violation preserved.
Verified: prettier, typecheck-tsgo, lint (single-file), and ViolationUtilsTest + useTransactionViolationsDistanceRateTest (173) + IOURequestStepConfirmationPageTest (13) all pass.
…xpense-newuser-reportid Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com> # Conflicts: # src/libs/Violations/ViolationsUtils.ts # src/pages/iou/request/step/IOURequestStepConfirmation.tsx
|
🤖 Merged Conflicts (2 files): 1. 2.
These are orthogonal, so I layered this PR's P2P check as the first branch inside Validation (both changed files):
Please re-verify the |
|
@MelvinBot Please check and address this comment #94755 (comment) |
…licy expense chat Addresses review feedback: a track distance expense moved onto a workspace intentionally keeps FAKE_P2P_ID until the user picks a workspace rate, and the out-of-policy violation is what prompts them. Gating the P2P clear on the transaction not being on a policy expense chat preserves that prompt while still suppressing the spurious violation for genuine P2P/self-DM recipients. Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
The failing check is unrelated to this PR — it's a flaky UI test, not a regression from these changes. Analysis: The only real failure is in This is a The other failing jobs ( Evidence it's unrelated: This PR changes only:
Neither is in the Recommended action: Re-run the failed jobs to clear the flake (I don't have permission to trigger a re-run from here). No code change is needed — per policy I don't push fixes for unrelated/flaky failures. If the same test keeps failing on re-run across unrelated PRs, it's worth filing a separate flaky-test issue for |
|
Re-checked after the latest run — the failures are confirmed flaky and unrelated to this PR. A different test failed this time than in the previous run, which is the definitive signature of flakiness rather than a real regression. This run's real failure — A 271s deep-linking session test that flips to failing is a timeout/async flake. Previous run's failure was a different test — Why unrelated: this PR changes only About the Recommended action: Re-run the failed test jobs to clear the flake. I don't have permission to trigger a re-run from this run, so a maintainer will need to hit "Re-run failed jobs" on the workflow. No code change is warranted — per policy I don't push fixes for unrelated/flaky failures. |
|
🤖 Fixed the failing Root cause: The
The Fix: Passed the missing arguments, mirroring the legacy P2P participants-step call at Verification
|
|
@MelvinBot Merge main please |
…xpense-newuser-reportid Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
🤖 Merged the latest |
|
@MelvinBot you have conflicts again. Can you merge main again? |
…xpense-newuser-reportid Co-authored-by: Puneet Lath <puneetlath@users.noreply.github.com> # Conflicts: # src/pages/iou/request/step/IOURequestStepConfirmation.tsx
|
🤖 Merged the latest There was one conflict in Please give the resolution a quick sanity check. |
|
@MelvinBot Oxfmt is failing. |
…roups Co-authored-by: Puneet Lath <puneetlath@users.noreply.github.com>
|
🤖 Fixed the failing Oxfmt check and pushed ( Cause: the PR removed the blank line between the |
melvin-bot
Bot
requested review from
puneetlath and
thelullabyy
and removed request for
a team
|
@thelullabyy @puneetlath 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] |
|
🚧 puneetlath 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/puneetlath in version: 9.4.28-0 🚀
|
|
🤖 Help site review: no doc changes required. I reviewed the changes in this PR against Why no docs are needed
Because the documented behavior was already correct and this PR simply makes the app match it, there is nothing to add or edit in the help site. @thelullabyy — no linked help site PR was created since no documentation changes are required. If you believe a specific article should be updated to cover this flow, let me know which one and I'll draft it. |
|
This PR failing because of the issue #95468 |
|
Checking... |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.28-2 🚀
Bundle Size Analysis (Sentry): |
5 participants
Explanation of Change
This fixes a real bug in the new manual expense flow (
NEW_MANUAL_EXPENSE_FLOWbeta): submitting a Track-distance expense from global-create to a brand‑new user (no existing chat) navigated to a Workspace and showed the backend error "There is a previously existing chat between these users." instead of opening the chat with the new user.Root cause: On the global‑create Track‑distance flow, the distance navigation auto‑assigns the user's default expense policy and lands on the confirmation page with
route.params.reportIDset to the Workspace policy‑expense chat. When the user then changes the participant to a brand‑new user via the new‑flow inline participant picker,handleParticipantsAddedre‑binds the transaction's report usingfirstParticipant?.reportID ?? reportID. A brand‑new user has no existing chat, sofirstParticipant?.reportIDisundefinedand the fallback resolves to the routereportID(the Workspace chat). The submission then carries a participant/report mismatch — the new user as participant but the Workspace as the report — which collides with an existing report and produces the "previously existing chat" error, landing on the Workspace.Fix: In
IOURequestStepConfirmation.tsx, generate a fresh optimisticreportIDfor a participant that has no existing report instead of falling back to the route (Workspace)reportID. This mirrors the already‑correct participant‑step behavior inuseParticipantSubmission.ts:282(firstParticipantReportID || generateReportID()). The now‑unusedreportIDdependency was removed from the callback's dependency array.The change is surgical: it only affects how the report is bound when the user explicitly picks a participant with no existing chat. The self‑DM branch and the default‑participant auto‑assignment are untouched.
Fixed Issues
$ #94289
PROPOSAL: #94289 (comment)
Tests
NEW_MANUAL_EXPENSE_FLOWbeta, and use an account that has an auto‑reporting default expense policy (Workspace).Automated tests run locally by the co-author:
npm test -- tests/ui/components/IOURequestStepConfirmationPageTest.tsx→ 13 passednpm test -- tests/ui/components/IOURequestStepDistanceTest.tsx→ 1 passednpm run typecheck-tsgo,npm run lint-changed,npm run prettier, andreact-compiler-compliance-check(no regression) all pass.Verify that no errors appear in the JS console
Offline tests
Same as Tests.
QA Steps
NEW_MANUAL_EXPENSE_FLOWbeta on a Workspace account with an auto‑reporting default policy.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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