Merge pull request #2423 from basecamp/forbid-comments-on-draft-cards

Forbid comments on draft cards

Author: flavorjones

app/controllers/cards/comments_controller.rb

@@ -3,6 +3,7 @@ class Cards::CommentsController < ApplicationController
 
   before_action :set_comment, only: %i[ show edit update destroy ]
   before_action :ensure_creatorship, only: %i[ edit update destroy ]
+  before_action :ensure_card_is_commentable, only: :create
 
   def index
     set_page_and_extract_portion_from @card.comments.chronologically
@@ -50,6 +51,10 @@ def ensure_creatorship
       head :forbidden if Current.user != @comment.creator
     end
 
+    def ensure_card_is_commentable
+      head :forbidden unless @card.commentable?
+    end
+
     def comment_params
       params.expect(comment: [ :body, :created_at ])
     end

app/models/card.rb

@@ -1,13 +1,12 @@
 class Card < ApplicationRecord
-  include Accessible, Assignable, Attachments, Broadcastable, Closeable, Colored, Entropic, Eventable,
-    Exportable, Golden, Mentions, Multistep, Pinnable, Postponable, Promptable,
+  include Accessible, Assignable, Attachments, Broadcastable, Closeable, Colored, Commentable,
+    Entropic, Eventable, Exportable, Golden, Mentions, Multistep, Pinnable, Postponable, Promptable,
     Readable, Searchable, Stallable, Statuses, Storage::Tracked, Taggable, Triageable, Watchable
 
   belongs_to :account, default: -> { board.account }
   belongs_to :board
   belongs_to :creator, class_name: "User", default: -> { Current.user }
 
-  has_many :comments, dependent: :destroy
   has_one_attached :image, dependent: :purge_later
 
   has_rich_text :description
@@ -66,55 +65,6 @@ def filled?
   end
 
   private
-    STORAGE_BATCH_SIZE = 1000
-
-    # Override to include comments, but only load comments that have attachments.
-    # Cards can have thousands of comments; most won't have attachments.
-    # Streams in batches to avoid loading all IDs into memory at once.
-    def storage_transfer_records
-      comment_ids_with_attachments = storage_comment_ids_with_attachments
-
-      if comment_ids_with_attachments.any?
-        [ self, *comments.where(id: comment_ids_with_attachments).to_a ]
-      else
-        [ self ]
-      end
-    end
-
-    def storage_comment_ids_with_attachments
-      direct = []
-      rich_text_map = {}
-
-      # Stream comment IDs in batches to avoid loading all into memory
-      comments.in_batches(of: STORAGE_BATCH_SIZE) do |batch|
-        batch_ids = batch.pluck(:id)
-
-        direct.concat \
-          ActiveStorage::Attachment
-            .where(record_type: "Comment", record_id: batch_ids)
-            .distinct
-            .pluck(:record_id)
-
-        ActionText::RichText
-          .where(record_type: "Comment", record_id: batch_ids)
-          .pluck(:id, :record_id)
-          .each { |rt_id, comment_id| rich_text_map[rt_id] = comment_id }
-      end
-
-      embed_comment_ids = if rich_text_map.any?
-        rich_text_map.keys.each_slice(STORAGE_BATCH_SIZE).flat_map do |batch_ids|
-          ActiveStorage::Attachment
-            .where(record_type: "ActionText::RichText", record_id: batch_ids)
-            .distinct
-            .pluck(:record_id)
-        end.filter_map { |rt_id| rich_text_map[rt_id] }
-      else
-        []
-      end
-
-      (direct + embed_comment_ids).uniq
-    end
-
     def set_default_title
       self.title = "Untitled" if title.blank?
     end

app/models/card/commentable.rb

@@ -0,0 +1,61 @@
+module Card::Commentable
+  extend ActiveSupport::Concern
+
+  included do
+    has_many :comments, dependent: :destroy
+  end
+
+  def commentable?
+    published?
+  end
+
+  private
+    STORAGE_BATCH_SIZE = 1000
+
+    # Override to include comments, but only load comments that have attachments.
+    # Cards can have thousands of comments; most won't have attachments.
+    # Streams in batches to avoid loading all IDs into memory at once.
+    def storage_transfer_records
+      comment_ids_with_attachments = storage_comment_ids_with_attachments
+
+      if comment_ids_with_attachments.any?
+        [ self, *comments.where(id: comment_ids_with_attachments).to_a ]
+      else
+        [ self ]
+      end
+    end
+
+    def storage_comment_ids_with_attachments
+      direct = []
+      rich_text_map = {}
+
+      # Stream comment IDs in batches to avoid loading all into memory
+      comments.in_batches(of: STORAGE_BATCH_SIZE) do |batch|
+        batch_ids = batch.pluck(:id)
+
+        direct.concat \
+          ActiveStorage::Attachment
+            .where(record_type: "Comment", record_id: batch_ids)
+            .distinct
+            .pluck(:record_id)
+
+        ActionText::RichText
+          .where(record_type: "Comment", record_id: batch_ids)
+          .pluck(:id, :record_id)
+          .each { |rt_id, comment_id| rich_text_map[rt_id] = comment_id }
+      end
+
+      embed_comment_ids = if rich_text_map.any?
+        rich_text_map.keys.each_slice(STORAGE_BATCH_SIZE).flat_map do |batch_ids|
+          ActiveStorage::Attachment
+            .where(record_type: "ActionText::RichText", record_id: batch_ids)
+            .distinct
+            .pluck(:record_id)
+        end.filter_map { |rt_id| rich_text_map[rt_id] }
+      else
+        []
+      end
+
+      (direct + embed_comment_ids).uniq
+    end
+end

app/models/comment.rb

@@ -8,6 +8,8 @@ class Comment < ApplicationRecord
 
   has_rich_text :body
 
+  validate :card_is_commentable
+
   scope :chronologically, -> { order created_at: :asc, id: :desc }
   scope :preloaded, -> { with_rich_text_body.includes(reactions: :reacter) }
   scope :by_system, -> { joins(:creator).where(creator: { role: :system }) }
@@ -22,6 +24,10 @@ def to_partial_path
   end
 
   private
+    def card_is_commentable
+      errors.add(:card, "does not allow comments") unless card.commentable?
+    end
+
     def watch_card_by_creator
       card.watch_by creator
     end

test/controllers/cards/comments_controller_test.rb

@@ -13,6 +13,16 @@ class Cards::CommentsControllerTest < ActionDispatch::IntegrationTest
     assert_response :success
   end
 
+  test "create on draft card is forbidden" do
+    draft_card = boards(:writebook).cards.create!(status: :drafted, creator: users(:kevin))
+
+    assert_no_difference -> { draft_card.comments.count } do
+      post card_comments_path(draft_card), params: { comment: { body: "This should be forbidden" } }, as: :json
+    end
+
+    assert_response :forbidden
+  end
+
   test "update" do
     put card_comment_path(cards(:logo), comments(:logo_agreement_kevin)), params: { comment: { body: "I've changed my mind" } }, as: :turbo_stream
 

test/controllers/searches_controller_test.rb

@@ -5,10 +5,10 @@ class SearchesControllerTest < ActionDispatch::IntegrationTest
 
   setup do
     @board.update!(all_access: true)
-    @card = @board.cards.create!(title: "Layout is broken", description: "Look at this mess.", creator: @user)
-    @comment_card = @board.cards.create!(title: "Some card", creator: @user)
+    @card = @board.cards.create!(title: "Layout is broken", description: "Look at this mess.", status: "published", creator: @user)
+    @comment_card = @board.cards.create!(title: "Some card", status: "published", creator: @user)
     @comment_card.comments.create!(body: "overflowing text issue", creator: @user)
-    @comment2_card = @board.cards.create!(title: "Just haggis", description: "More haggis", creator: @user)
+    @comment2_card = @board.cards.create!(title: "Just haggis", description: "More haggis", status: "published", creator: @user)
     @comment2_card.comments.create!(body: "I love haggis", creator: @user)
 
     untenanted { sign_in_as @user }
@@ -43,7 +43,7 @@ class SearchesControllerTest < ActionDispatch::IntegrationTest
   end
 
   test "search highlights matched terms with proper HTML marks" do
-    @board.cards.create!(title: "Testing search highlighting", creator: @user)
+    @board.cards.create!(title: "Testing search highlighting", status: "published", creator: @user)
 
     get search_path(q: "highlighting", script_name: "/#{@account.external_account_id}")
     assert_response :success
@@ -52,6 +52,7 @@ class SearchesControllerTest < ActionDispatch::IntegrationTest
   test "search preserves highlight marks but escapes surrounding HTML" do
     @board.cards.create!(
       title: "<b>Bold</b> testing content",
+      status: "published",
       creator: @user
     )
 

test/models/card/commentable_test.rb

@@ -1,6 +1,18 @@
 require "test_helper"
 
 class Card::CommentableTest < ActiveSupport::TestCase
+  setup do
+    Current.session = sessions(:david)
+  end
+
+  test "capturing comments" do
+    assert_difference -> { cards(:logo).comments.count }, +1 do
+      cards(:logo).comments.create!(body: "Agreed.")
+    end
+
+    assert_equal "Agreed.", cards(:logo).comments.last.body.to_plain_text.chomp
+  end
+
   test "creating a comment on a card makes the creator watch the card" do
     boards(:writebook).access_for(users(:kevin)).access_only!
     assert_not cards(:text).watched_by?(users(:kevin))
@@ -11,4 +23,9 @@ class Card::CommentableTest < ActiveSupport::TestCase
 
     assert cards(:text).watched_by?(users(:kevin))
   end
+
+  test "commentable is true for published cards, false for drafts" do
+    assert cards(:logo).commentable?
+    assert_not cards(:unfinished_thoughts).commentable?
+  end
 end

test/models/card/searchable_test.rb

@@ -5,18 +5,18 @@ class Card::SearchableTest < ActiveSupport::TestCase
 
   test "card search" do
     # Searching by title
-    card = @board.cards.create!(title: "layout is broken", creator: @user)
+    card = @board.cards.create!(title: "layout is broken", status: "published", creator: @user)
     results = Card.mentioning("layout", user: @user)
     assert_includes results, card
 
     # Searching by comment
-    card_with_comment = @board.cards.create!(title: "Some card", creator: @user)
+    card_with_comment = @board.cards.create!(title: "Some card", status: "published", creator: @user)
     card_with_comment.comments.create!(body: "overflowing text", creator: @user)
     results = Card.mentioning("overflowing", user: @user)
     assert_includes results, card_with_comment
 
     # Sanitizing search query
-    card_broken = @board.cards.create!(title: "broken layout", creator: @user)
+    card_broken = @board.cards.create!(title: "broken layout", status: "published", creator: @user)
     results = Card.mentioning("broken \"", user: @user)
     assert_includes results, card_broken
 
@@ -25,8 +25,8 @@ class Card::SearchableTest < ActiveSupport::TestCase
 
     # Filtering by board_ids
     other_board = Board.create!(name: "Other Board", account: @account, creator: @user)
-    card_in_board = @board.cards.create!(title: "searchable content", creator: @user)
-    card_in_other_board = other_board.cards.create!(title: "searchable content", creator: @user)
+    card_in_board = @board.cards.create!(title: "searchable content", status: "published", creator: @user)
+    card_in_other_board = other_board.cards.create!(title: "searchable content", status: "published", creator: @user)
     results = Card.mentioning("searchable", user: @user)
     assert_includes results, card_in_board
     assert_not_includes results, card_in_other_board
@@ -37,7 +37,7 @@ class Card::SearchableTest < ActiveSupport::TestCase
 
     # Create a card with unreasonably long content
     long_content = "asdf " * Searchable::SEARCH_CONTENT_LIMIT
-    card = @board.cards.create!(title: "Card with long description", creator: @user)
+    card = @board.cards.create!(title: "Card with long description", status: "published", creator: @user)
     card.description = ActionText::Content.new(long_content)
     card.save!
 
@@ -52,7 +52,7 @@ class Card::SearchableTest < ActiveSupport::TestCase
 
   test "deleting card removes search record and FTS entry" do
     search_record_class = Search::Record.for(@user.account_id)
-    card = @board.cards.create!(title: "Card to delete", creator: @user)
+    card = @board.cards.create!(title: "Card to delete", status: "published", creator: @user)
 
     # Verify search record exists
     search_record = search_record_class.find_by(searchable_type: "Card", searchable_id: card.id)

test/models/card_test.rb

@@ -18,14 +18,6 @@ class CardTest < ActiveSupport::TestCase
     assert_equal account.reload.cards_count, card.number
   end
 
-  test "capturing messages" do
-    assert_difference -> { cards(:logo).comments.count }, +1 do
-      cards(:logo).comments.create!(body: "Agreed.")
-    end
-
-    assert_equal "Agreed.", cards(:logo).comments.last.body.to_plain_text.chomp
-  end
-
   test "assignment states" do
     assert cards(:logo).assigned_to?(users(:kevin))
     assert_not cards(:logo).assigned_to?(users(:david))

test/models/comment/searchable_test.rb

@@ -4,7 +4,7 @@ class Comment::SearchableTest < ActiveSupport::TestCase
   include SearchTestHelper
 
   setup do
-    @card = @board.cards.create!(title: "Test Card", creator: @user)
+    @card = @board.cards.create!(title: "Test Card", status: "published", creator: @user)
   end
 
   test "comment search" do
@@ -40,9 +40,9 @@ class Comment::SearchableTest < ActiveSupport::TestCase
     end
 
     # Finding cards via comment search
-    card_with_comment = @board.cards.create!(title: "Card One", creator: @user)
+    card_with_comment = @board.cards.create!(title: "Card One", status: "published", creator: @user)
     card_with_comment.comments.create!(body: "unique searchable phrase", creator: @user)
-    card_without_comment = @board.cards.create!(title: "Card Two", creator: @user)
+    card_without_comment = @board.cards.create!(title: "Card Two", status: "published", creator: @user)
     results = Card.mentioning("searchable", user: @user)
     assert_includes results, card_with_comment
     assert_not_includes results, card_without_comment