Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce #reviewOptional as reviewState for non-impactful events on a subject #2235

Merged
merged 11 commits into from
Mar 6, 2024
Merged

Introduce #reviewOptional as reviewState for non-impactful events on a subject #2235

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8c62af0
:construction: Working through an nullable review state
foysalit Feb 25, 2024
c05f2ce
Merge branch 'main' of github.com:bluesky-social/atproto into nullabl…
foysalit Feb 27, 2024
ccc8ab9
:white_check_mark: Update snapshots on some tests
foysalit Feb 27, 2024
d7f6538
:white_check_mark: Update snapshots on some tests
foysalit Feb 27, 2024
93ac0e4
:white_check_mark: Add test for reviewOptional status mutation
foysalit Feb 27, 2024
1b92941
:twisted_rightwards_arrows: Merge with upstream
foysalit Feb 27, 2024
c5f2c10
:recycle: Rename reviewOptional -> reviewNone
foysalit Feb 29, 2024
9851565
Merge branch 'main' of github.com:bluesky-social/atproto into nullabl…
foysalit Feb 29, 2024
5c0e20a
:white_check_mark: Use FOR UPDATE to respect db transactions
foysalit Mar 4, 2024
f04c7b6
Merge branch 'main' of github.com:bluesky-social/atproto into nullabl…
foysalit Mar 6, 2024
f52cccd
Merge branch 'main' of github.com:bluesky-social/atproto into nullabl…
foysalit Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion lexicons/com/atproto/admin/defs.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,12 @@
},
"subjectReviewState": {
"type": "string",
"knownValues": ["#reviewOpen", "#reviewEscalated", "#reviewClosed"]
"knownValues": [
"#reviewOpen",
"#reviewEscalated",
"#reviewClosed",
"#reviewNone"
]
},
"reviewOpen": {
"type": "token",
Expand All @@ -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",
Expand Down
1 change: 1 addition & 0 deletions packages/api/src/client/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
6 changes: 6 additions & 0 deletions packages/api/src/client/lexicons.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand All @@ -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',
Expand Down
3 changes: 3 additions & 0 deletions packages/api/src/client/types/com/atproto/admin/defs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand All @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions packages/bsky/src/lexicon/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
6 changes: 6 additions & 0 deletions packages/bsky/src/lexicon/lexicons.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand All @@ -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',
Expand Down
3 changes: 3 additions & 0 deletions packages/bsky/src/lexicon/types/com/atproto/admin/defs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand All @@ -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 {
Expand Down
7 changes: 6 additions & 1 deletion packages/ozone/src/db/schema/moderation_subject_status.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
REVIEWCLOSED,
REVIEWOPEN,
REVIEWESCALATED,
REVIEWNONE,
} from '../../lexicon/types/com/atproto/admin/defs'

export const subjectStatusTableName = 'moderation_subject_status'
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions packages/ozone/src/lexicon/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
6 changes: 6 additions & 0 deletions packages/ozone/src/lexicon/lexicons.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand All @@ -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',
Expand Down
3 changes: 3 additions & 0 deletions packages/ozone/src/lexicon/types/com/atproto/admin/defs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand All @@ -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 {
Expand Down
66 changes: 40 additions & 26 deletions packages/ozone/src/mod-service/status.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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':
Expand All @@ -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 {}
}
}

Expand All @@ -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)

Expand All @@ -143,22 +138,41 @@ export const adjustModerationSubjectStatus = async (
.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 = {
Expand Down
2 changes: 1 addition & 1 deletion packages/ozone/tests/__snapshots__/moderation-events.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ Object {
"lastReportedAt": "1970-01-01T00:00:00.000Z",
"lastReviewedAt": "1970-01-01T00:00:00.000Z",
"lastReviewedBy": "user(1)",
"reviewState": "com.atproto.admin.defs#reviewEscalated",
"reviewState": "com.atproto.admin.defs#reviewOpen",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change stood out to me a little—just making sure we can account for the change in behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great catch! as discussed in slack, this was caused by a race condition and I missed the update in snapshot.
pushed a fix by adding forUpdate() to the current status selector query which should be safe since we shouldn't have that many concurrent events on the same subject.

"subject": Object {
"$type": "com.atproto.admin.defs#repoRef",
"did": "user(0)",
Expand Down
4 changes: 3 additions & 1 deletion packages/ozone/tests/moderation-events.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,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 () => {
Expand Down
Loading
Loading