From d3e27cabf6beea7f77f1a14be43d1976853dfe8e Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 28 Jun 2021 10:42:06 +1000 Subject: [PATCH] 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 | 58 ++++++------------ app/mailers/user_notifications.rb | 3 +- app/models/email_log.rb | 15 +++++ app/models/group.rb | 6 +- app/views/email/notification.html.erb | 5 ++ config/locales/server.en.yml | 1 + lib/email/styles.rb | 1 + spec/jobs/regular/group_smtp_email_spec.rb | 68 +++++++++++++++++++--- 8 files changed, 108 insertions(+), 49 deletions(-) diff --git a/app/mailers/group_smtp_mailer.rb b/app/mailers/group_smtp_mailer.rb index 574facc1578..045f669ec03 100644 --- a/app/mailers/group_smtp_mailer.rb +++ b/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 diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 993d46fa25e..d5a20e0f073 100644 --- a/app/mailers/user_notifications.rb +++ b/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 diff --git a/app/models/email_log.rb b/app/models/email_log.rb index c089746ade2..e8dd29a5dfa 100644 --- a/app/models/email_log.rb +++ b/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 diff --git a/app/models/group.rb b/app/models/group.rb index c70918ee905..8a6d7183a3a 100644 --- a/app/models/group.rb +++ b/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 diff --git a/app/views/email/notification.html.erb b/app/views/email/notification.html.erb index cc30418c062..b3d3a2cba7e 100644 --- a/app/views/email/notification.html.erb +++ b/app/views/email/notification.html.erb @@ -1,4 +1,9 @@
> + <%- if reply_above_line.present? %> +
+ <%= t('user_notifications.reply_above_line') %> +
+ <% end %>
%{header_instructions}
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 404d3f96dea..319b7728e81 100644 --- a/config/locales/server.en.yml +++ b/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}" diff --git a/lib/email/styles.rb b/lib/email/styles.rb index 250ecff7e27..5938dffaf47 100644 --- a/lib/email/styles.rb +++ b/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 diff --git a/spec/jobs/regular/group_smtp_email_spec.rb b/spec/jobs/regular/group_smtp_email_spec.rb index ffba33863d4..1bcae95af06 100644 --- a/spec/jobs/regular/group_smtp_email_spec.rb +++ b/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