Hide role-change options for the Authorized Payer - #97272
Conversation
…er resolution Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot the fix works for the reported case, "Make member" is now correctly hidden for the payer. Two things to add:
|
…imburserEmail tests Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
…d regression test Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
…o claude-hidePayerRoleOptions Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
Both addressed and pushed to 1. const reimburserEmail = getReimburserEmail(policy);
const isReimburser = !!reimburserEmail && reimburserEmail === memberLogin;
2. Added a guard-level regression test in I verified it has teeth: temporarily reverting the Members-page guard to the old Checks run locally
|
The failing check is unrelated to this PR — no code fix neededTwo checks are red, and neither warrants a code change to this branch: 1. This PR only touches Recommended action: re-run the failed jobs (Re-run failed jobs on the test run). I attempted the re-run myself but don't have permission to trigger workflow re-runs on this repo. 2. Evidence
Since the only substantive failure is an unrelated flake, I'm not pushing any changes. Just re-run the failed jobs and this should go green. |
Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
Merged
|
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@MelvinBot Please replace the TestsPrecondition: A Collect workspace owned by Account A, with Account B invited and set to the Admin role.
Note: steps 1–3 deliberately do not connect a bank account. Without a verified bank account, enabling payments puts the workspace into Indirect (manual) reimbursement, which is the mode the old guard failed to recognize — so this setup exercises the bug directly. Steps 10–13 confirm the fix is not over-restrictive and that role changes still succeed for a non-payer. Offline tests
QA StepsSame as tests. |
|
Done ✅ — I replaced the
The old placeholder text and the "must be filled out / [No QA]" reminders are gone (0 One thing I left as-is: I couldn't check off the checklist boxes or fill the per-platform Screenshots/Videos sections — those still need you to complete them before marking the PR ready for review. |
|
|
Done ✅ — updated the PR description:
Verified 0 unchecked boxes remain. The |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp
Android: mWeb ChromeiOS: HybridAppScreen.Recording.2026-07-29.at.01.17.06.moviOS: mWeb SafariScreen.Recording.2026-07-29.at.01.18.23.movMacOS: Chrome / SafariScreen.Recording.2026-07-29.at.01.15.02.mov |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #97027 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
@MelvinBot mark the PR as ready |
|
@abzokhattab 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] |
|
Done ✅ — I marked this PR as Ready for review (it's no longer a draft). Heads up: the PR Author Checklist boxes and the per-platform Screenshots/Videos sections are still empty, so a reviewer may ask you to complete those. |
![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: d6ae0a9588
ℹ️ 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".
| const isReimbursementEnabled = policy?.reimbursementChoice === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES; | ||
| const hasAtLeastOnePayer = isReimbursementEnabled && policy?.achAccount?.reimburser ? selectedEmployees.includes(policy?.achAccount?.reimburser) : false; | ||
| const reimburserEmail = getReimburserEmail(policy); | ||
| const hasAtLeastOnePayer = !!reimburserEmail && selectedEmployees.includes(reimburserEmail); |
There was a problem hiding this comment.
When the selected Authorized Payer has a non-admin payer-capable role (for example paymentsAdmin, which WorkspaceWorkflowsPayerPage allows because it has WORKFLOWS_PAYMENTS write access), this new payer flag suppresses the downgrade/special-role options but the makeAdmin option below is still added whenever hasAtLeastOneNonAdminRole && canAssignElevatedRoles. That leaves a bulk role-change path for the payer even though the member details page now makes payer roles non-interactive, so admins can still change the Authorized Payer's role from the members table instead of going through Workflows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot Codex is right about the makeAdmin gap — please fix it here.
adminOption is the only one of the six role options that was never gated on hasAtLeastOnePayer:
if (hasAtLeastOneNonAdminRole && canAssignElevatedRoles) {
options.push(adminOption);
}Add the guard:
if (hasAtLeastOneNonAdminRole && !hasAtLeastOnePayer && canAssignElevatedRoles) {
options.push(adminOption);
}PAYMENTS_ADMIN is the only non-admin role with write access to WORKFLOWS_PAYMENTS, so it's the one role that can hold the Authorized Payer role without already being an admin — which makes it the sole path to this. I confirmed on this branch that selecting such a payer still shows Make workspace admin. Without this, the members table contradicts the member details page, which this PR just made non-interactive for all payer role changes.
Please also extend the new test in tests/ui/WorkspaceMembersTest.tsx to cover a PAYMENTS_ADMIN payer and assert Make workspace admin is hidden — the current test uses an admin payer, so hasAtLeastOneNonAdminRole is already false and it can't catch this.
There was a problem hiding this comment.
Done — pushed in d16e3b4.
Fix: added the missing !hasAtLeastOnePayer guard to adminOption so it's now gated exactly like the other five role options:
src/pages/workspace/WorkspaceMembersPage.tsx:625
if (hasAtLeastOneNonAdminRole && !hasAtLeastOnePayer && canAssignElevatedRoles) {
options.push(adminOption);
}Test: added a case in tests/ui/WorkspaceMembersTest.tsx with a PAYMENTS_ADMIN member set as the Authorized Payer (via reimburser). Because their role isn't ADMIN, hasAtLeastOneNonAdminRole is true, so this exercises the exact gap the existing admin-payer test couldn't — it selects that payer, opens the dropdown, and asserts Make workspace admin is not on screen (while Remove still is).
WorkspaceMembersTest locally in this environment (the test runner is blocked here), so I couldn't execute it before pushing. The new test is closely modeled on the adjacent policy.reimburser payer test in the same file, but please rely on CI (or a local run) to confirm it's green before marking the PR ready.
The adminOption was the only role-change option not guarded by
!hasAtLeastOnePayer, so a PAYMENTS_ADMIN who is the Authorized Payer was
still offered Make workspace admin, contradicting the now non-interactive
member details page. Add the guard and extend WorkspaceMembersTest to
cover a PAYMENTS_ADMIN payer.
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
@abzokhattab 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] |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.46-0 🚀
|
Help site review — changes required ✅This PR hides the role-change options for the workspace's Authorized Payer — the Members bulk-select dropdown no longer offers Make member / Make workspace admin / etc. for the payer, and on the member details page the Role row becomes non-interactive with the hint "Role can't be changed because this member is a payer on this workspace." The help site documents how to Change a User's Role but never mentioned that the payer's role is locked, so this is a real documentation gap surfaced by your change. Draft help site PR: #97392 It adds a note to the Change a User's Role section of Managing Workspace Members explaining the payer's role can't be changed until a different Payer is assigned in Workflows > Payments. Applied the @abzokhattab, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.46-10 🚀
Bundle Size Analysis (Sentry): |
4 participants
Explanation of Change
When an admin is set as the workspace's Authorized Payer, bulk-selecting that admin in the Members list and opening the actions dropdown was incorrectly offering the "Make member" option (along with the auditor / card-admin / people-admin / payments-admin options). Selecting "Make member" tried to demote the payer, which the backend rejects with "An error occurred while updating the workspace. Please try again."
The root cause was that the
hasAtLeastOnePayerguard inWorkspaceMembersPageresolved the payer far more narrowly than the rest of the app: it only counted someone as the payer whenreimbursementChoice === REIMBURSEMENT_YESand their email was inachAccount.reimburser. The canonical resolution inPolicyUtils.isPolicyPayeris broader — it also honorsREIMBURSEMENT_MANUAL(Indirect), readspolicy.reimburserfirst, and falls back topolicy.owner. So whenever the payer was defined through any other path, the guard collapsed tofalseand the elevated/demote options were wrongly shown.This change extracts that reimburser-email resolution out of
isPolicyPayerinto a sharedgetReimburserEmail(policy)helper inPolicyUtils, has bothisPolicyPayerand the Members page consume it, and rewrites the guard as!!reimburserEmail && selectedEmployees.includes(reimburserEmail). This keeps the Members page in sync with the canonical payer definition so the invalid options are hidden for the Authorized Payer regardless of how the payer is configured. Unit tests forgetReimburserEmailcover each payer-definition path.Fixed Issues
$ #97027
PROPOSAL: #97027 (comment)
Tests
Precondition: A Collect workspace owned by Account A, with Account B invited and set to the Admin role.
Note: steps 1–2 deliberately do not connect a bank account. Without a verified bank account, setting a Payer puts the workspace into Indirect (manual) reimbursement, which is the mode the old guard failed to recognize — so this setup exercises the bug directly. Steps 9–12 confirm the fix is not over-restrictive and that role changes still succeed for a non-payer.
Offline tests
QA Steps
Same as tests.
PR 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
iOS: mWeb Safari
MacOS: Chrome / Safari