Merge pull request #6192 from tgxworld/drop_columns_on_email_logs

Drop `reply_key`, `skipped` and `skipped_reason` from `email_logs`.

app/controllers/admin/email_controller.rb

@@ -18,12 +18,11 @@ class Admin::EmailController < Admin::AdminController
   end
 
   def sent
-    email_logs = EmailLog.sent
+    email_logs = EmailLog.joins(<<~SQL)
-      .joins("
+      LEFT JOIN post_reply_keys
-        LEFT JOIN post_reply_keys
+      ON post_reply_keys.post_id = email_logs.post_id
-        ON post_reply_keys.post_id = email_logs.post_id
+      AND post_reply_keys.user_id = email_logs.user_id
-        AND post_reply_keys.user_id = email_logs.user_id
+    SQL
-      ")
 
     email_logs = filter_logs(email_logs, params)
 

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")
+      EmailLog.where("created_at < ?", threshold).delete_all
-        .where("created_at < ?", threshold)
-        .delete_all
-
       SkippedEmailLog.where("created_at < ?", threshold).delete_all
     end
 

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)
 

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 :bounced, -> { where(bounced: true) }
-  scope :skipped, -> { where(skipped: true) }
-  scope :bounced, -> { sent.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
+#  id         :integer          not null, primary key
-#  to_address     :string           not null
+#  to_address :string           not null
-#  email_type     :string           not null
+#  email_type :string           not null
-#  user_id        :integer
+#  user_id    :integer
-#  created_at     :datetime         not null
+#  created_at :datetime         not null
-#  updated_at     :datetime         not null
+#  updated_at :datetime         not null
-#  post_id        :integer
+#  post_id    :integer
-#  skipped        :boolean          default(FALSE)
+#  bounce_key :uuid
-#  skipped_reason :string
+#  bounced    :boolean          default(FALSE), not null
-#  bounce_key     :uuid
+#  message_id :string
-#  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_created_at                          (created_at)
+#  index_email_logs_on_message_id              (message_id)
-#  index_email_logs_on_message_id                          (message_id)
+#  index_email_logs_on_post_id                 (post_id)
-#  index_email_logs_on_post_id                             (post_id)
+#  index_email_logs_on_user_id                 (user_id)
-#  index_email_logs_on_reply_key                           (reply_key)
+#  index_email_logs_on_user_id_and_created_at  (user_id,created_at)
-#  index_email_logs_on_skipped_and_bounced_and_created_at  (skipped,bounced,created_at)
-#  index_email_logs_on_user_id                             (user_id)
 #

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!"

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

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)

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

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)

spec/models/email_log_spec.rb

@@ -13,23 +13,25 @@ describe EmailLog do
       post = Fabricate(:post)
       user = post.user
 
-      # skipped emails do not matter
+      ran = EmailLog.unique_email_per_post(post, user) do
-      Fabricate(:email_log, user: user, email_type: 'blah', post_id: post.id, to_address: user.email, user_id: user.id, skipped: true)
+        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)
+      expect(ran).to be(nil)
-
-      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
-
     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