Refactor TeacherMailer to take the application form

Currently the TeacherMailer takes in the teacher as a parameter and then extracts the most recent application form from it. This works in most cases, but there are a few specific cases where this causes problems, notably if the teacher has multiple application forms. In this case we can end up choosing the incorrect application form and emails can end up associated with the wrong application form.
DFE-Digital · Jan 18, 2024 · e6af8e8 · e6af8e8
1 parent 1d17bd9
commit e6af8e8
Show file tree

Hide file tree

Showing 23 changed files with 70 additions and 83 deletions.
diff --git a/app/controllers/assessor_interface/assessment_sections_controller.rb b/app/controllers/assessor_interface/assessment_sections_controller.rb
@@ -96,10 +96,7 @@ def notify_teacher
         return
       end
 
-      TeacherMailer
-        .with(teacher: application_form.teacher)
-        .initial_checks_passed
-        .deliver_later
+      TeacherMailer.with(application_form:).initial_checks_passed.deliver_later
     end
 
     def unassign_assessor

diff --git a/app/mailers/teacher_mailer.rb b/app/mailers/teacher_mailer.rb
@@ -4,9 +4,6 @@ class TeacherMailer < ApplicationMailer
   include RegionHelper
 
   before_action :set_application_form
-  before_action :set_further_information_request,
-                only: :further_information_reminder
-  before_action :set_further_information_requested, only: :application_declined
 
   helper :application_form, :region
 
@@ -76,6 +73,8 @@ def further_information_requested
   end
 
   def further_information_reminder
+    @further_information_request = params[:further_information_request]
+
     view_mail(
       GOVUK_NOTIFY_TEMPLATE_ID,
       to: teacher.email,
@@ -129,23 +128,13 @@ def references_requested
 
   private
 
-  def teacher
-    params[:teacher]
+  def application_form
+    params[:application_form]
   end
 
-  delegate :application_form, to: :teacher
-  delegate :assessment, :region, to: :application_form
+  delegate :assessment, :region, :teacher, to: :application_form
 
   def set_application_form
     @application_form = application_form
   end
-
-  def set_further_information_request
-    @further_information_request = params[:further_information_request]
-  end
-
-  def set_further_information_requested
-    @further_information_requested =
-      assessment.further_information_requests.any?
-  end
 end
diff --git a/app/models/application_form.rb b/app/models/application_form.rb
@@ -267,13 +267,13 @@ def send_reminder_email(name, number_of_reminders_sent)
     case name
     when "expiration"
       TeacherMailer
-        .with(teacher:, number_of_reminders_sent:)
+        .with(application_form: self, number_of_reminders_sent:)
         .application_not_submitted
         .deliver_later
     when "references"
       TeacherMailer
         .with(
-          teacher:,
+          application_form: self,
           number_of_reminders_sent:,
           reference_requests:
             reference_requests_not_yet_received_or_rejected.to_a,

diff --git a/app/models/further_information_request.rb b/app/models/further_information_request.rb
@@ -48,7 +48,7 @@ def should_send_reminder_email?(_name, number_of_reminders_sent)
 
   def send_reminder_email(_name, _number_of_reminders_sent)
     TeacherMailer
-      .with(teacher:, further_information_request: self)
+      .with(application_form:, further_information_request: self)
       .further_information_reminder
       .deliver_later
   end
@@ -63,7 +63,10 @@ def expires_after
   end
 
   def after_received(*)
-    TeacherMailer.with(teacher:).further_information_received.deliver_later
+    TeacherMailer
+      .with(application_form:)
+      .further_information_received
+      .deliver_later
   end
 
   def after_expired(user:)

diff --git a/app/models/professional_standing_request.rb b/app/models/professional_standing_request.rb
@@ -40,7 +40,10 @@ def expires_after
 
   def after_received(*)
     if should_send_received_email?
-      TeacherMailer.with(teacher:).professional_standing_received.deliver_later
+      TeacherMailer
+        .with(application_form:)
+        .professional_standing_received
+        .deliver_later
     end
   end
 

diff --git a/app/services/award_qts.rb b/app/services/award_qts.rb
@@ -24,12 +24,15 @@ def call
     raise MissingTRN if trn.blank?
 
     ActiveRecord::Base.transaction do
-      teacher.update!(trn:, access_your_teaching_qualifications_url:)
+      application_form.teacher.update!(
+        trn:,
+        access_your_teaching_qualifications_url:,
+      )
       application_form.update!(awarded_at: Time.zone.now)
       ApplicationFormStatusUpdater.call(application_form:, user:)
     end
 
-    TeacherMailer.with(teacher:).application_awarded.deliver_later
+    TeacherMailer.with(application_form:).application_awarded.deliver_later
   end
 
   class InvalidState < StandardError
@@ -44,6 +47,4 @@ class MissingTRN < StandardError
               :user,
               :trn,
               :access_your_teaching_qualifications_url
-
-  delegate :teacher, to: :application_form
 end
diff --git a/app/services/create_further_information_request.rb b/app/services/create_further_information_request.rb
@@ -29,7 +29,10 @@ def call
         requestable
       end
 
-    TeacherMailer.with(teacher:).further_information_requested.deliver_later
+    TeacherMailer
+      .with(application_form:)
+      .further_information_requested
+      .deliver_later
 
     further_information_request
   end
@@ -38,11 +41,5 @@ def call
 
   attr_reader :assessment, :user
 
-  def application_form
-    @application_form ||= assessment.application_form
-  end
-
-  def teacher
-    @teacher ||= application_form.teacher
-  end
+  delegate :application_form, to: :assessment
 end
diff --git a/app/services/decline_qts.rb b/app/services/decline_qts.rb
@@ -16,11 +16,10 @@ def call
       ApplicationFormStatusUpdater.call(application_form:, user:)
     end
 
-    TeacherMailer.with(teacher:).application_declined.deliver_later
+    TeacherMailer.with(application_form:).application_declined.deliver_later
   end
 
   private
 
   attr_reader :application_form, :user
-  delegate :teacher, to: :application_form
 end
diff --git a/app/services/submit_application_form.rb b/app/services/submit_application_form.rb
@@ -32,17 +32,11 @@ def call
       ApplicationFormStatusUpdater.call(application_form:, user:)
     end
 
-    TeacherMailer
-      .with(teacher: application_form.teacher)
-      .application_received
-      .deliver_later
+    TeacherMailer.with(application_form:).application_received.deliver_later
 
     if !application_form.requires_preliminary_check &&
          application_form.teaching_authority_provides_written_statement
-      TeacherMailer
-        .with(teacher: application_form.teacher)
-        .initial_checks_passed
-        .deliver_later
+      TeacherMailer.with(application_form:).initial_checks_passed.deliver_later
     end
 
     if region.teaching_authority_requires_submission_email

diff --git a/app/services/update_work_history_contact.rb b/app/services/update_work_history_contact.rb
@@ -28,7 +28,7 @@ def call
     if email.present? && (reference_request = work_history.reference_request)
       RefereeMailer.with(reference_request:).reference_requested.deliver_later
       TeacherMailer
-        .with(teacher:, reference_requests: [reference_request])
+        .with(application_form:, reference_requests: [reference_request])
         .references_requested
         .deliver_later
     end
@@ -39,7 +39,6 @@ def call
   attr_reader :work_history, :user, :name, :job, :email
 
   delegate :application_form, to: :work_history
-  delegate :teacher, to: :application_form
 
   def change_value(column_name, new_value)
     old_value = work_history.send(column_name)

diff --git a/app/services/verify_assessment.rb b/app/services/verify_assessment.rb
@@ -52,9 +52,7 @@ class AlreadyVerified < StandardError
               :work_histories
 
   delegate :application_form, to: :assessment
-  delegate :teacher,
-           :teaching_authority_provides_written_statement,
-           to: :application_form
+  delegate :teaching_authority_provides_written_statement, to: :application_form
 
   def create_professional_standing_request
     if professional_standing && !teaching_authority_provides_written_statement
@@ -84,7 +82,7 @@ def send_reference_request_emails(reference_requests)
     end
 
     TeacherMailer
-      .with(teacher:, reference_requests:)
+      .with(application_form:, reference_requests:)
       .references_requested
       .deliver_later
   end

diff --git a/app/views/assessor_interface/assessment_recommendation_award/preview.html.erb b/app/views/assessor_interface/assessment_recommendation_award/preview.html.erb
@@ -8,7 +8,7 @@
 <%= render(PreviewMailer::Component.new(
   mailer_class: TeacherMailer,
   name: :application_awarded,
-  teacher: @application_form.teacher,
+  application_form: @application_form,
 )) %>
 
 <div class="govuk-button-group">

diff --git a/app/views/assessor_interface/assessment_recommendation_decline/preview.html.erb b/app/views/assessor_interface/assessment_recommendation_decline/preview.html.erb
@@ -8,7 +8,7 @@
 <%= render(PreviewMailer::Component.new(
   mailer_class: TeacherMailer,
   name: :application_declined,
-  teacher: @application_form.teacher,
+  application_form: @application_form,
 )) %>
 
 <div class="govuk-button-group">

diff --git a/app/views/assessor_interface/assessment_recommendation_verify/preview_teacher.html.erb b/app/views/assessor_interface/assessment_recommendation_verify/preview_teacher.html.erb
@@ -10,7 +10,7 @@
 <%= render(PreviewMailer::Component.new(
   mailer_class: TeacherMailer,
   name: :references_requested,
-  teacher: @application_form.teacher,
+  application_form: @application_form,
   reference_requests: [OpenStruct.new(
     expires_at: Time.zone.now + 6.weeks,
   )],

diff --git a/app/views/assessor_interface/further_information_requests/new.html.erb b/app/views/assessor_interface/further_information_requests/new.html.erb
@@ -6,7 +6,7 @@
 <%= render(PreviewMailer::Component.new(
   mailer_class: TeacherMailer,
   name: :further_information_requested,
-  teacher: @application_form.teacher,
+  application_form: @application_form,
 )) %>
 
 <%= form_with model: [:assessor_interface, @application_form, @assessment, @further_information_request] do |f| %>

diff --git a/lib/tasks/application_forms.rake b/lib/tasks/application_forms.rake
@@ -29,7 +29,7 @@ namespace :application_forms do
       next unless teacher.application_form == application_form
 
       TeacherMailer
-        .with(teacher:)
+        .with(application_form:)
         .application_from_ineligible_country
         .deliver_now
     end

diff --git a/spec/lib/application_mailer_observer_spec.rb b/spec/lib/application_mailer_observer_spec.rb
@@ -31,9 +31,7 @@
 
   context "with a teacher mailer" do
     let(:application_form) { create(:application_form) }
-    let(:message) do
-      TeacherMailer.with(teacher: application_form.teacher).application_received
-    end
+    let(:message) { TeacherMailer.with(application_form:).application_received }
 
     include_examples "observes mail"
   end

diff --git a/spec/lib/mailer_preview_spec.rb b/spec/lib/mailer_preview_spec.rb
@@ -2,9 +2,10 @@
 
 RSpec.describe MailerPreview do
   let(:further_information_request) { create(:further_information_request) }
-  let(:teacher) do
-    further_information_request.assessment.application_form.teacher
+  let(:application_form) do
+    further_information_request.assessment.application_form
   end
+  let(:teacher) { application_form.teacher }
   let(:notify_key) { "notify-key" }
   let(:notify_client) do
     double(generate_template_preview: notify_template_preview)
@@ -27,7 +28,7 @@
     subject do
       described_class.with(
         TeacherMailer,
-        teacher:,
+        application_form:,
         further_information_request:,
       ).further_information_requested
     end