From 1aea6850f0f743332abf6cae6bab94908625307c Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Fri, 23 Jan 2026 10:26:50 -0500 Subject: [PATCH 1/3] Extract Card::Commentable --- app/models/card.rb | 54 +------------------------- app/models/card/commentable.rb | 57 ++++++++++++++++++++++++++++ test/models/card/commentable_test.rb | 8 ++++ test/models/card_test.rb | 8 ---- 4 files changed, 67 insertions(+), 60 deletions(-) create mode 100644 app/models/card/commentable.rb diff --git a/app/models/card.rb b/app/models/card.rb index d76512093..43e2d9361 100644 --- a/app/models/card.rb +++ b/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 @@ class Card < ApplicationRecord 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 diff --git a/app/models/card/commentable.rb b/app/models/card/commentable.rb new file mode 100644 index 000000000..5c07db47e --- /dev/null +++ b/app/models/card/commentable.rb @@ -0,0 +1,57 @@ +module Card::Commentable + extend ActiveSupport::Concern + + included do + has_many :comments, dependent: :destroy + 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 diff --git a/test/models/card/commentable_test.rb b/test/models/card/commentable_test.rb index 179828b3b..f122e3c46 100644 --- a/test/models/card/commentable_test.rb +++ b/test/models/card/commentable_test.rb @@ -1,6 +1,14 @@ require "test_helper" class Card::CommentableTest < ActiveSupport::TestCase + 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)) diff --git a/test/models/card_test.rb b/test/models/card_test.rb index bb72fddf8..55b1dc602 100644 --- a/test/models/card_test.rb +++ b/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)) From bb2d3a59b5ce392e68b3eb13d1eb2f51c547c8ee Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Thu, 22 Jan 2026 14:09:03 -0500 Subject: [PATCH 2/3] prefactor: update search to use published cards We're about to make a change to assert that draft cards are not added to the search index, and so let's make the intention behind these tests clear first. --- test/controllers/searches_controller_test.rb | 9 +++++---- test/models/card/searchable_test.rb | 14 +++++++------- test/models/comment/searchable_test.rb | 6 +++--- test/models/filter/search_test.rb | 3 +-- test/models/search_test.rb | 8 ++++---- test/test_helpers/search_test_helper.rb | 1 + 6 files changed, 21 insertions(+), 20 deletions(-) diff --git a/test/controllers/searches_controller_test.rb b/test/controllers/searches_controller_test.rb index 70ec15da9..2493fe43f 100644 --- a/test/controllers/searches_controller_test.rb +++ b/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: "Bold testing content", + status: "published", creator: @user ) diff --git a/test/models/card/searchable_test.rb b/test/models/card/searchable_test.rb index 5c7ca9c86..a67890620 100644 --- a/test/models/card/searchable_test.rb +++ b/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) diff --git a/test/models/comment/searchable_test.rb b/test/models/comment/searchable_test.rb index 7ba73847c..c1a62e607 100644 --- a/test/models/comment/searchable_test.rb +++ b/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 diff --git a/test/models/filter/search_test.rb b/test/models/filter/search_test.rb index f7b7e7968..cbdd9558d 100644 --- a/test/models/filter/search_test.rb +++ b/test/models/filter/search_test.rb @@ -4,8 +4,7 @@ class Filter::SearchTest < ActiveSupport::TestCase include SearchTestHelper test "deduplicate multiple results" do - card = @board.cards.create!(title: "Duplicate results test", description: "Have you had any haggis today?", creator: @user) - card.published! + card = @board.cards.create!(title: "Duplicate results test", description: "Have you had any haggis today?", creator: @user, status: "published") card.comments.create(body: "I hate haggis.", creator: @user) card.comments.create(body: "I love haggis.", creator: @user) diff --git a/test/models/search_test.rb b/test/models/search_test.rb index 1f4981090..343295dd0 100644 --- a/test/models/search_test.rb +++ b/test/models/search_test.rb @@ -5,8 +5,8 @@ class SearchTest < ActiveSupport::TestCase test "search" do # Search cards and comments - card = @board.cards.create!(title: "layout design", creator: @user) - comment_card = @board.cards.create!(title: "Some card", creator: @user) + card = @board.cards.create!(title: "layout design", creator: @user, status: "published") + comment_card = @board.cards.create!(title: "Some card", creator: @user, status: "published") comment_card.comments.create!(body: "overflowing text", creator: @user) results = Search::Record.for(@user.account_id).search("layout", user: @user) @@ -18,8 +18,8 @@ class SearchTest < ActiveSupport::TestCase # Don't include inaccessible boards other_user = User.create!(name: "Other User", account: @account) inaccessible_board = Board.create!(name: "Inaccessible Board", account: @account, creator: other_user) - accessible_card = @board.cards.create!(title: "searchable content", creator: @user) - inaccessible_card = inaccessible_board.cards.create!(title: "searchable content", creator: other_user) + accessible_card = @board.cards.create!(title: "searchable content", creator: @user, status: "published") + inaccessible_card = inaccessible_board.cards.create!(title: "searchable content", creator: other_user, status: "published") results = Search::Record.for(@user.account_id).search("searchable", user: @user) assert results.find { |it| it.card_id == accessible_card.id } diff --git a/test/test_helpers/search_test_helper.rb b/test/test_helpers/search_test_helper.rb index 78b113781..81cf271dc 100644 --- a/test/test_helpers/search_test_helper.rb +++ b/test/test_helpers/search_test_helper.rb @@ -17,6 +17,7 @@ module SearchTestHelper Current.account = @account @identity = Identity.create!(email_address: "test@example.com") @user = User.create!(name: "Test User", account: @account, identity: @identity) + Current.user = @user @board = Board.create!(name: "Test Board", account: @account, creator: @user) end From 9aa15e9fb2707a93eb913587657d643825b6f976 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Fri, 23 Jan 2026 11:28:15 -0500 Subject: [PATCH 3/3] Forbid comments on draft cards - Add Card#commentable? method that returns true only for published cards. - CardsController#create ensures that the card is commentable. Co-Authored-By: Claude Opus 4.5 --- app/controllers/cards/comments_controller.rb | 5 +++++ app/models/card/commentable.rb | 4 ++++ app/models/comment.rb | 6 ++++++ test/controllers/cards/comments_controller_test.rb | 10 ++++++++++ test/models/card/commentable_test.rb | 9 +++++++++ test/models/comment_test.rb | 13 +++++++++++++ test/models/concerns/mentions_test.rb | 10 ---------- 7 files changed, 47 insertions(+), 10 deletions(-) diff --git a/app/controllers/cards/comments_controller.rb b/app/controllers/cards/comments_controller.rb index 27c507abe..f2b318813 100644 --- a/app/controllers/cards/comments_controller.rb +++ b/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 @@ class Cards::CommentsController < ApplicationController 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 diff --git a/app/models/card/commentable.rb b/app/models/card/commentable.rb index 5c07db47e..4296a77a7 100644 --- a/app/models/card/commentable.rb +++ b/app/models/card/commentable.rb @@ -5,6 +5,10 @@ module Card::Commentable has_many :comments, dependent: :destroy end + def commentable? + published? + end + private STORAGE_BATCH_SIZE = 1000 diff --git a/app/models/comment.rb b/app/models/comment.rb index 41e068514..70a36562f 100644 --- a/app/models/comment.rb +++ b/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 @@ class Comment < ApplicationRecord 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 diff --git a/test/controllers/cards/comments_controller_test.rb b/test/controllers/cards/comments_controller_test.rb index ef6dbf6e2..0103a846f 100644 --- a/test/controllers/cards/comments_controller_test.rb +++ b/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 diff --git a/test/models/card/commentable_test.rb b/test/models/card/commentable_test.rb index f122e3c46..8b3c93f4d 100644 --- a/test/models/card/commentable_test.rb +++ b/test/models/card/commentable_test.rb @@ -1,6 +1,10 @@ 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.") @@ -19,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 diff --git a/test/models/comment_test.rb b/test/models/comment_test.rb index 13a5e36c3..bd2089f0e 100644 --- a/test/models/comment_test.rb +++ b/test/models/comment_test.rb @@ -5,6 +5,19 @@ class CommentTest < ActiveSupport::TestCase Current.session = sessions(:david) end + test "cannot create comment on a draft card" do + draft_card = cards(:unfinished_thoughts) + + comment = draft_card.comments.build(body: "This should fail") + + assert_not comment.valid? + assert_includes comment.errors[:card], "does not allow comments" + + assert_raises(ActiveRecord::RecordInvalid) do + draft_card.comments.create!(body: "This should raise") + end + end + test "rich text embed variants are processed immediately on attachment" do comment = cards(:logo).comments.create!(body: "Check this out") comment.body.body.attachables # force load diff --git a/test/models/concerns/mentions_test.rb b/test/models/concerns/mentions_test.rb index 48b94e6f1..fca379bf1 100644 --- a/test/models/concerns/mentions_test.rb +++ b/test/models/concerns/mentions_test.rb @@ -70,16 +70,6 @@ class MentionsTest < ActiveSupport::TestCase end end - test "don't create mentions from comments when belonging to unpublished cards" do - perform_enqueued_jobs only: Mention::CreateJob do - card = boards(:writebook).cards.create title: "Cleanup", description: "Some initial content" - - assert_no_difference -> { Mention.count } do - card.comments.create!(body: "Great work on this #{mention_html_for(users(:david))}!") - end - end - end - test "can't mention users that don't have access to the board" do boards(:writebook).update! all_access: false boards(:writebook).accesses.revoke_from(users(:david))