From 9e7f921ac0906834961b18dcff9b87b01ea22e9c Mon Sep 17 00:00:00 2001 From: Atharva Raykar <24277692+tfidfwastaken@users.noreply.github.com> Date: Tue, 17 Oct 2023 12:04:49 +0530 Subject: [PATCH] import api: fix duplication of nested patient resources (#5299) Sending the same patient resource more than once (either on accident or for an attribute update) leads to the addresses, phone numbers and business identifiers being duplicated, because we are generating a new ID for them every time. We now ensure that these IDs are generated deterministically so that the resource merge merely updates these values rather than creating new instances. --- .../bulk_api_import/fhir_importable.rb | 4 +- .../bulk_api_import/fhir_patient_importer.rb | 14 ++++--- .../fhir_patient_importer_spec.rb | 38 ++++++++++++++++--- 3 files changed, 44 insertions(+), 12 deletions(-) diff --git a/app/services/bulk_api_import/fhir_importable.rb b/app/services/bulk_api_import/fhir_importable.rb index 44fce98657..0a79281912 100644 --- a/app/services/bulk_api_import/fhir_importable.rb +++ b/app/services/bulk_api_import/fhir_importable.rb @@ -14,8 +14,8 @@ def timestamps } end - def translate_id(id) - Digest::UUID.uuid_v5(Digest::UUID::DNS_NAMESPACE, id) + def translate_id(id, ns_prefix: "") + Digest::UUID.uuid_v5(Digest::UUID::DNS_NAMESPACE + ns_prefix, id) end def translate_facility_id(id) diff --git a/app/services/bulk_api_import/fhir_patient_importer.rb b/app/services/bulk_api_import/fhir_patient_importer.rb index f496cea025..ba91cf05d2 100644 --- a/app/services/bulk_api_import/fhir_patient_importer.rb +++ b/app/services/bulk_api_import/fhir_patient_importer.rb @@ -21,9 +21,13 @@ def request_metadata request_facility_id: @resource.dig(:managingOrganization, 0, :value)} end + def identifier + @resource.dig(:identifier, 0, :value) + end + def build_attributes { - id: translate_id(@resource.dig(:identifier, 0, :value)), + id: translate_id(identifier), full_name: @resource.dig(:name, :value) || "Anonymous " + Faker::Name.first_name, gender: gender, status: status, @@ -68,7 +72,7 @@ def status def phone_numbers @resource[:telecom]&.map do |telecom| { - id: SecureRandom.uuid, + id: translate_id(identifier, ns_prefix: "patient_phone_number"), number: telecom[:value], phone_type: telecom[:use] == "mobile" || telecom[:use] == "old" ? "mobile" : "landline", active: !(telecom[:use] == "old"), @@ -80,7 +84,7 @@ def phone_numbers def address if (address = @resource.dig(:address, 0)) { - id: SecureRandom.uuid, + id: translate_id(identifier, ns_prefix: "patient_address"), street_address: address[:line]&.join("\n"), district: address[:district], state: address[:state], @@ -97,8 +101,8 @@ def address def business_identifiers [ { - id: SecureRandom.uuid, - identifier: @resource.dig(:identifier, 0, :value), + id: translate_id(identifier, ns_prefix: "patient_business_identifier"), + identifier: identifier, identifier_type: :external_import_id, **timestamps } diff --git a/spec/services/bulk_api_import/fhir_patient_importer_spec.rb b/spec/services/bulk_api_import/fhir_patient_importer_spec.rb index 12eeff20a0..06f80a9a2d 100644 --- a/spec/services/bulk_api_import/fhir_patient_importer_spec.rb +++ b/spec/services/bulk_api_import/fhir_patient_importer_spec.rb @@ -17,6 +17,33 @@ .except(:registrationOrganization) ).import }.to change(Patient, :count).by(1) + .and change(PatientBusinessIdentifier, :count).by(1) + end + + context "when a patient is imported twice" do + it "does not duplicate the patient or its nested resources" do + patient_resource = build_patient_import_resource.merge(address: [{line: ["abc"]}]) + + expect { + described_class.new( + patient_resource + .merge(managingOrganization: [{value: facility_identifier.identifier}]) + .except(:registrationOrganization) + ).import + }.to change(Patient, :count).by(1) + .and change(PatientBusinessIdentifier, :count).by(1) + .and change(Address, :count).by(1) + + expect { + described_class.new( + patient_resource + .merge(managingOrganization: [{value: facility_identifier.identifier}]) + .except(:registrationOrganization) + ).import + }.to change(Patient, :count).by(0) + .and change(PatientBusinessIdentifier, :count).by(0) + .and change(Address, :count).by(0) + end end end @@ -51,11 +78,11 @@ describe "#phone_numbers" do it "correctly populates phone_type and active attributes" do [ - {input: {telecom: [{use: "mobile"}]}, expected_phone_type: "mobile", expected_active: true}, - {input: {telecom: [{use: "home"}]}, expected_phone_type: "landline", expected_active: true}, - {input: {telecom: [{use: "work"}]}, expected_phone_type: "landline", expected_active: true}, - {input: {telecom: [{use: "temp"}]}, expected_phone_type: "landline", expected_active: true}, - {input: {telecom: [{use: "old"}]}, expected_phone_type: "mobile", expected_active: false} + {input: {identifier: [value: "foo"], telecom: [{use: "mobile"}]}, expected_phone_type: "mobile", expected_active: true}, + {input: {identifier: [value: "foo"], telecom: [{use: "home"}]}, expected_phone_type: "landline", expected_active: true}, + {input: {identifier: [value: "foo"], telecom: [{use: "work"}]}, expected_phone_type: "landline", expected_active: true}, + {input: {identifier: [value: "foo"], telecom: [{use: "temp"}]}, expected_phone_type: "landline", expected_active: true}, + {input: {identifier: [value: "foo"], telecom: [{use: "old"}]}, expected_phone_type: "mobile", expected_active: false} ].each do |input:, expected_phone_type:, expected_active:| expect(described_class.new(input).phone_numbers[0]) .to include(phone_type: expected_phone_type, active: expected_active) @@ -66,6 +93,7 @@ describe "#address" do specify do expect(described_class.new({ + identifier: [value: "foo"], address: [{line: %w[a b], district: "xyz", state: "foo",