From 78345e7cb77b06c6cff0a7d02809036a6c9695b4 Mon Sep 17 00:00:00 2001
From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com>
Date: Fri, 25 Feb 2022 15:48:07 +0000
Subject: [PATCH] Fix check that imported users are unique (#335)

* Match imported users on id and org

* Rubocop
---
 app/services/imports/user_import_service.rb   | 26 +++++++++++--------
 .../imports/user_import_service_spec.rb       | 26 ++++++++++++++-----
 2 files changed, 35 insertions(+), 17 deletions(-)

diff --git a/app/services/imports/user_import_service.rb b/app/services/imports/user_import_service.rb
index e8195fcd9..0c32b582f 100644
--- a/app/services/imports/user_import_service.rb
+++ b/app/services/imports/user_import_service.rb
@@ -12,17 +12,21 @@ module Imports
 
     def create_user(xml_document)
       organisation = Organisation.find_by(old_org_id: user_field_value(xml_document, "institution"))
-      User.create!(
-        email: user_field_value(xml_document, "user-name"),
-        name: user_field_value(xml_document, "full-name"),
-        password: Devise.friendly_token,
-        phone: user_field_value(xml_document, "telephone-no"),
-        old_user_id: user_field_value(xml_document, "id"),
-        organisation:,
-        role: PROVIDER_TYPE[user_field_value(xml_document, "user-type")],
-      )
-    rescue ActiveRecord::RecordNotUnique
-      @logger.warn("User #{name} with old user id #{old_user_id} is already present, skipping.")
+      old_user_id = user_field_value(xml_document, "id")
+      name = user_field_value(xml_document, "full-name")
+      if User.find_by(old_user_id:, organisation:)
+        @logger.warn("User #{name} with old user id #{old_user_id} is already present, skipping.")
+      else
+        User.create!(
+          email: user_field_value(xml_document, "user-name"),
+          name:,
+          password: Devise.friendly_token,
+          phone: user_field_value(xml_document, "telephone-no"),
+          old_user_id:,
+          organisation:,
+          role: PROVIDER_TYPE[user_field_value(xml_document, "user-type")],
+        )
+      end
     end
 
     def user_field_value(xml_document, field)
diff --git a/spec/services/imports/user_import_service_spec.rb b/spec/services/imports/user_import_service_spec.rb
index 0e0c4d8b0..8401d7149 100644
--- a/spec/services/imports/user_import_service_spec.rb
+++ b/spec/services/imports/user_import_service_spec.rb
@@ -2,7 +2,9 @@ require "rails_helper"
 
 RSpec.describe Imports::UserImportService do
   let(:fixture_directory) { "spec/fixtures/softwire_imports/users" }
-  let(:user_file) { File.open("#{fixture_directory}/fc7625a02b24ae16162aa63ae7cb33feeec0c373.xml") }
+  let(:old_user_id) { "fc7625a02b24ae16162aa63ae7cb33feeec0c373" }
+  let(:old_org_id) { "7c5bd5fb549c09a2c55d7cb90d7ba84927e64618" }
+  let(:user_file) { File.open("#{fixture_directory}/#{old_user_id}.xml") }
   let(:storage_service) { instance_double(StorageService) }
 
   context "when importing users" do
@@ -10,28 +12,40 @@ RSpec.describe Imports::UserImportService do
 
     before do
       allow(storage_service).to receive(:list_files)
-                                  .and_return(["user_directory/fc7625a02b24ae16162aa63ae7cb33feeec0c373.xml"])
+                                  .and_return(["user_directory/#{old_user_id}.xml"])
       allow(storage_service).to receive(:get_file_io)
-                                  .with("user_directory/fc7625a02b24ae16162aa63ae7cb33feeec0c373.xml")
+                                  .with("user_directory/#{old_user_id}.xml")
                                   .and_return(user_file)
     end
 
     it "successfully create a user with the expected data" do
-      FactoryBot.create(:organisation, old_org_id: "7c5bd5fb549c09a2c55d7cb90d7ba84927e64618")
+      FactoryBot.create(:organisation, old_org_id:)
       import_service.create_users("user_directory")
 
-      user = User.find_by(old_user_id: "fc7625a02b24ae16162aa63ae7cb33feeec0c373")
+      user = User.find_by(old_user_id:)
       expect(user.name).to eq("John Doe")
       expect(user.email).to eq("john.doe@gov.uk")
       expect(user.encrypted_password).not_to be_nil
       expect(user.phone).to eq("02012345678")
       expect(user).to be_data_provider
-      expect(user.organisation.old_org_id).to eq("7c5bd5fb549c09a2c55d7cb90d7ba84927e64618")
+      expect(user.organisation.old_org_id).to eq(old_org_id)
     end
 
     it "refuses to create a user belonging to a non existing organisation" do
       expect { import_service.create_users("user_directory") }
         .to raise_error(ActiveRecord::RecordInvalid, /Organisation must exist/)
     end
+
+    context "when the user has already been imported previously" do
+      before do
+        org = FactoryBot.create(:organisation, old_org_id:)
+        FactoryBot.create(:user, old_user_id:, organisation: org)
+      end
+
+      it "logs that the user already exists" do
+        expect(Rails.logger).to receive(:warn)
+        import_service.create_users("user_directory")
+      end
+    end
   end
 end