Merge pull request #8435 from colinux/email-error-retry

fix(email): ré-essai si une erreur intervient à l'envoi

app/lib/mail_delivery_error.rb

@@ -0,0 +1,6 @@
+# Inherit from `Exception` instead of `StandardError`
+# because this error is raised in a `rescue_from StandardError`,
+# so it would be shallowed otherwise.
+#
+# TODO: add a test which verify that the error will permit the job to retry
+class MailDeliveryError < Exception; end # rubocop:disable Lint/InheritException

app/mailers/application_mailer.rb

@@ -1,4 +1,5 @@
 class ApplicationMailer < ActionMailer::Base
+  include MailerDolistConcern
   include MailerMonitoringConcern
 
   helper :application # gives access to all helpers defined within `application_helper`.

app/mailers/concerns/mailer_dolist_concern.rb

@@ -0,0 +1,15 @@
+module MailerDolistConcern
+  extend ActiveSupport::Concern
+
+  included do
+    before_action :add_dolist_header
+
+    # mandatory for dolist
+    # used for tracking in Dolist UI
+    # the delivery_method is yet unknown (:balancer)
+    # so we add the dolist header for everyone
+    def add_dolist_header
+      headers['X-Dolist-Message-Name'] = action_name
+    end
+  end
+end

app/mailers/concerns/mailer_monitoring_concern.rb

@@ -2,7 +2,9 @@ module MailerMonitoringConcern
   extend ActiveSupport::Concern
 
   included do
-    before_action :add_dolist_header
+    # Intercept & log any error, then re-raise so job will retry.
+    # NOTE: rescue_from order matters, later matchers are tried first.
+    rescue_from StandardError, with: :log_and_raise_delivery_error
 
     # Don’t retry to send a message if the server rejects the recipient address
     rescue_from Net::SMTPSyntaxError do |_exception|
@@ -13,27 +15,16 @@ module MailerMonitoringConcern
       if /unexpected recipients/.match?(exception.message)
         message.perform_deliveries = false
       else
-        log_delivery_error(exception)
+        log_and_raise_delivery_error(exception)
       end
     end
 
-    rescue_from StandardError, with: :log_delivery_error
-
-    # mandatory for dolist
-    # used for tracking in Dolist UI
-    # the delivery_method is yet unknown (:balancer)
-    # so we add the dolist header for everyone
-    def add_dolist_header
-      headers['X-Dolist-Message-Name'] = action_name
-    end
-
-    protected
-
-    def log_delivery_error(exception)
+    def log_and_raise_delivery_error(exception)
       EmailEvent.create_from_message!(message, status: "dispatch_error")
       Sentry.capture_exception(exception, extra: { to: message.to, subject: message.subject })
 
-      # TODO find a way to re attempt the job
+      # re-raise another error so job will retry later
+      raise MailDeliveryError.new(exception)
     end
   end
 end

app/mailers/devise_user_mailer.rb

@@ -3,6 +3,7 @@ class DeviseUserMailer < Devise::Mailer
   helper :application # gives access to all helpers defined within `application_helper`.
   helper MailerHelper
   include Devise::Controllers::UrlHelpers # Optional. eg. `confirmation_url`
+  include MailerDolistConcern
   include MailerMonitoringConcern
   layout 'mailers/layout'
   before_action :add_delivery_method, if: :forced_delivery?

config/environments/development.rb

@@ -80,7 +80,7 @@ Rails.application.configure do
   ActionMailer::Base.add_delivery_method :balancer, BalancerDeliveryMethod
   config.action_mailer.balancer_settings = {
     helo: ENV['HELO_ENABLED'] == 'enabled' ? 100 : 0,
-      letter_opener: ENV['HELO_ENABLED'] == 'enabled' ? 0 : 100
+    letter_opener: ENV['HELO_ENABLED'] == 'enabled' ? 0 : 100
   }
   config.action_mailer.delivery_method = :balancer
 

spec/mailers/application_mailer_spec.rb

@@ -41,8 +41,8 @@ RSpec.describe ApplicationMailer, type: :mailer do
       expect(event.status).to eq('dispatched')
     end
 
-    context "when email is not sent" do
-      subject(:send_email) { UserMailer.ask_for_merge(user1, user2.email).deliver_now }
+    context "when there is an error and email are not sent" do
+      subject { UserMailer.ask_for_merge(user1, user2.email) }
 
       before do
         allow_any_instance_of(Mail::Message)
@@ -53,8 +53,8 @@ RSpec.describe ApplicationMailer, type: :mailer do
       context "smtp server busy" do
         let(:smtp_error) { Net::SMTPServerBusy.new }
 
-        it "creates an event" do
-          expect { send_email }.to change { EmailEvent.count }.by(1)
+        it "re-raise an error and creates an event" do
+          expect { subject.deliver_now }.to change { EmailEvent.count }.by(1).and raise_error(MailDeliveryError)
           expect(EmailEvent.last.status).to eq('dispatch_error')
         end
       end
@@ -62,8 +62,8 @@ RSpec.describe ApplicationMailer, type: :mailer do
       context "generic unknown error" do
         let(:smtp_error) { Net::OpenTimeout.new }
 
-        it "creates an event" do
-          expect { send_email }.to change { EmailEvent.count }.by(1)
+        it "re-raise an error and creates an event" do
+          expect { subject.deliver_now }.to change { EmailEvent.count }.by(1).and raise_error(MailDeliveryError)
           expect(EmailEvent.last.status).to eq('dispatch_error')
         end
       end

spec/mailers/invite_mailer_spec.rb

@@ -18,7 +18,13 @@ RSpec.describe InviteMailer, type: :mailer do
            v = send_mail_values.shift
            v == :raise ? raise("boom") : v
          end
-         mailer.body rescue nil
+
+         begin
+           mailer.body
+         rescue MailDeliveryError
+           nil
+         end
+
          mailer.body
          expect(TargetedUserLink.where(target_model: invite, user: invite.user).count).to eq(1)
        end
@@ -42,7 +48,13 @@ RSpec.describe InviteMailer, type: :mailer do
            v = send_mail_values.shift
            v == :raise ? raise("boom") : v
          end
-         mailer.body rescue nil
+
+         begin
+           mailer.body
+         rescue MailDeliveryError
+           nil
+         end
+
          mailer.body
          expect(TargetedUserLink.where(target_model: invite, user: invite.user).count).to eq(1)
        end