Skip to content

fix: prevent Move expense from appearing on forwarded reports#83146

Open
mavrickdeveloper wants to merge 19 commits intoExpensify:mainfrom
mavrickdeveloper:fix/79602-move-expense-forwarded-reports
Open

fix: prevent Move expense from appearing on forwarded reports#83146
mavrickdeveloper wants to merge 19 commits intoExpensify:mainfrom
mavrickdeveloper:fix/79602-move-expense-forwarded-reports

Conversation

@mavrickdeveloper
Copy link
Copy Markdown
Contributor

@mavrickdeveloper mavrickdeveloper commented Feb 21, 2026

Explanation of Change

  • This PR fixes the issue where the "Move expense" option incorrectly appears on forwarded reports when the FORWARDED action is missing from local Onyx data (e.g. in copilot/delegate sessions).

Root Cause:

  • isReportOutstanding() relies on hasForwardedAction() to detect forwarded reports, but this check only inspects locally available REPORT_ACTIONS.
  • In copilot/delegate sessions, the FORWARDED action 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.
  • This results in "Move expense" being shown on reports where it should be hidden.

Solution:

  • Add a hasForwardedByManagerChange() fallback in isReportOutstanding() that detects forwarded reports by comparing getSubmitToAccountID() against managerID for processing reports.
  • Include a DEW (Dynamic External Workflow) guard via hasPendingDEWApprove() / hasPendingDEWSubmit() to skip the fallback when a DEW operation is pending, preventing false positives.
  • The fallback only triggers for processing reports (isProcessingReport()), so open/draft/approved/paid reports are unaffected.

Known limitation:

  • When approver personal details are missing from PERSONAL_DETAILS_LIST (partial Onyx hydration), getSubmitToAccountID() may return a synthetic ID that doesn't match managerID, 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:

  • Two user accounts: User A (submitter), User B (approver/manager)
  • A workspace with a multi-level approval chain (e.g. User A → User B → User C)
  • User B is logged in

Primary Tests (Fix Validation):

  1. As User A, create and submit an expense report on the workspace
  2. As User B (first approver), approve the report ,it is now forwarded to User C
  3. As User B, navigate to Search > Expenses (type: expense, status: all)
  4. Select the forwarded expense via checkbox
  5. Open bulk actions menu ("1 selected" button)
  6. Verify "Move expense" is NOT shown in the menu

Non-Regression Tests:

  1. As User B, find an outstanding expense report (submitted by User A, not yet approved)

  2. Select the expense via checkbox in Search > Expenses

  3. Open bulk actions menu

  4. Verify "Move expense" IS shown in the menu

  5. Select a paid expense via checkbox

  6. Open bulk actions menu

  7. Verify "Move expense" is NOT shown

  8. Select a held expense via checkbox

  9. Open bulk actions menu

  10. Verify "Move expense" is NOT shown

Copilot/Delegate Session Test:

  1. Log in as a copilot of User A
  2. Navigate to a forwarded expense report where FORWARDED action may not be in local Onyx
  3. Select the expense and open bulk actions
  4. Verify "Move expense" is NOT shown
  • Verify that no errors appear in the JS console

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.

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
    • Windows: Chrome
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I verified there are no new alerts related to the canBeMissing param for useOnyx
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I used JaimeGPT to get English > Spanish translation. I then posted it in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

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

@mavrickdeveloper mavrickdeveloper force-pushed the fix/79602-move-expense-forwarded-reports branch from 2ee1a2c to cc33877 Compare February 21, 2026 20:12
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 21, 2026

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.

Files with missing lines Coverage Δ
src/components/Search/SearchSelectionUtils.ts 100.00% <100.00%> (ø)
src/hooks/useSearchBulkActions.ts 46.81% <100.00%> (-0.79%) ⬇️
src/hooks/useSelectedTransactionsActions.ts 75.90% <100.00%> (ø)
src/libs/ReportSecondaryActionUtils.ts 94.29% <100.00%> (ø)
...rc/pages/Search/SearchTransactionsChangeReport.tsx 0.00% <0.00%> (ø)
src/libs/ReportUtils.ts 82.60% <95.34%> (+0.17%) ⬆️
src/components/Search/index.tsx 0.91% <0.00%> (-0.01%) ⬇️
... and 10 files with indirect coverage changes

@mavrickdeveloper mavrickdeveloper force-pushed the fix/79602-move-expense-forwarded-reports branch 2 times, most recently from 5d9a8a4 to 8ed5224 Compare February 25, 2026 22:39
@mavrickdeveloper
Copy link
Copy Markdown
Contributor Author

Will sync with latest main + push a final update

@mavrickdeveloper mavrickdeveloper marked this pull request as ready for review March 2, 2026 07:49
@mavrickdeveloper mavrickdeveloper requested review from a team as code owners March 2, 2026 07:49
@melvin-bot melvin-bot Bot requested a review from hoangzinh March 2, 2026 07:49
@melvin-bot
Copy link
Copy Markdown

melvin-bot Bot commented Mar 2, 2026

@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]

@melvin-bot melvin-bot Bot requested review from heyjennahay and removed request for a team March 2, 2026 07:49
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/libs/ReportUtils.ts Outdated
const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${iouReport?.policyID}`];
const reportMetadata = getReportMetadata(iouReport?.reportID);

if (!iouReport || !policy || !reportMetadata || !isProcessingReport(iouReport) || !isNumber(iouReport.managerID)) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will address

@hoangzinh
Copy link
Copy Markdown
Contributor

@mavrickdeveloper is this PR ready for review?

@mavrickdeveloper
Copy link
Copy Markdown
Contributor Author

@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.
@mavrickdeveloper mavrickdeveloper force-pushed the fix/79602-move-expense-forwarded-reports branch from 72e3c14 to 7ba659f Compare March 9, 2026 12:36
@mavrickdeveloper
Copy link
Copy Markdown
Contributor Author

@hoangzinh code ready for review , please check

@hoangzinh
Copy link
Copy Markdown
Contributor

@hoangzinh code ready for review , please check

Thank you. I will review today

targetOwnerAccountID?: number;
};

function getSearchMoveSelectionValidation(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mavrickdeveloper why do we need this function?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate your effort here. Do you have a recording to visualize this issue, " this PR also fixes bulk-move menu/route drift."? TY

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here you go , lmk if anything is unclear

2026-04-03.15-13-07.mov

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our PR state for Bulk select & route drift :

2026-04-03.16-12-35.mp4

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Copy Markdown
Contributor

@hoangzinh hoangzinh Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@mavrickdeveloper mavrickdeveloper Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes, we should only revert the route drift issue.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mavrickdeveloper just wanna confirm, after revert, will we only have code related to your proposal here? #79602 (comment)

Comment thread src/libs/ReportUtils.ts Outdated
return false;
}

const isDEWPolicy = hasDynamicExternalWorkflow(policy);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented here #79602 (comment), I think we don't need to worry about DEW policy as it's beta

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do , i'll push that after checking the conflicts flagged below (src/hooks/useSearchBulkActions.ts)

Comment thread src/libs/ReportUtils.ts
return false;
}

if (!isReportOutstanding(moneyRequestReport, moneyRequestReport.policyID)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hoangzinh
Copy link
Copy Markdown
Contributor

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.

@hoangzinh
Copy link
Copy Markdown
Contributor

the report was approved and then reset (UNAPPROVED / REOPENED / etc.), where stale history should no longer keep Move blocked

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.

@hoangzinh
Copy link
Copy Markdown
Contributor

@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?

@mavrickdeveloper
Copy link
Copy Markdown
Contributor Author

@hoangzinh before i answer you , can you please specify which behavior is on main and our pr is removing?

@hoangzinh
Copy link
Copy Markdown
Contributor

@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

@hoangzinh
Copy link
Copy Markdown
Contributor

hoangzinh commented Apr 15, 2026

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.

@mavrickdeveloper
Copy link
Copy Markdown
Contributor Author

@hoangzinh Could you share a screen-record of the behavior that you are referring to in comparison to staging?
our PR does not falsely restore/allow move for retracted reports
in the below video a demonstration of that , LEFT is our PR , RIGHT is staging , both are showing same behavior of a retracted report post approval

2026-04-15.10-58-51.mov

@mavrickdeveloper
Copy link
Copy Markdown
Contributor Author

PS @hoangzinh please try to attach screen-recordings , to make sure we are on same page ,

this MOVE , feature has multiple conditions and they can behave differently pre-hydration and post-hydration , i think this is the core essence of the issue we are fixing , so please for faster collab if you face an unusual behavior share steps , conditions and a screen-record would be great , thnx

@hoangzinh
Copy link
Copy Markdown
Contributor

Hi @mavrickdeveloper sorry I don't have recordings. Uhm, basically I'm trying to understand why do we need getSearchMoveSelectionValidation. The simple change here works for me #83146 (comment).

Okay, let's go back to your explanation here

the report was approved and then reset (UNAPPROVED / REOPENED / etc.), where stale history should no longer keep Move blocked

I'm understanding that we need getSearchMoveSelectionValidation to fix this case. Can you share whether the "Move report" menu is displayed or hidden in this case, currently? and with getSearchMoveSelectionValidation will it hide or display the "Move report" menu? TY

@mavrickdeveloper
Copy link
Copy Markdown
Contributor Author

@hoangzinh im going to breakdown the changes and their direct effects via video , in the following comments

  1. this forwarded-like Search selection does not expose Move to report.
2026-04-20.03-08-39.mp4

LEFT = PR (fixed), RIGHT = staging

What changed in the PR:

  • getSearchMoveSelectionValidation() still respects the existing canChangeReport value already attached to each selected row.
  • On top of that, the PR adds shouldBlockForwardedMoveFromSearchSnapshot() for the pre-hydration case where Search already knows the report is waiting on the current manager, and the current submit-to approver forwards to that manager.
  • useSearchBulkActions() now renders Move to report only when that shared validation returns canMoveToReport: true.

https://github.com/mavrickdeveloper/Expensify/blob/d1a9149d7bc511c5ec159231d79c71140c1a9deb/src/components/Search/SearchSelectionUtils.ts#L23-L59

https://github.com/mavrickdeveloper/Expensify/blob/d1a9149d7bc511c5ec159231d79c71140c1a9deb/src/components/Search/SearchSelectionUtils.ts#L62-L103

https://github.com/mavrickdeveloper/Expensify/blob/d1a9149d7bc511c5ec159231d79c71140c1a9deb/src/hooks/useSearchBulkActions.ts#L1280-L1295
https://github.com/mavrickdeveloper/Expensify/blob/d1a9149d7bc511c5ec159231d79c71140c1a9deb/tests/unit/SearchSelectionUtilsTest.ts#L233-L262

https://github.com/mavrickdeveloper/Expensify/blob/d1a9149d7bc511c5ec159231d79c71140c1a9deb/tests/unit/SearchSelectionUtilsTest.ts#L292-L315

@mavrickdeveloper
Copy link
Copy Markdown
Contributor Author

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.mp4

What changed in the PR:

  • We did not replace the move permission with a broad submitToAccountID !== managerID rule.
  • The shared Search validation only blocks the narrow forwarded-like snapshot case.
  • Normal submitted expenses remain movable when submit-to and manager match.
  • Policy-mutation mismatch controls also remain movable when the next step points to the new approver instead of the stale manager.

Code :

https://github.com/mavrickdeveloper/Expensify/blob/d1a9149d7bc511c5ec159231d79c71140c1a9deb/src/components/Search/SearchSelectionUtils.ts#L62-L103
https://github.com/mavrickdeveloper/Expensify/blob/d1a9149d7bc511c5ec159231d79c71140c1a9deb/src/hooks/useSearchBulkActions.ts#L1280-L1295
https://github.com/mavrickdeveloper/Expensify/blob/d1a9149d7bc511c5ec159231d79c71140c1a9deb/tests/unit/SearchSelectionUtilsTest.ts#L265-L289
https://github.com/mavrickdeveloper/Expensify/blob/d1a9149d7bc511c5ec159231d79c71140c1a9deb/tests/unit/SearchSelectionUtilsTest.ts#L318-L364
https://github.com/mavrickdeveloper/Expensify/blob/d1a9149d7bc511c5ec159231d79c71140c1a9deb/tests/unit/SearchSelectionUtilsTest.ts#L367-L423

@mavrickdeveloper
Copy link
Copy Markdown
Contributor Author

3.Approved-then-reset/retracted case is not re-enabled by the PR

For the approved-then-reset/retracted case, Move to report is hidden currently, and it remains hidden with getSearchMoveSelectionValidation().

LEFT = PR, RIGHT = staging

2026-04-15.10-58-51.mov

What changed in the PR:

  • getSearchMoveSelectionValidation() is additive. It starts from the existing row-level canChangeReport result and does not create a new allow-path for a row that is already ineligible.
  • The PR's reset-action handling lives in the core move-eligibility fallback, where SUBMITTED, UNAPPROVED, RETRACTED, REOPENED, REJECTED, and REJECTED_TO_SUBMITTER reset older forwarding history for the current workflow cycle.
  • So this clip should be read as: the Search helper does not re-enable the reset/retracted case; the helper's separate job is still the forwarded-like Search snapshot block shown in Clip 1.

Code :

https://github.com/mavrickdeveloper/Expensify/blob/d1a9149d7bc511c5ec159231d79c71140c1a9deb/src/components/Search/SearchSelectionUtils.ts#L62-L103

https://github.com/mavrickdeveloper/Expensify/blob/d1a9149d7bc511c5ec159231d79c71140c1a9deb/src/libs/ReportUtils.ts#L11539-L11600

https://github.com/mavrickdeveloper/Expensify/blob/d1a9149d7bc511c5ec159231d79c71140c1a9deb/tests/unit/canEditFieldOfMoneyRequestTest.ts#L664-L727

@mavrickdeveloper
Copy link
Copy Markdown
Contributor Author

@hoangzinh i hope the above comments & demonstrations addresses all your questions

@hoangzinh
Copy link
Copy Markdown
Contributor

Thank you for the detailed explanation, @mavrickdeveloper.

As User A, create and submit an expense report on the workspace
As User B

@mavrickdeveloper
Copy link
Copy Markdown
Contributor Author

@hoangzinh Yes, your interpretation is correct for (2) and (3): getSearchMoveSelectionValidation() preserves the current Search behavior there, and only adds one extra hide-path for the forwarded-like snapshot from (1).

Why:

  • In SearchSelectionUtils.ts, the helper starts from the existing row-level canChangeReport result.
  • If a row is already ineligible, it stays ineligible. The helper does not create a new allow-path.
  • The only extra check is the narrow snapshot block for the pre-hydration case where Search already knows:
    1. the report is waitingToApprove for the current manager
    2. submitToAccountID !== managerID
    3. the current submit-to approver forwards to that manager

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:

  • (2) yes: valid forwarding-workspace controls still behave like staging and still show Move to report
  • (3) yes: if Search already has canChangeReport: false, the helper keeps Move to report hidden & it does not
    flip that case back to visible

For (1), if you want the exact seeded runtime case I used locally, the Search term was usd-src-04.

Generic debug steps for that forwarded-like snapshot case:

  1. Create a workspace with advanced approval workflow.
  2. Use User A as the submitter.
  3. Set User B as the current submit-to approver.
  4. Configure User B to forward (or over-limit forward) to User C.
  5. Submit an expense as User A so the report is in waitingToApprove and the acting manager is User C.
  6. Open Search > Expenses, find that expense, select it, and open Selected.
  7. On main, Move to report can still appear in the bulk menu from the Search snapshot.
  8. On this PR, it stays hidden because getSearchMoveSelectionValidation() blocks that forwarded-like snapshot
    before the menu renders the action.

@hoangzinh
Copy link
Copy Markdown
Contributor

hoangzinh commented Apr 21, 2026

Configure User B to forward (or over-limit forward) to User C.

Hi @mavrickdeveloper, does this mean we are setting User C as the second approver?

Screenshot2026-04-21_21-28-25

@hoangzinh
Copy link
Copy Markdown
Contributor

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.mp4

So, do you have another scenario with specific testing steps to help me understand how the current implementation using getSearchMoveSelectionValidation can resolve the issue, while the original proposal at #83146 (comment) cannot?

@mavrickdeveloper
Copy link
Copy Markdown
Contributor Author

@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.mov

in 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:

  • Bob: submitter/Workspace admin
  • Alice: original approver
  • Charlie: new approver after workflow change

Steps:

  1. Create/open a workspace with advanced approvals enabled.
  2. Set Bob submits to Alice.
  3. As Bob, submit an expense.
  4. Do not approve the report.
  5. here go and try to select the expense (not the report) from expenses page and see the Move action in the select menu and compare it with staging
  6. Change the workspace workflow so Bob now submits to Charlie (replace Alice).
  7. Sign in as Alice.
  8. Open Search > Expenses from a fresh session.
  9. Find the expense submitted in step 3.
  10. Select the row and open Selected.

Move Behavior

Bug Manifestation

The initial proposal hides Move to report incorrectly. only the workspace approval
workflow changed after submission. That makes the current submitTo resolve to Charlie, while the existing report
still has Alice as managerID.

The initial patch treats that mismatch as forwarded-like, so it blocks move. The latest PR avoids that false
positive.

@hoangzinh
Copy link
Copy Markdown
Contributor

I understand your point. Let me explore it in more detail.

@mavrickdeveloper
Copy link
Copy Markdown
Contributor Author

Great , let me know if there is other questions

@hoangzinh
Copy link
Copy Markdown
Contributor

hoangzinh commented Apr 24, 2026

Great , let me know if there is other questions

sure, I will try to give updates today

@hoangzinh
Copy link
Copy Markdown
Contributor

@MelvinBot review this PR

@MelvinBot
Copy link
Copy Markdown
Contributor

PR Review

Summary: This PR prevents the "Move expense" option from appearing on forwarded reports when the FORWARDED action is missing from local Onyx data (e.g. copilot/delegate sessions). It does this via two mechanisms:

  1. shouldTreatAsForwardedForMoveExpense() in ReportUtils.ts — a fallback that detects forwarded reports by comparing getSubmitToAccountID() against managerID, gated by hasCurrentForwardingWorkflowAction() to avoid false positives from policy approver mutations.
  2. getSearchMoveSelectionValidation() in a new SearchSelectionUtils.ts — an additional Search-level guard using shouldBlockForwardedMoveFromSearchSnapshot() that blocks the bulk "Move expenses" menu item using Search snapshot data (nextStep, forwardsTo/overLimitForwardsTo).

The approach is sound and the test coverage is thorough. A few observations:


Concerns

1. Complexity of the two-layer approach

The reviewer (hoangzinh) repeatedly asked whether the simpler hasForwardedByManagerChange fallback in isReportOutstanding would suffice. The PR author demonstrated a legitimate false-positive scenario (policy approver mutation after submission). 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.

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 REPORT_ACTIONS hydrate, while the ReportUtils path catches it after hydration via workflow action history.

2. SearchTransactionsChangeReport.tsx change is minimal but unrelated

The null-guard addition for transactionID in SearchTransactionsChangeReport.tsx (lines 37-39) is a defensive fix unrelated to the forwarded-report issue. It's harmless but should be called out as a separate concern.

3. New file SearchSelectionUtils.ts duplicates owner-resolution logic

getSearchMoveSelectionValidation() re-implements the owner-dedup logic that was previously inline in useSearchBulkActions.ts. The refactor is clean but the extracted function takes 5 callback params via dependency injection, which makes the API surface relatively complex. Consider whether a simpler interface (passing pre-resolved data) would be clearer.


Positives

  • Excellent test coverage: 426 lines of new SearchSelectionUtilsTest.ts, 287 lines in ReportUtilsTest.ts, and 445 lines in canEditFieldOfMoneyRequestTest.ts covering forwarded detection, policy mutation, DEW guards, reset actions, and partial hydration.
  • Conservative failure mode: The known limitation (over-hiding when personal details are missing) errs on the side of safety.
  • hasCurrentForwardingWorkflowAction() correctly uses sorted report actions and distinguishes forwarding actions from reset actions, preventing false positives from stale approval history.
  • The route-drift fix was properly reverted per reviewer request, keeping scope focused.

Nits

  1. MOVE_FORWARDING_WORKFLOW_ACTIONS and MOVE_FORWARDING_RESET_ACTIONS are module-level Set constants — good for performance, but the naming could be more descriptive (e.g., FORWARDING_ADVANCEMENT_ACTIONS vs FORWARDING_CYCLE_RESET_ACTIONS).

  2. In shouldBlockForwardedMoveFromSearchSnapshot, the !report.submitted guard is checking for a truthy submitted field — confirm this is always populated for processing reports in the Search snapshot.


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 SearchSelectionUtils snapshot-based check is necessary beyond what shouldTreatAsForwardedForMoveExpense alone provides in the bulk action path.

Comment on lines +72 to +77
if (!transaction.canChangeReport) {
return false;
}

const policy = getPolicyForPolicyID?.(transaction.policyID ?? transaction.report?.policyID);
return !shouldBlockForwardedMoveFromSearchSnapshot(transaction.report, policy, resolveSubmitToAccountID, getLoginForAccountID);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
     }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. It drops the workflow-cycle guard from hasCurrentForwardingWorkflowAction(). That guard prevents old
    APPROVED / FORWARDED history from blocking move after reset actions like UNAPPROVED, REOPENED, RETRACTED,
    etc.

  2. It does not copy the full Search snapshot condition. The Search guard also checks that the report is submitted,
    nextStep is WAITING_TO_APPROVE, and nextStep.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:

  • ReportUtils uses report action history to protect the core move eligibility path after report actions are
    available.
  • SearchSelectionUtils uses 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 :

  1. Keep isReportOutstanding() unchanged.
  2. Move the forwarded-like Search snapshot check into canEditFieldOfMoneyRequest() under the fieldToEdit === CONST.EDIT_REQUEST_FIELD.REPORT path.
  3. Keep the check narrow by requiring the same Search snapshot signals we use now:
    • processing expense report
    • submitted report
    • nextStep.messageKey === WAITING_TO_APPROVE
    • nextStep.actorAccountID === managerID
    • submitToAccountID !== managerID
    • submit-to approver forwards / over-limit-forwards to the current manager
  4. Then simplify SearchSelectionUtils so it only trusts transaction.canChangeReport plus 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that plan @mavrickdeveloper. Btw, why do we need to check

nextStep.messageKey === WAITING_TO_APPROVE
nextStep.actorAccountID === managerID

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I'm imagining submitToAccountID === managerID would do the same

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mismatch
  • nextStep.messageKey === WAITING_TO_APPROVE: confirms the report is currently in an approval step
  • nextStep.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

&
https://github.com/mavrickdeveloper/Expensify/blob/7405c95548fea3b093c24a314c715a48ab7593c6/src/components/Search/index.tsx#L772-L779

&
https://github.com/mavrickdeveloper/Expensify/blob/7405c95548fea3b093c24a314c715a48ab7593c6/src/components/Search/index.tsx#L827-L834

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait, ONYXKEYS.COLLECTION.NEXT_STEP is truly deprecated one lol

[ONYXKEYS.COLLECTION.NEXT_STEP]: OnyxTypes.ReportNextStepDeprecated;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go with that plan @mavrickdeveloper. Hopefully, it makes everything cleaner

@mavrickdeveloper
Copy link
Copy Markdown
Contributor Author

@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:

  • ReportUtils protects the core move eligibility path once report action history is available. That is why it uses
    the workflow-action-cycle check, so we avoid false positives from policy approver mutations or stale forwarded/
    approved history after reset actions.
  • SearchSelectionUtils protects the Search bulk-menu path before full REPORT_ACTIONS hydration, where the Search
    snapshot already has enough nextStep / forwarding data to know the selected report is forwarded-like.

That said , I agree we should document this distinction in code so future readers do not treat the two checks as accidental duplication.

the SearchTransactionsChangeReport.tsx null guard is separate from the forwarded-report fix. It is harmless, but I can call it out as defensive cleanup if needed.

Comment thread src/libs/ReportUtils.ts Outdated
* has already been advanced away from the original submit-to approver.
*/
function hasCurrentForwardingWorkflowAction(reportID: string): boolean {
const reportActions = getSortedReportActions(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mavrickdeveloper

This comment was marked as outdated.

@mavrickdeveloper
Copy link
Copy Markdown
Contributor Author

@hoangzinh Update ready for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants