Log when and why an email was not sent in email_logs

app/assets/javascripts/admin/models/email_log.js

@@ -20,7 +20,7 @@ Discourse.EmailLog.reopenClass({
 
   findAll: function(filter) {
     var result = Em.A();
-    Discourse.ajax("/admin/email/logs.json", {
+    Discourse.ajax("/admin/email/" + (filter === 'skipped' ? 'skipped' : 'logs') + ".json", {
       data: { filter: filter }
     }).then(function(logs) {
       _.each(logs,function(log) {

app/assets/javascripts/admin/routes/admin_email_skipped_route.js

@@ -0,0 +1,17 @@
+/**
+  Handles routes related to viewing email logs of emails that were NOT sent.
+
+  @class AdminEmailSkippedRoute
+  @extends Discourse.Route
+  @namespace Discourse
+  @module Discourse
+**/
+Discourse.AdminEmailSkippedRoute = Discourse.Route.extend({
+  model: function() {
+    return Discourse.EmailLog.findAll('skipped');
+  },
+
+  renderTemplate: function() {
+    this.render('admin/templates/email_skipped', {into: 'adminEmail'});
+  }
+});

app/assets/javascripts/admin/routes/admin_routes.js

@@ -17,6 +17,7 @@ Discourse.Route.buildRoutes(function() {
 
     this.resource('adminEmail', { path: '/email'}, function() {
       this.route('logs');
+      this.route('skipped');
       this.route('previewDigest', { path: '/preview-digest' });
     });
 

app/assets/javascripts/admin/templates/email.js.handlebars

@@ -3,6 +3,7 @@
     <ul class="nav nav-pills">
       <li>{{#link-to 'adminEmail.index'}}{{i18n admin.email.settings}}{{/link-to}}</li>
       <li>{{#link-to 'adminEmail.logs'}}{{i18n admin.email.logs}}{{/link-to}}</li>
+      <li>{{#link-to 'adminEmail.skipped'}}{{i18n admin.email.skipped}}{{/link-to}}</li>
       <li>{{#link-to 'adminEmail.previewDigest'}}{{i18n admin.email.preview_digest}}{{/link-to}}</li>
     </ul>
   </div>

app/assets/javascripts/admin/templates/email_skipped.js.handlebars

@@ -0,0 +1,33 @@
+<table class='table'>
+  <thead>
+    <tr>
+      <th>{{i18n admin.email.sent_at}}</th>
+      <th>{{i18n admin.email.user}}</th>
+      <th>{{i18n admin.email.to_address}}</th>
+      <th>{{i18n admin.email.email_type}}</th>
+      <th>{{i18n admin.email.skip_reason}}</th>
+    </tr>
+  </thead>
+
+  {{#if model.length}}
+    {{#groupedEach model}}
+      <tr>
+        <td>{{unboundDate created_at}}</td>
+        <td>
+          {{#if user}}
+            {{#link-to 'adminUser' user}}{{avatar user imageSize="tiny"}}{{/link-to}}
+            {{#link-to 'adminUser' user}}{{user.username}}{{/link-to}}
+          {{else}}
+            &mdash;
+          {{/if}}
+        </td>
+        <td><a href='mailto:{{unbound to_address}}'>{{to_address}}</a></td>
+        <td>{{email_type}}</td>
+        <td>
+          {{skipped_reason}}
+        </td>
+      </tr>
+    {{/groupedEach}}
+  {{/if}}
+
+</table>

app/controllers/admin/email_controller.rb

@@ -16,7 +16,12 @@ class Admin::EmailController < Admin::AdminController
   end
 
   def logs
-    @email_logs = EmailLog.limit(50).includes(:user).order('created_at desc').to_a
+    @email_logs = EmailLog.sent.limit(50).includes(:user).order('created_at desc').to_a
+    render_serialized(@email_logs, EmailLogSerializer)
+  end
+
+  def skipped
+    @email_logs = EmailLog.skipped.limit(50).includes(:user).order('created_at desc').to_a
     render_serialized(@email_logs, EmailLogSerializer)
   end
 

app/jobs/regular/user_email.rb

@@ -7,27 +7,28 @@ module Jobs
 
     def execute(args)
 
+      @args = args
+
       # Required parameters
       raise Discourse::InvalidParameters.new(:user_id) unless args[:user_id].present?
       raise Discourse::InvalidParameters.new(:type) unless args[:type].present?
 
       # Find the user
-      user = User.where(id: args[:user_id]).first
-      return unless user
-      return if user.suspended? && args[:type] != :user_private_message
+      @user = User.where(id: args[:user_id]).first
+      return skip(I18n.t("email_log.no_user", user_id: args[:user_id])) unless @user
+      return skip(I18n.t("email_log.suspended_not_pm")) if @user.suspended? && args[:type] != :user_private_message
 
-      seen_recently = (user.last_seen_at.present? && user.last_seen_at > SiteSetting.email_time_window_mins.minutes.ago)
-      seen_recently = false if user.email_always
+      seen_recently = (@user.last_seen_at.present? && @user.last_seen_at > SiteSetting.email_time_window_mins.minutes.ago)
+      seen_recently = false if @user.email_always
 
       email_args = {}
 
       if args[:post_id]
-
         # Don't email a user about a post when we've seen them recently.
-        return if seen_recently
+        return skip(I18n.t('email_log.seen_recently')) if seen_recently
 
         post = Post.where(id: args[:post_id]).first
-        return unless post.present?
+        return skip(I18n.t('email_log.post_not_found', post_id: args[:post_id])) unless post.present?
 
         email_args[:post] = post
       end
@@ -38,40 +39,51 @@ module Jobs
       notification = Notification.where(id: args[:notification_id]).first if args[:notification_id].present?
       if notification.present?
         # Don't email a user about a post when we've seen them recently.
-        return if seen_recently && !user.suspended?
+        return skip(I18n.t('email_log.seen_recently')) if seen_recently && !@user.suspended?
 
         # Load the post if present
         email_args[:post] ||= Post.where(id: notification.data_hash[:original_post_id].to_i).first
         email_args[:post] ||= notification.post
         email_args[:notification] = notification
 
-        # Don't send email if the notification this email is about has already been read
-        return if notification.read?
+        return skip(I18n.t('email_log.notification_already_read')) if notification.read?
       end
 
-      return if skip_email_for_post(email_args[:post], user)
+      skip_reason = skip_email_for_post(email_args[:post], @user)
+      return skip(skip_reason) if skip_reason
 
       # Make sure that mailer exists
       raise Discourse::InvalidParameters.new(:type) unless UserNotifications.respond_to?(args[:type])
 
-      message = UserNotifications.send(args[:type], user, email_args)
+      message = UserNotifications.send(args[:type], @user, email_args)
       # Update the to address if we have a custom one
       if args[:to_address].present?
         message.to = [args[:to_address]]
       end
 
-      Email::Sender.new(message, args[:type], user).send
+      Email::Sender.new(message, args[:type], @user).send
     end
 
     private
 
     # 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)
-      post &&
-      (post.topic.blank? ||
-       post.user_deleted? ||
-       (user.suspended? && !post.user.try(:staff?)) ||
-       PostTiming.where(topic_id: post.topic_id, post_number: post.post_number, user_id: user.id).present?)
+      if post
+        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.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?
+      else
+        false
+      end
+    end
+
+    def skip(reason)
+      EmailLog.create( email_type: @args[:type],
+                       to_address: @args[:to_address] || @user.try(:email) || "no_email_found",
+                       user_id: @user.try(:id),
+                       skipped: true,
+                       skipped_reason: reason)
     end
 
   end

app/models/email_log.rb

@@ -6,13 +6,16 @@ class EmailLog < ActiveRecord::Base
   belongs_to :post
   belongs_to :topic
 
+  scope :sent,    -> { where(skipped: false) }
+  scope :skipped, -> { where(skipped: true) }
+
   after_create do
-    # Update last_emailed_at if the user_id is present
-    User.where(id: user_id).update_all("last_emailed_at = CURRENT_TIMESTAMP") if user_id.present?
+    # Update last_emailed_at if the user_id is present and email was sent
+    User.where(id: user_id).update_all("last_emailed_at = CURRENT_TIMESTAMP") if user_id.present? and !skipped
   end
 
   def self.count_per_day(sinceDaysAgo = 30)
-    where('created_at > ?', sinceDaysAgo.days.ago).group('date(created_at)').order('date(created_at)').count
+    where('created_at > ? and skipped = false', sinceDaysAgo.days.ago).group('date(created_at)').order('date(created_at)').count
   end
 
   def self.for(reply_key)

app/serializers/email_log_serializer.rb

@@ -5,8 +5,13 @@ class EmailLogSerializer < ApplicationSerializer
              :to_address,
              :email_type,
              :user_id,
-             :created_at
+             :created_at,
+             :skipped,
+             :skipped_reason
 
   has_one :user, serializer: BasicUserSerializer, embed: :objects
 
+  def include_skipped_reason?
+    object.skipped
+  end
 end

config/locales/client.en.yml

@@ -1382,6 +1382,7 @@ en:
         title: "Email"
         settings: "Settings"
         logs: "Logs"
+        skipped: "Skipped"
         sent_at: "Sent At"
         user: "User"
         email_type: "Email Type"
@@ -1398,6 +1399,7 @@ en:
         text: "text"
         last_seen_user: "Last Seen User:"
         reply_key: "Reply Key"
+        skip_reason: "Skip Reason"
 
       logs:
         title: "Logs"

config/locales/server.en.yml

@@ -1327,3 +1327,17 @@ en:
     sockpuppet: "A new user created a topic, and another new user at the same IP address replied. See the flag_sockpuppets site setting."
     spam_hosts: "This user tried to create multiple posts with links to the same domain. See the newuser_spam_host_threshold site setting."
 
+  email_log:
+    no_user: "Can't find user with id %{user_id}"
+    suspended_not_pm: "User is supsended and email is not a private message"
+    seen_recently: "User was seen recently"
+    post_not_found: "Can't find a post with id %{post_id}"
+    notification_already_read: "The notification this email is about has already been read"
+    topic_nil: "post.topic is nil"
+    post_deleted: "post was deleted by the author"
+    user_suspended: "user was suspended"
+    already_read: "user has already read this post"
+    message_blank: "message is blank"
+    message_to_blank: "message.to is blank"
+    text_part_body_blank: "text_part.body is blank"
+    body_blank: "body is blank"

config/routes.rb

@@ -68,6 +68,7 @@ Discourse::Application.routes.draw do
       collection do
         post "test"
         get "logs"
+        get "skipped"
         get "preview-digest" => "email#preview_digest"
       end
     end

db/migrate/20140214151255_add_skipped_to_email_logs.rb

@@ -0,0 +1,7 @@
+class AddSkippedToEmailLogs < ActiveRecord::Migration
+  def change
+    add_column :email_logs, :skipped, :boolean, default: :false
+    add_column :email_logs, :skipped_reason, :string
+    add_index  :email_logs, [:skipped, :created_at]
+  end
+end

lib/email/sender.rb

@@ -19,13 +19,13 @@ module Email
     end
 
     def send
-      return if @message.blank?
-      return if @message.to.blank?
+      return skip(I18n.t('email_log.message_blank')) if @message.blank?
+      return skip(I18n.t('email_log.message_to_blank')) if @message.to.blank?
 
       if @message.text_part
-        return if @message.text_part.body.to_s.blank?
+        return skip(I18n.t('email_log.text_part_body_blank')) if @message.text_part.body.to_s.blank?
       else
-        return if @message.body.to_s.blank?
+        return skip(I18n.t('email_log.body_blank')) if @message.body.to_s.blank?
       end
 
       @message.charset = 'UTF-8'
@@ -48,8 +48,6 @@ module Email
       @message.text_part.content_type = 'text/plain; charset=UTF-8'
 
       # Set up the email log
-      to_address = @message.to
-      to_address = to_address.first if to_address.is_a?(Array)
       email_log = EmailLog.new(email_type: @email_type,
                                to_address: to_address,
                                user_id: @user.try(:id))
@@ -87,6 +85,13 @@ module Email
 
     end
 
+    def to_address
+      @to_address ||= begin
+        to = @message ? @message.to : nil
+        to.is_a?(Array) ? to.first : to
+      end
+    end
+
     def self.host_for(base_url)
       host = "localhost"
       if base_url.present?
@@ -103,6 +108,8 @@ module Email
       "\"#{site_name.gsub(/\"/, "'")}\" <discourse.forum.#{Slug.for(site_name)}.#{host}>"
     end
 
+
+
     private
 
     def header_value(name)
@@ -111,5 +118,13 @@ module Email
       header.value
     end
 
+    def skip(reason)
+      EmailLog.create(email_type: @email_type,
+                      to_address: to_address,
+                      user_id: @user.try(:id),
+                      skipped: true,
+                      skipped_reason: reason)
+    end
+
   end
 end

spec/models/email_log_spec.rb

@@ -6,24 +6,35 @@ describe EmailLog do
   it { should validate_presence_of :to_address }
   it { should validate_presence_of :email_type }
 
+  let(:user) { Fabricate(:user) }
+
   context 'after_create' do
-
     context 'with user' do
-      let(:user) { Fabricate(:user) }
-
       it 'updates the last_emailed_at value for the user' do
         lambda {
           user.email_logs.create(email_type: 'blah', to_address: user.email)
           user.reload
         }.should change(user, :last_emailed_at)
       end
-    end
 
+      it "doesn't update last_emailed_at if skipped is true" do
+        expect {
+          user.email_logs.create(email_type: 'blah', to_address: user.email, skipped: true)
+          user.reload
+        }.to_not change { user.last_emailed_at }
+      end
+    end
+  end
+
+  describe '#count_per_day' do
+    it "counts sent emails" do
+      user.email_logs.create(email_type: 'blah', to_address: user.email)
+      user.email_logs.create(email_type: 'blah', to_address: user.email, skipped: true)
+      described_class.count_per_day.first[1].should == 1
+    end
   end
 
   describe ".last_sent_email_address" do
-    let(:user) { Fabricate(:user) }
-
     context "when user's email exist in the logs" do
       before do
         user.email_logs.create(email_type: 'signup', to_address: user.email)