Skip to content

[CFI] Add reassign approver option - #99251

Open
hungvu193 wants to merge 16 commits into
Expensify:mainfrom
hungvu193:feat-add-reassign-approver
Open

hungvu193 wants to merge 16 commits into
Expensify:mainfrom
hungvu193:feat-add-reassign-approver

Conversation

@hungvu193

@hungvu193 hungvu193 commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Explanation of Change

Add a new Reassign approver option for admin. When an admin selects Reassign approver and selects a workspace member to reassign to:

  1. ​We mark the current approver's leg as satisfied​ (approved/skipped on the BE), so the live recompute doesn't re-include them.
  2. ​We assign the report to the admin-chosen next approver.​ When that person approves, the chain re-derives from the submitter's workflow and continues normally to the next approver and beyond.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/665935
PROPOSAL: N/A

Tests

Same as QA steps.

  • Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

// TODO: These must be filled out, or the issue title must include "[No QA]."
Prerequisite:

  • You're the admin of workspace.
  1. Create an expense > Submit.
  2. Go to More > Change Approver > Reassign Approver.
  3. Verify that you can reassign the approver.
  • 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
  • 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 followed proper code patterns (see Reviewing the code)
    • 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 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)
  • 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)
  • 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.
  • 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
Screen.Recording.2026-09-19.at.17.40.58.mov
Android: mWeb Chrome
Screen.Recording.2026-09-19.at.17.36.43.mov
iOS: Native
Screen.Recording.2026-09-19.at.17.25.20.mov
iOS: mWeb Safari
Screen.Recording.2026-09-19.at.17.27.26.mov
MacOS: Chrome / Safari
Screen.Recording.2026-08-23.at.00.12.52.mov

@melvin-bot

melvin-bot Bot commented Aug 22, 2026

Copy link
Copy Markdown

Hey, I noticed you changed src/languages/en.ts in a PR from a fork. For security reasons, translations are not generated automatically for PRs from forks.

If you want to automatically generate translations for other locales, an Expensify employee will have to:

  1. Look at the code and make sure there are no malicious changes.
  2. Run the Generate static translations GitHub workflow. If you have write access and the K2 extension, you can simply click: [this button]

Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running:

npx bun ./scripts/generateTranslations.ts --help

Typically, you'd want to translate only what you changed by running npx bun ./scripts/generateTranslations.ts --compare-ref main

@codecov

codecov Bot commented Aug 22, 2026

Copy link
Copy Markdown

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/SCREENS.ts 100.00% <ø> (ø)
...avigation/linkingConfig/RELATIONS/SEARCH_TO_RHP.ts 100.00% <ø> (ø)
src/libs/Navigation/linkingConfig/config.ts 62.50% <ø> (ø)
src/libs/ReportActionsUtils.ts 82.20% <100.00%> (+0.10%) ⬆️
src/libs/actions/IOU/Hold.ts 93.39% <ø> (ø)
src/libs/actions/IOU/ReportWorkflow.ts 82.54% <100.00%> (+0.04%) ⬆️
src/pages/Search/SearchAddApproverPage.tsx 100.00% <100.00%> (+23.25%) ⬆️
src/pages/Search/SearchReassignApproverPage.tsx 100.00% <100.00%> (ø)
src/ROUTES.ts 29.17% <0.00%> (-0.03%) ⬇️
src/libs/ReportUtils.ts 87.43% <90.90%> (+<0.01%) ⬆️
... and 5 more
... and 3 files with indirect coverage changes

@hungvu193

Copy link
Copy Markdown
Contributor Author

Note

We decided to keep the copy: changed the approver, skip...

@hungvu193 hungvu193 changed the title [HOLD][CFI] Add reassign approver option [CFI] Add reassign approver option Sep 12, 2026
@hungvu193
hungvu193 marked this pull request as ready for review September 13, 2026 11:42
@hungvu193
hungvu193 requested review from a team as code owners September 13, 2026 11:42
@melvin-bot
melvin-bot Bot requested review from dylanexpensify and mananjadhav and removed request for a team September 13, 2026 11:43
@melvin-bot

melvin-bot Bot commented Sep 13, 2026

Copy link
Copy Markdown

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

@hungvu193

Copy link
Copy Markdown
Contributor Author

This still need the BE changes from https://github.com/Expensify/Auth/pull/24465, but the logic in this PR is ready for review.

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

Copy link
Copy Markdown

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

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/pages/DynamicReportChangeApproverPage.tsx Outdated
Comment thread src/pages/ReportReassignApproverPage.tsx Outdated
Comment thread src/pages/ReportReassignApproverPage.tsx Outdated
Comment thread src/pages/DynamicReportChangeApproverPage.tsx Outdated
@mananjadhav

Copy link
Copy Markdown
Collaborator

Reviewing the PR.

@garrettmknight
garrettmknight requested review from garrettmknight and removed request for dylanexpensify September 14, 2026 13:43
@garrettmknight

Copy link
Copy Markdown
Contributor

@mananjadhav can you prioritize this review? We have a customer who wants this and I'd like to get it out ASAP.

Comment thread src/libs/ReportUtils.ts Outdated
Comment thread src/pages/ReportReassignApproverPage.tsx
Comment thread src/pages/DynamicReportChangeApproverPage.tsx Outdated
@mananjadhav

Copy link
Copy Markdown
Collaborator

@MelvinBot Review the PR.

@mananjadhav

Copy link
Copy Markdown
Collaborator

@codex Review

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

Copy link
Copy Markdown

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: 69e16c06ad

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/pages/Search/SearchAddApproverPage.tsx Outdated
@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 Reviewed at 69e16c0. The reassignment plumbing looks right and the three Codex P2s are genuinely fixed, but I found one state-cleanup bug, two guard mismatches that dead-end the user, and four of your earlier comments still open.

I didn't exercise the flow live — the backend half this depends on isn't deployed yet, so the reassign path can't be verified end-to-end.

Must fix

1. Successful reassign leaves the action stuck in a pending state. addReportApprover's successData clears pendingAction and errors but not isOptimisticAction, and ReportActionItem.tsx:588 falls back to action.isOptimisticAction ? ADD : undefined. So after the write succeeds, the "changed the approver to X, skipped Y" line keeps rendering as a pending add. assignReportToMe clears it at ReportWorkflow.ts:1758; add the same line at ReportWorkflow.ts:1891. (Still open from my earlier comment.)

2. Reassign row is offered for pending-delete policies, then 404s. hasAdminPermission at SearchChangeApproverPage.tsx:210-213 omits isPendingDeletePolicy, but canReassignAllReports at SearchAddApproverPage.tsx:197-200 includes it and renders NotFound. An admin of a workspace pending deletion picks "Reassign approver" and lands on a blank 404 RHP. Add && !isPendingDeletePolicy(policy) to hasAdminPermission.

3. The new admin gate makes auto-apply reachable only for users who are being 404'd. DynamicReportChangeApproverPage.tsx:146 already 404s non-admins, so past that gate isPolicyAdmin(policy) at line 122 is always true and approverTypes.length is never 1 — the approverTypes.length === 1 branch at line 140 and the hasNavigatedAwayRef rename are dead for admins.

The branch still fires for non-admins, because that useEffect sits above the NotFoundPage early return. Deep-link a non-admin to r/:reportID/change-approver: they see NotFound, the effect auto-applies "Add approver", and they get navigated to ReportAddApproverPage — which only checks money-request and pending-deletion, so the picker actually works for them. Move the early return above the effect (or drop the now-redundant isPolicyAdmin check and keep the gate in one place).

Should fix

4. ReportReassignApproverPage rolls its own SelectionList. ReportAddApproverPage.tsx:141 uses ApproverSelectionList; the new page uses a bare SelectionList at line 182. That loses the member search input (auto-enabled past STANDARD_LIST_ITEM_LIMIT), the empty state, and the "No results found" header. When every member filters out — submitter blocked by preventSelfApproval, plus the current manager — you get a lone header line and a disabled Save with no explanation. (Still open.)

5. Bulk reassign hides anyone who approves any selected report. SearchAddApproverPage.tsx:102-107. Select R1 (manager Alice) and R2 (manager Bob) and both disappear, even though the submit loop at line 144 already skips reports where the candidate is the manager. Add approver keeps them. Worst case the reassign list is empty while Add's is not. Exclude only people who are manager of every selected report — isApproverOfAllReports above already computes that. (Still open.)

6. Two product questions I still need answers on before approving:

  • Plan gating: Add approver forces a Control upgrade at line 79. Reassign has no plan gate, so every Collect/Team admin now sees it. Intended?
  • Does the skipped approver get notified?

7. A stale reassign URL can still submit. shouldShowNotFoundView at ReportReassignApproverPage.tsx:72-77 has no status check, so an approved or reimbursed report still renders the picker and writes optimistic managerID/nextStep. Adding isProcessingReport(report) would close it. Lower priority — the Dynamic page has the same gap today, so this isn't a regression.

Nits

  • Move SearchApproverPage out of SearchAddApproverPage.tsx into its own file — right now the shared component lives in the file named after one of its two variants, and SearchAddApproverPageTranslateTest.tsx now tests the reassign page too.
  • previousApproverID comes from report.managerID. When that's undefined the API still receives isReassignment: true but the optimistic originalMessage drops the flag (ReportUtils.ts:9170). Harmless today, but the two sides disagree.
  • reasoning and actorAccountID on OriginalMessageReassignApprover are added but never read.
  • Hold.ts:635 adds REASSIGN_APPROVER to the copied-action list, but this flow emits REROUTE/TAKE_CONTROL. Intentional?
  • Copy: reassignApproverPageHeader ("…and then follow the remaining approval workflow") reads as an instruction to the admin. addApprover.subtitle says "…before we route through the rest of the approval workflow" — worth matching.

Codex comment status

You asked for an update on these, so here's where they stand:

Codex finding Status
P1 — remove useCallback from shouldNativeToUpgradePage Stale/invalid. No such symbol exists in the file, and the useCallback/useMemo there predate this PR. React Compiler Compliance passes.
P2 — enforce admin authorization on the reassign route FixedisPolicyAdmin + isPendingDeletePolicy + empty-policy checks are in shouldShowNotFoundView.
P2 — exclude ineligible replacement approvers FixedisAllowedToApproveExpenseReport plus the current-manager exclusion.
P2 — show reassignment to admins who cannot self-approve Fixed — the gate is isPolicyAdmin now, not isAllowedToApproveExpenseReport.

Test coverage on the new page is good now, and CI is green apart from the two checklist checks.

@hungvu193

Copy link
Copy Markdown
Contributor Author

Working on review comments again.

@melvin-bot

melvin-bot Bot commented Sep 16, 2026

Copy link
Copy Markdown

I could not review this diff, so I did not approve the workflow runs. An Expensify employee will review it and approve them manually.

@melvin-bot

melvin-bot Bot commented Sep 17, 2026

Copy link
Copy Markdown

I could not review this diff, so I did not approve the workflow runs. An Expensify employee will review it and approve them manually.

@mananjadhav

Copy link
Copy Markdown
Collaborator

@hungvu193 Can you please confirm once this is ready for review again?

@melvin-bot

melvin-bot Bot commented Sep 17, 2026

Copy link
Copy Markdown

I could not review this diff, so I did not approve the workflow runs. An Expensify employee will review it and approve them manually.

@hungvu193

Copy link
Copy Markdown
Contributor Author

@hungvu193 Can you please confirm once this is ready for review again?

Yes, this is ready for your review again.

Comment thread src/pages/Search/SearchChangeApproverPage.tsx Outdated
Comment thread src/pages/Search/SearchApproverPage.tsx
Comment thread src/pages/DynamicReportChangeApproverPage.tsx Outdated
Comment thread src/pages/Search/SearchApproverPage.tsx
Comment thread src/pages/Search/SearchApproverPage.tsx Outdated
@hungvu193

Copy link
Copy Markdown
Contributor Author

@mananjadhav please do review in bulk please. Currently it's spamming my notification and email

@mananjadhav mananjadhav left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Last few comments, the rest looks good. Starting to test today.

@mananjadhav

Copy link
Copy Markdown
Collaborator

@mananjadhav please do review in bulk please. Currently it's spamming my notification and email

Noted.

I was checking the Author checklist and the screencasts for other platforms are missing. Can you please upload them?

@hungvu193

Copy link
Copy Markdown
Contributor Author

Yes. I'm preparing screenshot and videos

@hungvu193

Copy link
Copy Markdown
Contributor Author

My IOS simulator is broken after 26.5 upgrade 🥶. Still working on it.

@melvin-bot

melvin-bot Bot commented Sep 19, 2026

Copy link
Copy Markdown

I could not review this diff, so I did not approve the workflow runs. An Expensify employee will review it and approve them manually.

@hungvu193

Copy link
Copy Markdown
Contributor Author

Updated all screenshots and videos.

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.

4 participants