Handle invalid emails in DomainExpireMailer#expired, move logging from job to mailer

#186

app/jobs/domain_expire_email_job.rb

@@ -4,18 +4,6 @@ class DomainExpireEmailJob < Que::Job
 
     return if domain.registered?
 
-    log(domain)
     DomainExpireMailer.expired(domain: domain, registrar: domain.registrar).deliver_now
   end
-
-  private
-
-  def log(domain)
-    message = "Send DomainExpireMailer#expired email for domain ##{domain.id} to #{domain.primary_contact_emails.join(', ')}"
-    logger.info(message)
-  end
-
-  def logger
-    Rails.logger
-  end
 end

app/mailers/domain_expire_mailer.rb

@@ -1,9 +1,38 @@
 class DomainExpireMailer < ApplicationMailer
   def expired(domain:, registrar:)
-    @domain = DomainPresenter.new(domain: domain, view: view_context)
+    @domain = domain_presenter(domain: domain)
-    @registrar = RegistrarPresenter.new(registrar: registrar, view: view_context)
+    @registrar = registrar_presenter(registrar: registrar)
 
+    recipient = filter_invalid_emails(emails: domain.primary_contact_emails, domain: domain)
     subject = default_i18n_subject(domain_name: domain.name)
-    mail(to: domain.primary_contact_emails, subject: subject)
+
+    logger.info("Send DomainExpireMailer#expired email for domain ##{domain.id}" \
+    " to #{domain.primary_contact_emails.join(', ')}")
+
+    mail(to: recipient, subject: subject)
+  end
+
+  private
+
+  def domain_presenter(domain:)
+    DomainPresenter.new(domain: domain, view: view_context)
+  end
+
+  def registrar_presenter(registrar:)
+    RegistrarPresenter.new(registrar: registrar, view: view_context)
+  end
+
+  # Needed because there are invalid emails in the database, which have been imported from legacy app
+  def filter_invalid_emails(emails:, domain:)
+    emails.keep_if do |email|
+      valid = EmailValidator.new(email).valid?
+
+      unless valid
+        logger.info("Unable to send DomainExpireMailer#expired email for domain ##{domain.id}" \
+        " to invalid recipient #{email}")
+      end
+
+      valid
+    end
   end
 end

spec/jobs/domain_expire_email_job_spec.rb

@@ -18,21 +18,11 @@ RSpec.describe DomainExpireEmailJob do
 
       before :example do
         allow(domain).to receive_messages(
-                           id: 1,
                            registrar: 'registrar',
                            registered?: false,
                            primary_contact_emails: %w(test@test.com test@test.com))
       end
 
-      it 'creates log record' do
-        log_message = 'Send DomainExpireMailer#expired email for domain #1 to test@test.com, test@test.com'
-
-        allow(DomainExpireMailer).to receive(:expired).and_return(message)
-        allow(message).to receive(:deliver_now)
-
-        expect(Rails.logger).to receive(:info).with(log_message)
-      end
-
       it 'sends email' do
         expect(DomainExpireMailer).to receive(:expired).with(domain: domain, registrar: 'registrar')
                                         .and_return(message)
@@ -45,10 +35,6 @@ RSpec.describe DomainExpireEmailJob do
         allow(domain).to receive(:registered?).and_return(true)
       end
 
-      it 'does not create log record' do
-        expect(Rails.logger).to_not receive(:info)
-      end
-
       it 'does not send email' do
         expect(DomainExpireMailer).to_not receive(:expired)
       end

spec/mailers/domain_expire_mailer_spec.rb

@@ -2,32 +2,64 @@ require 'rails_helper'
 
 RSpec.describe DomainExpireMailer do
   describe '#expired' do
-    let(:domain) { instance_spy(Domain, name: 'test.com') }
+    let(:domain) { instance_spy(Domain,
-    let(:registrar) { 'registrar' }
+                                id: 1,
+                                name: 'test.com',
+                                primary_contact_emails: recipient)
+    }
     let(:domain_presenter) { instance_spy(DomainPresenter) }
     let(:registrar_presenter) { instance_spy(RegistrarPresenter) }
-    subject(:message) { described_class.expired(domain: domain, registrar: registrar) }
+    subject(:message) { described_class.expired(domain: domain, registrar: nil) }
 
     before :example do
       expect(DomainPresenter).to receive(:new).and_return(domain_presenter)
       expect(RegistrarPresenter).to receive(:new).and_return(registrar_presenter)
     end
 
+    context 'when all recipients are valid' do
+      let(:recipient) { %w[recipient@test.com recipient@test.com] }
+
       it 'has sender' do
         expect(message.from).to eq(['noreply@internet.ee'])
       end
 
-    it 'has recipient' do
+      it 'delivers to all recipients' do
-      expect(domain).to receive(:primary_contact_emails).and_return(['recipient@test.com'])
+        expect(message.to).to match_array(%w[recipient@test.com recipient@test.com])
-      expect(message.to).to match_array(['recipient@test.com'])
       end
 
       it 'has subject' do
         expect(message.subject).to eq('The test.com domain has expired')
       end
 
+      it 'creates log record' do
+        log_message = 'Send DomainExpireMailer#expired email for domain #1 to recipient@test.com,' \
+        ' recipient@test.com'
+        expect(described_class.logger).to receive(:info).with(log_message)
+        message.deliver_now
+      end
+
       it 'sends message' do
-      expect { message.deliver! }.to change { ActionMailer::Base.deliveries.count }.by(1)
+        expect { message.deliver_now }.to change { ActionMailer::Base.deliveries.count }.by(1)
+      end
+    end
+
+    context 'when some recipient is invalid' do
+      let(:recipient) { %w[invalid_email valid@test.com] }
+
+      before :example do
+        allow(described_class.logger).to receive(:info)
+      end
+
+      it 'does not deliver to invalid recipient' do
+        expect(message.to).to match_array(%w[valid@test.com])
+      end
+
+      it 'creates log record' do
+        log_message = 'Unable to send DomainExpireMailer#expired email for domain #1 to' \
+        ' invalid recipient invalid_email'
+        expect(described_class.logger).to receive(:info).with(log_message)
+        message.deliver_now
+      end
     end
   end
 end