diff --git a/app/assets/javascripts/admin/models/email_log.js b/app/assets/javascripts/admin/models/email_log.js index 28e7066173f..b22cc9e2933 100644 --- a/app/assets/javascripts/admin/models/email_log.js +++ b/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) { diff --git a/app/assets/javascripts/admin/routes/admin_email_skipped_route.js b/app/assets/javascripts/admin/routes/admin_email_skipped_route.js new file mode 100644 index 00000000000..c792deba2b8 --- /dev/null +++ b/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'}); + } +}); diff --git a/app/assets/javascripts/admin/routes/admin_routes.js b/app/assets/javascripts/admin/routes/admin_routes.js index e27ebd4372a..2864858bfb8 100644 --- a/app/assets/javascripts/admin/routes/admin_routes.js +++ b/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' }); }); diff --git a/app/assets/javascripts/admin/templates/email.js.handlebars b/app/assets/javascripts/admin/templates/email.js.handlebars index fbabe8bf1ae..0fcbc1b61be 100644 --- a/app/assets/javascripts/admin/templates/email.js.handlebars +++ b/app/assets/javascripts/admin/templates/email.js.handlebars @@ -3,6 +3,7 @@ diff --git a/app/assets/javascripts/admin/templates/email_skipped.js.handlebars b/app/assets/javascripts/admin/templates/email_skipped.js.handlebars new file mode 100644 index 00000000000..1a63c978b87 --- /dev/null +++ b/app/assets/javascripts/admin/templates/email_skipped.js.handlebars @@ -0,0 +1,33 @@ + + + + + + + + + + + + {{#if model.length}} + {{#groupedEach model}} + + + + + + + + {{/groupedEach}} + {{/if}} + +
{{i18n admin.email.sent_at}}{{i18n admin.email.user}}{{i18n admin.email.to_address}}{{i18n admin.email.email_type}}{{i18n admin.email.skip_reason}}
{{unboundDate created_at}} + {{#if user}} + {{#link-to 'adminUser' user}}{{avatar user imageSize="tiny"}}{{/link-to}} + {{#link-to 'adminUser' user}}{{user.username}}{{/link-to}} + {{else}} + — + {{/if}} + {{to_address}}{{email_type}} + {{skipped_reason}} +
diff --git a/app/controllers/admin/email_controller.rb b/app/controllers/admin/email_controller.rb index 4867fc9f2b3..139f6b8157d 100644 --- a/app/controllers/admin/email_controller.rb +++ b/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 diff --git a/app/jobs/regular/user_email.rb b/app/jobs/regular/user_email.rb index ab7bc548e19..3e4408475c8 100644 --- a/app/jobs/regular/user_email.rb +++ b/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 diff --git a/app/models/email_log.rb b/app/models/email_log.rb index c3db1dd542e..9318dc0b018 100644 --- a/app/models/email_log.rb +++ b/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) diff --git a/app/serializers/email_log_serializer.rb b/app/serializers/email_log_serializer.rb index 0461d739c5a..977e3ee93bf 100644 --- a/app/serializers/email_log_serializer.rb +++ b/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 diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 4f6bc3d5a13..cd188f92c55 100644 --- a/config/locales/client.en.yml +++ b/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" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 6d5b3784233..71f393c5796 100644 --- a/config/locales/server.en.yml +++ b/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" diff --git a/config/routes.rb b/config/routes.rb index 846cf1eb204..add242a980c 100644 --- a/config/routes.rb +++ b/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 diff --git a/db/migrate/20140214151255_add_skipped_to_email_logs.rb b/db/migrate/20140214151255_add_skipped_to_email_logs.rb new file mode 100644 index 00000000000..e6615453650 --- /dev/null +++ b/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 diff --git a/lib/email/sender.rb b/lib/email/sender.rb index d269bfce5a3..abdd350234c 100644 --- a/lib/email/sender.rb +++ b/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(/\"/, "'")}\" " 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 diff --git a/spec/models/email_log_spec.rb b/spec/models/email_log_spec.rb index a63936e95d9..9db88984fd2 100644 --- a/spec/models/email_log_spec.rb +++ b/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)