diff --git a/lib/pending_assigns_reminder.rb b/lib/pending_assigns_reminder.rb index c681d3d9..49db18c7 100644 --- a/lib/pending_assigns_reminder.rb +++ b/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 diff --git a/spec/lib/pending_assigns_reminder_spec.rb b/spec/lib/pending_assigns_reminder_spec.rb index 3c832933..8574591d 100644 --- a/spec/lib/pending_assigns_reminder_spec.rb +++ b/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)