diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b315c3b6..2b21ee3a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ For more information about each release including git tags and artifacts, see [R ### Fixed +- Existing manual review queues can now be converted to or from an appeals queue from the queue edit form ([#1176](https://github.com/roostorg/coop/pull/1176) by [@reitblatt](https://github.com/reitblatt)) - Rule history dropping other rules' versions when filtered by start date ([#1056](https://github.com/roostorg/coop/pull/1056) by [@juanmrad](https://github.com/juanmrad)) - `RetryFailedNcmecDecisionsJob` ignoring `NCMEC_ENV` and retrying test decisions ([#928](https://github.com/roostorg/coop/pull/928) by [@taobojlen](https://github.com/taobojlen)) - Queue creation failing with "name already exists" on the default reviewer selection ([#1069](https://github.com/roostorg/coop/pull/1069) by [@jess-upscrolled](https://github.com/jess-upscrolled), closes [#1074](https://github.com/roostorg/coop/issues/1074)) diff --git a/client/src/graphql/generated.ts b/client/src/graphql/generated.ts index 1bc2c55b0..6a2deee10 100644 --- a/client/src/graphql/generated.ts +++ b/client/src/graphql/generated.ts @@ -4772,6 +4772,21 @@ export type GQLTransformJobAndRecreateInQueueDecisionComponent = readonly type: GQLManualReviewDecisionType; }; +/** + * Returned when a queue can't be converted between a regular and an appeals + * queue, e.g. because it is the default queue, still has pending jobs, or is + * referenced by routing rules. The title explains which. + */ +export type GQLUnableToChangeQueueTypeError = GQLError & { + readonly __typename: 'UnableToChangeQueueTypeError'; + readonly detail?: Maybe; + readonly pointer?: Maybe; + readonly requestId?: Maybe; + readonly status: Scalars['Int']['output']; + readonly title: Scalars['String']['output']; + readonly type: ReadonlyArray; +}; + export type GQLUpdateActionInput = { readonly applyUserStrikes?: InputMaybe; readonly callbackUrl?: InputMaybe; @@ -4847,6 +4862,11 @@ export type GQLUpdateManualReviewQueueInput = { >; readonly description?: InputMaybe; readonly id: Scalars['ID']['input']; + /** + * When provided, converts the queue to or from an appeals queue. Omit to + * leave the queue's type unchanged. + */ + readonly isAppealsQueue?: InputMaybe; readonly name?: InputMaybe; readonly userIds: ReadonlyArray; }; @@ -4854,7 +4874,8 @@ export type GQLUpdateManualReviewQueueInput = { export type GQLUpdateManualReviewQueueQueueResponse = | GQLManualReviewQueueNameExistsError | GQLMutateManualReviewQueueSuccessResponse - | GQLNotFoundError; + | GQLNotFoundError + | GQLUnableToChangeQueueTypeError; export type GQLUpdateNcmecOrgSettingsResponse = { readonly __typename: 'UpdateNcmecOrgSettingsResponse'; @@ -10024,6 +10045,12 @@ export type GQLUpdateManualReviewQueueMutation = { readonly title: string; readonly status: number; readonly type: ReadonlyArray; + } + | { + readonly __typename: 'UnableToChangeQueueTypeError'; + readonly title: string; + readonly status: number; + readonly type: ReadonlyArray; }; }; @@ -33238,6 +33265,11 @@ export const GQLUpdateManualReviewQueueDocument = gql` status type } + ... on UnableToChangeQueueTypeError { + title + status + type + } ... on NotFoundError { title status diff --git a/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.test.tsx b/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.test.tsx new file mode 100644 index 000000000..fdd0a7dab --- /dev/null +++ b/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.test.tsx @@ -0,0 +1,192 @@ +import { TooltipProvider } from '@/coop-ui/Tooltip'; +import { MockedProvider, type MockedResponse } from '@apollo/client/testing'; +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { HelmetProvider } from 'react-helmet-async'; +import { MemoryRouter, Route, Routes } from 'react-router-dom'; + +import '@testing-library/jest-dom/extend-expect'; + +import { + GQLManualReviewQueueDocument, + GQLQueueFormDataDocument, + GQLUpdateManualReviewQueueDocument, + type GQLUpdateManualReviewQueueMutationVariables, +} from '@/graphql/generated'; + +import ManualReviewQueueForm from './ManualReviewQueueForm'; + +const QUEUE_ID = 'queue-1'; + +function queueFormDataMock(hasAppealsEnabled: boolean): MockedResponse { + return { + request: { query: GQLQueueFormDataDocument }, + maxUsageCount: Infinity, + result: { + data: { + myOrg: { + __typename: 'Org', + hasAppealsEnabled, + hasPartialItemsEndpoint: false, + users: [], + actions: [], + usersWhoCanReviewEveryQueue: [], + }, + }, + }, + }; +} + +function manualReviewQueueMock(isAppealsQueue: boolean): MockedResponse { + return { + request: { + query: GQLManualReviewQueueDocument, + variables: { id: QUEUE_ID }, + }, + maxUsageCount: Infinity, + result: { + data: { + manualReviewQueue: { + __typename: 'ManualReviewQueue', + id: QUEUE_ID, + name: 'Existing Queue', + description: null, + explicitlyAssignedReviewers: [], + hiddenActionIds: [], + isAppealsQueue, + autoCloseJobs: false, + clearReportsDisposition: null, + clearReportsScope: 'CURRENT_QUEUE', + clearReportsTriggerActionIds: [], + }, + }, + }, + }; +} + +function renderEditForm(mocks: MockedResponse[]) { + return render( + + + + + + } + /> + + + + + , + ); +} + +const appealsCheckbox = () => + screen.queryByRole('checkbox', { name: /this is an appeals queue/i }); + +describe('ManualReviewQueueForm (edit)', () => { + it('shows the appeals queue checkbox when appeals are enabled', async () => { + renderEditForm([queueFormDataMock(true), manualReviewQueueMock(false)]); + + await screen.findByText('Update Manual Review Queue'); + await waitFor(() => expect(appealsCheckbox()).toBeInTheDocument()); + expect(appealsCheckbox()).not.toBeChecked(); + expect( + screen.getByText(/can only be converted to or from an appeals queue/i), + ).toBeInTheDocument(); + }); + + it('reflects the queue being an appeals queue already', async () => { + renderEditForm([queueFormDataMock(true), manualReviewQueueMock(true)]); + + await screen.findByText('Update Manual Review Queue'); + await waitFor(() => expect(appealsCheckbox()).toBeChecked()); + }); + + it('hides the appeals queue checkbox when appeals are disabled', async () => { + renderEditForm([queueFormDataMock(false), manualReviewQueueMock(false)]); + + await screen.findByText('Update Manual Review Queue'); + expect(appealsCheckbox()).not.toBeInTheDocument(); + }); + + it('sends the new appeals flag when saving', async () => { + let calledVariables: + GQLUpdateManualReviewQueueMutationVariables | undefined; + const updateMock: MockedResponse = { + request: { query: GQLUpdateManualReviewQueueDocument }, + variableMatcher: (variables) => { + calledVariables = + variables as GQLUpdateManualReviewQueueMutationVariables; + return true; + }, + result: { + data: { + updateManualReviewQueue: { + __typename: 'MutateManualReviewQueueSuccessResponse', + data: { + __typename: 'ManualReviewQueue', + id: QUEUE_ID, + name: 'Existing Queue', + description: null, + }, + }, + }, + }, + }; + renderEditForm([ + queueFormDataMock(true), + manualReviewQueueMock(false), + updateMock, + ]); + + await screen.findByText('Update Manual Review Queue'); + await waitFor(() => expect(appealsCheckbox()).toBeInTheDocument()); + fireEvent.click(appealsCheckbox()!); + await waitFor(() => expect(appealsCheckbox()).toBeChecked()); + fireEvent.click(screen.getByRole('button', { name: /save changes/i })); + + await waitFor(() => expect(calledVariables).toBeDefined()); + expect(calledVariables?.input).toMatchObject({ + id: QUEUE_ID, + isAppealsQueue: true, + }); + expect(await screen.findByText('Changes Saved')).toBeInTheDocument(); + }); + + it('surfaces the server explanation when the queue cannot be converted', async () => { + const title = + 'This queue cannot be converted while it still has pending jobs. Empty the queue first.'; + const updateMock: MockedResponse = { + request: { query: GQLUpdateManualReviewQueueDocument }, + variableMatcher: () => true, + result: { + data: { + updateManualReviewQueue: { + __typename: 'UnableToChangeQueueTypeError', + title, + status: 409, + type: ['/errors/conflict'], + }, + }, + }, + }; + renderEditForm([ + queueFormDataMock(true), + manualReviewQueueMock(false), + updateMock, + ]); + + await screen.findByText('Update Manual Review Queue'); + await waitFor(() => expect(appealsCheckbox()).toBeInTheDocument()); + fireEvent.click(appealsCheckbox()!); + fireEvent.click(screen.getByRole('button', { name: /save changes/i })); + + expect(await screen.findByText(title)).toBeInTheDocument(); + }); +}); diff --git a/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.tsx b/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.tsx index f5e699aca..93c6280fb 100644 --- a/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.tsx +++ b/client/src/webpages/dashboard/mrt/ManualReviewQueueForm.tsx @@ -121,6 +121,11 @@ gql` status type } + ... on UnableToChangeQueueTypeError { + title + status + type + } ... on NotFoundError { title status @@ -215,6 +220,13 @@ export default function ManualReviewQueueForm() { buttonText: 'OK', }); break; + case 'UnableToChangeQueueTypeError': + setModalInfo({ + title: 'Error Saving Changes', + body: response.updateManualReviewQueue.title, + buttonText: 'OK', + }); + break; case 'MutateManualReviewQueueSuccessResponse': setModalInfo({ title: 'Changes Saved', @@ -388,6 +400,7 @@ export default function ManualReviewQueueForm() { hiddenActionIds, ), autoCloseJobs, + isAppealsQueue, clearReportsDisposition, clearReportsScope, clearReportsTriggerActionIds: @@ -405,6 +418,7 @@ export default function ManualReviewQueueForm() { hiddenActionIds, id, initiallyHiddenActionIds, + isAppealsQueue, moderatorsWithAccess, queueDescription, queueName, @@ -633,7 +647,7 @@ export default function ManualReviewQueueForm() { )} )} - {isCreateForm && data?.myOrg?.hasAppealsEnabled ? ( + {data?.myOrg?.hasAppealsEnabled ? (
+ {!isCreateForm && ( +
+ An existing queue can only be converted to or from an appeals + queue while it has no pending jobs and no routing rules point to + it. The default queue cannot be converted. +
+ )}
) : null} {divider()} diff --git a/server/graphql/generated.ts b/server/graphql/generated.ts index e50807283..660898d0d 100644 --- a/server/graphql/generated.ts +++ b/server/graphql/generated.ts @@ -4840,6 +4840,21 @@ export type GQLTransformJobAndRecreateInQueueDecisionComponent = readonly type: GQLManualReviewDecisionType; }; +/** + * Returned when a queue can't be converted between a regular and an appeals + * queue, e.g. because it is the default queue, still has pending jobs, or is + * referenced by routing rules. The title explains which. + */ +export type GQLUnableToChangeQueueTypeError = GQLError & { + readonly __typename?: 'UnableToChangeQueueTypeError'; + readonly detail?: Maybe; + readonly pointer?: Maybe; + readonly requestId?: Maybe; + readonly status: Scalars['Int']['output']; + readonly title: Scalars['String']['output']; + readonly type: ReadonlyArray; +}; + export type GQLUpdateActionInput = { readonly applyUserStrikes?: InputMaybe; readonly callbackUrl?: InputMaybe; @@ -4915,6 +4930,11 @@ export type GQLUpdateManualReviewQueueInput = { >; readonly description?: InputMaybe; readonly id: Scalars['ID']['input']; + /** + * When provided, converts the queue to or from an appeals queue. Omit to + * leave the queue's type unchanged. + */ + readonly isAppealsQueue?: InputMaybe; readonly name?: InputMaybe; readonly userIds: ReadonlyArray; }; @@ -4922,7 +4942,8 @@ export type GQLUpdateManualReviewQueueInput = { export type GQLUpdateManualReviewQueueQueueResponse = | GQLManualReviewQueueNameExistsError | GQLMutateManualReviewQueueSuccessResponse - | GQLNotFoundError; + | GQLNotFoundError + | GQLUnableToChangeQueueTypeError; export type GQLUpdateNcmecOrgSettingsResponse = { readonly __typename?: 'UpdateNcmecOrgSettingsResponse'; @@ -5698,7 +5719,8 @@ export type GQLResolversUnionTypes<_RefType extends Record> = { | (Omit & { data: _RefType['ManualReviewQueue']; }) - | GQLNotFoundError; + | GQLNotFoundError + | GQLUnableToChangeQueueTypeError; UpdatePolicyResponse: GQLNotFoundError | GQLPolicy; UpdateReportingRuleResponse: | (Omit & { @@ -5769,7 +5791,8 @@ export type GQLResolversInterfaceTypes< | GQLRuleHasRunningBacktestsError | GQLRuleNameExistsError | GQLSignUpUserExistsError - | GQLSubmittedJobActionNotFoundError; + | GQLSubmittedJobActionNotFoundError + | GQLUnableToChangeQueueTypeError; Field: | GQLBaseField | (Omit & { spec: _RefType['DerivedFieldSpec'] }); @@ -6575,6 +6598,7 @@ export type GQLResolversTypes = { TopPolicyViolationsInput: GQLTopPolicyViolationsInput; TransformJobAndRecreateInQueue: GQLTransformJobAndRecreateInQueue; TransformJobAndRecreateInQueueDecisionComponent: ResolverTypeWrapper; + UnableToChangeQueueTypeError: ResolverTypeWrapper; UpdateActionInput: GQLUpdateActionInput; UpdateContentItemTypeInput: GQLUpdateContentItemTypeInput; UpdateContentRuleInput: GQLUpdateContentRuleInput; @@ -7242,6 +7266,7 @@ export type GQLResolversParentTypes = { TopPolicyViolationsInput: GQLTopPolicyViolationsInput; TransformJobAndRecreateInQueue: GQLTransformJobAndRecreateInQueue; TransformJobAndRecreateInQueueDecisionComponent: GQLTransformJobAndRecreateInQueueDecisionComponent; + UnableToChangeQueueTypeError: GQLUnableToChangeQueueTypeError; UpdateActionInput: GQLUpdateActionInput; UpdateContentItemTypeInput: GQLUpdateContentItemTypeInput; UpdateContentRuleInput: GQLUpdateContentRuleInput; @@ -9036,7 +9061,8 @@ export type GQLErrorResolvers< | 'RuleHasRunningBacktestsError' | 'RuleNameExistsError' | 'SignUpUserExistsError' - | 'SubmittedJobActionNotFoundError', + | 'SubmittedJobActionNotFoundError' + | 'UnableToChangeQueueTypeError', ParentType, ContextType >; @@ -14518,6 +14544,36 @@ export type GQLTransformJobAndRecreateInQueueDecisionComponentResolvers< __isTypeOf?: IsTypeOfResolverFn; }; +export type GQLUnableToChangeQueueTypeErrorResolvers< + ContextType = Context, + ParentType extends GQLResolversParentTypes['UnableToChangeQueueTypeError'] = + GQLResolversParentTypes['UnableToChangeQueueTypeError'], +> = { + detail?: Resolver< + Maybe, + ParentType, + ContextType + >; + pointer?: Resolver< + Maybe, + ParentType, + ContextType + >; + requestId?: Resolver< + Maybe, + ParentType, + ContextType + >; + status?: Resolver; + title?: Resolver; + type?: Resolver< + ReadonlyArray, + ParentType, + ContextType + >; + __isTypeOf?: IsTypeOfResolverFn; +}; + export type GQLUpdateContentRuleResponseResolvers< ContextType = Context, ParentType extends GQLResolversParentTypes['UpdateContentRuleResponse'] = @@ -14542,7 +14598,8 @@ export type GQLUpdateManualReviewQueueQueueResponseResolvers< __resolveType: TypeResolveFn< | 'ManualReviewQueueNameExistsError' | 'MutateManualReviewQueueSuccessResponse' - | 'NotFoundError', + | 'NotFoundError' + | 'UnableToChangeQueueTypeError', ParentType, ContextType >; @@ -15490,6 +15547,7 @@ export type GQLResolvers = { ThreadWithMessagesAndIpAddress?: GQLThreadWithMessagesAndIpAddressResolvers; TimeToAction?: GQLTimeToActionResolvers; TransformJobAndRecreateInQueueDecisionComponent?: GQLTransformJobAndRecreateInQueueDecisionComponentResolvers; + UnableToChangeQueueTypeError?: GQLUnableToChangeQueueTypeErrorResolvers; UpdateContentRuleResponse?: GQLUpdateContentRuleResponseResolvers; UpdateManualReviewQueueQueueResponse?: GQLUpdateManualReviewQueueQueueResponseResolvers; UpdateNcmecOrgSettingsResponse?: GQLUpdateNcmecOrgSettingsResponseResolvers; diff --git a/server/graphql/modules/manualReviewTool.ts b/server/graphql/modules/manualReviewTool.ts index f76fefe89..ffaf1178b 100644 --- a/server/graphql/modules/manualReviewTool.ts +++ b/server/graphql/modules/manualReviewTool.ts @@ -412,9 +412,24 @@ const typeDefs = /* GraphQL */ ` | MutateManualReviewQueueSuccessResponse | ManualReviewQueueNameExistsError + """ + Returned when a queue can't be converted between a regular and an appeals + queue, e.g. because it is the default queue, still has pending jobs, or is + referenced by routing rules. The title explains which. + """ + type UnableToChangeQueueTypeError implements Error { + title: String! + status: Int! + type: [String!]! + pointer: String + detail: String + requestId: String + } + union UpdateManualReviewQueueQueueResponse = | MutateManualReviewQueueSuccessResponse | ManualReviewQueueNameExistsError + | UnableToChangeQueueTypeError | NotFoundError input CreateManualReviewQueueInput { @@ -437,6 +452,11 @@ const typeDefs = /* GraphQL */ ` actionIdsToHide: [ID!]! actionIdsToUnhide: [ID!]! autoCloseJobs: Boolean! + """ + When provided, converts the queue to or from an appeals queue. Omit to + leave the queue's type unchanged. + """ + isAppealsQueue: Boolean clearReportsDisposition: MrtClearReportsDisposition clearReportsScope: MrtClearReportsScope clearReportsTriggerActionIds: [ID!] @@ -2473,6 +2493,7 @@ const Mutation: GQLMutationResolvers = { actionIdsToHide, actionIdsToUnhide, autoCloseJobs, + isAppealsQueue, clearReportsDisposition, clearReportsScope, clearReportsTriggerActionIds, @@ -2490,6 +2511,7 @@ const Mutation: GQLMutationResolvers = { actionIdsToHide, actionIdsToUnhide, autoCloseJobs, + isAppealsQueue: isAppealsQueue ?? undefined, clearReportsDisposition, clearReportsScope: clearReportsScope ?? undefined, clearReportsTriggerActionIds: @@ -2500,7 +2522,12 @@ const Mutation: GQLMutationResolvers = { 'MutateManualReviewQueueSuccessResponse', ); } catch (e: unknown) { - if (isCoopErrorOfType(e, 'ManualReviewQueueNameExistsError')) { + if ( + isCoopErrorOfType(e, [ + 'ManualReviewQueueNameExistsError', + 'UnableToChangeQueueTypeError', + ]) + ) { return gqlErrorResult(e); } diff --git a/server/services/manualReviewToolService/manualReviewToolService.ts b/server/services/manualReviewToolService/manualReviewToolService.ts index bea70a9ca..ee92a8f8e 100644 --- a/server/services/manualReviewToolService/manualReviewToolService.ts +++ b/server/services/manualReviewToolService/manualReviewToolService.ts @@ -918,6 +918,7 @@ export class ManualReviewToolService { actionIdsToHide: readonly string[]; actionIdsToUnhide: readonly string[]; autoCloseJobs?: boolean; + isAppealsQueue?: boolean; clearReportsDisposition?: ClearReportsDisposition | null; clearReportsScope?: ClearReportsScope; clearReportsTriggerActionIds?: readonly string[]; diff --git a/server/services/manualReviewToolService/modules/QueueOperations.test.ts b/server/services/manualReviewToolService/modules/QueueOperations.test.ts index b0422f29c..fece08f4f 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.test.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.test.ts @@ -434,6 +434,171 @@ describe('QueueOperations', () => { | 'manual_review_tool.routing_rules' | 'manual_review_tool.appeals_routing_rules'; + // Converting a queue between regular and appeals: the two kinds keep their + // jobs in separate Bull queues with different payload shapes, so the + // conversion is only allowed while nothing would be orphaned. + describe('updateManualReviewQueue isAppealsQueue', () => { + const createNonDefaultQueue = async ( + { org, user, mrtService }: QueueFixture, + opts: { isAppealsQueue: boolean }, + ) => + mrtService.createManualReviewQueue({ + name: `convert-test-queue-${uid()}`, + description: null, + userIds: [user.id], + hiddenActionIds: [], + isAppealsQueue: opts.isAppealsQueue, + invokedBy: { + userId: user.id, + permissions: [UserPermission.EDIT_MRT_QUEUES], + orgId: org.id, + }, + }); + + const convert = async ( + { org, user, mrtService }: QueueFixture, + queueId: string, + isAppealsQueue: boolean | undefined, + ) => + mrtService.updateManualReviewQueue({ + orgId: org.id, + queueId, + userIds: [user.id], + actionIdsToHide: [], + actionIdsToUnhide: [], + // Always sent by the GraphQL resolver; without at least one column to + // set, the UPDATE has an empty SET clause. + autoCloseJobs: false, + isAppealsQueue, + }); + + testWithQueueAndActions()( + 'converts a non-default queue to an appeals queue and back', + async (fixture) => { + // The org already has a default appeals queue, so the converted + // queue must not become the default of either type. + await createNonDefaultQueue(fixture, { isAppealsQueue: true }); + const queue = await createNonDefaultQueue(fixture, { + isAppealsQueue: false, + }); + expect(queue.isDefaultQueue).toBe(false); + + const converted = await convert(fixture, queue.id, true); + expect(converted.isAppealsQueue).toBe(true); + expect(converted.isDefaultQueue).toBe(false); + + const reverted = await convert(fixture, queue.id, false); + expect(reverted.isAppealsQueue).toBe(false); + expect(reverted.isDefaultQueue).toBe(false); + }, + ); + + testWithQueueAndActions()( + 'a converted queue becomes the default appeals queue when the org has none', + async (fixture) => { + const queue = await createNonDefaultQueue(fixture, { + isAppealsQueue: false, + }); + + const converted = await convert(fixture, queue.id, true); + expect(converted.isAppealsQueue).toBe(true); + expect(converted.isDefaultQueue).toBe(true); + await expect( + fixture.mrtService['queueOps'].getDefaultAppealsQueueIdForOrg( + fixture.org.id, + ), + ).resolves.toBe(queue.id); + }, + ); + + testWithQueueAndActions()( + 'leaves the queue untouched when isAppealsQueue is omitted or unchanged', + async (fixture) => { + // The fixture queue is the org's default queue; passing its current + // type must not trip the default-queue guard. + const { queue } = fixture; + expect(queue.isDefaultQueue).toBe(true); + + const omitted = await convert(fixture, queue.id, undefined); + expect(omitted.isAppealsQueue).toBe(false); + expect(omitted.isDefaultQueue).toBe(true); + + const unchanged = await convert(fixture, queue.id, false); + expect(unchanged.isAppealsQueue).toBe(false); + expect(unchanged.isDefaultQueue).toBe(true); + }, + ); + + testWithQueueAndActions()( + 'refuses to convert the default queue', + async (fixture) => { + await expect( + convert(fixture, fixture.queue.id, true), + ).rejects.toMatchObject({ + name: 'UnableToChangeQueueTypeError', + title: expect.stringContaining('default queue'), + }); + }, + ); + + testWithQueueAndActions()( + 'refuses to convert a queue that still has pending jobs', + async (fixture) => { + const { org, mrtService } = fixture; + const queue = await createNonDefaultQueue(fixture, { + isAppealsQueue: false, + }); + await mrtService['queueOps']['addJob']({ + orgId: org.id, + queueId: queue.id, + enqueueSourceInfo: { kind: 'REPORT' }, + jobPayload: makeDummyMrtJobPayload(), + }); + + await expect(convert(fixture, queue.id, true)).rejects.toMatchObject({ + name: 'UnableToChangeQueueTypeError', + title: expect.stringContaining('pending jobs'), + }); + + const unchanged = + await mrtService.getQueueForOrgAndDangerouslyBypassPermissioning({ + orgId: org.id, + queueId: queue.id, + }); + expect(unchanged?.isAppealsQueue).toBe(false); + }, + ); + + testWithQueueAndActions()( + 'refuses to convert a queue referenced by a routing rule', + async (fixture) => { + const { org, user, kyselyPg } = fixture; + const queue = await createNonDefaultQueue(fixture, { + isAppealsQueue: false, + }); + await kyselyPg + .insertInto('manual_review_tool.routing_rules') + .values({ + id: uuidv1(), + org_id: org.id, + name: 'rule-blocking-conversion', + description: null, + status: 'LIVE', + condition_set: { conditions: [], conjunction: 'AND' }, + destination_queue_id: queue.id, + creator_id: user.id, + sequence_number: 98, + }) + .execute(); + + await expect(convert(fixture, queue.id, true)).rejects.toMatchObject({ + name: 'UnableToChangeQueueTypeError', + title: expect.stringContaining('rule-blocking-conversion'), + }); + }, + ); + }); + const expectDeletionBlockedByRoutingRule = async ( { org, user, mrtService, kyselyPg }: QueueFixture, table: RuleTable, diff --git a/server/services/manualReviewToolService/modules/QueueOperations.ts b/server/services/manualReviewToolService/modules/QueueOperations.ts index bc515f5bd..d5953576f 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.ts @@ -110,7 +110,8 @@ export type QueueOperationsErrorType = | 'QueueDoesNotExistError' | 'UnableToDeleteDefaultQueueError' | 'AccessibleQueueNotInOrgError' - | 'QueueHasDependentRoutingRulesError'; + | 'QueueHasDependentRoutingRulesError' + | 'UnableToChangeQueueTypeError'; // Compound identifier for a queue. orgId is needed for security, but also // because queues are/will be actually sharded across redis instances for @@ -280,16 +281,10 @@ export default class QueueOperations { try { return await this.transactionWithRetry(async (transaction) => { - // In newer versions of kysely, this is greatly simplified with - // `transaction.selectNoFrom(eb => eb.exists(...))`, but we're blocked on - // updating by https://github.com/kysely-org/kysely/issues/577#issuecomment-1804900006 - const orgHasQueuesAlready = await transaction - .selectFrom('manual_review_tool.manual_review_queues') - .where('org_id', '=', orgId) - .where('is_appeals_queue', '=', isAppealsQueue ?? false) - .limit(1) - .execute() - .then((queues) => queues.length > 0); + const orgHasQueuesAlready = await this.#orgHasQueuesOfType( + transaction, + { orgId, isAppealsQueue: isAppealsQueue ?? false }, + ); const queue = await transaction .insertInto('manual_review_tool.manual_review_queues') @@ -351,6 +346,10 @@ export default class QueueOperations { actionIdsToHide: readonly string[]; actionIdsToUnhide: readonly string[]; autoCloseJobs?: boolean; + // When provided and different from the queue's current value, converts + // the queue between a regular and an appeals queue. See + // {@link #assertQueueTypeCanChange} for the preconditions. + isAppealsQueue?: boolean; clearReportsDisposition?: ClearReportsDisposition | null; clearReportsScope?: ClearReportsScope; // When provided, replaces the queue's full set of trigger actions. @@ -365,6 +364,7 @@ export default class QueueOperations { actionIdsToHide, actionIdsToUnhide, autoCloseJobs, + isAppealsQueue, clearReportsDisposition, clearReportsScope, clearReportsTriggerActionIds, @@ -380,7 +380,29 @@ export default class QueueOperations { queueIds: [queueId], }); + const queueTypeChange = + isAppealsQueue === undefined + ? undefined + : await this.#assertQueueTypeCanChange({ + orgId, + queueId, + isAppealsQueue, + }); + return this.transactionWithRetry(async (transaction) => { + // A converted queue becomes the default queue of its new type if the + // org has none yet, mirroring createManualReviewQueue. The partial + // unique index manual_review_queue_is_default guarantees at most one + // default per (org, type), so a concurrent conversion fails loudly + // rather than leaving two defaults. + const isDefaultQueue = + queueTypeChange === undefined + ? undefined + : !(await this.#orgHasQueuesOfType(transaction, { + orgId, + isAppealsQueue: queueTypeChange.isAppealsQueue, + })); + const [updatedQueue, _, __] = await Promise.all([ transaction .updateTable('manual_review_tool.manual_review_queues') @@ -389,6 +411,8 @@ export default class QueueOperations { name, description: replaceEmptyStringWithNull(description), auto_close_jobs: autoCloseJobs, + is_appeals_queue: queueTypeChange?.isAppealsQueue, + is_default_queue: isDefaultQueue, // null disables the feature and must survive removeUndefinedKeys. clear_reports_disposition: clearReportsDisposition, clear_reports_scope: clearReportsScope, @@ -434,6 +458,121 @@ export default class QueueOperations { }); } + async #orgHasQueuesOfType( + db: + | Kysely + | Transaction, + opts: { orgId: string; isAppealsQueue: boolean }, + ) { + const { orgId, isAppealsQueue } = opts; + // In newer versions of kysely, this is greatly simplified with + // `db.selectNoFrom(eb => eb.exists(...))`, but we're blocked on updating + // by https://github.com/kysely-org/kysely/issues/577#issuecomment-1804900006 + const queues = await db + .selectFrom('manual_review_tool.manual_review_queues') + .select(['id']) + .where('org_id', '=', orgId) + .where('is_appeals_queue', '=', isAppealsQueue) + .limit(1) + .execute(); + return queues.length > 0; + } + + /** + * Checks whether a queue can be converted between a regular and an appeals + * queue. Regular and appeals jobs live in separate Bull queues with + * different payload shapes, so a conversion can't carry jobs across; and + * routing rules are type-specific, so a rule pointing at a converted queue + * would enqueue jobs nobody can see. Rather than silently orphaning either, + * we refuse the change until the operator has emptied the queue and + * repointed the rules. + * + * @returns undefined when no change is needed (the queue is already of the + * requested type), otherwise the type to convert to. + */ + async #assertQueueTypeCanChange(opts: { + orgId: string; + queueId: string; + isAppealsQueue: boolean; + }): Promise<{ isAppealsQueue: boolean } | undefined> { + const { orgId, queueId, isAppealsQueue } = opts; + const queue = await this.pgQuery + .selectFrom('manual_review_tool.manual_review_queues') + .select(['is_appeals_queue', 'is_default_queue']) + .where('id', '=', queueId) + .where('org_id', '=', orgId) + .executeTakeFirst(); + if (queue === undefined) { + throw makeQueueDoesNotExistError({ shouldErrorSpan: true }); + } + if (queue.is_appeals_queue === isAppealsQueue) { + return undefined; + } + + // Routing falls back to the default queue of each type, so converting + // it would leave the org with no fallback (see getDefaultQueueIdForOrg + // and getDefaultAppealsQueueIdForOrg). This matches the guard in + // deleteManualReviewQueue. + if (queue.is_default_queue) { + throw makeUnableToChangeQueueTypeError( + 'The default queue cannot be converted to or from an appeals queue.', + { shouldErrorSpan: false }, + ); + } + + const [routingRules, appealsRoutingRules] = await Promise.all([ + this.pgQuery + .selectFrom('manual_review_tool.routing_rules') + .select(['name']) + .where('destination_queue_id', '=', queueId) + .where('org_id', '=', orgId) + .execute(), + this.pgQuery + .selectFrom('manual_review_tool.appeals_routing_rules') + .select(['name']) + .where('destination_queue_id', '=', queueId) + .where('org_id', '=', orgId) + .execute(), + ]); + const ruleNames = [ + ...routingRules.map((r) => r.name), + ...appealsRoutingRules.map((r) => r.name), + ]; + if (ruleNames.length > 0) { + throw makeUnableToChangeQueueTypeError( + `This queue cannot be converted while it is used by the following routing rules: ${ruleNames.join(', ')}. Update or delete those rules first.`, + { shouldErrorSpan: false, detail: jsonStringify(ruleNames) }, + ); + } + + // Count every job that hasn't been fully processed yet, not just the + // waiting/delayed ones that Queue.count() reports: a job that a reviewer + // is currently holding a lock on would be orphaned all the same. + const bullQueue = queue.is_appeals_queue + ? await this.getOrCreateBullAppealQueue({ orgId, queueId }) + : await this.getOrCreateBullQueue({ orgId, queueId }); + const counts = await bullQueue.getJobCounts( + 'waiting', + 'delayed', + 'active', + 'prioritized', + 'paused', + 'waiting-children', + ); + const numPendingJobs = Object.values(counts).reduce( + (sum, count) => sum + count, + 0, + ); + if (numPendingJobs > 0) { + throw makeUnableToChangeQueueTypeError( + 'This queue cannot be converted while it still has pending jobs. Empty the queue first.', + { shouldErrorSpan: false }, + ); + } + + return { isAppealsQueue }; + } + /** * @returns true when the queue that was trying to be deleted * exists and is successfully deleted, false when the queue @@ -2115,6 +2254,18 @@ export const makeManualReviewQueueNameExistsError = (data: ErrorInstanceData) => ...data, }); +export const makeUnableToChangeQueueTypeError = ( + title: string, + data: ErrorInstanceData, +) => + new CoopError({ + status: 409, + type: [ErrorType.Conflict], + title, + name: 'UnableToChangeQueueTypeError', + ...data, + }); + export const makeQueueHasDependentRoutingRulesError = ( ruleNames: string[], data: ErrorInstanceData,