FIX: only create 1 email_log when an email is sent

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")
+      set_skip_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
+      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)
+    def set_skip_context(type, user_id, to_address)
-      @context = {type: type, user_id: user_id, to_address: to_address}
+      @skip_context = {type: type, user_id: user_id, to_address: to_address}
     end
 
 
-    def message_for_email(  user,
+    def message_for_email(user, post, type, notification,
-                            post,
+                          notification_type=nil, notification_data_hash=nil,
-                            type,
+                          email_token=nil, to_address=nil)
-                            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)
+    def skip(reason)
-      options = {
+      EmailLog.create!(
-        email_type: @context[:type],
+        email_type: @skip_context[:type],
-        to_address: @context[:to_address],
+        to_address: @skip_context[:to_address],
-        user_id: @context[:user_id],
+        user_id: @skip_context[:user_id],
-      }
+        skipped: true,
-
+        skipped_reason: reason,
-      if skipped_reason.present?
+      )
-        options[:skipped] = true
-        options[:skipped_reason] = skipped_reason
-      end
-
-      EmailLog.create!(options)
     end
 
   end

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,
+      email_log = EmailLog.new(email_type: @email_type, to_address: to_address, user_id: @user.try(:id))
-                               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 reply_key.present? && @message.header['Reply-To'] =~ /\<([^\>]+)\>/
-
+        email = Regexp.last_match[1]
-        if @message.header['Reply-To'] =~ /\<([^\>]+)\>/
+        @message.header['List-Post'] = "<mailto:#{email}>"
-          email = Regexp.last_match[1]
-          @message.header['List-Post'] = "<mailto:#{email}>"
-        end
       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-Topic-Id']  = nil if topic_id.present?
-      @message.header['X-Discourse-Post-Id'] = nil if post_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 =~ /<img[^>]+>/
+      if SiteSetting.strip_images_from_short_emails &&
+         @message.html_part.body.to_s.bytesize <= SiteSetting.short_email_length &&
+         @message.html_part.body =~ /<img[^>]+>/
         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,
+      EmailLog.create!(
-                      to_address: to_address,
+        email_type: @email_type,
-                      user_id: @user.try(:id),
+        to_address: to_address,
-                      skipped: true,
+        user_id: @user.try(:id),
-                      skipped_reason: reason)
+        skipped: true,
+        skipped_reason: reason
+      )
     end
 
   end

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)