Add People Admin workspace approval access#92875
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
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 ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
# Conflicts: # src/languages/de.ts # src/languages/es.ts # src/languages/fr.ts # src/languages/it.ts # src/languages/ja.ts # src/languages/nl.ts # src/languages/pl.ts # src/languages/pt-BR.ts # src/languages/zh-hans.ts # src/libs/actions/Policy/Member.ts # src/pages/workspace/WorkspaceMembersPage.tsx # src/pages/workspace/members/ImportedMembersPage.tsx # src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx # src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx # src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsExpensesFromPage.tsx
![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: 03a25dbf24
ℹ️ 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".
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
ShridharGoel
changed the title
# Conflicts: # src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsApproverPage.tsx # src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsExpensesFromPage.tsx
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-06-30.at.17.52.14.mov |
|
@MelvinBot review |
Review — People Admin workspace approval accessSolid, well-structured permission refactor. The role model is centralized in Notes (non-blocking)1. The Codex bot comment on 2. Confirm the submission-frequency visibility asymmetry is intended. 3. Minor DRY (flagged by the consistency bot). Several files repeat Reminder given the
|
| return ( | ||
| policy?.type === CONST.POLICY.TYPE.CORPORATE && | ||
| canMemberWrite(policy, login, CONST.POLICY.POLICY_FEATURE.MEMBERS) && | ||
| getPolicyRole(policy, login) === CONST.POLICY.ROLE.PEOPLE_ADMIN && | ||
| (role === CONST.POLICY.ROLE.USER || role === CONST.POLICY.ROLE.AUDITOR) | ||
| ); | ||
| } |
There was a problem hiding this comment.
| return ( | |
| policy?.type === CONST.POLICY.TYPE.CORPORATE && | |
| canMemberWrite(policy, login, CONST.POLICY.POLICY_FEATURE.MEMBERS) && | |
| getPolicyRole(policy, login) === CONST.POLICY.ROLE.PEOPLE_ADMIN && | |
| (role === CONST.POLICY.ROLE.USER || role === CONST.POLICY.ROLE.AUDITOR) | |
| ); | |
| } | |
| // a People Admin who manages members can only assign the User or Auditor role | |
| return ( | |
| getPolicyRole(policy, login) === CONST.POLICY.ROLE.PEOPLE_ADMIN && | |
| (role === CONST.POLICY.ROLE.USER || role === CONST.POLICY.ROLE.AUDITOR) | |
| ); | |
| } |
Will this work?
There was a problem hiding this comment.
It removes the corporate check and canMemberWrite(...MEMBERS) guard. That is needed because PEOPLE_ADMIN is a control-policy-only role, and canMemberWrite ensures that is followed through hasPolicyFeaturePermission
There was a problem hiding this comment.
Ok, but getPolicyRole(policy, login) === CONST.POLICY.ROLE.PEOPLE_ADMIN isn't itself enough to derive that it has access here. Bcoz of that, I thought we don't need canMemberWrite check here.
There was a problem hiding this comment.
Actually canMemberWrite also has control-policy-only role validation and any future common changes will be added to this method. So, don't you think we should keep it along with getPolicyRole(policy, login) === CONST.POLICY.ROLE.PEOPLE_ADMIN ?
# Conflicts: # src/pages/workspace/WorkspaceMembersPage.tsx # src/pages/workspace/members/ImportedMembersPage.tsx
ShridharGoel
changed the title
# Conflicts: # src/pages/workspace/WorkspaceMembersPage.tsx # src/pages/workspace/members/WorkspaceInviteMessageComponent.tsx
|
@ShridharGoel let's fix the conflicts |
- Languages: keep both peopleAdmins and members keys - WorkspaceMembersTable: add People Admins role filter, rename selection prop to canSelectMembers - WorkspaceMembersPage: port People Admin permission gating and Make People Admin bulk action onto the Table-based page - WorkspaceWorkflowsPage: gate submission frequency and payments sections by People Admin access
|
@Pujan92 Updated |
|
@MelvinBot review |
This comment was marked as outdated.
This comment was marked as outdated.
| accessVariants={[CONST.POLICY.ACCESS_VARIANTS.ADMIN]} | ||
| policyFeature={CONST.POLICY.POLICY_FEATURE.MEMBERS} | ||
| policyFeatureAccess={CONST.POLICY.POLICY_FEATURE_ACCESS.WRITE} | ||
| shouldBeBlocked={!canMemberAssignRole(policy, currentUserLogin, CONST.POLICY.ROLE.AUDITOR)} |
There was a problem hiding this comment.
Why are we passing AUDITOR here?
There was a problem hiding this comment.
This means "can this user assign the auditor role to someone else." That capability is true only for Admins and People Admins on a Control policy.
There was a problem hiding this comment.
But replaced these canMemberAssignRole(..., AUDITOR) gates with canMemberAssignElevatedRole(policy, login)
- canMemberAssignRole: drop the hardcoded PEOPLE_ADMIN check (members:write covers it, works for future custom roles) and reject control-only roles on non-control policies - WorkspaceMemberRoleList: rely on canMemberAssignRole instead of per-role isControlPolicy checks - Rename isPolicyAdmin -> canEditWorkspaceSettings in WorkspaceMembersPage to match what the value represents - Add canMemberAssignElevatedRole helper and use it for the elevated-role gates in the invite/import flows so Collect admins keep access
…canMemberAssignRole isControlPolicy expects OnyxEntry<Policy> but canMemberAssignRole receives the wider OnyxInputOrEntry<Policy>. Inline the type check to match the existing CORPORATE check in the same function.
The page now renders the Table component (from main) instead of SelectionList, so the old selection-button testID no longer exists. Use selectCheckboxByMemberName like the other Table-based tests.
| function canMemberAssignRole(policy: OnyxInputOrEntry<Policy>, login: string, role: string | undefined): boolean { | ||
| if (!role) { | ||
| return false; | ||
| } | ||
|
|
||
| if (isControlPolicyOnlyRole(role) && policy?.type !== CONST.POLICY.TYPE.CORPORATE) { | ||
| return false; | ||
| } | ||
|
|
||
| if (canMemberWrite(policy, login, CONST.POLICY.POLICY_FEATURE.ASSIGN_ELEVATED_ROLES)) { | ||
| return true; | ||
| } | ||
|
|
||
| return ( | ||
| policy?.type === CONST.POLICY.TYPE.CORPORATE && | ||
| canMemberWrite(policy, login, CONST.POLICY.POLICY_FEATURE.MEMBERS) && | ||
| (role === CONST.POLICY.ROLE.USER || role === CONST.POLICY.ROLE.AUDITOR) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Following this logic is not that easy... how about this? Is it better?
| function canMemberAssignRole(policy: OnyxInputOrEntry<Policy>, login: string, role: string | undefined): boolean { | |
| if (!role) { | |
| return false; | |
| } | |
| if (isControlPolicyOnlyRole(role) && policy?.type !== CONST.POLICY.TYPE.CORPORATE) { | |
| return false; | |
| } | |
| if (canMemberWrite(policy, login, CONST.POLICY.POLICY_FEATURE.ASSIGN_ELEVATED_ROLES)) { | |
| return true; | |
| } | |
| return ( | |
| policy?.type === CONST.POLICY.TYPE.CORPORATE && | |
| canMemberWrite(policy, login, CONST.POLICY.POLICY_FEATURE.MEMBERS) && | |
| (role === CONST.POLICY.ROLE.USER || role === CONST.POLICY.ROLE.AUDITOR) | |
| ); | |
| } | |
| function canMemberAssignRole(policy: OnyxInputOrEntry<Policy>, login: string, role: string | undefined): boolean { | |
| if (!role) { | |
| return false; | |
| } | |
| const isCorporatePolicy = policy?.type === CONST.POLICY.TYPE.CORPORATE; | |
| if (isControlPolicyOnlyRole(role) && !isCorporatePolicy) { | |
| return false; | |
| } | |
| if (canMemberWrite(policy, login, CONST.POLICY.POLICY_FEATURE.ASSIGN_ELEVATED_ROLES)) { | |
| return true; | |
| } | |
| // Non-control-only roles (USER, AUDITOR) can reach this point on non-corporate policies, | |
| // but only corporate policies allow assigning them via the MEMBERS permission. | |
| const isNonElevatedRole = role === CONST.POLICY.ROLE.USER || role === CONST.POLICY.ROLE.AUDITOR; | |
| return isCorporatePolicy && canMemberWrite(policy, login, CONST.POLICY.POLICY_FEATURE.MEMBERS) && isNonElevatedRole; | |
| } |
4 participants
Explanation of Change
This PR adds the frontend permission handling for the People Admin workspace role.
People Admins can access Overview, Members, and Workflows. They can invite and remove members, but cannot assign elevated workspace roles or remove Workspace Admins. In Workflows, they can edit approval workflows while payment-related workflow settings stay restricted.
This also updates the approval workflow RHP pages so they use scoped
WORKFLOWS_APPROVALSwrite access instead of checking for full Workspace Admin access.Depends on https://github.com/Expensify/Web-Expensify/pull/53610
Fixed Issues
$ #90502, #90503
PROPOSAL:
Tests
Offline tests
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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