From 96380bfd3803cb7c6b1d26e5b48b7a3dfb40b8ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Fri, 29 Jan 2016 16:49:49 +0100 Subject: [PATCH] FIX: only create 1 email_log when an email is sent --- app/jobs/regular/user_email.rb | 60 +++++++++++++--------------------- lib/email/sender.rb | 44 ++++++++++++------------- spec/jobs/user_email_spec.rb | 4 ++- 3 files changed, 47 insertions(+), 61 deletions(-) diff --git a/app/jobs/regular/user_email.rb b/app/jobs/regular/user_email.rb index 612ccb7be7f..c38d103fa1a 100644 --- a/app/jobs/regular/user_email.rb +++ b/app/jobs/regular/user_email.rb @@ -6,22 +6,21 @@ module Jobs class UserEmail < Jobs::Base def execute(args) - - notification,post = nil + notification, post = nil raise Discourse::InvalidParameters.new(:user_id) unless args[:user_id].present? - raise Discourse::InvalidParameters.new(:type) unless args[:type].present? + raise Discourse::InvalidParameters.new(:type) unless args[:type].present? type = args[:type] user = User.find_by(id: args[:user_id]) - set_context(type, args[:user_id], args[:to_address] || user.try(:email) || "no_email_found") - return create_email_log(I18n.t("email_log.no_user", user_id: args[:user_id])) unless user + set_skip_context(type, args[:user_id], args[:to_address] || user.try(:email) || "no_email_found") + return skip(I18n.t("email_log.no_user", user_id: args[:user_id])) unless user if args[:post_id] post = Post.find_by(id: args[:post_id]) - return create_email_log(I18n.t('email_log.post_not_found', post_id: args[:post_id])) unless post.present? + return skip(I18n.t('email_log.post_not_found', post_id: args[:post_id])) unless post.present? end if args[:notification_id].present? @@ -39,29 +38,24 @@ module Jobs if message - Email::Sender.new(message, args[:type], @user).send + Email::Sender.new(message, args[:type], user).send else skip_reason end end - def set_context(type, user_id, to_address) - @context = {type: type, user_id: user_id, to_address: to_address} + def set_skip_context(type, user_id, to_address) + @skip_context = {type: type, user_id: user_id, to_address: to_address} end - def message_for_email( user, - post, - type, - notification, - notification_type=nil, - notification_data_hash=nil, - email_token=nil, - to_address=nil) + def message_for_email(user, post, type, notification, + notification_type=nil, notification_data_hash=nil, + email_token=nil, to_address=nil) - set_context(type, user.id, to_address || user.email) + set_skip_context(type, user.id, to_address || user.email) - return skip_message(I18n.t("email_log.anonymous_user")) if user.anonymous? + return skip_message(I18n.t("email_log.anonymous_user")) if user.anonymous? return skip_message(I18n.t("email_log.suspended_not_pm")) if user.suspended? && type != :user_private_message return if user.staged && type == :digest @@ -80,7 +74,6 @@ module Jobs end if notification || notification_type - email_args[:notification_type] ||= notification_type || notification.try(:notification_type) email_args[:notification_data_hash] ||= notification_data_hash || notification.try(:data_hash) @@ -108,15 +101,13 @@ module Jobs message.to = [to_address] end - create_email_log - - [message,nil] + [message, nil] end private def skip_message(reason) - [nil, create_email_log(reason)] + [nil, skip(reason)] end # If this email has a related post, don't send an email if it's been deleted or seen recently. @@ -131,19 +122,14 @@ module Jobs end end - def create_email_log(skipped_reason=nil) - options = { - email_type: @context[:type], - to_address: @context[:to_address], - user_id: @context[:user_id], - } - - if skipped_reason.present? - options[:skipped] = true - options[:skipped_reason] = skipped_reason - end - - EmailLog.create!(options) + def skip(reason) + EmailLog.create!( + email_type: @skip_context[:type], + to_address: @skip_context[:to_address], + user_id: @skip_context[:user_id], + skipped: true, + skipped_reason: reason, + ) end end diff --git a/lib/email/sender.rb b/lib/email/sender.rb index 8e0080c9a2a..e1f81c84fb1 100644 --- a/lib/email/sender.rb +++ b/lib/email/sender.rb @@ -59,10 +59,7 @@ module Email @message.text_part.content_type = 'text/plain; charset=UTF-8' # Set up the email log - email_log = EmailLog.new(email_type: @email_type, - to_address: to_address, - user_id: @user.try(:id)) - + email_log = EmailLog.new(email_type: @email_type, to_address: to_address, user_id: @user.try(:id)) host = Email::Sender.host_for(Discourse.base_url) @@ -97,32 +94,30 @@ module Email else list_id = "<#{host}>" end - @message.header['List-ID'] = list_id - - @message.header['List-Archive'] = topic.url if topic # http://www.ietf.org/rfc/rfc3834.txt - @message.header['Precedence'] = 'list' + @message.header['Precedence'] = 'list' + @message.header['List-ID'] = list_id + @message.header['List-Archive'] = topic.url if topic end - if reply_key.present? - - if @message.header['Reply-To'] =~ /\<([^\>]+)\>/ - email = Regexp.last_match[1] - @message.header['List-Post'] = "" - end + if reply_key.present? && @message.header['Reply-To'] =~ /\<([^\>]+)\>/ + email = Regexp.last_match[1] + @message.header['List-Post'] = "" end email_log.post_id = post_id if post_id.present? email_log.reply_key = reply_key if reply_key.present? # Remove headers we don't need anymore - @message.header['X-Discourse-Topic-Id'] = nil if topic_id.present? - @message.header['X-Discourse-Post-Id'] = nil if post_id.present? + @message.header['X-Discourse-Topic-Id'] = nil if topic_id.present? + @message.header['X-Discourse-Post-Id'] = nil if post_id.present? @message.header['X-Discourse-Reply-Key'] = nil if reply_key.present? # Suppress images from short emails - if SiteSetting.strip_images_from_short_emails && @message.html_part.body.to_s.bytesize <= SiteSetting.short_email_length && @message.html_part.body =~ /]+>/ + if SiteSetting.strip_images_from_short_emails && + @message.html_part.body.to_s.bytesize <= SiteSetting.short_email_length && + @message.html_part.body =~ /]+>/ style = Email::Styles.new(@message.html_part.body.to_s) @message.html_part.body = style.strip_avatars_and_emojis end @@ -141,7 +136,8 @@ module Email def to_address @to_address ||= begin to = @message ? @message.to : nil - to.is_a?(Array) ? to.first : to + to = to.first if Array === to + to.presence || "no_email_found" end end @@ -166,11 +162,13 @@ module Email end def skip(reason) - EmailLog.create(email_type: @email_type, - to_address: to_address, - user_id: @user.try(:id), - skipped: true, - skipped_reason: reason) + EmailLog.create!( + email_type: @email_type, + to_address: to_address, + user_id: @user.try(:id), + skipped: true, + skipped_reason: reason + ) end end diff --git a/spec/jobs/user_email_spec.rb b/spec/jobs/user_email_spec.rb index a6467ad4d58..6b6c5a93cfd 100644 --- a/spec/jobs/user_email_spec.rb +++ b/spec/jobs/user_email_spec.rb @@ -63,7 +63,9 @@ describe Jobs::UserEmail do context "email_log" do - it "creates an email log when the mail is sent" do + before { Fabricate(:post) } + + it "creates an email log when the mail is sent (via Email::Sender)" do last_emailed_at = user.last_emailed_at expect { Jobs::UserEmail.new.execute(type: :digest, user_id: user.id) }.to change { EmailLog.count }.by(1)