Skip to content

Commit

Permalink
import api: fix duplication of nested patient resources (#5299)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
tfidfwastaken authored Oct 17, 2023
1 parent dd31193 commit 9e7f921
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 12 deletions.
4 changes: 2 additions & 2 deletions app/services/bulk_api_import/fhir_importable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
14 changes: 9 additions & 5 deletions app/services/bulk_api_import/fhir_patient_importer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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"),
Expand All @@ -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],
Expand All @@ -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
}
Expand Down
38 changes: 33 additions & 5 deletions spec/services/bulk_api_import/fhir_patient_importer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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)
Expand All @@ -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",
Expand Down

0 comments on commit 9e7f921

Please sign in to comment.