From e43a8af3bd592a88611905fc5fff15678f261518 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 26 Aug 2021 15:16:35 +1000 Subject: [PATCH] FIX: Do not send emails to mailing_list_mode subscribers for PMs (#14159) This bug was introduced by f66007ec83b62169b5c41016eecd40c72f27028f. In PostJobsEnqueuer we previously did not fire the after_post_create event and after_topic_create event for private message topics. This was changed in the above commit in order to publish message bus messages for topic tracking state updates. Unfortunately this caused the NotifyMailingListSubscribers job to be enqueued for all posts including private messages, and admins and the users involved in the PMs got emailed the contents of the PMs if they had mailing list mode enabled. Luckily the impact of this was mitigated by a Guardian#can_see? check for each mailing list mode user in the NotifyMailingListSubscribers job. We never want to notify mailing list mode subscribers for private messages so an early return has been added there, plus the logic in PostJobsEnqueuer has been fixed, and tests have been added to that class where there were none before. --- .../notify_mailing_list_subscribers.rb | 3 +- lib/post_jobs_enqueuer.rb | 11 +- .../notify_mailing_list_subscribers_spec.rb | 9 ++ spec/lib/post_jobs_enqueuer_spec.rb | 105 ++++++++++++++++++ 4 files changed, 123 insertions(+), 5 deletions(-) create mode 100644 spec/lib/post_jobs_enqueuer_spec.rb diff --git a/app/jobs/regular/notify_mailing_list_subscribers.rb b/app/jobs/regular/notify_mailing_list_subscribers.rb index 861dc27977c..c535296105f 100644 --- a/app/jobs/regular/notify_mailing_list_subscribers.rb +++ b/app/jobs/regular/notify_mailing_list_subscribers.rb @@ -28,7 +28,8 @@ module Jobs post_id = args[:post_id] post = post_id ? Post.with_deleted.find_by(id: post_id) : nil - return if !post || post.trashed? || post.user_deleted? || !post.topic || post.raw.blank? + return if !post || post.trashed? || post.user_deleted? || + !post.topic || post.raw.blank? || post.topic.private_message? users = User.activated.not_silenced.not_suspended.real diff --git a/lib/post_jobs_enqueuer.rb b/lib/post_jobs_enqueuer.rb index 80d49c59751..93a29f5663f 100644 --- a/lib/post_jobs_enqueuer.rb +++ b/lib/post_jobs_enqueuer.rb @@ -55,10 +55,13 @@ class PostJobsEnqueuer def after_post_create Jobs.enqueue(:post_update_topic_tracking_state, post_id: @post.id) - Jobs.enqueue_in(SiteSetting.email_time_window_mins.minutes, - :notify_mailing_list_subscribers, - post_id: @post.id, - ) + if !@topic.private_message? + Jobs.enqueue_in( + SiteSetting.email_time_window_mins.minutes, + :notify_mailing_list_subscribers, + post_id: @post.id, + ) + end end def after_topic_create diff --git a/spec/jobs/notify_mailing_list_subscribers_spec.rb b/spec/jobs/notify_mailing_list_subscribers_spec.rb index d0d2f758fd6..8b0421c4b82 100644 --- a/spec/jobs/notify_mailing_list_subscribers_spec.rb +++ b/spec/jobs/notify_mailing_list_subscribers_spec.rb @@ -80,6 +80,15 @@ describe Jobs::NotifyMailingListSubscribers do include_examples "no emails" end + context "with a private message" do + before do + post.topic.update!(archetype: Archetype.private_message, category: nil) + TopicAllowedUser.create(topic: post.topic, user: mailing_list_user) + post.topic.reload + end + include_examples "no emails" + end + context "with a valid post from another user" do context "to an inactive user" do diff --git a/spec/lib/post_jobs_enqueuer_spec.rb b/spec/lib/post_jobs_enqueuer_spec.rb new file mode 100644 index 00000000000..a5477cf4468 --- /dev/null +++ b/spec/lib/post_jobs_enqueuer_spec.rb @@ -0,0 +1,105 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe PostJobsEnqueuer do + let!(:post) { Fabricate(:post, topic: topic) } + let!(:topic) { Fabricate(:topic) } + let(:new_topic) { false } + let(:opts) { { post_alert_options: {} } } + + subject { described_class.new(post, topic, new_topic, opts) } + + context "for regular topics" do + it "enqueues the :post_alert job" do + expect_enqueued_with(job: :post_alert, args: { + post_id: post.id, + new_record: true, + options: opts[:post_alert_options] + }) do + subject.enqueue_jobs + end + end + + it "enqueues the :notify_mailing_list_subscribers job" do + expect_enqueued_with(job: :notify_mailing_list_subscribers, args: { post_id: post.id }) do + subject.enqueue_jobs + end + end + + it "enqueues the :post_update_topic_tracking_state job" do + expect_enqueued_with(job: :post_update_topic_tracking_state, args: { post_id: post.id }) do + subject.enqueue_jobs + end + end + + it "enqueues the :feature_topic_users job" do + expect_enqueued_with(job: :feature_topic_users, args: { topic_id: topic.id }) do + subject.enqueue_jobs + end + end + + context "for new topics" do + let(:new_topic) { true } + + it "calls the correct topic tracking state class to publish_new" do + TopicTrackingState.expects(:publish_new).with(topic) + PrivateMessageTopicTrackingState.expects(:publish_new).never + subject.enqueue_jobs + end + end + end + + context "for private messages" do + let!(:topic) { Fabricate(:private_message_topic) } + + it "does not enqueue the :notify_mailing_list_subscribers job" do + expect_not_enqueued_with(job: :notify_mailing_list_subscribers, args: { post_id: post.id }) do + subject.enqueue_jobs + end + end + + it "enqueues the :post_update_topic_tracking_state job" do + expect_enqueued_with(job: :post_update_topic_tracking_state, args: { post_id: post.id }) do + subject.enqueue_jobs + end + end + + it "enqueues the :feature_topic_users job" do + expect_enqueued_with(job: :feature_topic_users, args: { topic_id: topic.id }) do + subject.enqueue_jobs + end + end + + context "for new topics" do + let(:new_topic) { true } + + it "calls the correct topic tracking state class to publish_new" do + TopicTrackingState.expects(:publish_new).never + PrivateMessageTopicTrackingState.expects(:publish_new).with(topic) + subject.enqueue_jobs + end + end + + context "for a post > post_number 1" do + let!(:post) do + Fabricate(:post, topic: topic) + Fabricate(:post, topic: topic) + end + + context "when there is a topic embed" do + before do + SiteSetting.embed_unlisted = true + topic.update(visible: false) + Fabricate(:topic_embed, post: post, embed_url: "http://test.com") + end + + it "does not enqueue the :make_embedded_topic_visible job" do + expect_not_enqueued_with(job: :make_embedded_topic_visible, args: { topic_id: topic.id }) do + subject.enqueue_jobs + end + end + end + end + end +end