FEATURE: Use group SMTP job and mailer instead of UserNotifications c…

…hange (#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 @@ def execute(args)
       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 @@ def execute(args)
       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 @@
 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 @@ def send_mail(from_group, to_address, post)
       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 @@ def user_private_message(user, opts)
     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 @@ def notification_email(user, opts)
       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 @@ def send_notification_email(opts)
     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 @@ def send_notification_email(opts)
 
     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 @@ def send_notification_email(opts)
       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 @@ def send_notification_email(opts)
       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 @@ def notify_pm_users(post, reply_to_user, notified)
 
     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)
-
-    # Users that aren't part of any mentioned groups and who did not email
-    # the group directly at the group's email_username.
+    # 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)
+
+    # 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 @@ def notify_pm_users(post, reply_to_user, notified)
       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,35 +617,62 @@ def notify_pm_users(post, reply_to_user, notified)
   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
-    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
     # (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
+    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 @@ def build_args
         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 @@ def header_args
       # 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 @@ def merge_json_x_header(name, value)
     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