Repository navigation
Protect workspace auditors from removal in workspace chats - #102722
Conversation
Auditors are default members of every workspace chat, so removing one from the room members page did not stick and they reappeared after a refresh. Treat the member's own auditor role as protected, the same as admins and approvers. Co-authored-by: Kevin Brian Bader <ikevin127@users.noreply.github.com>
|
🤖 Web test steps verification results: ✅ pass The PR's Tests section is still empty, so these steps come from the linked issue and the PR description. Auditors are protected in workspace expense chats: their row checkbox and "Remove from chat" are both disabled, they stay listed after reopening the page, and a plain member in a custom room is still removable.
|
This comment has been minimized.
This comment has been minimized.
|
I updated the PR description, but I couldn't mark the PR ready for review. My environment blocked Changes to the description:
I left the Screenshots/Videos sections empty. You can reuse the screenshots from my earlier Web and Android test comments or add your own. |
|
@MelvinBot I cannot mark PR ready for review because I don' have permission. But how come you marked this PR ready for review from my exact same prompt - but are not able to mark this one ready ? Please tag CME from the issue if still not able to mark it ready for review for some reason. |
|
@marcaaron could you mark this PR ready for review? I still can't, and
|
This comment has been minimized.
This comment has been minimized.
|
I still couldn't mark this PR ready for review. My environment requires approval for Please click Ready for review at the bottom of the PR. The description is already filled in: the Tests, Offline tests, and QA Steps sections have steps, and every PR Author Checklist box is checked. |
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / SafariScreen.Recording.2026-10-01.at.15.21.49.mov |
| */ | ||
| const isRoomMemberProtectedByPolicyRole = (policy: OnyxInputOrEntry<Policy>, login: string | undefined, accountID: number | undefined): boolean => | ||
| isPolicyOwner(policy, accountID) || !login || isPolicyAdmin(policy, login, false) || isPolicyApprover(policy, login); | ||
| isPolicyOwner(policy, accountID) || !login || isPolicyAdmin(policy, login, false) || getPolicyRole(policy, login, false) === CONST.POLICY.ROLE.AUDITOR || isPolicyApprover(policy, login); |
There was a problem hiding this comment.
src/libs/PolicyUtils.ts L224
🟡 This inline getPolicyRole(policy, login, false) === CONST.POLICY.ROLE.AUDITOR does the same job as isPolicyAuditor. The only reason it can't call that helper is that isPolicyAuditor has no shouldCheckGlobalPolicyRole flag.
That's also why we now need a whole JSDoc paragraph explaining why we don't use it. Can we give isPolicyAuditor the same signature as isPolicyAdmin and call it here ?
Then the admin and auditor checks match, and the next role check won't hit the same trap:
/**
* Checks if the current user is an auditor of the policy.
*
* When `login` belongs to somebody other than the current user, pass `shouldCheckGlobalPolicyRole = false` (see `isPolicyAdmin`).
*/
const isPolicyAuditor = (policy: OnyxInputOrEntry<Policy>, login?: string, shouldCheckGlobalPolicyRole = true): boolean =>
getPolicyRole(policy, login, shouldCheckGlobalPolicyRole) === CONST.POLICY.ROLE.AUDITOR;const isRoomMemberProtectedByPolicyRole = (policy: OnyxInputOrEntry<Policy>, login: string | undefined, accountID: number | undefined): boolean =>
isPolicyOwner(policy, accountID) || !login || isPolicyAdmin(policy, login, false) || isPolicyAuditor(policy, login, false) || isPolicyApprover(policy, login);This is safe for the 3 existing callers (WorkspacesSettingsUtils, ReportUtils, WorkspaceRestrictedActionPage). They all use the default flag, so policy.role still wins.
The one difference is that they also get the lowercase employeeList fallback, which is a fix in its own right: WorkspaceRestrictedActionPage passes email directly today. If lint complains about using isPolicyAuditor before it's defined, move it up next to isPolicyAdmin.
After that, the JSDoc paragraph about "not through isPolicyAuditor" can go.
There was a problem hiding this comment.
Done in 8764e7b5. isPolicyAuditor now has the same signature as isPolicyAdmin, including the shouldCheckGlobalPolicyRole flag. It now sits next to isPolicyAdmin, and isRoomMemberProtectedByPolicyRole calls isPolicyAuditor(policy, login, false). I removed the JSDoc paragraph about not using isPolicyAuditor.
The 3 existing callers keep the default flag, so policy.role still wins. They also get the lowercase employeeList fallback now.
Checks
PolicyUtilsTest,ReportSecondaryActionUtilsTest,WorkspaceRestrictedActionPageTest: 777 passednpm run lint -- src/libs/PolicyUtils.ts: passednpm run typecheck: no errors in the changed files. The only errors are in unrelatedreact-native-gesture-handlerfiles because this environment'snode_modulesis out of date.
I also updated the PR description to match.
Give isPolicyAuditor the same shouldCheckGlobalPolicyRole flag as isPolicyAdmin, move it next to isPolicyAdmin, and call it from isRoomMemberProtectedByPolicyRole. Existing callers keep the default flag, and now also get getPolicyRole's lowercase employeeList fallback. Co-authored-by: Kevin Brian Bader <ikevin127@users.noreply.github.com>
|
Melvin:
|
|
@MelvinBot Fix the conflicts and mark this PR ready for review. |
…sFromRoomRemoval Co-authored-by: Kevin Brian Bader <ikevin127@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] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
I merged The conflict came from Checks I ran after the merge:
|
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM - Small change, tests well ✅
|
Looks good, but has conflicts now. |
|
@MelvinBot Fix the conflicts then tag Mark when ready to merge! |
…sFromRoomRemoval Co-authored-by: Kevin Brian Bader <ikevin127@users.noreply.github.com> # Conflicts: # src/libs/PolicyUtils/index.ts # tests/unit/PolicyUtilsTest.ts
|
@ikevin127 @marcaaron One of you needs to 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] |
|
@marcaaron I fixed the conflicts, and this is ready to merge. All CI checks pass except "Check independent approval", which needs your approval after the merge commit.
The diff against |
|
🚧 marcaaron has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.5.7-0 🚀
|
|
No help site update needed. This bug fix stops the App from offering Remove from chat for auditors, who are added back automatically, and no help article says auditors can be removed from a workspace chat. |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.5.7-3 🚀
Bundle Size Analysis (Sentry): |













Explanation of Change
Auditors are default members of every workspace chat. The workspace setup adds them, so removing one doesn't stick, and they come back after a refresh. Resolve the member's own policy role when checking room member removal added
isRoomMemberProtectedByPolicyRole. It protects the owner, admins and approvers, but it had no case for auditors, so the App offered "Remove from chat" for them.This PR adds an auditor clause to
isRoomMemberProtectedByPolicyRole:isPolicyAuditor(policy, login, false). To make that possible,isPolicyAuditornow has the same signature asisPolicyAdmin, including theshouldCheckGlobalPolicyRoleflag, and sits next to it. With the flag set tofalse, it reads the listed member's own role instead of the viewer's globalpolicy.role. Both callers get the fix:DynamicRoomMembersPage: the auditor's row can't be selected.DynamicRoomMemberDetailsPage: "Remove from chat" is disabled.The existing
isPolicyAuditorcallers (WorkspacesSettingsUtils,ReportUtils,WorkspaceRestrictedActionPage) use the default flag, sopolicy.rolestill wins for them. They now also getgetPolicyRole's lowercaseemployeeListfallback.The JSDoc now explains why auditors are protected. A unit test covers an auditor, including a mixed-case login.
AI tests run locally:
npm test -- tests/unit/PolicyUtilsTest.ts tests/unit/ReportSecondaryActionUtilsTest.ts tests/unit/pages/RestrictedAction/WorkspaceRestrictedActionPageTest.tsx: 777 passednpm test -- tests/unit/PolicyUtilsTest.ts tests/unit/ReportUtilsTest.ts(first commit): 1978 passednpm run lint -- src/libs/PolicyUtils.ts: passednpm run typecheck: no errors in the changed filesNot run: the full
npm testsuite and the Storybook smoke test. The change is limited to pure helpers and their tests.Fixed Issues
$ #102680
PROPOSAL: #102680 (comment)
Tests
Precondition: as the workspace owner, create a Control workspace. Go to Workspace > Members and invite User B as Auditor and User C as Member.
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