From be47d08e595a921f6524d22e66fbd7b21d147e18 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Thu, 4 Apr 2024 16:25:41 +0100 Subject: [PATCH] Add DeliverEmail This adds a service for encapsulating the logic related to sending emails and recording the emails in the timeline. This allows us to send emails in the fake data and record the correct time rather than adding the timeline event in the Sidekiq process. --- .../assessment_sections_controller.rb | 6 +- .../consent_requests_controller.rb | 6 +- .../consent_requests_controller.rb | 6 +- app/lib/application_mailer_observer.rb | 28 --------- app/mailers/application_mailer.rb | 18 +----- app/models/application_form.rb | 33 ++++++----- app/models/further_information_request.rb | 19 ++++--- app/models/professional_standing_request.rb | 9 +-- app/models/reference_request.rb | 11 ++-- app/services/award_qts.rb | 6 +- app/services/decline_qts.rb | 6 +- app/services/deliver_email.rb | 40 +++++++++++++ app/services/request_further_information.rb | 9 +-- app/services/submit_application_form.rb | 21 +++++-- app/services/update_work_history_contact.rb | 18 ++++-- app/services/verify_assessment.rb | 17 ++++-- lib/tasks/application_forms.rake | 9 +-- spec/lib/application_mailer_observer_spec.rb | 57 ------------------- spec/mailers/referee_mailer_spec.rb | 4 -- spec/mailers/teacher_mailer_spec.rb | 29 ---------- .../mailers/teaching_authority_mailer_spec.rb | 2 - .../shared_examples/observable_mailer.rb | 27 --------- 22 files changed, 159 insertions(+), 222 deletions(-) delete mode 100644 app/lib/application_mailer_observer.rb create mode 100644 app/services/deliver_email.rb delete mode 100644 spec/lib/application_mailer_observer_spec.rb delete mode 100644 spec/support/shared_examples/observable_mailer.rb diff --git a/app/controllers/assessor_interface/assessment_sections_controller.rb b/app/controllers/assessor_interface/assessment_sections_controller.rb index a929800641..ff7fec0211 100644 --- a/app/controllers/assessor_interface/assessment_sections_controller.rb +++ b/app/controllers/assessor_interface/assessment_sections_controller.rb @@ -98,7 +98,11 @@ def notify_teacher return end - TeacherMailer.with(application_form:).initial_checks_passed.deliver_later + DeliverEmail.call( + application_form:, + mailer: TeacherMailer, + action: :initial_checks_passed, + ) end def unassign_assessor diff --git a/app/controllers/assessor_interface/consent_requests_controller.rb b/app/controllers/assessor_interface/consent_requests_controller.rb index 0da446a96e..a27c02d651 100644 --- a/app/controllers/assessor_interface/consent_requests_controller.rb +++ b/app/controllers/assessor_interface/consent_requests_controller.rb @@ -52,7 +52,11 @@ def update_request ) end - TeacherMailer.with(application_form:).consent_requested.deliver_later + DeliverEmail.call( + application_form:, + mailer: TeacherMailer, + action: :consent_requested, + ) end redirect_to [ diff --git a/app/controllers/teacher_interface/consent_requests_controller.rb b/app/controllers/teacher_interface/consent_requests_controller.rb index 57c2959793..c2f5452d9a 100644 --- a/app/controllers/teacher_interface/consent_requests_controller.rb +++ b/app/controllers/teacher_interface/consent_requests_controller.rb @@ -26,7 +26,11 @@ def submit end end - TeacherMailer.with(application_form:).consent_submitted.deliver_later + DeliverEmail.call( + application_form:, + mailer: TeacherMailer, + action: :consent_submitted, + ) redirect_to %i[teacher_interface application_form] end diff --git a/app/lib/application_mailer_observer.rb b/app/lib/application_mailer_observer.rb deleted file mode 100644 index 0c9e6455a9..0000000000 --- a/app/lib/application_mailer_observer.rb +++ /dev/null @@ -1,28 +0,0 @@ -# frozen_string_literal: true - -class ApplicationMailerObserver - def self.delivered_email(message) - mailer_class_name = message.try(:mailer_class_name) - mailer_action_name = message.try(:mailer_action_name) - application_form_id = message.try(:application_form_id) - message_subject = message.try(:subject) - - if mailer_class_name.blank? || mailer_action_name.blank? || - application_form_id.blank? || message_subject.blank? - return - end - - application_form = ApplicationForm.find(application_form_id) - - CreateTimelineEvent.call( - "email_sent", - application_form:, - user: "Mailer", - mailer_class_name:, - mailer_action_name:, - message_subject:, - ) - end -end - -ActionMailer::Base.register_observer(ApplicationMailerObserver) diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index 0c95fd1a12..e894de7278 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class ApplicationMailer < Mail::Notify::Mailer default from: I18n.t("service.email.enquiries") @@ -6,20 +8,4 @@ class ApplicationMailer < Mail::Notify::Mailer "GOVUK_NOTIFY_TEMPLATE_ID_APPLICATION", "95adafaf-0920-4623-bddc-340853c047af", ) - - after_action :store_observer_metadata - - def store_observer_metadata - mailer_class_name = self.class.name.demodulize - mailer_action_name = action_name - application_form_id = application_form.id - - message.instance_variable_set(:@mailer_class_name, mailer_class_name) - message.instance_variable_set(:@mailer_action_name, mailer_action_name) - message.instance_variable_set(:@application_form_id, application_form_id) - - message.class.send(:attr_reader, :mailer_class_name) - message.class.send(:attr_reader, :mailer_action_name) - message.class.send(:attr_reader, :application_form_id) - end end diff --git a/app/models/application_form.rb b/app/models/application_form.rb index d2dfd08094..c1bc10a0a4 100644 --- a/app/models/application_form.rb +++ b/app/models/application_form.rb @@ -272,22 +272,27 @@ def should_send_reminder_email?(name, number_of_reminders_sent) def send_reminder_email(name, number_of_reminders_sent) case name when "consent" - TeacherMailer.with(application_form: self).consent_reminder.deliver_later + DeliverEmail.call( + application_form: self, + mailer: TeacherMailer, + action: :consent_reminder, + ) when "expiration" - TeacherMailer - .with(application_form: self, number_of_reminders_sent:) - .application_not_submitted - .deliver_later + DeliverEmail.call( + application_form: self, + mailer: TeacherMailer, + action: :application_not_submitted, + number_of_reminders_sent:, + ) when "references" - TeacherMailer - .with( - application_form: self, - number_of_reminders_sent:, - reference_requests: - reference_requests_not_yet_received_or_rejected.to_a, - ) - .references_reminder - .deliver_later + DeliverEmail.call( + application_form: self, + mailer: TeacherMailer, + action: :references_reminder, + number_of_reminders_sent:, + reference_requests: + reference_requests_not_yet_received_or_rejected.to_a, + ) end end diff --git a/app/models/further_information_request.rb b/app/models/further_information_request.rb index 3dda340c02..fa67b342ef 100644 --- a/app/models/further_information_request.rb +++ b/app/models/further_information_request.rb @@ -47,10 +47,12 @@ def should_send_reminder_email?(_name, number_of_reminders_sent) end def send_reminder_email(_name, _number_of_reminders_sent) - TeacherMailer - .with(application_form:, further_information_request: self) - .further_information_reminder - .deliver_later + DeliverEmail.call( + application_form:, + mailer: TeacherMailer, + action: :further_information_reminder, + further_information_request: self, + ) end def expires_after @@ -63,10 +65,11 @@ def expires_after end def after_received(*) - TeacherMailer - .with(application_form:) - .further_information_received - .deliver_later + DeliverEmail.call( + application_form:, + mailer: TeacherMailer, + action: :further_information_received, + ) end def after_expired(user:) diff --git a/app/models/professional_standing_request.rb b/app/models/professional_standing_request.rb index 3886a1e9d1..fc1f5a208a 100644 --- a/app/models/professional_standing_request.rb +++ b/app/models/professional_standing_request.rb @@ -40,10 +40,11 @@ def expires_after def after_received(*) if should_send_received_email? - TeacherMailer - .with(application_form:) - .professional_standing_received - .deliver_later + DeliverEmail.call( + application_form:, + mailer: TeacherMailer, + action: :professional_standing_received, + ) end end diff --git a/app/models/reference_request.rb b/app/models/reference_request.rb index 0d61d0c917..27b72842a8 100644 --- a/app/models/reference_request.rb +++ b/app/models/reference_request.rb @@ -100,10 +100,13 @@ def should_send_reminder_email?(_name, number_of_reminders_sent) end def send_reminder_email(_name, number_of_reminders_sent) - RefereeMailer - .with(reference_request: self, number_of_reminders_sent:) - .reference_reminder - .deliver_later + DeliverEmail.call( + application_form:, + mailer: RefereeMailer, + action: :reference_reminder, + reference_request: self, + number_of_reminders_sent:, + ) end def expires_after diff --git a/app/services/award_qts.rb b/app/services/award_qts.rb index 84228fe071..a5c0c759e9 100644 --- a/app/services/award_qts.rb +++ b/app/services/award_qts.rb @@ -32,7 +32,11 @@ def call ApplicationFormStatusUpdater.call(application_form:, user:) end - TeacherMailer.with(application_form:).application_awarded.deliver_later + DeliverEmail.call( + application_form:, + mailer: TeacherMailer, + action: :application_awarded, + ) end class InvalidState < StandardError diff --git a/app/services/decline_qts.rb b/app/services/decline_qts.rb index d22007b70e..0393544ec5 100644 --- a/app/services/decline_qts.rb +++ b/app/services/decline_qts.rb @@ -17,7 +17,11 @@ def call CreateTimelineEvent.call("application_declined", application_form:, user:) end - TeacherMailer.with(application_form:).application_declined.deliver_later + DeliverEmail.call( + application_form:, + mailer: TeacherMailer, + action: :application_declined, + ) end private diff --git a/app/services/deliver_email.rb b/app/services/deliver_email.rb new file mode 100644 index 0000000000..f7f1dcddb1 --- /dev/null +++ b/app/services/deliver_email.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +class DeliverEmail + include ServicePattern + + def initialize(application_form:, mailer:, action:, **params) + @application_form = application_form + @mailer = mailer + @action = action + @params = params + end + + def call + create_job + create_timeline_event + end + + private + + attr_reader :application_form, :mailer, :action, :params + + def mail + @mail ||= mailer.with(application_form:, **params).send(action) + end + + def create_job + mail.deliver_later + end + + def create_timeline_event + CreateTimelineEvent.call( + "email_sent", + application_form:, + user: "Mailer", + mailer_class_name: mailer.name.demodulize, + mailer_action_name: action, + message_subject: mail.subject, + ) + end +end diff --git a/app/services/request_further_information.rb b/app/services/request_further_information.rb index 63b11d8ee6..5bedd96074 100644 --- a/app/services/request_further_information.rb +++ b/app/services/request_further_information.rb @@ -48,9 +48,10 @@ def create_and_request end def send_email - TeacherMailer - .with(application_form:) - .further_information_requested - .deliver_later + DeliverEmail.call( + application_form:, + mailer: TeacherMailer, + action: :further_information_requested, + ) end end diff --git a/app/services/submit_application_form.rb b/app/services/submit_application_form.rb index bfa0ff90a5..ddfb23d373 100644 --- a/app/services/submit_application_form.rb +++ b/app/services/submit_application_form.rb @@ -32,18 +32,27 @@ def call ApplicationFormStatusUpdater.call(application_form:, user:) end - TeacherMailer.with(application_form:).application_received.deliver_later + DeliverEmail.call( + application_form:, + mailer: TeacherMailer, + action: :application_received, + ) if !application_form.requires_preliminary_check && application_form.teaching_authority_provides_written_statement - TeacherMailer.with(application_form:).initial_checks_passed.deliver_later + DeliverEmail.call( + application_form:, + mailer: TeacherMailer, + action: :initial_checks_passed, + ) end if region.teaching_authority_requires_submission_email - TeachingAuthorityMailer - .with(application_form:) - .application_submitted - .deliver_later + DeliverEmail.call( + application_form:, + mailer: TeachingAuthorityMailer, + action: :application_submitted, + ) end FindApplicantInDQTJob.perform_later( diff --git a/app/services/update_work_history_contact.rb b/app/services/update_work_history_contact.rb index df2674e2ca..0e414690c1 100644 --- a/app/services/update_work_history_contact.rb +++ b/app/services/update_work_history_contact.rb @@ -29,11 +29,19 @@ def call end if email.present? && (reference_request = work_history.reference_request) - RefereeMailer.with(reference_request:).reference_requested.deliver_later - TeacherMailer - .with(application_form:, reference_requests: [reference_request]) - .references_requested - .deliver_later + DeliverEmail.call( + application_form:, + mailer: RefereeMailer, + action: :reference_requested, + reference_request:, + ) + + DeliverEmail.call( + application_form:, + mailer: TeacherMailer, + action: :references_requested, + reference_requests: [reference_request], + ) end end diff --git a/app/services/verify_assessment.rb b/app/services/verify_assessment.rb index 5971b62185..5ff476d2ec 100644 --- a/app/services/verify_assessment.rb +++ b/app/services/verify_assessment.rb @@ -82,12 +82,19 @@ def create_reference_requests def send_reference_request_emails(reference_requests) reference_requests.each do |reference_request| - RefereeMailer.with(reference_request:).reference_requested.deliver_later + DeliverEmail.call( + application_form:, + mailer: RefereeMailer, + action: :reference_requested, + reference_request:, + ) end - TeacherMailer - .with(application_form:, reference_requests:) - .references_requested - .deliver_later + DeliverEmail.call( + application_form:, + mailer: TeacherMailer, + action: :references_requested, + reference_requests:, + ) end end diff --git a/lib/tasks/application_forms.rake b/lib/tasks/application_forms.rake index 996bf7b182..567e681b44 100644 --- a/lib/tasks/application_forms.rake +++ b/lib/tasks/application_forms.rake @@ -28,10 +28,11 @@ namespace :application_forms do teacher = application_form.teacher next unless teacher.application_form == application_form - TeacherMailer - .with(application_form:) - .application_from_ineligible_country - .deliver_now + DeliverEmail.call( + application_form:, + mailer: TeacherMailer, + action: :application_from_ineligible_country, + ) end end diff --git a/spec/lib/application_mailer_observer_spec.rb b/spec/lib/application_mailer_observer_spec.rb deleted file mode 100644 index 384b453a7c..0000000000 --- a/spec/lib/application_mailer_observer_spec.rb +++ /dev/null @@ -1,57 +0,0 @@ -# frozen_string_literal: true - -require "rails_helper" - -RSpec.describe ApplicationMailerObserver do - shared_examples "observes mail" do - subject(:delivered_email) { described_class.delivered_email(message) } - - it "records a timeline event" do - expect { delivered_email }.to have_recorded_timeline_event( - :email_sent, - application_form:, - ) - end - - it "is called when an email is sent" do - expect(ApplicationMailerObserver).to receive(:delivered_email).with( - message, - ) - message.deliver_now - end - end - - context "with a referee mailer" do - let(:reference_request) { create(:requested_reference_request) } - let(:application_form) { reference_request.assessment.application_form } - let(:message) { RefereeMailer.with(reference_request:).reference_requested } - - include_examples "observes mail" - end - - context "with a teacher mailer" do - let(:application_form) { create(:application_form) } - let(:message) { TeacherMailer.with(application_form:).application_received } - - include_examples "observes mail" - end - - context "with a teaching authority mailer" do - let(:region) do - create(:region, teaching_authority_emails: ["authority@region.com"]) - end - let(:application_form) do - create( - :application_form, - :with_personal_information, - :with_teaching_qualification, - region:, - ) - end - let(:message) do - TeachingAuthorityMailer.with(application_form:).application_submitted - end - - include_examples "observes mail" - end -end diff --git a/spec/mailers/referee_mailer_spec.rb b/spec/mailers/referee_mailer_spec.rb index 01e845bb6f..de5aaf233e 100644 --- a/spec/mailers/referee_mailer_spec.rb +++ b/spec/mailers/referee_mailer_spec.rb @@ -60,8 +60,6 @@ ) end end - - it_behaves_like "an observable mailer", "reference_reminder" end describe "#reference_requested" do @@ -100,7 +98,5 @@ ) end end - - it_behaves_like "an observable mailer", "reference_requested" end end diff --git a/spec/mailers/teacher_mailer_spec.rb b/spec/mailers/teacher_mailer_spec.rb index edb51b76ef..cb8d9fd830 100644 --- a/spec/mailers/teacher_mailer_spec.rb +++ b/spec/mailers/teacher_mailer_spec.rb @@ -53,8 +53,6 @@ it { is_expected.to include("ABCDEF") } it { is_expected.to include("https://aytq.com") } end - - it_behaves_like "an observable mailer", "application_awarded" end describe "#application_declined" do @@ -81,8 +79,6 @@ it { is_expected.to include("abc") } it { is_expected.to include("Reason for decline") } end - - it_behaves_like "an observable mailer", "application_declined" end describe "#application_from_ineligible_country" do @@ -119,9 +115,6 @@ ) end end - - it_behaves_like "an observable mailer", - "application_from_ineligible_country" end describe "#application_not_submitted" do @@ -202,8 +195,6 @@ end end end - - it_behaves_like "an observable mailer", "application_not_submitted" end describe "#application_received" do @@ -254,8 +245,6 @@ end end end - - it_behaves_like "an observable mailer", "application_received" end describe "#consent_reminder" do @@ -300,8 +289,6 @@ is_expected.to include("If you do not send them by 12 February 2020") end end - - it_behaves_like "an observable mailer", "consent_reminder" end describe "#consent_requested" do @@ -345,8 +332,6 @@ is_expected.to include("upload these documents by 12 February 2020") end end - - it_behaves_like "an observable mailer", "consent_requested" end describe "#consent_submitted" do @@ -373,8 +358,6 @@ end it { is_expected.to include("Application reference number: abc") } end - - it_behaves_like "an observable mailer", "consent_submitted" end describe "#further_information_received" do @@ -404,8 +387,6 @@ it { is_expected.to include("Dear First Last") } it { is_expected.to include("abc") } end - - it_behaves_like "an observable mailer", "further_information_received" end describe "#further_information_requested" do @@ -447,8 +428,6 @@ end it { is_expected.to include("http://localhost:3000/teacher/sign_in") } end - - it_behaves_like "an observable mailer", "further_information_requested" end describe "#further_information_reminder" do @@ -496,8 +475,6 @@ it { is_expected.to include("abc") } it { is_expected.to include("http://localhost:3000/teacher/sign_in") } end - - it_behaves_like "an observable mailer", "further_information_reminder" end describe "#professional_standing_received" do @@ -535,8 +512,6 @@ ) end end - - it_behaves_like "an observable mailer", "professional_standing_received" end describe "#references_reminder" do @@ -611,8 +586,6 @@ end end end - - it_behaves_like "an observable mailer", "references_reminder" end describe "#references_requested" do @@ -647,7 +620,5 @@ ) end end - - it_behaves_like "an observable mailer", "references_requested" end end diff --git a/spec/mailers/teaching_authority_mailer_spec.rb b/spec/mailers/teaching_authority_mailer_spec.rb index ab79ca8dc6..1fcbedd73d 100644 --- a/spec/mailers/teaching_authority_mailer_spec.rb +++ b/spec/mailers/teaching_authority_mailer_spec.rb @@ -91,7 +91,5 @@ TRA is an executive agency, sponsored by the Department for Education. We have responsibility for the regulation of the teaching profession in England. EMAIL end - - it_behaves_like "an observable mailer", "application_submitted" end end diff --git a/spec/support/shared_examples/observable_mailer.rb b/spec/support/shared_examples/observable_mailer.rb deleted file mode 100644 index 1b9d3a0ce1..0000000000 --- a/spec/support/shared_examples/observable_mailer.rb +++ /dev/null @@ -1,27 +0,0 @@ -# frozen_string_literal: true - -RSpec.shared_examples_for "an observable mailer" do |expected_action_name| - describe "#application_form_id" do - subject(:application_form_id) { mail.application_form_id } - - it { is_expected.to eq(application_form.id) } - end - - describe "#mailer_action_name" do - subject(:mailer_action_name) { mail.mailer_action_name } - - it { is_expected.to eq(expected_action_name) } - end - - describe "#mailer_class_name" do - subject(:mailer_class_name) { mail.mailer_class_name } - - it { is_expected.to eq(described_class.name) } - end - - describe "#subject" do - subject(:subject) { mail.subject } - - it { is_expected.to be_present } - end -end