Show Bypass approver option to the workspace admin when they are the report's current approver - #98717
Conversation
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.
|
|
PR is ready. I'll upload the screenshots tomorrow. github is down.... |
|
@bernhardoj 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b87753462
ℹ️ 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".
|
Short answer: it won't map cleanly here, because this page is a bulk flow while the hook is scoped to a single report.
That's why the If the goal is to cut the duplicated arg-assembly, a better fit than the hook/provider would be extracting a small plain helper (e.g. |
| } | ||
|
|
||
| if (isDelegateAccessRestricted) { | ||
| showDelegateNoAccessModal(); |
There was a problem hiding this comment.
We check for delegate access here, but we don't do the same in DynamicReportChangeApproverPage
There was a problem hiding this comment.
Just done reading through the AI reviews, this is the same as #98717 (comment). Is it really out of scope? This PR adds the delegate access check before reassigning and approving the report.
Before this PR: bypass approver only reassigns the approver; no delegate access check. This means a delegate can reassign the approver, bulk or individual.
After this PR: bypass approver also approves the report when the current user is the manager. Bulk bypass approver on the search page is totally prevented for a delegate, while individual bypass approver allows delegate to reassign the approver, but not approving the report.
There was a problem hiding this comment.
I'm thinking about removing this check because we don't have this before 🤔 Wdyt
There was a problem hiding this comment.
If we remove the delegate check (so it works like before), we are still left with confirmApproval which prevents approving as a delegate and shows the restricted access modal.
I think it would be weird if we also show the restricted access modal when doing bulk approval before approveMoneyRequest because there would be n calls to the showDelegateNoAccessModal function. Wdyt of just returning early? So, bypass approver will
- reassign the approver (allowed for delegate)
- approve (not allowed for delegate; return early; nothing happens; approver is successfully reassigned in step 1, so when the user presses Approve manually, they will see the restricted modal)
But this means we need to customize useConfirmApproval for this case (bypass approver).
There was a problem hiding this comment.
yeah, I think that works, early return is better since we shouldn't show delegate modal inside the bulk actions
There was a problem hiding this comment.
Ah right, I see. How is that screenshot possible? Doesn't clicking it when you're the report's current approver final approve the report? So then it's in the Paid state and you can't Change approver anymore on a report in that state? 🤔
There was a problem hiding this comment.
Oh I thought we will approve it only and the report will move to approved state. So we want to paid the expense instead when the manager choose the bypass customer option?
There was a problem hiding this comment.
Ah sorry, I got myself twisted in repro'ing the scenario. 😅 I think one way to handle it is to not show Bypass approver to the admin when they are the managerID of the report and it's in the Approved state.
There was a problem hiding this comment.
That works fine, but I think it's a little bit weird that I can bypass the approver by reassigning the approver to me multiple times. If that's expected or out of scope, then we are good.
@bernhardoj How did you do this? AFter approving the Change approver option will be hide 🤔
Screen.Recording.2026-08-22.at.22.07.45.mov
There was a problem hiding this comment.
Oh, it's when you are a delegate without access, so it only reassigns without approving.
|
I'm a bit confused by the title of this PR and its test steps. It's specifically supposed to only be for the case where the report's current manager is an admin:
|
hungvu193
changed the title
Updated. Does this look better? Prerequisite: You're the admin of the workspace also the approver of current report.
|
|
Yep, much better! 👍 |
4 participants
Explanation of Change
Bring back the By pass approver option for the manager. When manager choose by pass option, we will approve the money request
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/671638
PROPOSAL: N/A
Tests
Same as QA steps
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 the workspace also the approver of current report.
Bypass approveris visiblePR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
Screen.Recording.2026-08-18.at.10.23.41.mov
iOS: mWeb Safari
Screen.Recording.2026-08-18.at.10.18.12.mov
MacOS: Chrome / Safari
Screen.Recording.2026-08-18.at.10.01.35.mov