Moderation proof of concept implementation #9461

marcospri · 2025-04-08T12:54:53Z

This PR:

Migrate AnnotationModeration

Creates a new moderation_status column on Annotation (and Annotation Slim)

Every existing annotation will start with a null value there.

Creates a new moderation_log table.
Updates the current hide/unhide feature to also take into account the new column.

We'll set it to DENIED when hiding and to APPROVED when "`unhidden".

We'll also create an audit record on moderation_log for it.

Migration to backfill "DENIED" based on the existing AnnotationModeration rows.
TODO Migration to backfill moderation_log based on AnnotationModeration.
TODO switch reads off AnnotationModeration to the new moderation_status.

Pre moderated group creation

TODO feature flag for pre-moderation
Migration for Group.pre_moderated
TODO expose "pre_moderated" on the groups API used in the grup edit form.
TODO consume "pre_moderated" on the group edit API.
TODO UI for pre_moderated toggle.

Moderation queue

New endpoint to list the annotations of a group
New endpoint to change the moderation status of an annotation.
Expose new endpoints to the edit UI config.
FE for moderation
TODO Link to annotation in context
TODO allow moderated (or pending) annotations to be visible in context
TODO link to user

marcospri · 2025-04-09T07:30:39Z

h/db/mixins.py



-class Timestamps:
+class CreatedMixin:


Splitting this in two, we not always need updated but if we have created we most likely want also updated.

In case where updated is seemingly unnecessary (e.g. the moderation_log) I'd be tempted to just add updated anyway. You never know!

marcospri · 2025-04-09T07:31:04Z

h/events.py

@@ -1,7 +1,12 @@
+from typing import Literal
+
+AnnotationAction = Literal["create", "update", "delete"]


I don't remember why I did this but I reckon I was looking for possible values

I didn't know about typing.Literal 👍

marcospri · 2025-04-09T07:32:30Z

h/migrations/versions/96cde96b2fd7_backfill_denied_based_on_moderation.py

+down_revision = "c8f748cbfb8f"
+
+
+def upgrade() -> None:


This migrates existing AnnotationModeration rows to the moderation_status column.

marcospri · 2025-04-09T07:33:11Z

h/migrations/versions/96cde96b2fd7_backfill_denied_based_on_moderation.py

+    """
+        )
+    )
+    print("\tUpdated annotations as DENIED:", result.rowcount)  # noqa: T201


Also backfill moderation_log based on AnnotationModeration.

This is not going to be possible as we don't have the user who hide them.

We could:

Make user nullable on moderation_log.

Just don't migrate these to moderation_log

Pick one user, the creator of the group? Could be confusing on the future.

Hmm. I hadn't really considered that we might want to back-fill moderation_log, but that might be good to have. Will we be able to set moderation_log.created correctly based on annotation_moderation.created?

I think we could go with either just not back-filling it or letting user_id be nullable.

marcospri · 2025-04-09T07:39:18Z

h/migrations/versions/c8f748cbfb8f_create_the_moderation_status_column.py

+            nullable=True,
+        ),
+    )
+    op.add_column(


AnnotationSlim has a boolean moderated column. I'm going to keep it like that but take into consideration the new Annotation.moderation_status to keep them in sync.

We should have a spike/discussion about AnnotationSlim. We did some work there but we haven't committed to move the codebase over that table.

My current thinking is that we should:

Move annotation_metadata to be related to annotation.id

Make a assessment of current slow queries, existing indexes in annotation and make the necessary adjustments.

Move existing queries of AnnotationSlim back to Annotation.

I don't think we should block the pre-moderation work over this but it something we could work in parallel or right afterwards.

marcospri · 2025-04-09T13:04:22Z

h/services/annotation_write.py

            )

-    def upsert_annotation_slim(self, annotation):
+    def upsert_annotation_slim(self, annotation: Annotation, user: User):


We are passing user to various methods above now, pass it here to and avoid this query.

marcospri · 2025-04-09T13:09:34Z

h/views/api/moderation.py

+    permission=Permission.Annotation.MODERATE,
+)
+def change_annotation_moderation_status(context, request):
+    status = Annotation.ModerationStatus(request.json_body["moderation_status"].upper())


This endpoint needs the mechanism to avoid updates if the annotation the client saw was updated.

marcospri · 2025-04-09T13:11:06Z

h/views/groups.py

+    @view_config(
+        route_name="group_edit_moderation",
+        request_method="GET",
+        permission=Permission.Group.EDIT,  # TODO  permission for moderation?


Exposing the new endpoints should be based on:

A new feature flag

Whether or not you are moderator.

marcospri · 2025-04-09T13:11:44Z

tests/unit/h/services/annotation_moderation_test.py

        assert svc.all_hidden([]) == set()

+    # fmt: off
+    @pytest.mark.parametrize("action,is_private,pre_moderation_enabled,existing_status,expected_status", [


These are the test that don't require shared changing.

We should test those here too here for coverage but I wanted to make sure all looked good so those are tested as func test here.

marcospri · 2025-04-09T13:12:36Z

tests/unit/h/services/annotation_moderation_test.py

 )


-class TestAnnotationModerationServiceAllHidden:


Refactored this to one test class per target class instead of one test class per method.

marcospri · 2025-04-09T15:56:37Z

h/presenters/annotation_searchindex.py

        # moderated and hidden.
-        parents_and_replies = [self.annotation.id] + self.annotation.thread_ids  # noqa: RUF005
+        parents_and_replies = [self.annotation.id, *self.annotation.thread_ids]



TODO This method below is not reading from the new status column yet.

This has the effect that for example single annotation threads are not hidden from the response (but their content is hidden).

marcospri · 2025-04-09T16:05:22Z

tests/functional/api/annotations_test.py

+            (True, ModerationStatus.SPAM, ModerationStatus.SPAM),
+        ],
+    )
+    def test_sharing_a_private_annotation(


I'm using func test for this cases that require a visibility change.

This would be the edit view, calling the anno write service and that calling the moderation service.

These same cases need to be unit tested but I wanted to see if everything was wired correctly.

marcospri · 2025-04-09T16:07:13Z

h/views/api/moderation.py

 def delete(context, request):
-    request.find_service(AnnotationWriteService).unhide(context.annotation)
+    request.find_service(AnnotationWriteService).unhide(
+        context.annotation, request.user


Adding user to these to keep the audit log up to date.

marcospri · 2025-04-09T16:11:24Z

h/views/api/moderation.py

+def _present_for_user(service, annotation, user):
+    annotation_json = service.present_for_user(annotation, user, hide_moderated=False)
+
+    annotation_json["moderation_status"] = (


This is duplicated on the group_annotations endpoint as well. We'll need a better way to deal with this.

marcospri · 2025-04-09T16:12:03Z

h/views/api/group_annotations.py

+def list_annotations(context: GroupContext, request):
+    group = context.group
+    params = validate_query_params(PaginationQueryParamsSchema(), request.params)
+    page_number = params["page[number]"]


I haven't tested any of the pagination.

marcospri · 2025-04-09T16:15:57Z

h/services/annotation_read.py


    @staticmethod
-    def _annotation_search_query(
+    def annotation_search_query(


Making this public and adding a bunch of parameters.

marcospri · 2025-04-09T16:17:31Z

h/services/annotation_moderation.py

        return {m.annotation_id for m in query}

+    def set_status(
+        self, annotation: Annotation, user: User, status: ModerationStatus | None


Here we set an status (almost) unconditionally.

We log it on the moderation_log table as well.

marcospri · 2025-04-09T16:18:03Z

h/services/annotation_moderation.py

+            )
+            annotation.moderation_status = status
+
+    def update_status(


Here we might change the status on creation/updates

marcospri · 2025-04-17T11:16:51Z

I reckon this PR has serve its function. Closing it.

seanh · 2025-04-10T08:57:28Z

h/db/mixins.py



-class Timestamps:
+class CreatedMixin:


In case where updated is seemingly unnecessary (e.g. the moderation_log) I'd be tempted to just add updated anyway. You never know!

seanh · 2025-04-10T08:59:21Z

h/events.py

@@ -1,7 +1,12 @@
+from typing import Literal
+
+AnnotationAction = Literal["create", "update", "delete"]


I didn't know about typing.Literal 👍

seanh · 2025-04-10T09:01:35Z

h/migrations/versions/96cde96b2fd7_backfill_denied_based_on_moderation.py

+        sa.text(
+            """
+    UPDATE annotation
+        SET moderation_status = 'DENIED'
+    FROM annotation_moderation
+    WHERE annotation.id = annotation_moderation.annotation_id
+    AND annotation.moderation_status  is null
+    """
+        )


Do you think there's few enough rows in annotation_moderation for this to work as a single query? Look like almost 9,000 rows, so I guess this should be fine.

seanh · 2025-04-10T09:04:01Z

h/migrations/versions/96cde96b2fd7_backfill_denied_based_on_moderation.py

+    """
+        )
+    )
+    print("\tUpdated annotations as DENIED:", result.rowcount)  # noqa: T201


Hmm. I hadn't really considered that we might want to back-fill moderation_log, but that might be good to have. Will we be able to set moderation_log.created correctly based on annotation_moderation.created?

I think we could go with either just not back-filling it or letting user_id be nullable.

seanh · 2025-04-10T13:00:09Z

h/migrations/versions/c8f748cbfb8f_create_the_moderation_status_column.py

+        "APPROVED",
+        "DENIED",
+        "SPAM",
+        "PENDING",


I don't think we want to put these actual strings in the DB in case we decide to change them in future. For example Rob was saying he thought "Rejected" would be better than "Denied". I'm wondering if the DB should just contain ints, but then the code can contain aliases/constants for the ints like APPROVED = 1
etc.

marcospri added 23 commits April 7, 2025 09:43

Create migration for new moderation column

3978d9e

Model changes for new annotation_moderation column

a3e3c16

Basic write change for the new moderation_status column

336d097

Migration to backfill status based on existing rows

d4cca1e

Migration for Group.pre_moderated

8c2245d

Model changes for Group.pre_moderated

0ce47b2

Basic status changes

c52d4d4

API

660d458

FRONT END

9305a30

Formatting

d157b9b

FRONTEND

d348dc6

Static

7a12fbb

MODel

75f4119

API

6a3eafe

F MACHINE

882a229

Status changes

f3c7e89

API

28e5f0b

Add some tests and fix all the code

8d76500

Add pre_moderation on the right model

5100cca

Index moderation status

fe9fbf2

Dont include PRIVATE on moderation status

09c6076

Sort annotations in new endpoint

f19a24a

Filter annotations by status

ee8ff56

marcospri changed the title ~~Moderation column~~ [PoC] Pre moderation Apr 8, 2025

marcospri added 6 commits April 8, 2025 15:25

Don't expose text and tags of moderated annos

fc1312f

Use service method for hide/unhide

f4fa8dd

Centralize moderation status changes

9fc40d7

Keep an log of moderation changes

75a47a3

Migration for moderation log

fc4f741

Store moderation status changes

922fded

marcospri added 2 commits April 9, 2025 09:19

Filter annos after a status change

b4ba3b9

Clean up

c106ca4

marcospri force-pushed the moderation-column branch from e1a888f to fe7c4a7 Compare April 9, 2025 12:55

marcospri changed the title ~~[PoC] Pre moderation~~ Moderation proof of concept implementation Apr 9, 2025

marcospri force-pushed the moderation-column branch from fe7c4a7 to f87c9b0 Compare April 9, 2025 13:39

marcospri commented Apr 9, 2025

View reviewed changes

Func test

a6671c5

marcospri force-pushed the moderation-column branch from f87c9b0 to a6671c5 Compare April 9, 2025 14:08

Add moderation link on header

2eb14c5

marcospri commented Apr 9, 2025

View reviewed changes

marcospri mentioned this pull request Apr 15, 2025

Migration to create Annotation.moderation_status #9475

Merged

marcospri closed this Apr 17, 2025

seanh reviewed Apr 22, 2025

View reviewed changes

seanh mentioned this pull request Apr 22, 2025

Add the Annotation.moderation_status column #9476

Merged

marcospri deleted the moderation-column branch May 14, 2025 07:38

		@@ -1,7 +1,12 @@
		from typing import Literal

		AnnotationAction = Literal["create", "update", "delete"]

Moderation proof of concept implementation #9461

Moderation proof of concept implementation #9461

Uh oh!

Conversation

marcospri commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Migrate AnnotationModeration

Pre moderated group creation

Moderation queue

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcospri Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcospri Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcospri commented Apr 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marcospri commented Apr 8, 2025 •

edited

Loading

marcospri Apr 9, 2025 •

edited

Loading

marcospri Apr 9, 2025 •

edited

Loading