fix: prevent Move expense from appearing on forwarded reports#83146
fix: prevent Move expense from appearing on forwarded reports#83146mavrickdeveloper wants to merge 19 commits intoExpensify:mainfrom
Conversation
2ee1a2c to
cc33877
Compare
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.
|
5d9a8a4 to
8ed5224
Compare
|
Will sync with latest main + push a final update |
|
@hoangzinh 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: 8ed5224a2c
ℹ️ 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".
| const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${iouReport?.policyID}`]; | ||
| const reportMetadata = getReportMetadata(iouReport?.reportID); | ||
|
|
||
| if (!iouReport || !policy || !reportMetadata || !isProcessingReport(iouReport) || !isNumber(iouReport.managerID)) { |
There was a problem hiding this comment.
Drop hard requirement for report metadata in fallback
hasForwardedByManagerChange() returns early when getReportMetadata() is missing, so the new forwarded-detection fallback never runs for reports that do not yet have a REPORT_METADATA entry. In that state (which is common for untouched reports), a forwarded report with missing local FORWARDED action is still treated as outstanding and "Move expense" remains visible, so the bug this change targets is not actually fixed for those sessions. The DEW checks already handle undefined metadata safely, so this guard is overly restrictive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
will address
|
@mavrickdeveloper is this PR ready for review? |
|
@hoangzinh will push an update in few hours that'll address codex review above then i'll ping you here |
Add hasForwardedByManagerChange() fallback in isReportOutstanding() to detect forwarded reports when the FORWARDED action is missing from local Onyx data (e.g. in copilot/delegate sessions). The fallback compares getSubmitToAccountID() against managerID for processing reports, with a DEW guard to skip when pending approve/submit exists.
72e3c14 to
7ba659f
Compare
|
@hoangzinh code ready for review , please check |
Thank you. I will review today |
| targetOwnerAccountID?: number; | ||
| }; | ||
|
|
||
| function getSearchMoveSelectionValidation( |
There was a problem hiding this comment.
@mavrickdeveloper why do we need this function?
There was a problem hiding this comment.
So i needed getSearchMoveSelectionValidation() because this PR also fixes bulk-move menu/route drift.
During testing, i found the bulk menu and the RHP were applying different checks, so preserved/direct navigation could still expose Move for invalid selections. This helper centralizes the move-selection contract so both surfaces enforce the same rules , only transaction rows, every item must have canChangeReport, all items must belong to one owner, and the search must not be an expense-report search , thoughts?
There was a problem hiding this comment.
I appreciate your effort here. Do you have a recording to visualize this issue, " this PR also fixes bulk-move menu/route drift."? TY
There was a problem hiding this comment.
here you go , lmk if anything is unclear
2026-04-03.15-13-07.mov
There was a problem hiding this comment.
Our PR state for Bulk select & route drift :
2026-04-03.16-12-35.mp4
There was a problem hiding this comment.
@hoangzinh can you please confirm if we should revert the bulk/snapshot fix because that mismatch is mentioned in the issue OP in the actual result (Appears to fail when the BE provides a response to acknowledge that expenses can't be moved off an approved report.)
so the wrong move appearance in bulk select is actually a reported issue , but the route drift is not
lmk what you think
There was a problem hiding this comment.
so the wrong move appearance in bulk select is actually a reported issue , but the route drift is not
lmk what you think
Can you elaborate on it, please? The expectation of the original issue is that we shouldn't show "Moving report" in bulk select. Route drift (or direct access) is not what a normal user will do.
There was a problem hiding this comment.
@hoangzinh yes then we should keep the bulk fix , my last commit fixes 2 part , Move action appearance in action button before BE hydration + route drift
we should revert the route drift issue only , because it is not explicitly mentioned in the OP , but we should keep the bulk menu fix , this :
2026-04-06.16-01-43.mp4
if you look closely staging is wrongly showing move report vs our fix which is correctly blocking it before and after hydration which is included in the issue expected/actual behavior
We should preserve this fix behavior , we are on same page right ?
There was a problem hiding this comment.
ah yes, we should only revert the route drift issue.
There was a problem hiding this comment.
@mavrickdeveloper just wanna confirm, after revert, will we only have code related to your proposal here? #79602 (comment)
| return false; | ||
| } | ||
|
|
||
| const isDEWPolicy = hasDynamicExternalWorkflow(policy); |
There was a problem hiding this comment.
I commented here #79602 (comment), I think we don't need to worry about DEW policy as it's beta
There was a problem hiding this comment.
I agree we shouldn’t expand scope for DEW , but the current code doesn’t do that, it only preserves the existing pending-DEW suppression for the manager-mismatch fallback, my aim for that guard is to prevents false positives while DEW approve/submit is pending. i already removed the broader metadata requirement, so this is the minimal safe version of the fix , lmk what you think?
There was a problem hiding this comment.
Because we don't have test account to test this logic, so it's fine to exclude DEW until we need to support DEW policy
There was a problem hiding this comment.
Will do , i'll push that after checking the conflicts flagged below (src/hooks/useSearchBulkActions.ts)
| return false; | ||
| } | ||
|
|
||
| if (!isReportOutstanding(moneyRequestReport, moneyRequestReport.policyID)) { |
There was a problem hiding this comment.
I recall in proposal, you suggested to call shouldTreatAsForwardedForMoveExpense in isReportOutstanding as a backfill of hasForwardedAction. Is there any reason why we didn't do that anymore?
There was a problem hiding this comment.
my the proposal originally suggested putting the manager-mismatch fallback in isReportOutstanding(). i changed that after deeper investigation, because adding it to isReportOutstanding() introduced blast radius in a shared predicate. The final implementation scopes that fallback to the move path via shouldTreatAsForwardedForMoveExpense() in canEditFieldOfMoneyRequest(..., REPORT), which keeps the move fix while avoiding shared outstanding-classification drift , make sense?
There was a problem hiding this comment.
It makes sense, but isReportOutstanding is still returning an incorrect result. If it's possible, I highly recommend that we should update so that the function return correct value for all cases, including copilot access.
|
Hi @mavrickdeveloper, please check out my comment here. I want to see if we should address this in this PR. Our main goal should be to resolve the original issue so we can ship it as quickly as possible. |
This is an existing behavior. Please bring this to the internal team's attention and discuss it in the GitHub issue at #79602 if you believe it's a bug. If our assumption is incorrect, we could face deploy blockers. |
|
@mavrickdeveloper so in scenario "the workspace approver changes later”, you're expecting that after step 3, , existing reports still show "Move expense", don't you? If yes, can you check if BE supports us to "Move expense" for those reports? |
|
@hoangzinh before i answer you , can you please specify which behavior is on main and our pr is removing? |
Sure @mavrickdeveloper. Currently, when a money report is retracted. Then the "move report" is hidden for this report |
|
Oh, hold on, @mavrickdeveloper. When a workspace sets up multilevel approval and a money report is fully approved but then retracted, we currently do not display the "move report" option for those reports. If you would like to address this issue, could you please raise it with the internal team and ask if we would like to fix this bug or if it's expected behavior, or if we should leave it as it is. |
|
@hoangzinh Could you share a screen-record of the behavior that you are referring to in comparison to staging? 2026-04-15.10-58-51.mov |
|
PS @hoangzinh please try to attach screen-recordings , to make sure we are on same page , this |
|
Hi @mavrickdeveloper sorry I don't have recordings. Uhm, basically I'm trying to understand why do we need Okay, let's go back to your explanation here
I'm understanding that we need |
|
@hoangzinh im going to breakdown the changes and their direct effects via video , in the following comments
2026-04-20.03-08-39.mp4
What changed in the PR:
https://github.com/mavrickdeveloper/Expensify/blob/d1a9149d7bc511c5ec159231d79c71140c1a9deb/src/hooks/useSearchBulkActions.ts#L1280-L1295 |
|
2.Move to report is still visible for this valid forwarding-workspace control. This is the control that shows the fix is not overbroad: the workspace can have forwarding configuration, but this selected report does not match the PR's forwarded-blocking checks. LEFT = PR , RIGHT = Staging 2026-04-20.03-29-10.mp4What changed in the PR:
Code : https://github.com/mavrickdeveloper/Expensify/blob/d1a9149d7bc511c5ec159231d79c71140c1a9deb/src/components/Search/SearchSelectionUtils.ts#L62-L103 |
|
3.Approved-then-reset/retracted case is not re-enabled by the PR For the approved-then-reset/retracted case, LEFT = PR, RIGHT = staging 2026-04-15.10-58-51.movWhat changed in the PR:
Code : |
|
@hoangzinh i hope the above comments & demonstrations addresses all your questions |
|
Thank you for the detailed explanation, @mavrickdeveloper.
|
|
@hoangzinh Yes, your interpretation is correct for (2) and (3): Why:
That is also the same bulk-menu surface we agreed to keep in scope earlier when we reverted only the route-drift part and kept the bulk-search fix. So, in short:
For (1), if you want the exact seeded runtime case I used locally, the Search term was Generic debug steps for that forwarded-like snapshot case:
|
Hi @mavrickdeveloper, does this mean we are setting User C as the second approver?
|
|
If it's correct, I can still use your original proposal from this link: #83146 (comment) to address (1). Here is proof: Screen.Recording.2026-04-21.at.21.47.49.mp4So, do you have another scenario with specific testing steps to help me understand how the current implementation using |
|
@hoangzinh ok , i'll explain more , so the approach you linked here #83146 (comment) , fixes the happy path but causes a regression in the historical change can , i'll walk you through how i demonstrated the regression in the below video , on the left is the approach you linked #83146 (comment) & on the right is latest main (staging) 2026-04-22.12-41-26.movin the video the simple patch you referenced is problematic, it demonstrate a regression vs latest main in the case of workflow approvals change after expense creation To reproduce this you'll need 3 accounts on same workspace Accounts:
Steps:
Move Behavior
Bug Manifestation The initial proposal hides Move to report incorrectly. only the workspace approval The initial patch treats that mismatch as forwarded-like, so it blocks move. The latest PR avoids that false |
|
I understand your point. Let me explore it in more detail. |
|
Great , let me know if there is other questions |
sure, I will try to give updates today |
|
@MelvinBot review this PR |
PR ReviewSummary: This PR prevents the "Move expense" option from appearing on forwarded reports when the
The approach is sound and the test coverage is thorough. A few observations: Concerns1. Complexity of the two-layer approach The reviewer ( Recommendation: If the reviewer is satisfied with the policy-mutation regression scenario demonstrated in the Apr 22 comment, this is acceptable. But document (in code comments) why both layers are needed — specifically that the Search snapshot path catches forwarded state before 2. The null-guard addition for 3. New file
Positives
Nits
Overall: The PR addresses a real bug with a well-thought-out approach that avoids regressions in edge cases (policy mutations, retracted reports). The two-layer detection is more complex than ideal but is justified by the different data availability contexts (pre-hydration Search snapshot vs. post-hydration Onyx state). The test coverage is strong. I'd suggest the reviewer focus on confirming the policy-mutation scenario is reproducible on staging, and that the |
| if (!transaction.canChangeReport) { | ||
| return false; | ||
| } | ||
|
|
||
| const policy = getPolicyForPolicyID?.(transaction.policyID ?? transaction.report?.policyID); | ||
| return !shouldBlockForwardedMoveFromSearchSnapshot(transaction.report, policy, resolveSubmitToAccountID, getLoginForAccountID); |
There was a problem hiding this comment.
However, the current solution adds two separate detection mechanisms across three files — one in ReportUtils (workflow-action-history-based) and one in SearchSelectionUtils (snapshot-based). This dual approach has different logic for the same conceptual check, which increases long-term maintenance risk.
@mavrickdeveloper, what do you think about this Melvin's feedback? This is also my concern. What do you think if we combine conditions of shouldBlockForwardedMoveFromSearchSnapshot and transaction.canChangeReport ?
Something like this:
diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts
index e31c7e87665..72f4da958f9 100644
--- a/src/libs/ReportUtils.ts
+++ b/src/libs/ReportUtils.ts
@@ -131,6 +131,7 @@ import {getParsedMessageWithShortMentions} from './ParsingUtils';
import {getBankAccountLastFourDigits} from './PaymentUtils';
import Permissions from './Permissions';
import {
+ getLoginByAccountID,
getAccountIDsByLogins,
getDisplayNameOrDefault,
getEffectiveDisplayName,
@@ -11504,6 +11505,36 @@ function hasForwardedAction(reportID: string): boolean {
return Object.values(reportActions).some((action) => action?.actionName === CONST.REPORT.ACTIONS.TYPE.FORWARDED);
}
+/**
+ * Fallback for when forwarded actions are unavailable in local Onyx.
+ * A processing report is considered forwarded when managerID no longer matches the first approver.
+ */
+function hasForwardedByManagerChange(iouReport: OnyxInputOrEntry<Report>): boolean {
+ // Original approach
+ const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${iouReport?.policyID}`];
+
+ if (!iouReport || !policy || !iouReport.reportID || !isProcessingReport(iouReport) || !isNumber(iouReport.managerID)) {
+ return false;
+ }
+
+ const submitToAccountID = getSubmitToAccountID(policy, iouReport);
+
+ if (submitToAccountID === iouReport.managerID) {
+ return false;
+ }
+
+ // Copied from shouldBlockForwardedMoveFromSearchSnapshot
+ const submitToLogin = getLoginByAccountID(submitToAccountID);
+ const managerLogin = getLoginByAccountID(iouReport.managerID);
+ if (!submitToLogin || !managerLogin) {
+ return false;
+ }
+
+ const submitToEmployee = policy.employeeList?.[submitToLogin];
+
+ return submitToEmployee?.forwardsTo === managerLogin || submitToEmployee?.overLimitForwardsTo === managerLogin;
+}
+
function isReportOutstanding(
iouReport: OnyxInputOrEntry<Report>,
policyID: string | undefined,
@@ -11517,7 +11548,8 @@ function isReportOutstanding(
iouReport?.stateNum === undefined ||
iouReport?.statusNum === undefined ||
iouReport?.policyID !== policyID ||
- hasForwardedAction(iouReport.reportID)
+ hasForwardedAction(iouReport.reportID) ||
+ hasForwardedByManagerChange(iouReport)
) {
return false;
}
There was a problem hiding this comment.
I agree with the concern, but I do not think we should move this into isReportOutstanding() as suggested.
This suggested version ☝️ is safer than the earlier raw submitToAccountID !== managerID fallback because it also
checks forwardsTo / overLimitForwardsTo. However, putting it in isReportOutstanding() still changes a shared
function that is used outside the move-expense flow.
There are also two behavior gaps compared with the current PR:
-
It drops the workflow-cycle guard from
hasCurrentForwardingWorkflowAction(). That guard prevents old
APPROVED/FORWARDEDhistory from blocking move after reset actions likeUNAPPROVED,REOPENED,RETRACTED,
etc. -
It does not copy the full Search snapshot condition. The Search guard also checks that the report is submitted,
nextStepisWAITING_TO_APPROVE, andnextStep.actorAccountID === managerID. Those checks keep the Search bulk-menu block narrow.
I think extracting the shared forwarding relationship check is possible, but I am not sure it improves this PR enough to justify the extra churn.
The repeated part is small, and the two callers rely on different signals:
ReportUtilsuses report action history to protect the core move eligibility path after report actions are
available.SearchSelectionUtilsuses Search snapshot data to hide the bulk menu before full report actions are available.
Keeping them separate makes those different data states explicit. A shared helper would also couple these two paths more tightly, which i don't think is something we want here ?
The Search path currently stays snapshot-based and receives its dependencies through callbacks, while the ReportUtils path uses Onyx/report-action state directly. If we share the helper, future changes to one path could accidentally affect the other, even though they are handling different hydration states.
So my preference is to keep the current shape and add a short comment explaining why the Search snapshot guard exists separately from the ReportUtils workflow-history guard, lmk what you think.
There was a problem hiding this comment.
Fair enough, @mavrickdeveloper. The current implementation still makes "transaction.canChangeReport" unreliable for use in "Search" if we would like to use this boolean check somewhere else. If we don't want to move to isReportOutstanding(), how about considering canEditFieldOfMoneyRequest()? I believe we should correct transaction.canChangeReport rather than adding another layer for double-checking.
There was a problem hiding this comment.
@hoangzinh. I agree that transaction.canChangeReport should represent the final move eligibility for Search, instead of Search needing to double-check it later.
i still do not think we should move this into isReportOutstanding(), because that function is used outside the move-expense flow. But I think your suggestion about canEditFieldOfMoneyRequest() makes sense.
So my Plan would be :
- Keep
isReportOutstanding()unchanged. - Move the forwarded-like Search snapshot check into
canEditFieldOfMoneyRequest()under thefieldToEdit === CONST.EDIT_REQUEST_FIELD.REPORTpath. - Keep the check narrow by requiring the same Search snapshot signals we use now:
- processing expense report
- submitted report
nextStep.messageKey === WAITING_TO_APPROVEnextStep.actorAccountID === managerIDsubmitToAccountID !== managerID- submit-to approver forwards / over-limit-forwards to the current manager
- Then simplify
SearchSelectionUtilsso it only truststransaction.canChangeReportplus the selection-level checks like same owner, transaction rows only, and not expense-report search.
That should make transaction.canChangeReport reliable for Search, remove the extra double-checking layer, and still avoid changing isReportOutstanding() globally, lmk what you think
There was a problem hiding this comment.
I like that plan @mavrickdeveloper. Btw, why do we need to check
nextStep.messageKey === WAITING_TO_APPROVE
nextStep.actorAccountID === managerID
There was a problem hiding this comment.
Because I'm imagining submitToAccountID === managerID would do the same
There was a problem hiding this comment.
I do not think the submit-to / manager comparison covers the same thing as the nextStep checks.
Small clarification: if submitToAccountID === managerID, the fallback does not block today. The forwarded-likecase is when submitToAccountID !== managerID, plus the forwarding relationship matches. See the current check here:
https://github.com/mavrickdeveloper/Expensify/blob/7405c95548fea3b093c24a314c715a48ab7593c6/src/components/Search/SearchSelectionUtils.ts#L52-L64
The reason I think nextStep is still needed is that submitToAccountID comes from current policy config:
https://github.com/mavrickdeveloper/Expensify/blob/7405c95548fea3b093c24a314c715a48ab7593c6/src/libs/PolicyUtils.ts#L1182-L1192
But nextStep is the current Search/report snapshot state. These checks confirm the report is actually waiting for approval from the same manager we are treating as forwarded-to:
https://github.com/mavrickdeveloper/Expensify/blob/7405c95548fea3b093c24a314c715a48ab7593c6/src/components/Search/SearchSelectionUtils.ts#L47-L49
So the intent is:
submitToAccountID !== managerID+ forwarding relationship: identifies a forwarded-like policy/report mismatchnextStep.messageKey === WAITING_TO_APPROVE: confirms the report is currently in an approval stepnextStep.actorAccountID === managerID: confirms it is waiting on the forwarded-to manager
This matters because Search currently builds transaction.canChangeReport through canEditFieldOfMoneyRequest() here:
https://github.com/mavrickdeveloper/Expensify/blob/7405c95548fea3b093c24a314c715a48ab7593c6/src/components/Search/index.tsx#L134-L141
And the REPORT edit path is here:
https://github.com/mavrickdeveloper/Expensify/blob/7405c95548fea3b093c24a314c715a48ab7593c6/src/libs/ReportUtils.ts#L5020-L5039
I might be able to move the narrow snapshot check into that canEditFieldOfMoneyRequest() path so transaction.canChangeReport becomes the reliable source for Search, but I think we should keep the nextStep checks there so we do not block move based only on current policy config.
There was a problem hiding this comment.
The reason why I think nextStep is not really helpful is that it's outdated. It seems we changed to use ONYXKEYS.COLLECTION.NEXT_STEP. But it's up to you. I don't see any issue if we use it.
There was a problem hiding this comment.
Oh wait, ONYXKEYS.COLLECTION.NEXT_STEP is truly deprecated one lol
Line 1285 in c07f8e4
There was a problem hiding this comment.
Let's go with that plan @mavrickdeveloper. Hopefully, it makes everything cleaner
|
@hoangzinh about Melvins review , the two-layer approach is more complex, but I think it is justified here because the two checks are protecting different data states:
That said , I agree we should document this distinction in code so future readers do not treat the two checks as accidental duplication. the |
| * has already been advanced away from the original submit-to approver. | ||
| */ | ||
| function hasCurrentForwardingWorkflowAction(reportID: string): boolean { | ||
| const reportActions = getSortedReportActions( |
There was a problem hiding this comment.
We're going to deprecate this method soon in this issue #66405. It means if we want to fix this unapproved bug, we need to pass reportActions from the View component down to here.
This comment was marked as outdated.
This comment was marked as outdated.
|
@hoangzinh Update ready for review |

Explanation of Change
FORWARDEDaction is missing from local Onyx data (e.g. in copilot/delegate sessions).Root Cause:
isReportOutstanding()relies onhasForwardedAction()to detect forwarded reports, but this check only inspects locally availableREPORT_ACTIONS.FORWARDEDaction may not be present in local Onyx data, causing the report to appear outstanding when it has actually been forwarded to a higher-level approver.Solution:
hasForwardedByManagerChange()fallback inisReportOutstanding()that detects forwarded reports by comparinggetSubmitToAccountID()againstmanagerIDfor processing reports.hasPendingDEWApprove()/hasPendingDEWSubmit()to skip the fallback when a DEW operation is pending, preventing false positives.isProcessingReport()), so open/draft/approved/paid reports are unaffected.Known limitation:
PERSONAL_DETAILS_LIST(partial Onyx hydration),getSubmitToAccountID()may return a synthetic ID that doesn't matchmanagerID, causing a conservative false positive (Move hidden when it should be visible). The failure mode is conservative , it over-hides rather than over-shows.Fixed Issues
$ #79602
PROPOSAL: #79602 (comment)
Tests
Prerequisites:
Primary Tests (Fix Validation):
Non-Regression Tests:
As User B, find an outstanding expense report (submitted by User A, not yet approved)
Select the expense via checkbox in Search > Expenses
Open bulk actions menu
Verify "Move expense" IS shown in the menu
Select a paid expense via checkbox
Open bulk actions menu
Verify "Move expense" is NOT shown
Select a held expense via checkbox
Open bulk actions menu
Verify "Move expense" is NOT shown
Copilot/Delegate Session Test:
Offline tests
N/A - The fix operates on locally cached Onyx data (
REPORT_ACTIONS,REPORT_METADATA,PERSONAL_DETAILS_LIST). Move expense eligibility is determined client-side from already-synced state.QA Steps
Same as Primary Tests (steps 1–6) and Non-Regression Tests (steps 7–16) above.
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
iOS: Native
Video.Project.22.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
2026-03-06.04-45-56.mov