From 9b938669cb23ec890020b73a258e5fe89dc8983a Mon Sep 17 00:00:00 2001
From: Atharva Raykar <24277692+tfidfwastaken@users.noreply.github.com>
Date: Fri, 15 Dec 2023 13:22:51 +0530
Subject: [PATCH] import user: separate users from multiple orgs (#5346)

The Import User was previously created by finding the first facility in
the database and assigning the user to that facility. This is a problem
in any instance that has more than one organization, since it will
affiliate the import user with whichever organization that first
facility belonged to.

We resolve this by requiring an organization ID when finding or creating
an Import User.

**Story card:** [sc-11563]

## Test instructions

Call the import API from two different organizations, and ensure that
there are two different import users with the correct organization
affiliations.
---
 .../fhir_appointment_importer.rb              |  5 ++--
 .../fhir_condition_importer.rb                |  5 ++--
 .../bulk_api_import/fhir_importable.rb        |  4 +--
 .../fhir_medication_request_importer.rb       |  5 ++--
 .../fhir_observation_importer.rb              |  9 +++---
 .../bulk_api_import/fhir_patient_importer.rb  |  5 ++--
 app/services/import_user.rb                   | 16 ++++++----
 app/services/patient_import/importer.rb       |  4 +--
 .../patient_import/spreadsheet_transformer.rb |  2 +-
 spec/jobs/bulk_api_import_job_spec.rb         |  4 +--
 .../fhir_appointment_importer_spec.rb         |  7 ++---
 .../fhir_condition_importer_spec.rb           |  6 ++--
 .../bulk_api_import/fhir_importable_spec.rb   |  9 +++---
 .../fhir_medication_request_importer_spec.rb  | 11 ++++---
 .../fhir_observation_importer_spec.rb         |  6 ++--
 .../fhir_patient_importer_spec.rb             |  7 ++---
 .../services/bulk_api_import/importer_spec.rb |  9 +++---
 .../patient_import/import_user_spec.rb        | 29 ++++++++++++-------
 .../spreadsheet_transformer_spec.rb           |  2 +-
 19 files changed, 79 insertions(+), 66 deletions(-)

diff --git a/app/services/bulk_api_import/fhir_appointment_importer.rb b/app/services/bulk_api_import/fhir_appointment_importer.rb
index 68c7aa4126..b37a7f4da4 100644
--- a/app/services/bulk_api_import/fhir_appointment_importer.rb
+++ b/app/services/bulk_api_import/fhir_appointment_importer.rb
@@ -6,6 +6,7 @@ class BulkApiImport::FhirAppointmentImporter
   def initialize(resource:, organization_id:)
     @resource = resource
     @organization_id = organization_id
+    @import_user = find_or_create_import_user(organization_id)
   end
 
   def import
@@ -16,12 +17,12 @@ def import
     appointment = Appointment.merge(transformed_params)
     appointment.update_patient_status
 
-    AuditLog.merge_log(import_user, appointment) if appointment.present?
+    AuditLog.merge_log(@import_user, appointment) if appointment.present?
     appointment
   end
 
   def request_metadata
-    {user_id: import_user.id}
+    {user_id: @import_user.id}
   end
 
   def build_attributes
diff --git a/app/services/bulk_api_import/fhir_condition_importer.rb b/app/services/bulk_api_import/fhir_condition_importer.rb
index 4cee828137..eaa37cf194 100644
--- a/app/services/bulk_api_import/fhir_condition_importer.rb
+++ b/app/services/bulk_api_import/fhir_condition_importer.rb
@@ -9,6 +9,7 @@ class BulkApiImport::FhirConditionImporter
   def initialize(resource:, organization_id:)
     @resource = resource
     @organization_id = organization_id
+    @import_user = find_or_create_import_user(organization_id)
   end
 
   def import
@@ -16,12 +17,12 @@ def import
       .then { Api::V3::MedicalHistoryTransformer.from_request(_1).merge(metadata) }
       .then { MedicalHistory.merge(_1) }
 
-    AuditLog.merge_log(import_user, merge_result) if merge_result.present?
+    AuditLog.merge_log(@import_user, merge_result) if merge_result.present?
     merge_result
   end
 
   def metadata
-    {user_id: import_user.id}
+    {user_id: @import_user.id}
   end
 
   def build_attributes
diff --git a/app/services/bulk_api_import/fhir_importable.rb b/app/services/bulk_api_import/fhir_importable.rb
index 269e88c8c2..b97085e521 100644
--- a/app/services/bulk_api_import/fhir_importable.rb
+++ b/app/services/bulk_api_import/fhir_importable.rb
@@ -1,6 +1,6 @@
 module BulkApiImport::FhirImportable
-  def import_user
-    ImportUser.find_or_create
+  def find_or_create_import_user(org_id)
+    ImportUser.find_or_create(org_id: org_id)
   end
 
   def import
diff --git a/app/services/bulk_api_import/fhir_medication_request_importer.rb b/app/services/bulk_api_import/fhir_medication_request_importer.rb
index d558dcbd0d..7ed8a42c3a 100644
--- a/app/services/bulk_api_import/fhir_medication_request_importer.rb
+++ b/app/services/bulk_api_import/fhir_medication_request_importer.rb
@@ -17,6 +17,7 @@ class BulkApiImport::FhirMedicationRequestImporter
   def initialize(resource:, organization_id:)
     @resource = resource
     @organization_id = organization_id
+    @import_user = find_or_create_import_user(organization_id)
   end
 
   def import
@@ -24,12 +25,12 @@ def import
       .then { Api::V3::PrescriptionDrugTransformer.from_request(_1).merge(metadata) }
       .then { PrescriptionDrug.merge(_1) }
 
-    AuditLog.merge_log(import_user, merge_result) if merge_result.present?
+    AuditLog.merge_log(@import_user, merge_result) if merge_result.present?
     merge_result
   end
 
   def metadata
-    {user_id: import_user.id}
+    {user_id: @import_user.id}
   end
 
   def build_attributes
diff --git a/app/services/bulk_api_import/fhir_observation_importer.rb b/app/services/bulk_api_import/fhir_observation_importer.rb
index 936c74789e..b5aa01a6c1 100644
--- a/app/services/bulk_api_import/fhir_observation_importer.rb
+++ b/app/services/bulk_api_import/fhir_observation_importer.rb
@@ -12,6 +12,7 @@ class BulkApiImport::FhirObservationImporter
   def initialize(resource:, organization_id:)
     @resource = resource
     @organization_id = organization_id
+    @import_user = find_or_create_import_user(organization_id)
   end
 
   def import
@@ -24,7 +25,7 @@ def import
       raise "unknown observation type"
     end
 
-    AuditLog.merge_log(import_user, merge_result) if merge_result.present?
+    AuditLog.merge_log(@import_user, merge_result) if merge_result.present?
     merge_result
   end
 
@@ -45,7 +46,7 @@ def build_blood_pressure_attributes
       id: translate_id(@resource.dig(:identifier, 0, :value), org_id: @organization_id),
       patient_id: translate_id(@resource[:subject][:identifier], org_id: @organization_id),
       facility_id: translate_facility_id(@resource[:performer][0][:identifier], org_id: @organization_id),
-      user_id: import_user.id,
+      user_id: @import_user.id,
       recorded_at: @resource[:effectiveDateTime],
       **dig_blood_pressure,
       **timestamps
@@ -65,7 +66,7 @@ def build_blood_sugar_attributes
       id: translate_id(@resource.dig(:identifier, 0, :value), org_id: @organization_id),
       patient_id: translate_id(@resource[:subject][:identifier], org_id: @organization_id),
       facility_id: translate_facility_id(@resource[:performer][0][:identifier], org_id: @organization_id),
-      user_id: import_user.id,
+      user_id: @import_user.id,
       recorded_at: @resource[:effectiveDateTime],
       **dig_blood_sugar,
       **timestamps
@@ -88,6 +89,6 @@ def current_timezone_offset
 
   # For compatibility with SyncEncounterObservation
   def current_user
-    import_user
+    @import_user
   end
 end
diff --git a/app/services/bulk_api_import/fhir_patient_importer.rb b/app/services/bulk_api_import/fhir_patient_importer.rb
index 730031682b..86d39e9ff7 100644
--- a/app/services/bulk_api_import/fhir_patient_importer.rb
+++ b/app/services/bulk_api_import/fhir_patient_importer.rb
@@ -6,6 +6,7 @@ class BulkApiImport::FhirPatientImporter
   def initialize(resource:, organization_id:)
     @resource = resource
     @organization_id = organization_id
+    @import_user = find_or_create_import_user(organization_id)
   end
 
   def import
@@ -13,12 +14,12 @@ def import
       .then { Api::V3::PatientTransformer.from_nested_request(_1) }
       .then { MergePatientService.new(_1, request_metadata: request_metadata).merge }
 
-    AuditLog.merge_log(import_user, merge_result) if merge_result.present?
+    AuditLog.merge_log(@import_user, merge_result) if merge_result.present?
     merge_result
   end
 
   def request_metadata
-    {request_user_id: import_user.id,
+    {request_user_id: @import_user.id,
      request_facility_id: @resource.dig(:managingOrganization, 0, :value)}
   end
 
diff --git a/app/services/import_user.rb b/app/services/import_user.rb
index 2c73ab9028..4b7aac2fc7 100644
--- a/app/services/import_user.rb
+++ b/app/services/import_user.rb
@@ -1,16 +1,20 @@
 class ImportUser
   IMPORT_USER_PHONE_NUMBER = "0000000001"
 
-  def self.find_or_create
-    find || create
+  def self.find_or_create(org_id:)
+    find(org_id) || create(org_id)
   end
 
-  def self.find
-    PhoneNumberAuthentication.find_by(phone_number: IMPORT_USER_PHONE_NUMBER)&.user
+  def self.find(org_id)
+    PhoneNumberAuthentication.joins(:user)
+      .find_by(phone_number: IMPORT_USER_PHONE_NUMBER, user: {organization_id: org_id})&.user
   end
 
-  def self.create
-    facility = Facility.first
+  def self.create(org_id)
+    facility = Organization.find_by(id: org_id).facilities.first
+    unless facility.present?
+      raise ArgumentError, "given organization: #{org_id} does not exist or has no facilities"
+    end
 
     user = User.new(
       full_name: "import-user",
diff --git a/app/services/patient_import/importer.rb b/app/services/patient_import/importer.rb
index 10ed545f77..61fa919844 100644
--- a/app/services/patient_import/importer.rb
+++ b/app/services/patient_import/importer.rb
@@ -52,7 +52,7 @@ def import
   def import_patient(params)
     MergePatientService.new(params, request_metadata: {
       request_facility_id: facility.id,
-      request_user_id: ImportUser.find_or_create.id
+      request_user_id: ImportUser.find_or_create(org_id: facility.organization_id).id
     }).merge
   end
 
@@ -79,6 +79,6 @@ def current_timezone_offset
 
   # SyncEncounterObservation compability
   def current_user
-    ImportUser.find_or_create
+    ImportUser.find_or_create(org_id: facility.organization_id)
   end
 end
diff --git a/app/services/patient_import/spreadsheet_transformer.rb b/app/services/patient_import/spreadsheet_transformer.rb
index 3716d64b0a..81cb1a92c9 100644
--- a/app/services/patient_import/spreadsheet_transformer.rb
+++ b/app/services/patient_import/spreadsheet_transformer.rb
@@ -217,7 +217,7 @@ def registration_facility_id
     end
 
     def import_user
-      @import_user = ImportUser.find_or_create
+      @import_user = ImportUser.find_or_create(org_id: facility.organization_id)
     end
 
     def patient_status(row)
diff --git a/spec/jobs/bulk_api_import_job_spec.rb b/spec/jobs/bulk_api_import_job_spec.rb
index 122204043f..b45ec6b933 100644
--- a/spec/jobs/bulk_api_import_job_spec.rb
+++ b/spec/jobs/bulk_api_import_job_spec.rb
@@ -2,11 +2,11 @@
 
 RSpec.describe BulkApiImportJob do
   include ActiveJob::TestHelper
-  before { FactoryBot.create(:facility) } # needed for our bot import user
+  let(:facility) { create(:facility) }
 
   describe "#perform_later" do
     let(:resource) { build_condition_import_resource }
-    let(:job) { described_class.perform_later(resources: [resource], organization_id: "org_id") }
+    let(:job) { described_class.perform_later(resources: [resource], organization_id: facility.organization_id) }
 
     it "queues the job" do
       expect { job }.to have_enqueued_job(described_class).once.on_queue("default")
diff --git a/spec/services/bulk_api_import/fhir_appointment_importer_spec.rb b/spec/services/bulk_api_import/fhir_appointment_importer_spec.rb
index 03de21320b..cf51fcf64d 100644
--- a/spec/services/bulk_api_import/fhir_appointment_importer_spec.rb
+++ b/spec/services/bulk_api_import/fhir_appointment_importer_spec.rb
@@ -1,10 +1,9 @@
 require "rails_helper"
 
 RSpec.describe BulkApiImport::FhirAppointmentImporter do
-  before { create(:facility) }
-  let(:import_user) { ImportUser.find_or_create }
-  let(:org_id) { import_user.organization_id }
-  let(:facility) { import_user.facility }
+  let(:facility) { create(:facility) }
+  let(:org_id) { facility.organization_id }
+  let(:import_user) { ImportUser.find_or_create(org_id: org_id) }
   let(:facility_identifiers) do
     create(:facility_business_identifier, facility: facility, identifier_type: :external_org_facility_id)
   end
diff --git a/spec/services/bulk_api_import/fhir_condition_importer_spec.rb b/spec/services/bulk_api_import/fhir_condition_importer_spec.rb
index 805acf0c21..f510ff2a73 100644
--- a/spec/services/bulk_api_import/fhir_condition_importer_spec.rb
+++ b/spec/services/bulk_api_import/fhir_condition_importer_spec.rb
@@ -1,9 +1,9 @@
 require "rails_helper"
 
 RSpec.describe BulkApiImport::FhirConditionImporter do
-  before { create(:facility) }
-  let(:import_user) { ImportUser.find_or_create }
-  let(:org_id) { import_user.organization_id }
+  let(:facility) { create(:facility) }
+  let(:org_id) { facility.organization_id }
+  let(:import_user) { ImportUser.find_or_create(org_id: org_id) }
   let(:identifier) { SecureRandom.uuid }
   let(:patient) do
     build_stubbed(:patient, id: Digest::UUID.uuid_v5(Digest::UUID::DNS_NAMESPACE + org_id, identifier))
diff --git a/spec/services/bulk_api_import/fhir_importable_spec.rb b/spec/services/bulk_api_import/fhir_importable_spec.rb
index b50edf2eda..e2ec28d739 100644
--- a/spec/services/bulk_api_import/fhir_importable_spec.rb
+++ b/spec/services/bulk_api_import/fhir_importable_spec.rb
@@ -1,16 +1,15 @@
 require "rails_helper"
 
 RSpec.describe BulkApiImport::FhirImportable do
-  before { create(:facility) }
-  let(:import_user) { ImportUser.find_or_create }
-  let(:facility) { import_user.facility }
+  let(:facility) { create(:facility) }
   let(:org_id) { facility.organization_id }
+  let(:import_user) { ImportUser.find_or_create(org_id: org_id) }
   let(:facility_identifiers) do
     create(:facility_business_identifier, facility: facility, identifier_type: :external_org_facility_id)
   end
 
-  describe "#import_user" do
-    specify { expect(Object.new.extend(described_class).import_user).to eq(import_user) }
+  describe "#find_or_create_import_user" do
+    specify { expect(Object.new.extend(described_class).find_or_create_import_user(org_id)).to eq(import_user) }
   end
 
   describe "#translate_facility_id" do
diff --git a/spec/services/bulk_api_import/fhir_medication_request_importer_spec.rb b/spec/services/bulk_api_import/fhir_medication_request_importer_spec.rb
index f1a911b075..e5378c60a1 100644
--- a/spec/services/bulk_api_import/fhir_medication_request_importer_spec.rb
+++ b/spec/services/bulk_api_import/fhir_medication_request_importer_spec.rb
@@ -1,10 +1,9 @@
 require "rails_helper"
 
 RSpec.describe BulkApiImport::FhirMedicationRequestImporter do
-  before { create(:facility) }
-  let(:import_user) { ImportUser.find_or_create }
-  let(:facility) { import_user.facility }
+  let(:facility) { create(:facility) }
   let(:org_id) { facility.organization_id }
+  let(:import_user) { ImportUser.find_or_create(org_id: org_id) }
   let(:facility_identifier) do
     create(:facility_business_identifier, facility: facility, identifier_type: :external_org_facility_id)
   end
@@ -48,7 +47,7 @@
 
   describe "#contained_medication" do
     it "fetches the contained medication" do
-      expect(described_class.new(resource: {contained: [{code: {}}]}, organization_id: "").contained_medication)
+      expect(described_class.new(resource: {contained: [{code: {}}]}, organization_id: org_id).contained_medication)
         .to eq({code: {}})
     end
   end
@@ -65,7 +64,7 @@
           resource: {
             dosageInstruction: [{timing: {code: input_code}}]
           },
-          organization_id: ""
+          organization_id: org_id
         ).frequency).to eq(expected_value)
       end
     end
@@ -111,7 +110,7 @@
         {status: "inactive", deletion_status: true},
         {status: "inactive", deletion_status: true}
       ].each do |status:, deletion_status:|
-        expect(described_class.new(resource: {contained: [{status: status}]}, organization_id: "").drug_deleted?)
+        expect(described_class.new(resource: {contained: [{status: status}]}, organization_id: org_id).drug_deleted?)
           .to eq(deletion_status)
       end
     end
diff --git a/spec/services/bulk_api_import/fhir_observation_importer_spec.rb b/spec/services/bulk_api_import/fhir_observation_importer_spec.rb
index bcc15a0409..8113154b77 100644
--- a/spec/services/bulk_api_import/fhir_observation_importer_spec.rb
+++ b/spec/services/bulk_api_import/fhir_observation_importer_spec.rb
@@ -1,9 +1,9 @@
 require "rails_helper"
 
 RSpec.describe BulkApiImport::FhirObservationImporter do
-  before { create(:facility) }
-  let(:import_user) { ImportUser.find_or_create }
-  let(:facility) { import_user.facility }
+  let(:facility) { create(:facility) }
+  let(:org_id) { facility.organization_id }
+  let(:import_user) { ImportUser.find_or_create(org_id: org_id) }
   let(:org_id) { facility.organization_id }
   let(:facility_identifier) do
     create(:facility_business_identifier, facility: facility, identifier_type: :external_org_facility_id)
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 7117dbdfc2..779ca4f206 100644
--- a/spec/services/bulk_api_import/fhir_patient_importer_spec.rb
+++ b/spec/services/bulk_api_import/fhir_patient_importer_spec.rb
@@ -1,10 +1,9 @@
 require "rails_helper"
 
 RSpec.describe BulkApiImport::FhirPatientImporter do
-  before { create(:facility) }
-  let(:import_user) { ImportUser.find_or_create }
-  let(:org_id) { import_user.organization_id }
-  let(:facility) { import_user.facility }
+  let(:facility) { create(:facility) }
+  let(:org_id) { facility.organization_id }
+  let(:import_user) { ImportUser.find_or_create(org_id: org_id) }
   let(:facility_identifier) do
     create(:facility_business_identifier, facility: facility, identifier_type: :external_org_facility_id)
   end
diff --git a/spec/services/bulk_api_import/importer_spec.rb b/spec/services/bulk_api_import/importer_spec.rb
index 967e989fe1..efb9652b14 100644
--- a/spec/services/bulk_api_import/importer_spec.rb
+++ b/spec/services/bulk_api_import/importer_spec.rb
@@ -1,10 +1,8 @@
 require "rails_helper"
 
 RSpec.describe BulkApiImport::Importer do
-  before { FactoryBot.create(:facility) } # needed for our bot import user
-
   describe "#import" do
-    let(:facility) { Facility.first }
+    let(:facility) { create(:facility) }
     let(:organization_id) { facility.organization_id }
     let(:facility_identifier) do
       create(:facility_business_identifier, facility: facility, identifier_type: :external_org_facility_id)
@@ -80,7 +78,8 @@
 
   describe "#resource_importer" do
     it "fetches the correct importer" do
-      importer = described_class.new(resource_list: [], organization_id: "org_id")
+      org_id = create(:facility).organization_id
+      importer = described_class.new(resource_list: [], organization_id: org_id)
 
       [
         {input: {resourceType: "Patient"}, expected_importer: BulkApiImport::FhirPatientImporter},
@@ -89,7 +88,7 @@
         {input: {resourceType: "MedicationRequest"}, expected_importer: BulkApiImport::FhirMedicationRequestImporter},
         {input: {resourceType: "Condition"}, expected_importer: BulkApiImport::FhirConditionImporter}
       ].each do |input:, expected_importer:|
-        expect(importer.resource_importer(input, "org_id"))
+        expect(importer.resource_importer(input, org_id))
           .to be_an_instance_of(expected_importer)
       end
     end
diff --git a/spec/services/patient_import/import_user_spec.rb b/spec/services/patient_import/import_user_spec.rb
index 03d4fa610f..883bb5a1be 100644
--- a/spec/services/patient_import/import_user_spec.rb
+++ b/spec/services/patient_import/import_user_spec.rb
@@ -3,34 +3,43 @@
 RSpec.describe ImportUser do
   describe ".find" do
     it "finds existing import user by phone number" do
-      import_user = create(:user, phone_number: ImportUser::IMPORT_USER_PHONE_NUMBER)
+      org_id = build_stubbed(:organization).id
+      import_user = create(:user, phone_number: ImportUser::IMPORT_USER_PHONE_NUMBER, organization_id: org_id)
 
-      expect(ImportUser.find).to eq(import_user)
+      expect(ImportUser.find(org_id)).to eq(import_user)
     end
 
     it "returns nil if not found" do
-      expect(ImportUser.find).to be_nil
+      expect(ImportUser.find("foo")).to be_nil
+    end
+
+    it "returns nil if it does not exist in the given organization" do
+      org_id = build_stubbed(:organization).id
+      _user = create(:user, phone_number: ImportUser::IMPORT_USER_PHONE_NUMBER, organization_id: org_id)
+
+      expect(ImportUser.find("bar")).to be_nil
     end
   end
 
   describe ".find_or_create" do
     it "finds existing import user by phone number" do
-      import_user = create(:user, phone_number: ImportUser::IMPORT_USER_PHONE_NUMBER)
+      org_id = build_stubbed(:organization).id
+      import_user = create(:user, phone_number: ImportUser::IMPORT_USER_PHONE_NUMBER, organization_id: org_id)
 
-      expect { ImportUser.find_or_create }.not_to change { User.count }
-      expect(ImportUser.find_or_create).to eq(import_user)
+      expect { ImportUser.find_or_create(org_id: org_id) }.not_to change { User.count }
+      expect(ImportUser.find_or_create(org_id: org_id)).to eq(import_user)
     end
 
     it "creates a new user if not found" do
-      _facility = create(:facility)
-      import_user = ImportUser.find_or_create
+      facility = create(:facility)
+      import_user = ImportUser.find_or_create(org_id: facility.organization_id)
       expect(import_user).to be_persisted
       expect(import_user.phone_number).to eq(ImportUser::IMPORT_USER_PHONE_NUMBER)
     end
 
     it "ensures the new user cannot sync data" do
-      _facility = create(:facility)
-      import_user = ImportUser.find_or_create
+      facility = create(:facility)
+      import_user = ImportUser.find_or_create(org_id: facility.organization_id)
 
       expect(import_user.otp_valid?).to eq(false)
       expect(import_user).to be_sync_approval_status_denied
diff --git a/spec/services/patient_import/spreadsheet_transformer_spec.rb b/spec/services/patient_import/spreadsheet_transformer_spec.rb
index e312c6c1d9..d247f16906 100644
--- a/spec/services/patient_import/spreadsheet_transformer_spec.rb
+++ b/spec/services/patient_import/spreadsheet_transformer_spec.rb
@@ -8,7 +8,7 @@
   it "parses patient data" do
     params = PatientImport::SpreadsheetTransformer.call(data, facility: facility)
 
-    import_user = ImportUser.find_or_create
+    import_user = ImportUser.find_or_create(org_id: facility.organization_id)
     patient = params.find { |p| p[:patient][:full_name] == "Basic Patient 1" }.deep_symbolize_keys
     patient_id = patient[:patient][:id]
     registration_time = Time.parse("2020-10-16").rfc3339