From 9b2d05b8a1ab3677890ab9ea27925f68074611b7 Mon Sep 17 00:00:00 2001 From: Martin Date: Mon, 19 Jun 2023 15:24:10 +0200 Subject: [PATCH] =?UTF-8?q?amelioration(email=5Fevent):=20re-lever=20une?= =?UTF-8?q?=20erreur=20dans=20un=20rescue=5Ffrom=20ne=20la=20fait=20pas=20?= =?UTF-8?q?remonter.=20change=20de=20strat=C3=A9gie=20pour=20savoir=20si?= =?UTF-8?q?=20oui=20ou=20non=20un=20mail=20a=20ete=20envoye=20avec=20succe?= =?UTF-8?q?ss.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/lib/mail_delivery_error.rb | 12 --------- .../concerns/mailer_monitoring_concern.rb | 14 ---------- app/models/email_event.rb | 1 + app/services/email_delivering_interceptor.rb | 5 ++++ config/initializers/mail_observers.rb | 2 ++ spec/mailers/application_mailer_spec.rb | 26 ++++++++++++------- spec/mailers/invite_mailer_spec.rb | 4 +-- 7 files changed, 26 insertions(+), 38 deletions(-) delete mode 100644 app/lib/mail_delivery_error.rb create mode 100644 app/services/email_delivering_interceptor.rb diff --git a/app/lib/mail_delivery_error.rb b/app/lib/mail_delivery_error.rb deleted file mode 100644 index 630496528..000000000 --- a/app/lib/mail_delivery_error.rb +++ /dev/null @@ -1,12 +0,0 @@ -# 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 # rubocop:disable Lint/InheritException - def initialize(original_exception) - super(original_exception.message) - - set_backtrace(original_exception.backtrace) - end -end diff --git a/app/mailers/concerns/mailer_monitoring_concern.rb b/app/mailers/concerns/mailer_monitoring_concern.rb index f140c20fa..dca983bcc 100644 --- a/app/mailers/concerns/mailer_monitoring_concern.rb +++ b/app/mailers/concerns/mailer_monitoring_concern.rb @@ -2,10 +2,6 @@ module MailerMonitoringConcern extend ActiveSupport::Concern included do - # 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 @@ -14,21 +10,11 @@ module MailerMonitoringConcern rescue_from Net::SMTPServerBusy do |exception| if /unexpected recipients/.match?(exception.message) message.perform_deliveries = false - else - log_and_raise_delivery_error(exception) end end rescue_from Dolist::IgnorableError, with: :log_delivery_error - def log_and_raise_delivery_error(exception) - log_delivery_error(exception) - Sentry.capture_exception(exception, extra: { to: message.to, subject: message.subject }) - - # re-raise another error so job will retry later - raise MailDeliveryError.new(exception) - end - def log_delivery_error(exception) EmailEvent.create_from_message!(message, status: "dispatch_error") end diff --git a/app/models/email_event.rb b/app/models/email_event.rb index 1c2d20ec7..89e6e64ee 100644 --- a/app/models/email_event.rb +++ b/app/models/email_event.rb @@ -16,6 +16,7 @@ class EmailEvent < ApplicationRecord RETENTION_DURATION = 1.month enum status: { + pending: 'pending', dispatched: 'dispatched', dispatch_error: 'dispatch_error' } diff --git a/app/services/email_delivering_interceptor.rb b/app/services/email_delivering_interceptor.rb new file mode 100644 index 000000000..e4a7648d6 --- /dev/null +++ b/app/services/email_delivering_interceptor.rb @@ -0,0 +1,5 @@ +class EmailDeliveringInterceptor + def self.delivering_email(message) + EmailEvent.create_from_message!(message, status: "pending") + end +end diff --git a/config/initializers/mail_observers.rb b/config/initializers/mail_observers.rb index 985682909..e91e89d2e 100644 --- a/config/initializers/mail_observers.rb +++ b/config/initializers/mail_observers.rb @@ -2,7 +2,9 @@ # otherwise the observer won't be invoked. # require_relative "../../app/services/email_delivery_observer" +require_relative "../../app/services/email_delivering_interceptor" ActiveSupport.on_load(:action_mailer) do |mailer| + mailer.register_interceptor EmailDeliveringInterceptor mailer.register_observer EmailDeliveryObserver end diff --git a/spec/mailers/application_mailer_spec.rb b/spec/mailers/application_mailer_spec.rb index c22381030..99585b8a2 100644 --- a/spec/mailers/application_mailer_spec.rb +++ b/spec/mailers/application_mailer_spec.rb @@ -37,8 +37,8 @@ RSpec.describe ApplicationMailer, type: :mailer do end it 'raise classic error to retry' do - expect { subject }.to raise_error(MailDeliveryError) - expect(EmailEvent.dolist_api.dispatch_error.count).to eq(1) + expect { subject }.to raise_error(RuntimeError) + expect(EmailEvent.pending.count).to eq(1) end end @@ -76,8 +76,9 @@ RSpec.describe ApplicationMailer, type: :mailer do let(:user2) { create(:user, email: "your@email.com") } it 'creates a new EmailEvent record with the correct information' do - expect { UserMailer.ask_for_merge(user1, user2.email).deliver_now }.to change { EmailEvent.count }.by(1) + expect { UserMailer.ask_for_merge(user1, user2.email).deliver_now }.to change { EmailEvent.count }.by(2) event = EmailEvent.last + expect(EmailEvent.first.status).to eq('pending') expect(event.to).to eq("your@email.com") expect(event.method).to eq("test") @@ -85,31 +86,36 @@ RSpec.describe ApplicationMailer, type: :mailer do expect(event.processed_at).to be_within(1.second).of(Time.zone.now) expect(event.status).to eq('dispatched') end + end + + context 'EmailDeliveringInterceptor is invoked' do + let(:user1) { create(:user) } + let(:user2) { create(:user, email: "your@email.com") } 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) - .to receive(:deliver) + .to receive(:do_delivery) .and_raise(smtp_error) end context "smtp server busy" do let(:smtp_error) { Net::SMTPServerBusy.new('451 4.7.500 Server busy') } - 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') + it "catches the smtp error" do + expect { subject.deliver_now }.not_to raise_error + expect(EmailEvent.pending.count).to eq(1) end end - context "generic unknown error" do + context "does not catches other error" do let(:smtp_error) { Net::OpenTimeout.new } 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') + expect { subject.deliver_now }.to raise_error(Net::OpenTimeout) + expect(EmailEvent.pending.count).to eq(1) end end end diff --git a/spec/mailers/invite_mailer_spec.rb b/spec/mailers/invite_mailer_spec.rb index 9be780237..0e5102dc9 100644 --- a/spec/mailers/invite_mailer_spec.rb +++ b/spec/mailers/invite_mailer_spec.rb @@ -21,7 +21,7 @@ RSpec.describe InviteMailer, type: :mailer do begin mailer.body - rescue MailDeliveryError + rescue => e nil end @@ -70,7 +70,7 @@ RSpec.describe InviteMailer, type: :mailer do begin mailer.body - rescue MailDeliveryError + rescue => e nil end