FIX: *always* create an EmailLog whenever we run the UserEmail job

There were actually 2 bugs:

1/ Calling '.try(:key)' on a hash doesn't work. So EmailLogs were never associated to a user.

2/ Turns out that we update the 'user.last_emailed_at' whenever we create an EmailLog (in the 'after_create' callback).
So we need to always create an EmailLog (whenever the email is sent or skipped).

app/jobs/regular/user_email.rb

@@ -16,12 +16,12 @@ module Jobs
 
       user = User.find_by(id: args[:user_id])
 
-      set_skip_context(type, args[:user_id], args[:to_address] || user.try(:email) || "no_email_found")
+      set_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
+      return create_email_log(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 skip(I18n.t('email_log.post_not_found', post_id: args[:post_id])) unless post.present?
+        return create_email_log(I18n.t('email_log.post_not_found', post_id: args[:post_id])) unless post.present?
       end
 
       if args[:notification_id].present?
@@ -45,8 +45,8 @@ module Jobs
       end
     end
 
-    def set_skip_context(type, user_id, to_address)
+    def set_context(type, user_id, to_address)
-      @skip_context = {type: type, user_id: user_id, to_address: to_address}
+      @context = {type: type, user_id: user_id, to_address: to_address}
     end
 
 
@@ -59,7 +59,7 @@ module Jobs
                             email_token=nil,
                             to_address=nil)
 
-      set_skip_context(type, user.id, to_address || user.email)
+      set_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.suspended_not_pm")) if user.suspended? && type != :user_private_message
@@ -108,34 +108,42 @@ module Jobs
         message.to = [to_address]
       end
 
-      [message,nil]
+      create_email_log
 
+      [message,nil]
     end
 
     private
 
     def skip_message(reason)
-      [nil, skip(reason)]
+      [nil, create_email_log(reason)]
     end
 
     # If this email has a related post, don't send an email if it's been deleted or seen recently.
     def skip_email_for_post(post, user)
       if post
-        return I18n.t('email_log.topic_nil') if post.topic.blank?
+        return I18n.t('email_log.topic_nil')      if post.topic.blank?
-        return I18n.t('email_log.post_deleted') if post.user_deleted?
+        return I18n.t('email_log.post_deleted')   if post.user_deleted?
         return I18n.t('email_log.user_suspended') if (user.suspended? && !post.user.try(:staff?))
-        return I18n.t('email_log.already_read') if PostTiming.where(topic_id: post.topic_id, post_number: post.post_number, user_id: user.id).present?
+        return I18n.t('email_log.already_read')   if PostTiming.where(topic_id: post.topic_id, post_number: post.post_number, user_id: user.id).present?
       else
         false
       end
     end
 
-    def skip(reason)
+    def create_email_log(skipped_reason=nil)
-      EmailLog.create( email_type: @skip_context[:type],
+      options = {
-                       to_address: @skip_context[:to_address],
+        email_type: @context[:type],
-                       user_id: @skip_context.try(:user_id),
+        to_address: @context[:to_address],
-                       skipped: true,
+        user_id: @context[:user_id],
-                       skipped_reason: reason)
+      }
+
+      if skipped_reason.present?
+        options[:skipped] = true
+        options[:skipped_reason] = skipped_reason
+      end
+
+      EmailLog.create!(options)
     end
 
   end

app/jobs/scheduled/enqueue_digest_emails.rb

@@ -18,8 +18,8 @@ module Jobs
                   .where(email_digests: true, active: true, staged: false)
                   .not_suspended
                   .where("COALESCE(last_emailed_at, '2010-01-01') <= CURRENT_TIMESTAMP - ('1 DAY'::INTERVAL * digest_after_days)")
-                  .where("(COALESCE(last_seen_at, '2010-01-01') <= CURRENT_TIMESTAMP - ('1 DAY'::INTERVAL * digest_after_days)) AND
+                  .where("COALESCE(last_seen_at, '2010-01-01') <= CURRENT_TIMESTAMP - ('1 DAY'::INTERVAL * digest_after_days)")
-                           COALESCE(last_seen_at, '2010-01-01') >= CURRENT_TIMESTAMP - ('1 DAY'::INTERVAL * #{SiteSetting.suppress_digest_email_after_days})")
+                  .where("COALESCE(last_seen_at, '2010-01-01') >= CURRENT_TIMESTAMP - ('1 DAY'::INTERVAL * #{SiteSetting.suppress_digest_email_after_days})")
 
       # If the site requires approval, make sure the user is approved
       if SiteSetting.must_approve_users?

spec/jobs/user_email_spec.rb

@@ -61,6 +61,26 @@ describe Jobs::UserEmail do
     end
   end
 
+  context "email_log" do
+
+    it "creates an email log when the mail is sent" do
+      expect { Jobs::UserEmail.new.execute(type: :digest, user_id: user.id) }.to change { EmailLog.count }.by(1)
+      email_log = EmailLog.last
+      expect(email_log.skipped).to eq(false)
+      expect(email_log.user_id).to eq(user.id)
+    end
+
+    it "creates an email log when the mail is skipped" do
+      user.update_columns(suspended_till: 1.year.from_now)
+      expect { Jobs::UserEmail.new.execute(type: :digest, user_id: user.id) }.to change { EmailLog.count }.by(1)
+      email_log = EmailLog.last
+      expect(email_log.skipped).to eq(true)
+      expect(email_log.skipped_reason).to be_present
+      expect(email_log.user_id).to eq(user.id)
+    end
+
+  end
+
   context 'args' do
 
     it 'passes a token as an argument when a token is present' do