Switch reads of AnnotationModeration to Annotation.moderation_status

After:

1/ Adding the new column
2/ Concurrently writing both the old table and the new column
3/ Backfilling the new column based on AnnotationModeration

we are ready ot switch the reads over the new column.

This commit doesn't introduce new functionality or any behavior changes.

Author: marcospri

h/models/__init__.py

@@ -20,7 +20,6 @@
 from h.models.activation import Activation
 from h.models.annotation import Annotation
 from h.models.annotation_metadata import AnnotationMetadata
-from h.models.annotation_moderation import AnnotationModeration
 from h.models.annotation_slim import AnnotationSlim
 from h.models.auth_client import AuthClient
 from h.models.auth_ticket import AuthTicket
@@ -33,6 +32,7 @@
 from h.models.group import Group, GroupMembership, GroupMembershipRoles
 from h.models.group_scope import GroupScope
 from h.models.job import Job
+from h.models.legacy_annotation_moderation import _LegacyAnnotationModeration
 from h.models.mention import Mention
 from h.models.moderation_log import ModerationLog
 from h.models.notification import Notification
@@ -49,7 +49,6 @@
 __all__ = (
     "Activation",
     "Annotation",
-    "AnnotationModeration",
     "AnnotationSlim",
     "AuthClient",
     "AuthTicket",

h/models/annotation.py

@@ -280,10 +280,12 @@ def authority(self):
         return split_user(self.userid)["domain"]
 
     @property
-    def is_hidden(self):
+    def is_hidden(self) -> bool:
         """Check if this annotation id is hidden."""
-
-        return self.moderation is not None
+        return bool(
+            self.moderation_status
+            and self.moderation_status != ModerationStatus.APPROVED
+        )
 
     def __repr__(self):
         return f"<Annotation {self.id}>"

h/models/annotation_moderation.py renamed to h/models/legacy_annotation_moderation.py

@@ -4,12 +4,15 @@
 from h.db.mixins import Timestamps
 
 
-class AnnotationModeration(Base, Timestamps):
+class _LegacyAnnotationModeration(Base, Timestamps):
     """
     A flag for a moderated and hidden annotation.
 
     This means that the annotation is violating the community guidelines and
     should be hidden from other users.
+
+    This model/table is deprecated and replaced by Annoation.moderation_status column
+    and the ModerationLog table.
     """
 
     __tablename__ = "annotation_moderation"
@@ -27,12 +30,12 @@ class AnnotationModeration(Base, Timestamps):
     annotation = sa.orm.relationship(
         "Annotation",
         backref=sa.orm.backref(
-            "moderation",
+            "_moderation",
             uselist=False,
             cascade="all, delete-orphan",
             passive_deletes=True,
         ),
     )
 
     def __repr__(self):
-        return f"<AnnotationModeration annotation_id={self.annotation_id}>"
+        return f"<_LegacyAnnotationModeration annotation_id={self.annotation_id}>"

h/search/index.py

@@ -149,7 +149,6 @@ def _eager_loaded_annotations(session):
             models.Document.document_uris
         ),
         subqueryload(models.Annotation.document).subqueryload(models.Document.meta),
-        subqueryload(models.Annotation.moderation),
         subqueryload(models.Annotation.thread).load_only(models.Annotation.id),
     )
 

h/services/annotation_json.py

@@ -168,8 +168,6 @@ def present_all_for_user(self, annotation_ids, user: User):
             eager_load=[
                 # Optimise access to the document
                 Annotation.document,
-                # Optimise the check used for "hidden" above
-                Annotation.moderation,
                 # Optimise the permissions check for MODERATE permissions,
                 # which ultimately depends on group permissions, causing a
                 # group lookup for every annotation without this

h/services/annotation_moderation.py

@@ -1,4 +1,4 @@
-from h.models import Annotation, AnnotationModeration, User
+from h.models import Annotation, User
 from h.models.annotation import ModerationStatus
 from h.models.moderation_log import ModerationLog
 
@@ -7,7 +7,7 @@ class AnnotationModerationService:
     def __init__(self, session):
         self._session = session
 
-    def all_hidden(self, annotation_ids):
+    def all_hidden(self, annotation_ids: str) -> set[str]:
         """
         Check which of the given annotation ids is hidden.
 
@@ -17,10 +17,10 @@ def all_hidden(self, annotation_ids):
         if not annotation_ids:
             return set()
 
-        query = self._session.query(AnnotationModeration.annotation_id).filter(
-            AnnotationModeration.annotation_id.in_(annotation_ids)
+        query = self._session.query(Annotation).filter(
+            Annotation.id.in_(annotation_ids)
         )
-        return {m.annotation_id for m in query}
+        return {a.id for a in query if a.is_hidden}
 
     def set_status(
         self, annotation: Annotation, user: User, status: ModerationStatus | None

h/services/annotation_write.py

@@ -1,12 +1,12 @@
 from collections.abc import Callable
 from datetime import datetime
 
-from sqlalchemy import exists, select
+from sqlalchemy import select
 from sqlalchemy.dialects.postgresql import insert
 from sqlalchemy.orm import Session
 
 from h import i18n
-from h.models import Annotation, AnnotationModeration, AnnotationSlim, User
+from h.models import Annotation, AnnotationSlim, User
 from h.models.document import update_document_metadata
 from h.schemas import ValidationError
 from h.security import Permission
@@ -168,7 +168,6 @@ def update_annotation(
     def hide(self, annotation: Annotation, user: User):
         """Hides  an annotation marking it it as "moderated"."""
         if not annotation.is_hidden:
-            annotation.moderation = AnnotationModeration()
             self._moderation_service.set_status(
                 annotation, user, Annotation.ModerationStatus.DENIED
             )
@@ -177,11 +176,10 @@ def hide(self, annotation: Annotation, user: User):
 
     def unhide(self, annotation: Annotation, user: User):
         """Remove the moderation status of an annotation."""
-        annotation.moderation = None
-        self.upsert_annotation_slim(annotation)
         self._moderation_service.set_status(
             annotation, user, Annotation.ModerationStatus.APPROVED
         )
+        self.upsert_annotation_slim(annotation)
 
     @staticmethod
     def change_document(db, old_document_ids, new_document):
@@ -235,7 +233,7 @@ def _validate_group(self, annotation: Annotation, enforce_write_permission=True)
                 + _("Annotations for this target URI are not allowed in this group")
             )
 
-    def upsert_annotation_slim(self, annotation):
+    def upsert_annotation_slim(self, annotation: Annotation) -> None:
         self._db.flush()  # See the last model changes in the transaction
 
         user_id = self._db.scalar(
@@ -247,15 +245,6 @@ def upsert_annotation_slim(self, annotation):
             # The AnnotationSlim records will get deleted by a cascade, no need to do anything here.
             return
 
-        moderated = self._db.scalar(
-            select(
-                exists(
-                    select(AnnotationModeration.id).where(
-                        AnnotationModeration.annotation_id == annotation.id
-                    )
-                )
-            )
-        )
         stmt = insert(AnnotationSlim).values(
             [
                 {
@@ -270,7 +259,7 @@ def upsert_annotation_slim(self, annotation):
                     # Fields of AnnotationSlim
                     "group_id": annotation.group.id,
                     "user_id": user_id,
-                    "moderated": moderated,
+                    "moderated": annotation.is_hidden,
                 }
             ]
         )

tests/common/factories/__init__.py

@@ -3,7 +3,6 @@
 from tests.common.factories.activation import Activation
 from tests.common.factories.annotation import Annotation
 from tests.common.factories.annotation_metadata import AnnotationMetadata
-from tests.common.factories.annotation_moderation import AnnotationModeration
 from tests.common.factories.annotation_slim import AnnotationSlim
 from tests.common.factories.auth_client import AuthClient, ConfidentialAuthClient
 from tests.common.factories.auth_ticket import AuthTicket

tests/common/factories/annotation_moderation.py

@@ -3,7 +3,7 @@
 import pytest
 
 from h.db.types import URLSafeUUID
-from h.models.annotation import Annotation
+from h.models.annotation import Annotation, ModerationStatus
 
 
 def test_parent_id_of_direct_reply():
@@ -234,13 +234,20 @@ def subreply(self, factories, root, reply):
         return factories.Annotation(references=[root.id, reply.id])
 
 
-@pytest.mark.parametrize("has_moderation", (True, False))
-def test_is_hidden(factories, has_moderation):
+@pytest.mark.parametrize(
+    "moderation_status,hidden",
+    [
+        (None, False),
+        (ModerationStatus.APPROVED, False),
+        (ModerationStatus.DENIED, True),
+    ],
+)
+def test_is_hidden(factories, moderation_status, hidden):
     annotation = factories.Annotation(
-        moderation=factories.AnnotationModeration() if has_moderation else None
+        moderation_status=moderation_status,
     )
 
-    assert annotation.is_hidden == has_moderation
+    assert annotation.is_hidden == hidden
 
 
 def test_uuid(factories):

tests/unit/h/models/annotation_moderation_test.py

@@ -0,0 +1,8 @@
+from h.models.legacy_annotation_moderation import _LegacyAnnotationModeration
+
+
+class TestAnnotationModeration:
+    def test___repr__(self):
+        moderation = _LegacyAnnotationModeration(annotation_id=123)
+
+        assert repr(moderation) == "<_LegacyAnnotationModeration annotation_id=123>"

tests/unit/h/models/annotation_test.py

@@ -204,7 +204,6 @@ def test_present_all_for_user(
             ids=sentinel.annotation_ids,
             eager_load=[
                 Annotation.document,
-                Annotation.moderation,
                 Annotation.group,
                 Annotation.mentions,
             ],
@@ -244,15 +243,14 @@ def user(self, factories):
     @pytest.fixture
     def annotation(self, factories):
         return factories.Annotation(
-            moderation=None,
             groupid="NOT WORLD",
             tags=["some-tags"],
             slim=factories.AnnotationSlim(),
         )
 
     @pytest.fixture
-    def with_hidden_annotation(self, annotation, factories):
-        annotation.moderation = factories.AnnotationModeration()
+    def with_hidden_annotation(self, annotation):
+        annotation.moderation_status = Annotation.ModerationStatus.DENIED
 
     @pytest.fixture(autouse=True)
     def Identity(self, patch):

tests/unit/h/models/legacy_annotation_moderation_test.py

@@ -8,8 +8,10 @@
 
 
 class TestAnnotationModerationService:
-    def test_all_hidden_lists_moderated_annotation_ids(self, svc, mods):
-        ids = [m.annotation.id for m in mods[0:-1]]
+    def test_all_hidden_lists_moderated_annotation_ids(
+        self, svc, moderated_annotations
+    ):
+        ids = [a.id for a in moderated_annotations[0:-1]]
         assert svc.all_hidden(ids) == set(ids)
 
     def test_all_hidden_skips_non_moderated_annotations(self, svc, factories):
@@ -68,8 +70,10 @@ def test_update_status_initializes_moderation_status(
             assert annotation.moderation_status == moderation_status
 
     @pytest.fixture(autouse=True)
-    def mods(self, factories):
-        return factories.AnnotationModeration.create_batch(3)
+    def moderated_annotations(self, factories):
+        return factories.Annotation.create_batch(
+            3, moderation_status=ModerationStatus.DENIED
+        )
 
     @pytest.fixture
     def annotation(self, factories):

tests/unit/h/services/annotation_json_test.py

@@ -29,7 +29,7 @@ def test_get_annotations_by_id(self, svc, factories, reverse):
     def test_get_annotations_by_id_with_no_input(self, svc):
         assert not svc.get_annotations_by_id(ids=[])
 
-    @pytest.mark.parametrize("attribute", ("document", "moderation", "group"))
+    @pytest.mark.parametrize("attribute", ("document", "group"))
     def test_get_annotations_by_id_preloading(
         self, svc, factories, db_session, query_counter, attribute
     ):

tests/unit/h/services/annotation_moderation_test.py

@@ -4,7 +4,7 @@
 import pytest
 from h_matchers import Any
 
-from h.models import Annotation, AnnotationModeration, AnnotationSlim, User
+from h.models import Annotation, AnnotationSlim, User
 from h.schemas import ValidationError
 from h.security import Permission
 from h.services.annotation_write import AnnotationWriteService, service_factory
@@ -100,6 +100,7 @@ def test_update_annotation(
         update_document_metadata,
         queue_service,
         _validate_group,  # noqa: PT019
+        moderation_service,
     ):
         then = datetime.now() - timedelta(days=1)  # noqa: DTZ005
         annotation.extra = {"key": "value"}
@@ -130,6 +131,7 @@ def test_update_annotation(
             {"uri": 1},
             updated=anno.updated,
         )
+        moderation_service.update_status.assert_called_once_with(anno)
 
         queue_service.add_by_id.assert_called_once_with(
             "sync_annotation",
@@ -219,35 +221,24 @@ def test__validate_group_with_url_not_in_scopes(
             svc._validate_group(annotation)  # noqa: SLF001
 
     def test_hide_hides_the_annotation(self, annotation, svc, user, moderation_service):
-        annotation.moderation = None
-
         svc.hide(annotation, user)
 
-        assert annotation.is_hidden
         moderation_service.set_status.assert_called_once_with(
             annotation, user, Annotation.ModerationStatus.DENIED
         )
 
     def test_hide_does_not_modify_an_already_hidden_annotation(
         self, annotation, svc, user, moderation_service
     ):
-        moderation = AnnotationModeration()
-        annotation.moderation = moderation
+        annotation.moderation_status = Annotation.ModerationStatus.DENIED
 
         svc.hide(annotation, user)
 
-        assert annotation.is_hidden
-        # It's the same one not a new one
-        assert annotation.moderation == moderation
         moderation_service.set_status.assert_not_called()
 
     def test_unhide(self, annotation, svc, user, moderation_service):
-        moderation = AnnotationModeration()
-        annotation.moderation = moderation
-
         svc.unhide(annotation, user)
 
-        assert not annotation.is_hidden
         moderation_service.set_status.assert_called_once_with(
             annotation, user, Annotation.ModerationStatus.APPROVED
         )