Skip to content

Add edit and delete discussions and comments#8566

Merged
aapeliv merged 9 commits into
developfrom
na/backend/edit-delete-discussions
Jun 8, 2026
Merged

Add edit and delete discussions and comments#8566
aapeliv merged 9 commits into
developfrom
na/backend/edit-delete-discussions

Conversation

@nabramow

@nabramow nabramow commented May 13, 2026

Copy link
Copy Markdown
Member

Adds the ability to update and edit discussions, comments and replies. A few decisions were made here:

  • Store edits in three versions tables, requested by @aapeliv for moderation purposes
  • When a discussion/comment is deleted, the replies and comments within it will stay, and only that card will be replaced with one that says "This discussion/comment was deleted".
  • For the versions tables I am storing the new and previous content in each entry rather than just the previous. My thinking is that this is better for whatever moderation queries we'll need to make.

Since this PR contains backend code it can't be tested on Vercel.

Closes #8565

Testing

Explain how you tested this PR and give clear steps so the reviewer can replicate.

  • Go to a community and add a discussion
  • Add a comment to it
  • Add a reply to that comment
  • Delete the comment, the reply to that comment should still be there and comment replaced with "This comment has been deleted"
  • Add another comment
  • Delete the main discussion
  • It should be replaced with "This discussion has been deleted" but still show the replies
  • Now delete all the comments and replies
  • Go back to the main discussions page and the thread should be gone

Backend checklist

  • Added tests for any new code or added a regression test if fixing a bug
  • Run the backend locally and it works
  • Added migrations if there are any database changes, rebased onto develop if necessary for linear migration history

Web frontend checklist

  • There are no console warnings when running the app
  • Added tests where relevant
  • Clicked around my changes running locally and it works
  • Checked Desktop, Mobile and Tablet screen sizes

For maintainers

  • Maintainers can push commits to my branch
  • Maintainers can merge this PR for me

@vercel

vercel Bot commented May 13, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
couchers Ready Ready Preview Jun 8, 2026 4:06am

Request Review

@couchersbot

couchersbot Bot commented May 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @nabramow's task in 57s —— View job


I'll analyze this and get back to you.

@couchersbot

couchersbot Bot commented May 13, 2026

Copy link
Copy Markdown
Contributor

Migration Review

One 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: SAFE

Files Reviewed

  • 0158_add_edit_delete_to_discussions_and_comments.py — Adds nullable deleted + last_edited to discussions and last_edited to comments/replies; creates a new contentchangetype enum and three audit tables (discussion_versions, comment_versions, reply_versions) with FKs and indexes.

Findings

No issues found.

  • Additive columns: discussions.deleted, discussions.last_edited, comments.last_edited, and replies.last_edited are all nullable=True with no server default. Verified against schema.sql: discussions has neither column today, and comments/replies already have deleted, so only last_edited is added there. Adding a nullable column without a default is a metadata-only operation in PostgreSQL 11+, so no backfill is needed and lock contention is negligible.
  • New tables: discussion_versions, comment_versions, and reply_versions do not exist in schema.sql today. Their NOT NULL columns and foreign keys (to discussions/comments/replies/users) carry no risk against existing data because the tables start empty — no orphan-record or constraint-violation exposure. Their indexes are created on empty tables, so CONCURRENTLY is not needed.
  • New enum: contentchangetype is brand new. The type is created once via the first create_table (default create_type=True) and referenced with create_type=False in the other two tables — the correct idiomatic Alembic pattern that avoids a "type already exists" error. CREATE TYPE is transactional, so a failed upgrade rolls back cleanly.
  • Downgrade: Reverses the upgrade correctly — drops the three version tables (which use the enum) before dropping the enum with checkfirst=True, then drops the four added columns. Data in the new columns/tables is lost on downgrade, which is expected for newly added objects.
  • Revision chain: 0158 -> 0157 is correct (0157_log_feature_flag_usage_per_user is the current latest migration on the branch). Linear, no branches.

Summary

Purely additive schema change — new nullable columns plus three brand-new audit tables and a new enum, all with no impact on existing rows. No backfill, constraint violations, or unsafe casts. Safe to run against production data.

@tristanlabelle tristanlabelle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can sign off on the backend changes if you split this PR. It's very large to review the frontend part as well. Left noncritical comments.

Comment thread app/backend/src/couchers/servicers/admin.py
Comment thread app/backend/src/couchers/servicers/communities.py Outdated
Comment thread app/backend/src/couchers/servicers/discussions.py
Comment thread app/backend/src/couchers/servicers/discussions.py
Comment thread app/backend/src/couchers/servicers/threads.py Outdated
Comment thread app/backend/src/couchers/servicers/threads.py Outdated
Comment thread app/backend/src/couchers/servicers/threads.py
Comment thread app/backend/src/tests/test_discussions.py

@aapeliv aapeliv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let's also wait for ums/mod stuff first

@couchersbot

couchersbot Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

Claude finished @nabramow's task in 45s —— View job


Migration Review Complete

Reviewed 0151_add_edit_delete_to_discussions_and_comments.py against the production schema.

Overall Risk: SAFE — purely additive, all-nullable columns, no constraints or backfill needed. Full details posted in the migration review comment.

@couchersbot

couchersbot Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

Claude finished @nabramow's task in 57s —— View job


Migration Review Complete

Reviewed 0155_add_edit_delete_to_discussions_and_comments.py against the production schema.

Overall Risk: SAFE — purely additive, all-nullable columns, no constraints or backfill needed. Verified comments/replies already have deleted (so only last_edited is added there) and that the 0155 -> 0154 revision chain is correct. Full details in the migration review comment.

@couchersbot

couchersbot Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @nabramow's task in 1m 57s —— View job


Migration Review

One 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: SAFE

Files Reviewed

  • 0158_add_edit_delete_to_discussions_and_comments.py — Adds nullable deleted + last_edited to discussions, nullable last_edited to comments and replies, and creates three new audit tables (discussion_versions, comment_versions, reply_versions) plus a new contentchangetype enum.

Findings

No issues found.

Verification notes for this push (the migration grew since the previous review — it now also creates the version/audit tables):

  • Additive columns: All four added columns (discussions.deleted, discussions.last_edited, comments.last_edited, replies.last_edited) are nullable=True with no server default → no backfill needed, existing rows unaffected. Confirmed against schema.sql: comments/replies already have deleted, so only last_edited is added there; discussions has neither today, so both are added. Adding nullable columns is a metadata-only op in PostgreSQL 11+, so negligible lock contention.
  • New tables: discussion_versions, comment_versions, reply_versions and their indexes/FKs are all net-new (confirmed absent from schema.sql). FKs to discussions/comments/replies/users carry no orphan risk since the tables start empty, and index creation on empty tables is instant.
  • Enum: contentchangetype does not exist in production yet, so this is a CREATE (transactional/safe), not an ADD VALUE. The pattern is correct — the first create_table creates the type implicitly (create_type=True default) and the other two use create_type=False, avoiding a "type already exists" failure.
  • Downgrade: Reverses the upgrade in the right order — drops the three tables, then drops the enum (checkfirst=True), then drops the four columns. Data in the new objects is lost on downgrade, which is expected.
  • Revision chain: 0158 → 0157 is correct (0157 is the current head on develop). The file was renumbered from 0155 during rebase — fine, since it has not been applied to production.

Summary

Purely additive change: nullable columns on existing tables plus net-new audit tables and enum. No existing-data risk. Safe to run against production.

@couchersbot

couchersbot Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @nabramow's task in 4m 36s —— View job


Migration Review Complete

Reviewed 0158_add_edit_delete_to_discussions_and_comments.py against the production schema (schema.sql).

Overall Risk: SAFE — purely additive. It adds nullable columns (discussions.deleted/last_edited, comments.last_edited, replies.last_edited) plus three brand-new audit tables (discussion_versions, comment_versions, reply_versions) and a new contentchangetype enum. Verified against schema.sql: discussions has neither new column today, comments/replies already have deleted (so only last_edited is added), and none of the new tables/enum exist yet. New tables start empty, so their NOT NULL columns, FKs, and indexes carry no risk against existing data. The enum uses the correct create-once / create_type=False pattern, the downgrade reverses cleanly, and the 0158 -> 0157 revision chain is correct and linear.

No backfill, constraint violations, or unsafe casts against existing data. Full details in the migration review comment.

@aapeliv
aapeliv force-pushed the na/backend/edit-delete-discussions branch from d7e817b to 062d459 Compare June 8, 2026 04:01
@couchersbot

couchersbot Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @aapeliv's task in 44s —— View job


Migration Review

One 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: SAFE

Files Reviewed

  • 0165_add_edit_delete_to_discussions_and_comments.py — Adds nullable deleted + last_edited to discussions, nullable last_edited to comments and replies, and creates three new audit tables (discussion_versions, comment_versions, reply_versions) plus a new contentchangetype enum.

Findings

No issues found.

  • Additive columns: All four added columns (discussions.deleted, discussions.last_edited, comments.last_edited, replies.last_edited) are nullable=True with no server default → no backfill needed, existing rows unaffected. Verified against schema.sql: discussions has neither column today (both added), and comments/replies already have deleted (so only last_edited is added). Adding nullable columns is a metadata-only op in PostgreSQL 11+, so negligible lock contention.
  • New tables: discussion_versions, comment_versions, reply_versions and their FKs (to discussions/comments/replies/users) and indexes are all net-new (confirmed absent from schema.sql). NOT NULL columns and FKs carry no orphan/constraint risk since the tables start empty; index creation on empty tables is instant, so CONCURRENTLY is not needed.
  • Enum: contentchangetype does not exist in production yet, so this is a transactional CREATE TYPE (safe), not an ADD VALUE. The pattern is correct — the first create_table creates the type implicitly (create_type=True default) and the other two use create_type=False, avoiding a "type already exists" failure.
  • Downgrade: Reverses the upgrade in the right order — drops the three tables, then drops the enum (checkfirst=True), then drops the four columns. Data in the new objects is lost on downgrade, which is expected for newly added objects.
  • Revision chain: 0165 → 0164 is correct and linear (0164 is the current head on develop).

Summary

Purely additive change: nullable columns on existing tables plus net-new audit tables and a new enum. No existing-data risk — no backfill, constraint violations, or unsafe casts. Safe to run against production.

@aapeliv aapeliv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let's also wait for ums/mod stuff first

moderation_state_id: Mapped[int] = mapped_column(ForeignKey("moderation_states.id"), index=True)
created: Mapped[datetime] = mapped_column(DateTime(timezone=True), server_default=func.now(), init=False)
deleted: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), default=None)
last_edited: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), default=None)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

non-nullable

content: Mapped[str] = mapped_column(String) # CommonMark without images
created: Mapped[datetime] = mapped_column(DateTime(timezone=True), server_default=func.now(), init=False)
deleted: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), default=None)
last_edited: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), default=None)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ditto. server default func.now

content: Mapped[str] = mapped_column(String) # CommonMark without images
created: Mapped[datetime] = mapped_column(DateTime(timezone=True), server_default=func.now(), init=False)
deleted: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), default=None)
last_edited: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), default=None)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same

thread=thread_to_pb(session, context, discussion.thread_id),
can_moderate=can_moderate,
can_edit=(context.user_id == discussion.creator_user_id),
last_edited=Timestamp_from_datetime(discussion.last_edited) if discussion.last_edited else None,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

redundant

@aapeliv
aapeliv merged commit cd3de36 into develop Jun 8, 2026
10 checks passed
@aapeliv
aapeliv deleted the na/backend/edit-delete-discussions branch June 8, 2026 04:12
@github-actions github-actions Bot added the release notes: pending Add to stuff that should be included in release notes label Jun 8, 2026
@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

📝 Release Notes

This PR should be included in the release notes.

Suggested release note:

Added the ability to edit and delete community discussions, comments, and replies while preserving the rest of the conversation thread
🤖 Bot Debug Information

Model: couchers.openai.gpt-5.4
Decision: include
Reasoning: This adds noticeable new user-facing functionality in community discussions: users can now edit and delete their discussions, comments, and replies, with deleted posts replaced by placeholders so conversation threads remain readable. It also includes moderation/audit support, but the main impact is directly visible to end users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: pending Add to stuff that should be included in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add update and delete to discussion posts

3 participants