diff --git a/client/src/graphql/generated.ts b/client/src/graphql/generated.ts index 3d6b33e..78d8dac 100644 --- a/client/src/graphql/generated.ts +++ b/client/src/graphql/generated.ts @@ -2527,6 +2527,7 @@ export type GQLMutation = { readonly updatePreviewJobsViewEnabled: Scalars['Boolean']['output']; readonly updateReportingRule: GQLUpdateReportingRuleResponse; readonly updateRequiresDecisionReason: Scalars['Boolean']['output']; + readonly updateRequiresDecisionReasonOnIgnore: Scalars['Boolean']['output']; readonly updateRequiresPolicyForDecisions: Scalars['Boolean']['output']; readonly updateRole?: Maybe; readonly updateRolePermissions: GQLRole; @@ -2870,6 +2871,10 @@ export type GQLMutationUpdateRequiresDecisionReasonArgs = { enabled: Scalars['Boolean']['input']; }; +export type GQLMutationUpdateRequiresDecisionReasonOnIgnoreArgs = { + enabled: Scalars['Boolean']['input']; +}; + export type GQLMutationUpdateRequiresPolicyForDecisionsArgs = { enabled: Scalars['Boolean']['input']; }; @@ -3228,6 +3233,7 @@ export type GQLOrg = { readonly publicSigningKey: Scalars['String']['output']; readonly reportingRules: ReadonlyArray; readonly requiresDecisionReasonInMrt: Scalars['Boolean']['output']; + readonly requiresDecisionReasonOnIgnoreInMrt: Scalars['Boolean']['output']; readonly requiresPolicyForDecisionsInMrt: Scalars['Boolean']['output']; readonly routingRules: ReadonlyArray; readonly rules: ReadonlyArray; @@ -12205,6 +12211,7 @@ export type GQLManualReviewJobInfoQuery = { readonly hasNCMECReportingEnabled: boolean; readonly requiresPolicyForDecisionsInMrt: boolean; readonly requiresDecisionReasonInMrt: boolean; + readonly requiresDecisionReasonOnIgnoreInMrt: boolean; readonly allowMultiplePoliciesPerAction: boolean; readonly hideSkipButtonForNonAdmins: boolean; readonly policies: ReadonlyArray<{ @@ -24815,6 +24822,7 @@ export type GQLDeploymentSettingsQuery = { readonly allowMultiplePoliciesPerAction: boolean; readonly requiresPolicyForDecisionsInMrt: boolean; readonly requiresDecisionReasonInMrt: boolean; + readonly requiresDecisionReasonOnIgnoreInMrt: boolean; readonly previewJobsViewEnabled: boolean; readonly hideSkipButtonForNonAdmins: boolean; readonly userStrikeTTL: number; @@ -24887,6 +24895,15 @@ export type GQLUpdateRequiresDecisionReasonMutation = { readonly updateRequiresDecisionReason: boolean; }; +export type GQLUpdateRequiresDecisionReasonOnIgnoreMutationVariables = Exact<{ + enabled: Scalars['Boolean']['input']; +}>; + +export type GQLUpdateRequiresDecisionReasonOnIgnoreMutation = { + readonly __typename: 'Mutation'; + readonly updateRequiresDecisionReasonOnIgnore: boolean; +}; + export type GQLUpdateHideSkipButtonForNonAdminsMutationVariables = Exact<{ enabled: Scalars['Boolean']['input']; }>; @@ -34313,6 +34330,7 @@ export const GQLManualReviewJobInfoDocument = gql` hasNCMECReportingEnabled requiresPolicyForDecisionsInMrt requiresDecisionReasonInMrt + requiresDecisionReasonOnIgnoreInMrt allowMultiplePoliciesPerAction hideSkipButtonForNonAdmins } @@ -43955,6 +43973,7 @@ export const GQLDeploymentSettingsDocument = gql` allowMultiplePoliciesPerAction requiresPolicyForDecisionsInMrt requiresDecisionReasonInMrt + requiresDecisionReasonOnIgnoreInMrt previewJobsViewEnabled hideSkipButtonForNonAdmins userStrikeTTL @@ -44358,6 +44377,55 @@ export type GQLUpdateRequiresDecisionReasonMutationOptions = GQLUpdateRequiresDecisionReasonMutation, GQLUpdateRequiresDecisionReasonMutationVariables >; +export const GQLUpdateRequiresDecisionReasonOnIgnoreDocument = gql` + mutation UpdateRequiresDecisionReasonOnIgnore($enabled: Boolean!) { + updateRequiresDecisionReasonOnIgnore(enabled: $enabled) + } +`; +export type GQLUpdateRequiresDecisionReasonOnIgnoreMutationFn = + Apollo.MutationFunction< + GQLUpdateRequiresDecisionReasonOnIgnoreMutation, + GQLUpdateRequiresDecisionReasonOnIgnoreMutationVariables + >; + +/** + * __useGQLUpdateRequiresDecisionReasonOnIgnoreMutation__ + * + * To run a mutation, you first call `useGQLUpdateRequiresDecisionReasonOnIgnoreMutation` within a React component and pass it any options that fit your needs. + * When your component renders, `useGQLUpdateRequiresDecisionReasonOnIgnoreMutation` returns a tuple that includes: + * - A mutate function that you can call at any time to execute the mutation + * - An object with fields that represent the current status of the mutation's execution + * + * @param baseOptions options that will be passed into the mutation, supported options are listed on: https://www.apollographql.com/docs/react/api/react-hooks/#options-2; + * + * @example + * const [gqlUpdateRequiresDecisionReasonOnIgnoreMutation, { data, loading, error }] = useGQLUpdateRequiresDecisionReasonOnIgnoreMutation({ + * variables: { + * enabled: // value for 'enabled' + * }, + * }); + */ +export function useGQLUpdateRequiresDecisionReasonOnIgnoreMutation( + baseOptions?: Apollo.MutationHookOptions< + GQLUpdateRequiresDecisionReasonOnIgnoreMutation, + GQLUpdateRequiresDecisionReasonOnIgnoreMutationVariables + >, +) { + const options = { ...defaultOptions, ...baseOptions }; + return Apollo.useMutation< + GQLUpdateRequiresDecisionReasonOnIgnoreMutation, + GQLUpdateRequiresDecisionReasonOnIgnoreMutationVariables + >(GQLUpdateRequiresDecisionReasonOnIgnoreDocument, options); +} +export type GQLUpdateRequiresDecisionReasonOnIgnoreMutationHookResult = + ReturnType; +export type GQLUpdateRequiresDecisionReasonOnIgnoreMutationResult = + Apollo.MutationResult; +export type GQLUpdateRequiresDecisionReasonOnIgnoreMutationOptions = + Apollo.BaseMutationOptions< + GQLUpdateRequiresDecisionReasonOnIgnoreMutation, + GQLUpdateRequiresDecisionReasonOnIgnoreMutationVariables + >; export const GQLUpdateHideSkipButtonForNonAdminsDocument = gql` mutation UpdateHideSkipButtonForNonAdmins($enabled: Boolean!) { updateHideSkipButtonForNonAdmins(enabled: $enabled) @@ -45189,6 +45257,8 @@ export const namedOperations = { UpdateSamlEnabled: 'UpdateSamlEnabled', UpdateRequiresPolicyForDecisions: 'UpdateRequiresPolicyForDecisions', UpdateRequiresDecisionReason: 'UpdateRequiresDecisionReason', + UpdateRequiresDecisionReasonOnIgnore: + 'UpdateRequiresDecisionReasonOnIgnore', UpdateHideSkipButtonForNonAdmins: 'UpdateHideSkipButtonForNonAdmins', UpdatePreviewJobsViewEnabled: 'UpdatePreviewJobsViewEnabled', UpdateIgnoreCallbackUrl: 'UpdateIgnoreCallbackUrl', diff --git a/client/src/webpages/dashboard/mrt/manual_review_job/ManualReviewJobReview.tsx b/client/src/webpages/dashboard/mrt/manual_review_job/ManualReviewJobReview.tsx index 06417a6..2a13188 100644 --- a/client/src/webpages/dashboard/mrt/manual_review_job/ManualReviewJobReview.tsx +++ b/client/src/webpages/dashboard/mrt/manual_review_job/ManualReviewJobReview.tsx @@ -147,6 +147,7 @@ gql` hasNCMECReportingEnabled requiresPolicyForDecisionsInMrt requiresDecisionReasonInMrt + requiresDecisionReasonOnIgnoreInMrt allowMultiplePoliciesPerAction hideSkipButtonForNonAdmins } @@ -680,11 +681,16 @@ function ManualReviewJobReviewImpl(props: { } // If the org requires a decision reason, and no decision reason has been - // provided, return false - if ( - data?.myOrg?.requiresDecisionReasonInMrt && - !isNonEmptyString(decisionReason) - ) { + // provided, return false. The requirement is split (see #757): ignoring a + // job (a decision made up solely of the built-in IGNORE action) is governed + // by its own setting, separate from acting on a violating job. + const isIgnoreDecision = selectedPrimaryActions.every( + (it) => 'type' in it.action && it.action.type === 'IGNORE', + ); + const reasonRequired = isIgnoreDecision + ? data?.myOrg?.requiresDecisionReasonOnIgnoreInMrt + : data?.myOrg?.requiresDecisionReasonInMrt; + if (reasonRequired && !isNonEmptyString(decisionReason)) { return false; } @@ -1669,14 +1675,13 @@ function ManualReviewJobReviewImpl(props: {
Policy
{policiesSection} - {org.requiresDecisionReasonInMrt ? ( -
-
- Reason -
- {decisionReasonSection} -
- ) : null} + {/* The reason field is always shown; whether it's required + depends on the org's require-decision-reason settings and the + decision type (see canBeSubmitted). */} +
+
Reason
+ {decisionReasonSection} +
({ // NB: We don't include any iconUrl or otherImageUrls here yet, since we're still diff --git a/client/src/webpages/settings/SettingsPage.test.tsx b/client/src/webpages/settings/SettingsPage.test.tsx index bfafe32..e21ac46 100644 --- a/client/src/webpages/settings/SettingsPage.test.tsx +++ b/client/src/webpages/settings/SettingsPage.test.tsx @@ -78,6 +78,7 @@ const deploymentSettingsMock: MockedResponse = { allowMultiplePoliciesPerAction: false, requiresPolicyForDecisionsInMrt: false, requiresDecisionReasonInMrt: false, + requiresDecisionReasonOnIgnoreInMrt: false, hideSkipButtonForNonAdmins: false, previewJobsViewEnabled: false, ignoreCallbackUrl: null, @@ -454,6 +455,7 @@ describe('SettingsPage', () => { const mock = makeDeploymentMock({ requiresPolicyForDecisionsInMrt: true, requiresDecisionReasonInMrt: true, + requiresDecisionReasonOnIgnoreInMrt: true, hideSkipButtonForNonAdmins: true, previewJobsViewEnabled: true, }); diff --git a/client/src/webpages/settings/SettingsPage.tsx b/client/src/webpages/settings/SettingsPage.tsx index 2f65987..4a2b956 100644 --- a/client/src/webpages/settings/SettingsPage.tsx +++ b/client/src/webpages/settings/SettingsPage.tsx @@ -28,6 +28,7 @@ gql` allowMultiplePoliciesPerAction requiresPolicyForDecisionsInMrt requiresDecisionReasonInMrt + requiresDecisionReasonOnIgnoreInMrt previewJobsViewEnabled hideSkipButtonForNonAdmins userStrikeTTL @@ -63,6 +64,9 @@ gql` mutation UpdateRequiresDecisionReason($enabled: Boolean!) { updateRequiresDecisionReason(enabled: $enabled) } + mutation UpdateRequiresDecisionReasonOnIgnore($enabled: Boolean!) { + updateRequiresDecisionReasonOnIgnore(enabled: $enabled) + } mutation UpdateHideSkipButtonForNonAdmins($enabled: Boolean!) { updateHideSkipButtonForNonAdmins(enabled: $enabled) } diff --git a/client/src/webpages/settings/tabs/ReviewConsoleTab.tsx b/client/src/webpages/settings/tabs/ReviewConsoleTab.tsx index cff0e9e..4e0aa16 100644 --- a/client/src/webpages/settings/tabs/ReviewConsoleTab.tsx +++ b/client/src/webpages/settings/tabs/ReviewConsoleTab.tsx @@ -9,6 +9,7 @@ import { useGQLUpdateIgnoreCallbackUrlMutation, useGQLUpdatePreviewJobsViewEnabledMutation, useGQLUpdateRequiresDecisionReasonMutation, + useGQLUpdateRequiresDecisionReasonOnIgnoreMutation, useGQLUpdateRequiresPolicyForDecisionsMutation, } from '@/graphql/generated'; import { isValidUrl } from '@/lib/utils'; @@ -26,6 +27,7 @@ export default function ReviewConsoleTab() { const [requirePolicy, setRequirePolicy] = useState(false); const [requireReason, setRequireReason] = useState(false); + const [requireReasonOnIgnore, setRequireReasonOnIgnore] = useState(false); const [hideSkip, setHideSkip] = useState(false); const [previewJobs, setPreviewJobs] = useState(false); const [ignoreCallbackUrl, setIgnoreCallbackUrl] = useState(''); @@ -34,6 +36,7 @@ export default function ReviewConsoleTab() { if (org) { setRequirePolicy(org.requiresPolicyForDecisionsInMrt); setRequireReason(org.requiresDecisionReasonInMrt); + setRequireReasonOnIgnore(org.requiresDecisionReasonOnIgnoreInMrt); setHideSkip(org.hideSkipButtonForNonAdmins); setPreviewJobs(org.previewJobsViewEnabled); setIgnoreCallbackUrl(org.ignoreCallbackUrl ?? ''); @@ -54,6 +57,10 @@ export default function ReviewConsoleTab() { useGQLUpdateRequiresPolicyForDecisionsMutation(mutationOpts); const [updateRequireReason, { loading: requireReasonLoading }] = useGQLUpdateRequiresDecisionReasonMutation(mutationOpts); + const [ + updateRequireReasonOnIgnore, + { loading: requireReasonOnIgnoreLoading }, + ] = useGQLUpdateRequiresDecisionReasonOnIgnoreMutation(mutationOpts); const [updateHideSkipMutation, { loading: hideSkipLoading }] = useGQLUpdateHideSkipButtonForNonAdminsMutation(mutationOpts); const [updatePreviewJobsMutation, { loading: previewJobsLoading }] = @@ -66,6 +73,7 @@ export default function ReviewConsoleTab() { const saveLoading = requirePolicyLoading || requireReasonLoading || + requireReasonOnIgnoreLoading || hideSkipLoading || previewJobsLoading || ignoreUrlLoading; @@ -73,6 +81,7 @@ export default function ReviewConsoleTab() { const hasChanges = requirePolicy !== org.requiresPolicyForDecisionsInMrt || requireReason !== org.requiresDecisionReasonInMrt || + requireReasonOnIgnore !== org.requiresDecisionReasonOnIgnoreInMrt || hideSkip !== org.hideSkipButtonForNonAdmins || previewJobs !== org.previewJobsViewEnabled || ignoreCallbackUrl !== (org.ignoreCallbackUrl ?? ''); @@ -84,6 +93,11 @@ export default function ReviewConsoleTab() { if (requireReason !== org.requiresDecisionReasonInMrt) { updateRequireReason({ variables: { enabled: requireReason } }); } + if (requireReasonOnIgnore !== org.requiresDecisionReasonOnIgnoreInMrt) { + updateRequireReasonOnIgnore({ + variables: { enabled: requireReasonOnIgnore }, + }); + } if (hideSkip !== org.hideSkipButtonForNonAdmins) { updateHideSkipMutation({ variables: { enabled: hideSkip } }); } @@ -97,6 +111,16 @@ export default function ReviewConsoleTab() { } }; + // The "Require Decision Reason" master toggle is derived from the two + // underlying flags: it's on when a reason is required for either action or + // ignore decisions. Turning it on defaults to requiring a reason for actions + // only; turning it off clears both. + const requireReasonAny = requireReason || requireReasonOnIgnore; + const handleRequireReasonToggle = (checked: boolean) => { + setRequireReason(checked); + setRequireReasonOnIgnore(false); + }; + return (
@@ -120,19 +144,43 @@ export default function ReviewConsoleTab() { onCheckedChange={setRequirePolicy} />
-
-
- - Require Decision Reason - - - Moderators must provide a written decision when completing a job - +
+
+
+ + Require Decision Reason + + + Moderators must provide a written reason when completing a job + +
+
- + {requireReasonAny ? ( +
+
+ + When applying an action + + +
+
+ + When ignoring jobs + + +
+
+ ) : null}
diff --git a/db/src/scripts/api-server-pg/2026.06.15T12.00.00.split_mrt_requires_decision_reason.sql b/db/src/scripts/api-server-pg/2026.06.15T12.00.00.split_mrt_requires_decision_reason.sql new file mode 100644 index 0000000..1e4d03a --- /dev/null +++ b/db/src/scripts/api-server-pg/2026.06.15T12.00.00.split_mrt_requires_decision_reason.sql @@ -0,0 +1,23 @@ +-- Splits the single "require decision reason" org setting into two independent +-- settings so that ignoring a job (which means "no violation / no action") can +-- be governed separately from acting on a violating job. See issue #757. +-- +-- The existing column is renamed to make its new, narrower meaning explicit +-- (it now applies only to non-ignore "violating" decisions), and a new column +-- governs ignores. We backfill the ignore column from the existing value so +-- upgrading Coop does not change behaviour for any org: an org that previously +-- required a reason for every decision keeps requiring one for ignores too. +ALTER TABLE manual_review_tool.manual_review_tool_settings + RENAME COLUMN mrt_requires_decision_reason TO mrt_requires_decision_reason_on_action; + +ALTER TABLE manual_review_tool.manual_review_tool_settings + ADD COLUMN IF NOT EXISTS mrt_requires_decision_reason_on_ignore boolean NOT NULL DEFAULT false; + +UPDATE manual_review_tool.manual_review_tool_settings + SET mrt_requires_decision_reason_on_ignore = mrt_requires_decision_reason_on_action; + +COMMENT ON COLUMN manual_review_tool.manual_review_tool_settings.mrt_requires_decision_reason_on_action IS + 'Require a written decision reason for non-ignore (violating) job decisions, e.g. custom actions and appeals.'; + +COMMENT ON COLUMN manual_review_tool.manual_review_tool_settings.mrt_requires_decision_reason_on_ignore IS + 'Require a written decision reason when ignoring a job (a decision composed solely of IGNORE).'; diff --git a/server/graphql/generated.ts b/server/graphql/generated.ts index 7b48083..6a24080 100644 --- a/server/graphql/generated.ts +++ b/server/graphql/generated.ts @@ -2595,6 +2595,7 @@ export type GQLMutation = { readonly updatePreviewJobsViewEnabled: Scalars['Boolean']['output']; readonly updateReportingRule: GQLUpdateReportingRuleResponse; readonly updateRequiresDecisionReason: Scalars['Boolean']['output']; + readonly updateRequiresDecisionReasonOnIgnore: Scalars['Boolean']['output']; readonly updateRequiresPolicyForDecisions: Scalars['Boolean']['output']; readonly updateRole?: Maybe; readonly updateRolePermissions: GQLRole; @@ -2938,6 +2939,10 @@ export type GQLMutationUpdateRequiresDecisionReasonArgs = { enabled: Scalars['Boolean']['input']; }; +export type GQLMutationUpdateRequiresDecisionReasonOnIgnoreArgs = { + enabled: Scalars['Boolean']['input']; +}; + export type GQLMutationUpdateRequiresPolicyForDecisionsArgs = { enabled: Scalars['Boolean']['input']; }; @@ -3296,6 +3301,7 @@ export type GQLOrg = { readonly publicSigningKey: Scalars['String']['output']; readonly reportingRules: ReadonlyArray; readonly requiresDecisionReasonInMrt: Scalars['Boolean']['output']; + readonly requiresDecisionReasonOnIgnoreInMrt: Scalars['Boolean']['output']; readonly requiresPolicyForDecisionsInMrt: Scalars['Boolean']['output']; readonly routingRules: ReadonlyArray; readonly rules: ReadonlyArray; @@ -11383,6 +11389,15 @@ export type GQLMutationResolvers< ContextType, RequireFields >; + updateRequiresDecisionReasonOnIgnore?: Resolver< + GQLResolversTypes['Boolean'], + ParentType, + ContextType, + RequireFields< + GQLMutationUpdateRequiresDecisionReasonOnIgnoreArgs, + 'enabled' + > + >; updateRequiresPolicyForDecisions?: Resolver< GQLResolversTypes['Boolean'], ParentType, @@ -11945,6 +11960,11 @@ export type GQLOrgResolvers< ParentType, ContextType >; + requiresDecisionReasonOnIgnoreInMrt?: Resolver< + GQLResolversTypes['Boolean'], + ParentType, + ContextType + >; requiresPolicyForDecisionsInMrt?: Resolver< GQLResolversTypes['Boolean'], ParentType, diff --git a/server/graphql/modules/org.ts b/server/graphql/modules/org.ts index a5c4b70..ba1b043 100644 --- a/server/graphql/modules/org.ts +++ b/server/graphql/modules/org.ts @@ -71,6 +71,7 @@ const typeDefs = /* GraphQL */ ` failedNcmecSubmissions: [NcmecFailedSubmission!]! requiresPolicyForDecisionsInMrt: Boolean! requiresDecisionReasonInMrt: Boolean! + requiresDecisionReasonOnIgnoreInMrt: Boolean! previewJobsViewEnabled: Boolean! allowMultiplePoliciesPerAction: Boolean! hideSkipButtonForNonAdmins: Boolean! @@ -172,6 +173,7 @@ const typeDefs = /* GraphQL */ ` updateSamlEnabled(enabled: Boolean!): Boolean! updateRequiresPolicyForDecisions(enabled: Boolean!): Boolean! updateRequiresDecisionReason(enabled: Boolean!): Boolean! + updateRequiresDecisionReasonOnIgnore(enabled: Boolean!): Boolean! updateHideSkipButtonForNonAdmins(enabled: Boolean!): Boolean! updatePreviewJobsViewEnabled(enabled: Boolean!): Boolean! updateIgnoreCallbackUrl(url: String): Boolean! @@ -599,6 +601,11 @@ const Org: GQLOrgResolvers = { org.id, ); }, + async requiresDecisionReasonOnIgnoreInMrt(org, _, context) { + return context.services.ManualReviewToolService.getRequiresDecisionReasonOnIgnore( + org.id, + ); + }, async previewJobsViewEnabled(org, _, context) { return context.services.ManualReviewToolService.getPreviewJobsViewEnabled( org.id, @@ -997,6 +1004,22 @@ const Mutation: GQLMutationResolvers = { ); return true; }, + async updateRequiresDecisionReasonOnIgnore(_, { enabled }, context) { + const user = context.getUser(); + if (!user) { + throw unauthenticatedError('User required.'); + } + if (!user.getPermissions().includes(UserPermission.MANAGE_ORG)) { + throw forbiddenError( + 'User does not have permission to update org settings', + ); + } + await context.services.ManualReviewToolService.updateRequiresDecisionReasonOnIgnore( + user.orgId, + enabled, + ); + return true; + }, async updateHideSkipButtonForNonAdmins(_, { enabled }, context) { const user = context.getUser(); if (!user) { diff --git a/server/services/manualReviewToolService/dbTypes.ts b/server/services/manualReviewToolService/dbTypes.ts index 1836343..7fb5778 100644 --- a/server/services/manualReviewToolService/dbTypes.ts +++ b/server/services/manualReviewToolService/dbTypes.ts @@ -203,7 +203,8 @@ export type ManualReviewToolServicePg = { 'manual_review_tool.manual_review_tool_settings': { org_id: string; requires_policy_for_decisions: boolean; - mrt_requires_decision_reason: boolean; + mrt_requires_decision_reason_on_action: boolean; + mrt_requires_decision_reason_on_ignore: boolean; hide_skip_button_for_non_admins: boolean; ignore_callback_url: string | null; preview_jobs_view_enabled: boolean; diff --git a/server/services/manualReviewToolService/manualReviewToolService.test.ts b/server/services/manualReviewToolService/manualReviewToolService.test.ts index 104bcea..8ab9164 100644 --- a/server/services/manualReviewToolService/manualReviewToolService.test.ts +++ b/server/services/manualReviewToolService/manualReviewToolService.test.ts @@ -270,10 +270,15 @@ describe('Manual Review Tool Service', () => { it.skip('should reject duplicate decisions on jobs dequeued again after the lock expires', async () => {}); }); - // Issue #616: when an org sets `mrt_requires_decision_reason`, submitDecision - // must reject decisions whose reason is empty. The UI already blocks this; - // the server-side check closes the API-bypass gap. Parallels the + // Issue #616: when an org sets `mrt_requires_decision_reason_on_action`, + // submitDecision must reject decisions whose reason is empty. The UI already + // blocks this; the server-side check closes the API-bypass gap. Parallels the // requires_policy_for_decisions enforcement from #533. + // + // Issue #757: the requirement is split into two flags — one for violating + // (non-ignore) decisions (`..._on_action`) and one for ignores + // (`..._on_ignore`) — so the cases below also cover that an IGNORE decision is + // gated by the ignore flag, not the action flag. describe('requires_decision_reason enforcement', () => { const orgId = 'e7c89ce7729'; const queueId = '1'; @@ -285,7 +290,16 @@ describe('Manual Review Tool Service', () => { await mrtService.upsertDefaultSettings({ orgId }); await mrtService['pgQuery'] .updateTable('manual_review_tool.manual_review_tool_settings') - .set({ mrt_requires_decision_reason: value }) + .set({ mrt_requires_decision_reason_on_action: value }) + .where('org_id', '=', orgId) + .execute(); + }; + + const setRequiresDecisionReasonOnIgnore = async (value: boolean) => { + await mrtService.upsertDefaultSettings({ orgId }); + await mrtService['pgQuery'] + .updateTable('manual_review_tool.manual_review_tool_settings') + .set({ mrt_requires_decision_reason_on_ignore: value }) .where('org_id', '=', orgId) .execute(); }; @@ -309,9 +323,10 @@ describe('Manual Review Tool Service', () => { }); afterEach(async () => { - // Reset so the flag doesn't leak into other tests in this file or + // Reset so the flags don't leak into other tests in this file or // subsequent runs that reuse the seeded org. await setRequiresDecisionReason(false); + await setRequiresDecisionReasonOnIgnore(false); }); it('rejects a decision with no reason when the flag is on', async () => { @@ -461,12 +476,149 @@ describe('Manual Review Tool Service', () => { }); }); + // Issue #757: an IGNORE decision is gated by the ignore flag, not the + // action flag. With only the ignore flag on, an IGNORE with no reason is + // rejected. + it('rejects an IGNORE decision with no reason when only the ignore flag is on', async () => { + await setRequiresDecisionReason(false); + await setRequiresDecisionReasonOnIgnore(true); + + const reviewerId = uuidv1(); + const reviewerEmail = 'test@test.com'; + const jobPayload = makeDummyJob(); + + await mrtService['queueOps']['addJob']({ + jobPayload, + orgId, + queueId, + enqueueSourceInfo: { kind: 'REPORT' }, + }); + + const dequeuedJob = await mrtService.dequeueNextJob({ + orgId, + queueId, + userId: reviewerId, + }); + + if (!dequeuedJob) { + throw new Error("should've returned a job"); + } + + await expect( + mrtService.submitDecision({ + queueId, + reportHistory: [], + jobId: dequeuedJob.job.id, + lockToken: dequeuedJob.lockToken, + decisionComponents: [{ type: 'IGNORE' }], + relatedActions: [], + reviewerId, + reviewerEmail, + orgId, + // decisionReason intentionally omitted + }), + ).rejects.toThrow(/requires every decision to include a reason/i); + }); + + // Issue #757: with only the action flag on, ignoring a job must NOT require + // a reason — this is the bug from the issue. + it('allows an IGNORE decision with no reason when only the action flag is on', async () => { + await setRequiresDecisionReason(true); + await setRequiresDecisionReasonOnIgnore(false); + + const reviewerId = uuidv1(); + const reviewerEmail = 'test@test.com'; + const jobPayload = makeDummyJob(); + + await mrtService['queueOps']['addJob']({ + jobPayload, + orgId, + queueId, + enqueueSourceInfo: { kind: 'REPORT' }, + }); + + const dequeuedJob = await mrtService.dequeueNextJob({ + orgId, + queueId, + userId: reviewerId, + }); + + if (!dequeuedJob) { + throw new Error("should've returned a job"); + } + + await mrtService.submitDecision({ + queueId, + reportHistory: [], + jobId: dequeuedJob.job.id, + lockToken: dequeuedJob.lockToken, + decisionComponents: [{ type: 'IGNORE' }], + relatedActions: [], + reviewerId, + reviewerEmail, + orgId, + // decisionReason intentionally omitted + }); + }); + + // Issue #757: with only the ignore flag on, acting on a violating job must + // NOT require a reason. + it('allows a CUSTOM_ACTION decision with no reason when only the ignore flag is on', async () => { + await setRequiresDecisionReason(false); + await setRequiresDecisionReasonOnIgnore(true); + + const reviewerId = uuidv1(); + const reviewerEmail = 'test@test.com'; + const jobPayload = makeDummyJob(); + const itemId = jobPayload.payload.item.itemId; + const itemTypeId = jobPayload.payload.item.itemTypeIdentifier.id; + + await mrtService['queueOps']['addJob']({ + jobPayload, + orgId, + queueId, + enqueueSourceInfo: { kind: 'REPORT' }, + }); + + const dequeuedJob = await mrtService.dequeueNextJob({ + orgId, + queueId, + userId: reviewerId, + }); + + if (!dequeuedJob) { + throw new Error("should've returned a job"); + } + + await mrtService.submitDecision({ + queueId, + reportHistory: [], + jobId: dequeuedJob.job.id, + lockToken: dequeuedJob.lockToken, + decisionComponents: [ + { + type: 'CUSTOM_ACTION', + actions: [{ id: seededActionId }], + policies: [{ id: uuidv1() }], + itemIds: [itemId], + itemTypeId, + }, + ], + relatedActions: [], + reviewerId, + reviewerEmail, + orgId, + // decisionReason intentionally omitted + }); + }); + // Issue #736: NCMEC review uses Submit NCMEC Report or Ignore, neither of // which carries a written decision reason. The require-reason flag is for // moderation decisions on standard MRT jobs and should not block the // NCMEC path. it('allows an IGNORE decision on an NCMEC job with no reason when the flag is on', async () => { await setRequiresDecisionReason(true); + await setRequiresDecisionReasonOnIgnore(true); const reviewerId = uuidv1(); const reviewerEmail = 'test@test.com'; diff --git a/server/services/manualReviewToolService/manualReviewToolService.ts b/server/services/manualReviewToolService/manualReviewToolService.ts index 925ce20..49e1431 100644 --- a/server/services/manualReviewToolService/manualReviewToolService.ts +++ b/server/services/manualReviewToolService/manualReviewToolService.ts @@ -1251,6 +1251,12 @@ export class ManualReviewToolService { return this.manualReviewToolSettings.getRequiresDecisionReason(orgId); } + async getRequiresDecisionReasonOnIgnore(orgId: string) { + return this.manualReviewToolSettings.getRequiresDecisionReasonOnIgnore( + orgId, + ); + } + async getPreviewJobsViewEnabled(orgId: string) { return this.manualReviewToolSettings.getPreviewJobsViewEnabled(orgId); } @@ -1273,6 +1279,13 @@ export class ManualReviewToolService { ); } + async updateRequiresDecisionReasonOnIgnore(orgId: string, enabled: boolean) { + return this.manualReviewToolSettings.updateRequiresDecisionReasonOnIgnore( + orgId, + enabled, + ); + } + async updateHideSkipButtonForNonAdmins(orgId: string, enabled: boolean) { return this.manualReviewToolSettings.updateHideSkipButtonForNonAdmins( orgId, diff --git a/server/services/manualReviewToolService/modules/JobDecisioning.ts b/server/services/manualReviewToolService/modules/JobDecisioning.ts index 2263898..1226570 100644 --- a/server/services/manualReviewToolService/modules/JobDecisioning.ts +++ b/server/services/manualReviewToolService/modules/JobDecisioning.ts @@ -224,29 +224,41 @@ export default class JobDecisioning { } } - // Enforce `mrt_requires_decision_reason` server-side. The MRT UI already - // disables submit when this is on, but API/script callers can bypass that. - // Skip the AUTOMATIC_CLOSE path (no moderator, no reason to require) and - // only read the flag when there's actually a missing reason to enforce - // against, so the common path does no extra DB work. Matches the client - // gate at ManualReviewJobReview.tsx, which uses isNonEmptyString. + // Enforce the "require decision reason" settings server-side. The MRT UI + // already disables submit when these are on, but API/script callers can + // bypass that. Skip the AUTOMATIC_CLOSE path (no moderator, no reason to + // require) and only read a flag when there's actually a missing reason to + // enforce against, so the common path does no extra DB work. Matches the + // client gate at ManualReviewJobReview.tsx, which uses isNonEmptyString. // - // Bypass the check when the decision is NCMEC-native (Submit NCMEC Report - // or Ignore on an NCMEC job, no CUSTOM_ACTION mixed in): those decisions - // don't carry a written reason and the flag is irrelevant for them. A - // CUSTOM_ACTION on an NCMEC job still requires a reason. See #736. + // The requirement is split in two (see #757): ignoring a job means "no + // violation / no action" and is governed by its own flag, separate from + // the flag for violating (non-ignore) decisions. A decision composed + // solely of IGNORE components is an ignore; anything else (custom actions, + // appeals, etc.) is a violating decision. + // + // Bypass the check entirely when the decision is NCMEC-native (Submit NCMEC + // Report or Ignore on an NCMEC job, no CUSTOM_ACTION mixed in): those + // decisions don't carry a written reason and the flags are irrelevant for + // them. A CUSTOM_ACTION on an NCMEC job still requires a reason. See #736. const isNcmecNativeDecision = job.payload.kind === 'NCMEC' && customActionDecisions.length === 0; + const isIgnoreDecision = + decisions.length > 0 && + decisions.every((decision) => decision.type === 'IGNORE'); if ( decisionComponents != null && !isNonEmptyString(decisionReason) && !isNcmecNativeDecision ) { - const requiresReason = - await this.manualReviewToolSettings.getRequiresDecisionReason(orgId); + const requiresReason = isIgnoreDecision + ? await this.manualReviewToolSettings.getRequiresDecisionReasonOnIgnore( + orgId, + ) + : await this.manualReviewToolSettings.getRequiresDecisionReason(orgId); if (requiresReason) { throw makeMissingRequiredDecisionReasonError({ - detail: 'This org requires every decision to include a reason.', + detail: 'This org requires a decision reason for this decision', shouldErrorSpan: true, }); } diff --git a/server/services/manualReviewToolService/modules/ManualReviewToolSettings.ts b/server/services/manualReviewToolService/modules/ManualReviewToolSettings.ts index eaf02e7..f0005b4 100644 --- a/server/services/manualReviewToolService/modules/ManualReviewToolSettings.ts +++ b/server/services/manualReviewToolService/modules/ManualReviewToolSettings.ts @@ -20,7 +20,8 @@ export default class ManualReviewToolSettings { .values({ org_id: orgId, requires_policy_for_decisions: false, - mrt_requires_decision_reason: false, + mrt_requires_decision_reason_on_action: false, + mrt_requires_decision_reason_on_ignore: false, hide_skip_button_for_non_admins: false, preview_jobs_view_enabled: false, ignore_callback_url: null, @@ -43,10 +44,19 @@ export default class ManualReviewToolSettings { async getRequiresDecisionReason(orgId: string) { const decisionReasonRow = await this.pgQuery .selectFrom('manual_review_tool.manual_review_tool_settings') - .select(['org_id', 'mrt_requires_decision_reason']) + .select(['org_id', 'mrt_requires_decision_reason_on_action']) .where('org_id', '=', orgId) .executeTakeFirst(); - return decisionReasonRow?.mrt_requires_decision_reason ?? false; + return decisionReasonRow?.mrt_requires_decision_reason_on_action ?? false; + } + + async getRequiresDecisionReasonOnIgnore(orgId: string) { + const decisionReasonRow = await this.pgQuery + .selectFrom('manual_review_tool.manual_review_tool_settings') + .select(['org_id', 'mrt_requires_decision_reason_on_ignore']) + .where('org_id', '=', orgId) + .executeTakeFirst(); + return decisionReasonRow?.mrt_requires_decision_reason_on_ignore ?? false; } async getHideSkipButtonForNonAdmins(orgId: string) { @@ -87,7 +97,8 @@ export default class ManualReviewToolSettings { .values({ org_id: orgId, requires_policy_for_decisions: false, - mrt_requires_decision_reason: false, + mrt_requires_decision_reason_on_action: false, + mrt_requires_decision_reason_on_ignore: false, hide_skip_button_for_non_admins: false, preview_jobs_view_enabled: false, ignore_callback_url: null, @@ -104,7 +115,15 @@ export default class ManualReviewToolSettings { } async updateRequiresDecisionReason(orgId: string, enabled: boolean) { - await this.upsertSettings(orgId, { mrt_requires_decision_reason: enabled }); + await this.upsertSettings(orgId, { + mrt_requires_decision_reason_on_action: enabled, + }); + } + + async updateRequiresDecisionReasonOnIgnore(orgId: string, enabled: boolean) { + await this.upsertSettings(orgId, { + mrt_requires_decision_reason_on_ignore: enabled, + }); } async updateHideSkipButtonForNonAdmins(orgId: string, enabled: boolean) {