Ensure only one school move exists per patient

thomasleese · thomasleese · commit 1db22136f21b · 2025-08-26T12:54:58.000+01:00
This updates the indexes on the school move table to ensure that only
one school move can exist per patient, and makes it so that when you
import a new school move it replaces any that already exist. This is to
resolve the current confusing behaviour where accepting a school move
deletes any others, so users need to know which order to accept school
moves in.

Jira-Issue: MAV-1806
diff --git a/app/jobs/commit_patient_changesets_job.rb b/app/jobs/commit_patient_changesets_job.rb
@@ -92,9 +92,9 @@ def import_school_moves(changesets, import)
       school_move.confirm! if school_move.patient.persisted?
     end
 
-    SchoolMove.import(
+    SchoolMove.import!(
       importable_school_moves.to_a,
-      on_duplicate_key_ignore: :all
+      on_duplicate_key_update: :all
     )
   end
 
diff --git a/app/models/class_import.rb b/app/models/class_import.rb
@@ -77,7 +77,7 @@ def postprocess_rows!
         )
       end
 
-    SchoolMove.import(school_moves, on_duplicate_key_ignore: true)
+    SchoolMove.import!(school_moves, on_duplicate_key_ignore: true)
 
     PatientsAgedOutOfSchoolJob.perform_later
   end
diff --git a/app/models/consent_form.rb b/app/models/consent_form.rb
@@ -443,13 +443,8 @@ def match_with_patient!(patient, current_user:)
         patient.school != school || patient.home_educated != home_educated
 
       if school_changed && !patient.deceased? && !patient.invalidated?
-        school_move =
-          if school
-            SchoolMove.find_or_initialize_by(patient:, school:)
-          else
-            SchoolMove.find_or_initialize_by(patient:, home_educated:, team:)
-          end
-
+        school_move = SchoolMove.find_or_initialize_by(patient:)
+        school_move.assign_from(school:, home_educated:, team:)
         school_move.update!(academic_year:, source: :parental_consent_form)
       end
 
diff --git a/app/models/patient_changeset.rb b/app/models/patient_changeset.rb
@@ -175,17 +175,13 @@ def school_move
              patient.home_educated != home_educated ||
              patient.not_in_team?(team:, academic_year:) ||
              patient.archived?(team:)
-          school_move =
-            if school
-              SchoolMove.find_or_initialize_by(patient:, school:)
-            else
-              SchoolMove.find_or_initialize_by(patient:, home_educated:, team:)
-            end
-
-          school_move.tap do
-            it.academic_year = academic_year
-            it.source = school_move_source
-          end
+          school_move = SchoolMove.find_or_initialize_by(patient:)
+          school_move.assign_from(school:, home_educated:, team:)
+          school_move.assign_attributes(
+            academic_year:,
+            source: school_move_source
+          )
+          school_move
         end
       end
   end
diff --git a/app/models/patient_import.rb b/app/models/patient_import.rb
@@ -130,7 +130,10 @@ def bulk_import(rows: 100)
     end
     @school_moves_to_confirm.clear
 
-    SchoolMove.import(@school_moves_to_save.to_a, on_duplicate_key_ignore: :all)
+    SchoolMove.import!(
+      @school_moves_to_save.to_a,
+      on_duplicate_key_update: :all
+    )
     @school_moves_to_save.clear
   end
 end
diff --git a/app/models/patient_import_row.rb b/app/models/patient_import_row.rb
@@ -44,17 +44,10 @@ def to_school_move(patient)
     if patient.new_record? || patient.school != school ||
          patient.home_educated != home_educated ||
          patient.not_in_team?(team:, academic_year:) || patient.archived?(team:)
-      school_move =
-        if school
-          SchoolMove.find_or_initialize_by(patient:, school:)
-        else
-          SchoolMove.find_or_initialize_by(patient:, home_educated:, team:)
-        end
-
-      school_move.tap do
-        it.academic_year = academic_year
-        it.source = school_move_source
-      end
+      school_move = SchoolMove.find_or_initialize_by(patient:)
+      school_move.assign_from(school:, home_educated:, team:)
+      school_move.assign_attributes(academic_year:, source: school_move_source)
+      school_move
     end
   end
 
diff --git a/app/models/school_move.rb b/app/models/school_move.rb
@@ -16,10 +16,9 @@
 #
 # Indexes
 #
-#  index_school_moves_on_patient_id_and_home_educated_and_team_id  (patient_id,home_educated,team_id) UNIQUE
-#  index_school_moves_on_patient_id_and_school_id                  (patient_id,school_id) UNIQUE
-#  index_school_moves_on_school_id                                 (school_id)
-#  index_school_moves_on_team_id                                   (team_id)
+#  index_school_moves_on_patient_id  (patient_id) UNIQUE
+#  index_school_moves_on_school_id   (school_id)
+#  index_school_moves_on_team_id     (team_id)
 #
 # Foreign Keys
 #
@@ -49,13 +48,21 @@ class SchoolMove < ApplicationRecord
               unless: -> { school.nil? }
             }
 
+  def assign_from(school:, home_educated:, team:)
+    if school
+      assign_attributes(school:, home_educated: nil, team: nil)
+    else
+      assign_attributes(school: nil, home_educated:, team:)
+    end
+  end
+
   def confirm!(user: nil)
     ActiveRecord::Base.transaction do
       update_patient!
       update_archive_reasons!(user:)
       update_sessions!
       create_log_entry!(user:)
-      SchoolMove.where(patient:).destroy_all if persisted?
+      destroy! if persisted?
     end
   end
 
diff --git a/db/migrate/20250820163919_make_school_moves_unique_on_patient.rb b/db/migrate/20250820163919_make_school_moves_unique_on_patient.rb
@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+class MakeSchoolMovesUniqueOnPatient < ActiveRecord::Migration[8.0]
+  def change
+    change_table :school_moves, bulk: true do |t|
+      t.remove_index %i[patient_id home_educated team_id], unique: true
+      t.remove_index %i[patient_id school_id], unique: true
+    end
+
+    reversible do |dir|
+      dir.up do
+        patient_ids_with_multiple_school_moves =
+          SchoolMove
+            .group(:patient_id)
+            .having("COUNT(*) > 1")
+            .pluck(:patient_id)
+
+        patient_ids_with_multiple_school_moves.each do |patient_id|
+          newest_school_move_id =
+            SchoolMove.where(patient_id:).order(:created_at).last.id
+          SchoolMove
+            .where(patient_id:)
+            .where.not(id: newest_school_move_id)
+            .delete_all
+        end
+      end
+    end
+
+    add_index :school_moves, :patient_id, unique: true
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
@@ -771,8 +771,7 @@
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
     t.integer "academic_year", null: false
-    t.index ["patient_id", "home_educated", "team_id"], name: "index_school_moves_on_patient_id_and_home_educated_and_team_id", unique: true
-    t.index ["patient_id", "school_id"], name: "index_school_moves_on_patient_id_and_school_id", unique: true
+    t.index ["patient_id"], name: "index_school_moves_on_patient_id", unique: true
     t.index ["school_id"], name: "index_school_moves_on_school_id"
     t.index ["team_id"], name: "index_school_moves_on_team_id"
   end
diff --git a/spec/factories/school_moves.rb b/spec/factories/school_moves.rb
@@ -16,10 +16,9 @@
 #
 # Indexes
 #
-#  index_school_moves_on_patient_id_and_home_educated_and_team_id  (patient_id,home_educated,team_id) UNIQUE
-#  index_school_moves_on_patient_id_and_school_id                  (patient_id,school_id) UNIQUE
-#  index_school_moves_on_school_id                                 (school_id)
-#  index_school_moves_on_team_id                                   (team_id)
+#  index_school_moves_on_patient_id  (patient_id) UNIQUE
+#  index_school_moves_on_school_id   (school_id)
+#  index_school_moves_on_team_id     (team_id)
 #
 # Foreign Keys
 #
diff --git a/spec/models/cohort_import_spec.rb b/spec/models/cohort_import_spec.rb
@@ -381,6 +381,41 @@
       end
     end
 
+    context "with an existing patient with a school move" do
+      let!(:existing_patient) do
+        create(
+          :patient,
+          given_name: "Jimmy",
+          family_name: "Smith",
+          date_of_birth: Date.new(2010, 1, 2),
+          nhs_number: nil,
+          team:,
+          school: create(:school)
+        )
+      end
+      let!(:school_move) do
+        create(:school_move, :to_school, patient: existing_patient)
+      end
+
+      before do
+        team.sessions.each do |session|
+          create(:patient_session, session:, patient: existing_patient)
+        end
+      end
+
+      it "doesn't create an additional patient" do
+        expect { process! }.to change(Patient, :count).by(2)
+      end
+
+      it "replaces the existing school move" do
+        expect { process! }.not_to(
+          change { existing_patient.reload.school_moves.count }
+        )
+
+        expect(school_move.reload.school).to eq(location)
+      end
+    end
+
     context "with an unscheduled session" do
       let(:session) do
         create(:session, :unscheduled, team:, programmes:, location:)
diff --git a/spec/models/school_move_spec.rb b/spec/models/school_move_spec.rb
@@ -16,10 +16,9 @@
 #
 # Indexes
 #
-#  index_school_moves_on_patient_id_and_home_educated_and_team_id  (patient_id,home_educated,team_id) UNIQUE
-#  index_school_moves_on_patient_id_and_school_id                  (patient_id,school_id) UNIQUE
-#  index_school_moves_on_school_id                                 (school_id)
-#  index_school_moves_on_team_id                                   (team_id)
+#  index_school_moves_on_patient_id  (patient_id) UNIQUE
+#  index_school_moves_on_school_id   (school_id)
+#  index_school_moves_on_team_id     (team_id)
 #
 # Foreign Keys
 #
@@ -155,17 +154,12 @@
     end
 
     shared_examples "destroys the school move" do
-      it "destroys the school move and any others" do
-        other_school_move = create(:school_move, :to_school, patient:)
-
+      it "destroys the school move" do
         expect(school_move).to be_persisted
-        expect { confirm! }.to change(described_class, :count).by(-2)
+        expect { confirm! }.to change(described_class, :count).by(-1)
         expect { school_move.reload }.to raise_error(
           ActiveRecord::RecordNotFound
         )
-        expect { other_school_move.reload }.to raise_error(
-          ActiveRecord::RecordNotFound
-        )
       end
     end
 

Original file line number	Diff line number	Diff line change
`@@ -77,7 +77,7 @@ def postprocess_rows!`
`77`	`77`	`)`
`78`	`78`	`end`
`79`	`79`
`80`		`- SchoolMove.import(school_moves, on_duplicate_key_ignore: true)`
	`80`	`+ SchoolMove.import!(school_moves, on_duplicate_key_ignore: true)`
`81`	`81`
`82`	`82`	`PatientsAgedOutOfSchoolJob.perform_later`
`83`	`83`	`end`
Original file line number	Diff line number	Diff line change
`@@ -16,10 +16,9 @@`
`16`	`16`	`#`
`17`	`17`	`# Indexes`
`18`	`18`	`#`
`19`		`-# index_school_moves_on_patient_id_and_home_educated_and_team_id (patient_id,home_educated,team_id) UNIQUE`
`20`		`-# index_school_moves_on_patient_id_and_school_id (patient_id,school_id) UNIQUE`
`21`		`-# index_school_moves_on_school_id (school_id)`
`22`		`-# index_school_moves_on_team_id (team_id)`
	`19`	`+# index_school_moves_on_patient_id (patient_id) UNIQUE`
	`20`	`+# index_school_moves_on_school_id (school_id)`
	`21`	`+# index_school_moves_on_team_id (team_id)`
`23`	`22`	`#`
`24`	`23`	`# Foreign Keys`
`25`	`24`	`#`