Merge pull request #4291 from nhsuk/simplify-pds-update-during-import

Simplify PDS update during import

Author: thomasleese

app/jobs/process_patient_changesets_job.rb

@@ -8,38 +8,9 @@ class ProcessPatientChangesetsJob < ApplicationJob
   def perform(patient_changeset)
     attrs = patient_changeset.child_attributes
 
-    pds_patient =
-      if attrs["nhs_number"].present?
-        patient_found = find_patient(attrs["nhs_number"])
-
-        if patient_found == :invalid
-          search_for_patient(attrs).then do |newly_found_patient|
-            # If we found a patient, update the changeset with the new NHS
-            # number. If we couldn't determine who this patient should really
-            # be, we won't have an NHS number to replace the invalid one, so
-            # just mark it as invalid.
-            patient_changeset.invalidate! if newly_found_patient.nil?
-            newly_found_patient
-          end
-        elsif patient_found == :not_found
-          patient_changeset.update!(nhs_number: nil)
-          search_for_patient(attrs)
-        else
-          patient_found
-        end
-      else
-        search_for_patient(attrs)
-      end
-
-    if pds_patient.present?
-      patient_changeset.pending_changes.tap do |changes|
-        changes["pds"] = {
-          nhs_number: pds_patient.nhs_number,
-          restricted: pds_patient.restricted,
-          gp_ods_code: pds_patient.gp_ods_code,
-          date_of_death: pds_patient.date_of_death
-        }
-      end
+    if attrs["nhs_number"].blank? &&
+         (pds_patient = search_for_patient(attrs)).present?
+      attrs["nhs_number"] = pds_patient.nhs_number
     end
 
     patient_changeset.processed!
@@ -57,14 +28,6 @@ def perform(patient_changeset)
 
   private
 
-  def find_patient(nhs_number)
-    PDS::Patient.find(nhs_number)
-  rescue NHS::PDS::InvalidatedResource, NHS::PDS::InvalidNHSNumber
-    :invalid
-  rescue NHS::PDS::PatientNotFound
-    :not_found
-  end
-
   def search_for_patient(attrs)
     PDS::Patient.search(
       family_name: attrs["family_name"],

app/models/concerns/csv_importable.rb

@@ -126,6 +126,22 @@ def process!
     parse_rows! if rows.nil?
     return if invalid?
 
+    if is_a?(PatientImport) && Flipper.enabled?(:pds_lookup_during_import)
+      changesets =
+        rows.each_with_index.map do |row, row_number|
+          PatientChangeset.from_import_row(row:, import: self, row_number:)
+        end
+
+      changesets.each do
+        if slow?
+          ProcessPatientChangesetsJob.perform_later(it)
+        else
+          ProcessPatientChangesetsJob.perform_now(it)
+        end
+      end
+      return
+    end
+
     counts = COUNT_COLUMNS.index_with(0)
 
     ActiveRecord::Base.transaction do

app/models/patient_changeset.rb

@@ -213,7 +213,7 @@ def auto_accept_child_attributes(existing_patient)
       existing_patient,
       %w[male female not_specified]
     )
-    set_child_attribute_if_valid(:invalidated_at, existing_patient)
+    set_child_attribute_if_valid(:nhs_number, existing_patient)
   end
 
   def set_child_attribute_if_valid(

spec/features/import_child_pds_lookup_extravaganza_spec.rb

@@ -1,21 +1,41 @@
 # frozen_string_literal: true
 
 describe "Import child records" do
-  scenario "PDS lookup extravaganza" do
-    skip "Feature is completely disabled temporarily"
+  let(:today) { Time.zone.local(2025, 9, 1, 12, 0, 0) }
 
+  around { |example| travel_to(today) { example.run } }
+
+  scenario "PDS lookup extravaganza with multiple patient scenarios" do
     given_i_am_signed_in
     and_an_hpv_programme_is_underway
     and_an_existing_patient_record_exists
     and_pds_lookup_during_import_is_enabled
 
     when_i_visit_the_import_page
-    and_i_upload_the_pds_extravaganza_import_file
+    and_i_upload_import_file("pds_extravaganza.csv")
     then_i_should_see_the_import_page
-    and_i_should_see_the_existing_patient_as_imported
-
-    when_i_click_on_the_invalidated_patient
-    then_i_should_see_invalidated_patient_record
+    and_i_should_see_one_new_patient_created
+
+    # Case 1: Patient with existing NHS number (Albert) - nothing should happen
+    and_i_see_the_patient_uploaded_with_nhs_number
+
+    # Case 2: Existing patient without NHS number (Betty) - should not show duplicate review
+    and_i_do_not_see_an_import_review_for_the_first_patient_uploaded_without_nhs_number
+    when_i_click_on_the_patient_without_review
+    then_i_see_the_new_patient_has_an_nhs_number
+
+    # Case 3: Existing patient with NHS number (Catherine) - should show duplicate review
+    when_i_go_back_to_the_import_page
+    then_i_see_an_import_review_for_the_second_patient_uploaded_without_nhs_number
+    when_i_click_review
+    then_i_see_both_records_have_an_nhs_number
+    when_i_use_duplicate_record_during_merge
+    then_the_existing_patient_has_an_nhs_number_in_mavis
+
+    # Case 4: New patient without NHS number (Charlie) - should be created with NHS number from PDS
+    when_i_go_back_to_the_import_page
+    when_i_click_on_new_patient_uploaded_without_an_nhs_number
+    then_i_see_the_new_patient_now_has_an_nhs_number
   end
 
   def given_i_am_signed_in
@@ -38,7 +58,7 @@ def and_an_hpv_programme_is_underway
   end
 
   def and_an_existing_patient_record_exists
-    @existing_patient =
+    @existing_patient_uploaded_with_nhs_number =
       create(
         :patient,
         given_name: "Albert",
@@ -50,40 +70,72 @@ def and_an_existing_patient_record_exists
         address_line_2: "",
         address_town: "London",
         address_postcode: "SW11 1EH",
-        school: nil, # Unknown school, should be silently updated
+        school: nil,
         session: @session
       )
-    @invalidated_patient =
+
+    # Betty - will match exactly except NHS number (no review needed)
+    @existing_patient_uploaded_without_nhs_number =
       create(
         :patient,
         given_name: "Betty",
         family_name: "Samson",
-        nhs_number: "9993524689",
+        nhs_number: nil,
         date_of_birth: Date.new(2010, 1, 1),
         gender_code: :female,
-        address_line_1: "10 Downing Street",
+        address_line_1: "123 High Street",
         address_line_2: "",
         address_town: "London",
-        address_postcode: "SW11 1AA",
-        school: nil, # Unknown school, should be silently updated
+        address_postcode: "SW1A 1AA",
+        school: nil,
+        session: @session
+      )
+
+    # Catherine - will have different address, causing review
+    @existing_patient_duplicate_review =
+      create(
+        :patient,
+        given_name: "Catherine",
+        family_name: "Williams",
+        nhs_number: "9876543210",
+        date_of_birth: Date.new(2010, 5, 15),
+        gender_code: :female,
+        address_line_1: "999 Old Street", # Different from CSV
+        address_line_2: "",
+        address_town: "Birmingham", # Different from CSV
+        address_postcode: "B1 1AA", # Different from CSV
+        school: nil,
         session: @session
       )
+
+    expect(Patient.count).to eq(3)
   end
 
   def and_pds_lookup_during_import_is_enabled
     Flipper.enable(:pds_lookup_during_import)
 
-    stub_pds_get_nhs_number_to_return_a_patient(@existing_patient.nhs_number)
+    stub_pds_search_to_return_a_patient(
+      "9449306168",
+      "family" => "Samson",
+      "given" => "Betty",
+      "birthdate" => "eq2010-01-01",
+      "address-postalcode" => "SW1A 1AA"
+    )
 
-    stub_pds_search_to_return_no_patients(
-      "family" => @invalidated_patient.family_name,
-      "given" => @invalidated_patient.given_name,
-      "birthdate" => "eq#{@invalidated_patient.date_of_birth}",
-      "address-postalcode" => @invalidated_patient.address_postcode
+    stub_pds_search_to_return_a_patient(
+      "9876543210",
+      "family" => "Williams",
+      "given" => "Catherine",
+      "birthdate" => "eq2009-05-15",
+      "address-postalcode" => "SW2 2BB"
     )
 
-    stub_pds_get_nhs_number_to_return_an_invalidated_patient(
-      @invalidated_patient.nhs_number
+    stub_pds_search_to_return_a_patient(
+      "4491459835",
+      "family" => "Brown",
+      "given" => "Charlie",
+      "birthdate" => "eq2011-03-15",
+      "address-postalcode" => "SW2 2BB"
     )
   end
 
@@ -92,35 +144,82 @@ def when_i_visit_the_import_page
     click_link "Import", match: :first
   end
 
-  def and_i_upload_the_pds_extravaganza_import_file
+  def when_i_go_back_to_the_import_page
+    visit "/imports"
+    click_link "1 September 2025 at 12:00pm"
+  end
+
+  def when_i_click_review
+    click_link "Review"
+  end
+
+  def and_i_upload_import_file(filename)
     click_button "Import records"
     choose "Child records"
     click_button "Continue"
-    attach_file(
-      "cohort_import[csv]",
-      "spec/fixtures/cohort_import/pds_extravaganza.csv"
-    )
+    attach_file("cohort_import[csv]", "spec/fixtures/cohort_import/#{filename}")
     click_on "Continue"
   end
 
+  def and_i_do_not_see_an_import_review_for_the_first_patient_uploaded_without_nhs_number
+    expect(page).not_to have_content("Actions Review SAMSON, Betty")
+  end
+
+  def when_i_click_on_the_patient_without_review
+    click_link "SAMSON, Betty"
+  end
+
+  def then_i_see_an_import_review_for_the_second_patient_uploaded_without_nhs_number
+    expect(page).to have_content("Actions Review WILLIAMS, Catherine")
+  end
+
+  def when_i_click_on_new_patient_uploaded_without_an_nhs_number
+    click_link "BROWN, Charlie"
+  end
+
+  def when_i_use_duplicate_record_during_merge
+    choose "Use duplicate record"
+    click_on "Resolve duplicate"
+  end
+
   def then_i_should_see_the_import_page
     expect(page).to have_content("Imported on")
     expect(page).to have_content("Imported byUSER, Test")
   end
 
-  def and_i_should_see_the_existing_patient_as_imported
-    expect(page).not_to have_content("Actions Review TWEEDLE, Albert")
+  def and_i_should_see_one_new_patient_created
+    perform_enqueued_jobs
+    expect(Patient.count).to eq(4)
   end
 
-  def when_i_click_on_the_invalidated_patient
-    click_link "SAMSON, Betty"
+  def and_i_see_the_patient_uploaded_with_nhs_number
+    expect(page).to have_content(
+      "Name and NHS number TWEEDLE, Albert 999 907 5320"
+    )
   end
 
-  def then_i_should_see_invalidated_patient_record
-    expect(page).to have_content("Record flagged as invalid")
+  def then_i_see_the_new_patient_has_an_nhs_number
+    expect(page).to have_content("944 930 6168")
     expect(page).to have_content("SAMSON, Betty")
-    expect(page).to have_content("NHS number999 352 4689")
-    expect(page).to have_content("Date of birth1 January 2010 (aged 15)")
-    expect(page).to have_content("SW11 1AA")
+    expect(page).to have_content("1 January 2010 (aged 15)")
+  end
+
+  def then_i_see_both_records_have_an_nhs_number
+    expect(page).to have_text("987 654 3210", count: 2)
+  end
+
+  def then_i_see_the_new_patient_now_has_an_nhs_number
+    expect(page).to have_content("NHS number449 145 9835")
+    expect(page).to have_content("Full nameBROWN, Charlie")
+    expect(page).to have_content("Date of birth15 March 2011 (aged 14)")
+  end
+
+  def then_the_existing_patient_has_an_nhs_number_in_mavis
+    expect(Patient.count).to eq(4)
+    patient = Patient.where(given_name: "Catherine").first
+    expect(patient.nhs_number).to eq("9876543210")
+    expect(patient.address_line_1).to eq("456 New Street")
+    expect(patient.address_town).to eq("London")
+    expect(patient.address_postcode).to eq("SW2 2BB")
   end
 end

spec/fixtures/cohort_import/pds_extravaganza.csv

@@ -1,3 +1,5 @@
 CHILD_SCHOOL_URN,PARENT_1_NAME,PARENT_1_RELATIONSHIP,PARENT_1_EMAIL,PARENT_1_PHONE,PARENT_2_NAME,PARENT_2_RELATIONSHIP,PARENT_2_EMAIL,PARENT_2_PHONE,CHILD_FIRST_NAME,CHILD_LAST_NAME,CHILD_PREFERRED_GIVEN_NAME,CHILD_DATE_OF_BIRTH,CHILD_YEAR_GROUP,CHILD_ADDRESS_LINE_1,CHILD_ADDRESS_LINE_2,CHILD_TOWN,CHILD_POSTCODE,CHILD_NHS_NUMBER
-123456,,,,,,,,,Albert,Tweedle,,2009-12-29,9,38A Battersaa Rise,,London,SW11 1EH,9999075320
-123456,,,,,,,,,Betty,Samson,Beth,2010-01-01,9,10 Downing Street,,London,SW11 1AA,9993524689
+123456,,,,,,,,,Albert,Tweedle,,2009-12-29,8,38A Battersea Rise,,London,SW11 1EH,9999075320
+123456,,,,,,,,,Betty,Samson,Beth,2010-01-01,8,123 High Street,,London,SW1A 1AA,
+123456,,,,,,,,,Catherine,Williams,,2009-05-15,8,456 New Street,,London,SW2 2BB,
+123456,,,,,,,,,Charlie,Brown,,2011-03-15,9,42 Wallaby Way,,London,SW2 2BB,

spec/support/pds_helper.rb

@@ -10,12 +10,25 @@ def stub_pds_search_to_return_no_patients(**query)
     ).with(query: hash_including(query)).to_return_json(body: { total: 0 })
   end
 
-  def stub_pds_search_to_return_a_patient
+  def stub_pds_search_to_return_a_patient(nhs_number = "9449306168", **query)
+    query["_history"] ||= "true"
+
+    response_data =
+      JSON.parse(file_fixture("pds/search-patients-response.json").read)
+
+    patient_resource = response_data["entry"][0]["resource"]
+
+    patient_resource["id"] = nhs_number
+    patient_resource["identifier"][0]["value"] = nhs_number
+    response_data["entry"][0][
+      "fullUrl"
+    ] = "https://int.api.service.nhs.uk/personal-demographics/FHIR/R4/Patient/#{nhs_number}"
+
     stub_request(
       :get,
       "https://sandbox.api.service.nhs.uk/personal-demographics/FHIR/R4/Patient"
-    ).with(query: hash_including({})).to_return(
-      body: file_fixture("pds/search-patients-response.json"),
+    ).with(query: hash_including(query)).to_return(
+      body: response_data.to_json,
       headers: {
         "Content-Type" => "application/fhir+json"
       }