From 45c8c4f5bf8b3d229fae823b2488b97df8560c32 Mon Sep 17 00:00:00 2001 From: Juliet Shen Date: Mon, 24 Aug 2026 13:52:32 -0400 Subject: [PATCH 1/3] Add role-based access assignment for manual review queues (#1059) Intermediate fix for #383. Admins and managers can now grant queue access to everyone with a persisted organization role, alongside individual reviewer assignment. Store role IDs in the queue-access join table, validate that assigned roles belong to the queue organization, and use those IDs through the service, GraphQL API, and queue form UI. Co-authored-by: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_0165yqq7Qs92KDXBKWF3ahFb Amp-Thread-ID: https://ampcode.com/threads/T-01a0395d-6d80-73a8-b33a-86a09a22cede Co-authored-by: Amp --- client/src/graphql/generated.ts | 15 ++ .../dashboard/mrt/ManualReviewQueueForm.tsx | 35 +++ ....add_roles_and_accessible_queues_table.sql | 16 ++ server/graphql/generated.ts | 9 + .../graphql/modules/manualReviewTool.test.ts | 18 ++ server/graphql/modules/manualReviewTool.ts | 18 ++ server/graphql/modules/roles.resolver.test.ts | 6 + server/graphql/modules/roles.ts | 3 +- .../manualReviewToolService/dbTypes.ts | 7 +- .../manualReviewToolService.ts | 9 + .../modules/CommentOperations.test.ts | 1 + .../modules/JobRouting.test.ts | 5 +- .../modules/QueueOperations.test.ts | 236 +++++++++++++++++- .../modules/QueueOperations.ts | 168 ++++++++++--- .../modules/UserReportSweep.test.ts | 1 + server/test/fixtureHelpers/createMrtQueue.ts | 1 + 16 files changed, 514 insertions(+), 34 deletions(-) create mode 100644 db/src/scripts/api-server-pg/2026.09.01T18.44.45.add_roles_and_accessible_queues_table.sql create mode 100644 server/graphql/modules/manualReviewTool.test.ts diff --git a/client/src/graphql/generated.ts b/client/src/graphql/generated.ts index 01ab5a353..807fa3fc0 100644 --- a/client/src/graphql/generated.ts +++ b/client/src/graphql/generated.ts @@ -791,6 +791,7 @@ export type GQLCreateManualReviewQueueInput = { readonly hiddenActionIds: ReadonlyArray; readonly isAppealsQueue: Scalars['Boolean']['input']; readonly name: Scalars['String']['input']; + readonly roleIds: ReadonlyArray; readonly userIds: ReadonlyArray; }; @@ -2237,6 +2238,8 @@ export type GQLManualReviewJobWithDecisions = { export type GQLManualReviewQueue = { readonly __typename: 'ManualReviewQueue'; + /** Roles whose members can review this queue, in addition to explicitly assigned reviewers. */ + readonly assignedRoleIds: ReadonlyArray; readonly autoCloseJobs: Scalars['Boolean']['output']; readonly clearReportsDisposition?: Maybe; readonly clearReportsScope: GQLMrtClearReportsScope; @@ -4843,6 +4846,7 @@ export type GQLUpdateManualReviewQueueInput = { readonly description?: InputMaybe; readonly id: Scalars['ID']['input']; readonly name?: InputMaybe; + readonly roleIds: ReadonlyArray; readonly userIds: ReadonlyArray; }; @@ -9899,6 +9903,11 @@ export type GQLQueueFormDataQueryVariables = Exact<{ [key: string]: never }>; export type GQLQueueFormDataQuery = { readonly __typename: 'Query'; + readonly rolesForOrg: ReadonlyArray<{ + readonly __typename: 'Role'; + readonly id: string; + readonly displayName: string; + }>; readonly myOrg?: { readonly __typename: 'Org'; readonly hasAppealsEnabled: boolean; @@ -9950,6 +9959,7 @@ export type GQLManualReviewQueueQuery = { readonly id: string; readonly name: string; readonly description?: string | null; + readonly assignedRoleIds: ReadonlyArray; readonly hiddenActionIds: ReadonlyArray; readonly isAppealsQueue: boolean; readonly autoCloseJobs: boolean; @@ -32891,6 +32901,10 @@ export type GQLGetDecisionsTableQueryResult = Apollo.QueryResult< >; export const GQLQueueFormDataDocument = gql` query QueueFormData { + rolesForOrg { + id + displayName + } myOrg { hasAppealsEnabled hasPartialItemsEndpoint @@ -33013,6 +33027,7 @@ export const GQLManualReviewQueueDocument = gql` explicitlyAssignedReviewers { id } + assignedRoleIds hiddenActionIds isAppealsQueue autoCloseJobs diff --git a/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.tsx b/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.tsx index f5e699aca..f0ed24fa0 100644 --- a/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.tsx +++ b/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.tsx @@ -46,6 +46,10 @@ const CLEAR_REPORTS_SCOPE_LABELS: Record = { gql` query QueueFormData { + rolesForOrg { + id + displayName + } myOrg { hasAppealsEnabled hasPartialItemsEndpoint @@ -76,6 +80,7 @@ gql` explicitlyAssignedReviewers { id } + assignedRoleIds hiddenActionIds isAppealsQueue autoCloseJobs @@ -149,6 +154,7 @@ export default function ManualReviewQueueForm() { const [moderatorsWithAccess, setModeratorsWithAccess] = useState( [], ); + const [rolesWithAccess, setRolesWithAccess] = useState([]); const [hiddenActionIds, setHiddenActionIds] = useState([]); const [autoCloseJobs, setAutoCloseJobs] = useState(false); const [isAppealsQueue, setIsAppealsQueue] = useState(false); @@ -251,6 +257,7 @@ export default function ManualReviewQueueForm() { [data?.myOrg?.users], ); const orgActions = data?.myOrg?.actions ?? []; + const orgRoles = data?.rolesForOrg ?? []; const userIdsWhoCanReviewEveryQueue = useMemo( () => (data?.myOrg?.usersWhoCanReviewEveryQueue ?? []).map((it) => it.id), @@ -308,6 +315,7 @@ export default function ManualReviewQueueForm() { setQueueName(queue.name); setQueueDescription(queue.description ?? undefined); + setRolesWithAccess([...queue.assignedRoleIds]); setHiddenActionIds([...queue.hiddenActionIds]); setAutoCloseJobs(queue.autoCloseJobs); setIsAppealsQueue(queue.isAppealsQueue); @@ -342,6 +350,7 @@ export default function ManualReviewQueueForm() { // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing description: queueDescription || null, userIds: moderatorsWithAccess, + roleIds: rolesWithAccess, hiddenActionIds, isAppealsQueue, autoCloseJobs, @@ -365,6 +374,7 @@ export default function ManualReviewQueueForm() { moderatorsWithAccess, queueDescription, queueName, + rolesWithAccess, ], ); @@ -379,6 +389,7 @@ export default function ManualReviewQueueForm() { // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing description: queueDescription || null, userIds: moderatorsWithAccess, + roleIds: rolesWithAccess, actionIdsToHide: difference( hiddenActionIds, initiallyHiddenActionIds, @@ -408,6 +419,7 @@ export default function ManualReviewQueueForm() { moderatorsWithAccess, queueDescription, queueName, + rolesWithAccess, updateManualReviewQueue, ], ); @@ -507,6 +519,29 @@ export default function ManualReviewQueueForm() { }); })} +
Roles with Access
+
+ Everyone with a selected role can review this queue, so you don't have + to add them one by one. Admins and Moderator Managers already have + access to every queue. +
+ + className="self-start !min-w-[160px]" + mode="multiple" + placeholder="Add Roles" + dropdownMatchSelectWidth={false} + allowClear + showSearch + filterOption={selectFilterByLabelOption} + value={rolesWithAccess} + onChange={setRolesWithAccess} + > + {orgRoles.map((role) => ( + + ))} + {orgActions.length > 0 && (
Hidden Actions
diff --git a/db/src/scripts/api-server-pg/2026.09.01T18.44.45.add_roles_and_accessible_queues_table.sql b/db/src/scripts/api-server-pg/2026.09.01T18.44.45.add_roles_and_accessible_queues_table.sql new file mode 100644 index 000000000..3d6100f75 --- /dev/null +++ b/db/src/scripts/api-server-pg/2026.09.01T18.44.45.add_roles_and_accessible_queues_table.sql @@ -0,0 +1,16 @@ +-- Add per-queue role-based access for the manual review tool. A queue becomes +-- accessible to a reviewer when their persisted role is assigned here, in +-- addition to any individual grant in `users_and_accessible_queues`. This +-- migration is additive and idempotent. + +CREATE TABLE IF NOT EXISTS manual_review_tool.roles_and_accessible_queues ( + queue_id character varying(255) NOT NULL + REFERENCES manual_review_tool.manual_review_queues(id) ON DELETE CASCADE, + role_id uuid NOT NULL REFERENCES public.roles(id) ON DELETE CASCADE, + PRIMARY KEY (queue_id, role_id) +); + +ALTER TABLE manual_review_tool.roles_and_accessible_queues OWNER TO CURRENT_USER; + +CREATE INDEX IF NOT EXISTS roles_and_accessible_queues_role_id_idx + ON manual_review_tool.roles_and_accessible_queues (role_id); diff --git a/server/graphql/generated.ts b/server/graphql/generated.ts index 8e2172975..49f06a064 100644 --- a/server/graphql/generated.ts +++ b/server/graphql/generated.ts @@ -859,6 +859,7 @@ export type GQLCreateManualReviewQueueInput = { readonly hiddenActionIds: ReadonlyArray; readonly isAppealsQueue: Scalars['Boolean']['input']; readonly name: Scalars['String']['input']; + readonly roleIds: ReadonlyArray; readonly userIds: ReadonlyArray; }; @@ -2305,6 +2306,8 @@ export type GQLManualReviewJobWithDecisions = { export type GQLManualReviewQueue = { readonly __typename?: 'ManualReviewQueue'; + /** Roles whose members can review this queue, in addition to explicitly assigned reviewers. */ + readonly assignedRoleIds: ReadonlyArray; readonly autoCloseJobs: Scalars['Boolean']['output']; readonly clearReportsDisposition?: Maybe; readonly clearReportsScope: GQLMrtClearReportsScope; @@ -4911,6 +4914,7 @@ export type GQLUpdateManualReviewQueueInput = { readonly description?: InputMaybe; readonly id: Scalars['ID']['input']; readonly name?: InputMaybe; + readonly roleIds: ReadonlyArray; readonly userIds: ReadonlyArray; }; @@ -10486,6 +10490,11 @@ export type GQLManualReviewQueueResolvers< ParentType extends GQLResolversParentTypes['ManualReviewQueue'] = GQLResolversParentTypes['ManualReviewQueue'], > = { + assignedRoleIds?: Resolver< + ReadonlyArray, + ParentType, + ContextType + >; autoCloseJobs?: Resolver< GQLResolversTypes['Boolean'], ParentType, diff --git a/server/graphql/modules/manualReviewTool.test.ts b/server/graphql/modules/manualReviewTool.test.ts new file mode 100644 index 000000000..e37732ad9 --- /dev/null +++ b/server/graphql/modules/manualReviewTool.test.ts @@ -0,0 +1,18 @@ +import { buildASTSchema, isInputObjectType } from 'graphql'; + +import typeDefs from '../schema.js'; + +describe('manual review queue inputs', () => { + test.each(['CreateManualReviewQueueInput', 'UpdateManualReviewQueueInput'])( + '%s requires roleIds', + (inputName) => { + const input = buildASTSchema(typeDefs).getType(inputName); + expect(isInputObjectType(input)).toBe(true); + if (!isInputObjectType(input)) { + return; + } + + expect(input.getFields().roleIds.type.toString()).toBe('[ID!]!'); + }, + ); +}); diff --git a/server/graphql/modules/manualReviewTool.ts b/server/graphql/modules/manualReviewTool.ts index 94c22aec6..29eb18ff9 100644 --- a/server/graphql/modules/manualReviewTool.ts +++ b/server/graphql/modules/manualReviewTool.ts @@ -67,6 +67,8 @@ const typeDefs = /* GraphQL */ ` pendingJobCount: Int! oldestJobCreatedAt: DateTime explicitlyAssignedReviewers: [User!]! + "Roles whose members can review this queue, in addition to explicitly assigned reviewers." + assignedRoleIds: [ID!]! hiddenActionIds: [ID!]! isAppealsQueue: Boolean! autoCloseJobs: Boolean! @@ -416,6 +418,7 @@ const typeDefs = /* GraphQL */ ` name: String! description: String userIds: [ID!]! + roleIds: [ID!]! hiddenActionIds: [ID!]! isAppealsQueue: Boolean! autoCloseJobs: Boolean! @@ -429,6 +432,7 @@ const typeDefs = /* GraphQL */ ` name: String description: String userIds: [ID!]! + roleIds: [ID!]! actionIdsToHide: [ID!]! actionIdsToUnhide: [ID!]! autoCloseJobs: Boolean! @@ -1793,6 +1797,16 @@ const ManualReviewQueue: GQLManualReviewQueueResolvers = { ).map((it) => it.userId); return context.dataSources.userAPI.getGraphQLUsersFromIds(userIds); }, + async assignedRoleIds(queue, _, context) { + const user = context.getUser(); + if (user == null) { + throw unauthenticatedError('User required.'); + } + return context.services.ManualReviewToolService.getAssignedRoleIdsForQueue({ + queueId: queue.id, + orgId: user.orgId, + }); + }, async hiddenActionIds(queue, _, context) { const user = context.getUser(); if (user == null) { @@ -2404,6 +2418,7 @@ const Mutation: GQLMutationResolvers = { name, description, userIds, + roleIds, hiddenActionIds, isAppealsQueue, autoCloseJobs, @@ -2420,6 +2435,7 @@ const Mutation: GQLMutationResolvers = { description: description ?? null, name, userIds: userIdsWithCurrentUser, + roleIds, hiddenActionIds, isAppealsQueue, autoCloseJobs, @@ -2461,6 +2477,7 @@ const Mutation: GQLMutationResolvers = { name, description, userIds, + roleIds, actionIdsToHide, actionIdsToUnhide, autoCloseJobs, @@ -2478,6 +2495,7 @@ const Mutation: GQLMutationResolvers = { // Include the user who's creating the queue as having permission to see // the queue userIds: [...userIds, user.id], + roleIds, actionIdsToHide, actionIdsToUnhide, autoCloseJobs, diff --git a/server/graphql/modules/roles.resolver.test.ts b/server/graphql/modules/roles.resolver.test.ts index fa00f6a12..32ff2804a 100644 --- a/server/graphql/modules/roles.resolver.test.ts +++ b/server/graphql/modules/roles.resolver.test.ts @@ -116,6 +116,12 @@ describe('roles resolvers', () => { await Query.rolesForOrg({}, {}, ctx); expect(roleAPI.listRolesForOrg).toHaveBeenCalledWith('org-1'); }); + + it('delegates to roleAPI.listRolesForOrg when caller has EDIT_MRT_QUEUES', async () => { + const { ctx, roleAPI } = makeCtx([UserPermission.EDIT_MRT_QUEUES]); + await Query.rolesForOrg({}, {}, ctx); + expect(roleAPI.listRolesForOrg).toHaveBeenCalledWith('org-1'); + }); }); describe('Query.permissionGroups', () => { diff --git a/server/graphql/modules/roles.ts b/server/graphql/modules/roles.ts index 0ed187452..6764a694b 100644 --- a/server/graphql/modules/roles.ts +++ b/server/graphql/modules/roles.ts @@ -122,7 +122,8 @@ function requireRoleVisibility(context: Context) { const perms = user.getPermissions(); if ( !perms.includes(UserPermission.MANAGE_ROLES) && - !perms.includes(UserPermission.MANAGE_USERS) + !perms.includes(UserPermission.MANAGE_USERS) && + !perms.includes(UserPermission.EDIT_MRT_QUEUES) ) { throw forbiddenError( 'User does not have permission to view roles in this organization', diff --git a/server/services/manualReviewToolService/dbTypes.ts b/server/services/manualReviewToolService/dbTypes.ts index ae3d1e2fe..2a254af22 100644 --- a/server/services/manualReviewToolService/dbTypes.ts +++ b/server/services/manualReviewToolService/dbTypes.ts @@ -73,8 +73,9 @@ export type RoutingRuleExecutionsRow = { ); export type ManualReviewToolServicePg = { - // Shared with CoreAppTablesPg so org-scoping checks can query public.users. + // Shared with CoreAppTablesPg so org-scoping checks can query users/roles. 'public.users': CoreAppTablesPg['public.users']; + 'public.roles': CoreAppTablesPg['public.roles']; 'manual_review_tool.manual_review_queues': { id: string; name: string; @@ -173,6 +174,10 @@ export type ManualReviewToolServicePg = { user_id: string; queue_id: string; }; + 'manual_review_tool.roles_and_accessible_queues': { + role_id: string; + queue_id: string; + }; 'manual_review_tool.dim_mrt_decisions': { org_id: GeneratedAlways; job_id: GeneratedAlways; diff --git a/server/services/manualReviewToolService/manualReviewToolService.ts b/server/services/manualReviewToolService/manualReviewToolService.ts index 7b663d5e0..3845f693b 100644 --- a/server/services/manualReviewToolService/manualReviewToolService.ts +++ b/server/services/manualReviewToolService/manualReviewToolService.ts @@ -889,6 +889,7 @@ export class ManualReviewToolService { name: string; description: string | null; userIds: readonly string[]; + roleIds: readonly string[]; hiddenActionIds: readonly string[]; invokedBy: Invoker; isAppealsQueue: boolean; @@ -906,6 +907,7 @@ export class ManualReviewToolService { name?: string; description?: string | null; userIds: readonly string[]; + roleIds: readonly string[]; actionIdsToHide: readonly string[]; actionIdsToUnhide: readonly string[]; autoCloseJobs?: boolean; @@ -1156,6 +1158,13 @@ export class ManualReviewToolService { return this.queueOps.getUsersWhoCanSeeQueue({ orgId, queueId }); } + async getAssignedRoleIdsForQueue(opts: { + orgId: string; + queueId: string; + }): Promise { + return this.queueOps.getAssignedRoleIdsForQueue(opts); + } + async getDecisionTimeToAction(input: TimeToActionInput) { return this.decisionAnalytics.getTimeToAction(input); } diff --git a/server/services/manualReviewToolService/modules/CommentOperations.test.ts b/server/services/manualReviewToolService/modules/CommentOperations.test.ts index 7e1801ef6..df5c3d2f0 100644 --- a/server/services/manualReviewToolService/modules/CommentOperations.test.ts +++ b/server/services/manualReviewToolService/modules/CommentOperations.test.ts @@ -32,6 +32,7 @@ describe('CommentOperations', () => { name: 'Test Queue', description: null, userIds: [user.id], + roleIds: [], hiddenActionIds: [], isAppealsQueue: false, invokedBy: { diff --git a/server/services/manualReviewToolService/modules/JobRouting.test.ts b/server/services/manualReviewToolService/modules/JobRouting.test.ts index ff8040968..7e02d8646 100644 --- a/server/services/manualReviewToolService/modules/JobRouting.test.ts +++ b/server/services/manualReviewToolService/modules/JobRouting.test.ts @@ -1,4 +1,3 @@ -/* eslint-disable max-lines */ import { ScalarTypes } from '@roostorg/coop-types'; import { uid } from 'uid'; @@ -53,6 +52,7 @@ describe('JobRouting tests', () => { name: 'Default Queue', description: null, userIds: [userId], + roleIds: [], hiddenActionIds: [], isAppealsQueue: false, invokedBy: { @@ -66,6 +66,7 @@ describe('JobRouting tests', () => { name: 'Another Queue', description: null, userIds: [userId], + roleIds: [], hiddenActionIds: [], isAppealsQueue: false, invokedBy: { @@ -79,6 +80,7 @@ describe('JobRouting tests', () => { name: 'Policy Queue', description: null, userIds: [userId], + roleIds: [], hiddenActionIds: [], isAppealsQueue: false, invokedBy: { @@ -93,6 +95,7 @@ describe('JobRouting tests', () => { name: 'No Policy Queue', description: null, userIds: [userId], + roleIds: [], hiddenActionIds: [], isAppealsQueue: false, invokedBy: { diff --git a/server/services/manualReviewToolService/modules/QueueOperations.test.ts b/server/services/manualReviewToolService/modules/QueueOperations.test.ts index b0422f29c..3487414c6 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.test.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.test.ts @@ -9,7 +9,7 @@ import createOrg from '../../../test/fixtureHelpers/createOrg.js'; import createUser from '../../../test/fixtureHelpers/createUser.js'; import makeDummyMrtJobPayload from '../../../test/fixtureHelpers/makeDummyMrtJobPayload.js'; import { makeTransactionalTestWithFixture } from '../../../test/harness/transactionalTest.js'; -import { UserPermission } from '../../userManagementService/index.js'; +import { UserPermission, UserRole } from '../../userManagementService/index.js'; import { bullJobIdtoExternalJobId, itemIdToBullJobId, @@ -392,6 +392,7 @@ describe('QueueOperations', () => { name: 'attacker-queue', description: null, userIds: [victim.user.id], + roleIds: [], hiddenActionIds: [], isAppealsQueue: false, invokedBy: { @@ -412,6 +413,7 @@ describe('QueueOperations', () => { orgId: attacker.org.id, queueId: attacker.queue.id, userIds: [attacker.user.id, victim.user.id], + roleIds: [], actionIdsToHide: [], actionIdsToUnhide: [], }), @@ -426,6 +428,237 @@ describe('QueueOperations', () => { }, ); + // A queue creator holds EDIT_MRT_QUEUES; the reviewer is a plain Moderator + // who is never added to the queue individually, so any access they get comes + // solely from a role assignment. + const testWithRoleAssignableQueue = () => + makeTransactionalTestWithFixture(async ({ deps }) => { + const { org } = await createOrg( + { + KyselyPg: deps.KyselyPg, + ModerationConfigService: deps.ModerationConfigService, + ApiKeyService: deps.ApiKeyService, + }, + uid(), + ); + const { user: creator } = await createUser(deps.KyselyPg, org.id, { + role: UserRole.ADMIN, + }); + const { user: moderator } = await createUser(deps.KyselyPg, org.id, { + role: UserRole.MODERATOR, + }); + return { + org, + creator, + moderator, + apiKeyService: deps.ApiKeyService, + moderationConfigService: deps.ModerationConfigService, + kyselyPg: deps.KyselyPg, + mrtService: deps.ManualReviewToolService, + }; + }); + + const reviewerInvoker = (userId: string, orgId: string) => ({ + userId, + permissions: [UserPermission.VIEW_MRT], + orgId, + }); + + const getRoleId = async ( + db: QueueFixtureRole['kyselyPg'], + orgId: string, + role: UserRole, + ) => + db + .selectFrom('public.roles') + .select('id') + .where('org_id', '=', orgId) + .where('key', '=', role) + .executeTakeFirstOrThrow() + .then((row) => row.id); + + const createQueueWithRoles = async ( + mrtService: QueueFixtureRole['mrtService'], + org: QueueFixtureRole['org'], + creator: QueueFixtureRole['creator'], + roleIds: string[], + ) => + mrtService.createManualReviewQueue({ + name: `role-queue-${uid()}`, + description: null, + userIds: [creator.id], + roleIds, + hiddenActionIds: [], + isAppealsQueue: false, + invokedBy: { + userId: creator.id, + permissions: [UserPermission.EDIT_MRT_QUEUES], + orgId: org.id, + }, + }); + + type QueueFixtureRole = Parameters< + Parameters>[1] + >[0]; + + testWithRoleAssignableQueue()( + 'a role assigned to a queue grants its members reviewer access', + async ({ org, creator, moderator, mrtService, kyselyPg }) => { + const roleId = await getRoleId(kyselyPg, org.id, UserRole.MODERATOR); + const queue = await createQueueWithRoles(mrtService, org, creator, [ + roleId, + ]); + + const reviewable = await mrtService.getReviewableQueuesForUser({ + invoker: reviewerInvoker(moderator.id, org.id), + }); + expect(reviewable.map((q) => q.id)).toContain(queue.id); + }, + ); + + testWithRoleAssignableQueue()( + 'a repeated role assignment creates the queue with one assignment and grants access', + async ({ org, creator, moderator, mrtService, kyselyPg }) => { + const roleId = await getRoleId(kyselyPg, org.id, UserRole.MODERATOR); + const queue = await createQueueWithRoles(mrtService, org, creator, [ + roleId, + roleId, + ]); + + expect( + await mrtService.getAssignedRoleIdsForQueue({ + orgId: org.id, + queueId: queue.id, + }), + ).toEqual([roleId]); + const reviewable = await mrtService.getReviewableQueuesForUser({ + invoker: reviewerInvoker(moderator.id, org.id), + }); + expect(reviewable.map((q) => q.id)).toContain(queue.id); + }, + ); + + testWithRoleAssignableQueue()( + 'a queue with no assigned roles is not reviewable by an unassigned moderator', + async ({ org, creator, moderator, mrtService }) => { + const queue = await createQueueWithRoles(mrtService, org, creator, []); + + const reviewable = await mrtService.getReviewableQueuesForUser({ + invoker: reviewerInvoker(moderator.id, org.id), + }); + expect(reviewable.map((q) => q.id)).not.toContain(queue.id); + }, + ); + + testWithRoleAssignableQueue()( + 'getQueueForOrg returns a role-accessible queue for a moderator', + async ({ org, creator, moderator, mrtService, kyselyPg }) => { + const roleId = await getRoleId(kyselyPg, org.id, UserRole.MODERATOR); + const queue = await createQueueWithRoles(mrtService, org, creator, [ + roleId, + ]); + + const fetched = await mrtService.getQueueForOrg({ + orgId: org.id, + userId: moderator.id, + queueId: queue.id, + }); + expect(fetched?.id).toBe(queue.id); + }, + ); + + testWithRoleAssignableQueue()( + 'removing an assigned role revokes access and updates assigned roles', + async ({ org, creator, moderator, mrtService, kyselyPg }) => { + const roleId = await getRoleId(kyselyPg, org.id, UserRole.MODERATOR); + const queue = await createQueueWithRoles(mrtService, org, creator, [ + roleId, + ]); + expect( + await mrtService.getAssignedRoleIdsForQueue({ + orgId: org.id, + queueId: queue.id, + }), + ).toEqual([roleId]); + + await mrtService.updateManualReviewQueue({ + orgId: org.id, + queueId: queue.id, + userIds: [creator.id], + roleIds: [], + actionIdsToHide: [], + actionIdsToUnhide: [], + }); + + expect( + await mrtService.getAssignedRoleIdsForQueue({ + orgId: org.id, + queueId: queue.id, + }), + ).toEqual([]); + const reviewable = await mrtService.getReviewableQueuesForUser({ + invoker: reviewerInvoker(moderator.id, org.id), + }); + expect(reviewable.map((q) => q.id)).not.toContain(queue.id); + }, + ); + + testWithRoleAssignableQueue()( + 'a role from another organization is rejected without changing queue access', + async ({ + org, + creator, + moderator, + mrtService, + kyselyPg, + moderationConfigService, + apiKeyService, + }) => { + const roleId = await getRoleId(kyselyPg, org.id, UserRole.MODERATOR); + const queue = await createQueueWithRoles(mrtService, org, creator, [ + roleId, + ]); + const { org: otherOrg } = await createOrg( + { + KyselyPg: kyselyPg, + ModerationConfigService: moderationConfigService, + ApiKeyService: apiKeyService, + }, + uid(), + ); + await createUser(kyselyPg, otherOrg.id, { role: UserRole.MODERATOR }); + const otherRoleId = await getRoleId( + kyselyPg, + otherOrg.id, + UserRole.MODERATOR, + ); + + await expect( + mrtService.updateManualReviewQueue({ + orgId: org.id, + queueId: queue.id, + userIds: [creator.id], + roleIds: [otherRoleId], + actionIdsToHide: [], + actionIdsToUnhide: [], + }), + ).rejects.toThrow(); + expect( + await mrtService.getAssignedRoleIdsForQueue({ + orgId: org.id, + queueId: queue.id, + }), + ).toEqual([roleId]); + expect( + ( + await mrtService.getReviewableQueuesForUser({ + invoker: reviewerInvoker(moderator.id, org.id), + }) + ).map((q) => q.id), + ).toContain(queue.id); + }, + ); + // Regression: RESTRICT FK must block queue deletion when routing rules reference it. type QueueFixture = Parameters< Parameters>[1] @@ -443,6 +676,7 @@ describe('QueueOperations', () => { name: `delete-test-queue-${uid()}`, description: null, userIds: [user.id], + roleIds: [], hiddenActionIds: [], isAppealsQueue: false, invokedBy: { diff --git a/server/services/manualReviewToolService/modules/QueueOperations.ts b/server/services/manualReviewToolService/modules/QueueOperations.ts index bc515f5bd..19fc6d972 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.ts @@ -244,6 +244,7 @@ export default class QueueOperations { name: string; description: string | null; userIds: readonly string[]; + roleIds: readonly string[]; hiddenActionIds: readonly string[]; invokedBy: Invoker; isAppealsQueue?: boolean; @@ -256,6 +257,7 @@ export default class QueueOperations { name, description, userIds, + roleIds, hiddenActionIds, invokedBy, isAppealsQueue, @@ -277,6 +279,8 @@ export default class QueueOperations { // queue must belong to the caller's org. The queue itself is always // created in the caller's org (orgId from the invoker). await assertUsersInOrg(this.pgQuery, { orgId, userIds }); + await assertRolesInOrg(this.pgQuery, { orgId, roleIds }); + const uniqueRoleIds = [...new Set(roleIds)]; try { return await this.transactionWithRetry(async (transaction) => { @@ -319,6 +323,18 @@ export default class QueueOperations { ) .executeTakeFirstOrThrow(); + if (uniqueRoleIds.length > 0) { + await transaction + .insertInto('manual_review_tool.roles_and_accessible_queues') + .values( + uniqueRoleIds.map((roleId) => ({ + queue_id: queue.id, + role_id: roleId, + })), + ) + .execute(); + } + await this.updateHiddenActionsForQueue({ transaction, queueId: queue.id, @@ -348,6 +364,7 @@ export default class QueueOperations { name?: string; description?: string | null; userIds: readonly string[]; + roleIds: readonly string[]; actionIdsToHide: readonly string[]; actionIdsToUnhide: readonly string[]; autoCloseJobs?: boolean; @@ -362,6 +379,7 @@ export default class QueueOperations { name, description, userIds, + roleIds, actionIdsToHide, actionIdsToUnhide, autoCloseJobs, @@ -379,25 +397,34 @@ export default class QueueOperations { userIds, queueIds: [queueId], }); + await assertRolesInOrg(this.pgQuery, { orgId, roleIds }); + + const queueMetadataUpdate = removeUndefinedKeys({ + name, + description: replaceEmptyStringWithNull(description), + auto_close_jobs: autoCloseJobs, + // null disables the feature and must survive removeUndefinedKeys. + clear_reports_disposition: clearReportsDisposition, + clear_reports_scope: clearReportsScope, + }); return this.transactionWithRetry(async (transaction) => { + const updatedQueueQuery = + Object.keys(queueMetadataUpdate).length > 0 + ? transaction + .updateTable('manual_review_tool.manual_review_queues') + .set(queueMetadataUpdate) + .where('id', '=', queueId) + .where('org_id', '=', orgId) + .returning(PgQueueSelection) + : transaction + .selectFrom('manual_review_tool.manual_review_queues') + .select(PgQueueSelection) + .where('id', '=', queueId) + .where('org_id', '=', orgId); + const [updatedQueue, _, __] = await Promise.all([ - transaction - .updateTable('manual_review_tool.manual_review_queues') - .set( - removeUndefinedKeys({ - name, - description: replaceEmptyStringWithNull(description), - auto_close_jobs: autoCloseJobs, - // null disables the feature and must survive removeUndefinedKeys. - clear_reports_disposition: clearReportsDisposition, - clear_reports_scope: clearReportsScope, - }), - ) - .where('id', '=', queueId) - .where('org_id', '=', orgId) - .returning(PgQueueSelection) - .executeTakeFirstOrThrow(), + updatedQueueQuery.executeTakeFirstOrThrow(), transaction .insertInto('manual_review_tool.users_and_accessible_queues') .values( @@ -415,6 +442,29 @@ export default class QueueOperations { .executeTakeFirstOrThrow(), ]); + if (roleIds.length > 0) { + await transaction + .insertInto('manual_review_tool.roles_and_accessible_queues') + .values( + roleIds.map((roleId) => ({ + queue_id: queueId, + role_id: roleId, + })), + ) + .onConflict((oc) => oc.doNothing()) + .execute(); + await transaction + .deleteFrom('manual_review_tool.roles_and_accessible_queues') + .where('queue_id', '=', queueId) + .where('role_id', 'not in', roleIds) + .execute(); + } else { + await transaction + .deleteFrom('manual_review_tool.roles_and_accessible_queues') + .where('queue_id', '=', queueId) + .execute(); + } + await this.updateHiddenActionsForQueue({ transaction, queueId, @@ -612,18 +662,40 @@ export default class QueueOperations { .select(PgQueueSelection) .where('org_id', '=', orgId) .$if(!bypassQueuePermissions, (query) => - query.where( - 'id', - 'in', - this.pgQuery - .selectFrom('manual_review_tool.users_and_accessible_queues') - .select('queue_id') - .where('user_id', '=', userId), + query.where((eb) => + eb.or([ + eb('id', 'in', this.individuallyAccessibleQueueIds(userId)), + eb('id', 'in', this.roleAccessibleQueueIds(userId, orgId)), + ]), ), ) .execute(); } + // A queue is accessible when the reviewer is individually assigned to it, or + // when their role is assigned to it (see `roleAccessibleQueueIds`). + private individuallyAccessibleQueueIds(userId: string) { + return this.pgQuery + .selectFrom('manual_review_tool.users_and_accessible_queues') + .select('queue_id') + .where('user_id', '=', userId); + } + + private roleAccessibleQueueIds(userId: string, orgId: string) { + return this.pgQuery + .selectFrom('manual_review_tool.roles_and_accessible_queues') + .select('queue_id') + .where( + 'role_id', + '=', + this.pgQuery + .selectFrom('public.users') + .select('role_id') + .where('id', '=', userId) + .where('org_id', '=', orgId), + ); + } + async getQueueForOrg(opts: { orgId: string; userId: string; @@ -635,13 +707,11 @@ export default class QueueOperations { .select(PgQueueSelection) .where('org_id', '=', orgId) .where('id', '=', queueId) - .where( - 'id', - 'in', - this.pgQuery - .selectFrom('manual_review_tool.users_and_accessible_queues') - .select('queue_id') - .where('user_id', '=', userId), + .where((eb) => + eb.or([ + eb('id', 'in', this.individuallyAccessibleQueueIds(userId)), + eb('id', 'in', this.roleAccessibleQueueIds(userId, orgId)), + ]), ) .executeTakeFirst(); } @@ -738,6 +808,24 @@ export default class QueueOperations { .execute(); } + async getAssignedRoleIdsForQueue(opts: { orgId: string; queueId: string }) { + const { orgId, queueId } = opts; + const rows = await this.pgQuery + .selectFrom('manual_review_tool.roles_and_accessible_queues') + .select(['role_id as roleId']) + .where('queue_id', '=', queueId) + .where( + 'queue_id', + 'in', + this.pgQuery + .selectFrom('manual_review_tool.manual_review_queues') + .select('id') + .where('org_id', '=', orgId), + ) + .execute(); + return rows.map((it) => it.roleId); + } + async addAccessibleQueuesForUser(opts: { orgId: string; userIds: readonly string[]; @@ -2036,6 +2124,26 @@ async function assertUsersInOrg( } } +/** Rejects before any write if a target role does not belong to the caller's org. */ +async function assertRolesInOrg( + db: Kysely, + opts: { orgId: string; roleIds: readonly string[] }, +) { + const { orgId, roleIds } = opts; + if (roleIds.length === 0) { + return; + } + const inOrgRoles = await db + .selectFrom('public.roles') + .select('id') + .where('org_id', '=', orgId) + .where('id', 'in', roleIds) + .execute(); + if (inOrgRoles.length !== new Set(roleIds).size) { + throw makeAccessibleQueueNotInOrgError({ shouldErrorSpan: true }); + } +} + /** * Rejects before any write if a target queue or user does not belong to the * caller's org. diff --git a/server/services/manualReviewToolService/modules/UserReportSweep.test.ts b/server/services/manualReviewToolService/modules/UserReportSweep.test.ts index 68a8ded10..6814042a1 100644 --- a/server/services/manualReviewToolService/modules/UserReportSweep.test.ts +++ b/server/services/manualReviewToolService/modules/UserReportSweep.test.ts @@ -85,6 +85,7 @@ const testWithQueue = () => orgId: org.id, queueId: opts.queueId ?? queue.id, userIds: [user.id], + roleIds: [], actionIdsToHide: [], actionIdsToUnhide: [], clearReportsDisposition: opts.disposition, diff --git a/server/test/fixtureHelpers/createMrtQueue.ts b/server/test/fixtureHelpers/createMrtQueue.ts index 551ca8167..8cd8b2e61 100644 --- a/server/test/fixtureHelpers/createMrtQueue.ts +++ b/server/test/fixtureHelpers/createMrtQueue.ts @@ -12,6 +12,7 @@ export default async function (opts: { name: 'test-queue', description: null, userIds: [userId], + roleIds: [], hiddenActionIds: [], isAppealsQueue: false, invokedBy: { From 4d3980a22fdeaaa0853bd8a9b69dd9185e29f74d Mon Sep 17 00:00:00 2001 From: Juliet Shen Date: Mon, 24 Aug 2026 15:21:43 -0400 Subject: [PATCH 2/3] Consolidate queue access into a single Manage Access field Merge the separate Reviewer Access and Roles with Access selectors into one grouped multi-select (Roles / Reviewers) with purple helper text. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_0165yqq7Qs92KDXBKWF3ahFb --- .../dashboard/mrt/ManualReviewQueueForm.tsx | 119 ++++++++++-------- 1 file changed, 69 insertions(+), 50 deletions(-) diff --git a/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.tsx b/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.tsx index f0ed24fa0..b4a4b2fb9 100644 --- a/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.tsx +++ b/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.tsx @@ -27,7 +27,13 @@ import { import { titleCaseEnumStringWithArticle } from '../../../utils/string'; import { optionWithTooltip } from './queue_routing/ManualReviewQueueRuleFormCondition'; -const { Option } = Select; +const { Option, OptGroup } = Select; + +// The single "Manage Access" selector holds both roles and individual +// reviewers. Values are prefixed so onChange can split them back into role +// IDs and user IDs for the mutation. +const ACCESS_ROLE_PREFIX = 'role:'; +const ACCESS_USER_PREFIX = 'user:'; const CLEAR_REPORTS_DISPOSITION_LABELS: Record< GQLMrtClearReportsDisposition, @@ -308,6 +314,28 @@ export default function ManualReviewQueueForm() { userIdsWhoCanReviewEveryQueue, ]); + const accessValue = useMemo( + () => [ + ...rolesWithAccess.map((role) => `${ACCESS_ROLE_PREFIX}${role}`), + ...moderatorsWithAccess.map((id) => `${ACCESS_USER_PREFIX}${id}`), + ], + [rolesWithAccess, moderatorsWithAccess], + ); + + const onChangeAccess = useCallback((values: string[]) => { + const roleIds: string[] = []; + const userIds: string[] = []; + for (const value of values) { + if (value.startsWith(ACCESS_ROLE_PREFIX)) { + roleIds.push(value.slice(ACCESS_ROLE_PREFIX.length)); + } else if (value.startsWith(ACCESS_USER_PREFIX)) { + userIds.push(value.slice(ACCESS_USER_PREFIX.length)); + } + } + setRolesWithAccess(roleIds); + setModeratorsWithAccess(userIds); + }, []); + useEffect(() => { if (!queue) { return; @@ -483,64 +511,55 @@ export default function ManualReviewQueueForm() { onChangeDescription={setQueueDescription} />
-
Reviewer Access
-
- Select which moderators should have access to this queue. Note: Users - who are Admins or Moderator Managers automatically have access to - every queue, so you don't need to add them as moderators here. You can - see each user's role in our{' '} +
Manage Access
+
+ Add individual reviewers or a whole role. Everyone with a selected + role can review this queue; Admins and Moderator Managers can already + see every queue. You can view each user's role on the{' '} Users page.
- className="self-start !min-w-[160px]" - mode="multiple" - placeholder="Add Moderators" - dropdownMatchSelectWidth={false} - allowClear - showSearch - filterOption={selectFilterByLabelOption} - value={moderatorsWithAccess} - onChange={setModeratorsWithAccess} - > - {orgUsers.map((user, index) => { - const userIsAdmin = userIdsWhoCanReviewEveryQueue.includes(user.id); - return optionWithTooltip({ - title: `${user.firstName} ${user.lastName}`, - value: user.id, - disabled: userIsAdmin, - description: userIsAdmin - ? `This user is ${titleCaseEnumStringWithArticle( - user.role!, - )} and can therefore see every queue, so you can't remove them from individual queues` - : undefined, - key: user.id, - index, - isInOptionGroup: false, - }); - })} - -
Roles with Access
-
- Everyone with a selected role can review this queue, so you don't have - to add them one by one. Admins and Moderator Managers already have - access to every queue. -
- - className="self-start !min-w-[160px]" + className="self-start !min-w-[240px]" mode="multiple" - placeholder="Add Roles" + placeholder="Add roles or reviewers" dropdownMatchSelectWidth={false} allowClear showSearch filterOption={selectFilterByLabelOption} - value={rolesWithAccess} - onChange={setRolesWithAccess} + value={accessValue} + onChange={onChangeAccess} > - {orgRoles.map((role) => ( - - ))} + + {orgRoles.map((role) => ( + + ))} + + + {orgUsers.map((user, index) => { + const userIsAdmin = userIdsWhoCanReviewEveryQueue.includes( + user.id, + ); + return optionWithTooltip({ + title: `${user.firstName} ${user.lastName}`, + value: `${ACCESS_USER_PREFIX}${user.id}`, + disabled: userIsAdmin, + description: userIsAdmin + ? `This user is ${titleCaseEnumStringWithArticle( + user.role!, + )} and can therefore see every queue, so you can't remove them from individual queues` + : undefined, + key: user.id, + index, + isInOptionGroup: true, + }); + })} + {orgActions.length > 0 && (
From a262aa7d49e902feac71d0195fe1d924b40881d9 Mon Sep 17 00:00:00 2001 From: Juliet Shen Date: Mon, 24 Aug 2026 15:27:26 -0400 Subject: [PATCH 3/3] Match Manage Access helper text to page style and reword Use text-slate-500 like the other field descriptions instead of the accent color, and reword the reviewer/role sentence. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_0165yqq7Qs92KDXBKWF3ahFb --- .../src/webpages/dashboard/mrt/ManualReviewQueueForm.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.tsx b/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.tsx index b4a4b2fb9..6dc239def 100644 --- a/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.tsx +++ b/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.tsx @@ -512,10 +512,10 @@ export default function ManualReviewQueueForm() { />
Manage Access
-
- Add individual reviewers or a whole role. Everyone with a selected - role can review this queue; Admins and Moderator Managers can already - see every queue. You can view each user's role on the{' '} +
+ Grant access to individual reviewers or by role. Everyone with a + selected role can review this queue; Admins and Moderator Managers + already see every queue. You can view each user's role on the{' '} Users page.