From 8bdf14834b4334cbe4da6bed5081d32996b7cd2e Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Tue, 21 Aug 2018 11:14:43 +0800 Subject: [PATCH] PERF: Restrict number of skipped email log for `Jobs::UserEmail`. --- app/jobs/concerns/skippable.rb | 28 +++++++++++++++++++ .../notify_mailing_list_subscribers.rb | 15 ++-------- app/jobs/regular/user_email.rb | 3 +- .../notify_mailing_list_subscribers_spec.rb | 14 +++++++--- spec/jobs/user_email_spec.rb | 25 +++++++++++++---- 5 files changed, 62 insertions(+), 23 deletions(-) create mode 100644 app/jobs/concerns/skippable.rb diff --git a/app/jobs/concerns/skippable.rb b/app/jobs/concerns/skippable.rb new file mode 100644 index 00000000000..a8c14824606 --- /dev/null +++ b/app/jobs/concerns/skippable.rb @@ -0,0 +1,28 @@ +module Skippable + extend ActiveSupport::Concern + + def create_skipped_email_log(email_type:, + to_address:, + user_id:, + post_id:, + reason_type:) + + attributes = { + email_type: email_type, + to_address: to_address, + user_id: user_id, + post_id: post_id, + reason_type: reason_type + } + + if reason_type == SkippedEmailLog.reason_types[:exceeded_emails_limit] + exists = SkippedEmailLog.exists?({ + created_at: (Time.zone.now.beginning_of_day..Time.zone.now.end_of_day) + }.merge!(attributes.except(:post_id))) + + return if exists + end + + SkippedEmailLog.create!(attributes) + end +end diff --git a/app/jobs/regular/notify_mailing_list_subscribers.rb b/app/jobs/regular/notify_mailing_list_subscribers.rb index 811d75b6253..747f935a0cd 100644 --- a/app/jobs/regular/notify_mailing_list_subscribers.rb +++ b/app/jobs/regular/notify_mailing_list_subscribers.rb @@ -3,6 +3,7 @@ require_dependency 'post' module Jobs class NotifyMailingListSubscribers < Jobs::Base + include Skippable sidekiq_options queue: 'low' @@ -80,23 +81,13 @@ module Jobs end def skip(to_address, user_id, post_id, reason_type) - attributes = { + create_skipped_email_log( email_type: 'mailing_list', to_address: to_address, user_id: user_id, post_id: post_id, reason_type: reason_type - } - - if reason_type == SkippedEmailLog.reason_types[:exceeded_emails_limit] - exists = SkippedEmailLog.exists?({ - created_at: (Time.zone.now.beginning_of_day..Time.zone.now.end_of_day) - }.merge!(attributes.except(:post_id))) - - return if exists - end - - SkippedEmailLog.create!(attributes) + ) end end end diff --git a/app/jobs/regular/user_email.rb b/app/jobs/regular/user_email.rb index 30039e905dd..28bb418a30f 100644 --- a/app/jobs/regular/user_email.rb +++ b/app/jobs/regular/user_email.rb @@ -5,6 +5,7 @@ module Jobs # Asynchronously send an email to a user class UserEmail < Jobs::Base + include Skippable sidekiq_options queue: 'low' @@ -202,7 +203,7 @@ module Jobs end def skip(reason_type) - SkippedEmailLog.create!( + create_skipped_email_log( email_type: @skip_context[:type], to_address: @skip_context[:to_address], user_id: @skip_context[:user_id], diff --git a/spec/jobs/notify_mailing_list_subscribers_spec.rb b/spec/jobs/notify_mailing_list_subscribers_spec.rb index 9af92d9e863..6d94894c22a 100644 --- a/spec/jobs/notify_mailing_list_subscribers_spec.rb +++ b/spec/jobs/notify_mailing_list_subscribers_spec.rb @@ -136,7 +136,7 @@ describe Jobs::NotifyMailingListSubscribers do end Jobs::NotifyMailingListSubscribers.new.execute( - post_id: Fabricate(:post, user: user) + post_id: Fabricate(:post, user: user).id ) end.to change { SkippedEmailLog.count }.by(1) @@ -148,13 +148,19 @@ describe Jobs::NotifyMailingListSubscribers do reason_type: SkippedEmailLog.reason_types[:exceeded_emails_limit] )).to eq(true) - freeze_time(Time.zone.now + 1.day) + freeze_time(Time.zone.now.tomorrow + 1.second) expect do + post = Fabricate(:post, user: user) + + UserNotifications.expects(:mailing_list_notify) + .with(mailing_list_user, post) + .once + Jobs::NotifyMailingListSubscribers.new.execute( - post_id: Fabricate(:post, user: user) + post_id: post.id ) - end.to change { SkippedEmailLog.count }.by(1) + end.to change { SkippedEmailLog.count }.by(0) end end diff --git a/spec/jobs/user_email_spec.rb b/spec/jobs/user_email_spec.rb index 7a82606d205..25dd02807c5 100644 --- a/spec/jobs/user_email_spec.rb +++ b/spec/jobs/user_email_spec.rb @@ -245,12 +245,14 @@ describe Jobs::UserEmail do it "does not send notification if limit is reached" do expect do - Jobs::UserEmail.new.execute( - type: :user_mentioned, - user_id: user.id, - notification_id: notification.id, - post_id: post.id - ) + 2.times do + Jobs::UserEmail.new.execute( + type: :user_mentioned, + user_id: user.id, + notification_id: notification.id, + post_id: post.id + ) + end end.to change { SkippedEmailLog.count }.by(1) expect(SkippedEmailLog.exists?( @@ -260,6 +262,17 @@ describe Jobs::UserEmail do to_address: user.email, reason_type: SkippedEmailLog.reason_types[:exceeded_emails_limit] )).to eq(true) + + freeze_time(Time.zone.now.tomorrow + 1.second) + + 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(0) end it "sends critical email" do