diff --git a/app/services/imports/user_import_service.rb b/app/services/imports/user_import_service.rb index 4db913171..5cd412fca 100644 --- a/app/services/imports/user_import_service.rb +++ b/app/services/imports/user_import_service.rb @@ -14,11 +14,16 @@ module Imports organisation = Organisation.find_by(old_org_id: user_field_value(xml_document, "institution")) old_user_id = user_field_value(xml_document, "id") name = user_field_value(xml_document, "full-name") + email = user_field_value(xml_document, "email") if User.find_by(old_user_id:, organisation:) @logger.warn("User #{name} with old user id #{old_user_id} is already present, skipping.") + elsif (user = User.find_by(email:, organisation:)) + is_dpo = user.is_data_protection_officer? || is_dpo?(user_field_value(xml_document, "user-type")) + role = highest_role(user.role, role(user_field_value(xml_document, "user-type"))) + user.update!(role:, is_dpo:) else User.create!( - email: user_field_value(xml_document, "user-name"), + email:, name:, password: Devise.friendly_token, phone: user_field_value(xml_document, "telephone-no"), @@ -45,6 +50,14 @@ module Imports }[field_value.downcase.strip] end + def highest_role(role_a, role_b) + return unless role_a || role_b + return role_a unless role_b + return role_b unless role_a + + [role_a, role_b].map(&:to_sym).sort! { |a, b| User::ROLES[b] <=> User::ROLES[a] }.first + end + def is_dpo?(field_value) return false if field_value.blank? diff --git a/spec/fixtures/softwire_imports/users/10c887710550844e2551b3e0fb88dc9b4a8a642b.xml b/spec/fixtures/softwire_imports/users/10c887710550844e2551b3e0fb88dc9b4a8a642b.xml new file mode 100644 index 000000000..70d0d8ac9 --- /dev/null +++ b/spec/fixtures/softwire_imports/users/10c887710550844e2551b3e0fb88dc9b4a8a642b.xml @@ -0,0 +1,13 @@ + + 10c887710550844e2551b3e0fb88dc9b4a8a642b + xxx + John Doe + john.doe + 7c5bd5fb549c09a2c55d7cb90d7ba84927e64618 + john.doe@gov.uk + Data Protection Officer + true + false + None + 02012345678 + diff --git a/spec/fixtures/softwire_imports/users/b7829b1a5dfb68bb1e01c08445830c0add40907c.xml b/spec/fixtures/softwire_imports/users/b7829b1a5dfb68bb1e01c08445830c0add40907c.xml index f2387384f..2f6a59ee8 100644 --- a/spec/fixtures/softwire_imports/users/b7829b1a5dfb68bb1e01c08445830c0add40907c.xml +++ b/spec/fixtures/softwire_imports/users/b7829b1a5dfb68bb1e01c08445830c0add40907c.xml @@ -2,7 +2,7 @@ b7829b1a5dfb68bb1e01c08445830c0add40907c xxx John Doe - john.doe@gov.uk + john.doe 7c5bd5fb549c09a2c55d7cb90d7ba84927e64618 john.doe@gov.uk Private Data Downloader diff --git a/spec/fixtures/softwire_imports/users/d4729b1a5dfb68bb1e01c08445830c0add40907c.xml b/spec/fixtures/softwire_imports/users/d4729b1a5dfb68bb1e01c08445830c0add40907c.xml index 77ac91600..3af76d2df 100644 --- a/spec/fixtures/softwire_imports/users/d4729b1a5dfb68bb1e01c08445830c0add40907c.xml +++ b/spec/fixtures/softwire_imports/users/d4729b1a5dfb68bb1e01c08445830c0add40907c.xml @@ -2,7 +2,7 @@ d4729b1a5dfb68bb1e01c08445830c0add40907c xxx John Doe - john.doe@gov.uk + john.doe 7c5bd5fb549c09a2c55d7cb90d7ba84927e64618 john.doe@gov.uk Co-ordinator diff --git a/spec/fixtures/softwire_imports/users/d6717836154cd9a58f9e2f1d3077e3ab81e07613.xml b/spec/fixtures/softwire_imports/users/d6717836154cd9a58f9e2f1d3077e3ab81e07613.xml index 9b390fa29..83a8b6a05 100644 --- a/spec/fixtures/softwire_imports/users/d6717836154cd9a58f9e2f1d3077e3ab81e07613.xml +++ b/spec/fixtures/softwire_imports/users/d6717836154cd9a58f9e2f1d3077e3ab81e07613.xml @@ -2,7 +2,7 @@ d6717836154cd9a58f9e2f1d3077e3ab81e07613 xxx John Doe - john.doe@gov.uk + john.doe 7c5bd5fb549c09a2c55d7cb90d7ba84927e64618 john.doe@gov.uk Co-ordinator diff --git a/spec/fixtures/softwire_imports/users/fc7625a02b24ae16162aa63ae7cb33feeec0c373.xml b/spec/fixtures/softwire_imports/users/fc7625a02b24ae16162aa63ae7cb33feeec0c373.xml index 535756aca..be9a3f946 100644 --- a/spec/fixtures/softwire_imports/users/fc7625a02b24ae16162aa63ae7cb33feeec0c373.xml +++ b/spec/fixtures/softwire_imports/users/fc7625a02b24ae16162aa63ae7cb33feeec0c373.xml @@ -2,7 +2,7 @@ fc7625a02b24ae16162aa63ae7cb33feeec0c373 xxx John Doe - john.doe@gov.uk + john.doe 7c5bd5fb549c09a2c55d7cb90d7ba84927e64618 john.doe@gov.uk Data Provider diff --git a/spec/services/imports/user_import_service_spec.rb b/spec/services/imports/user_import_service_spec.rb index 9dd78fe09..70552f41a 100644 --- a/spec/services/imports/user_import_service_spec.rb +++ b/spec/services/imports/user_import_service_spec.rb @@ -57,6 +57,18 @@ RSpec.describe Imports::UserImportService do end end + context "when the user is a data protection officer" do + let(:old_user_id) { "10c887710550844e2551b3e0fb88dc9b4a8a642b" } + + it "marks them as a data protection officer" do + FactoryBot.create(:organisation, old_org_id:) + import_service.create_users("user_directory") + + user = User.find_by(old_user_id:) + expect(user.is_data_protection_officer?).to be true + end + end + context "when the user was a 'Key Performance Contact' in the old system" do let(:old_user_id) { "d4729b1a5dfb68bb1e01c08445830c0add40907c" } @@ -92,5 +104,54 @@ RSpec.describe Imports::UserImportService do import_service.create_users("user_directory") end end + + context "when a user has already been imported with that email" do + let!(:org) { FactoryBot.create(:organisation, old_org_id:) } + let!(:user) { FactoryBot.create(:user, :data_provider, organisation: org, email: "john.doe@gov.uk") } + + context "when the duplicate role is higher than the original role" do + let(:old_user_id) { "d4729b1a5dfb68bb1e01c08445830c0add40907c" } + + it "upgrades their role" do + import_service.create_users("user_directory") + expect(user.reload).to be_data_coordinator + end + + it "does not create a new record" do + expect { import_service.create_users("user_directory") } + .not_to change(User, :count) + end + end + + context "when the duplicate role is lower than the original role" do + let!(:user) { FactoryBot.create(:user, :data_coordinator, organisation: org, email: "john.doe@gov.uk") } + let(:old_user_id) { "fc7625a02b24ae16162aa63ae7cb33feeec0c373" } + + it "does not change their role" do + expect { import_service.create_users("user_directory") } + .not_to(change { user.reload.role }) + end + + it "does not create a new record" do + expect { import_service.create_users("user_directory") } + .not_to change(User, :count) + end + end + + context "when the duplicate record is a data protection officer role" do + let!(:user) { FactoryBot.create(:user, :data_coordinator, organisation: org, email: "john.doe@gov.uk") } + let(:old_user_id) { "10c887710550844e2551b3e0fb88dc9b4a8a642b" } + + it "marks them as a data protection officer" do + import_service.create_users("user_directory") + expect(user.reload.is_data_protection_officer?).to be true + end + + it "does not create a new record" do + expect { import_service.create_users("user_directory") } + .not_to change(User, :count) + end + end + end end end