DEV: Remove the use of stubs and mocks in `Jobs::UserEmail` tests.

DEV: Remove the use of stubs and mocks in Jobs::UserEmail tests.

We can only be sure that an email is sent when we get a mailer in ActionMailer::Deliveries. A couple of tests were actually incorrect because it didn’t flow through our email sender where there are more conditions in determining whether an email is sent or not.

diff --git a/app/jobs/regular/user_email.rb b/app/jobs/regular/user_email.rb
index 4b0ae6b..322d9c0 100644
--- a/app/jobs/regular/user_email.rb
+++ b/app/jobs/regular/user_email.rb
@@ -54,7 +54,10 @@ module Jobs
       )
 
       if message
-        Email::Sender.new(message, type, user).send
+        Email::Sender.new(message, type, user).send(
+          is_critical: self.class == Jobs::CriticalUserEmail
+        )
+
         if (b = user.user_stat.bounce_score) > SiteSetting.bounce_score_erode_on_send
           # erode bounce score each time we send an email
           # this means that we are punished a lot less for bounces
diff --git a/lib/email/sender.rb b/lib/email/sender.rb
index 4cc6bcf..40217f3 100644
--- a/lib/email/sender.rb
+++ b/lib/email/sender.rb
@@ -21,8 +21,13 @@ module Email
       @user = user
     end
 
-    def send
-      return if SiteSetting.disable_emails == "yes" && @email_type.to_s != "admin_login"
+    def send(is_critical: false)
+      if SiteSetting.disable_emails == "yes" &&
+         @email_type.to_s != "admin_login" &&
+         !is_critical
+
+        return
+      end
 
       return if ActionMailer::Base::NullMail === @message
       return if ActionMailer::Base::NullMail === (@message.message rescue nil)
diff --git a/spec/jobs/user_email_spec.rb b/spec/jobs/user_email_spec.rb
index 299aaba..d35844b 100644
--- a/spec/jobs/user_email_spec.rb
+++ b/spec/jobs/user_email_spec.rb
@@ -11,7 +11,6 @@ describe Jobs::UserEmail do
   let(:staged) { Fabricate(:user, staged: true, last_seen_at: 11.minutes.ago) }
   let(:suspended) { Fabricate(:user, last_seen_at: 10.minutes.ago, suspended_at: 5.minutes.ago, suspended_till: 7.days.from_now) }
   let(:anonymous) { Fabricate(:anonymous, last_seen_at: 11.minutes.ago) }
-  let(:mailer) { Mail::Message.new(to: user.email) }
 
   it "raises an error when there is no user" do
     expect { Jobs::UserEmail.new.execute(type: :digest) }.to raise_error(Discourse::InvalidParameters)
@@ -26,13 +25,15 @@ describe Jobs::UserEmail do
   end
 
   it "doesn't call the mailer when the user is missing" do
-    UserNotifications.expects(:digest).never
     Jobs::UserEmail.new.execute(type: :digest, user_id: 1234)
+
+    expect(ActionMailer::Base.deliveries).to eq([])
   end
 
   it "doesn't call the mailer when the user is staged" do
-    UserNotifications.expects(:digest).never
     Jobs::UserEmail.new.execute(type: :digest, user_id: staged.id)
+
+    expect(ActionMailer::Base.deliveries).to eq([])
   end
 
   context "bounce score" do
@@ -45,55 +46,48 @@ describe Jobs::UserEmail do
 
       email_log = EmailLog.where(user_id: user.id).last
       expect(email_log.email_type).to eq("signup")
+
+      expect(ActionMailer::Base.deliveries.first.to).to contain_exactly(
+        user.email
+      )
     end
 
   end
 
   context 'to_address' do
     it 'overwrites a to_address when present' do
-      UserNotifications.expects(:confirm_new_email).returns(mailer)
-      Email::Sender.any_instance.expects(:send)
       Jobs::UserEmail.new.execute(type: :confirm_new_email, user_id: user.id, to_address: 'jake@adventuretime.ooo')
-      expect(mailer.to).to eq(['jake@adventuretime.ooo'])
+
+      expect(ActionMailer::Base.deliveries.first.to).to contain_exactly(
+        'jake@adventuretime.ooo'
+      )
     end
   end
 
   context "disable_emails setting" do
     it "sends when no" do
       SiteSetting.disable_emails = 'no'
-      Email::Sender.any_instance.expects(:send).once
       Jobs::UserEmail.new.execute(type: :confirm_new_email, user_id: user.id)
+
+      expect(ActionMailer::Base.deliveries.first.to).to contain_exactly(
+        user.email
+      )
     end
 
     it "does not send an email when yes" do
       SiteSetting.disable_emails = 'yes'
-      Email::Sender.any_instance.expects(:send).never
       Jobs::UserEmail.new.execute(type: :confirm_new_email, user_id: user.id)
+
+      expect(ActionMailer::Base.deliveries).to eq([])
     end
 
     it "sends when critical" do
       SiteSetting.disable_emails = 'yes'
-      Email::Sender.any_instance.expects(:send)
       Jobs::CriticalUserEmail.new.execute(type: :confirm_new_email, user_id: user.id)
-    end
-  end
 
-  context "recently seen" do
-    let(:post) { Fabricate(:post, user: user) }
-
-    it "doesn't send an email to a user that's been recently seen" do
-      user.update_column(:last_seen_at, 9.minutes.ago)
-      Email::Sender.any_instance.expects(:send).never
-      Jobs::UserEmail.new.execute(type: :user_replied, user_id: user.id, post_id: post.id)
-    end
-
-    it "does send an email to a user that's been recently seen but has email_always set" do
-      user.update_attributes(last_seen_at: 9.minutes.ago)
-      user.user_option.update_attributes(email_always: true)
-      PostTiming.create!(topic_id: post.topic_id, post_number: post.post_number, user_id: user.id, msecs: 100)
-
-      Email::Sender.any_instance.expects(:send)
-      Jobs::UserEmail.new.execute(type: :user_replied, user_id: user.id, post_id: post.id)
+      expect(ActionMailer::Base.deliveries.first.to).to contain_exactly(
+        user.email
+      )
     end
   end
 
@@ -145,49 +139,65 @@ describe Jobs::UserEmail do
   context 'args' do
 
     it 'passes a token as an argument when a token is present' do
-      UserNotifications.expects(:forgot_password).with(user, email_token: 'asdfasdf').returns(mailer)
-      Email::Sender.any_instance.expects(:send)
       Jobs::UserEmail.new.execute(type: :forgot_password, user_id: user.id, email_token: 'asdfasdf')
+
+      mail = ActionMailer::Base.deliveries.first
+
+      expect(mail.to).to contain_exactly(user.email)
+      expect(mail.body).to include("asdfasdf")
     end
 
     context "post" do
       let(:post) { Fabricate(:post, user: user) }
 
-      it 'passes a post as an argument when a post_id is present' do
-        UserNotifications.expects(:user_private_message).with(user, post: post).returns(mailer)
-        Email::Sender.any_instance.expects(:send)
-        Jobs::UserEmail.new.execute(type: :user_private_message, user_id: user.id, post_id: post.id)
-      end
-
       it "doesn't send the email if you've seen the post" do
-        Email::Sender.any_instance.expects(:send).never
         PostTiming.record_timing(topic_id: post.topic_id, user_id: user.id, post_number: post.post_number, msecs: 6666)
         Jobs::UserEmail.new.execute(type: :user_private_message, user_id: user.id, post_id: post.id)
+
+        expect(ActionMailer::Base.deliveries).to eq([])
       end
 
       it "doesn't send the email if the user deleted the post" do
-        Email::Sender.any_instance.expects(:send).never
         post.update_column(:user_deleted, true)
         Jobs::UserEmail.new.execute(type: :user_private_message, user_id: user.id, post_id: post.id)
+
+        expect(ActionMailer::Base.deliveries).to eq([])
       end
 
       it "doesn't send the email if user of the post has been deleted" do
-        Email::Sender.any_instance.expects(:send).never
         post.update_attributes!(user_id: nil)
         Jobs::UserEmail.new.execute(type: :user_replied, user_id: user.id, post_id: post.id)
+
+        expect(ActionMailer::Base.deliveries).to eq([])
       end
 
       context 'user is suspended' do
         it "doesn't send email for a pm from a regular user" do
-          Email::Sender.any_instance.expects(:send).never
           Jobs::UserEmail.new.execute(type: :user_private_message, user_id: suspended.id, post_id: post.id)
+
+          expect(ActionMailer::Base.deliveries).to eq([])
         end
 
         it "does send an email for a pm from a staff user" do
           pm_from_staff = Fabricate(:post, user: Fabricate(:moderator))
           pm_from_staff.topic.topic_allowed_users.create!(user_id: suspended.id)
-          Email::Sender.any_instance.expects(:send)
-          Jobs::UserEmail.new.execute(type: :user_private_message, user_id: suspended.id, post_id: pm_from_staff.id)
+
+          pm_notification = Fabricate(:notification,
+            user: suspended,

[... diff too long, it was truncated ...]

GitHub sha: 34b29f62

3 Likes

The addition of !is_critical is a functional change - now “critical emails” bypass the “disable emails” site setting. That means that users can receive various emails including password reset emails, invites, account activation, suspicious login reports, etc.

I think this is kinda risky, since people depend on the disable_emails setting to secure their sites during maintenance, or an import. What do you think @tgxworld?

2 Likes

I realise this change was made so that the fixed specs would pass, however I still think it is unexpected now that disable_email has become an important part of our workflows. My suggested change is here: FIX: Prevent critical emails bypassing disable, and improve email test logic by davidtaylorhq · Pull Request #7238 · discourse/discourse · GitHub

1 Like

Yea I was following the expectations of the spec previously. Thanks for following up on this.

3 Likes

FIX: Prevent critical emails bypassing disable, and improve email test logic

This test (and maybe others) isn’t testing anything. It will always pass now:

it "doesn't call the mailer when the user is missing" do
    Jobs::UserEmail.new.execute(type: :digest, user_id: 1234)

    expect(ActionMailer::Base.deliveries).to eq([])
  end

UserNotifications.digest will always return nil because there are no topics, so even with a valid user this test will pass.

So with the removal of stubs, we need to seed some topics, posts, and likes according to the digest logic so that we can test this?

2 Likes

Actually the test will fail if a valid user is given and there is another test case that covers this.

Yeah I added those tests here: FIX: prevent sending multiple summary emails due to Sidekiq delays · discourse/discourse@399e937 · GitHub

1 Like

Ah icic thank you so much! I didn’t see the follow up hehe