From 5f28aac818088907dee5ce5488245a2c5be97ebe Mon Sep 17 00:00:00 2001 From: Shujat Khalid Date: Thu, 8 Aug 2024 14:39:23 +0100 Subject: [PATCH 1/8] Able to pull through email info through header --- app/mailers/application_mailer.rb | 19 ++++++++++++ .../anonymous/test_notify_error.text.erb | 1 + spec/mailers/application_mailer_spec.rb | 30 +++++++++++++++++++ 3 files changed, 50 insertions(+) create mode 100644 app/views/anonymous/test_notify_error.text.erb create mode 100644 spec/mailers/application_mailer_spec.rb diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index e894de7278..630698da85 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -8,4 +8,23 @@ class ApplicationMailer < Mail::Notify::Mailer "GOVUK_NOTIFY_TEMPLATE_ID_APPLICATION", "95adafaf-0920-4623-bddc-340853c047af", ) + + rescue_from Notifications::Client::RequestError do + # WARNING: this needs to be a block, otherwise the exception will not be + # re-raised and we will not be notified via Sentry, and the job will not retry. + # + # @see https://github.com/rails/rails/issues/39018 + if respond_to?(:headers) + binding.break + email = Email.find(headers['email-log-id'].to_s) + email.update!(delivery_status: 'notify_error') + end + + raise + end + + def notify_email(headers) + headers = headers.merge(rails_mailer: mailer_name, rails_mail_template: action_name) + view_mail(GOVUK_NOTIFY_TEMPLATE_ID, headers) + end end diff --git a/app/views/anonymous/test_notify_error.text.erb b/app/views/anonymous/test_notify_error.text.erb new file mode 100644 index 0000000000..5110cc44ff --- /dev/null +++ b/app/views/anonymous/test_notify_error.text.erb @@ -0,0 +1 @@ +This is used by the ApplicationMailer spec. diff --git a/spec/mailers/application_mailer_spec.rb b/spec/mailers/application_mailer_spec.rb new file mode 100644 index 0000000000..d38b120d3d --- /dev/null +++ b/spec/mailers/application_mailer_spec.rb @@ -0,0 +1,30 @@ +require 'rails_helper' + +RSpec.describe ApplicationMailer, :sidekiq do + describe '.rescue_from' do + fake_mailer = Class.new(ApplicationMailer) do + self.delivery_method = :notify + self.notify_settings = { + api_key: 'not-real-e1f4c969-b675-4a0d-a14d-623e7c2d3fd8-24fea27b-824e-4259-b5ce-1badafe98150', + } + + def test_notify_error + notify_email( + to: 'test@example.com', + subject: 'Some subject', + ) + end + end + + it 'marks errors as failed and re-raises the error' do + stub_request(:post, 'https://api.notifications.service.gov.uk/v2/notifications/email') + .to_return(status: 404) + + expect { + fake_mailer.test_notify_error.deliver_now + }.to raise_error(Notifications::Client::NotFoundError) + + expect(Email.last.delivery_status).to eql('notify_error') + end + end +end From f09cfe0fcda579015b202e9894242e18b7971b1f Mon Sep 17 00:00:00 2001 From: Shujat Khalid Date: Thu, 15 Aug 2024 14:46:34 +0100 Subject: [PATCH 2/8] Save email to MailDeliveryFailure --- app/mailers/application_mailer.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index 630698da85..56881fd967 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -15,9 +15,10 @@ class ApplicationMailer < Mail::Notify::Mailer # # @see https://github.com/rails/rails/issues/39018 if respond_to?(:headers) - binding.break - email = Email.find(headers['email-log-id'].to_s) - email.update!(delivery_status: 'notify_error') + recipient_email = headers['To'].value + MailDeliveryFailure.create!( + recipient_email: recipient_email + ) end raise From 55f0780d8c75b25e2fee45ad21d88d5fb5cfc276 Mon Sep 17 00:00:00 2001 From: Shujat Khalid Date: Thu, 15 Aug 2024 15:25:55 +0100 Subject: [PATCH 3/8] linting fix --- app/mailers/application_mailer.rb | 33 +++++++++---------- spec/mailers/application_mailer_spec.rb | 43 +++++++++++++------------ 2 files changed, 39 insertions(+), 37 deletions(-) diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index 56881fd967..83e7530d5c 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -9,23 +9,22 @@ class ApplicationMailer < Mail::Notify::Mailer "95adafaf-0920-4623-bddc-340853c047af", ) - rescue_from Notifications::Client::RequestError do - # WARNING: this needs to be a block, otherwise the exception will not be - # re-raised and we will not be notified via Sentry, and the job will not retry. - # - # @see https://github.com/rails/rails/issues/39018 - if respond_to?(:headers) - recipient_email = headers['To'].value - MailDeliveryFailure.create!( - recipient_email: recipient_email - ) - end - - raise + rescue_from Notifications::Client::RequestError do + # WARNING: this needs to be a block, otherwise the exception will not be + # re-raised and we will not be notified via Sentry, and the job will not retry. + # + # @see https://github.com/rails/rails/issues/39018 + if respond_to?(:headers) + recipient_email = headers["To"].value + MailDeliveryFailure.create!(recipient_email:) end - def notify_email(headers) - headers = headers.merge(rails_mailer: mailer_name, rails_mail_template: action_name) - view_mail(GOVUK_NOTIFY_TEMPLATE_ID, headers) - end + raise + end + + def notify_email(headers) + headers = + headers.merge(rails_mailer: mailer_name, rails_mail_template: action_name) + view_mail(GOVUK_NOTIFY_TEMPLATE_ID, headers) + end end diff --git a/spec/mailers/application_mailer_spec.rb b/spec/mailers/application_mailer_spec.rb index d38b120d3d..a786e60c74 100644 --- a/spec/mailers/application_mailer_spec.rb +++ b/spec/mailers/application_mailer_spec.rb @@ -1,30 +1,33 @@ -require 'rails_helper' +# frozen_string_literal: true + +require "rails_helper" RSpec.describe ApplicationMailer, :sidekiq do - describe '.rescue_from' do - fake_mailer = Class.new(ApplicationMailer) do - self.delivery_method = :notify - self.notify_settings = { - api_key: 'not-real-e1f4c969-b675-4a0d-a14d-623e7c2d3fd8-24fea27b-824e-4259-b5ce-1badafe98150', - } + describe ".rescue_from" do + fake_mailer = + Class.new(ApplicationMailer) do + self.delivery_method = :notify + self.notify_settings = { + api_key: + "not-real-e1f4c969-b675-4a0d-a14d-623e7c2d3fd8-24fea27b-824e-4259-b5ce-1badafe98150", + } - def test_notify_error - notify_email( - to: 'test@example.com', - subject: 'Some subject', - ) + def test_notify_error + notify_email(to: "test@example.com", subject: "Some subject") + end end - end - it 'marks errors as failed and re-raises the error' do - stub_request(:post, 'https://api.notifications.service.gov.uk/v2/notifications/email') - .to_return(status: 404) + it "marks errors as failed and re-raises the error" do + stub_request( + :post, + "https://api.notifications.service.gov.uk/v2/notifications/email", + ).to_return(status: 404) - expect { - fake_mailer.test_notify_error.deliver_now - }.to raise_error(Notifications::Client::NotFoundError) + expect { fake_mailer.test_notify_error.deliver_now }.to raise_error( + Notifications::Client::NotFoundError, + ) - expect(Email.last.delivery_status).to eql('notify_error') + expect(Email.last.delivery_status).to eql("notify_error") end end end From 0e8c245b9e8b33479d3bad751a364dde85ded0fd Mon Sep 17 00:00:00 2001 From: Shujat Khalid Date: Fri, 16 Aug 2024 10:08:36 +0100 Subject: [PATCH 4/8] Able to save all elements of maildeliveryfailure --- app/mailers/application_mailer.rb | 8 ++++---- spec/mailers/application_mailer_spec.rb | 6 +++++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index 83e7530d5c..e28f9f1f8f 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -15,16 +15,16 @@ class ApplicationMailer < Mail::Notify::Mailer # # @see https://github.com/rails/rails/issues/39018 if respond_to?(:headers) - recipient_email = headers["To"].value - MailDeliveryFailure.create!(recipient_email:) + email_address = headers["To"].value + mailer_action_method = action_name + mailer_class = mailer_name + MailDeliveryFailure.create!(email_address:, mailer_action_method:, mailer_class:) end raise end def notify_email(headers) - headers = - headers.merge(rails_mailer: mailer_name, rails_mail_template: action_name) view_mail(GOVUK_NOTIFY_TEMPLATE_ID, headers) end end diff --git a/spec/mailers/application_mailer_spec.rb b/spec/mailers/application_mailer_spec.rb index a786e60c74..b19a896335 100644 --- a/spec/mailers/application_mailer_spec.rb +++ b/spec/mailers/application_mailer_spec.rb @@ -27,7 +27,11 @@ def test_notify_error Notifications::Client::NotFoundError, ) - expect(Email.last.delivery_status).to eql("notify_error") + failure = MailDeliveryFailure.last + expect(failure).not_to be_nil + expect(failure.email_address).to eq("test@example.com") + expect(failure.mailer_action_method).to eq("test_notify_error") + expect(failure.mailer_class).to eq("anonymous") end end end From cdd8878ed6c0d2884dad778887f01b0d7e70fd2c Mon Sep 17 00:00:00 2001 From: Shujat Khalid Date: Fri, 16 Aug 2024 10:25:37 +0100 Subject: [PATCH 5/8] linting fix --- app/mailers/application_mailer.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index e28f9f1f8f..062c7a5ca7 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -18,7 +18,11 @@ class ApplicationMailer < Mail::Notify::Mailer email_address = headers["To"].value mailer_action_method = action_name mailer_class = mailer_name - MailDeliveryFailure.create!(email_address:, mailer_action_method:, mailer_class:) + MailDeliveryFailure.create!( + email_address:, + mailer_action_method:, + mailer_class:, + ) end raise From 85517cdbf9f40e2a004a720243846552582d31a3 Mon Sep 17 00:00:00 2001 From: Shujat Khalid Date: Mon, 19 Aug 2024 10:27:30 +0100 Subject: [PATCH 6/8] Made changes according to comments --- spec/mailers/application_mailer_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/mailers/application_mailer_spec.rb b/spec/mailers/application_mailer_spec.rb index b19a896335..bbc297fa86 100644 --- a/spec/mailers/application_mailer_spec.rb +++ b/spec/mailers/application_mailer_spec.rb @@ -2,7 +2,7 @@ require "rails_helper" -RSpec.describe ApplicationMailer, :sidekiq do +RSpec.describe ApplicationMailer, type: :mailer do describe ".rescue_from" do fake_mailer = Class.new(ApplicationMailer) do From 7fd9b596d3708ce75edfc1fa8d02f41e730af882 Mon Sep 17 00:00:00 2001 From: Shujat Khalid Date: Thu, 29 Aug 2024 15:14:43 +0100 Subject: [PATCH 7/8] replace notify_email with view_mail --- app/mailers/application_mailer.rb | 4 ---- spec/mailers/application_mailer_spec.rb | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index 062c7a5ca7..764423d4f1 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -27,8 +27,4 @@ class ApplicationMailer < Mail::Notify::Mailer raise end - - def notify_email(headers) - view_mail(GOVUK_NOTIFY_TEMPLATE_ID, headers) - end end diff --git a/spec/mailers/application_mailer_spec.rb b/spec/mailers/application_mailer_spec.rb index bbc297fa86..ef782fbee1 100644 --- a/spec/mailers/application_mailer_spec.rb +++ b/spec/mailers/application_mailer_spec.rb @@ -13,7 +13,7 @@ } def test_notify_error - notify_email(to: "test@example.com", subject: "Some subject") + view_mail("testGOVUK_NOTIFY_TEMPLATE_ID_APPLICATION", to: "test@example.com", subject: "Some subject") end end From bc18745868066ce5d9d789c936ee95c1675c9d74 Mon Sep 17 00:00:00 2001 From: Shujat Khalid Date: Fri, 30 Aug 2024 16:33:44 +0100 Subject: [PATCH 8/8] Using guid for template value --- spec/mailers/application_mailer_spec.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/spec/mailers/application_mailer_spec.rb b/spec/mailers/application_mailer_spec.rb index ef782fbee1..18f8164d3e 100644 --- a/spec/mailers/application_mailer_spec.rb +++ b/spec/mailers/application_mailer_spec.rb @@ -13,7 +13,11 @@ } def test_notify_error - view_mail("testGOVUK_NOTIFY_TEMPLATE_ID_APPLICATION", to: "test@example.com", subject: "Some subject") + view_mail( + ApplicationMailer::GOVUK_NOTIFY_TEMPLATE_ID, + to: "test@example.com", + subject: "Some subject", + ) end end