bring back byass approver for manager - #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.
Updated. Can you check?
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.
There was a problem hiding this comment.
Cc @trjExpensify , should we drop assigning approver in this case?
3 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 manager of the report (or you're the approver and admin)
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