Backend/moderation: UMS coverage for thread comments and replies#8510
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @CouchersBot[bot]'s task in 2m 12s —— View job Migration Review CompletePosted full migration review as a separate comment (with the `` marker so future pushes update in place).
Overall risk: HIGH RISK. As the PR description already flags, the migration is the bare autogen scaffold and will fail against production data on three independent issues: |
Migration ReviewOne of our most common issues is migrations that don't work with existing data in production. This is very hard to catch with CI, so we have this bot to check for potential problems. Overall Risk: LOW RISK (down from HIGH — chain conflict resolved by rebase + renumber) Files Reviewed
FindingsThe chain is now linear and the migration body itself is well-formed:
Minor notes (non-blocking):
SummaryMigration is safe to merge. The previous chain conflict was the only blocking issue and is fully resolved by the rebase + renumber to |
![couchersbot[bot]](https://avatars.githubusercontent.com/in/983009?s=60&v=4){kind=small}
| def upgrade() -> None: | ||
| op.add_column("comments", sa.Column("moderation_state_id", sa.BigInteger(), nullable=False)) | ||
| op.create_index(op.f("ix_comments_moderation_state_id"), "comments", ["moderation_state_id"], unique=False) | ||
| op.create_foreign_key( | ||
| op.f("fk_comments_moderation_state_id_moderation_states"), | ||
| "comments", | ||
| "moderation_states", | ||
| ["moderation_state_id"], | ||
| ["id"], | ||
| ) | ||
| op.add_column("replies", sa.Column("moderation_state_id", sa.BigInteger(), nullable=False)) |
There was a problem hiding this comment.
HIGH RISK — this migration will fail in production. As the PR description already notes, this is the autogen scaffold and needs hand-written data migration work before merge. Three blocking issues:
nullable=Falseonadd_columnwith no default / no backfill — PostgreSQL will reject theALTER TABLE comments ADD COLUMN moderation_state_id BIGINT NOT NULLoutright on any non-emptycomments(andreplies) table, because there is no value to fill into existing rows. CI's fresh DB passes only because the tables are empty.- Missing enum values —
moderationobjecttypein production is('host_request', 'group_chat', 'friend_request', 'event_occurrence')(seeschema.sql:162). The model addscommentandreply, but this migration does not. Any insert intomoderation_stateswithobject_type='comment'or'reply'will fail. BecauseADD VALUEis not transactional and cannot be used in the same transaction as DML that references the new value, the rename-and-recreate pattern from0131_implement_ums_for_events.py:24-33should be used. - No backfill of
ModerationState/ModerationLog— every existing comment and reply needs aModerationStaterow (atvisible) and aModerationLogentry created, then the FK column populated, before the NOT NULL constraint can be applied.
Suggested shape (mirroring 0131_implement_ums_for_events.py):
- Rename + recreate
moderationobjecttypeenum to addcommentandreply. add_columnas nullable for both tables.- Insert one
moderation_statesrow per existingcomments/repliesrow atvisibility='visible'. - Insert matching
moderation_logrows (action='create',moderator_user_id = author_user_id,new_visibility='visible'). UPDATE comments / replies SET moderation_state_id = ms.id FROM moderation_states ms WHERE ms.object_type = ... AND ms.object_id = ....op.alter_column(..., nullable=False).- Then create the index + FK.
The downgrade() should also clean up the moderation_states / moderation_log rows for object_type IN ('comment', 'reply'), like 0131's downgrade does for event_occurrence.
|
Claude finished @aapeliv's task in 3m 54s —— View job Migration Review CompleteRe-reviewed
Overall risk: now LOW RISK (down from HIGH). All three previously blocking issues are resolved: enum is updated via the correct rename/recreate pattern; |
| return divmod(thread_id, 10) | ||
|
|
||
|
|
||
| _PUBLIC_VISIBILITIES = (ModerationVisibility.visible, ModerationVisibility.unlisted) |
| author=user_model_to_pb(author_user, session, context), | ||
| ), | ||
| moderation_state_id=occurrence.moderation_state_id, | ||
| moderation_state_id=comment.moderation_state_id, |
There was a problem hiding this comment.
need a mechanism for parent-> child dependency
There was a problem hiding this comment.
i think for now it's fine as for all objects, children are unreachable if parent is not visible
|
Claude finished @aapeliv's task in 2m 21s —— View job Migration Re-Review CompleteRe-reviewed
Overall risk: still LOW RISK. The rewrite is exactly what was requested in the review ( |
tristanlabelle
left a comment
tristanlabelle
left a comment
There was a problem hiding this comment.
Looks good to me, modulo your own todo comments.
| packed_thread_id uses the convention from couchers.servicers.threads.pack_thread_id: | ||
| the trailing digit is depth (1=Comment, 2=Reply); the rest is the database id. | ||
| """ | ||
| database_id, depth = divmod(packed_thread_id, 10) |
There was a problem hiding this comment.
Use unpack_thread_id to avoid duplicating the logic. You could also move the comment about how it works to that function's documentation.
| """ | ||
| comments = select(func.count()).select_from(Comment).where(Comment.thread_id == database_id) | ||
| replies = ( | ||
| def total_num_responses(session: Session, context: CouchersContext, database_id: int) -> int: |
There was a problem hiding this comment.
The caveat in your PR description could be useful in a comment here.
total_num_responses now counts only publicly visible content (visible + unlisted); authors of shadowed content will see one extra reply in the list than the count suggests — accepted to avoid threading a context through event_to_pb / discussion_to_pb / page_to_pb.
65242f4 to
476ce19
Compare
|
Claude finished @aapeliv's task in 3m 50s —— View job Claude finished migration re-review. Migration Re-Review CompleteUpdated the existing migration-review comment (with the `` marker) in place, and posted an inline comment on the revision identifiers.
Overall risk: HIGH RISK (re-raised from LOW). The migration body itself is still well-formed and safe on its own, but Fix: rebase onto |
| revision = "0149" | ||
| down_revision = "0148" |
There was a problem hiding this comment.
develop now has 0149_backend_feature_implement_shadowed_at.py (revision="0149", down_revision="0148") and 0150_add_last_updated_field_to_photogallery.py (revision="0150", down_revision="0149"). After merging this PR Alembic would see two heads both claiming revision="0149" from 0148 — branched, non-linear history, which CLAUDE.md forbids.
Rebase onto develop and renumber this migration to 0151_… with revision="0151" and down_revision="0150". The upgrade/downgrade body shouldn't need changes — the two new migrations on develop touch users / lite_users and photo_galleries, which don't interact with comments / replies / moderation_states.
| revision = "0149" | |
| down_revision = "0148" | |
| revision = "0151" | |
| down_revision = "0150" |
|
Claude finished @aapeliv's task in 1m 54s —— View job Migration Re-Review CompleteUpdated the existing migration-review comment (with the `` marker) in place.
Overall risk: now LOW RISK (down from HIGH). The rebase + renumber fully resolves the chain conflict flagged in the previous review. The migration body itself was already approved: well-formed enum rename/recreate, nullable-add → backfill → NOT NULL ordering, proper • Branch |
New comments and replies start shadowed and require moderator approval; notifications about them are deferred via the new content's own moderation_state_id. total_num_responses now counts only publicly visible content. Adds Moderator.approve_comment / approve_reply test helpers and updates affected tests. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The autogen migration only added the moderation_state_id FK columns. This extends it with the enum rename/recreate trick (Postgres can't ADD VALUE in the same transaction as DML using it) and a per-row backfill that creates a 'visible' ModerationState + 'create' log entry for every existing comment and reply, so the new NOT NULL FK columns can be populated and historical content stays visible. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…n 0149
- threads.total_num_responses/thread_to_pb now use where_moderated_content_visible
and take a CouchersContext; callers in discussions/events/pages updated.
- Migration 0149 backfill switched from nextval('moderation_seq') to the
(MAX(id) WHERE id < 2000000) + ROW_NUMBER() pattern from 0131.
- thread_to_pb / total_num_responses now take viewing_user_id instead of the full CouchersContext; callers pass context.user_id. - approve_thread_post uses unpack_thread_id rather than duplicating the divmod-by-10 convention; moved the depth convention to pack_thread_id's docstring.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
bc4cc78 to
4180a6f
Compare
|
Claude finished @aapeliv's task in 1m 23s —— View job Migration ReviewOne of our most common issues is migrations that don't work with existing data in production. This is very hard to catch with CI, so we have this bot to check for potential problems. Overall Risk: LOW RISK Files Reviewed
FindingsThe migration body is well-formed and the chain is linear:
Minor notes (non-blocking):
SummaryMigration is safe to merge. The previously blocking chain conflict is fully resolved by the rebase + renumber to |
github-actions
Bot
added
the
release notes: pending
📝 Release NotesThis PR should be included in the release notes. Suggested release note: 🤖 Bot Debug InformationModel: |
2 participants
Adds UMS (Unified Moderation System) integration for thread
CommentandReplycontent — the first of five mod-team-prioritised content types being onboarded onto UMS.New comments and replies are created with
ModerationVisibility.shadowedand aninitial_reviewqueue item, matching the existing UMS pattern (host requests, event occurrences, etc.). Authors can see their own shadowed content; everyone else sees nothing until a moderator approves. Notifications generated by a new comment or reply are now gated on the new content's ownmoderation_state_id, so they fire only after that content is approved (rather than firing immediately and only being deferred by the parent event's state, as previously).total_num_responsesnow counts only publicly visible content (visible+unlisted); authors of shadowed content will see one extra reply in the list than the count suggests — accepted to avoid threading a context throughevent_to_pb/discussion_to_pb/page_to_pb.Adds
Moderator.approve_comment,approve_reply, and a packed-id-awareapprove_thread_posthelper so existing tests can keep asserting visibility assumptions after the default-shadowed change.moderation_state_idFK columns oncommentsandreplies, but those columns are NOT NULL and the new enum values can't be added in the same transaction as DML. A hand-written data migration is needed before merging — closely modelled on0131_implement_ums_for_events.py(enum rename/recreate + per-rowModerationStatebackfill atvisible+ log entries).Testing
make format— cleanmake mypy— clean for changed files (one preexisting error intests/fixtures/misc.py:31unrelated to this PR)uv run pytest— 938 tests pass; the only failure istest_db.py::test_migrations, which is the expected schema-vs-migration diff that the autogen migration will resolveNew tests in
test_threads.pycover:total_num_responsesexcludes shadowed contentExisting affected tests updated:
test_event_threads,test_event_comment_notification_has_moderation_state,test_event_thread_reply_notification_has_moderation_state(test_events.py)test_discussion_notifications_regression(test_discussions.py)Backend checklist
developif necessary for linear migration history — seeFor maintainers
This PR was created with the Couchers PR skill.