PERF: Split skipped email logs into a seperate table.

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

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

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

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

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

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