FEATURE: Use group SMTP job and mailer instead of UserNotifications change (#13489)

This PR backtracks a fair bit on this one https://github.com/discourse/discourse/pull/13220/files.

Instead of sending the group SMTP email for each user via `UserNotifications`, we are changing to send only one email with the existing `Jobs::GroupSmtpEmail` job and `GroupSmtpMailer`. We are changing this job and mailer along with `PostAlerter` to make the first topic allowed user the `to_address` for the email and any other `topic_allowed_users` to be the CC address on the email. This is to cut down on emails sent via SMTP, which is subject to daily limits from providers such as Gmail. We log these details in the `EmailLog` table now.

In addition to this, we have changed `PostAlerter` to no longer rely on incoming email email addresses for sending the `GroupSmtpEmail` job. This was unreliable as a user's email could have changed in the meantime. Also it was a little overcomplicated to use the incoming email records -- it is far simpler to reason about to just use topic allowed users.

This also adds a fix to include cc_addresses in the EmailLog.addressed_to_user scope.

app/jobs/regular/group_smtp_email.rb

@@ -10,6 +10,7 @@ module Jobs
       group = Group.find_by(id: args[:group_id])
       post = Post.find_by(id: args[:post_id])
       email = args[:email]
+      cc_addresses = args[:cc_emails]
 
       # There is a rare race condition causing the Imap::Sync class to create
       # an incoming email and associated post/topic, which then kicks off
@@ -27,11 +28,17 @@ module Jobs
       ImapSyncLog.debug("Sending SMTP email for post #{post.id} in topic #{post.topic_id} to #{email}.", group)
 
       recipient_user = ::UserEmail.find_by(email: email, primary: true)&.user
-      message = GroupSmtpMailer.send_mail(group, email, post)
+      message = GroupSmtpMailer.send_mail(group, email, post, cc_addresses)
+
+      # The EmailLog record created by the sender will have the raw email
+      # stored, the group smtp ID, and any cc addresses recorded for later
+      # cross referencing.
       Email::Sender.new(message, :group_smtp, recipient_user).send
 
       # Create an incoming email record to avoid importing again from IMAP
-      # server.
+      # server. While this may not be technically required if IMAP is not
+      # currently enabled for the group, it will help a lot with the initial
+      # sync if it is turned on at a later date.
       IncomingEmail.create!(
         user_id: post.user_id,
         topic_id: post.topic_id,

app/mailers/group_smtp_mailer.rb

@@ -5,13 +5,10 @@ require_dependency 'email/message_builder'
 class GroupSmtpMailer < ActionMailer::Base
   include Email::BuildEmailHelper
 
-  def send_mail(from_group, to_address, post)
+  def send_mail(from_group, to_address, post, cc_addresses = nil)
     raise 'SMTP is disabled' if !SiteSetting.enable_smtp
 
-    incoming_email = IncomingEmail.joins(:post)
-      .where('imap_uid IS NOT NULL')
-      .where(topic_id: post.topic_id, posts: { post_number: 1 })
-      .limit(1).first
+    op_incoming_email = post.topic.first_post.incoming_email
 
     context_posts = Post
       .where(topic_id: post.topic_id)
@@ -38,29 +35,32 @@ class GroupSmtpMailer < ActionMailer::Base
       user_name = post.user.name unless post.user.name.blank?
     end
 
-    build_email(to_address,
+    group_name = from_group.full_name.presence || from_group.name
+    build_email(
+      to_address,
       message: post.raw,
       url: post.url(without_slug: SiteSetting.private_email?),
       post_id: post.id,
       topic_id: post.topic_id,
       context: context(context_posts),
       username: post.user.username,
-      group_name: from_group.name,
+      group_name: group_name,
       allow_reply_by_email: true,
       only_reply_by_email: true,
-      use_from_address_for_reply_to: SiteSetting.enable_imap && from_group.imap_enabled?,
+      use_from_address_for_reply_to: SiteSetting.enable_smtp && from_group.smtp_enabled?,
       private_reply: post.topic.private_message?,
       participants: participants(post),
       include_respond_instructions: true,
       template: 'user_notifications.user_posted_pm',
       use_topic_title_subject: true,
-      topic_title: incoming_email&.subject || post.topic.title,
+      topic_title: op_incoming_email&.subject || post.topic.title,
       add_re_to_subject: true,
       locale: SiteSetting.default_locale,
       delivery_method_options: delivery_options,
       from: from_group.email_username,
-      from_alias: I18n.t('email_from', user_name: user_name, site_name: Email.site_title),
-      html_override: html_override(post, context_posts: context_posts)
+      from_alias: I18n.t('email_from', user_name: group_name, site_name: Email.site_title),
+      html_override: html_override(post, context_posts: context_posts),
+      cc: cc_addresses
     )
   end
 

app/mailers/user_notifications.rb

@@ -327,7 +327,6 @@ class UserNotifications < ActionMailer::Base
     opts[:show_category_in_subject] = false
     opts[:show_tags_in_subject] = false
     opts[:show_group_in_subject] = true if SiteSetting.group_in_subject
-    opts[:use_group_smtp_if_configured] = true
 
     # We use the 'user_posted' event when you are emailed a post in a PM.
     opts[:notification_type] = 'posted'
@@ -461,7 +460,6 @@ class UserNotifications < ActionMailer::Base
       notification_type: notification_type,
       use_invite_template: opts[:use_invite_template],
       use_topic_title_subject: use_topic_title_subject,
-      use_group_smtp_if_configured: opts[:use_group_smtp_if_configured],
       user: user
     }
 
@@ -487,13 +485,6 @@ class UserNotifications < ActionMailer::Base
     group_name = opts[:group_name]
     locale = user_locale(user)
 
-    # this gets set in MessageBuilder if it is nil here, we just want to be
-    # able to override it if the group has SMTP enabled
-    from_address = nil
-    delivery_method_options = nil
-    use_from_address_for_reply_to = false
-    using_group_smtp = false
-
     template = +"user_notifications.user_#{notification_type}"
     if post.topic.private_message?
       template << "_pm"
@@ -531,41 +522,6 @@ class UserNotifications < ActionMailer::Base
 
     group = post.topic.allowed_groups&.first
 
-    # If the group has IMAP enabled, then this will be handled by
-    # the Jobs::GroupSmtpEmail which is enqueued from the PostAlerter
-    #
-    # use_group_smtp_if_configured is used to ensure that no notifications
-    # expect for specific ones that we bless (such as user_private_message)
-    # accidentally get sent with the group SMTP settings.
-    if group.present? &&
-       group.smtp_enabled &&
-       !group.imap_enabled &&
-       SiteSetting.enable_smtp &&
-       opts[:use_group_smtp_if_configured]
-
-      port, enable_tls, enable_starttls_auto = EmailSettingsValidator.provider_specific_ssl_overrides(
-        group.smtp_server, group.smtp_port, group.smtp_ssl, group.smtp_ssl
-      )
-
-      delivery_method_options = {
-        address: group.smtp_server,
-        port: port,
-        domain: group.email_username_domain,
-        user_name: group.email_username,
-        password: group.email_password,
-        authentication: GlobalSetting.smtp_authentication,
-        enable_starttls_auto: enable_starttls_auto
-      }
-
-      # We want from to be the same as the group's email_username, so if
-      # someone emails support@discourse.org they will get a reply from
-      # support@discourse.org and be able to email the SMTP email, which
-      # will forward the email back into Discourse and process/link it correctly.
-      use_from_address_for_reply_to = true
-      from_address = group.email_username
-      using_group_smtp = true
-    end
-
     if post.topic.private_message?
       subject_pm =
         if opts[:show_group_in_subject] && group.present?
@@ -692,7 +648,7 @@ class UserNotifications < ActionMailer::Base
       context: context,
       username: username,
       group_name: group_name,
-      add_unsubscribe_link: !user.staged && !using_group_smtp,
+      add_unsubscribe_link: !user.staged,
       mailing_list_mode: user.user_option.mailing_list_mode,
       unsubscribe_url: post.unsubscribe_url(user),
       allow_reply_by_email: allow_reply_by_email,
@@ -710,10 +666,7 @@ class UserNotifications < ActionMailer::Base
       site_description: SiteSetting.site_description,
       site_title: SiteSetting.title,
       site_title_url_encoded: UrlHelper.encode_component(SiteSetting.title),
-      locale: locale,
-      delivery_method_options: delivery_method_options,
-      use_from_address_for_reply_to: use_from_address_for_reply_to,
-      from: from_address
+      locale: locale
     }
 
     unless translation_override_exists

app/models/email_log.rb

@@ -28,7 +28,8 @@ class EmailLog < ActiveRecord::Base
         SELECT 1
         FROM user_emails
         WHERE user_emails.user_id = :user_id AND
-        email_logs.to_address = user_emails.email
+        (email_logs.to_address = user_emails.email OR
+         email_logs.cc_addresses ILIKE '%' || user_emails.email || '%')
       )
     SQL
   end

app/services/post_alerter.rb

@@ -582,20 +582,16 @@ class PostAlerter
 
     warn_if_not_sidekiq
 
-    # Users who interacted with the post by _directly_ emailing the group
-    # via the group's email_username which is configured via SMTP/IMAP.
-    #
-    # This excludes people who replied via email to a user_private_message
-    # notification email which will have a PostReplyKey. These people should
-    # not be emailed again by the user_private_message notifications below.
-    #
-    # This also excludes people who emailed the group by one of its incoming_email
-    # addresses, e.g. somegroup+support@discoursemail.com, which is part of the
-    # normal group email flow and has nothing to do with SMTP/IMAP.
-    emails_to_skip_send = notify_group_direct_emailers(post)
+    # To simplify things and to avoid IMAP double sync issues, and to cut down
+    # on emails sent via SMTP, any topic_allowed_users (except those who are
+    # not_allowed?) for a group that has SMTP enabled will have their notification
+    # email combined into one and sent via a single group SMTP email with CC addresses.
+    emails_to_skip_send = email_using_group_smtp_if_configured(post)
 
-    # Users that aren't part of any mentioned groups and who did not email
-    # the group directly at the group's email_username.
+    # We create notifications for all directly_targeted_users and email those
+    # who do _not_ have their email addresses in the emails_to_skip_send array
+    # (which will include all topic allowed users' email addresses if group SMTP
+    # is enabled).
     users = directly_targeted_users(post).reject { |u| notified.include?(u) }
     DiscourseEvent.trigger(:before_create_notifications_for_users, users, post)
     users.each do |user|
@@ -605,7 +601,8 @@ class PostAlerter
       end
     end
 
-    # Users that are part of all mentioned groups.
+    # Users that are part of all mentioned groups. Emails sent by this notification
+    # flow will not be sent via group SMTP if it is enabled.
     users = indirectly_targeted_users(post).reject { |u| notified.include?(u) }
     DiscourseEvent.trigger(:before_create_notifications_for_users, users, post)
     users.each do |user|
@@ -620,27 +617,54 @@ class PostAlerter
   end
 
   def group_notifying_via_smtp(post)
-    return nil if !SiteSetting.enable_smtp || !SiteSetting.enable_imap || post.post_type != Post.types[:regular]
-    post.topic.allowed_groups.where(smtp_enabled: true, imap_enabled: true).first
+    return nil if !SiteSetting.enable_smtp || post.post_type != Post.types[:regular]
+    post.topic.allowed_groups.where(smtp_enabled: true).first
   end
 
-  def notify_group_direct_emailers(post)
-    email_addresses = []
+  def email_using_group_smtp_if_configured(post)
     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
+    to_address = nil
+    cc_addresses = []
+
+    # We need to use topic_allowed_users here instead of directly_targeted_users
+    # because we want to make sure the to_address goes to the OP of the topic.
+    topic_allowed_users_by_age = post.topic.topic_allowed_users.includes(:user).order(:created_at).reject do |tau|
+      not_allowed?(tau.user, post)
     end
+    return emails_to_skip_send if topic_allowed_users_by_age.empty?
+
+    # This should usually be the OP of the topic, unless they are the one
+    # replying by email (they are excluded by not_allowed? then)
+    to_address = topic_allowed_users_by_age.first.user.email
+    cc_addresses = topic_allowed_users_by_age[1..-1].map { |tau| tau.user.email }
+    email_addresses = [to_address, cc_addresses].flatten
+
+    # If any of these email addresses were cc address on the
+    # incoming email for the target post, do not send them emails (they
+    # already have been notified by the CC on the email)
+    if post.incoming_email.present?
+      cc_addresses = cc_addresses - post.incoming_email.cc_addresses_split
+
+      # If the to address is one of the recently added CC addresses, then we
+      # need to bail early, because otherwise we are sending a notification
+      # email to the user who was just added by CC. In this case the OP probably
+      # replied and CC'd some people, and they are the only other topic users.
+      return if post.incoming_email.cc_addresses_split.include?(to_address)
+    end
+
+    # Send a single email using group SMTP settings to cut down on the
+    # number of emails sent via SMTP, also to replicate how support systems
+    # and group inboxes generally work in other systems.
+    Jobs.enqueue(
+      :group_smtp_email,
+      group_id: group.id,
+      post_id: post.id,
+      email: to_address,
+      cc_emails: cc_addresses
+    )
 
     # Add the group's email_username into the array, because it is used for
     # skip_send_email_to in the case of user private message notifications
@@ -648,7 +672,7 @@ class PostAlerter
     # 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
+    emails_to_skip_send.uniq
   end
 
   def notify_post_users(post, notified, group_ids: nil, include_topic_watchers: true, include_category_watchers: true, include_tag_watchers: true, new_record: false)

lib/email/message_builder.rb

@@ -140,7 +140,8 @@ module Email
         subject: subject,
         body: body,
         charset: 'UTF-8',
-        from: from_value
+        from: from_value,
+        cc: @opts[:cc]
       }
 
       args[:delivery_method_options] = @opts[:delivery_method_options] if @opts[:delivery_method_options]
@@ -161,11 +162,24 @@ module Email
       # please, don't send us automatic responses...
       result['X-Auto-Response-Suppress'] = 'All'
 
-      if allow_reply_by_email? && !@opts[:use_from_address_for_reply_to]
-        result[ALLOW_REPLY_BY_EMAIL_HEADER] = true
-        result['Reply-To'] = reply_by_email_address
-      else
+      if !allow_reply_by_email?
+        # This will end up being the notification_email, which is a
+        # noreply address.
         result['Reply-To'] = from_value
+      else
+
+        # The only reason we use from address for reply to is for group
+        # SMTP emails, where the person will be replying to the group's
+        # email_username.
+        if !@opts[:use_from_address_for_reply_to]
+          result[ALLOW_REPLY_BY_EMAIL_HEADER] = true
+          result['Reply-To'] = reply_by_email_address
+        else
+          # No point in adding a reply-to header if it is going to be identical
+          # to the from address/alias. If the from option is not present, then
+          # the default reply-to address is used.
+          result['Reply-To'] = from_value if from_value != alias_email(@opts[:from])
+        end
       end
 
       result.merge(MessageBuilder.custom_headers(SiteSetting.email_custom_headers))

lib/email/sender.rb

@@ -429,6 +429,8 @@ module Email
     end
 
     def set_reply_key(post_id, user_id)
+      # ALLOW_REPLY_BY_EMAIL_HEADER is only added if we are _not_ sending
+      # via group SMTP and if reply by email site settings are configured
       return if !user_id || !post_id || !header_value(Email::MessageBuilder::ALLOW_REPLY_BY_EMAIL_HEADER).present?
 
       # use safe variant here cause we tend to see concurrency issue

spec/components/email/message_builder_spec.rb

@@ -152,12 +152,15 @@ describe Email::MessageBuilder do
 
   context "header args" do
 
+    let(:additional_opts) { {} }
     let(:message_with_header_args) do
       Email::MessageBuilder.new(
         to_address,
+        {
         body: 'hello world',
         topic_id: 1234,
         post_id: 4567,
+        }.merge(additional_opts)
       )
     end
 
@@ -169,6 +172,42 @@ describe Email::MessageBuilder do
       expect(message_with_header_args.header_args['X-Discourse-Topic-Id']).to eq('1234')
     end
 
+    it "uses the default reply-to header" do
+      expect(message_with_header_args.header_args['Reply-To']).to eq("\"Discourse\" <#{SiteSetting.notification_email}>")
+    end
+
+    context "when allow_reply_by_email is enabled " do
+      let(:additional_opts) { { allow_reply_by_email: true } }
+
+      it "uses the reply by email address if that is enabled" do
+        SiteSetting.manual_polling_enabled = true
+        SiteSetting.reply_by_email_address = "test+%{reply_key}@test.com"
+        SiteSetting.reply_by_email_enabled = true
+        expect(message_with_header_args.header_args['Reply-To']).to eq("\"Discourse\" <test+%{reply_key}@test.com>")
+      end
+    end
+
+    context "when allow_reply_by_email is enabled and use_from_address_for_reply_to is enabled but no from address is specified" do
+      let(:additional_opts) { { allow_reply_by_email: true, use_from_address_for_reply_to: true } }
+
+      it "uses the notification_email address, the default reply-to header" do
+        SiteSetting.manual_polling_enabled = true
+        SiteSetting.reply_by_email_address = "test+%{reply_key}@test.com"
+        SiteSetting.reply_by_email_enabled = true
+        expect(message_with_header_args.header_args['Reply-To']).to eq("\"Discourse\" <#{SiteSetting.notification_email}>")
+      end
+    end
+
+    context "when allow_reply_by_email is enabled and use_from_address_for_reply_to is enabled and from is specified" do
+      let(:additional_opts) { { allow_reply_by_email: true, use_from_address_for_reply_to: true, from: "team@test.com" } }
+
+      it "removes the reply-to header because it is identical to the from header" do
+        SiteSetting.manual_polling_enabled = true
+        SiteSetting.reply_by_email_address = "test+%{reply_key}@test.com"
+        SiteSetting.reply_by_email_enabled = true
+        expect(message_with_header_args.header_args['Reply-To']).to eq(nil)
+      end
+    end
   end
 
   context "unsubscribe link" do
@@ -337,7 +376,5 @@ describe Email::MessageBuilder do
       SiteSetting.stubs(:email_site_title).returns("::>>>Best \"Forum\", EU: Award Winning<<<")
       expect(build_args[:from]).to eq("\"Best Forum EU Award Winning\" <#{SiteSetting.notification_email}>")
     end
-
   end
-
 end

spec/components/email/sender_spec.rb

@@ -343,11 +343,10 @@ describe Email::Sender do
         let(:reply) { Fabricate(:post, topic: post.topic, reply_to_user: post.user, reply_to_post_number: post.post_number) }
         let(:notification) { Fabricate(:posted_notification, user: post.user, post: reply) }
         let(:message) do
-          UserNotifications.user_private_message(
-            post.user,
-            post: reply,
-            notification_type: notification.notification_type,
-            notification_data_hash: notification.data_hash
+          GroupSmtpMailer.send_mail(
+            group,
+            post.user.email,
+            post
           )
         end
         let(:group) { Fabricate(:smtp_group) }

spec/jobs/regular/group_smtp_email_spec.rb

@@ -4,63 +4,91 @@ require 'rails_helper'
 
 RSpec.describe Jobs::GroupSmtpEmail do
   fab!(:post) do
-    topic = Fabricate(:topic)
+    topic = Fabricate(:topic, title: "Help I need support")
     Fabricate(:post, topic: topic)
     Fabricate(:post, topic: topic)
   end
-  fab!(:group) { Fabricate(:imap_group) }
+  fab!(:group) { Fabricate(:smtp_group, name: "support-group", full_name: "Support Group") }
   fab!(:recipient_user) { Fabricate(:user, email: "test@test.com") }
   let(:post_id) { post.id }
   let(:args) do
     {
       group_id: group.id,
       post_id: post_id,
-      email: "test@test.com"
+      email: "test@test.com",
+      cc_emails: ["otherguy@test.com", "cormac@lit.com"]
     }
   end
 
   before do
-    SiteSetting.reply_by_email_address = "test+%{reply_key}@incoming.com"
-    SiteSetting.manual_polling_enabled = true
-    SiteSetting.reply_by_email_enabled = true
     SiteSetting.enable_smtp = true
+    SiteSetting.manual_polling_enabled = true
+    SiteSetting.reply_by_email_address = "test+%{reply_key}@test.com"
+    SiteSetting.reply_by_email_enabled = true
   end
 
   it "sends an email using the GroupSmtpMailer and Email::Sender" do
     message = Mail::Message.new(body: "hello", to: "myemail@example.invalid")
-    GroupSmtpMailer.expects(:send_mail).with(group, "test@test.com", post).returns(message)
-    Email::Sender.expects(:new).with(message, :group_smtp, recipient_user).returns(stub(send: nil))
+    GroupSmtpMailer.expects(:send_mail).with(group, "test@test.com", post, ["otherguy@test.com", "cormac@lit.com"]).returns(message)
     subject.execute(args)
   end
 
-  it "creates an IncomingEmail record to avoid double processing via IMAP" do
+  it "creates an EmailLog record with the correct details" do
     subject.execute(args)
-    incoming = IncomingEmail.find_by(post_id: post.id, user_id: post.user_id, topic_id: post.topic_id)
-    expect(incoming).not_to eq(nil)
-    expect(incoming.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost")
-    expect(incoming.created_via).to eq(IncomingEmail.created_via_types[:group_smtp])
+    email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id)
+    expect(email_log).not_to eq(nil)
+    expect(email_log.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost")
   end
 
-  it "creates a PostReplyKey and correctly uses it for the email reply_key substitution" do
+  it "creates an IncomingEmail record with the correct details to avoid double processing IMAP" do
     subject.execute(args)
-    incoming = IncomingEmail.find_by(post_id: post.id, user_id: post.user_id, topic_id: post.topic_id)
+    incoming_email = IncomingEmail.find_by(post_id: post.id, topic_id: post.topic_id, user_id: post.user.id)
+    expect(incoming_email).not_to eq(nil)
+    expect(incoming_email.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost")
+    expect(incoming_email.created_via).to eq(IncomingEmail.created_via_types[:group_smtp])
+    expect(incoming_email.to_addresses).to eq("test@test.com")
+    expect(incoming_email.cc_addresses).to eq("otherguy@test.com;cormac@lit.com")
+    expect(incoming_email.subject).to eq("Re: Help I need support")
+  end
+
+  it "does not create a post reply key, it always replies to the group email_username" do
+    subject.execute(args)
+    email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id)
     post_reply_key = PostReplyKey.where(user_id: recipient_user, post_id: post.id).first
-    expect(post_reply_key).not_to eq(nil)
-    expect(incoming.raw).to include("Reply-To: Discourse <test+#{post_reply_key.reply_key}@incoming.com>")
+    expect(post_reply_key).to eq(nil)
+    expect(email_log.raw).not_to include("Reply-To: Support Group via Discourse <#{group.email_username}")
+    expect(email_log.raw).to include("From: Support Group via Discourse <#{group.email_username}")
   end
 
-  it "has the from_address and the to_addresses and subject filled in correctly" do
+  it "falls back to the group name if full name is blank" do
+    group.update(full_name: "")
     subject.execute(args)
-    incoming = IncomingEmail.find_by(post_id: post.id, user_id: post.user_id, topic_id: post.topic_id)
-    expect(incoming.to_addresses).to eq("test@test.com")
-    expect(incoming.subject).to include("Re: This is a test topic")
-    expect(incoming.from_address).to eq("discourseteam@ponyexpress.com")
+    email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id)
+    expect(email_log.raw).to include("From: support-group via Discourse <#{group.email_username}")
+  end
+
+  it "has the group_smtp_id and the to_address filled in correctly" do
+    subject.execute(args)
+    email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id)
+    expect(email_log.to_address).to eq("test@test.com")
+    expect(email_log.smtp_group_id).to eq(group.id)
+  end
+
+  context "when there are cc_addresses" do
+    let!(:cormac_user) { Fabricate(:user, email: "cormac@lit.com") }
+
+    it "has the cc_addresses and cc_user_ids filled in correctly" do
+      subject.execute(args)
+      email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id)
+      expect(email_log.cc_addresses).to eq("otherguy@test.com;cormac@lit.com")
+      expect(email_log.cc_user_ids).to eq([cormac_user.id])
+    end
   end
 
   context "when the post in the argument is the OP" do
     let(:post_id) { post.topic.posts.first.id }
     it "aborts and does not send a group SMTP email; the OP is the one that sent the email in the first place" do
-      expect { subject.execute(args) }.not_to(change { IncomingEmail.count })
+      expect { subject.execute(args) }.not_to(change { EmailLog.count })
     end
   end
 end

spec/mailers/group_smtp_mailer_spec.rb

@@ -3,7 +3,7 @@ require 'rails_helper'
 require 'email/receiver'
 
 describe GroupSmtpMailer do
-  let(:group) {
+  let(:group) do
     Fabricate(:group,
               name: 'Testers',
               title: 'Tester',
@@ -19,15 +19,15 @@ describe GroupSmtpMailer do
               email_username: 'bugs@gmail.com',
               email_password: 'super$secret$password'
              )
-  }
+  end
 
-  let(:user) {
+  let(:user) do
     user = Fabricate(:user)
     group.add_owner(user)
     user
-  }
+  end
 
-  let(:email) {
+  let(:email) do
     <<~EOF
     Delivered-To: bugs@gmail.com
     MIME-Version: 1.0
@@ -42,17 +42,18 @@ describe GroupSmtpMailer do
 
     How are you doing?
     EOF
-  }
+  end
 
-  let(:receiver) {
-    receiver = Email::Receiver.new(email,
-                                   destinations: [group],
-                                   uid_validity: 1,
-                                   uid: 10000
-                                  )
-  receiver.process!
-  receiver
-  }
+  let(:receiver) do
+    receiver = Email::Receiver.new(
+      email,
+      destinations: [group],
+      uid_validity: 1,
+      uid: 10000
+    )
+    receiver.process!
+    receiver
+  end
 
   let(:raw) { 'hello, how are you doing?' }
 
@@ -60,6 +61,9 @@ describe GroupSmtpMailer do
     SiteSetting.enable_smtp = true
     SiteSetting.enable_imap = true
     Jobs.run_immediately!
+    SiteSetting.manual_polling_enabled = true
+    SiteSetting.reply_by_email_address = "test+%{reply_key}@test.com"
+    SiteSetting.reply_by_email_enabled = true
   end
 
   it 'sends an email as reply' do
@@ -72,11 +76,21 @@ describe GroupSmtpMailer do
 
     sent_mail = ActionMailer::Base.deliveries[0]
     expect(sent_mail.to).to contain_exactly('john@doe.com')
-    expect(sent_mail.reply_to).to contain_exactly('bugs@gmail.com')
+    expect(sent_mail.reply_to).to eq(nil)
     expect(sent_mail.subject).to eq('Re: Hello from John')
     expect(sent_mail.to_s).to include(raw)
   end
 
+  it "uses the OP incoming email subject for the subject over topic title" do
+    receiver.incoming_email.topic.update(title: "blah")
+    post = PostCreator.create(user,
+                              topic_id: receiver.incoming_email.topic.id,
+                              raw: raw
+                             )
+    sent_mail = ActionMailer::Base.deliveries[0]
+    expect(sent_mail.subject).to eq('Re: Hello from John')
+  end
+
   context "when the site has a reply by email address configured" do
     before do
       SiteSetting.manual_polling_enabled = true
@@ -84,7 +98,7 @@ describe GroupSmtpMailer do
       SiteSetting.reply_by_email_enabled = true
     end
 
-    it 'uses the correct IMAP/SMTP reply to address' do
+    it 'uses the correct IMAP/SMTP reply to address and does not create a post reply key' do
       post = PostCreator.create(user,
                                 topic_id: receiver.incoming_email.topic.id,
                                 raw: raw
@@ -92,8 +106,11 @@ describe GroupSmtpMailer do
 
       expect(ActionMailer::Base.deliveries.size).to eq(1)
 
+      expect(PostReplyKey.find_by(user_id: user.id, post_id: post.id)).to eq(nil)
+
       sent_mail = ActionMailer::Base.deliveries[0]
-      expect(sent_mail.reply_to).to contain_exactly('bugs@gmail.com')
+      expect(sent_mail.reply_to).to eq(nil)
+      expect(sent_mail.from).to contain_exactly('bugs@gmail.com')
     end
 
     context "when IMAP is disabled for the group" do
@@ -101,6 +118,21 @@ describe GroupSmtpMailer do
         group.update(imap_enabled: false)
       end
 
+      it "does send the email" do
+        post = PostCreator.create(user,
+                                  topic_id: receiver.incoming_email.topic.id,
+                                  raw: raw
+                                 )
+
+        expect(ActionMailer::Base.deliveries.size).to eq(1)
+      end
+    end
+
+    context "when SMTP is disabled for the group" do
+      before do
+        group.update(smtp_enabled: false)
+      end
+
       it "does not send the email" do
         post = PostCreator.create(user,
                                   topic_id: receiver.incoming_email.topic.id,

spec/mailers/user_notifications_spec.rb

@@ -611,84 +611,6 @@ describe UserNotifications do
       expect(mail.body).to include("[group1 (2)](http://test.localhost/groups/group1), [group2 (1)](http://test.localhost/groups/group2), [one](http://test.localhost/u/one), [two](http://test.localhost/u/two)")
     end
 
-    context "when group smtp is configured and SiteSetting.enable_smtp" do
-      let!(:group1) do
-        Fabricate(
-          :group,
-          name: "group1",
-          smtp_enabled: true,
-          smtp_port: 587,
-          smtp_ssl: true,
-          smtp_server: "smtp.test.com",
-          email_username: "user@test.com",
-          email_password: "password"
-        )
-      end
-
-      before do
-        SiteSetting.enable_smtp = true
-        topic.allowed_groups = [group1]
-      end
-
-      it "uses the from address, which is the group's email_username, for reply-to" do
-        mail = UserNotifications.user_private_message(
-          user,
-          post: response,
-          notification_type: notification.notification_type,
-          notification_data_hash: notification.data_hash
-        )
-
-        expect(mail.from).to eq([group1.email_username])
-        expect(mail.reply_to).to eq([group1.email_username])
-      end
-
-      it "uses the SMTP settings from the group for delivery" do
-        mail = UserNotifications.user_private_message(
-          user,
-          post: response,
-          notification_type: notification.notification_type,
-          notification_data_hash: notification.data_hash
-        )
-
-        delivery_method = mail.delivery_method.settings
-        expect(delivery_method[:port]).to eq(group1.smtp_port)
-        expect(delivery_method[:address]).to eq(group1.smtp_server)
-        expect(delivery_method[:domain]).to eq("test.com")
-        expect(delivery_method[:password]).to eq("password")
-        expect(delivery_method[:user_name]).to eq("user@test.com")
-      end
-
-      context "when imap is configured for the group" do
-        before do
-          group1.update(
-            imap_server: "imap.test.com",
-            imap_port: 993,
-            imap_ssl: true,
-            imap_enabled: true,
-            imap_mailbox_name: "All Mail"
-          )
-        end
-
-        it "does not use group SMTP settings for delivery, this is handled by Jobs::GroupSmtpEmail" do
-          mail = UserNotifications.user_private_message(
-            user,
-            post: response,
-            notification_type: notification.notification_type,
-            notification_data_hash: notification.data_hash
-          )
-
-          expect(mail.from).to eq([SiteSetting.notification_email])
-          expect(mail.reply_to).to eq([SiteSetting.notification_email])
-          delivery_method = mail.delivery_method.settings
-          expect(delivery_method[:port]).not_to eq(group1.smtp_port)
-          expect(delivery_method[:address]).not_to eq(group1.smtp_server)
-          expect(delivery_method[:domain]).not_to eq("test.com")
-          expect(delivery_method[:password]).not_to eq("password")
-          expect(delivery_method[:user_name]).not_to eq("user@test.com")
-        end
-      end
-    end
-
     context "when SiteSetting.group_name_in_subject is true" do
       before do
         SiteSetting.group_in_subject = true

spec/models/email_log_spec.rb

@@ -138,4 +138,27 @@ describe EmailLog do
       end
     end
   end
+
+  describe ".addressed_to_user scope" do
+    let(:user) { Fabricate(:user, email: "test@test.com") }
+    before do
+      Fabricate(:email_log, to_address: "john@smith.com")
+      Fabricate(:email_log, cc_addresses: "jane@jones.com;elle@someplace.org")
+      user.reload
+    end
+
+    it "returns email logs where the to address matches" do
+      user.user_emails.first.update!(email: "john@smith.com")
+      expect(EmailLog.addressed_to_user(user).count).to eq(1)
+    end
+
+    it "returns email logs where a cc address matches" do
+      user.user_emails.first.update!(email: "elle@someplace.org")
+      expect(EmailLog.addressed_to_user(user).count).to eq(1)
+    end
+
+    it "returns nothing if no emails match" do
+      expect(EmailLog.addressed_to_user(user).count).to eq(0)
+    end
+  end
 end

spec/services/post_alerter_spec.rb

@@ -1295,7 +1295,7 @@ describe PostAlerter do
   context "SMTP (group_smtp_email)" do
     before do
       SiteSetting.enable_smtp = true
-      SiteSetting.enable_imap = true
+      SiteSetting.email_in = true
       Jobs.run_immediately!
     end
 
@@ -1315,99 +1315,257 @@ describe PostAlerter do
       )
     end
 
-    fab!(:topic) do
-      Fabricate(
-        :private_message_topic,
-        topic_allowed_groups: [
-          Fabricate.build(:topic_allowed_group, group: group)
-        ]
-      )
-    end
-
     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"
-        )
-      )
+      raw_mail = <<~MAIL
+      From: Foo <foo@discourse.org>
+      To: discourse@example.com
+      Cc: bar@discourse.org, jim@othersite.com
+      Subject: Full email group username flow
+      Date: Fri, 15 Jan 2021 00:12:43 +0100
+      Message-ID: <u4w8c9r4y984yh98r3h69873@example.com.mail>
+      Mime-Version: 1.0
+      Content-Type: text/plain
+      Content-Transfer-Encoding: 7bit
+
+      This is the first email.
+      MAIL
+
+      Email::Receiver.new(raw_mail, {}).process!
     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
+    it "sends a group smtp email because SMTP is enabled for the site and the group" do
+      incoming_email_post = create_post_with_incoming
+      topic = incoming_email_post.topic
       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.to).to contain_exactly(topic.reload.topic_allowed_users.order(:created_at).first.user.email)
+      expect(email.cc).to match_array(["bar@discourse.org", "jim@othersite.com"])
       expect(email.subject).to eq("Re: #{topic.title}")
     end
 
-    it "does not send a group smtp email if imap is not enabled for the group" do
-      group.update!(imap_enabled: false)
-      create_post_with_incoming
-      post = Fabricate(:post, topic: topic)
-      expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(0)
-    end
-
-    it "does not send a group smtp email if SiteSetting.enable_imap is false" do
-      SiteSetting.enable_imap = false
-      create_post_with_incoming
+    it "does not send a group smtp email if smtp is not enabled for the group" do
+      group.update!(smtp_enabled: false)
+      incoming_email_post = create_post_with_incoming
+      topic = incoming_email_post.topic
       post = Fabricate(:post, topic: topic)
       expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(0)
     end
 
     it "does not send a group smtp email if SiteSetting.enable_smtp is false" do
       SiteSetting.enable_smtp = false
-      create_post_with_incoming
+      incoming_email_post = create_post_with_incoming
+      topic = incoming_email_post.topic
       post = Fabricate(:post, topic: topic)
       expect { PostAlerter.new.after_save_post(post, true) }.to change { ActionMailer::Base.deliveries.size }.by(0)
     end
 
     it "does not send group smtp emails for a whisper" do
-      create_post_with_incoming
+      incoming_email_post = create_post_with_incoming
+      topic = incoming_email_post.topic
       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)
+    it "skips sending a notification email to the group and all other email addresses that are _not_ members of the group,
+    sends a group_smtp_email instead" do
+      NotificationEmailer.enable
+
+      incoming_email_post = create_post_with_incoming
+      topic = incoming_email_post.topic
+
+      group_user1 = Fabricate(:group_user, group: group)
+      group_user2 = Fabricate(:group_user, group: group)
+      TopicUser.create(user: group_user1.user, notification_level: TopicUser.notification_levels[:watching], topic: topic)
+      post = Fabricate(:post, topic: topic.reload)
+
+      # Sends an email for:
+      #
+      # 1. the group user that is watching the post (but does not send this email with group SMTO)
+      # 2. the group smtp email to notify all topic_users not in the group
+      expect { PostAlerter.new.after_save_post(post, true) }.to change {
+        ActionMailer::Base.deliveries.size
+      }.by(2).and change { Notification.count }.by(2)
+
+      # The group smtp email
+      email = ActionMailer::Base.deliveries.first
+      expect(email.from).to eq([group.email_username])
+      expect(email.to).to contain_exactly("foo@discourse.org")
+      expect(email.cc).to match_array(["bar@discourse.org", "jim@othersite.com"])
+      expect(email.subject).to eq("Re: #{topic.title}")
+
+      # The watching group user notification email
+      email = ActionMailer::Base.deliveries.last
+      expect(email.from).to eq([SiteSetting.notification_email])
+      expect(email.to).to contain_exactly(group_user1.user.email)
+      expect(email.cc).to eq(nil)
+      expect(email.subject).to eq("[Discourse] [PM] #{topic.title}")
     end
 
-    it "skips sending a notification email to the group and all other incoming email addresses" do
+    it "skips sending a notification email to the cc address that was added on the same post with an incoming email" do
+      NotificationEmailer.enable
 
-      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
+      incoming_email_post = create_post_with_incoming
+      topic = incoming_email_post.topic
 
       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)
+      expect { PostAlerter.new.after_save_post(post, true) }.to change {
+        ActionMailer::Base.deliveries.size
+      }.by(1).and change { Notification.count }.by(1)
       email = ActionMailer::Base.deliveries.last
+
+      # the reply post from someone who was emailed
+      reply_raw_mail = <<~MAIL
+      From: Bar <bar@discourse.org>
+      To: discourse@example.com
+      Cc: someothernewcc@baz.com, finalnewcc@doom.com
+      Subject: #{email.subject}
+      Date: Fri, 16 Jan 2021 00:12:43 +0100
+      Message-ID: <sdugj3o4iyu4832x3487@discourse.org.mail>
+      In-Reply-To: #{email.message_id}
+      Mime-Version: 1.0
+      Content-Type: text/plain
+      Content-Transfer-Encoding: 7bit
+
+      Hey here is my reply!
+      MAIL
+
+      reply_post_from_email = nil
+      expect {
+        reply_post_from_email = Email::Receiver.new(reply_raw_mail, {}).process!
+      }.to change {
+        User.count # the two new cc addresses have users created
+      }.by(2).and change {
+        TopicAllowedUser.where(topic: topic).count # and they are added as topic allowed users
+      }.by(2).and change {
+        # but they are not sent emails because they were cc'd on an email, only jim@othersite.com
+        # is emailed because he is a topic allowed user cc'd on the _original_ email and he is not
+        # the one creating the post, and foo@discourse.org, who is the OP of the topic
+        ActionMailer::Base.deliveries.size
+      }.by(1).and change {
+        Notification.count # and they are still sent their normal discourse notification
+      }.by(2)
+
+      email = ActionMailer::Base.deliveries.last
+
+      expect(email.to).to eq(["foo@discourse.org"])
+      expect(email.cc).to eq(["jim@othersite.com"])
       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
+
+    it "handles the OP of the topic replying by email and sends a group email to the other topic allowed users successfully" do
+      NotificationEmailer.enable
+
+      incoming_email_post = create_post_with_incoming
+      topic = incoming_email_post.topic
+      post = Fabricate(:post, topic: topic.reload)
+      expect { PostAlerter.new.after_save_post(post, true) }.to change {
+        ActionMailer::Base.deliveries.size
+      }.by(1).and change { Notification.count }.by(1)
+      email = ActionMailer::Base.deliveries.last
+
+      # the reply post from someone who was emailed
+      reply_raw_mail = <<~MAIL
+      From: Foo <foo@discourse.org>
+      To: discourse@example.com
+      Cc: someothernewcc@baz.com, finalnewcc@doom.com
+      Subject: #{email.subject}
+      Date: Fri, 16 Jan 2021 00:12:43 +0100
+      Message-ID: <sgk094238uc0348c334483@discourse.org.mail>
+      In-Reply-To: #{email.message_id}
+      Mime-Version: 1.0
+      Content-Type: text/plain
+      Content-Transfer-Encoding: 7bit
+
+      I am ~~Commander Shepherd~~ the OP and I approve of this message.
+      MAIL
+
+      reply_post_from_email = nil
+      expect {
+        reply_post_from_email = Email::Receiver.new(reply_raw_mail, {}).process!
+      }.to change {
+        User.count # the two new cc addresses have users created
+      }.by(2).and change {
+        TopicAllowedUser.where(topic: topic).count # and they are added as topic allowed users
+      }.by(2).and change {
+        # but they are not sent emails because they were cc'd on an email, only jim@othersite.com
+        # is emailed because he is a topic allowed user cc'd on the _original_ email and he is not
+        # the one creating the post
+        ActionMailer::Base.deliveries.size
+      }.by(1).and change {
+        Notification.count # and they are still sent their normal discourse notification
+      }.by(2)
+
+      email = ActionMailer::Base.deliveries.last
+
+      expect(email.to).to eq(["bar@discourse.org"])
+      expect(email.cc).to eq(["jim@othersite.com"])
+      expect(email.from).to eq([group.email_username])
+      expect(email.subject).to eq("Re: #{topic.title}")
+    end
+
+    it "handles the OP of the topic replying by email and cc'ing new people, and does not send a group SMTP email to those newly cc'd users" do
+      NotificationEmailer.enable
+
+      # this is a special case where we are not CC'ing on the original email,
+      # only on the follow up email
+      raw_mail = <<~MAIL
+      From: Foo <foo@discourse.org>
+      To: discourse@example.com
+      Subject: Full email group username flow
+      Date: Fri, 14 Jan 2021 00:12:43 +0100
+      Message-ID: <f4832ujfc3498u398i3@example.com.mail>
+      Mime-Version: 1.0
+      Content-Type: text/plain
+      Content-Transfer-Encoding: 7bit
+
+      This is the first email.
+      MAIL
+
+      incoming_email_post = Email::Receiver.new(raw_mail, {}).process!
+      topic = incoming_email_post.topic
+      post = Fabricate(:post, topic: topic.reload)
+      expect { PostAlerter.new.after_save_post(post, true) }.to change {
+        ActionMailer::Base.deliveries.size
+      }.by(1).and change { Notification.count }.by(1)
+      email = ActionMailer::Base.deliveries.last
+
+      # the reply post from the OP, cc'ing new people in
+      reply_raw_mail = <<~MAIL
+      From: Foo <foo@discourse.org>
+      To: discourse@example.com
+      Cc: someothernewcc@baz.com, finalnewcc@doom.com
+      Subject: #{email.subject}
+      Date: Fri, 16 Jan 2021 00:12:43 +0100
+      Message-ID: <3849cu9843yncr9834yr9348x934@discourse.org.mail>
+      In-Reply-To: #{email.message_id}
+      Mime-Version: 1.0
+      Content-Type: text/plain
+      Content-Transfer-Encoding: 7bit
+
+      I am inviting my mates to this email party.
+      MAIL
+
+      reply_post_from_email = nil
+      expect {
+        reply_post_from_email = Email::Receiver.new(reply_raw_mail, {}).process!
+      }.to change {
+        User.count # the two new cc addresses have users created
+      }.by(2).and change {
+        TopicAllowedUser.where(topic: topic).count # and they are added as topic allowed users
+      }.by(2).and change {
+        # but they are not sent emails because they were cc'd on an email.
+        # no group smtp message is sent because the OP is not sent an email,
+        # they made this post.
+        ActionMailer::Base.deliveries.size
+      }.by(0).and change {
+        Notification.count # and they are still sent their normal discourse notification
+      }.by(2)
+
+      last_email = ActionMailer::Base.deliveries.last
+      expect(email).to eq(last_email)
+    end
   end
 end