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

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

This PR backtracks a fair bit on this one FEATURE: Use group SMTP settings for sending user notification emails (initial) by martin-brennan · Pull Request #13220 · discourse/discourse · GitHub.

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.

diff --git a/app/jobs/regular/group_smtp_email.rb b/app/jobs/regular/group_smtp_email.rb
index ba7afa9..8092dc3 100644
--- a/app/jobs/regular/group_smtp_email.rb
+++ b/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,
diff --git a/app/mailers/group_smtp_mailer.rb b/app/mailers/group_smtp_mailer.rb
index 44edec9..574facc 100644
--- a/app/mailers/group_smtp_mailer.rb
+++ b/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
 
diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb
index 31336ce..993d46f 100644
--- a/app/mailers/user_notifications.rb
+++ b/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,

[... diff too long, it was truncated ...]

GitHub sha: 87684f7c5e7e48841743d38df8a57ed827715b47

This commit appears in #13489 which was approved by udan11. It was merged by martin.