diff --git a/app/assets/javascripts/admin/controllers/admin-email-bounced.js.es6 b/app/assets/javascripts/admin/controllers/admin-email-bounced.js.es6 index 230fa07afb7..0071850fe6e 100644 --- a/app/assets/javascripts/admin/controllers/admin-email-bounced.js.es6 +++ b/app/assets/javascripts/admin/controllers/admin-email-bounced.js.es6 @@ -1,9 +1,9 @@ -import AdminEmailLogsController from "admin/controllers/admin-email-logs"; -import debounce from "discourse/lib/debounce"; -import EmailLog from "admin/models/email-log"; +import AdminEmailLogsController from 'admin/controllers/admin-email-logs'; +import debounce from 'discourse/lib/debounce'; +import EmailLog from 'admin/models/email-log'; export default AdminEmailLogsController.extend({ filterEmailLogs: debounce(function() { - EmailLog.findAll(this.get("filter")).then(logs => this.set("model", logs)); - }, 250).observes("filter.{user,address,type,skipped_reason}") + EmailLog.findAll(this.get('filter')).then(logs => this.set('model', logs)); + }, 250).observes('filter.{user,address,type}') }); diff --git a/app/assets/javascripts/admin/controllers/admin-email-skipped.js.es6 b/app/assets/javascripts/admin/controllers/admin-email-skipped.js.es6 index 230fa07afb7..c2ec582af28 100644 --- a/app/assets/javascripts/admin/controllers/admin-email-skipped.js.es6 +++ b/app/assets/javascripts/admin/controllers/admin-email-skipped.js.es6 @@ -5,5 +5,5 @@ import EmailLog from "admin/models/email-log"; export default AdminEmailLogsController.extend({ filterEmailLogs: debounce(function() { EmailLog.findAll(this.get("filter")).then(logs => this.set("model", logs)); - }, 250).observes("filter.{user,address,type,skipped_reason}") + }, 250).observes("filter.{user,address,type}") }); diff --git a/app/assets/javascripts/admin/templates/email-skipped.hbs b/app/assets/javascripts/admin/templates/email-skipped.hbs index b03e6179bec..ce5bbaf8742 100644 --- a/app/assets/javascripts/admin/templates/email-skipped.hbs +++ b/app/assets/javascripts/admin/templates/email-skipped.hbs @@ -15,7 +15,7 @@ {{text-field value=filter.user placeholderKey="admin.email.logs.filters.user_placeholder"}} {{text-field value=filter.address placeholderKey="admin.email.logs.filters.address_placeholder"}} {{text-field value=filter.type placeholderKey="admin.email.logs.filters.type_placeholder"}} - {{text-field value=filter.skipped_reason placeholderKey="admin.email.logs.filters.skipped_reason_placeholder"}} + {{#each model as |l|}} diff --git a/app/controllers/admin/email_controller.rb b/app/controllers/admin/email_controller.rb index e4e59c19f9b..8fb8c6713b7 100644 --- a/app/controllers/admin/email_controller.rb +++ b/app/controllers/admin/email_controller.rb @@ -18,17 +18,17 @@ class Admin::EmailController < Admin::AdminController end def sent - email_logs = filter_email_logs(EmailLog.sent, params) + email_logs = filter_logs(EmailLog, params) render_serialized(email_logs, EmailLogSerializer) end def skipped - email_logs = filter_email_logs(EmailLog.skipped, params) - render_serialized(email_logs, EmailLogSerializer) + skipped_email_logs = filter_logs(SkippedEmailLog, params) + render_serialized(skipped_email_logs, SkippedEmailLogSerializer) end def bounced - email_logs = filter_email_logs(EmailLog.bounced, params) + email_logs = filter_logs(EmailLog.bounced, params) render_serialized(email_logs, EmailLogSerializer) end @@ -137,20 +137,20 @@ class Admin::EmailController < Admin::AdminController private - def filter_email_logs(email_logs, params) - email_logs = email_logs.includes(:user, post: :topic) + def filter_logs(logs, params) + table_name = logs.table_name + + logs = logs.includes(:user, post: :topic) .references(:user) .order(created_at: :desc) .offset(params[:offset] || 0) .limit(50) - email_logs = email_logs.where("users.username ILIKE ?", "%#{params[:user]}%") if params[:user].present? - email_logs = email_logs.where("email_logs.to_address ILIKE ?", "%#{params[:address]}%") if params[:address].present? - email_logs = email_logs.where("email_logs.email_type ILIKE ?", "%#{params[:type]}%") if params[:type].present? - email_logs = email_logs.where("email_logs.reply_key ILIKE ?", "%#{params[:reply_key]}%") if params[:reply_key].present? - email_logs = email_logs.where("email_logs.skipped_reason ILIKE ?", "%#{params[:skipped_reason]}%") if params[:skipped_reason].present? - - email_logs + logs = logs.where("users.username ILIKE ?", "%#{params[:user]}%") if params[:user].present? + logs = logs.where("#{table_name}.to_address ILIKE ?", "%#{params[:address]}%") if params[:address].present? + logs = logs.where("#{table_name}.email_type ILIKE ?", "%#{params[:type]}%") if params[:type].present? + logs = logs.where("#{table_name}.reply_key ILIKE ?", "%#{params[:reply_key]}%") if params[:reply_key].present? + logs end def filter_incoming_emails(incoming_emails, params) diff --git a/app/jobs/regular/notify_mailing_list_subscribers.rb b/app/jobs/regular/notify_mailing_list_subscribers.rb index f966dbaa196..a75603b172e 100644 --- a/app/jobs/regular/notify_mailing_list_subscribers.rb +++ b/app/jobs/regular/notify_mailing_list_subscribers.rb @@ -42,17 +42,26 @@ module Jobs users.find_each do |user| if Guardian.new(user).can_see?(post) if EmailLog.reached_max_emails?(user) - skip(user.email, user.id, post.id, I18n.t('email_log.exceeded_emails_limit')) + skip(user.email, user.id, post.id, + SkippedEmailLog.reason_types[:exceeded_emails_limit] + ) + next end if user.user_stat.bounce_score >= SiteSetting.bounce_score_threshold - skip(user.email, user.id, post.id, I18n.t('email_log.exceeded_bounces_limit')) + skip(user.email, user.id, post.id, + SkippedEmailLog.reason_types[:exceeded_bounces_limit] + ) + next end if (user.id == post.user_id) && (user.user_option.mailing_list_mode_frequency == 2) - skip(user.email, user.id, post.id, I18n.t('email_log.no_echo_mailing_list_mode')) + skip(user.email, user.id, post.id, + SkippedEmailLog.reason_types[:mailing_list_no_echo_mode] + ) + next end @@ -70,14 +79,13 @@ module Jobs end - def skip(to_address, user_id, post_id, reason) - EmailLog.create!( + def skip(to_address, user_id, post_id, reason_type) + SkippedEmailLog.create!( email_type: 'mailing_list', to_address: to_address, user_id: user_id, post_id: post_id, - skipped: true, - skipped_reason: "[MailingList] #{reason}" + reason_type: reason_type ) end end diff --git a/app/jobs/regular/user_email.rb b/app/jobs/regular/user_email.rb index ac7e1d58d09..30039e905dd 100644 --- a/app/jobs/regular/user_email.rb +++ b/app/jobs/regular/user_email.rb @@ -20,18 +20,21 @@ module Jobs set_skip_context(type, args[:user_id], to_address, args[:post_id]) - return skip(I18n.t("email_log.no_user", user_id: args[:user_id])) unless user + return skip(SkippedEmailLog.reason_types[:user_email_no_user]) unless user if args[:post_id].present? 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? + + unless post.present? + return skip(SkippedEmailLog.reason_types[:user_email_post_not_found]) + end end if args[:notification_id].present? notification = Notification.find_by(id: args[:notification_id]) end - message, skip_reason = message_for_email( + message, skip_reason_type = message_for_email( user, post, type, @@ -42,7 +45,7 @@ module Jobs if message Email::Sender.new(message, type, user).send else - skip_reason + skip_reason_type end end @@ -68,10 +71,12 @@ module Jobs set_skip_context(type, user.id, to_address || user.email, post.try(:id)) - return skip_message(I18n.t("email_log.anonymous_user")) if user.anonymous? + if user.anonymous? + return skip_message(SkippedEmailLog.reason_types[:user_email_anonymous_user]) + end if user.suspended? && !["user_private_message", "account_suspended"].include?(type.to_s) - return skip_message(I18n.t("email_log.suspended_not_pm")) + return skip_message(SkippedEmailLog.reason_types[:user_email_user_suspended_not_pm]) end return if user.staged && type.to_s == "digest" @@ -81,8 +86,10 @@ module Jobs email_args = {} - if post || notification || notification_type - return skip_message(I18n.t('email_log.seen_recently')) if seen_recently && !user.suspended? + if (post || notification || notification_type) && + (seen_recently && !user.suspended?) + + return skip_message(SkippedEmailLog.reason_types[:user_email_seen_recently]) end email_args[:post] = post if post @@ -108,13 +115,13 @@ module Jobs unless user.user_option.email_always? if (notification && notification.read?) || (post && post.seen?(user)) - return skip_message(I18n.t('email_log.notification_already_read')) + return skip_message(SkippedEmailLog.reason_types[:user_email_notification_already_read]) end end end - skip_reason = skip_email_for_post(post, user) - return skip_message(skip_reason) if skip_reason.present? + skip_reason_type = skip_email_for_post(post, user) + return skip_message(skip_reason_type) if skip_reason_type.present? # Make sure that mailer exists raise Discourse::InvalidParameters.new("type=#{type}") unless UserNotifications.respond_to?(type) @@ -123,11 +130,11 @@ module Jobs email_args[:new_email] = user.email if type.to_s == "notify_old_email" if EmailLog.reached_max_emails?(user, type.to_s) - return skip_message(I18n.t('email_log.exceeded_emails_limit')) + return skip_message(SkippedEmailLog.reason_types[:exceeded_emails_limit]) end if !EmailLog::CRITICAL_EMAIL_TYPES.include?(type.to_s) && user.user_stat.bounce_score >= SiteSetting.bounce_score_threshold - return skip_message(I18n.t('email_log.exceeded_bounces_limit')) + return skip_message(SkippedEmailLog.reason_types[:exceeded_bounces_limit]) end if args[:user_history_id] @@ -169,26 +176,38 @@ module Jobs # 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.post_user_deleted') if post.user.blank? - return I18n.t('email_log.post_deleted') if post.user_deleted? - return I18n.t('email_log.user_suspended') if user.suspended? && !post.user&.staff? + if post.topic.blank? + return SkippedEmailLog.reason_types[:user_email_topic_nil] + end + + if post.user.blank? + return SkippedEmailLog.reason_types[:user_email_post_user_deleted] + end + + if post.user_deleted? + return SkippedEmailLog.reason_types[:user_email_post_deleted] + end + + if user.suspended? && !post.user&.staff? + return SkippedEmailLog.reason_types[:user_email_user_suspended] + end already_read = !user.user_option.email_always? && PostTiming.exists?(topic_id: post.topic_id, post_number: post.post_number, user_id: user.id) - return I18n.t('email_log.already_read') if already_read + if already_read + return SkippedEmailLog.reason_types[:user_email_already_read] + end else false end end - def skip(reason) - EmailLog.create!( + def skip(reason_type) + SkippedEmailLog.create!( email_type: @skip_context[:type], to_address: @skip_context[:to_address], user_id: @skip_context[:user_id], post_id: @skip_context[:post_id], - skipped: true, - skipped_reason: "[UserEmail] #{reason}", + reason_type: reason_type ) end diff --git a/app/jobs/scheduled/clean_up_email_logs.rb b/app/jobs/scheduled/clean_up_email_logs.rb index 48421c1cfa2..68505b0aaca 100644 --- a/app/jobs/scheduled/clean_up_email_logs.rb +++ b/app/jobs/scheduled/clean_up_email_logs.rb @@ -11,6 +11,8 @@ module Jobs EmailLog.where(reply_key: nil) .where("created_at < ?", threshold) .delete_all + + SkippedEmailLog.where("created_at < ?", threshold).delete_all end end diff --git a/app/models/skipped_email_log.rb b/app/models/skipped_email_log.rb new file mode 100644 index 00000000000..ee07aac8dcc --- /dev/null +++ b/app/models/skipped_email_log.rb @@ -0,0 +1,85 @@ +class SkippedEmailLog < ActiveRecord::Base + belongs_to :email_log + + belongs_to :user + belongs_to :post + has_one :topic, through: :post + + validates :email_type, :to_address, :reason_type, presence: true + + validates :custom_reason, presence: true, if: -> { is_custom? } + validates :custom_reason, absence: true, if: -> { !is_custom? } + validate :ensure_valid_reason_type + + def self.reason_types + Enum.new( + custom: 1, + exceeded_emails_limit: 2, + exceeded_bounces_limit: 3, + mailing_list_no_echo_mode: 4, + user_email_no_user: 5, + user_email_post_not_found: 6, + user_email_anonymous_user: 7, + user_email_user_suspended_not_pm: 8, + user_email_seen_recently: 9, + user_email_notification_already_read: 10, + user_email_topic_nil: 11, + user_email_post_user_deleted: 12, + user_email_post_deleted: 13, + user_email_user_suspended: 14, + user_email_already_read: 15, + sender_message_blank: 16, + sender_message_to_blank: 17, + sender_text_part_body_blank: 18, + sender_body_blank: 19 + ) + end + + def reason + if is_custom? + self.custom_reason + else + type = self.reason_type + + I18n.t( + "skipped_email_log.#{SkippedEmailLog.reason_types[type]}", + user_id: self.user_id, + post_id: self.post_id + ) + end + end + + private + + def is_custom? + self.reason_type == self.class.reason_types[:custom] + end + + def ensure_valid_reason_type + unless self.class.reason_types[self.reason_type] + self.errors.add(:reason_type, :invalid) + end + end +end + +# == Schema Information +# +# Table name: skipped_email_logs +# +# id :bigint(8) not null, primary key +# email_type :string not null +# to_address :string not null +# user_id :integer +# post_id :integer +# reason_type :integer not null +# custom_reason :text +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# index_skipped_email_logs_on_created_at (created_at) +# index_skipped_email_logs_on_post_id (post_id) +# index_skipped_email_logs_on_reason_type (reason_type) +# index_skipped_email_logs_on_user_id (user_id) +# diff --git a/app/serializers/concerns/email_logs_mixin.rb b/app/serializers/concerns/email_logs_mixin.rb new file mode 100644 index 00000000000..1e3470e2b73 --- /dev/null +++ b/app/serializers/concerns/email_logs_mixin.rb @@ -0,0 +1,29 @@ +module EmailLogsMixin + def self.included(klass) + klass.attributes :id, + :to_address, + :email_type, + :user_id, + :created_at, + :post_url, + :post_description + + klass.has_one :user, serializer: BasicUserSerializer, embed: :objects + end + + def post_url + object.post.url + end + + def include_post_url? + object.post.present? + end + + def include_post_description? + object.post.present? && object.post.topic.present? + end + + def post_description + "#{object.post.topic.title} ##{object.post.post_number}" + end +end diff --git a/app/serializers/email_log_serializer.rb b/app/serializers/email_log_serializer.rb index 1d83496b85a..8f21bb3a391 100644 --- a/app/serializers/email_log_serializer.rb +++ b/app/serializers/email_log_serializer.rb @@ -1,37 +1,8 @@ class EmailLogSerializer < ApplicationSerializer + include EmailLogsMixin - attributes :id, - :reply_key, - :to_address, - :email_type, - :user_id, - :created_at, - :skipped, - :skipped_reason, - :post_url, - :post_description, + attributes :reply_key, :bounced has_one :user, serializer: BasicUserSerializer, embed: :objects - - def include_skipped_reason? - object.skipped - end - - def post_url - object.post.url - end - - def include_post_url? - object.post.present? - end - - def include_post_description? - object.post.present? && object.post.topic.present? - end - - def post_description - "#{object.post.topic.title} ##{object.post.post_number}" - end - end diff --git a/app/serializers/skipped_email_log_serializer.rb b/app/serializers/skipped_email_log_serializer.rb new file mode 100644 index 00000000000..9527f6f8c76 --- /dev/null +++ b/app/serializers/skipped_email_log_serializer.rb @@ -0,0 +1,9 @@ +class SkippedEmailLogSerializer < ApplicationSerializer + include EmailLogsMixin + + attributes :skipped_reason + + def skipped_reason + object.reason + end +end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 544fb39c7c6..111713f7735 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -3352,7 +3352,6 @@ en: address_placeholder: "name@example.com" type_placeholder: "digest, signup..." reply_key_placeholder: "reply key" - skipped_reason_placeholder: "reason" moderation_history: performed_by: "Performed By" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 33d6865f80e..2e07af6bd18 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -3163,25 +3163,25 @@ en: sockpuppet: "A new user created a topic, and another new user at the same IP address (%{ip_address}) replied. See the `flag_sockpuppets` site setting." spam_hosts: "This new user tried to create multiple posts with links to the same domain (%{domain}). See the `newuser_spam_host_threshold` site setting." - email_log: - post_user_deleted: "User of the post has been deleted." - no_user: "Can't find user with id %{user_id}" - anonymous_user: "User is anonymous" - suspended_not_pm: "User is suspended, not a 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" + skipped_email_log: exceeded_emails_limit: "Exceeded max_emails_per_day_per_user" exceeded_bounces_limit: "Exceeded bounce_score_threshold" - 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" - no_echo_mailing_list_mode: "Mailing list notifications disabled for user's own posts" + mailing_list_no_echo_mode: "Mailing list notifications disabled for user's own posts" + user_email_no_user: "Can't find user with id %{user_id}" + user_email_post_not_found: "Can't find a post with id %{post_id}" + user_email_anonymous_user: "User is anonymous" + user_email_user_suspended_not_pm: "User is suspended, not a message" + user_email_seen_recently: "User was seen recently" + user_email_notification_already_read: "The notification this email is about has already been read" + user_email_notification_topic_nil: "post.topic is nil" + user_email_post_user_deleted: "User of the post has been deleted." + user_email_post_deleted: "post was deleted by the author" + user_email_user_suspended: "user was suspended" + user_email_already_read: "user has already read this post" + sender_message_blank: "message is blank" + sender_message_to_blank: "message.to is blank" + sender_text_part_body_blank: "text_part.body is blank" + sender_text_body_blank: "body is blank" color_schemes: base_theme_name: "Base" diff --git a/db/migrate/20180720054856_create_skipped_email_logs.rb b/db/migrate/20180720054856_create_skipped_email_logs.rb new file mode 100644 index 00000000000..2a4ee6ddbff --- /dev/null +++ b/db/migrate/20180720054856_create_skipped_email_logs.rb @@ -0,0 +1,47 @@ +require 'migration/column_dropper' + +class CreateSkippedEmailLogs < ActiveRecord::Migration[5.2] + def change + create_table :skipped_email_logs do |t| + t.string :email_type, null: false + t.string :to_address, null: false + t.integer :user_id + t.integer :post_id + t.integer :reason_type, null: false + t.text :custom_reason + t.timestamps + end + + add_index :skipped_email_logs, :created_at + add_index :skipped_email_logs, :user_id + add_index :skipped_email_logs, :post_id + add_index :skipped_email_logs, :reason_type + + sql = <<~SQL + INSERT INTO skipped_email_logs ( + email_type, + to_address, + user_id, + post_id, + reason_type, + custom_reason, + created_at, + updated_at + ) SELECT + email_type, + to_address, + user_id, + post_id, + 1, + skipped_reason, + created_at, + updated_at + FROM email_logs + WHERE skipped IS TRUE + SQL + + execute(sql) + + Migration::ColumnDropper.mark_readonly('email_logs', 'skipped_reason') + end +end diff --git a/lib/email/sender.rb b/lib/email/sender.rb index 7dca1fa8550..c503c750a0c 100644 --- a/lib/email/sender.rb +++ b/lib/email/sender.rb @@ -27,8 +27,8 @@ module Email return if ActionMailer::Base::NullMail === @message return if ActionMailer::Base::NullMail === (@message.message rescue nil) - return skip(I18n.t('email_log.message_blank')) if @message.blank? - return skip(I18n.t('email_log.message_to_blank')) if @message.to.blank? + return skip(SkippedEmailLog.reason_types[:sender_message_blank]) if @message.blank? + return skip(SkippedEmailLog.reason_types[:sender_message_to_blank]) if @message.to.blank? if SiteSetting.disable_emails == "non-staff" user = User.find_by_email(to_address) @@ -36,9 +36,13 @@ module Email end if @message.text_part - return skip(I18n.t('email_log.text_part_body_blank')) if @message.text_part.body.to_s.blank? + if @message.text_part.body.to_s.blank? + return skip(SkippedEmailLog.reason_types[:sender_text_part_body_blank]) + end else - return skip(I18n.t('email_log.body_blank')) if @message.body.to_s.blank? + if @message.body.to_s.blank? + return skip(SkippedEmailLog.reason_types[:sender_body_blank]) + end end @message.charset = 'UTF-8' @@ -186,7 +190,7 @@ module Email begin @message.deliver_now rescue *SMTP_CLIENT_ERRORS => e - return skip(e.message) + return skip(SkippedEmailLog.reason_types[:custom], custom_reason: e.message) end # Save and return the email log @@ -222,14 +226,16 @@ module Email header.value end - def skip(reason) - EmailLog.create!( + def skip(reason_type, custom_reason: nil) + attributes = { email_type: @email_type, to_address: to_address, - user_id: @user.try(:id), - skipped: true, - skipped_reason: "[Sender] #{reason}" - ) + user_id: @user&.id, + reason_type: reason_type + } + + attributes[:custom_reason] = custom_reason if custom_reason + SkippedEmailLog.create!(attributes) end def merge_json_x_header(name, value) diff --git a/spec/fabricators/skipped_email_log_fabricator.rb b/spec/fabricators/skipped_email_log_fabricator.rb new file mode 100644 index 00000000000..ec4692a6d75 --- /dev/null +++ b/spec/fabricators/skipped_email_log_fabricator.rb @@ -0,0 +1,5 @@ +Fabricator(:skipped_email_log) do + to_address { sequence(:address) { |i| "blah#{i}@example.com" } } + email_type :invite + reason_type SkippedEmailLog.reason_types[:exceeded_emails_limit] +end diff --git a/spec/jobs/clean_up_email_logs_spec.rb b/spec/jobs/clean_up_email_logs_spec.rb index fcddbfae30a..177a7414cde 100644 --- a/spec/jobs/clean_up_email_logs_spec.rb +++ b/spec/jobs/clean_up_email_logs_spec.rb @@ -7,17 +7,22 @@ describe Jobs::CleanUpEmailLogs do Fabricate(:email_log, created_at: 2.years.ago) Fabricate(:email_log, created_at: 2.weeks.ago) Fabricate(:email_log, created_at: 2.days.ago) + + Fabricate(:skipped_email_log, created_at: 2.years.ago) + Fabricate(:skipped_email_log) end it "removes old email logs without a reply_key" do Jobs::CleanUpEmailLogs.new.execute({}) expect(EmailLog.count).to eq(3) + expect(SkippedEmailLog.count).to eq(1) end it "does not remove old email logs when delete_email_logs_after_days is 0" do SiteSetting.delete_email_logs_after_days = 0 Jobs::CleanUpEmailLogs.new.execute({}) expect(EmailLog.count).to eq(4) + expect(SkippedEmailLog.count).to eq(2) end end diff --git a/spec/jobs/notify_mailing_list_subscribers_spec.rb b/spec/jobs/notify_mailing_list_subscribers_spec.rb index 57692656e97..ec42df7bd4b 100644 --- a/spec/jobs/notify_mailing_list_subscribers_spec.rb +++ b/spec/jobs/notify_mailing_list_subscribers_spec.rb @@ -129,7 +129,13 @@ describe Jobs::NotifyMailingListSubscribers do Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id) UserNotifications.expects(:mailing_list_notify).with(mailing_list_user, post).never - expect(EmailLog.where(user: mailing_list_user, skipped: true).count).to eq(1) + expect(SkippedEmailLog.exists?( + email_type: "mailing_list", + user: mailing_list_user, + post: post, + to_address: mailing_list_user.email, + reason_type: SkippedEmailLog.reason_types[:exceeded_emails_limit] + )).to eq(true) end end @@ -141,7 +147,13 @@ describe Jobs::NotifyMailingListSubscribers do Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id) UserNotifications.expects(:mailing_list_notify).with(mailing_list_user, post).never - expect(EmailLog.where(user: mailing_list_user, skipped: true).count).to eq(1) + expect(SkippedEmailLog.exists?( + email_type: "mailing_list", + user: mailing_list_user, + post: post, + to_address: mailing_list_user.email, + reason_type: SkippedEmailLog.reason_types[:exceeded_bounces_limit] + )).to eq(true) end end diff --git a/spec/jobs/user_email_spec.rb b/spec/jobs/user_email_spec.rb index 67876f14ad3..4f26d500eea 100644 --- a/spec/jobs/user_email_spec.rb +++ b/spec/jobs/user_email_spec.rb @@ -79,38 +79,46 @@ describe Jobs::UserEmail do end context "email_log" do + let(:post) { Fabricate(:post) } before do SiteSetting.editing_grace_period = 0 - Fabricate(:post) + post end 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) + expect do + Jobs::UserEmail.new.execute(type: :digest, user_id: user.id,) + end.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) + expect(email_log.user).to eq(user) + expect(email_log.post).to eq(nil) # last_emailed_at should have changed expect(email_log.user.last_emailed_at).to_not eq(last_emailed_at) end - it "creates an email log when the mail is skipped" do + it "creates a skipped email log when the mail is skipped" do last_emailed_at = user.last_emailed_at 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) + expect do + Jobs::UserEmail.new.execute(type: :digest, user_id: user.id) + end.to change { SkippedEmailLog.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) + expect(SkippedEmailLog.exists?( + email_type: "digest", + user: user, + post: nil, + to_address: user.email, + reason_type: SkippedEmailLog.reason_types[:user_email_user_suspended_not_pm] + )).to eq(true) # last_emailed_at doesn't change - expect(email_log.user.last_emailed_at).to eq(last_emailed_at) + expect(user.last_emailed_at).to eq(last_emailed_at) end end @@ -205,8 +213,15 @@ describe Jobs::UserEmail do notification_data_hash: notification.data_hash ) - expect(message).to eq nil - expect(err.skipped_reason).to match(/notification.*already/) + expect(message).to eq(nil) + + expect(SkippedEmailLog.exists?( + email_type: "user_mentioned", + user: user, + post: post, + to_address: user.email, + reason_type: SkippedEmailLog.reason_types[:user_email_notification_already_read] + )).to eq(true) end it "does send the email if the notification has been seen but the user is set for email_always" do @@ -230,20 +245,59 @@ describe Jobs::UserEmail do end it "does not send notification if limit is reached" do - Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_id: notification.id, post_id: post.id) - expect(EmailLog.where(user_id: user.id, skipped: true).count).to eq(1) + expect do + Jobs::UserEmail.new.execute( + type: :user_mentioned, + user_id: user.id, + notification_id: notification.id, + post_id: post.id + ) + end.to change { SkippedEmailLog.count }.by(1) + + expect(SkippedEmailLog.exists?( + email_type: "user_mentioned", + user: user, + post: post, + to_address: user.email, + reason_type: SkippedEmailLog.reason_types[:exceeded_emails_limit] + )).to eq(true) end it "sends critical email" do - Jobs::UserEmail.new.execute(type: :forgot_password, user_id: user.id, notification_id: notification.id, post_id: post.id) - expect(EmailLog.where(user_id: user.id, skipped: true).count).to eq(0) + expect do + Jobs::UserEmail.new.execute( + type: :forgot_password, + user_id: user.id, + notification_id: notification.id, + ) + end.to change { EmailLog.count }.by(1) + + expect(EmailLog.exists?( + email_type: "forgot_password", + user: user, + )).to eq(true) end end it "does not send notification if bounce threshold is reached" do user.user_stat.update(bounce_score: SiteSetting.bounce_score_threshold) - Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_id: notification.id, post_id: post.id) - expect(EmailLog.where(user_id: user.id, skipped: true).count).to eq(1) + + expect do + Jobs::UserEmail.new.execute( + type: :user_mentioned, + user_id: user.id, + notification_id: notification.id, + post_id: post.id + ) + end.to change { SkippedEmailLog.count }.by(1) + + expect(SkippedEmailLog.exists?( + email_type: "user_mentioned", + user: user, + post: post, + to_address: user.email, + reason_type: SkippedEmailLog.reason_types[:exceeded_bounces_limit] + )).to eq(true) end it "doesn't send the mail if the user is using individual mailing list mode" do diff --git a/spec/models/skipped_email_log_spec.rb b/spec/models/skipped_email_log_spec.rb new file mode 100644 index 00000000000..a498e49f4a2 --- /dev/null +++ b/spec/models/skipped_email_log_spec.rb @@ -0,0 +1,103 @@ +require 'rails_helper' + +RSpec.describe SkippedEmailLog, type: :model do + let(:custom_skipped_email_log) do + Fabricate.build(:skipped_email_log, + reason_type: SkippedEmailLog.reason_types[:custom] + ) + end + + let(:skipped_email_log) { Fabricate.build(:skipped_email_log) } + + describe 'validations' do + it { is_expected.to validate_presence_of(:email_type) } + it { is_expected.to validate_presence_of(:to_address) } + it { is_expected.to validate_presence_of(:reason_type) } + + describe '#reason_type' do + describe 'when reason_type is not valid' do + it 'should not be valid' do + skipped_email_log.reason_type = 999999 + + expect(skipped_email_log.valid?).to eq(false) + expect(skipped_email_log.errors.messages).to include(:reason_type) + end + end + end + + describe '#custom_reason' do + describe 'when log is a custom reason type' do + describe 'when custom reason is blank' do + it 'should not be valid' do + expect(custom_skipped_email_log.valid?).to eq(false) + + expect(custom_skipped_email_log.errors.messages) + .to include(:custom_reason) + end + end + + describe 'when custom reason is not blank' do + it 'should be valid' do + custom_skipped_email_log.custom_reason = 'test' + + expect(custom_skipped_email_log.valid?).to eq(true) + end + end + end + + describe 'when log is not a custom reason type' do + describe 'when custom reason is blank' do + it 'should be valid' do + expect(skipped_email_log.valid?).to eq(true) + end + end + + describe 'when custom reason is not blank' do + it 'should not be valid' do + skipped_email_log.custom_reason = 'test' + + expect(skipped_email_log.valid?).to eq(false) + expect(skipped_email_log.errors.messages).to include(:custom_reason) + end + end + end + end + end + + describe '.reason_types' do + describe "verify enum sequence" do + it 'should return the right sequence' do + expect(SkippedEmailLog.reason_types[:custom]).to eq(1) + expect(SkippedEmailLog.reason_types[:user_email_already_read]).to eq(15) + end + end + end + + describe '#reason' do + describe 'for a custom log' do + it 'should return the right output' do + custom_skipped_email_log.custom_reason = 'test' + expect(custom_skipped_email_log.reason).to eq('test') + end + end + + describe 'for a non custom log' do + it 'should return the right output' do + expect(skipped_email_log.reason).to eq(" + #{I18n.t('skipped_email_log.exceeded_emails_limit')} + ".strip) + + skipped_email_log.reason_type = + SkippedEmailLog.reason_types[:user_email_no_user] + + skipped_email_log.user_id = 9999 + + expect(skipped_email_log.reason).to eq(" + #{I18n.t( + 'skipped_email_log.user_email_no_user', user_id: 9999 + )} + ".strip) + end + end + end +end diff --git a/spec/requests/admin/email_controller_spec.rb b/spec/requests/admin/email_controller_spec.rb index ff8b46e71b6..681dcdcc5da 100644 --- a/spec/requests/admin/email_controller_spec.rb +++ b/spec/requests/admin/email_controller_spec.rb @@ -39,9 +39,34 @@ describe Admin::EmailController do end describe '#skipped' do + let(:user) { Fabricate(:user) } + let!(:log1) { Fabricate(:skipped_email_log, user: user) } + let!(:log2) { Fabricate(:skipped_email_log) } + it "succeeds" do get "/admin/email/skipped.json" + expect(response.status).to eq(200) + + logs = JSON.parse(response.body) + + expect(logs.first["id"]).to eq(log2.id) + expect(logs.last["id"]).to eq(log1.id) + end + + describe 'when filtered by username' do + it 'should return the right response' do + get "/admin/email/skipped.json", params: { + user: user.username + } + + expect(response.status).to eq(200) + + logs = JSON.parse(response.body) + + expect(logs.count).to eq(1) + expect(logs.first["id"]).to eq(log1.id) + end end end @@ -54,6 +79,7 @@ describe Admin::EmailController do context 'with an email address' do it 'enqueues a test email job' do post "/admin/email/test.json", params: { email_address: 'eviltrout@test.domain' } + expect(response.status).to eq(200) expect(ActionMailer::Base.deliveries.map(&:to).flatten).to include('eviltrout@test.domain') end