Merge pull request #2418 from basecamp/filter-drafts-in-search

Exclude draft cards from search index

Author: flavorjones

app/models/card/searchable.rb

@@ -25,4 +25,8 @@ def search_card_id
   def search_board_id
     board_id
   end
+
+  def searchable?
+    published?
+  end
 end

app/models/comment/searchable.rb

@@ -20,4 +20,8 @@ def search_card_id
   def search_board_id
     card.board_id
   end
+
+  def searchable?
+    card.published?
+  end
 end

app/models/concerns/searchable.rb

@@ -15,11 +15,17 @@ def reindex
 
   private
     def create_in_search_index
-      search_record_class.create!(search_record_attributes)
+      if searchable?
+        search_record_class.create!(search_record_attributes)
+      end
     end
 
     def update_in_search_index
-      search_record_class.upsert!(search_record_attributes)
+      if searchable?
+        search_record_class.upsert!(search_record_attributes)
+      else
+        remove_from_search_index
+      end
     end
 
     def remove_from_search_index
@@ -53,4 +59,5 @@ def search_record_class
   # - search_content: returns content string
   # - search_card_id: returns the card id (self.id for cards, card_id for comments)
   # - search_board_id: returns the board id
+  # - searchable?: returns whether this record should be indexed
 end

script/migrations/20260123-remove-draft-cards-from-search-index.rb

@@ -0,0 +1,22 @@
+#!/usr/bin/env ruby
+
+require_relative "../../config/environment"
+
+total_deleted = 0
+
+Account.find_each do |account|
+  search_record_class = Search::Record.for(account.id)
+
+  # Find search records for draft cards (both Card and Comment searchables)
+  draft_card_ids = Card.where(account_id: account.id, status: "drafted").pluck(:id)
+
+  if draft_card_ids.any?
+    count = search_record_class.where(card_id: draft_card_ids).delete_all
+    if count > 0
+      puts "#{account.name}: deleted #{count} search records for draft cards"
+      total_deleted += count
+    end
+  end
+end
+
+puts "Migration completed! Total deleted: #{total_deleted}"

test/models/card/searchable_test.rb

@@ -3,6 +3,16 @@
 class Card::SearchableTest < ActiveSupport::TestCase
   include SearchTestHelper
 
+  test "searchable? returns true for published cards" do
+    card = @board.cards.create!(title: "Published Card", status: "published", creator: @user)
+    assert card.searchable?
+  end
+
+  test "searchable? returns false for draft cards" do
+    card = @board.cards.create!(title: "Draft Card", status: "drafted", creator: @user)
+    assert_not card.searchable?
+  end
+
   test "card search" do
     # Searching by title
     card = @board.cards.create!(title: "layout is broken", status: "published", creator: @user)
@@ -78,4 +88,46 @@ class Card::SearchableTest < ActiveSupport::TestCase
       assert_equal 0, fts_count, "FTS entry should be deleted"
     end
   end
+
+  test "updating a draft card does not index it" do
+    search_record_class = Search::Record.for(@user.account_id)
+
+    card = @board.cards.create!(title: "Draft card", creator: @user, status: "drafted")
+    assert_nil search_record_class.find_by(searchable_type: "Card", searchable_id: card.id)
+
+    card.update!(title: "Updated draft card")
+    assert_nil search_record_class.find_by(searchable_type: "Card", searchable_id: card.id),
+      "Draft card should not be indexed after update"
+
+    results = Card.mentioning("Updated", user: @user)
+    assert_not_includes results, card
+  end
+
+  test "publishing a draft card indexes it" do
+    search_record_class = Search::Record.for(@user.account_id)
+
+    card = @board.cards.create!(title: "Draft to publish", creator: @user, status: "drafted")
+    assert_nil search_record_class.find_by(searchable_type: "Card", searchable_id: card.id)
+
+    card.publish
+    search_record = search_record_class.find_by(searchable_type: "Card", searchable_id: card.id)
+    assert_not_nil search_record, "Published card should be indexed"
+    assert_equal card.id, search_record.card_id
+
+    results = Card.mentioning("publish", user: @user)
+    assert_includes results, card
+  end
+
+  test "unpublishing a draft card removes it from the search index" do
+    search_record_class = Search::Record.for(@user.account_id)
+
+    card = @board.cards.create!(title: "Draft to publish", creator: @user, status: "published")
+    assert_not_nil search_record_class.find_by(searchable_type: "Card", searchable_id: card.id)
+
+    card.update!(status: "drafted")
+
+    assert_nil search_record_class.find_by(searchable_type: "Card", searchable_id: card.id)
+    results = Card.mentioning("publish", user: @user)
+    assert_not_includes results, card
+  end
 end

test/models/comment/searchable_test.rb

@@ -7,6 +7,17 @@ class Comment::SearchableTest < ActiveSupport::TestCase
     @card = @board.cards.create!(title: "Test Card", status: "published", creator: @user)
   end
 
+  test "searchable? returns true for comments on published cards" do
+    comment = @card.comments.create!(body: "test comment", creator: @user)
+    assert comment.searchable?
+  end
+
+  test "searchable? returns false for comments on draft cards" do
+    draft_card = @board.cards.create!(title: "Draft Card", status: "drafted", creator: @user)
+    comment = draft_card.comments.build(body: "test comment", creator: @user)
+    assert_not comment.searchable?
+  end
+
   test "comment search" do
     search_record_class = Search::Record.for(@user.account_id)
     # Comment is indexed on create

test/models/search_test.rb

@@ -15,6 +15,11 @@ class SearchTest < ActiveSupport::TestCase
     results = Search::Record.for(@user.account_id).search("overflowing", user: @user)
     assert results.find { |it| it.card_id == comment_card.id && it.searchable_type == "Comment" }
 
+    # Drafted cards are excluded from search results
+    drafted_card = @board.cards.create!(title: "drafted searchable content", creator: @user, status: "drafted")
+    results = Search::Record.for(@user.account_id).search("drafted", user: @user)
+    assert_not results.find { |it| it.card_id == drafted_card.id }
+
     # 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)