From 44f15d4281e0cb16be858200790ed3674f85d378 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 20 Jan 2021 10:53:08 +1000 Subject: [PATCH] FIX: Do not email group user with user_private_message notification (#11754) There was an issue that occurred with this order of operations: * An IMAP topic was created by emailing a group * A second user was invited to the topic (not the OP and not the group) * A user with access to the group replies to the topic * The second user receives a user_private_message notification email because of their involvement in the topic * The second user replies to the email via email This new reply would then go and notify the other group PM users, except for those who emailed the group topic directly, which is handled via the group SMTP mailer. However because the new post already has an incoming email because it is parsed via the Email::Receiver via POP3 the group SMTP section of the post alerter is skipped, and the group's email address is not ignored for the user_private_message notification. This PR fixes it so the group is not ever sent an email via the PM notification. This is important because any new emails in the group's IMAP inbox will be picked up by the Imap::Sync code and created as a new topic which is not at all desirable. Also in this PR I split up the specs a bit more for group SMTP in the post alerter to make them easier to read and they each only test one thing. --- app/services/post_alerter.rb | 59 ++++++++++++-------- spec/services/post_alerter_spec.rb | 88 +++++++++++++++++------------- 2 files changed, 84 insertions(+), 63 deletions(-) diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index 8701af812c3..eb5f5bbf384 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -555,12 +555,6 @@ class PostAlerter def group_notifying_via_smtp(post) return nil if !SiteSetting.enable_smtp || post.post_type != Post.types[:regular] - # If the post already has an incoming email, it has been set in the - # Email::Receiver or via the GroupSmtpEmail job, and thus it was created - # via the IMAP/SMTP flow, so there is no need to notify those involved - # in the email chain again. - return nil if post.incoming_email.present? - post.topic.allowed_groups .where.not(smtp_server: nil) .where.not(smtp_port: nil) @@ -574,24 +568,14 @@ class PostAlerter warn_if_not_sidekiq - # users who interacted with the post by _directly_ emailing the group - emails_to_skip_send = nil - if group = group_notifying_via_smtp(post) - email_addresses = post.topic.incoming_email_addresses(group: group) + # Users who interacted with the post by _directly_ emailing the group + # via the group's email_username. This excludes people who replied via + # email to a user_private_message notification email. These people should + # not be emailed again by the user_private_message notifications below. + emails_to_skip_send = notify_group_direct_emailers(post) - if email_addresses.any? - Jobs.enqueue(:group_smtp_email, group_id: group.id, post_id: post.id, email: email_addresses) - end - - # add the group's email back into the array, because it is used for - # skip_send_email_to in the case of user private message notifications - # (we do not want the group to be sent any emails from here because it - # will make another email for IMAP to pick up in the group's mailbox) - emails_to_skip_send = email_addresses.dup - emails_to_skip_send << group.email_username - end - - # users that aren't part of any mentioned groups + # Users that aren't part of any mentioned groups and who did not email + # the group directly. users = directly_targeted_users(post).reject { |u| notified.include?(u) } DiscourseEvent.trigger(:before_create_notifications_for_users, users, post) users.each do |user| @@ -601,7 +585,7 @@ class PostAlerter end end - # users that are part of all mentioned groups + # Users that are part of all mentioned groups users = indirectly_targeted_users(post).reject { |u| notified.include?(u) } DiscourseEvent.trigger(:before_create_notifications_for_users, users, post) users.each do |user| @@ -615,6 +599,33 @@ class PostAlerter end end + def notify_group_direct_emailers(post) + email_addresses = [] + emails_to_skip_send = [] + group = group_notifying_via_smtp(post) + + return emails_to_skip_send if group.blank? + + # If the post already has an incoming email, it has been set in the + # Email::Receiver or via the GroupSmtpEmail job, and thus it was created + # via the IMAP/SMTP flow, so there is no need to notify those involved + # in the email chain again. + if post.incoming_email.blank? + email_addresses = post.topic.incoming_email_addresses(group: group) + if email_addresses.any? + Jobs.enqueue(:group_smtp_email, group_id: group.id, post_id: post.id, email: email_addresses) + end + end + + # Add the group's email into the array, because it is used for + # skip_send_email_to in the case of user private message notifications + # (we do not want the group to be sent any emails from here because it + # will make another email for IMAP to pick up in the group's mailbox) + emails_to_skip_send = email_addresses.dup if email_addresses.any? + emails_to_skip_send << group.email_username + emails_to_skip_send + end + def notify_post_users(post, notified, include_topic_watchers: true, include_category_watchers: true, include_tag_watchers: true, new_record: false) return unless post.topic diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index b1aaa58dd10..1ee1ac93b15 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -1261,67 +1261,77 @@ describe PostAlerter do end fab!(:topic) do - Fabricate(:private_message_topic, + Fabricate( + :private_message_topic, topic_allowed_groups: [ Fabricate.build(:topic_allowed_group, group: group) ] ) end - it "sends notifications for new posts in topic" do - PostAlerter.any_instance.expects(:create_notification).at_least_once - post = Fabricate( + def create_post_with_incoming + Fabricate( :post, topic: topic, incoming_email: - Fabricate( - :incoming_email, - topic: topic, - from_address: "foo@discourse.org", - to_addresses: group.email_username, - cc_addresses: "bar@discourse.org" - ) + Fabricate( + :incoming_email, + topic: topic, + from_address: "foo@discourse.org", + to_addresses: group.email_username, + cc_addresses: "bar@discourse.org" + ) ) - staged_group_user = Fabricate(:staged, email: "discourse@example.com") - Fabricate(:topic_user, user: staged_group_user, topic: post.topic) - topic.allowed_users << staged_group_user - topic.save + end + + it "does not send a group smtp email when the post already has an incoming email" do + post = create_post_with_incoming expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(0) + end + it "sends a group smtp email when the post does not have an incoming email" do + create_post_with_incoming post = Fabricate(:post, topic: topic) expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(1) email = ActionMailer::Base.deliveries.last expect(email.from).to include(group.email_username) expect(email.to).to contain_exactly("foo@discourse.org", "bar@discourse.org") expect(email.subject).to eq("Re: #{topic.title}") + end - post = Fabricate( - :post, - topic: topic, - incoming_email: - Fabricate( - :incoming_email, - topic: topic, - from_address: "bar@discourse.org", - to_addresses: group.email_username, - cc_addresses: "baz@discourse.org" - ) - ) - expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(0) - - # we never want the group to be notified by email about these posts - PostAlerter.any_instance.expects(:create_notification).with(kind_of(User), Notification.types[:private_message], kind_of(Post), skip_send_email_to: ["foo@discourse.org", "bar@discourse.org", "baz@discourse.org", "discourse@example.com"]).at_least_once - - post = Fabricate(:post, topic: topic.reload) - expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(1) - email = ActionMailer::Base.deliveries.last - expect(email.from).to eq([group.email_username]) - expect(email.to).to contain_exactly("foo@discourse.org", "bar@discourse.org", "baz@discourse.org") - expect(email.subject).to eq("Re: #{topic.title}") - + it "does not send group smtp emails for a whisper" do + create_post_with_incoming post = Fabricate(:post, topic: topic, post_type: Post.types[:whisper]) expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(0) end + + it "does not send a notification email to the group when the post does not have an incoming email" do + PostAlerter.any_instance.expects(:create_notification).with(kind_of(User), Notification.types[:private_message], kind_of(Post), skip_send_email_to: ["discourse@example.com"]).at_least_once + post = create_post_with_incoming + staged_group_user = Fabricate(:staged, email: "discourse@example.com") + Fabricate(:topic_user, user: staged_group_user, topic: post.topic) + topic.allowed_users << staged_group_user + topic.save + expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(0) + end + + it "skips sending a notification email to the group and all other incoming email addresses" do + + create_post_with_incoming + PostAlerter.any_instance.expects(:create_notification).with(kind_of(User), Notification.types[:private_message], kind_of(Post), skip_send_email_to: ["foo@discourse.org", "bar@discourse.org", "discourse@example.com"]).at_least_once + + post = Fabricate(:post, topic: topic.reload) + staged_group_user = Fabricate(:staged, email: "discourse@example.com") + Fabricate(:topic_user, user: staged_group_user, topic: post.topic) + topic.allowed_users << staged_group_user + topic.save + + expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(1) + email = ActionMailer::Base.deliveries.last + expect(email.from).to eq([group.email_username]) + expect(email.to).to contain_exactly("foo@discourse.org", "bar@discourse.org") + expect(email.subject).to eq("Re: #{topic.title}") + end end end