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

@@ -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)