Introduce #reviewOptional as reviewState for non-impactful events on …

…a subject (#2235) * 🚧 Working through an nullable review state * ✅ Update snapshots on some tests * ✅ Update snapshots on some tests * ✅ Add test for reviewOptional status mutation * ♻️ Rename reviewOptional -> reviewNone * ✅ Use FOR UPDATE to respect db transactions
bluesky-social · Mar 6, 2024 · bae2ce3 · bae2ce3
1 parent 06353c6
commit bae2ce3
Show file tree

Hide file tree

Showing 17 changed files with 189 additions and 29 deletions.
diff --git a/lexicons/com/atproto/admin/defs.json b/lexicons/com/atproto/admin/defs.json
@@ -449,7 +449,12 @@
     },
     "subjectReviewState": {
       "type": "string",
-      "knownValues": ["#reviewOpen", "#reviewEscalated", "#reviewClosed"]
+      "knownValues": [
+        "#reviewOpen",
+        "#reviewEscalated",
+        "#reviewClosed",
+        "#reviewNone"
+      ]
     },
     "reviewOpen": {
       "type": "token",
@@ -463,6 +468,10 @@
       "type": "token",
       "description": "Moderator review status of a subject: Closed. Indicates that the subject was already reviewed and resolved by a moderator"
     },
+    "reviewNone": {
+      "type": "token",
+      "description": "Moderator review status of a subject: Unnecessary. Indicates that the subject does not need a review at the moment but there is probably some moderation related metadata available for it"
+    },
     "modEventTakedown": {
       "type": "object",
       "description": "Take down a subject permanently or temporarily",

diff --git a/packages/api/src/client/index.ts b/packages/api/src/client/index.ts
@@ -319,6 +319,7 @@ export const COM_ATPROTO_ADMIN = {
   DefsReviewOpen: 'com.atproto.admin.defs#reviewOpen',
   DefsReviewEscalated: 'com.atproto.admin.defs#reviewEscalated',
   DefsReviewClosed: 'com.atproto.admin.defs#reviewClosed',
+  DefsReviewNone: 'com.atproto.admin.defs#reviewNone',
 }
 export const COM_ATPROTO_MODERATION = {
   DefsReasonSpam: 'com.atproto.moderation.defs#reasonSpam',

diff --git a/packages/api/src/client/lexicons.ts b/packages/api/src/client/lexicons.ts
@@ -747,6 +747,7 @@ export const schemaDict = {
           'lex:com.atproto.admin.defs#reviewOpen',
           'lex:com.atproto.admin.defs#reviewEscalated',
           'lex:com.atproto.admin.defs#reviewClosed',
+          'lex:com.atproto.admin.defs#reviewNone',
         ],
       },
       reviewOpen: {
@@ -764,6 +765,11 @@ export const schemaDict = {
         description:
           'Moderator review status of a subject: Closed. Indicates that the subject was already reviewed and resolved by a moderator',
       },
+      reviewNone: {
+        type: 'token',
+        description:
+          'Moderator review status of a subject: Unnecessary. Indicates that the subject does not need a review at the moment but there is probably some moderation related metadata available for it',
+      },
       modEventTakedown: {
         type: 'object',
         description: 'Take down a subject permanently or temporarily',

diff --git a/packages/api/src/client/types/com/atproto/admin/defs.ts b/packages/api/src/client/types/com/atproto/admin/defs.ts
@@ -497,6 +497,7 @@ export type SubjectReviewState =
   | 'lex:com.atproto.admin.defs#reviewOpen'
   | 'lex:com.atproto.admin.defs#reviewEscalated'
   | 'lex:com.atproto.admin.defs#reviewClosed'
+  | 'lex:com.atproto.admin.defs#reviewNone'
   | (string & {})
 
 /** Moderator review status of a subject: Open. Indicates that the subject needs to be reviewed by a moderator */
@@ -505,6 +506,8 @@ export const REVIEWOPEN = 'com.atproto.admin.defs#reviewOpen'
 export const REVIEWESCALATED = 'com.atproto.admin.defs#reviewEscalated'
 /** Moderator review status of a subject: Closed. Indicates that the subject was already reviewed and resolved by a moderator */
 export const REVIEWCLOSED = 'com.atproto.admin.defs#reviewClosed'
+/** Moderator review status of a subject: Unnecessary. Indicates that the subject does not need a review at the moment but there is probably some moderation related metadata available for it */
+export const REVIEWNONE = 'com.atproto.admin.defs#reviewNone'
 
 /** Take down a subject permanently or temporarily */
 export interface ModEventTakedown {

diff --git a/packages/bsky/src/lexicon/index.ts b/packages/bsky/src/lexicon/index.ts
@@ -142,6 +142,7 @@ export const COM_ATPROTO_ADMIN = {
   DefsReviewOpen: 'com.atproto.admin.defs#reviewOpen',
   DefsReviewEscalated: 'com.atproto.admin.defs#reviewEscalated',
   DefsReviewClosed: 'com.atproto.admin.defs#reviewClosed',
+  DefsReviewNone: 'com.atproto.admin.defs#reviewNone',
 }
 export const COM_ATPROTO_MODERATION = {
   DefsReasonSpam: 'com.atproto.moderation.defs#reasonSpam',

diff --git a/packages/bsky/src/lexicon/lexicons.ts b/packages/bsky/src/lexicon/lexicons.ts
@@ -747,6 +747,7 @@ export const schemaDict = {
           'lex:com.atproto.admin.defs#reviewOpen',
           'lex:com.atproto.admin.defs#reviewEscalated',
           'lex:com.atproto.admin.defs#reviewClosed',
+          'lex:com.atproto.admin.defs#reviewNone',
         ],
       },
       reviewOpen: {
@@ -764,6 +765,11 @@ export const schemaDict = {
         description:
           'Moderator review status of a subject: Closed. Indicates that the subject was already reviewed and resolved by a moderator',
       },
+      reviewNone: {
+        type: 'token',
+        description:
+          'Moderator review status of a subject: Unnecessary. Indicates that the subject does not need a review at the moment but there is probably some moderation related metadata available for it',
+      },
       modEventTakedown: {
         type: 'object',
         description: 'Take down a subject permanently or temporarily',

diff --git a/packages/bsky/src/lexicon/types/com/atproto/admin/defs.ts b/packages/bsky/src/lexicon/types/com/atproto/admin/defs.ts
@@ -497,6 +497,7 @@ export type SubjectReviewState =
   | 'lex:com.atproto.admin.defs#reviewOpen'
   | 'lex:com.atproto.admin.defs#reviewEscalated'
   | 'lex:com.atproto.admin.defs#reviewClosed'
+  | 'lex:com.atproto.admin.defs#reviewNone'
   | (string & {})
 
 /** Moderator review status of a subject: Open. Indicates that the subject needs to be reviewed by a moderator */
@@ -505,6 +506,8 @@ export const REVIEWOPEN = 'com.atproto.admin.defs#reviewOpen'
 export const REVIEWESCALATED = 'com.atproto.admin.defs#reviewEscalated'
 /** Moderator review status of a subject: Closed. Indicates that the subject was already reviewed and resolved by a moderator */
 export const REVIEWCLOSED = 'com.atproto.admin.defs#reviewClosed'
+/** Moderator review status of a subject: Unnecessary. Indicates that the subject does not need a review at the moment but there is probably some moderation related metadata available for it */
+export const REVIEWNONE = 'com.atproto.admin.defs#reviewNone'
 
 /** Take down a subject permanently or temporarily */
 export interface ModEventTakedown {

diff --git a/packages/ozone/src/db/schema/moderation_subject_status.ts b/packages/ozone/src/db/schema/moderation_subject_status.ts
@@ -3,6 +3,7 @@ import {
   REVIEWCLOSED,
   REVIEWOPEN,
   REVIEWESCALATED,
+  REVIEWNONE,
 } from '../../lexicon/types/com/atproto/admin/defs'
 
 export const subjectStatusTableName = 'moderation_subject_status'
@@ -13,7 +14,11 @@ export interface ModerationSubjectStatus {
   recordPath: string
   recordCid: string | null
   blobCids: string[] | null
-  reviewState: typeof REVIEWCLOSED | typeof REVIEWOPEN | typeof REVIEWESCALATED
+  reviewState:
+    | typeof REVIEWCLOSED
+    | typeof REVIEWOPEN
+    | typeof REVIEWESCALATED
+    | typeof REVIEWNONE
   createdAt: string
   updatedAt: string
   lastReviewedBy: string | null

diff --git a/packages/ozone/src/lexicon/index.ts b/packages/ozone/src/lexicon/index.ts
@@ -142,6 +142,7 @@ export const COM_ATPROTO_ADMIN = {
   DefsReviewOpen: 'com.atproto.admin.defs#reviewOpen',
   DefsReviewEscalated: 'com.atproto.admin.defs#reviewEscalated',
   DefsReviewClosed: 'com.atproto.admin.defs#reviewClosed',
+  DefsReviewNone: 'com.atproto.admin.defs#reviewNone',
 }
 export const COM_ATPROTO_MODERATION = {
   DefsReasonSpam: 'com.atproto.moderation.defs#reasonSpam',

diff --git a/packages/ozone/src/lexicon/lexicons.ts b/packages/ozone/src/lexicon/lexicons.ts
@@ -747,6 +747,7 @@ export const schemaDict = {
           'lex:com.atproto.admin.defs#reviewOpen',
           'lex:com.atproto.admin.defs#reviewEscalated',
           'lex:com.atproto.admin.defs#reviewClosed',
+          'lex:com.atproto.admin.defs#reviewNone',
         ],
       },
       reviewOpen: {
@@ -764,6 +765,11 @@ export const schemaDict = {
         description:
           'Moderator review status of a subject: Closed. Indicates that the subject was already reviewed and resolved by a moderator',
       },
+      reviewNone: {
+        type: 'token',
+        description:
+          'Moderator review status of a subject: Unnecessary. Indicates that the subject does not need a review at the moment but there is probably some moderation related metadata available for it',
+      },
       modEventTakedown: {
         type: 'object',
         description: 'Take down a subject permanently or temporarily',

diff --git a/packages/ozone/src/lexicon/types/com/atproto/admin/defs.ts b/packages/ozone/src/lexicon/types/com/atproto/admin/defs.ts
@@ -497,6 +497,7 @@ export type SubjectReviewState =
   | 'lex:com.atproto.admin.defs#reviewOpen'
   | 'lex:com.atproto.admin.defs#reviewEscalated'
   | 'lex:com.atproto.admin.defs#reviewClosed'
+  | 'lex:com.atproto.admin.defs#reviewNone'
   | (string & {})
 
 /** Moderator review status of a subject: Open. Indicates that the subject needs to be reviewed by a moderator */
@@ -505,6 +506,8 @@ export const REVIEWOPEN = 'com.atproto.admin.defs#reviewOpen'
 export const REVIEWESCALATED = 'com.atproto.admin.defs#reviewEscalated'
 /** Moderator review status of a subject: Closed. Indicates that the subject was already reviewed and resolved by a moderator */
 export const REVIEWCLOSED = 'com.atproto.admin.defs#reviewClosed'
+/** Moderator review status of a subject: Unnecessary. Indicates that the subject does not need a review at the moment but there is probably some moderation related metadata available for it */
+export const REVIEWNONE = 'com.atproto.admin.defs#reviewNone'
 
 /** Take down a subject permanently or temporarily */
 export interface ModEventTakedown {

diff --git a/packages/ozone/src/mod-service/status.ts b/packages/ozone/src/mod-service/status.ts
@@ -7,23 +7,30 @@ import {
   REVIEWOPEN,
   REVIEWCLOSED,
   REVIEWESCALATED,
+  REVIEWNONE,
 } from '../lexicon/types/com/atproto/admin/defs'
 import { ModerationEventRow, ModerationSubjectStatusRow } from './types'
 import { HOUR } from '@atproto/common'
 import { REASONAPPEAL } from '../lexicon/types/com/atproto/moderation/defs'
 import { jsonb } from '../db/types'
 
 const getSubjectStatusForModerationEvent = ({
+  currentStatus,
   action,
   createdBy,
   createdAt,
   durationInHours,
 }: {
+  currentStatus?: ModerationSubjectStatusRow
   action: string
   createdBy: string
   createdAt: string
   durationInHours: number | null
-}): Partial<ModerationSubjectStatusRow> | null => {
+}): Partial<ModerationSubjectStatusRow> => {
+  const defaultReviewState = currentStatus
+    ? currentStatus.reviewState
+    : REVIEWNONE
+
   switch (action) {
     case 'com.atproto.admin.defs#modEventAcknowledge':
       return {
@@ -54,7 +61,9 @@ const getSubjectStatusForModerationEvent = ({
       return {
         lastReviewedBy: createdBy,
         muteUntil: null,
-        reviewState: REVIEWOPEN,
+        // It's not likely to receive an unmute event that does not already have a status row
+        // but if it does happen, default to unnecessary
+        reviewState: defaultReviewState,
         lastReviewedAt: createdAt,
       }
     case 'com.atproto.admin.defs#modEventTakedown':
@@ -70,26 +79,29 @@ const getSubjectStatusForModerationEvent = ({
     case 'com.atproto.admin.defs#modEventMute':
       return {
         lastReviewedBy: createdBy,
-        reviewState: REVIEWOPEN,
         lastReviewedAt: createdAt,
         // By default, mute for 24hrs
         muteUntil: new Date(
           Date.now() + (durationInHours || 24) * HOUR,
         ).toISOString(),
+        // It's not likely to receive a mute event on a subject that does not already have a status row
+        // but if it does happen, default to unnecessary
+        reviewState: defaultReviewState,
       }
     case 'com.atproto.admin.defs#modEventComment':
       return {
         lastReviewedBy: createdBy,
         lastReviewedAt: createdAt,
+        reviewState: defaultReviewState,
       }
     case 'com.atproto.admin.defs#modEventTag':
-      return { tags: [] }
+      return { tags: [], reviewState: defaultReviewState }
     case 'com.atproto.admin.defs#modEventResolveAppeal':
       return {
         appealed: false,
       }
     default:
-      return null
+      return {}
   }
 }
 
@@ -114,23 +126,6 @@ export const adjustModerationSubjectStatus = async (
     createdAt,
   } = moderationEvent
 
-  const isAppealEvent =
-    action === 'com.atproto.admin.defs#modEventReport' &&
-    meta?.reportType === REASONAPPEAL
-
-  const subjectStatus = getSubjectStatusForModerationEvent({
-    action,
-    createdBy,
-    createdAt,
-    durationInHours: moderationEvent.durationInHours,
-  })
-
-  // If there are no subjectStatus that means there are no side-effect of the incoming event
-  if (!subjectStatus) {
-    return null
-  }
-
-  const now = new Date().toISOString()
   // If subjectUri exists, it's not a repoRef so pass along the uri to get identifier back
   const identifier = getStatusIdentifierFromSubject(subjectUri || subjectDid)
 
@@ -140,25 +135,46 @@ export const adjustModerationSubjectStatus = async (
     .selectFrom('moderation_subject_status')
     .where('did', '=', identifier.did)
     .where('recordPath', '=', identifier.recordPath)
+    // Make sure we respect other updates that may be happening at the same time
+    .forUpdate()
     .selectAll()
     .executeTakeFirst()
 
+  const isAppealEvent =
+    action === 'com.atproto.admin.defs#modEventReport' &&
+    meta?.reportType === REASONAPPEAL
+
+  const subjectStatus = getSubjectStatusForModerationEvent({
+    currentStatus,
+    action,
+    createdBy,
+    createdAt,
+    durationInHours: moderationEvent.durationInHours,
+  })
+
+  const now = new Date().toISOString()
   if (
     currentStatus?.reviewState === REVIEWESCALATED &&
-    subjectStatus.reviewState === REVIEWOPEN
+    subjectStatus.reviewState !== REVIEWCLOSED
   ) {
-    // If the current status is escalated and the incoming event is to open the review
-    // We want to keep the status as escalated
+    // If the current status is escalated only allow incoming events to move the state to
+    // reviewClosed because escalated subjects should never move to any other state
     subjectStatus.reviewState = REVIEWESCALATED
   }
 
+  if (currentStatus && subjectStatus.reviewState === REVIEWNONE) {
+    // reviewNone is ONLY allowed when there is no current status
+    // If there is a current status, it should not be allowed to move back to reviewNone
+    subjectStatus.reviewState = currentStatus.reviewState
+  }
+
   // Set these because we don't want to override them if they're already set
   const defaultData = {
     comment: null,
     // Defaulting reviewState to open for any event may not be the desired behavior.
     // For instance, if a subject never had any event and we just want to leave a comment to keep an eye on it
     // that shouldn't mean we want to review the subject
-    reviewState: REVIEWOPEN,
+    reviewState: REVIEWNONE,
     recordCid: subjectCid || null,
   }
   const newStatus = {

diff --git a/packages/ozone/tests/moderation-events.test.ts b/packages/ozone/tests/moderation-events.test.ts
@@ -209,7 +209,9 @@ describe('moderation-events', () => {
       expect(defaultEvents.length).toEqual(allEvents.data.events.length)
       expect(reversedEvents.length).toEqual(allEvents.data.events.length)
       // First event in the reversed list is the last item in the default list
-      expect(reversedEvents[0].id).toEqual(defaultEvents[5].id)
+      expect(reversedEvents[0].id).toEqual(
+        defaultEvents[defaultEvents.length - 1].id,
+      )
     })
 
     it('returns report events matching reportType filters', async () => {