From be2609ad7fd94e8ce037acfd890b5d0ec56406c0 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Wed, 20 Aug 2025 18:15:26 +0100 Subject: [PATCH] Ensure only one school move exists per patient 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 --- app/jobs/commit_patient_changesets_job.rb | 4 +-- app/models/class_import.rb | 2 +- app/models/consent_form.rb | 9 ++--- app/models/patient_changeset.rb | 18 ++++------ app/models/patient_import.rb | 5 ++- app/models/patient_import_row.rb | 15 +++----- app/models/school_move.rb | 17 ++++++--- ...919_make_school_moves_unique_on_patient.rb | 31 ++++++++++++++++ db/schema.rb | 3 +- spec/factories/school_moves.rb | 7 ++-- spec/models/cohort_import_spec.rb | 35 +++++++++++++++++++ spec/models/school_move_spec.rb | 16 +++------ 12 files changed, 107 insertions(+), 55 deletions(-) create mode 100644 db/migrate/20250820163919_make_school_moves_unique_on_patient.rb diff --git a/app/jobs/commit_patient_changesets_job.rb b/app/jobs/commit_patient_changesets_job.rb index 55100e886a..d7d7da32cf 100644 --- a/app/jobs/commit_patient_changesets_job.rb +++ b/app/jobs/commit_patient_changesets_job.rb @@ -93,9 +93,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 index d1e2f28992..9a2cadc313 100644 --- 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 index 459a36a7fa..16fe394a54 100644 --- 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 index 2c098a0062..2c70a07d49 100644 --- 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 index 8b0d216647..db68d4014e 100644 --- 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 index 876e6bf0e9..cd0dd2ea99 100644 --- a/app/models/patient_import_row.rb +++ b/app/models/patient_import_row.rb @@ -62,17 +62,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 index f31a303eac..e16187e993 100644 --- 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 new file mode 100644 index 0000000000..7865bb7248 --- /dev/null +++ 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 index 7673f8b546..6bdbf8bda5 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -772,8 +772,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 index 5cba74b3e2..6e6aeb03e8 100644 --- 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 index 9c7cbdf5b8..a5403ef090 100644 --- a/spec/models/cohort_import_spec.rb +++ b/spec/models/cohort_import_spec.rb @@ -385,6 +385,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 index 12cfaa6a09..6c9a929228 100644 --- 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