From e54f2da1a579bc2148a4cfc65604e62cf9b8541e Mon Sep 17 00:00:00 2001
From: Roman Rizzi <roman@discourse.org>
Date: Fri, 22 Nov 2024 12:07:27 -0300
Subject: [PATCH] FIX: Unnecessary complex preloading accidentally filters some
 topics. (#945)

The `topic_query_create_list_topics` modifier we append was always meant to avoid an N+1 situation when serializing gists. However, I tried to be too smart and only preload these, which resulted in some topics with *only* regular summaries getting removed from the list. This issue became apparent now we are adding gists to other lists besides hot.

Let's simplify the preloading, which still solves the N+1 issue, and let the serializer get the needed summary.
---
 lib/summarization/entry_point.rb              | 22 ++++------
 .../modules/summarization/entry_point_spec.rb | 40 +++++++++++++++----
 2 files changed, 41 insertions(+), 21 deletions(-)

diff --git a/lib/summarization/entry_point.rb b/lib/summarization/entry_point.rb
index e97cd022..3117bcc4 100644
--- a/lib/summarization/entry_point.rb
+++ b/lib/summarization/entry_point.rb
@@ -17,20 +17,14 @@ module DiscourseAi
           scope.can_see_summary?(object.topic)
         end
 
-        # plugin.register_modifier(:topic_query_create_list_topics) do |topics, options|
-        #   if Discourse.filters.include?(options[:filter]) && SiteSetting.ai_summarization_enabled &&
-        #        SiteSetting.ai_summarize_max_hot_topics_gists_per_batch > 0
-        #     topics
-        #       .includes(:ai_summaries)
-        #       .where(
-        #         "ai_summaries.id IS NULL OR ai_summaries.summary_type = ?",
-        #         AiSummary.summary_types[:gist],
-        #       )
-        #       .references(:ai_summaries)
-        #   else
-        #     topics
-        #   end
-        # end
+        plugin.register_modifier(:topic_query_create_list_topics) do |topics, options|
+          if Discourse.filters.include?(options[:filter]) && SiteSetting.ai_summarization_enabled &&
+               SiteSetting.ai_summarize_max_hot_topics_gists_per_batch > 0
+            topics.includes(:ai_summaries)
+          else
+            topics
+          end
+        end
 
         plugin.add_to_serializer(
           :topic_list_item,
diff --git a/spec/lib/modules/summarization/entry_point_spec.rb b/spec/lib/modules/summarization/entry_point_spec.rb
index 1e571c31..9fa06b0a 100644
--- a/spec/lib/modules/summarization/entry_point_spec.rb
+++ b/spec/lib/modules/summarization/entry_point_spec.rb
@@ -4,6 +4,7 @@ RSpec.describe DiscourseAi::Summarization::EntryPoint do
   before do
     assign_fake_provider_to(:ai_summarization_model)
     SiteSetting.ai_summarization_enabled = true
+    SiteSetting.ai_summarize_max_hot_topics_gists_per_batch = 100
   end
 
   fab!(:user)
@@ -11,7 +12,6 @@ RSpec.describe DiscourseAi::Summarization::EntryPoint do
   describe "#inject_into" do
     describe "hot topics gist summarization" do
       fab!(:topic_ai_gist)
-      fab!(:regular_summary) { Fabricate(:ai_summary, target: topic_ai_gist.target) }
 
       before { TopicHotScore.create!(topic_id: topic_ai_gist.target_id, score: 1.0) }
 
@@ -19,13 +19,22 @@ RSpec.describe DiscourseAi::Summarization::EntryPoint do
 
       describe "topic_query_create_list_topics modifier" do
         context "when hot topic summarization is enabled" do
-          before { SiteSetting.ai_summarize_max_hot_topics_gists_per_batch = 100 }
+          it "doesn't duplicate records when there more than one summary type" do
+            Fabricate(:ai_summary, target: topic_ai_gist.target)
 
-          skip "preloads only gist summaries" do
-            gist_topic = topic_query.list_hot.topics.find { |t| t.id == topic_ai_gist.target_id }
+            expect(topic_query.list_hot.topics.map(&:id)).to contain_exactly(
+              topic_ai_gist.target_id,
+            )
+          end
 
-            expect(gist_topic.ai_summaries.size).to eq(1)
-            expect(gist_topic.ai_summaries.first).to eq(topic_ai_gist)
+          it "doesn't exclude records when the topic has a single different summary" do
+            regular_summary_2 = Fabricate(:ai_summary)
+            TopicHotScore.create!(topic_id: regular_summary_2.target_id, score: 1.0)
+
+            expect(topic_query.list_hot.topics.map(&:id)).to contain_exactly(
+              regular_summary_2.target_id,
+              topic_ai_gist.target_id,
+            )
           end
 
           it "doesn't filter out hot topics without summaries" do
@@ -118,11 +127,28 @@ RSpec.describe DiscourseAi::Summarization::EntryPoint do
                 gist_topic,
                 scope: Guardian.new(admin),
                 root: false,
-                filter: :hot,
+                filter: :unread,
               ).as_json
 
             expect(serialized[:ai_topic_gist]).to be_present
           end
+
+          it "doesn't include the summary if it's not a gist" do
+            regular_summary_2 = Fabricate(:ai_summary)
+            TopicHotScore.create!(topic_id: regular_summary_2.target_id, score: 1.0)
+
+            hot_topic = topic_query.list_hot.topics.find { |t| t.id == regular_summary_2.target_id }
+
+            serialized =
+              TopicListItemSerializer.new(
+                hot_topic,
+                scope: Guardian.new(user),
+                root: false,
+                filter: :hot,
+              ).as_json
+
+            expect(serialized[:ai_topic_gist]).to be_nil
+          end
         end
       end
     end