diff --git a/app/lib/mail_delivery_error.rb b/app/lib/mail_delivery_error.rb new file mode 100644 index 000000000..4ce1cffed --- /dev/null +++ b/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 diff --git a/app/mailers/concerns/mailer_monitoring_concern.rb b/app/mailers/concerns/mailer_monitoring_concern.rb index 398ac79d5..849b6f016 100644 --- a/app/mailers/concerns/mailer_monitoring_concern.rb +++ b/app/mailers/concerns/mailer_monitoring_concern.rb @@ -4,6 +4,10 @@ module MailerMonitoringConcern 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| message.perform_deliveries = false @@ -13,12 +17,10 @@ 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) @@ -27,13 +29,12 @@ module MailerMonitoringConcern 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 diff --git a/spec/mailers/application_mailer_spec.rb b/spec/mailers/application_mailer_spec.rb index d81fee7e2..4146d56ad 100644 --- a/spec/mailers/application_mailer_spec.rb +++ b/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 diff --git a/spec/mailers/invite_mailer_spec.rb b/spec/mailers/invite_mailer_spec.rb index ebce58778..78f05cefc 100644 --- a/spec/mailers/invite_mailer_spec.rb +++ b/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