From 2776c061b6a84bc6a4f18c73b677143e32d887da Mon Sep 17 00:00:00 2001 From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com> Date: Wed, 30 Mar 2022 16:45:26 +0100 Subject: [PATCH] CLDC-1063: Key Contacts (#435) * Add data field * Add field to views * Only data coordinators can change role, dpo, keycontact * Import key contact field * Rubocop * Import data accessors * Handle non-unqiue email imports --- app/controllers/users_controller.rb | 22 +- app/models/user.rb | 8 + app/services/imports/user_import_service.rb | 41 ++- app/views/users/edit.html.erb | 24 +- app/views/users/new.html.erb | 24 +- app/views/users/show.html.erb | 16 +- ...329125601_add_key_contact_field_to_user.rb | 7 + db/schema.rb | 1 + spec/features/user_spec.rb | 240 +++++++++++------- ...c887710550844e2551b3e0fb88dc9b4a8a642b.xml | 13 + ...829b1a5dfb68bb1e01c08445830c0add40907c.xml | 13 + ...729b1a5dfb68bb1e01c08445830c0add40907c.xml | 13 + ...717836154cd9a58f9e2f1d3077e3ab81e07613.xml | 13 + ...7625a02b24ae16162aa63ae7cb33feeec0c373.xml | 2 +- spec/models/user_spec.rb | 10 +- spec/requests/users_controller_spec.rb | 106 +++++--- .../imports/user_import_service_spec.rb | 106 ++++++++ 17 files changed, 495 insertions(+), 164 deletions(-) create mode 100644 db/migrate/20220329125601_add_key_contact_field_to_user.rb create mode 100644 spec/fixtures/softwire_imports/users/10c887710550844e2551b3e0fb88dc9b4a8a642b.xml create mode 100644 spec/fixtures/softwire_imports/users/b7829b1a5dfb68bb1e01c08445830c0add40907c.xml create mode 100644 spec/fixtures/softwire_imports/users/d4729b1a5dfb68bb1e01c08445830c0add40907c.xml create mode 100644 spec/fixtures/softwire_imports/users/d6717836154cd9a58f9e2f1d3077e3ab81e07613.xml diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index c4062f6d5..7c90ea738 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -3,7 +3,7 @@ class UsersController < ApplicationController include Helpers::Email before_action :authenticate_user! before_action :find_resource, except: %i[new create] - before_action :authenticate_scope!, except: %i[new create] + before_action :authenticate_scope!, except: %i[new] def update if @user.update(user_params) @@ -76,9 +76,13 @@ private def user_params if @user == current_user - params.require(:user).permit(:email, :name, :password, :password_confirmation, :role, :is_dpo) - else - params.require(:user).permit(:email, :name, :role, :is_dpo) + if current_user.data_coordinator? + params.require(:user).permit(:email, :name, :password, :password_confirmation, :role, :is_dpo, :is_key_contact) + else + params.require(:user).permit(:email, :name, :password, :password_confirmation) + end + elsif current_user.data_coordinator? + params.require(:user).permit(:email, :name, :role, :is_dpo, :is_key_contact) end end @@ -87,8 +91,12 @@ private end def authenticate_scope! - render_not_found and return unless current_user.organisation == @user.organisation - render_not_found and return if action_name == "edit_password" && current_user != @user - render_not_found and return unless current_user.role == "data_coordinator" || current_user == @user + if action_name == "create" + head :unauthorized and return unless current_user.data_coordinator? + else + render_not_found and return unless current_user.organisation == @user.organisation + render_not_found and return if action_name == "edit_password" && current_user != @user + render_not_found and return unless current_user.data_coordinator? || current_user == @user + end end end diff --git a/app/models/user.rb b/app/models/user.rb index 2fd63c71d..5defc6129 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -37,6 +37,14 @@ class User < ApplicationRecord last_sign_in_at ? RESET_PASSWORD_TEMPLATE_ID : SET_PASSWORD_TEMPLATE_ID end + def is_key_contact? + is_key_contact + end + + def is_key_contact! + update(is_key_contact: true) + end + def is_data_protection_officer? is_dpo end diff --git a/app/services/imports/user_import_service.rb b/app/services/imports/user_import_service.rb index 0c32b582f..5cd412fca 100644 --- a/app/services/imports/user_import_service.rb +++ b/app/services/imports/user_import_service.rb @@ -14,17 +14,24 @@ 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"), old_user_id:, organisation:, - role: PROVIDER_TYPE[user_field_value(xml_document, "user-type")], + role: role(user_field_value(xml_document, "user-type")), + is_dpo: is_dpo?(user_field_value(xml_document, "user-type")), + is_key_contact: is_key_contact?(user_field_value(xml_document, "contact-priority-id")), ) end end @@ -32,5 +39,35 @@ module Imports def user_field_value(xml_document, field) field_value(xml_document, "user", field) end + + def role(field_value) + return unless field_value + + { + "co-ordinator" => "data_coordinator", + "data provider" => "data_provider", + "private data downloader" => "data_accessor", + }[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? + + field_value.downcase.strip == "data protection officer" + end + + def is_key_contact?(field_value) + return false if field_value.blank? + + ["ecore contact", "key performance contact"].include?(field_value.downcase.strip) + end end end diff --git a/app/views/users/edit.html.erb b/app/views/users/edit.html.erb index 649d4f614..6582903d1 100644 --- a/app/views/users/edit.html.erb +++ b/app/views/users/edit.html.erb @@ -26,13 +26,23 @@ spellcheck: "false" %> - <%= f.govuk_collection_radio_buttons :is_dpo, - [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")], - :id, - :name, - inline: true, - legend: { text: "Are #{current_user == @user ? "you" : "they"} a data protection officer?", size: "m" } - %> + <% if current_user.data_coordinator? %> + <%= f.govuk_collection_radio_buttons :is_dpo, + [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")], + :id, + :name, + inline: true, + legend: { text: "Are #{current_user == @user ? "you" : "they"} a data protection officer?", size: "m" } + %> + + <%= f.govuk_collection_radio_buttons :is_key_contact, + [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")], + :id, + :name, + inline: true, + legend: { text: "Are #{current_user == @user ? "you" : "they"} a key contact?", size: "m" } + %> + <% end %> <%= f.govuk_submit "Save changes" %> diff --git a/app/views/users/new.html.erb b/app/views/users/new.html.erb index 8b89d0c47..bc9e0b1f0 100644 --- a/app/views/users/new.html.erb +++ b/app/views/users/new.html.erb @@ -31,13 +31,23 @@ f.govuk_collection_radio_buttons :role, roles, :id, :name, legend: { text: "Role", size: "m" } %> - <%= f.govuk_collection_radio_buttons :is_dpo, - [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")], - :id, - :name, - inline: true, - legend: { text: "Are #{current_user == @user ? "you" : "they"} a data protection officer?", size: "m" } - %> + <% if current_user.data_coordinator? %> + <%= f.govuk_collection_radio_buttons :is_dpo, + [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")], + :id, + :name, + inline: true, + legend: { text: "Are #{current_user == @user ? "you" : "they"} a data protection officer?", size: "m" } + %> + + <%= f.govuk_collection_radio_buttons :is_key_contact, + [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")], + :id, + :name, + inline: true, + legend: { text: "Are #{current_user == @user ? "you" : "they"} a key contact?", size: "m" } + %> + <% end %> <%= f.govuk_submit "Continue" %> diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index 66c0a9145..61a8c8c09 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -48,7 +48,21 @@ <%= summary_list.row do |row| row.key { 'Data protection officer' } row.value { @user.is_data_protection_officer? ? "Yes" : "No" } - row.action(visually_hidden_text: "are #{current_user == @user ? "you" : "they"} a data protection officer?", href: edit_user_path, html_attributes: { "data-qa": "change-are-#{current_user == @user ? "you" : "they"}-a-data-protection-officer" }) + if current_user.data_coordinator? + row.action(visually_hidden_text: "are #{current_user == @user ? "you" : "they"} a data protection officer?", href: edit_user_path, html_attributes: { "data-qa": "change-are-#{current_user == @user ? "you" : "they"}-a-data-protection-officer" }) + else + row.action() + end + end %> + + <%= summary_list.row do |row| + row.key { 'Key contact' } + row.value { @user.is_key_contact? ? "Yes" : "No" } + if current_user.data_coordinator? + row.action(visually_hidden_text: "are #{current_user == @user ? "you" : "they"} a key contact?", href: edit_user_path, html_attributes: { "data-qa": "change-are-#{current_user == @user ? "you" : "they"}-a-key-contact" }) + else + row.action() + end end %> <% end %> diff --git a/db/migrate/20220329125601_add_key_contact_field_to_user.rb b/db/migrate/20220329125601_add_key_contact_field_to_user.rb new file mode 100644 index 000000000..15634e574 --- /dev/null +++ b/db/migrate/20220329125601_add_key_contact_field_to_user.rb @@ -0,0 +1,7 @@ +class AddKeyContactFieldToUser < ActiveRecord::Migration[7.0] + def change + change_table :users, bulk: true do |t| + t.column :is_key_contact, :boolean, default: false + end + end +end diff --git a/db/schema.rb b/db/schema.rb index f92f6a2d9..965fcee3d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -317,6 +317,7 @@ ActiveRecord::Schema[7.0].define(version: 202202071123100) do t.string "unlock_token" t.datetime "locked_at", precision: nil t.boolean "is_dpo", default: false + t.boolean "is_key_contact", default: false t.string "phone" t.index ["email"], name: "index_users_on_email", unique: true t.index ["organisation_id"], name: "index_users_on_organisation_id" diff --git a/spec/features/user_spec.rb b/spec/features/user_spec.rb index b30f8a855..8ff9bed98 100644 --- a/spec/features/user_spec.rb +++ b/spec/features/user_spec.rb @@ -172,112 +172,156 @@ RSpec.describe "User Features" do end end - context "when viewing your account" do - before do - visit("/logs") - fill_in("user[email]", with: user.email) - fill_in("user[password]", with: "pAssword1") - click_button("Sign in") - end - - it "shows 'Your account' link in navigation if logged in and redirect to correct page" do - visit("/logs") - expect(page).to have_link("Your account") - click_link("Your account") - expect(page).to have_current_path("/users/#{user.id}") - end - - it "can navigate to change your password page from main account page" do - visit("/users/#{user.id}") - find('[data-qa="change-password"]').click - expect(page).to have_content("Change your password") - fill_in("user[password]", with: "Password123!") - fill_in("user[password_confirmation]", with: "Password123!") - click_button("Update") - expect(page).to have_current_path("/users/#{user.id}") - end - - it "allow user to change name" do - visit("/users/#{user.id}") - find('[data-qa="change-name"]').click - expect(page).to have_content("Change your personal details") - fill_in("user[name]", with: "Test New") - click_button("Save changes") - expect(page).to have_current_path("/users/#{user.id}") - expect(page).to have_content("Test New") + context "when signed in as a data provider" do + context "when viewing your account" do + before do + visit("/logs") + fill_in("user[email]", with: user.email) + fill_in("user[password]", with: "pAssword1") + click_button("Sign in") + end + + it "does not have change links for dpo and key contact" do + visit("/users/#{user.id}") + expect(page).not_to have_selector('[data-qa="change-are-you-a-data-protection-officer"]') + expect(page).not_to have_selector('[data-qa="change-are-you-a-key-contact"]') + end + + it "does not have dpo and key contact as editable fields" do + visit("/users/#{user.id}/edit") + expect(page).not_to have_field("user[is_dpo]") + expect(page).not_to have_field("user[is_key_contact]") + end end end - context "when adding a new user" do - before do - visit("/logs") - fill_in("user[email]", with: user.email) - fill_in("user[password]", with: "pAssword1") - click_button("Sign in") - end - - it "validates an email has been provided" do - visit("users/new") - fill_in("user[name]", with: "New User") - click_button("Continue") - expect(page).to have_selector("#error-summary-title") - expect(page).to have_selector("#user-email-field-error") - expect(page).to have_content(/Enter an email address/) - expect(page).to have_title("Error") - end - - it "validates email" do - visit("users/new") - fill_in("user[name]", with: "New User") - fill_in("user[email]", with: "thisis'tanemail") - click_button("Continue") - expect(page).to have_selector("#error-summary-title") - expect(page).to have_selector("#user-email-field-error") - expect(page).to have_content(/Enter an email address in the correct format, like name@example.com/) - expect(page).to have_title("Error") - end - - it "sets name, email, role and is_dpo" do - visit("users/new") - fill_in("user[name]", with: "New User") - fill_in("user[email]", with: "newuser@example.com") - choose("user-role-data-provider-field") - choose("user-is-dpo-true-field") - click_button("Continue") - expect( - User.find_by(name: "New User", email: "newuser@example.com", role: "data_provider", is_dpo: true), - ).to be_a(User) - end + context "when signed in as a data coordinator" do + let!(:user) { FactoryBot.create(:user, :data_coordinator, last_sign_in_at: Time.zone.now) } - it "defaults to is_dpo false" do - visit("users/new") - expect(page).to have_field("user[is_dpo]", with: false) + context "when viewing your account" do + before do + visit("/logs") + fill_in("user[email]", with: user.email) + fill_in("user[password]", with: "pAssword1") + click_button("Sign in") + end + + it "shows 'Your account' link in navigation if logged in and redirect to correct page" do + visit("/logs") + expect(page).to have_link("Your account") + click_link("Your account") + expect(page).to have_current_path("/users/#{user.id}") + end + + it "can navigate to change your password page from main account page" do + visit("/users/#{user.id}") + find('[data-qa="change-password"]').click + expect(page).to have_content("Change your password") + fill_in("user[password]", with: "Password123!") + fill_in("user[password_confirmation]", with: "Password123!") + click_button("Update") + expect(page).to have_current_path("/users/#{user.id}") + end + + it "allow user to change name" do + visit("/users/#{user.id}") + find('[data-qa="change-name"]').click + expect(page).to have_content("Change your personal details") + fill_in("user[name]", with: "Test New") + click_button("Save changes") + expect(page).to have_current_path("/users/#{user.id}") + expect(page).to have_content("Test New") + end + + it "has dpo and key contact as editable fields" do + visit("/users/#{user.id}") + expect(page).to have_selector('[data-qa="change-are-you-a-data-protection-officer"]') + expect(page).to have_selector('[data-qa="change-are-you-a-key-contact"]') + end end - end - - context "when editing someone elses account details" do - let!(:user) { FactoryBot.create(:user, :data_coordinator, last_sign_in_at: Time.zone.now) } - let!(:other_user) { FactoryBot.create(:user, name: "Other name", is_dpo: true, organisation: user.organisation) } - before do - visit("/logs") - fill_in("user[email]", with: user.email) - fill_in("user[password]", with: "pAssword1") - click_button("Sign in") + context "when adding a new user" do + before do + visit("/logs") + fill_in("user[email]", with: user.email) + fill_in("user[password]", with: "pAssword1") + click_button("Sign in") + end + + it "validates an email has been provided" do + visit("users/new") + fill_in("user[name]", with: "New User") + click_button("Continue") + expect(page).to have_selector("#error-summary-title") + expect(page).to have_selector("#user-email-field-error") + expect(page).to have_content(/Enter an email address/) + expect(page).to have_title("Error") + end + + it "validates email" do + visit("users/new") + fill_in("user[name]", with: "New User") + fill_in("user[email]", with: "thisis'tanemail") + click_button("Continue") + expect(page).to have_selector("#error-summary-title") + expect(page).to have_selector("#user-email-field-error") + expect(page).to have_content(/Enter an email address in the correct format, like name@example.com/) + expect(page).to have_title("Error") + end + + it "sets name, email, role, is_dpo and is_key_contact fields" do + visit("users/new") + fill_in("user[name]", with: "New User") + fill_in("user[email]", with: "newuser@example.com") + choose("user-role-data-provider-field") + choose("user-is-dpo-true-field") + choose("user-is-key-contact-true-field") + click_button("Continue") + expect(User.find_by( + name: "New User", + email: "newuser@example.com", + role: "data_provider", + is_dpo: true, + is_key_contact: true, + )).to be_a(User) + end + + it "defaults to is_dpo false" do + visit("users/new") + expect(page).to have_field("user[is_dpo]", with: false) + end end - it "allows updating other users details" do - visit("/organisations/#{user.organisation.id}") - click_link("Users") - click_link(other_user.name) - expect(page).to have_title("Other name’s account") - first(:link, "Change").click - expect(page).to have_field("user[is_dpo]", with: true) - choose("user-is-dpo-field") - fill_in("user[name]", with: "Updated new name") - click_button("Save changes") - expect(page).to have_title("Updated new name’s account") - expect(User.find_by(name: "Updated new name", role: "data_provider", is_dpo: false)).to be_a(User) + context "when editing someone elses account details" do + let!(:user) { FactoryBot.create(:user, :data_coordinator, last_sign_in_at: Time.zone.now) } + let!(:other_user) { FactoryBot.create(:user, name: "Other name", is_dpo: true, organisation: user.organisation) } + + before do + visit("/logs") + fill_in("user[email]", with: user.email) + fill_in("user[password]", with: "pAssword1") + click_button("Sign in") + end + + it "allows updating other users details" do + visit("/organisations/#{user.organisation.id}") + click_link("Users") + click_link(other_user.name) + expect(page).to have_title("Other name’s account") + first(:link, "Change").click + expect(page).to have_field("user[is_dpo]", with: true) + choose("user-is-dpo-field") + choose("user-is-key-contact-true-field") + fill_in("user[name]", with: "Updated new name") + click_button("Save changes") + expect(page).to have_title("Updated new name’s account") + expect(User.find_by( + name: "Updated new name", + role: "data_provider", + is_dpo: false, + is_key_contact: true, + )).to be_a(User) + end end end end 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 new file mode 100644 index 000000000..2f6a59ee8 --- /dev/null +++ b/spec/fixtures/softwire_imports/users/b7829b1a5dfb68bb1e01c08445830c0add40907c.xml @@ -0,0 +1,13 @@ + + b7829b1a5dfb68bb1e01c08445830c0add40907c + xxx + John Doe + john.doe + 7c5bd5fb549c09a2c55d7cb90d7ba84927e64618 + john.doe@gov.uk + Private Data Downloader + true + false + None + 02012345678 + diff --git a/spec/fixtures/softwire_imports/users/d4729b1a5dfb68bb1e01c08445830c0add40907c.xml b/spec/fixtures/softwire_imports/users/d4729b1a5dfb68bb1e01c08445830c0add40907c.xml new file mode 100644 index 000000000..3af76d2df --- /dev/null +++ b/spec/fixtures/softwire_imports/users/d4729b1a5dfb68bb1e01c08445830c0add40907c.xml @@ -0,0 +1,13 @@ + + d4729b1a5dfb68bb1e01c08445830c0add40907c + xxx + John Doe + john.doe + 7c5bd5fb549c09a2c55d7cb90d7ba84927e64618 + john.doe@gov.uk + Co-ordinator + true + false + Key Performance Contact + 02012345678 + diff --git a/spec/fixtures/softwire_imports/users/d6717836154cd9a58f9e2f1d3077e3ab81e07613.xml b/spec/fixtures/softwire_imports/users/d6717836154cd9a58f9e2f1d3077e3ab81e07613.xml new file mode 100644 index 000000000..83a8b6a05 --- /dev/null +++ b/spec/fixtures/softwire_imports/users/d6717836154cd9a58f9e2f1d3077e3ab81e07613.xml @@ -0,0 +1,13 @@ + + d6717836154cd9a58f9e2f1d3077e3ab81e07613 + xxx + John Doe + john.doe + 7c5bd5fb549c09a2c55d7cb90d7ba84927e64618 + john.doe@gov.uk + Co-ordinator + true + false + eCORE Contact + 02012345678 + 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/models/user_spec.rb b/spec/models/user_spec.rb index a59cd8fdd..275bb540c 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -47,13 +47,17 @@ RSpec.describe User, type: :model do expect(user.data_coordinator?).to be false end + it "is not a key contact by default" do + expect(user.is_key_contact?).to be false + end + it "is not a data protection officer by default" do expect(user.is_data_protection_officer?).to be false end - it "can be set to data protection officer" do - expect { user.is_data_protection_officer! } - .to change { user.reload.is_data_protection_officer? }.from(false).to(true) + it "can be set to key contact" do + expect { user.is_key_contact! } + .to change { user.reload.is_key_contact? }.from(false).to(true) end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index c71eaf0f8..3c06aef5f 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -210,13 +210,14 @@ RSpec.describe UsersController, type: :request do expect(whodunnit_actor.id).to eq(user.id) end - context "when user changes email and dpo" do - let(:params) { { id: user.id, user: { name: new_name, email: new_email, is_dpo: "true" } } } + context "when user changes email, dpo, key_contact" do + let(:params) { { id: user.id, user: { name: new_name, email: new_email, is_dpo: "true", is_key_contact: "true" } } } - it "allows changing email and dpo" do + it "allows changing email but not dpo or key_contact" do user.reload expect(user.email).to eq(new_email) - expect(user.is_data_protection_officer?).to be true + expect(user.is_data_protection_officer?).to be false + expect(user.is_key_contact?).to be false end end end @@ -265,6 +266,32 @@ RSpec.describe UsersController, type: :request do end end end + + describe "#create" do + let(:params) do + { + "user": { + name: "new user", + email: "new_user@example.com", + role: "data_coordinator", + }, + } + end + let(:request) { post "/users/", headers: headers, params: params } + + before do + sign_in user + end + + it "does not invite a new user" do + expect { request }.not_to change(User, :count) + end + + it "returns 401 unauthorized" do + request + expect(response).to have_http_status(:unauthorized) + end + end end context "when user is signed in as a data coordinator" do @@ -399,12 +426,13 @@ RSpec.describe UsersController, type: :request do end context "when user changes email and dpo" do - let(:params) { { id: user.id, user: { name: new_name, email: new_email, is_dpo: "true" } } } + let(:params) { { id: user.id, user: { name: new_name, email: new_email, is_dpo: "true", is_key_contact: "true" } } } it "allows changing email and dpo" do user.reload expect(user.email).to eq(new_email) expect(user.is_data_protection_officer?).to be true + expect(user.is_key_contact?).to be true end end @@ -443,14 +471,15 @@ RSpec.describe UsersController, type: :request do .to change { other_user.reload.versions.last.actor&.id }.from(nil).to(user.id) end - context "when user changes email and dpo" do - let(:params) { { id: other_user.id, user: { name: new_name, email: new_email, is_dpo: "true" } } } + context "when user changes email, dpo, key_contact" do + let(:params) { { id: other_user.id, user: { name: new_name, email: new_email, is_dpo: "true", is_key_contact: "true" } } } - it "allows changing email and dpo" do + it "allows changing email, dpo, key_contact" do patch "/users/#{other_user.id}", headers: headers, params: params other_user.reload expect(other_user.email).to eq(new_email) expect(other_user.is_data_protection_officer?).to be true + expect(other_user.is_key_contact?).to be true end end @@ -510,42 +539,43 @@ RSpec.describe UsersController, type: :request do end end end - end - - describe "#create" do - let(:params) do - { - "user": { - name: "new user", - email: "new_user@example.com", - role: "data_coordinator", - }, - } - end - let(:request) { post "/users/", headers: headers, params: params } - before do - sign_in user - end - - it "invites a new user" do - expect { request }.to change(User, :count).by(1) - end - - it "redirects back to organisation users page" do - request - expect(response).to redirect_to("/organisations/#{user.organisation.id}/users") - end + describe "#create" do + let(:user) { FactoryBot.create(:user, :data_coordinator) } + let(:params) do + { + "user": { + name: "new user", + email: "new_user@example.com", + role: "data_coordinator", + }, + } + end + let(:request) { post "/users/", headers: headers, params: params } - context "when the email is already taken" do before do - FactoryBot.create(:user, email: "new_user@example.com") + sign_in user end - it "shows an error" do + it "invites a new user" do + expect { request }.to change(User, :count).by(1) + end + + it "redirects back to organisation users page" do request - expect(response).to have_http_status(:unprocessable_entity) - expect(page).to have_content(I18n.t("validations.email.taken")) + expect(response).to redirect_to("/organisations/#{user.organisation.id}/users") + end + + context "when the email is already taken" do + before do + FactoryBot.create(:user, email: "new_user@example.com") + end + + it "shows an error" do + request + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content(I18n.t("validations.email.taken")) + end end end end diff --git a/spec/services/imports/user_import_service_spec.rb b/spec/services/imports/user_import_service_spec.rb index 8401d7149..70552f41a 100644 --- a/spec/services/imports/user_import_service_spec.rb +++ b/spec/services/imports/user_import_service_spec.rb @@ -29,6 +29,7 @@ RSpec.describe Imports::UserImportService do expect(user.phone).to eq("02012345678") expect(user).to be_data_provider expect(user.organisation.old_org_id).to eq(old_org_id) + expect(user.is_key_contact?).to be false end it "refuses to create a user belonging to a non existing organisation" do @@ -36,6 +37,62 @@ RSpec.describe Imports::UserImportService do .to raise_error(ActiveRecord::RecordInvalid, /Organisation must exist/) end + context "when the user is a data coordinator" do + let(:old_user_id) { "d4729b1a5dfb68bb1e01c08445830c0add40907c" } + + it "sets their role correctly" do + FactoryBot.create(:organisation, old_org_id:) + import_service.create_users("user_directory") + expect(User.find_by(old_user_id:)).to be_data_coordinator + end + end + + context "when the user is a data accessor" do + let(:old_user_id) { "b7829b1a5dfb68bb1e01c08445830c0add40907c" } + + it "sets their role correctly" do + FactoryBot.create(:organisation, old_org_id:) + import_service.create_users("user_directory") + expect(User.find_by(old_user_id:)).to be_data_accessor + 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" } + + it "marks them as a key contact" do + FactoryBot.create(:organisation, old_org_id:) + import_service.create_users("user_directory") + + user = User.find_by(old_user_id:) + expect(user.is_key_contact?).to be true + end + end + + context "when the user was a 'eCORE Contact' in the old system" do + let(:old_user_id) { "d6717836154cd9a58f9e2f1d3077e3ab81e07613" } + + it "marks them as a key contact" do + FactoryBot.create(:organisation, old_org_id:) + import_service.create_users("user_directory") + + user = User.find_by(old_user_id:) + expect(user.is_key_contact?).to be true + end + end + context "when the user has already been imported previously" do before do org = FactoryBot.create(:organisation, old_org_id:) @@ -47,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