Fix/75109: When auto-reporting is disabled, user cannot submit expens…#75286
Fix/75109: When auto-reporting is disabled, user cannot submit expens…#75286mollfpr merged 4 commits intoExpensify:mainfrom
Conversation
…e to 1:1 DM from FAB
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
@ikevin127 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
joekaufmanexpensify
and removed request for
a team
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good from a product perspective 👍
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-hybrid.mp4Android: mWeb Chromeandroid-mweb.mp4iOS: HybridAppios-hybrid.moviOS: mWeb Safariios-mweb.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
|
@IjaazA Good job on the detailed PR Explanation of Change and thorough manual tests!
☝️ These are all mandatory on every PR for both contributor and reviewer, without this we're blocked from proceeding. 🔄 Continuing with the code review... |
| const shouldUseParentChatReport = participant.isPolicyExpenseChat; | ||
| const parentChatReport = shouldUseParentChatReport ? report : undefined; |
There was a problem hiding this comment.
🟡 Issue: Duplication of parentChatReport logic across three functions
The same two–three lines of logic are repeated in:
• requestMoney
• trackExpense
• createDistanceRequest
While this is small, it couples three flows to the same business rule in a copy-pasted way. If a future bug fix tweaks the logic (for example, handling a new chat type, or a flag where we must ignore report even for certain policy chats), it’d be easy to update one call site and forget the others.
Why this matters / potential bug if left as-is
Imagine we later decide that, for certain policies, distance requests should never inherit the route report, even when isPolicyExpenseChat is true. If we only remember to update requestMoney and forget createDistanceRequest, we’d see:
• Expenses created correctly for manual receipts in the right chat.
• Distance expenses still created in the wrong workspace chat.
That would be a subtle behavior regression that’s hard to spot, because only some IOU types and request types would misbehave.
Suggested fix
Factor this into a small helper inside the component:
function getParentChatReportForParticipant(participant: Participant | undefined, report: Report | undefined) {
if (!participant?.isPolicyExpenseChat) {
return undefined;
}
return report;
}Then reuse it in for all 3 scenarios:
const participant = selectedParticipants.at(0);
if (!participant) {
return;
}
// new code
const parentChatReport = getParentChatReportForParticipant(participant, report);
requestMoneyIOUActions({
report: parentChatReport,
// ...
});There was a problem hiding this comment.
🟢 The suggested fix for this duplication will be resolved if the suggested fix from this comment is applied first.
| if (!transaction) { | ||
| return; | ||
| } | ||
| const participant = selectedParticipants.at(0); |
There was a problem hiding this comment.
🟡 Issue: Missing defensive empty-participant guard in createDistanceRequest
Unlike requestMoney and trackExpense, this function does not early-return when participant is empty.
Today the UI likely always passes at least one participant, but this function is a critical backend boundary between UI and createDistanceRequestIOUActions. If a future regression allows confirming with no participant (e.g., a bug in MoneyRequestConfirmationList), selectedParticipants could be [].
createDistanceRequestIOUActions then calls participants.at(0) internally and passes a participant with no accountID to getMoneyRequestInformation which will:
• Set payerAccountID = Number(participant.accountID) → 0.
• Try getChatByParticipants([0, payeeAccountID]), possibly creating a bogus self / ghost chat.
Why this matters / potential bug if left as-is
If this edge case happens, you could end up with:
• Distance expenses linked to a chat that has no real participant (payer account ID 0)
• Confusing history/actions in some “ghost” conversation where nothing else lives
• Potential future crashes if other code assumes a chat’s participants always map to real account IDs
Suggested fix
Mirror the pattern in requestMoney / trackExpense:
const participant = selectedParticipants.at(0);
if (!participant) {
return;
}There was a problem hiding this comment.
👉 This one is still valid even if the "🔴 Regression Alert" fix is applied.
🔴 Regression Alert@IjaazA When testing the flow while offline, optimistically the report still thinks it's being sent to a workspace therefore requiring category and getting the This is how it looks optimistically (while offline): MacOS: Desktop
The issue can be seen even when online, briefly on mWeb & Native I noticed it for a split second, same reason: iOS: Nativeios-hybrid.mov🟢👇See RCA + SolutionHere’s what’s going on and how I’d fix it: 1. Root cause: Violations still use the workspace policy for DM expenses The “Missing category” text is the violations.missingCategory string, driven by this logic in ViolationsUtils.getViolationsOnyxData: const policyRequiresCategories = !!policy.requiresCategory;
if (policyRequiresCategories) {
const hasMissingCategoryViolation = transactionViolations.some((violation) => violation.name === 'missingCategory');
const categoryKey = updatedTransaction.category;
const isCategoryInPolicy = categoryKey ? policyCategories?.[categoryKey]?.enabled : false;
// Remove 'missingCategory' violation if category is valid according to policy
if (hasMissingCategoryViolation && (isCategoryInPolicy || isSelfDM)) {
newTransactionViolations = reject(newTransactionViolations, {name: 'missingCategory'});
}
// Add 'missingCategory' violation if category is required and not set
if (!hasMissingCategoryViolation && policyRequiresCategories && !categoryKey && !isSelfDM) {
newTransactionViolations.push({name: 'missingCategory', type: CONST.VIOLATION_TYPES.VIOLATION, showInReview: true});
}
}The key pieces: • It uses policy.requiresCategory to decide whether category is required. 1.2 Who calls this for newly created expenses When you call requestMoneyIOUActions from IOURequestStepConfirmation, it eventually goes through: • requestMoney() in src/libs/actions/IOU.ts → // We don't need to compute violations unless we're on a paid policy
if (!policy || !isPaidGroupPolicy(policy) || transaction.reportID === CONST.REPORT.UNREPORTED_REPORT_ID) {
return [optimisticData, successData, failureData];
}
const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(
transaction,
[],
policy,
policyTagList ?? {},
policyCategories ?? {},
hasDependentTags(policy, policyTagList ?? {}),
false, // <-- isInvoiceTransaction
// NOTE: isSelfDM is *not* passed here, so it's implicitly undefined/false
)Crucially: • It passes your workspace policy into ViolationsUtils. 1.3 Why this shows up only now with DM flows Before your change, for “Create expense from workspace”: • You passed report (workspace chat) into requestMoneyIOUActions. After your change: • For 1:1 DM selections, you correctly pass report: undefined so the IOU logic finds/creates the DM chat. So: • The chat/report is now correct (DM). That matches exactly what you’re seeing: the expense is created in the DM correctly, but the optimistic preview is polluted by the workspace’s policy. 2. Concrete fix: Don’t pass workspace policy for non‑workspace participants We want to: • Use workspace policy only when the destination is a policy expense chat. Why this fixes the bug • For policy expense chat selections: If this isn’t done, DM expenses created from a paid workspace will continue to show false-positive policy violations (like missing category) in their optimistic previews, even though they aren’t governed by that workspace’s policy when you open them. 2.2 Mirror this approach for trackExpense and createDistanceRequest (recommended) The same pattern exists in: • trackExpense → getTrackExpenseInformation → its own buildOnyxDataForMoneyRequest call and violation computation. To prevent the same class of bug for: • Tracked expenses moved into DMs, and you should apply the same conditional policyParams logic in those callbacks, then use policyParams in the respective trackExpenseIOUActions / createDistanceRequestIOUActions calls. If you don’t do this, you’ll end up with the exact same type of issue for those flows: DM expenses that look like they still need to obey workspace policy in their previews, while the actual opened transaction doesn’t show the required fields. Solution - Centralize in “workspace vs DM” contextTo make this harder to regress later, you can:
function getMoneyRequestContextForParticipant(participant: Participant | undefined, report: Report | undefined) {
const isWorkspaceTarget = !!participant?.isPolicyExpenseChat;
return {
parentChatReport: isWorkspaceTarget ? report : undefined,
policyParams: isWorkspaceTarget
? {
policy,
policyTagList: policyTags,
policyCategories,
policyRecentlyUsedCategories,
}
: {},
};
}
Example DIFFdiff --git a/src/pages/iou/request/step/IOURequestStepConfirmation.tsx b/src/pages/iou/request/step/IOURequestStepConfirmation.tsx
index 94c57d2ed80..17538f64c24 100644
--- a/src/pages/iou/request/step/IOURequestStepConfirmation.tsx
+++ b/src/pages/iou/request/step/IOURequestStepConfirmation.tsx
@@ -510,6 +510,21 @@ function IOURequestStepConfirmation({
});
}, [requestType, iouType, initialTransactionID, reportID, action, report, transactions, participants]);
+ function getMoneyRequestContextForParticipant(participant: Participant | undefined, report: Report | undefined) {
+ const isWorkspaceTarget = !!participant?.isPolicyExpenseChat;
+ return {
+ parentChatReport: isWorkspaceTarget ? report : undefined,
+ policyParams: isWorkspaceTarget
+ ? {
+ policy,
+ policyTagList: policyTags,
+ policyCategories,
+ policyRecentlyUsedCategories,
+ }
+ : {},
+ };
+ }
+
const requestMoney = useCallback(
(selectedParticipants: Participant[], gpsPoint?: GpsPoint) => {
if (!transactions.length) {
@@ -520,9 +535,8 @@ function IOURequestStepConfirmation({
if (!participant) {
return;
}
- const shouldUseParentChatReport = participant.isPolicyExpenseChat;
- const parentChatReport = shouldUseParentChatReport ? report : undefined;
+ const {parentChatReport, policyParams} = getMoneyRequestContextForParticipant(participant, report);
const optimisticChatReportID = generateReportID();
const optimisticCreatedReportActionID = rand64();
const optimisticReportPreviewActionID = rand64();
@@ -551,12 +565,7 @@ function IOURequestStepConfirmation({
payeeAccountID: currentUserPersonalDetails.accountID,
participant,
},
- policyParams: {
- policy,
- policyTagList: policyTags,
- policyCategories,
- policyRecentlyUsedCategories,
- },
+ policyParams,
gpsPoint,
action,
transactionParams: {
@@ -677,8 +686,7 @@ function IOURequestStepConfirmation({
if (!participant) {
return;
}
- const shouldUseParentChatReport = participant.isPolicyExpenseChat;
- const parentChatReport = shouldUseParentChatReport ? report : undefined;
+ const {parentChatReport, policyParams} = getMoneyRequestContextForParticipant(participant, report);
transactions.forEach((item, index) => {
const isLinkedTrackedExpenseReportArchived =
!!item.linkedTrackedExpenseReportID && archivedReportsIdSet.has(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${item.linkedTrackedExpenseReportID}`);
@@ -692,11 +700,7 @@ function IOURequestStepConfirmation({
payeeAccountID: currentUserPersonalDetails.accountID,
participant,
},
- policyParams: {
- policy,
- policyCategories,
- policyTagList: policyTags,
- },
+ policyParams,
transactionParams: {
amount: item.amount,
distance: isManualDistanceRequest ? (item.comment?.customUnit?.quantity ?? undefined) : undefined,
@@ -754,8 +758,7 @@ function IOURequestStepConfirmation({
return;
}
const participant = selectedParticipants.at(0);
- const shouldUseParentChatReport = participant?.isPolicyExpenseChat ?? false;
- const parentChatReport = shouldUseParentChatReport ? report : undefined;
+ const {parentChatReport, policyParams} = getMoneyRequestContextForParticipant(participant, report);
createDistanceRequestIOUActions({
report: parentChatReport,
participants: selectedParticipants,
@@ -763,12 +766,7 @@ function IOURequestStepConfirmation({
currentUserAccountID: currentUserPersonalDetails.accountID,
iouType,
existingTransaction: transaction,
- policyParams: {
- policy,
- policyCategories,
- policyTagList: policyTags,
- policyRecentlyUsedCategories,
- },
+ policyParams,
transactionParams: {
amount: transaction.amount,
comment: trimmedComment,This ensures that chat routing and policy / violation behavior always stay in sync: if you’re not using the workspace report, you’re also not using the workspace policy.
If this isn’t addressed: • Any expense created from a paid workspace but sent to a 1:1 DM will: By conditioning the policyParams on participant.isPolicyExpenseChat, you keep: • Workspace expenses fully policy-aware (including category violations), |
|
✅ Reviewer checklist completed. Awaiting for the requested changes to be applied and conflicts to be resolved before I can review again and approve if all good. Please make sure to run |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
LGTM 🟢
Looks like there are gonna be some conflicts once you merge main, make sure not to lose the changes when resolving the conflicts.
|
@IjaazA Could you resolve the conflict? Thank you! |
|
@mollfpr conflicts were resolved. |
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.
|
|
✋ 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/mollfpr in version: 9.2.61-0 🚀
|
|
🚀 Deployed to production by https://github.com/grgia in version: 9.2.61-5 🚀
|
|
🚀 Deployed to staging by https://github.com/mollfpr in version: 9.2.62-0 🚀
|
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.2.62-5 🚀
|
5 participants
Explanation of Change
When auto-reporting is disabled and a user selects a 1:1 DM participant (not a policy expense chat) on the expense confirmation page, the expense was still being created in the workspace chat instead of the selected 1:1 DM. This happened because
requestMoneyandcreateDistanceRequestfunctions were always using the report prop from the route parameters, which contained the workspace chat reportID, even when the user selected a different participant.The fix ensures that when a user selects a 1:1 DM participant on the confirm page, we pass
undefinedfor the report parameter torequestMoneyandcreateDistanceRequestfunctions. This allows the underlyinggetMoneyRequestInformationfunction to:parentChatReport(since it's undefined)getChatByParticipants([payerAccountID, payeeAccountID])to find the existing 1:1 DM chatFixed Issues
$ #75109
PROPOSAL: #75109 (comment)
Tests
Additional test - Verify workspace chat still works:
Offline tests
QA Steps
Additional QA test - Verify workspace chat still works:
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: mWeb Chrome
Recording_20251117_125655.mp4
iOS: mWeb Safari
Screen.Recording.2025-11-17.at.12.12.00.PM.mov
Android: mWeb Chrome
Screen.Recording.2025-11-18.at.12.58.58.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2025-11-17.at.4.36.47.PM.mov
MacOS: Desktop
Screen.Recording.2025-11-17.at.3.20.43.PM.mov