diff --git a/app/services/import_user.rb b/app/services/import_user.rb index 4b7aac2fc7..c1c6fdcaee 100644 --- a/app/services/import_user.rb +++ b/app/services/import_user.rb @@ -1,13 +1,11 @@ class ImportUser - IMPORT_USER_PHONE_NUMBER = "0000000001" - def self.find_or_create(org_id:) find(org_id) || create(org_id) end def self.find(org_id) PhoneNumberAuthentication.joins(:user) - .find_by(phone_number: IMPORT_USER_PHONE_NUMBER, user: {organization_id: org_id})&.user + .find_by(phone_number: bot_phone_number(org_id), user: {organization_id: org_id})&.user end def self.create(org_id) @@ -24,7 +22,7 @@ def self.create(org_id) ) phone_number_authentication = PhoneNumberAuthentication.new( - phone_number: IMPORT_USER_PHONE_NUMBER, + phone_number: bot_phone_number(org_id), password: generate_pin, registration_facility_id: facility.id ).tap do |pna| @@ -43,4 +41,12 @@ def self.create(org_id) def self.generate_pin "#{rand(10)}#{rand(10)}#{rand(10)}#{rand(10)}" end + + def self.bot_phone_number(org_id) + # Losslessly compresses an organisation UUID value into a url-safe base64 representation. + # This is a bit more compact than stuffing a UUID string as a phone number directly. Other + # forms of hashing may lead to information loss, doing it this way keeps the collision + # probability the same as the organization UUID itself. + [[org_id.remove("-")].pack("H*")].pack("m0").tr("+/", "-_")[..21] + end end diff --git a/spec/services/patient_import/import_user_spec.rb b/spec/services/patient_import/import_user_spec.rb index 883bb5a1be..0d5ac4e3e7 100644 --- a/spec/services/patient_import/import_user_spec.rb +++ b/spec/services/patient_import/import_user_spec.rb @@ -4,7 +4,7 @@ describe ".find" do it "finds existing import user by phone number" do org_id = build_stubbed(:organization).id - import_user = create(:user, phone_number: ImportUser::IMPORT_USER_PHONE_NUMBER, organization_id: org_id) + import_user = create(:user, phone_number: ImportUser.bot_phone_number(org_id), organization_id: org_id) expect(ImportUser.find(org_id)).to eq(import_user) end @@ -15,7 +15,7 @@ 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) + _user = create(:user, phone_number: ImportUser.bot_phone_number(org_id), organization_id: org_id) expect(ImportUser.find("bar")).to be_nil end @@ -24,7 +24,7 @@ describe ".find_or_create" do it "finds existing import user by phone number" do org_id = build_stubbed(:organization).id - import_user = create(:user, phone_number: ImportUser::IMPORT_USER_PHONE_NUMBER, organization_id: org_id) + import_user = create(:user, phone_number: ImportUser.bot_phone_number(org_id), organization_id: org_id) 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) @@ -33,8 +33,10 @@ it "creates a new user if not found" do facility = create(:facility) import_user = ImportUser.find_or_create(org_id: facility.organization_id) + import_user_phone_number = ImportUser.bot_phone_number(facility.organization_id) + expect(import_user).to be_persisted - expect(import_user.phone_number).to eq(ImportUser::IMPORT_USER_PHONE_NUMBER) + expect(import_user.phone_number).to eq(import_user_phone_number) end it "ensures the new user cannot sync data" do @@ -45,4 +47,12 @@ expect(import_user).to be_sync_approval_status_denied end end + + describe "#bot_phone_number" do + it "always gives the same output for an org ID" do + some_org_id = SecureRandom.uuid + + expect(5.times.map { ImportUser.bot_phone_number(some_org_id) }.uniq.length).to eq(1) + end + end end