From 87537b679c74da58e9439c322f009f7bbd09966a Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Fri, 27 Jul 2018 12:32:07 +0800 Subject: [PATCH] Drop `reply_key`, `skipped` and `skipped_reason` from `email_logs`. --- app/controllers/admin/email_controller.rb | 11 ++--- app/jobs/scheduled/clean_up_email_logs.rb | 5 +- app/mailers/user_notifications.rb | 2 +- app/models/email_log.rb | 48 +++++++++---------- db/fixtures/000_delayed_drops.rb | 5 +- ..._skipped_skipped_reason_from_email_logs.rb | 11 +++++ spec/jobs/clean_up_email_logs_spec.rb | 2 +- spec/jobs/user_email_spec.rb | 1 - spec/mailers/user_notifications_spec.rb | 7 ++- spec/models/email_log_spec.rb | 35 ++++++-------- 10 files changed, 65 insertions(+), 62 deletions(-) create mode 100644 db/migrate/20180727042448_drop_reply_key_skipped_skipped_reason_from_email_logs.rb diff --git a/app/controllers/admin/email_controller.rb b/app/controllers/admin/email_controller.rb index 5ce8f2c170c..c626ffb2254 100644 --- a/app/controllers/admin/email_controller.rb +++ b/app/controllers/admin/email_controller.rb @@ -18,12 +18,11 @@ class Admin::EmailController < Admin::AdminController end def sent - email_logs = EmailLog.sent - .joins(" - LEFT JOIN post_reply_keys - ON post_reply_keys.post_id = email_logs.post_id - AND post_reply_keys.user_id = email_logs.user_id - ") + email_logs = EmailLog.joins(<<~SQL) + LEFT JOIN post_reply_keys + ON post_reply_keys.post_id = email_logs.post_id + AND post_reply_keys.user_id = email_logs.user_id + SQL email_logs = filter_logs(email_logs, params) diff --git a/app/jobs/scheduled/clean_up_email_logs.rb b/app/jobs/scheduled/clean_up_email_logs.rb index 68650829b23..b4e0fafa262 100644 --- a/app/jobs/scheduled/clean_up_email_logs.rb +++ b/app/jobs/scheduled/clean_up_email_logs.rb @@ -8,10 +8,7 @@ module Jobs threshold = SiteSetting.delete_email_logs_after_days.days.ago - EmailLog.where("reply_key IS NULL") - .where("created_at < ?", threshold) - .delete_all - + EmailLog.where("created_at < ?", threshold).delete_all SkippedEmailLog.where("created_at < ?", threshold).delete_all end diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 72eefc69376..cde42b3ee6d 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -541,7 +541,7 @@ class UserNotifications < ActionMailer::Base end else reached_limit = SiteSetting.max_emails_per_day_per_user > 0 - reached_limit &&= (EmailLog.where(user_id: user.id, skipped: false) + reached_limit &&= (EmailLog.where(user_id: user.id) .where('created_at > ?', 1.day.ago) .count) >= (SiteSetting.max_emails_per_day_per_user - 1) diff --git a/app/models/email_log.rb b/app/models/email_log.rb index ba588e1b430..08839f290cf 100644 --- a/app/models/email_log.rb +++ b/app/models/email_log.rb @@ -4,6 +4,8 @@ class EmailLog < ActiveRecord::Base self.ignored_columns = %w{ topic_id reply_key + skipped + skipped_reason } CRITICAL_EMAIL_TYPES ||= Set.new %w{ @@ -23,20 +25,18 @@ class EmailLog < ActiveRecord::Base validates :email_type, :to_address, presence: true - scope :sent, -> { where(skipped: false) } - scope :skipped, -> { where(skipped: true) } - scope :bounced, -> { sent.where(bounced: true) } + scope :bounced, -> { where(bounced: true) } after_create do # 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? && !skipped + User.where(id: user_id).update_all("last_emailed_at = CURRENT_TIMESTAMP") if user_id.present? end def self.unique_email_per_post(post, user) return yield unless post && user DistributedMutex.synchronize("email_log_#{post.id}_#{user.id}") do - if where(post_id: post.id, user_id: user.id, skipped: false).exists? + if where(post_id: post.id, user_id: user.id).exists? nil else yield @@ -47,7 +47,7 @@ class EmailLog < ActiveRecord::Base def self.reached_max_emails?(user, email_type = nil) return false if SiteSetting.max_emails_per_day_per_user == 0 || CRITICAL_EMAIL_TYPES.include?(email_type) - count = sent.where('created_at > ?', 1.day.ago) + count = where('created_at > ?', 1.day.ago) .where(user_id: user.id) .count @@ -55,7 +55,7 @@ class EmailLog < ActiveRecord::Base end def self.count_per_day(start_date, end_date) - sent.where("created_at BETWEEN ? AND ?", start_date, end_date) + where("created_at BETWEEN ? AND ?", start_date, end_date) .group("DATE(created_at)") .order("DATE(created_at)") .count @@ -83,26 +83,22 @@ end # # Table name: email_logs # -# id :integer not null, primary key -# to_address :string not null -# email_type :string not null -# user_id :integer -# created_at :datetime not null -# updated_at :datetime not null -# post_id :integer -# skipped :boolean default(FALSE) -# skipped_reason :string -# bounce_key :uuid -# bounced :boolean default(FALSE), not null -# message_id :string +# id :integer not null, primary key +# to_address :string not null +# email_type :string not null +# user_id :integer +# created_at :datetime not null +# updated_at :datetime not null +# post_id :integer +# bounce_key :uuid +# bounced :boolean default(FALSE), not null +# message_id :string # # Indexes # -# idx_email_logs_user_created_filtered (user_id,created_at) WHERE (skipped = false) -# index_email_logs_on_created_at (created_at) -# index_email_logs_on_message_id (message_id) -# index_email_logs_on_post_id (post_id) -# index_email_logs_on_reply_key (reply_key) -# index_email_logs_on_skipped_and_bounced_and_created_at (skipped,bounced,created_at) -# index_email_logs_on_user_id (user_id) +# index_email_logs_on_created_at (created_at) +# index_email_logs_on_message_id (message_id) +# index_email_logs_on_post_id (post_id) +# index_email_logs_on_user_id (user_id) +# index_email_logs_on_user_id_and_created_at (user_id,created_at) # diff --git a/db/fixtures/000_delayed_drops.rb b/db/fixtures/000_delayed_drops.rb index edf88f772dd..5b976a7bdb1 100644 --- a/db/fixtures/000_delayed_drops.rb +++ b/db/fixtures/000_delayed_drops.rb @@ -241,9 +241,12 @@ Migration::ColumnDropper.drop( Migration::ColumnDropper.drop( table: 'email_logs', - after_migration: 'DropTopicIdOnEmailLogs', + after_migration: 'DropReplyKeySkippedSkippedReasonFromEmailLogs', columns: %w{ topic_id + reply_key + skipped + skipped_reason }, on_drop: ->() { STDERR.puts "Removing superflous email_logs columns!" diff --git a/db/migrate/20180727042448_drop_reply_key_skipped_skipped_reason_from_email_logs.rb b/db/migrate/20180727042448_drop_reply_key_skipped_skipped_reason_from_email_logs.rb new file mode 100644 index 00000000000..48905b0c258 --- /dev/null +++ b/db/migrate/20180727042448_drop_reply_key_skipped_skipped_reason_from_email_logs.rb @@ -0,0 +1,11 @@ +class DropReplyKeySkippedSkippedReasonFromEmailLogs < ActiveRecord::Migration[5.2] + def up + remove_index :email_logs, [:skipped, :bounced, :created_at] + remove_index :email_logs, name: 'idx_email_logs_user_created_filtered' + add_index :email_logs, [:user_id, :created_at] + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/spec/jobs/clean_up_email_logs_spec.rb b/spec/jobs/clean_up_email_logs_spec.rb index 59305c492f9..e56e1453b7f 100644 --- a/spec/jobs/clean_up_email_logs_spec.rb +++ b/spec/jobs/clean_up_email_logs_spec.rb @@ -11,7 +11,7 @@ describe Jobs::CleanUpEmailLogs do Fabricate(:skipped_email_log) end - it "removes old email logs without a reply_key" do + it "removes old email logs" do Jobs::CleanUpEmailLogs.new.execute({}) expect(EmailLog.count).to eq(2) expect(SkippedEmailLog.count).to eq(1) diff --git a/spec/jobs/user_email_spec.rb b/spec/jobs/user_email_spec.rb index 4f26d500eea..7a82606d205 100644 --- a/spec/jobs/user_email_spec.rb +++ b/spec/jobs/user_email_spec.rb @@ -45,7 +45,6 @@ describe Jobs::UserEmail do email_log = EmailLog.where(user_id: user.id).last expect(email_log.email_type).to eq("signup") - expect(email_log.skipped).to eq(false) end end diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb index 67033ff4018..b144995f1b6 100644 --- a/spec/mailers/user_notifications_spec.rb +++ b/spec/mailers/user_notifications_spec.rb @@ -506,7 +506,12 @@ describe UserNotifications do 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) + + user.email_logs.create!( + email_type: 'blah', + to_address: user.email, + user_id: user.id + ) post = Fabricate(:post) reply = Fabricate(:post, topic_id: post.topic_id) diff --git a/spec/models/email_log_spec.rb b/spec/models/email_log_spec.rb index 5a749dedbdc..f0f60dabb0c 100644 --- a/spec/models/email_log_spec.rb +++ b/spec/models/email_log_spec.rb @@ -13,23 +13,25 @@ describe EmailLog do post = Fabricate(:post) user = post.user - # skipped emails do not matter - Fabricate(:email_log, user: user, email_type: 'blah', post_id: post.id, to_address: user.email, user_id: user.id, skipped: true) + ran = EmailLog.unique_email_per_post(post, user) do + true + end + + expect(ran).to be(true) + + Fabricate(:email_log, + user: user, + email_type: 'blah', + post_id: post.id, + to_address: user.email, + user_id: user.id + ) ran = EmailLog.unique_email_per_post(post, user) do true end - expect(ran).to eq(true) - - Fabricate(:email_log, user: user, email_type: 'blah', post_id: post.id, to_address: user.email, user_id: user.id) - - ran = EmailLog.unique_email_per_post(post, user) do - true - end - - expect(ran).to be_falsy - + expect(ran).to be(nil) end end @@ -41,20 +43,12 @@ describe EmailLog do user.reload }.to change(user, :last_emailed_at) end - - it "doesn't update last_emailed_at if skipped is true" do - expect { - Fabricate(:email_log, user: user, email_type: 'blah', to_address: user.email, skipped: true) - user.reload - }.to_not change { user.last_emailed_at } - end end end describe '#reached_max_emails?' do before do SiteSetting.max_emails_per_day_per_user = 2 - Fabricate(:email_log, user: user, email_type: 'blah', to_address: user.email, user_id: user.id, skipped: true) Fabricate(:email_log, user: user, email_type: 'blah', to_address: user.email, user_id: user.id) Fabricate(:email_log, user: user, email_type: 'blah', to_address: user.email, user_id: user.id, created_at: 3.days.ago) end @@ -76,7 +70,6 @@ describe EmailLog do describe '#count_per_day' do it "counts sent emails" do Fabricate(:email_log, user: user, email_type: 'blah', to_address: user.email) - Fabricate(:email_log, user: user, email_type: 'blah', to_address: user.email, skipped: true) expect(described_class.count_per_day(1.day.ago, Time.now).first[1]).to eq 1 end end