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