Merge pull request #5969 from nhsuk/rework-already-had-notification-sender

Rework already had notification sender

Author: MartinVanIJcken

app/jobs/search_vaccination_records_in_nhs_job.rb

@@ -45,14 +45,15 @@ def perform(patient_id)
       end
 
       # Remaining incoming_vaccination_records are new
-      incoming_vaccination_records.each do |vaccination_record|
-        vaccination_record.save!
-        AlreadyHadNotificationSender.call(vaccination_record:)
-      end
+      incoming_vaccination_records.each(&:save!)
 
       update_vaccination_search_timestamps if patient.nhs_number.present?
 
       PatientStatusUpdater.call(patient:)
+
+      incoming_vaccination_records.each do |vaccination_record|
+        AlreadyHadNotificationSender.call(vaccination_record:)
+      end
     end
   end
 

app/lib/already_had_notification_sender.rb

@@ -7,7 +7,8 @@ def initialize(vaccination_record:)
 
   def call
     return if vaccination_record.sourced_from_service?
-    return if would_still_be_vaccinated?
+    return if was_already_vaccinated?
+    return if is_still_eligible_for_vaccination?
 
     consents = patient.consents.includes(:parent)
 
@@ -58,15 +59,25 @@ def self.call(...) = new(...).call
 
   attr_reader :vaccination_record
 
-  delegate :patient, :programme_type, to: :vaccination_record
+  delegate :patient, :programme_type, :programme, to: :vaccination_record
 
   def academic_year = AcademicYear.current
 
-  def other_vaccination_records
-    patient.vaccination_records.where.not(id: vaccination_record.id)
+  def previous_vaccination_records
+    # We need to ensure there is a deterministic order of vaccination records on
+    # a child so that the :already_vaccinated_notification is sent once and only
+    # once per child. Using created_at alone is not sufficient as batching
+    # allows for multiple vaccination records to be created with the same
+    # created_at timestamp. We chose the ID as a tie-breaker since IDs are
+    # unique and sequential.
+    patient.vaccination_records.where(
+      "(created_at < :created_at) OR (created_at = :created_at AND id < :id)",
+      created_at: vaccination_record.created_at,
+      id: vaccination_record.id
+    )
   end
 
-  def would_still_be_vaccinated?
+  def was_already_vaccinated?
     # We're not using the existing `Patient::ProgrammeStatus` instance here
     # because we want to know if the patient would still be vaccinated if we
     # took away the vaccination record in question, to know whether to send
@@ -79,11 +90,15 @@ def would_still_be_vaccinated?
       programme_type:,
       academic_year:,
       patient:,
-      vaccination_records: other_vaccination_records,
+      vaccination_records: previous_vaccination_records,
       patient_locations: [],
       consents: [],
       triages: [],
       attendance_record: nil
     ).status == :vaccinated
   end
+
+  def is_still_eligible_for_vaccination?
+    !patient.programme_status(programme, academic_year:).vaccinated?
+  end
 end

app/lib/notification_parent_selector.rb

@@ -3,21 +3,14 @@
 class NotificationParentSelector
   def initialize(vaccination_record:, consents: nil)
     @vaccination_record = vaccination_record
-
     @consents =
-      if consents.present?
-        consents
+      if @vaccination_record.patient.send_notifications?(
+           team: @vaccination_record.team,
+           send_to_archived: true
+         ) && @vaccination_record.notify_parents
+        consents || @vaccination_record.patient.consents
       else
-        patient = @vaccination_record.patient
-
-        if patient.send_notifications?(
-             team: @vaccination_record.team,
-             send_to_archived: true
-           ) && @vaccination_record.notify_parents
-          patient.consents
-        else
-          []
-        end
+        []
       end
   end
 
@@ -47,6 +40,10 @@ def parents = parents_with_consent.map(&:first)
 
   def latest_consents
     @latest_consents ||=
-      ConsentGrouper.call(consents, programme_type:, academic_year:)
+      ConsentGrouper.call(
+        consents,
+        programme_type:,
+        academic_year: AcademicYear.current
+      )
   end
 end

app/models/immunisation_import.rb

@@ -129,10 +129,6 @@ def bulk_import(rows: 100)
       on_duplicate_key_update: :all
     )
 
-    vaccination_records.each do |vaccination_record|
-      AlreadyHadNotificationSender.call(vaccination_record:)
-    end
-
     PatientLocation.import(patient_locations, on_duplicate_key_ignore: :all)
 
     ArchiveReason.import(archive_reasons, on_duplicate_key_ignore: :all)
@@ -175,6 +171,12 @@ def postprocess_rows!
 
     PatientTeamUpdater.call(patient_scope: patients)
     PatientStatusUpdater.call(patient_scope: patients)
+
+    vaccination_records
+      .includes(:patient, :team)
+      .find_each do |vaccination_record|
+        AlreadyHadNotificationSender.call(vaccination_record:)
+      end
   end
 
   def post_commit!