FIX: pending_assigns_reminder reminder leaking total assigned topics count

lib/pending_assigns_reminder.rb

@@ -55,21 +55,23 @@ def delete_previous_reminders(user)
     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

spec/lib/pending_assigns_reminder_spec.rb

@@ -60,7 +60,7 @@ def assert_reminder_not_created
 
       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 @@ def assert_reminder_not_created
       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 @@ def assert_reminder_not_created
       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 @@ def assert_reminder_not_created
       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 @@ def assert_reminder_not_created
     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)