feat: add CommentPolicy and CommentThreadPolicy (part 2 of Commontator removal) - #7835
feat: add CommentPolicy and CommentThreadPolicy (part 2 of Commontator removal)#7835Shruti2110-coder wants to merge 7 commits into
Conversation
First step of replacing the Commontator gem. Adds the comment_threads and comments tables together with their models, factories and model specs. Mirrors Commontator's existing structure: threads attached polymorphically to a commentable, comments with nested replies, soft delete and edit tracking, and thread open/closed state. Nothing is wired up and nothing is removed. Commontator remains fully in use, so there is no behaviour change.
A comment could previously be set as its own parent, or two comments could reference each other, causing replies to recurse indefinitely during dependent: :destroy.
Second step of replacing the Commontator gem. Ports the comment and thread permission rules into Pundit policies. Rules were read from commontator-7.0.1's Comment model rather than inferred from the initializer, which surfaced three behaviours the config alone does not describe: moderators may delete inside a closed thread, editing requires the comment not to have been edited by someone else, and voting is blocked on closed threads and deleted comments. Nothing calls these policies yet, so there is no behaviour change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. WalkthroughIntroduces Merge Risk: ⚪ Minimal · up to This change adds comment and thread permission policies without changing the controllers that currently enforce behavior, so no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: bbba17b5-16b1-429f-87c2-a543472c70e7
📒 Files selected for processing (12)
app/models/comment.rbapp/models/comment_thread.rbapp/models/project.rbapp/policies/comment_policy.rbapp/policies/comment_thread_policy.rbdb/migrate/20260901000000_create_comment_tables.rbdb/schema.rbspec/factories/comment.rbspec/models/comment_spec.rbspec/models/comment_thread_spec.rbspec/policies/comment_policy_spec.rbspec/policies/comment_thread_policy_spec.rb
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
restore? delegated to destroy?, which obscured that the two rules differ for authors: restoring additionally requires the comment to be deleted and the author to have been the one who deleted it.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 9a111c84-9cd7-40c2-8abc-5e60de5378f6
📒 Files selected for processing (2)
app/policies/comment_policy.rbspec/policies/comment_policy_spec.rb
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.
restore? returned true for moderators before checking comment.deleted?, so it authorized restoring a comment that was never deleted.
JituRewar
left a comment
There was a problem hiding this comment.
@Shruti2110-coder The cycle guard is currently application-level only, so writes through update_column, bulk SQL, or concurrent updates can still introduce a parent cycle. Since replies uses dependent: :destroy, such a cycle could cause recursive destruction issues.
Could we ensure the upcoming Commontator data migration and any bulk-write paths explicitly validate/preserve this invariant? If DB-level enforcement isn't practical, a migration-time validation would at least prevent importing invalid cycles.
|
@JituRewar Yes, agreed — that's the plan. The data migration will do a bulk import from commontator_comments, which bypasses model validations entirely, so it's the one path where a cycle could realistically be introduced. I'll include in that PR: A cycle check on the source data before insert, so anything invalid in the Commontator tables is caught rather than carried over. Beyond that, any bulk write goes through the model rather than raw SQL. I'll link back to this thread from the migration PR so the requirement is traceable. |
|
@Shruti2110-coder Great |
Depends on #7831.
Second step of replacing the Commontator gem. Ports the comment and thread
permission rules into Pundit policies.
What this adds
CommentThreadPolicy— read, comment, subscribe, close/reopenCommentPolicy— show, create, update, destroy, restore, voteRules ported
Note on how these were derived
Rules were read from
commontator-7.0.1'sCommentmodel rather thaninferred from the initializer. That surfaced three behaviours the config alone
does not describe:
can_be_deleted_by?, the moderator branch returns before theclosed-thread check, so moderators may delete inside a closed thread.
can_be_edited_by?also requireseditor.nil? || user == editor.can_be_voted_on?blocks voting on closed threads and deleted comments.What this does not do
Nothing calls these policies yet. The controllers still use Commontator, so
there is no behaviour change.
Approach note
Neither policy calls
super.ApplicationPolicy#initializeraises for nilusers, but comments on public projects must stay readable anonymously — the
existing API spec asserts this.
ProjectPolicyskipssuperfor the samereason.
Summary by CodeRabbit