From e2965201519eebcefb93bd3520bea477fe164bc2 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Mon, 9 Sep 2024 11:04:00 +0100 Subject: [PATCH] Link imports with created records This updates the cohort and immunisation imports to create links between the import record (`CohortImport` and `ImmunisationImport`) with the objects that are getting imported. This doesn't necessarily mean that the import was the one that first created the object because of duplicates, but it does tell us whether the record was part of that import in some way. This slightly changes the meaning of this relationship, but it introduces a number of improvements: - We can simplify the patient and vaccination record duplicate controller. - It allows us to have a more complete picture of the import process, users can see if the same record was part of multiple imports, and trace how the object might have changed over time. - We can still determine if an import was responsible for creating an object by comparing the `processed_at` and `created_at` times, or the `recorded_at` times, albeit without a direct relationship. --- .../patients_controller.rb | 12 +---- .../vaccination_records_controller.rb | 4 +- app/models/cohort_import.rb | 9 ++-- app/models/immunisation_import.rb | 54 +++++++++++-------- app/models/immunisation_import_row.rb | 41 ++++++-------- app/views/immunisation_imports/show.html.erb | 4 +- app/views/vaccination_records/show.html.erb | 2 +- spec/models/immunisation_import_row_spec.rb | 10 +--- spec/models/immunisation_import_spec.rb | 32 ++++++----- 9 files changed, 78 insertions(+), 90 deletions(-) diff --git a/app/controllers/immunisation_imports/patients_controller.rb b/app/controllers/immunisation_imports/patients_controller.rb index c16e7c094..228436c85 100644 --- a/app/controllers/immunisation_imports/patients_controller.rb +++ b/app/controllers/immunisation_imports/patients_controller.rb @@ -28,16 +28,6 @@ def set_immunisation_import end def set_patient - @patient = - Patient - .joins(patient_sessions: :vaccination_records) - .where( - patient_sessions: { - vaccination_records: { - imported_from: @immunisation_import - } - } - ) - .find(params[:id]) + @patient = @immunisation_import.patients.find(params[:id]) end end diff --git a/app/controllers/vaccination_records_controller.rb b/app/controllers/vaccination_records_controller.rb index 01dcdf88b..5be375422 100644 --- a/app/controllers/vaccination_records_controller.rb +++ b/app/controllers/vaccination_records_controller.rb @@ -39,9 +39,9 @@ def vaccination_records policy_scope(VaccinationRecord) .includes( :batch, - :programme, - :imported_from, + :immunisation_imports, :performed_by_user, + :programme, :vaccine, patient: :school, session: :location diff --git a/app/models/cohort_import.rb b/app/models/cohort_import.rb index e69a8de06..bb51ab03f 100644 --- a/app/models/cohort_import.rb +++ b/app/models/cohort_import.rb @@ -65,9 +65,12 @@ def parse_row(row_data) def process_row(row) location = Location.find_by(urn: row.school_urn) - location.patients.create!( - row.to_patient.merge(parent: Parent.new(row.to_parent)) - ) + patient = + location.patients.create!( + row.to_patient.merge(parent: Parent.new(row.to_parent)) + ) + + patient.cohort_imports << self unless patient.cohort_imports.exists?(id) :new_record_count end diff --git a/app/models/immunisation_import.rb b/app/models/immunisation_import.rb index 50ef398cf..e6df15a33 100644 --- a/app/models/immunisation_import.rb +++ b/app/models/immunisation_import.rb @@ -69,38 +69,48 @@ def count_columns end def parse_row(row_data) - ImmunisationImportRow.new( - data: row_data, - programme:, - user: uploaded_by, - imported_from: self - ) + ImmunisationImportRow.new(data: row_data, programme:, user: uploaded_by) end def process_row(row) if (vaccination_record = row.to_vaccination_record) - if vaccination_record.new_record? - vaccination_record.save! - :new_record_count - else - :exact_duplicate_record_count - end + count_column_to_increment = + ( + if vaccination_record.new_record? || vaccination_record.draft? + :new_record_count + else + :exact_duplicate_record_count + end + ) + + vaccination_record.save! + + link_records( + vaccination_record, + vaccination_record.batch, + vaccination_record.location, + vaccination_record.patient, + vaccination_record.patient_session, + vaccination_record.session + ) + + count_column_to_increment else :not_administered_record_count end end - def record_rows - vaccination_records.draft.each do |vaccination_record| - if (patient_session = vaccination_record.patient_session).draft? - patient_session.update!(active: true) + def link_records(*records) + records.each do |record| + unless record.immunisation_imports.exists?(id) + record.immunisation_imports << self end - - if (session = vaccination_record.session).draft? - session.update!(active: true) - end - - vaccination_record.update!(recorded_at: Time.zone.now) end end + + def record_rows + patient_sessions.draft.update_all(active: true) + sessions.draft.update_all(active: true) + vaccination_records.draft.update_all(recorded_at: Time.zone.now) + end end diff --git a/app/models/immunisation_import_row.rb b/app/models/immunisation_import_row.rb index e9227ff36..e8dec08ab 100644 --- a/app/models/immunisation_import_row.rb +++ b/app/models/immunisation_import_row.rb @@ -64,11 +64,10 @@ class ImmunisationImportRow presence: true, if: :requires_performed_by? - def initialize(data:, programme:, user:, imported_from:) + def initialize(data:, programme:, user:) @data = data @programme = programme @user = user - @imported_from = imported_from end def to_vaccination_record @@ -76,21 +75,20 @@ def to_vaccination_record return unless administered - VaccinationRecord - .recorded - .or(VaccinationRecord.where(imported_from:)) - .create_with(imported_from:, notes:, recorded_at: nil) - .find_or_initialize_by( - administered_at:, - batch:, - delivery_method:, - delivery_site:, - dose_sequence:, - patient_session:, - performed_by_family_name:, - performed_by_given_name:, - vaccine: - ) + VaccinationRecord.create_with( + notes:, + recorded_at: nil + ).find_or_initialize_by( + administered_at:, + batch:, + delivery_method:, + delivery_site:, + dose_sequence:, + patient_session:, + performed_by_family_name:, + performed_by_given_name:, + vaccine: + ) end def patient @@ -111,9 +109,7 @@ def session @session ||= @programme .sessions - .active - .or(Session.where(imported_from:)) - .create_with(imported_from:, active: false) + .create_with(active: false) .find_or_create_by!( date: session_date, location:, @@ -267,8 +263,6 @@ def performed_by_family_name private - attr_reader :imported_from - def administered_at administered ? (session_date.in_time_zone + 12.hours) : nil end @@ -392,7 +386,6 @@ def patient_attributes first_name: patient_first_name, gender_code: patient_gender_code, home_educated:, - imported_from:, last_name: patient_last_name, nhs_number: patient_nhs_number, school: @@ -400,7 +393,7 @@ def patient_attributes end def staged_patient_attributes - patient_attributes.except(:imported_from, :school).merge( + patient_attributes.except(:school).merge( school_id: patient_attributes[:school]&.id ) end diff --git a/app/views/immunisation_imports/show.html.erb b/app/views/immunisation_imports/show.html.erb index 7b8501fd0..602497383 100644 --- a/app/views/immunisation_imports/show.html.erb +++ b/app/views/immunisation_imports/show.html.erb @@ -26,4 +26,6 @@ <% end %> <% end %> -<%= render AppVaccinationRecordTableComponent.new(@vaccination_records) %> +<%= render AppVaccinationRecordTableComponent.new( + @immunisation_import.recorded? ? @vaccination_records.recorded : @vaccination_records.draft + ) %> diff --git a/app/views/vaccination_records/show.html.erb b/app/views/vaccination_records/show.html.erb index dd207096d..18ddaeadd 100644 --- a/app/views/vaccination_records/show.html.erb +++ b/app/views/vaccination_records/show.html.erb @@ -5,7 +5,7 @@ { text: @programme.name, href: programme_vaccination_records_path(@programme) }, { text: t("vaccination_records.index.title"), href: programme_vaccination_records_path(@programme) }, ]) %> - <% elsif (immunisation_import = @vaccination_record.imported_from) %> + <% elsif (immunisation_import = @vaccination_record.immunisation_imports.sort_by(&:created_at).last) %> <%= render AppBacklinkComponent.new( href: edit_programme_immunisation_import_path( @programme, diff --git a/spec/models/immunisation_import_row_spec.rb b/spec/models/immunisation_import_row_spec.rb index 2d0a46b8e..7a48a6305 100644 --- a/spec/models/immunisation_import_row_spec.rb +++ b/spec/models/immunisation_import_row_spec.rb @@ -2,20 +2,12 @@ describe ImmunisationImportRow, type: :model do subject(:immunisation_import_row) do - described_class.new( - data:, - programme:, - user: uploaded_by, - imported_from: immunisation_import - ) + described_class.new(data:, programme:, user: uploaded_by) end let(:programme) { create(:programme, :flu, academic_year: 2023) } let(:team) { create(:team, ods_code: "abc") } let(:uploaded_by) { create(:user, teams: [team]) } - let(:immunisation_import) do - create(:immunisation_import, programme:, uploaded_by:) - end let(:nhs_number) { "1234567890" } let(:first_name) { "Harry" } diff --git a/spec/models/immunisation_import_spec.rb b/spec/models/immunisation_import_spec.rb index d4d8fdea6..cb5e75006 100644 --- a/spec/models/immunisation_import_spec.rb +++ b/spec/models/immunisation_import_spec.rb @@ -125,11 +125,11 @@ expect { process! } .to change(immunisation_import, :processed_at).from(nil) .and change(immunisation_import.vaccination_records, :count).by(7) - .and not_change(immunisation_import.locations, :count) + .and change(immunisation_import.locations, :count).by(1) .and change(immunisation_import.patients, :count).by(7) .and change(immunisation_import.sessions, :count).by(1) - .and change(PatientSession, :count).by(7) - .and change(Batch, :count).by(4) + .and change(immunisation_import.patient_sessions, :count).by(7) + .and change(immunisation_import.batches, :count).by(4) # Second import should not duplicate the vaccination records if they're # identical. @@ -137,10 +137,10 @@ # stree-ignore expect { immunisation_import.process! } .to not_change(immunisation_import, :processed_at) - .and not_change(immunisation_import.vaccination_records, :count) - .and not_change(immunisation_import.locations, :count) - .and not_change(immunisation_import.patients, :count) - .and not_change(immunisation_import.sessions, :count) + .and not_change(VaccinationRecord, :count) + .and not_change(Location, :count) + .and not_change(Patient, :count) + .and not_change(Session, :count) .and not_change(PatientSession, :count) .and not_change(Batch, :count) end @@ -171,11 +171,11 @@ expect { process! } .to change(immunisation_import, :processed_at).from(nil) .and change(immunisation_import.vaccination_records, :count).by(7) - .and not_change(immunisation_import.locations, :count) + .and change(immunisation_import.locations, :count).by(1) .and change(immunisation_import.patients, :count).by(7) .and change(immunisation_import.sessions, :count).by(1) - .and change(PatientSession, :count).by(7) - .and change(Batch, :count).by(5) + .and change(immunisation_import.patient_sessions, :count).by(7) + .and change(immunisation_import.batches, :count).by(5) # Second import should not duplicate the vaccination records if they're # identical. @@ -183,10 +183,10 @@ # stree-ignore expect { immunisation_import.process! } .to not_change(immunisation_import, :processed_at) - .and not_change(immunisation_import.vaccination_records, :count) - .and not_change(immunisation_import.locations, :count) - .and not_change(immunisation_import.patients, :count) - .and not_change(immunisation_import.sessions, :count) + .and not_change(VaccinationRecord, :count) + .and not_change(Location, :count) + .and not_change(Patient, :count) + .and not_change(Session, :count) .and not_change(PatientSession, :count) .and not_change(Batch, :count) end @@ -233,9 +233,7 @@ end it "doesn't create an additional patient" do - expect { process! }.to change(immunisation_import.patients, :count).by( - 6 - ) + expect { process! }.to change(Patient, :count).by(6) end it "doesn't update the NHS number on the existing patient" do