✨ Track record/account delete and update data in subject status #2804

foysalit · 2024-09-09T17:45:44Z

This PR updates moderation subject status with 3 new properties recordUpdatedAt recordDeletedAt and recordStatus. as account/record events are received, these help moderators query subject statuses with delete/update time range and record status.

foysalit · 2024-09-09T19:29:10Z

packages/ozone/src/lexicon/lexicons.ts

+            type: 'string',
+            format: 'datetime',
+            description:
+              'Last update timestamp of the record the subject is associated with',


I feel like the language here is quite convoluted. would love suggestions to make it clearer.

Perhaps something along the lines of: "Timestamp at which the record or account was last updated."

foysalit · 2024-09-09T19:29:42Z

packages/ozone/src/lexicon/lexicons.ts

@@ -11127,7 +11145,7 @@ export const schemaDict = {
        type: 'object',
        description:
          'Logs account status related events on a repo subject. Normally captured by automod from the firehose and emitted to ozone for historical tracking.',
-        required: ['timestamp', 'status', 'active'],
+        required: ['timestamp', 'active'],


TODO: if this PR doesn't make it into the main branch, we'd wanna port this change over to the parent PR.

foysalit · 2024-09-09T19:38:41Z

packages/ozone/src/mod-service/status.ts

+      // When deactivated accounts are re-activated, we receive the event with just the active flag set to true
+      // so we want to make sure that the recordStatus is not set to an outdated value
+      if (currentStatus?.recordStatus !== 'active' && event.meta?.active) {
+        status.recordStatus = 'active'


default for recordStatus is null, may be we should fallback to that? although, doing this is maybe helpful if we want to see which accounts reactivated.

I'd recommend reflecting the exact hosting status here (where active is either null or 'active', slight preference for the latter), and then if we want to keep tract of reactivations we do so with a separate field such as reactivatedAt. That allows the status to not be dependent on past statuses, and if we want to know if something is active we don't need to check for multiple different values.

devinivy · 2024-09-20T21:14:01Z

packages/ozone/src/mod-service/status.ts

+
+  if (event.action === 'tools.ozone.moderation.defs#accountEvent') {
+    const status: Partial<ModerationSubjectStatusRow> = {
+      recordStatus: `${event.meta?.status}`,


Based on the types here, as written this could end-up containing the string 'undefined'.

devinivy · 2024-09-20T21:14:31Z

packages/ozone/src/mod-service/status.ts

+    }
+
+    if (event.meta?.tombstone) {
+      status.recordStatus = 'tombstoned'


I see a couple values for recordStatus in here that aren't reflected in the lexicon.

(P.S. is this separate from 'deleted'? Historically we have used the term "tombstoned" in a couple different ways, and I'm not sure which one is meant here.)

devinivy · 2024-09-20T21:20:14Z

lexicons/tools/ozone/moderation/defs.json

+        "recordUpdatedAt": {
+          "type": "string",
+          "format": "datetime",
+          "description": "Last update timestamp of the record the subject is associated with"
+        },
+        "recordDeletedAt": {
+          "type": "string",
+          "format": "datetime",
+          "description": "Timestamp referencing when the record the subject is associated with was deleted"
+        },
+        "recordStatus": {
+          "type": "string",
+          "description": "Status of the record the subject is associated with. Statuses are different when the subject references an account vs. a record",
+          "knownValues": ["takendown", "suspended", "deleted", "deactivated"]
+        },


These names work alright if the subject is a record, but if the subject is a repository or chat message, using the term "record" here doesn't seem like the best fit. Would there be any issue with using something like subjectUpdatedAt/subjectDeletedAt instead?

devinivy · 2024-09-20T22:07:36Z

lexicons/tools/ozone/moderation/defs.json

+        "recordStatus": {
+          "type": "string",
+          "description": "Status of the record the subject is associated with. Statuses are different when the subject references an account vs. a record",
+          "knownValues": ["takendown", "suspended", "deleted", "deactivated"]
+        },


This is quite a complex field, I think there are a few potential issues with it:

It's called "record status" but it may contain the hosting status of an account.

The hosting statuses of records and accounts are mixed together, which could make it seem like a record can be marked as takendown here, which isn't the case.

Potential confusion between the ozone status and the hosting status: the terms "takendown" and "suspended" are also terms used by ozone overlapping with the takendown and suspendedUntil fields.

Here's one idea: we could combine the three new fields into something more like this (using typescript just to illustrate the concept):

type SubjectStatusView = { // ... hosting?: AccountHosting | RecordHosting } type AccountHosting = { $type: 'tools.ozone.moderation.defs#accountHosting', status: 'active' | 'takendown' | 'suspended' | 'deleted' | 'deactivated' createdAt: Date updatedAt: Date deletedAt?: Date suspendedAt?: Date deactivatedAt?: Date takendownAt?: Date reactivatedAt?: Date } type RecordHosting = { $type: 'tools.ozone.moderation.defs#recordHosting' status: 'active' | 'deleted' createdAt: Date updatedAt: Date deletedAt?: Date }

This also gives us a way to make certain fields required together if we want, e.g. if hosting is present then hosting.createdAt can always be present too.

devinivy · 2024-09-20T22:09:10Z

lexicons/tools/ozone/moderation/queryStatuses.json

@@ -28,6 +28,31 @@
            "format": "datetime",
            "description": "Search subjects reviewed after a given timestamp"
          },
+          "recordDeletedAfter": {


My qualm with these fields is just that they are named "record" when they really may apply to any subject, e.g. an account rather than a record.

devinivy · 2024-09-20T22:10:12Z

packages/ozone/src/db/migrations/20240909T205730722Z-add-record-status-to-subject-status.ts

+  await db.schema
+    .alterTable('moderation_subject_status')
+    .addColumn('recordStatus', 'varchar')
+    .execute()
+  await db.schema
+    .alterTable('moderation_subject_status')
+    .addColumn('recordDeletedAt', 'varchar')
+    .execute()
+  await db.schema
+    .alterTable('moderation_subject_status')
+    .addColumn('recordUpdatedAt', 'varchar')
+    .execute()


I'd recommend distinguishing these as e.g. hostingStatus, hostingDeletedAt, etc.

devinivy · 2024-09-20T22:12:39Z

packages/ozone/src/lexicon/lexicons.ts

+            type: 'string',
+            format: 'datetime',
+            description:
+              'Last update timestamp of the record the subject is associated with',


Perhaps something along the lines of: "Timestamp at which the record or account was last updated."

devinivy · 2024-09-20T22:17:24Z

packages/ozone/src/mod-service/status.ts

+  const timestamp = event.meta?.timestamp
+    ? `${event.meta?.timestamp}`
+    : event.createdAt


The types here are a little funky to wrangle. To avoid ending up with a string like 'true' here, could we go with something more explicit like:

Suggested change

const timestamp = event.meta?.timestamp

? `${event.meta?.timestamp}`

: event.createdAt

const timestamp = typeof event.meta?.timestamp === 'string'

? event.meta.timestamp

: event.createdAt

devinivy · 2024-09-20T22:19:51Z

packages/ozone/src/mod-service/status.ts

+      // When deactivated accounts are re-activated, we receive the event with just the active flag set to true
+      // so we want to make sure that the recordStatus is not set to an outdated value
+      if (currentStatus?.recordStatus !== 'active' && event.meta?.active) {
+        status.recordStatus = 'active'


I'd recommend reflecting the exact hosting status here (where active is either null or 'active', slight preference for the latter), and then if we want to keep tract of reactivations we do so with a separate field such as reactivatedAt. That allows the status to not be dependent on past statuses, and if we want to know if something is active we don't need to check for multiple different values.

✨ Store deleted/updated event data in subject_status

445077f

foysalit commented Sep 9, 2024

View reviewed changes

🐛 Fix query for recordDeletedAt and recordUpdatedAt

d333440

foysalit commented Sep 9, 2024

View reviewed changes

✨ Add tombstoned status

5d14dc4

foysalit requested review from devinivy and bnewbold September 11, 2024 16:23

devinivy reviewed Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Track record/account delete and update data in subject status #2804

✨ Track record/account delete and update data in subject status #2804

foysalit commented Sep 9, 2024

foysalit Sep 9, 2024

devinivy Sep 20, 2024

foysalit Sep 9, 2024

foysalit Sep 9, 2024

devinivy Sep 20, 2024

devinivy Sep 20, 2024

devinivy Sep 20, 2024

devinivy Sep 20, 2024

devinivy Sep 20, 2024

devinivy Sep 20, 2024

devinivy Sep 20, 2024

devinivy Sep 20, 2024

devinivy Sep 20, 2024

devinivy Sep 20, 2024

devinivy Sep 20, 2024

✨ Track record/account delete and update data in subject status #2804

Are you sure you want to change the base?

✨ Track record/account delete and update data in subject status #2804

Conversation

foysalit commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment