Allow promoting the Authorized Payer to Workspace Admin - #98123
Allow promoting the Authorized Payer to Workspace Admin#98123MelvinBot wants to merge 9 commits into
Conversation
Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
I analyzed the failing checks — there's nothing to fix in code. All 35 code/test/lint checks (typecheck, ESLint, Bun tests, Jest
Both are expected to be red on a draft PR that hasn't been finished and reviewed yet, and neither indicates a bug in the change. Once you complete the checklist/test sections and get a review, both will go green. |
| const phoneNumber = getPhoneNumber(details); | ||
| const reimburserEmail = getReimburserEmail(policy); | ||
| const isReimburser = !!reimburserEmail && reimburserEmail === memberLogin; | ||
| // An Authorized Payer (reimburser) must stay an admin, so lock the Role row only once they are already an Admin — every |
There was a problem hiding this comment.
| // An Authorized Payer (reimburser) must stay an admin, so lock the Role row only once they are already an Admin — every | |
| // An Authorized Payer (reimburser) must be either an admin or a payments admin. Lock the Role row only once they are already an Admin — every |
@MelvinBot Apply this suggestion
There was a problem hiding this comment.
Applied in ada29a1 — the comment now reads "An Authorized Payer (reimburser) must be either an admin or a payments admin."
| // The Authorized Payer (reimburser) must remain an admin, so they may only be promoted to Admin — never demoted to another role. | ||
| const reimburserEmail = getReimburserEmail(policy); | ||
| const isReimburser = !!reimburserEmail && reimburserEmail === memberLogin; | ||
| const allowedRoles = isReimburser ? [CONST.POLICY.ROLE.ADMIN] : undefined; |
There was a problem hiding this comment.
@MelvinBot why are we not allowing payments admin here?
There was a problem hiding this comment.
Good catch — Payments Admin is a valid payer, so restricting allowedRoles to [ADMIN] was too strict. Fixed in d27cd99: allowedRoles is now [ADMIN, PAYMENTS_ADMIN], so a payer can be Admin or Payments Admin and switch between the two. Details are in the other thread.
| // Guard the direct-navigation path: a reimburser can only be promoted to Admin, so reject any demotion. | ||
| if (isReimburser && value !== CONST.POLICY.ROLE.ADMIN) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
@MelvinBot What if a reimburser is an admin and being demoted to payments admin?
There was a problem hiding this comment.
Good catch — this needs a decision, because "Payments Admin" is a distinct role from "Admin" (ROLE.PAYMENTS_ADMIN = 'paymentsAdmin' vs ROLE.ADMIN = 'admin'), and the PR currently treats only a full Admin as a valid payer.
What the PR does today: an Admin reimburser is blocked from being changed to Payments Admin, in two places:
- The Role row is locked on the member details page whenever the reimburser is already an Admin —
WorkspaceMemberDetailsPage.tsx:152. - Via the direct-navigation path, the guard rejects any target that isn't Admin —
WorkspaceMemberDetailsRolePage.tsx:61.
So the demotion you're describing can't happen through the UI right now. Whether that's correct depends on one backend fact that I couldn't verify from here (no Web-Expensify access in this environment):
- If a Payments Admin can be the Authorized Payer (which the role's own purpose — "Manage workflow payments" — and the comment I just applied both suggest), then this PR is too strict:
allowedRolesshould includePAYMENTS_ADMIN, the line 61 guard should allow Admin↔Payments Admin, andisReimburserAdminshould lock only against roles that drop pay capability (Member/Auditor/etc.), not against Payments Admin. - If the payer must be a full Admin — which is how I read the backend guard cited in the issue's root-cause section (
sharePolicyrejects a reimburser whose role!== ROLE_ADMIN) — then blocking Admin→Payments Admin is correct (allowing it would let the frontend make a change the backend rejects, re-breaking reimbursements), but then the comment I just applied is misleading and should be reworded.
The whole PR hinges on this: does sharePolicy accept a Payments Admin as the reimburser, or must they be a full Admin? (Granular admin roles are often stored as admin + a scope on the backend, in which case a Payments Admin would pass !== ROLE_ADMIN — but I can't confirm that here.)
Confirm which it is and I'll adjust:
- Payments Admin is a valid payer → I'll widen
allowedRoles+ the guard to includePAYMENTS_ADMINand relax the lock condition. - Must be full Admin → current logic stays, and I'll reword the comment so it doesn't imply Payments Admin is allowed.
There was a problem hiding this comment.
@MelvinBot Yes, Payments Admin is a valid payer
There was a problem hiding this comment.
Fixed in d27cd99 — Admin↔Payments Admin is now allowed, so an Admin payer can be changed to Payments Admin (and vice versa), while demotions to non-payer roles stay blocked.
What changed:
WorkspaceMemberDetailsRolePage.tsx:46—allowedRolesfor a reimburser is now[ADMIN, PAYMENTS_ADMIN], and the direct-navigation guard at line 61 rejects only targets that aren't one of those two.WorkspaceMemberDetailsPage.tsx— dropped theisReimburserAdminfull-row lock (and its now-unused hint). A payer always has a valid alternate payer role available, so the Role row stays interactive; the role page enforces the Admin/Payments Admin restriction.WorkspaceMembersPage.tsx— the bulk actions had the same over-strictness: they hid all role changes for a selected payer. Now "Make workspace admin" and "Make payments admin" are offered for a payer (both valid payer roles); the demotions stay gated.
Updated the unit tests in WorkspaceMemberDetailsPageTest.tsx and WorkspaceMembersTest.tsx to cover the new behavior; both suites pass.
Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
👋
All actual code checks are green (Bun tests, To get to green: fill out the PR Author Checklist and get a review/approval — no code changes are needed from me. |
|
👋
All the actual code checks (Bun tests, To get to green: complete the checklists and get a review/approval. No code changes are needed from me. |
|
👋
Every actual code check is green (Bun tests, |
|
All code checks (Bun tests, |
Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
👋
All code checks that have finished are green, and the rest (Bun tests, To get to green: fill out the PR Author Checklist and get a review/approval. No code changes are needed from me. |
|
👋
Every actual code check is green (Bun tests, |
|
@MelvinBot Fix conflicts , add testing steps, mark all checkboxes as checked in the description, then mark this as ready for review. |
…rRolePromotion Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com> # Conflicts: # src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx
|
🤖 Done — conflicts resolved, testing steps added, checklist completed, and the PR is now Ready for review. 1. Merge conflicts fixed — merged
The PR is now 2. Verified locally (relevant AI tests for the touched TSX):
3. Testing steps added — filled in the 4. Checklist — checked every box as requested, and marked the PR ready for review.
|
|
No code fix is needed — every substantive check on this PR is now green (tests jobs 1–8, The only check reporting a failure is What's actually needed: an independent reviewer's approval. Once that lands, this check will pass. Full check status
|
![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: 124be1f76f
ℹ️ 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".
| // The Authorized Payer (reimburser) is intentionally NOT locked out here: they can still be promoted/changed to another | ||
| // valid payer role (Admin or Payments Admin). WorkspaceMemberDetailsRolePage restricts the offered roles accordingly. | ||
| const canEditSelectedMemberRole = !isSelectedMemberOwner && !isSelectedMemberCurrentUser && canManageSelectedMemberRole; |
There was a problem hiding this comment.
In a Team workspace, Payments Admin is not a valid role (canMemberAssignRole() filters Control-only roles), so a non-owner Authorized Payer who is already an Admin has no valid role change. Removing the payer check here nevertheless makes the Role row interactive; opening it displays only the already-selected Admin role and no actionable alternative. Limit the unlock to payers that can actually transition—such as Payments Admin payers being promoted, or Admin payers on Control workspaces—or retain the read-only state otherwise.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot does this make sense? If yes, address it.
There was a problem hiding this comment.
Yes, this makes sense — it's a real (minor) dead-end, and I've addressed it in 6a6bcaf.
Why it's valid: PAYMENTS_ADMIN is a control-only role (PolicyUtils.ts:199), and canMemberAssignRole returns false for control-only roles on non-corporate policies (PolicyUtils.ts:237). So on a Team workspace an Admin payer's only assignable payer role is the Admin role they already hold — unlocking the Role row just opened a page with no actionable alternative.
Fix: the payer's Role row is now interactive only when there's another payer role they can actually move to (WorkspaceMemberDetailsPage.tsx:152-155):
- non-admin payer → promotable to an assignable payer role (Admin, or Payments Admin on Control) ✅
- Admin/Payments Admin payer on a Control workspace → can switch between the two ✅
- Admin payer on a non-Control workspace → no assignable alternative, so the row stays read-only (rendered like the owner's row: not dimmed, no caret) ✅
Added a unit test covering the non-Control admin-payer case (WorkspaceMemberDetailsPageTest.tsx). All 10 tests in the file pass, and typecheck/lint are clean.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const memberLogin = personalDetails?.[accountID]?.login ?? ''; | ||
| const member = policy?.employeeList?.[memberLogin]; | ||
| const canManageSelectedMemberRole = canMemberAssignRole(policy, currentUserLogin, member?.role); | ||
| // The Authorized Payer (reimburser) must stay a valid payer, so restrict them to Admin or Payments Admin — the two roles that can pay. |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
This comment uses an em dash (—), which the plain-comment-style guideline calls out as a stylistic tic to avoid. Comments should read like plain, natural sentences.
Replace the em dash with a plain sentence break, e.g.:
// The Authorized Payer (reimburser) must stay a valid payer, so restrict them to Admin or Payments Admin, which are the two roles that can pay.Reviewed at: 124be1f | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Addressed in 43f0f1d — replaced the em dash with a plain parenthetical: WorkspaceMemberDetailsRolePage.tsx:43 now reads "…so restrict them to the roles that can pay (Admin or Payments Admin)."
| // The Authorized Payer (reimburser) must stay a valid payer, so restrict them to Admin or Payments Admin — the two roles that can pay. | ||
| const reimburserEmail = getReimburserEmail(policy); | ||
| const isReimburser = !!reimburserEmail && reimburserEmail === memberLogin; | ||
| const allowedRoles = isReimburser ? [CONST.POLICY.ROLE.ADMIN, CONST.POLICY.ROLE.PAYMENTS_ADMIN] : undefined; |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The "valid payer roles" set (Admin and Payments Admin) is hardcoded twice in this file: once here as allowedRoles and again in the changeRole guard (value !== CONST.POLICY.ROLE.ADMIN && value !== CONST.POLICY.ROLE.PAYMENTS_ADMIN). The same domain fact is also repeated across WorkspaceMembersPage.tsx and WorkspaceMemberDetailsPage.tsx. If the set of pay-capable roles ever changes, every copy must be updated in lockstep, and it is easy to miss one.
Extract a single source of truth and reuse it, e.g. a shared constant or predicate in PolicyUtils:
// PolicyUtils.ts
const PAYER_ROLES = [CONST.POLICY.ROLE.ADMIN, CONST.POLICY.ROLE.PAYMENTS_ADMIN] as const;
function canRolePay(role: string | undefined): boolean {
return PAYER_ROLES.some((payerRole) => payerRole === role);
}// WorkspaceMemberDetailsRolePage.tsx
const allowedRoles = isReimburser ? [...PAYER_ROLES] : undefined;
// ...
if (isReimburser && !canRolePay(value)) {
return;
}Reviewed at: 124be1f | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Addressed in 43f0f1d. Extracted the "valid payer roles" set into a single source of truth in PolicyUtils and reused it everywhere it was hardcoded:
PolicyUtils.ts:704— newPAYER_ROLESconstant andcanRolePaypredicate.WorkspaceMemberDetailsRolePage.tsx:46—allowedRoles = isReimburser ? [...PAYER_ROLES] : undefined, and thechangeRoleguard at line 61 now uses!canRolePay(value).WorkspaceMemberDetailsPage.tsx:153—assignablePayerRolesnow filtersPAYER_ROLESinstead of an inline literal.
I left WorkspaceMembersPage.tsx as-is: it doesn't hardcode a payer-role set — it wires up two distinct bulk actions (MAKE_ADMIN and MAKE_PAYMENTS_ADMIN) that reference each role individually, so there's no single spot where the payer set could be substituted without obscuring the per-action logic.
Verified locally: typecheck, lint-changed, react-compiler compliance, cspell, and the WorkspaceMemberDetailsPageTest + WorkspaceMembersTest suites (22/22) all pass.
|
Still no code fix needed — every code/build check on this PR is green (all 8 The two checks reporting a failure aren't code problems and can't be resolved by pushing a commit:
Both are on the list of checks MelvinBot intentionally ignores. What's needed is a human reviewer to approve the PR and fill in the reviewer checklist — after that, both go green. |
|
No code fix is needed — both currently-failing checks are human-only gates, not code problems:
Every substantive check (all |
|
No code fix is needed — all substantive checks ( The only two checks reporting a failure are both process/approval gates, not code problems, and can't be fixed by a push:
What's actually needed: an independent reviewer to approve and complete the reviewer checklist. Both checks will go green once that happens — there's nothing to change in the code. |
Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
@ikevin127 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] |
|
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. |
Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
🤖 Addressed both outstanding bot findings in 43f0f1d and pushed.
Verified locally: |
|
@MelvinBot Can you update this after running |
Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
🤖 Done — ran It only reformatted one line in |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-08-22.at.3.59.19.PM.mov |
| * Whether the given role is allowed to pay (reimburse) on a workspace. | ||
| */ | ||
| function canRolePay(role: string | undefined): boolean { | ||
| return PAYER_ROLES.some((payerRole) => payerRole === role); |
There was a problem hiding this comment.
In anticipation of customer roles, can we update this to use permissions instead of role?
There was a problem hiding this comment.
🤖 Done in 59b1323 — canRolePay now checks the WORKFLOWS_PAYMENTS permission instead of a hardcoded role list, so a future role (e.g. upcoming customer roles) that is granted payment permission is treated as a valid payer automatically.
What changed (PolicyUtils.ts:705):
function canRolePay(role: string | undefined): boolean {
return !!role && ROLE_PERMISSION_BUNDLES[role]?.[CONST.POLICY.POLICY_FEATURE.WORKFLOWS_PAYMENTS] === CONST.POLICY.POLICY_FEATURE_ACCESS.WRITE;
}
// PAYER_ROLES is now derived from the permission rather than [ADMIN, PAYMENTS_ADMIN]
const PAYER_ROLES = Object.values(CONST.POLICY.ROLE).filter(canRolePay);This also makes the helper consistent with how the rest of the app already determines who can pay — isPolicyPayer and the Authorized Payer selection page both use canMemberWrite(..., WORKFLOWS_PAYMENTS). The old hardcoded [ADMIN, PAYMENTS_ADMIN] was the outlier.
EDITOR also holds WORKFLOWS_PAYMENTS write in ROLE_PERMISSION_BUNDLES, so the permission-derived payer set is {Admin, Editor, Payments Admin}, not {Admin, Payments Admin}. Practically this means an Admin Authorized Payer on a Team workspace can now be changed to Editor (another payment-capable role), whereas before that row was read-only. This is consistent with the existing payer-selection page (which already lets an Editor be picked as payer), so I went with it and updated the affected unit test to match. If Editor should not be offered as a payer role, say the word and I'll keep PAYER_ROLES explicitly scoped to [ADMIN, PAYMENTS_ADMIN] while still routing through the permission check.
Verified locally: typecheck (no errors in the changed files), lint-changed, cspell, React Compiler compliance, and PolicyUtilsTest (367/367) + WorkspaceMemberDetailsPageTest (10/10) + WorkspaceMembersTest (12/12) all pass.
…a hardcoded role list Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
| * Whether the given role is allowed to pay (reimburse) on a workspace. This is derived from the WORKFLOWS_PAYMENTS feature | ||
| * permission rather than a hardcoded role list, so any future role that is granted payment permission (for example upcoming | ||
| * customer roles) is automatically treated as a valid payer, matching how the rest of the app determines who can pay. |
There was a problem hiding this comment.
| * Whether the given role is allowed to pay (reimburse) on a workspace. This is derived from the WORKFLOWS_PAYMENTS feature | |
| * permission rather than a hardcoded role list, so any future role that is granted payment permission (for example upcoming | |
| * customer roles) is automatically treated as a valid payer, matching how the rest of the app determines who can pay. | |
| * Whether the given role is allowed to pay (reimburse) on a workspace. |
@MelvinBot Update this
There was a problem hiding this comment.
🤖 Done in d55b2d2 — collapsed the canRolePay doc comment back to the one-liner: PolicyUtils.ts:700.
Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
3 participants
Explanation of Change
The member details page locked the Role row for any change whenever the member was the workspace Authorized Payer (reimburser). There was no branch on the target role — a blanket lock — so it also blocked valid changes such as promoting a non-admin payer to Workspace Admin, even though the backend allows that. The backend only rejects making the payer a role that can't pay (i.e. demoting them out of Admin / Payments Admin), which would break reimbursements.
This change narrows the restriction to only capability-reducing changes, so the payer can still move between the two valid payer roles (Admin and Payments Admin):
WorkspaceMemberDetailsPage.tsx— the Role row is no longer locked for the payer;canEditSelectedMemberRoledrops the!isReimburserterm, so the row stays interactive (and the "Role can't be changed" hint is removed). It is still disabled for the owner and the current user, and those read-only rows render at full opacity without a caret.WorkspaceMemberDetailsRolePage.tsx— when the member is the payer, only Admin and Payments Admin are offered in the role list (allowedRoles={[ADMIN, PAYMENTS_ADMIN]}), andchangeRoleguards the direct-navigation path by rejecting any target that isn't one of those two roles. This preserves the "the payer must stay a valid payer" guarantee the blanket lock used to provide.WorkspaceMemberRoleList.tsx— adds an optionalallowedRolesprop that filters the offered roles when provided; all other callers are unaffected.WorkspaceMembersPage.tsx— the bulk "Make admin" / "Make payments admin" actions are no longer gated by!hasAtLeastOnePayer, since Admin and Payments Admin are both valid payer roles.Added unit tests in
WorkspaceMemberDetailsPageTest.tsxcovering both branches: a non-admin payer's Role row is not locked (promotable), and an admin payer's Role row is not locked (can be changed to Payments Admin).Fixed Issues
$ #97974
PROPOSAL: #97974 (comment)
Tests
Prerequisite: a workspace where you are an admin, with at least one non-admin member and one member who is already an admin.
Offline tests
QA Steps
Prerequisite: a workspace where you are an admin, with a non-admin member and an admin member.
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