Skip to content

Switch reads of AnnotationModeration to Annotation.moderation_status #9481

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.

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

Merged
merged 1 commit into from
Apr 23, 2025

Conversation

marcospri
Copy link
Contributor

@marcospri marcospri commented Apr 15, 2025

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 to switch the reads over the new column.

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

Testing

  • Double check hidden annotations are still hidden
  • Sanity check hiding/unhiding annotations.

@@ -4,12 +4,15 @@
from h.db.mixins import Timestamps


class AnnotationModeration(Base, Timestamps):
class _LegacyAnnotationModeration(Base, Timestamps):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a while we can remove the model and table.

@@ -27,12 +30,12 @@ class AnnotationModeration(Base, Timestamps):
annotation = sa.orm.relationship(
"Annotation",
backref=sa.orm.backref(
"moderation",
"_moderation",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renaming the model and references helps identifying current usages and not introducing new ones.

@@ -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),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need for a bunch of these, we can now derive this directly from Annotation.

@@ -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()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stop writing to the old table.

@@ -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,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AnnotationSlim doesn't need more details that the existing boolean.

@marcospri marcospri requested a review from seanh April 15, 2025 14:11
@marcospri marcospri force-pushed the moderation-backfill branch from 31f7046 to 4f9688a Compare April 23, 2025 14:41
Base automatically changed from moderation-backfill to main April 23, 2025 14:48
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.
@marcospri marcospri merged commit 2ea8b4d into main Apr 23, 2025
11 checks passed
@marcospri marcospri deleted the moderation-reads branch April 23, 2025 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants