FIX: Improve participant display in group SMTP emails (#13539)

This PR makes several changes to the group SMTP email contents to make it look more like a support inbox message.

* Remove the context posts, they only add clutter to the email and replies
* Display email addresses of staged users instead of odd generated usernames
* Add a "please reply above this line" message to sent emails

app/mailers/group_smtp_mailer.rb

@@ -10,16 +10,6 @@ class GroupSmtpMailer < ActionMailer::Base
 
     op_incoming_email = post.topic.first_post.incoming_email
 
-    context_posts = Post
-      .where(topic_id: post.topic_id)
-      .where("post_number < ?", post.post_number)
-      .where(user_deleted: false)
-      .where(hidden: false)
-      .where(post_type: Post.types[:regular])
-      .order(created_at: :desc)
-      .limit(SiteSetting.email_posts_context)
-      .to_a
-
     delivery_options = {
       address: from_group.smtp_server,
       port: from_group.smtp_port,
@@ -35,14 +25,14 @@ class GroupSmtpMailer < ActionMailer::Base
       user_name = post.user.name unless post.user.name.blank?
     end
 
-    group_name = from_group.full_name.presence || from_group.name
+    group_name = from_group.name_full_preferred
     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),
+      context: "",
       username: post.user.username,
       group_name: group_name,
       allow_reply_by_email: true,
@@ -59,45 +49,25 @@ class GroupSmtpMailer < ActionMailer::Base
       delivery_method_options: delivery_options,
       from: from_group.email_username,
       from_alias: I18n.t('email_from', user_name: group_name, site_name: Email.site_title),
-      html_override: html_override(post, context_posts: context_posts),
+      html_override: html_override(post),
       cc: cc_addresses
     )
   end
 
   private
 
-  def context(context_posts)
-    return "" if SiteSetting.private_email?
-
-    context = +""
-
-    if context_posts.size > 0
-      context << +"-- \n*#{I18n.t('user_notifications.previous_discussion')}*\n"
-      context_posts.each { |post| context << email_post_markdown(post, true) }
-    end
-
-    context
-  end
-
-  def email_post_markdown(post, add_posted_by = false)
-    result = +"#{post.raw}\n\n"
-    if add_posted_by
-      result << "#{I18n.t('user_notifications.posted_by', username: post.username, post_date: post.created_at.strftime("%m/%d/%Y"))}\n\n"
-    end
-    result
-  end
-
-  def html_override(post, context_posts: nil)
+  def html_override(post)
     UserNotificationRenderer.render(
       template: 'email/notification',
       format: :html,
       locals: {
-        context_posts: context_posts,
+        context_posts: nil,
         reached_limit: nil,
         post: post,
         in_reply_to_post: post.reply_to_post,
         classes: Rtl.new(nil).css_class,
-        first_footer_classes: ''
+        first_footer_classes: '',
+        reply_above_line: true
       }
     )
   end
@@ -106,14 +76,22 @@ class GroupSmtpMailer < ActionMailer::Base
     list = []
 
     post.topic.allowed_groups.each do |g|
-      list.push("[#{g.name} (#{g.users.count})](#{Discourse.base_url}/groups/#{g.name})")
+      list.push("[#{g.name_full_preferred}](#{Discourse.base_url}/groups/#{g.name})")
     end
 
     post.topic.allowed_users.each do |u|
       if SiteSetting.prioritize_username_in_ux?
-        list.push("[#{u.username}](#{Discourse.base_url}/u/#{u.username_lower})")
+        if u.staged?
+          list.push("[#{u.email}](#{Discourse.base_url}/u/#{u.username_lower})")
+        else
+          list.push("[#{u.username}](#{Discourse.base_url}/u/#{u.username_lower})")
+        end
       else
-        list.push("[#{u.name.blank? ? u.username : u.name}](#{Discourse.base_url}/u/#{u.username_lower})")
+        if u.staged?
+          list.push("[#{u.email}](#{Discourse.base_url}/u/#{u.username_lower})")
+        else
+          list.push("[#{u.name.blank? ? u.username : u.name}](#{Discourse.base_url}/u/#{u.username_lower})")
+        end
       end
     end
 

app/mailers/user_notifications.rb

@@ -632,7 +632,8 @@ class UserNotifications < ActionMailer::Base
                     post: post,
                     in_reply_to_post: in_reply_to_post,
                     classes: Rtl.new(user).css_class,
-                    first_footer_classes: first_footer_classes
+                    first_footer_classes: first_footer_classes,
+                    reply_above_line: false
           }
         )
       end

app/models/email_log.rb

@@ -96,6 +96,21 @@ class EmailLog < ActiveRecord::Base
   def cc_addresses_split
     @cc_addresses_split ||= self.cc_addresses&.split(";") || []
   end
+
+  def as_mail_message
+    return if self.raw.blank?
+    @mail_message ||= Mail.new(self.raw)
+  end
+
+  def raw_headers
+    return if self.raw.blank?
+    as_mail_message.header.raw_source
+  end
+
+  def raw_body
+    return if self.raw.blank?
+    as_mail_message.body
+  end
 end
 
 # == Schema Information

app/models/group.rb

@@ -894,11 +894,15 @@ class Group < ActiveRecord::Base
     SystemMessage.create_from_system_user(
       user,
       owner ? :user_added_to_group_as_owner : :user_added_to_group_as_member,
-      group_name: self.full_name.presence || self.name,
+      group_name: name_full_preferred,
       group_path: "/g/#{self.name}"
     )
   end
 
+  def name_full_preferred
+    self.full_name.presence || self.name
+  end
+
   def message_count
     return 0 unless self.has_messages
     TopicAllowedGroup.where(group_id: self.id).joins(:topic).count

app/views/email/notification.html.erb

@@ -1,4 +1,9 @@
 <div id='main' class=<%= classes %>>
+  <%- if reply_above_line.present? %>
+  <div class="reply-above-line">
+    <%= t('user_notifications.reply_above_line') %>
+  </div>
+  <% end %>
 
   <div class='header-instructions'>%{header_instructions}</div>
 

config/locales/server.en.yml

@@ -3471,6 +3471,7 @@ en:
     only_reply_by_email_pm: "Reply to this email to respond to %{participants}."
     visit_link_to_respond: "[Visit Topic](%{base_url}%{url}) to respond."
     visit_link_to_respond_pm: "[Visit Message](%{base_url}%{url}) to respond to %{participants}."
+    reply_above_line: "## Please type your reply above this line. ##"
 
     posted_by: "Posted by %{username} on %{post_date}"
 

lib/email/styles.rb

@@ -232,6 +232,7 @@ module Email
       style('.lightbox-wrapper .meta', 'display: none')
       style('div.undecorated-link-footer a', "font-weight: normal;")
       style('.mso-accent-link', "mso-border-alt: 6px solid #{SiteSetting.email_accent_bg_color}; background-color: #{SiteSetting.email_accent_bg_color};")
+      style('.reply-above-line', "font-size: 10px;font-family:'lucida grande',tahoma,verdana,arial,sans-serif;color: #b5b5b5;padding: 5px 0px 20px;border-top: 1px dotted #ddd;")
 
       onebox_styles
       plugin_styles

spec/jobs/regular/group_smtp_email_spec.rb

@@ -3,10 +3,10 @@
 require 'rails_helper'
 
 RSpec.describe Jobs::GroupSmtpEmail do
+  fab!(:topic) { Fabricate(:private_message_topic, title: "Help I need support") }
   fab!(:post) do
-    topic = Fabricate(:topic, title: "Help I need support")
-    Fabricate(:post, topic: topic)
-    Fabricate(:post, topic: topic)
+    Fabricate(:post, topic: topic, raw: "some first post content")
+    Fabricate(:post, topic: topic, raw: "this is the second post reply")
   end
   fab!(:group) { Fabricate(:smtp_group, name: "support-group", full_name: "Support Group") }
   fab!(:recipient_user) { Fabricate(:user, email: "test@test.com") }
@@ -19,12 +19,19 @@ RSpec.describe Jobs::GroupSmtpEmail do
       cc_emails: ["otherguy@test.com", "cormac@lit.com"]
     }
   end
+  let(:staged1) { Fabricate(:staged, email: "otherguy@test.com") }
+  let(:staged2) { Fabricate(:staged, email: "cormac@lit.com") }
+  let(:normaluser) { Fabricate(:user, email: "justanormalguy@test.com", username: "normaluser") }
 
   before do
     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
+    TopicAllowedGroup.create(group: group, topic: topic)
+    TopicAllowedUser.create(user: staged1, topic: topic)
+    TopicAllowedUser.create(user: staged2, topic: topic)
+    TopicAllowedUser.create(user: normaluser, topic: topic)
   end
 
   it "sends an email using the GroupSmtpMailer and Email::Sender" do
@@ -33,6 +40,55 @@ RSpec.describe Jobs::GroupSmtpEmail do
     subject.execute(args)
   end
 
+  it "includes a 'reply above this line' message" 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.as_mail_message.html_part.to_s).to include(I18n.t("user_notifications.reply_above_line"))
+  end
+
+  it "does not include context posts" 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.as_mail_message.text_part.to_s).not_to include(I18n.t("user_notifications.previous_discussion"))
+    expect(email_log.as_mail_message.text_part.to_s).not_to include("some first post content")
+  end
+
+  it "includes the participants in the correct format" 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.as_mail_message.text_part.to_s).to include("Support Group")
+    expect(email_log.as_mail_message.text_part.to_s).to include("otherguy@test.com")
+    expect(email_log.as_mail_message.text_part.to_s).to include("cormac@lit.com")
+    expect(email_log.as_mail_message.text_part.to_s).to include("normaluser")
+  end
+
+  it "creates an EmailLog record with the correct details" 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).not_to eq(nil)
+    expect(email_log.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost")
+  end
+
+  it "creates an IncomingEmail record with the correct details to avoid double processing IMAP" do
+    subject.execute(args)
+    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).to eq(nil)
+    expect(email_log.raw_headers).not_to include("Reply-To: Support Group via Discourse <#{group.email_username}")
+    expect(email_log.raw_headers).to include("From: Support Group via Discourse <#{group.email_username}")
+  end
+
   it "creates an EmailLog record with the correct details" do
     subject.execute(args)
     email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id)
@@ -64,7 +120,7 @@ RSpec.describe Jobs::GroupSmtpEmail do
     group.update(full_name: "")
     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.raw).to include("From: support-group via Discourse <#{group.email_username}")
+    expect(email_log.raw_headers).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
@@ -75,13 +131,11 @@ RSpec.describe Jobs::GroupSmtpEmail do
   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])
+      expect(email_log.cc_user_ids).to match_array([staged1.id, staged2.id])
     end
   end