From 1a5091ecfb8beb9846b6240a9a535023f9e50b39 Mon Sep 17 00:00:00 2001
From: Gabriel Grubba <70247653+Grubba27@users.noreply.github.com>
Date: Wed, 4 Dec 2024 17:01:44 -0300
Subject: [PATCH] FIX: pending_assigns_reminder reminder leaking total assigned
 topics count (#617)

Before this commit, the reminder was leaking the total assigned topics count in the title.

In the post raw it was ensured that it was not being leaked but the title was still leaking the count.

Also add specs and updated existing specs to ensure that the count is not leaked in the title.
---
 lib/pending_assigns_reminder.rb           | 16 +++++----
 spec/lib/pending_assigns_reminder_spec.rb | 43 ++++++++++++++++++++---
 2 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/lib/pending_assigns_reminder.rb b/lib/pending_assigns_reminder.rb
index c681d3d..49db18c 100644
--- a/lib/pending_assigns_reminder.rb
+++ b/lib/pending_assigns_reminder.rb
@@ -55,21 +55,23 @@ class PendingAssignsReminder
     posts.find_each { |post| PostDestroyer.new(Discourse.system_user, post).destroy }
   end
 
+  def visible_topics(user)
+    Topic.listable_topics.secured(Guardian.new(user)).or(Topic.private_messages_for_user(user))
+  end
+
   def assigned_count_for(user)
     assignments =
-      Assignment.joins_with_topics.where(
-        assigned_to_id: user.id,
-        assigned_to_type: "User",
-        active: true,
-      )
+      Assignment
+        .joins_with_topics
+        .where(assigned_to_id: user.id, assigned_to_type: "User", active: true)
+        .merge(visible_topics(user))
     assignments =
       DiscoursePluginRegistry.apply_modifier(:assigned_count_for_user_query, assignments, user)
     assignments.count
   end
 
   def assigned_topics(user, order:)
-    secure =
-      Topic.listable_topics.secured(Guardian.new(user)).or(Topic.private_messages_for_user(user))
+    secure = visible_topics(user)
 
     topics =
       Topic
diff --git a/spec/lib/pending_assigns_reminder_spec.rb b/spec/lib/pending_assigns_reminder_spec.rb
index 3c83293..8574591 100644
--- a/spec/lib/pending_assigns_reminder_spec.rb
+++ b/spec/lib/pending_assigns_reminder_spec.rb
@@ -60,7 +60,7 @@ RSpec.describe PendingAssignsReminder do
 
       expect(topic.topic_allowed_users.pluck(:user_id)).to contain_exactly(system.id, user.id)
 
-      expect(topic.title).to eq(I18n.t("pending_assigns_reminder.title", pending_assignments: 3))
+      expect(topic.title).to eq(I18n.t("pending_assigns_reminder.title", pending_assignments: 2))
 
       expect(post.raw).to include(@post1.topic.fancy_title)
       expect(post.raw).to include(@post2.topic.fancy_title)
@@ -119,6 +119,41 @@ RSpec.describe PendingAssignsReminder do
       expect(reminders_count).to eq(2)
     end
 
+    it "doesn't leak assigned topics that were moved to PM" do
+      # we already add a fail to assign when the assigned user cannot view the pm
+      # so we don't need to test that here
+      # but if we move a topic to a PM that the user can't see, we should not
+      # include it in the reminder
+      post = Fabricate(:post)
+      Assigner.new(post.topic, user).assign(user)
+      post.topic.update(archetype: Archetype.private_message, category: nil)
+      reminder.remind(user)
+
+      post = Post.last
+      topic = post.topic
+      expect(topic.title).to eq(I18n.t("pending_assigns_reminder.title", pending_assignments: 2))
+      expect(post.raw).to include(@post1.topic.fancy_title)
+      expect(post.raw).to include(@post2.topic.fancy_title)
+
+      expect(post.raw).to_not include(post.topic.fancy_title)
+    end
+
+    it "reminds about PMs" do
+      pm = Fabricate(:private_message_topic, user: user)
+      Fabricate(:post, topic: pm)
+
+      Assigner.new(pm, user).assign(user)
+      reminder.remind(user)
+
+      post = Post.last
+      topic = post.topic
+
+      expect(topic.title).to eq(I18n.t("pending_assigns_reminder.title", pending_assignments: 3))
+      expect(post.raw).to include(@post1.topic.fancy_title)
+      expect(post.raw).to include(@post2.topic.fancy_title)
+      expect(post.raw).to include(pm.fancy_title)
+    end
+
     it "closed topics aren't included as active assigns" do
       SiteSetting.unassign_on_close = true
 
@@ -130,7 +165,7 @@ RSpec.describe PendingAssignsReminder do
       post = Post.last
       topic = post.topic
 
-      expect(topic.title).to eq(I18n.t("pending_assigns_reminder.title", pending_assignments: 4))
+      expect(topic.title).to eq(I18n.t("pending_assigns_reminder.title", pending_assignments: 3))
 
       @post5.topic.update_status("closed", true, Discourse.system_user)
       expect(@post5.topic.closed).to eq(true)
@@ -140,7 +175,7 @@ RSpec.describe PendingAssignsReminder do
       post = Post.last
       topic = post.topic
 
-      expect(topic.title).to eq(I18n.t("pending_assigns_reminder.title", pending_assignments: 3))
+      expect(topic.title).to eq(I18n.t("pending_assigns_reminder.title", pending_assignments: 2))
     end
 
     context "with assigns_reminder_assigned_topics_query modifier" do
@@ -162,7 +197,7 @@ RSpec.describe PendingAssignsReminder do
     context "with assigned_count_for_user_query modifier" do
       let(:modifier_block) { Proc.new { |query, user| query.where.not(assigned_to_id: user.id) } }
       it "updates the query correctly" do
-        expect(reminder.send(:assigned_count_for, user)).to eq(3)
+        expect(reminder.send(:assigned_count_for, user)).to eq(2)
 
         plugin_instance = Plugin::Instance.new
         plugin_instance.register_modifier(:assigned_count_for_user_query, &modifier_block)