From c095304d6df578edff399cf19876385e9aabaf06 Mon Sep 17 00:00:00 2001 From: Sam Date: Wed, 23 Mar 2016 15:08:34 +1100 Subject: [PATCH] FEATURE: limit daily emails per user to 100 per day via site setting - controlled via max_emails_per_day_per_user, 0 to disable - when limit is reached user is warned --- .../notify_mailing_list_subscribers.rb | 15 +++++++++-- app/jobs/regular/user_email.rb | 4 +++ app/mailers/user_notifications.rb | 9 ++++++- app/models/email_log.rb | 10 +++++++ app/views/email/notification.html.erb | 7 +++-- config/locales/server.en.yml | 5 ++++ config/site_settings.yml | 1 + .../notify_mailing_list_subscribers_spec.rb | 27 ++++++++++++++++++- spec/jobs/user_email_spec.rb | 13 ++++++++- spec/mailers/user_notifications_spec.rb | 25 +++++++++++++++++ spec/models/email_log_spec.rb | 15 +++++++++++ 11 files changed, 124 insertions(+), 7 deletions(-) diff --git a/app/jobs/regular/notify_mailing_list_subscribers.rb b/app/jobs/regular/notify_mailing_list_subscribers.rb index 540b472c233..c273ce0e47b 100644 --- a/app/jobs/regular/notify_mailing_list_subscribers.rb +++ b/app/jobs/regular/notify_mailing_list_subscribers.rb @@ -35,8 +35,19 @@ module Jobs users.each do |user| if Guardian.new(user).can_see?(post) begin - message = UserNotifications.mailing_list_notify(user, post) - Email::Sender.new(message, :mailing_list, user).send if message + if EmailLog.reached_max_emails?(user) + EmailLog.create!( + email_type: 'mailing_list', + to_address: user.email, + user_id: user.id, + post_id: post.id, + skipped: true, + skipped_reason: "[MailingList] #{I18n.t('email_log.exceeded_limit')}" + ) + else + message = UserNotifications.mailing_list_notify(user, post) + Email::Sender.new(message, :mailing_list, user).send if message + end rescue => e Discourse.handle_job_exception(e, error_context(args, "Sending post to mailing list subscribers", { user_id: user.id, user_email: user.email })) end diff --git a/app/jobs/regular/user_email.rb b/app/jobs/regular/user_email.rb index ca7b73dc710..160629dfefe 100644 --- a/app/jobs/regular/user_email.rb +++ b/app/jobs/regular/user_email.rb @@ -113,6 +113,10 @@ module Jobs email_args[:new_email] = user.email end + if EmailLog.reached_max_emails?(user) + return skip_message(I18n.t('email_log.exceeded_limit')) + end + message = UserNotifications.send(type, user, email_args) # Update the to address if we have a custom one diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 943b2a127a5..f549b606c2f 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -317,6 +317,11 @@ class UserNotifications < ActionMailer::Base end end + reached_limit = SiteSetting.max_emails_per_day_per_user > 0 + reached_limit &&= (EmailLog.where(user_id: user.id, skipped: false) + .where('created_at > ?', 1.day.ago) + .count) >= (SiteSetting.max_emails_per_day_per_user-1) + topic_excerpt = "" if opts[:use_template_html] topic_excerpt = post.excerpt.gsub("\n", " ") if post.is_first_post? && post.excerpt @@ -326,6 +331,7 @@ class UserNotifications < ActionMailer::Base template: 'email/notification', format: :html, locals: { context_posts: context_posts, + reached_limit: reached_limit, post: post, in_reply_to_post: in_reply_to_post, classes: RTL.new(user).css_class @@ -339,10 +345,11 @@ class UserNotifications < ActionMailer::Base template << "_staged" if user.staged? end + email_opts = { topic_title: title, topic_excerpt: topic_excerpt, - message: email_post_markdown(post), + message: email_post_markdown(post) + (reached_limit ? "\n\n#{I18n.t "user_notifications.reached_limit", count: SiteSetting.max_emails_per_day_per_user}" : ""), url: post.url, post_id: post.id, topic_id: post.topic_id, diff --git a/app/models/email_log.rb b/app/models/email_log.rb index 7a8e9ef6673..eb6c1a31738 100644 --- a/app/models/email_log.rb +++ b/app/models/email_log.rb @@ -13,6 +13,16 @@ class EmailLog < ActiveRecord::Base User.where(id: user_id).update_all("last_emailed_at = CURRENT_TIMESTAMP") if user_id.present? && !skipped end + def self.reached_max_emails?(user) + return false if SiteSetting.max_emails_per_day_per_user == 0 + + count = sent.where('created_at > ?', 1.day.ago) + .where(user_id: user.id) + .count + + count >= SiteSetting.max_emails_per_day_per_user + end + def self.count_per_day(start_date, end_date) sent.where("created_at BETWEEN ? AND ?", start_date, end_date) .group("DATE(created_at)") diff --git a/app/views/email/notification.html.erb b/app/views/email/notification.html.erb index 363fa65ca44..d4c200807bf 100644 --- a/app/views/email/notification.html.erb +++ b/app/views/email/notification.html.erb @@ -10,12 +10,12 @@ <% end %> <% if in_reply_to_post.present? %> -

<%= t "user_notifications.in_reply_to" %>

+

<%= t "user_notifications.in_reply_to" %>

<%= render partial: 'email/post', locals: { post: in_reply_to_post, use_excerpt: true} %> <% end %> <% if context_posts.present? %> -

<%= t "user_notifications.previous_discussion" %>

+

<%= t "user_notifications.previous_discussion" %>

<% context_posts.each do |p| %> <%= render partial: 'email/post', locals: { post: p, use_excerpt: false } %> <% end %> @@ -23,6 +23,9 @@
+ <% if reached_limit %> +
<%= t "user_notifications.reached_limit", count: SiteSetting.max_emails_per_day_per_user %>
+ <% end %>
%{respond_instructions}
%{unsubscribe_link}%{unsubscribe_via_email_link}
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index c6dc3a4c450..6d6505b7811 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1161,6 +1161,7 @@ en: unsubscribe_via_email_footer: "Attach an unsubscribe link to the footer of sent emails" delete_email_logs_after_days: "Delete email logs after (N) days. 0 to keep indefinitely" + max_emails_per_day_per_user: "Maximum number of emails to send users per day. 0 to disable the limit" manual_polling_enabled: "Push emails using the API for email replies." pop3_polling_enabled: "Poll via POP3 for email replies." @@ -2037,6 +2038,9 @@ en: user_notifications: previous_discussion: "Previous Replies" + reached_limit: + one: "WARNING: you reached the limit of daily emails. Further email notifications will be suppressed." + other: "WARNING: you reached the limit of %{count} daily emails. Further email notifications will be suppressed." in_reply_to: "In Reply To" unsubscribe: title: "Unsubscribe" @@ -2363,6 +2367,7 @@ en: post_deleted: "post was deleted by the author" user_suspended: "user was suspended" already_read: "user has already read this post" + exceeded_limit: "Exceeded max_emails_per_day_per_user" message_blank: "message is blank" message_to_blank: "message.to is blank" text_part_body_blank: "text_part.body is blank" diff --git a/config/site_settings.yml b/config/site_settings.yml index dd1e45019a6..096b4eab5ad 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -559,6 +559,7 @@ email: delete_email_logs_after_days: default: 365 min: 0 + max_emails_per_day_per_user: 100 files: max_image_size_kb: 3072 diff --git a/spec/jobs/notify_mailing_list_subscribers_spec.rb b/spec/jobs/notify_mailing_list_subscribers_spec.rb index d3a0022f1fa..f63b4225ccc 100644 --- a/spec/jobs/notify_mailing_list_subscribers_spec.rb +++ b/spec/jobs/notify_mailing_list_subscribers_spec.rb @@ -3,9 +3,34 @@ require "rails_helper" describe Jobs::NotifyMailingListSubscribers do context "with mailing list on" do - before { SiteSetting.stubs(:default_email_mailing_list_mode).returns(true) } + before { SiteSetting.default_email_mailing_list_mode = true } let(:user) { Fabricate(:user) } + context "SiteSetting.max_emails_per_day_per_user" do + + it 'stops sending mail once limit is reached' do + SiteSetting.max_emails_per_day_per_user = 2 + post = Fabricate(:post) + + user.email_logs.create(email_type: 'blah', to_address: user.email, user_id: user.id) + user.email_logs.create(email_type: 'blah', to_address: user.email, user_id: user.id) + + Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id) + expect(EmailLog.where(user_id: user.id, skipped: true).count).to eq(1) + end + end + + context "totally skipped if mailing list mode disabled" do + + it "sends no email to the user" do + SiteSetting.disable_mailing_list_mode = true + + post = Fabricate(:post) + Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id) + expect(EmailLog.count).to eq(0) + end + end + context "with a valid post" do let!(:post) { Fabricate(:post, user: user) } diff --git a/spec/jobs/user_email_spec.rb b/spec/jobs/user_email_spec.rb index 92fd5abd2a3..e7405bade79 100644 --- a/spec/jobs/user_email_spec.rb +++ b/spec/jobs/user_email_spec.rb @@ -193,6 +193,17 @@ describe Jobs::UserEmail do Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_id: notification.id) end + it "does not send notification if limit is reached" do + SiteSetting.max_emails_per_day_per_user = 2 + + user.email_logs.create(email_type: 'blah', to_address: user.email, user_id: user.id) + user.email_logs.create(email_type: 'blah', to_address: user.email, user_id: user.id) + + 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) + end + it "doesn't send the mail if the user is using mailing list mode" do Email::Sender.any_instance.expects(:send).never user.user_option.update_column(:mailing_list_mode, true) @@ -205,7 +216,7 @@ describe Jobs::UserEmail do # When post does not have a topic post = Fabricate(:post) post.topic.destroy - Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_type: "posted", post_id: post) + Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_type: "posted", post_id: post.id) end it "doesn't send the email if the post has been user deleted" do diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb index ebc9b27e4c5..00ceb74a3af 100644 --- a/spec/mailers/user_notifications_spec.rb +++ b/spec/mailers/user_notifications_spec.rb @@ -243,6 +243,31 @@ describe UserNotifications do end end + + it 'adds a warning when mail limit is reached' do + SiteSetting.max_emails_per_day_per_user = 2 + user = Fabricate(:user) + user.email_logs.create(email_type: 'blah', to_address: user.email, user_id: user.id, skipped: false) + + post = Fabricate(:post) + reply = Fabricate(:post, topic_id: post.topic_id) + + notification = Fabricate(:notification, topic_id: post.topic_id, post_number: reply.post_number, + user: post.user, data: {original_username: 'bob'}.to_json) + + mail = UserNotifications.user_replied( + user, + post: reply, + notification_type: notification.notification_type, + notification_data_hash: notification.data_hash + ) + + # WARNING: you reached the limit of 100 email notifications per day. Further emails will be suppressed. + # Consider watching less topics or disabling mailing list mode. + expect(mail.html_part.to_s).to match("WARNING: ") + expect(mail.body.to_s).to match("WARNING: ") + end + def expects_build_with(condition) UserNotifications.any_instance.expects(:build_email).with(user.email, condition) mailer = UserNotifications.send(mail_type, user, diff --git a/spec/models/email_log_spec.rb b/spec/models/email_log_spec.rb index 8c33e2b5f8b..3b5ca7df465 100644 --- a/spec/models/email_log_spec.rb +++ b/spec/models/email_log_spec.rb @@ -26,6 +26,21 @@ describe EmailLog do end end + describe '#reached_max_emails?' do + it "tracks when max emails are reached" do + SiteSetting.max_emails_per_day_per_user = 2 + user.email_logs.create(email_type: 'blah', to_address: user.email, user_id: user.id, skipped: true) + user.email_logs.create(email_type: 'blah', to_address: user.email, user_id: user.id) + user.email_logs.create(email_type: 'blah', to_address: user.email, user_id: user.id, created_at: 3.days.ago) + + expect(EmailLog.reached_max_emails?(user)).to eq(false) + + user.email_logs.create(email_type: 'blah', to_address: user.email, user_id: user.id) + + expect(EmailLog.reached_max_emails?(user)).to eq(true) + end + end + describe '#count_per_day' do it "counts sent emails" do user.email_logs.create(email_type: 'blah', to_address: user.email)