<%= f.govuk_error_summary %>
@@ -26,6 +26,14 @@
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" }
+ %>
+
<%= f.govuk_submit "Save changes" %>
diff --git a/app/views/users/new.html.erb b/app/views/users/new.html.erb
index 53e64eafa..8b89d0c47 100644
--- a/app/views/users/new.html.erb
+++ b/app/views/users/new.html.erb
@@ -31,6 +31,14 @@
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" }
+ %>
+
<%= f.govuk_submit "Continue" %>
diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb
index 7472d46f8..66c0a9145 100644
--- a/app/views/users/show.html.erb
+++ b/app/views/users/show.html.erb
@@ -1,4 +1,4 @@
-<% content_for :title, "Your account" %>
+<% content_for :title, current_user == @user ? "Your account" : "#{@user.name}’s account" %>
@@ -13,33 +13,43 @@
<%= govuk_summary_list do |summary_list| %>
<%= summary_list.row do |row|
row.key { 'Name' }
- row.value { current_user.name }
+ row.value { @user.name }
row.action(visually_hidden_text: 'name', href: edit_user_path, html_attributes: { 'data-qa': 'change-name' })
end %>
<%= summary_list.row() do |row|
row.key { 'Email address' }
- row.value { current_user.email }
+ row.value { @user.email }
row.action(visually_hidden_text: 'email address', href: edit_user_path, html_attributes: { 'data-qa': 'change-email' })
end %>
<%= summary_list.row do |row|
row.key { 'Password' }
row.value { '••••••••' }
- row.action(visually_hidden_text: 'password', href: password_edit_user_path, html_attributes: { 'data-qa': 'change-password' })
+ if current_user == @user
+ row.action(visually_hidden_text: 'password', href: password_edit_user_path, html_attributes: { 'data-qa': 'change-password' })
+ else
+ row.action()
+ end
end %>
<%= summary_list.row do |row|
row.key { 'Organisation' }
- row.value { current_user.organisation.name }
+ row.value { @user.organisation.name }
row.action()
end %>
<%= summary_list.row do |row|
row.key { 'Role' }
- row.value { current_user.role.humanize }
+ row.value { @user.role.humanize }
row.action()
end %>
+
+ <%= 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" })
+ end %>
<% end %>
diff --git a/db/migrate/20220328105332_change_dpo_to_attribute.rb b/db/migrate/20220328105332_change_dpo_to_attribute.rb
new file mode 100644
index 000000000..6461fd87c
--- /dev/null
+++ b/db/migrate/20220328105332_change_dpo_to_attribute.rb
@@ -0,0 +1,7 @@
+class ChangeDpoToAttribute < ActiveRecord::Migration[7.0]
+ def change
+ change_table :users, bulk: true do |t|
+ t.column :is_dpo, :boolean, default: false
+ end
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 1468b9dc9..c5592dc25 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -315,6 +315,7 @@ ActiveRecord::Schema[7.0].define(version: 202202071123100) do
t.integer "failed_attempts", default: 0
t.string "unlock_token"
t.datetime "locked_at", precision: nil
+ t.boolean "is_dpo", 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/factories/user.rb b/spec/factories/user.rb
index b3dc82583..0de22af97 100644
--- a/spec/factories/user.rb
+++ b/spec/factories/user.rb
@@ -9,7 +9,7 @@ FactoryBot.define do
role { "data_coordinator" }
end
trait :data_protection_officer do
- role { "data_protection_officer" }
+ is_dpo { true }
end
created_at { Time.zone.now }
updated_at { Time.zone.now }
diff --git a/spec/features/user_spec.rb b/spec/features/user_spec.rb
index 5f68bcc62..b30f8a855 100644
--- a/spec/features/user_spec.rb
+++ b/spec/features/user_spec.rb
@@ -236,5 +236,48 @@ RSpec.describe "User Features" do
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
+
+ it "defaults to is_dpo false" do
+ visit("users/new")
+ expect(page).to have_field("user[is_dpo]", with: false)
+ 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")
+ 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)
+ end
end
end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 934ab57a1..a59cd8fdd 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -46,6 +46,15 @@ RSpec.describe User, type: :model do
expect(user.data_provider?).to be true
expect(user.data_coordinator?).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)
+ end
end
describe "paper trail" do
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index fb4623e28..c71eaf0f8 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@@ -2,11 +2,12 @@ require "rails_helper"
RSpec.describe UsersController, type: :request do
let(:user) { FactoryBot.create(:user) }
- let(:unauthorised_user) { FactoryBot.create(:user) }
+ let(:other_user) { FactoryBot.create(:user) }
let(:headers) { { "Accept" => "text/html" } }
let(:page) { Capybara::Node::Simple.new(response.body) }
- let(:new_value) { "new test name" }
- let(:params) { { id: user.id, user: { name: new_value } } }
+ let(:new_name) { "new test name" }
+ let(:new_email) { "new@example.com" }
+ let(:params) { { id: user.id, user: { name: new_name } } }
let(:notify_client) { instance_double(Notifications::Client) }
let(:devise_notify_mailer) { DeviseNotifyMailer.new }
@@ -56,7 +57,7 @@ RSpec.describe UsersController, type: :request do
context "when the reset token is valid" do
let(:params) do
{
- id: user.id, user: { password: new_value, password_confirmation: "something_else" }
+ id: user.id, user: { password: new_name, password_confirmation: "something_else" }
}
end
@@ -78,8 +79,8 @@ RSpec.describe UsersController, type: :request do
{
id: user.id,
user: {
- password: new_value,
- password_confirmation: new_value,
+ password: new_name,
+ password_confirmation: new_name,
reset_password_token: raw,
},
}
@@ -109,147 +110,404 @@ RSpec.describe UsersController, type: :request do
end
end
- describe "#show" do
- context "when the current user matches the user ID" do
- before do
- sign_in user
- get "/users/#{user.id}", headers: headers, params: {}
- end
+ context "when user is signed in as a data provider" do
+ describe "#show" do
+ context "when the current user matches the user ID" do
+ before do
+ sign_in user
+ get "/users/#{user.id}", headers: headers, params: {}
+ end
- it "show the user details" do
- expect(page).to have_content("Your account")
+ it "show the user details" do
+ expect(page).to have_content("Your account")
+ end
end
- end
- context "when the current user does not matches the user ID" do
- before do
- sign_in user
- get "/users/#{unauthorised_user.id}", headers: headers, params: {}
- end
+ context "when the current user does not matches the user ID" do
+ before do
+ sign_in user
+ get "/users/#{other_user.id}", headers: headers, params: {}
+ end
- it "returns not found 404" do
- expect(response).to have_http_status(:not_found)
- end
+ it "returns not found 404" do
+ expect(response).to have_http_status(:not_found)
+ end
- it "shows the 404 view" do
- expect(page).to have_content("Page not found")
+ it "shows the 404 view" do
+ expect(page).to have_content("Page not found")
+ end
end
end
- end
- describe "#edit" do
- context "when the current user matches the user ID" do
- before do
- sign_in user
- get "/users/#{user.id}/edit", headers: headers, params: {}
+ describe "#edit" do
+ context "when the current user matches the user ID" do
+ before do
+ sign_in user
+ get "/users/#{user.id}/edit", headers: headers, params: {}
+ end
+
+ it "show the edit personal details page" do
+ expect(page).to have_content("Change your personal details")
+ end
end
- it "show the edit personal details page" do
- expect(page).to have_content("Change your personal details")
+ context "when the current user does not matches the user ID" do
+ before do
+ sign_in user
+ get "/users/#{other_user.id}/edit", headers: headers, params: {}
+ end
+
+ it "returns not found 404" do
+ expect(response).to have_http_status(:not_found)
+ end
end
end
- context "when the current user does not matches the user ID" do
- before do
- sign_in user
- get "/users/#{unauthorised_user.id}/edit", headers: headers, params: {}
+ describe "#edit_password" do
+ context "when the current user matches the user ID" do
+ before do
+ sign_in user
+ get "/users/#{user.id}/password/edit", headers: headers, params: {}
+ end
+
+ it "shows the edit password page" do
+ expect(page).to have_content("Change your password")
+ end
+
+ it "shows the password requirements hint" do
+ expect(page).to have_css("#user-password-hint")
+ end
end
- it "returns not found 404" do
- expect(response).to have_http_status(:not_found)
+ context "when the current user does not matches the user ID" do
+ before do
+ sign_in user
+ get "/users/#{other_user.id}/edit", headers: headers, params: {}
+ end
+
+ it "returns not found 404" do
+ expect(response).to have_http_status(:not_found)
+ end
end
end
- end
- describe "#edit_password" do
- context "when the current user matches the user ID" do
- before do
- sign_in user
- get "/users/#{user.id}/password/edit", headers: headers, params: {}
- end
+ describe "#update" do
+ context "when the current user matches the user ID" do
+ before do
+ sign_in user
+ patch "/users/#{user.id}", headers: headers, params: params
+ end
- it "shows the edit password page" do
- expect(page).to have_content("Change your password")
+ it "updates the user" do
+ user.reload
+ expect(user.name).to eq(new_name)
+ end
+
+ it "tracks who updated the record" do
+ user.reload
+ whodunnit_actor = user.versions.last.actor
+ expect(whodunnit_actor).to be_a(User)
+ 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" } } }
+
+ 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
+ end
+ end
end
- it "shows the password requirements hint" do
- expect(page).to have_css("#user-password-hint")
+ context "when the update fails to persist" do
+ before do
+ sign_in user
+ allow(User).to receive(:find_by).and_return(user)
+ allow(user).to receive(:update).and_return(false)
+ patch "/users/#{user.id}", headers: headers, params: params
+ end
+
+ it "show an error" do
+ expect(response).to have_http_status(:unprocessable_entity)
+ end
end
- end
- context "when the current user does not matches the user ID" do
- before do
- sign_in user
- get "/users/#{unauthorised_user.id}/edit", headers: headers, params: {}
+ context "when the current user does not matches the user ID" do
+ let(:params) { { id: other_user.id, user: { name: new_name } } }
+
+ before do
+ sign_in user
+ patch "/users/#{other_user.id}", headers: headers, params: params
+ end
+
+ it "returns not found 404" do
+ expect(response).to have_http_status(:not_found)
+ end
end
- it "returns not found 404" do
- expect(response).to have_http_status(:not_found)
+ context "when we update the user password" do
+ let(:params) do
+ {
+ id: user.id, user: { password: new_name, password_confirmation: "something_else" }
+ }
+ end
+
+ before do
+ sign_in user
+ patch "/users/#{user.id}", headers: headers, params: params
+ end
+
+ it "shows an error if passwords don't match" do
+ expect(response).to have_http_status(:unprocessable_entity)
+ expect(page).to have_selector("#error-summary-title")
+ end
end
end
end
- describe "#update" do
- context "when the current user matches the user ID" do
- before do
- sign_in user
- patch "/users/#{user.id}", headers: headers, params: params
- end
+ context "when user is signed in as a data coordinator" do
+ let(:user) { FactoryBot.create(:user, :data_coordinator) }
+ let(:other_user) { FactoryBot.create(:user, organisation: user.organisation) }
- it "updates the user" do
- user.reload
- expect(user.name).to eq(new_value)
+ describe "#show" do
+ context "when the current user matches the user ID" do
+ before do
+ sign_in user
+ get "/users/#{user.id}", headers: headers, params: {}
+ end
+
+ it "show the user details" do
+ expect(page).to have_content("Your account")
+ end
end
- it "tracks who updated the record" do
- user.reload
- whodunnit_actor = user.versions.last.actor
- expect(whodunnit_actor).to be_a(User)
- expect(whodunnit_actor.id).to eq(user.id)
+ context "when the current user does not matches the user ID" do
+ before do
+ sign_in user
+ get "/users/#{other_user.id}", headers: headers, params: {}
+ end
+
+ context "when the user is part of the same organisation as the current user" do
+ it "returns 200" do
+ expect(response).to have_http_status(:ok)
+ end
+
+ it "shows the user details page" do
+ expect(page).to have_content("#{other_user.name}’s account")
+ end
+ end
+
+ context "when the user is not part of the same organisation as the current user" do
+ let(:other_user) { FactoryBot.create(:user) }
+
+ it "returns not found 404" do
+ expect(response).to have_http_status(:not_found)
+ end
+
+ it "shows the 404 view" do
+ expect(page).to have_content("Page not found")
+ end
+ end
end
end
- context "when the update fails to persist" do
- before do
- sign_in user
- allow(User).to receive(:find_by).and_return(user)
- allow(user).to receive(:update).and_return(false)
- patch "/users/#{user.id}", headers: headers, params: params
+ describe "#edit" do
+ context "when the current user matches the user ID" do
+ before do
+ sign_in user
+ get "/users/#{user.id}/edit", headers: headers, params: {}
+ end
+
+ it "show the edit personal details page" do
+ expect(page).to have_content("Change your personal details")
+ end
end
- it "show an error" do
- expect(response).to have_http_status(:unprocessable_entity)
+ context "when the current user does not matches the user ID" do
+ before do
+ sign_in user
+ get "/users/#{other_user.id}/edit", headers: headers, params: {}
+ end
+
+ context "when the user is part of the same organisation as the current user" do
+ it "returns 200" do
+ expect(response).to have_http_status(:ok)
+ end
+
+ it "shows the user details page" do
+ expect(page).to have_content("Change #{other_user.name}’s personal details")
+ end
+ end
+
+ context "when the user is not part of the same organisation as the current user" do
+ let(:other_user) { FactoryBot.create(:user) }
+
+ it "returns not found 404" do
+ expect(response).to have_http_status(:not_found)
+ end
+ end
end
end
- context "when the current user does not matches the user ID" do
- let(:params) { { id: unauthorised_user.id, user: { name: new_value } } }
+ describe "#edit_password" do
+ context "when the current user matches the user ID" do
+ before do
+ sign_in user
+ get "/users/#{user.id}/password/edit", headers: headers, params: {}
+ end
- before do
- sign_in user
- patch "/users/#{unauthorised_user.id}", headers: headers, params: params
+ it "shows the edit password page" do
+ expect(page).to have_content("Change your password")
+ end
+
+ it "shows the password requirements hint" do
+ expect(page).to have_css("#user-password-hint")
+ end
end
- it "returns not found 404" do
- expect(response).to have_http_status(:not_found)
+ context "when the current user does not matches the user ID" do
+ before do
+ sign_in user
+ get "/users/#{other_user.id}/password/edit", headers: headers, params: {}
+ end
+
+ it "returns not found 404" do
+ expect(response).to have_http_status(:not_found)
+ end
end
end
- context "when we update the user password" do
- let(:params) do
- {
- id: user.id, user: { password: new_value, password_confirmation: "something_else" }
- }
+ describe "#update" do
+ context "when the current user matches the user ID" do
+ before do
+ sign_in user
+ patch "/users/#{user.id}", headers: headers, params: params
+ end
+
+ it "updates the user" do
+ user.reload
+ expect(user.name).to eq(new_name)
+ end
+
+ it "tracks who updated the record" do
+ user.reload
+ whodunnit_actor = user.versions.last.actor
+ expect(whodunnit_actor).to be_a(User)
+ 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" } } }
+
+ 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
+ end
+ end
+
+ context "when we update the user password" do
+ let(:params) do
+ {
+ id: user.id, user: { password: new_name, password_confirmation: "something_else" }
+ }
+ end
+
+ before do
+ sign_in user
+ patch "/users/#{user.id}", headers: headers, params: params
+ end
+
+ it "shows an error if passwords don't match" do
+ expect(response).to have_http_status(:unprocessable_entity)
+ expect(page).to have_selector("#error-summary-title")
+ end
+ end
end
- before do
- sign_in user
- patch "/users/#{user.id}", headers: headers, params: params
+ context "when the current user does not matches the user ID" do
+ before do
+ sign_in user
+ end
+
+ context "when the user is part of the same organisation as the current user" do
+ it "updates the user" do
+ expect { patch "/users/#{other_user.id}", headers: headers, params: params }
+ .to change { other_user.reload.name }.from(other_user.name).to(new_name)
+ end
+
+ it "tracks who updated the record" do
+ expect { patch "/users/#{other_user.id}", headers: headers, params: params }
+ .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" } } }
+
+ it "allows changing email and dpo" 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
+ end
+ end
+
+ it "does not bypass sign in for the coordinator" do
+ patch "/users/#{other_user.id}", headers: headers, params: params
+ follow_redirect!
+ expect(page).to have_content("#{other_user.reload.name}’s account")
+ expect(page).to have_content(other_user.reload.email.to_s)
+ end
+
+ context "when we try to update the user password" do
+ let(:params) do
+ {
+ id: user.id, user: { password: new_name, password_confirmation: new_name, name: "new name" }
+ }
+ end
+
+ it "does not update the password" do
+ expect { patch "/users/#{other_user.id}", headers: headers, params: params }
+ .not_to change(other_user, :encrypted_password)
+ end
+
+ it "does update other values" do
+ expect { patch "/users/#{other_user.id}", headers: headers, params: params }
+ .to change { other_user.reload.name }.from("Danny Rojas").to("new name")
+ end
+ end
+ end
+
+ context "when the current user does not matches the user ID" do
+ context "when the user is not part of the same organisation as the current user" do
+ let(:other_user) { FactoryBot.create(:user) }
+ let(:params) { { id: other_user.id, user: { name: new_name } } }
+
+ before do
+ sign_in user
+ patch "/users/#{other_user.id}", headers: headers, params: params
+ end
+
+ it "returns not found 404" do
+ expect(response).to have_http_status(:not_found)
+ end
+ end
+ end
end
- it "shows an error if passwords don't match" do
- expect(response).to have_http_status(:unprocessable_entity)
- expect(page).to have_selector("#error-summary-title")
+ context "when the update fails to persist" do
+ before do
+ sign_in user
+ allow(User).to receive(:find_by).and_return(user)
+ allow(user).to receive(:update).and_return(false)
+ patch "/users/#{user.id}", headers: headers, params: params
+ end
+
+ it "show an error" do
+ expect(response).to have_http_status(:unprocessable_entity)
+ end
end
end
end
diff --git a/spec/services/imports/data_protection_confirmation_import_service_spec.rb b/spec/services/imports/data_protection_confirmation_import_service_spec.rb
index fd513138c..eddfca6a2 100644
--- a/spec/services/imports/data_protection_confirmation_import_service_spec.rb
+++ b/spec/services/imports/data_protection_confirmation_import_service_spec.rb
@@ -34,7 +34,7 @@ RSpec.describe Imports::DataProtectionConfirmationImportService do
it "creates a data protection officer without sign in credentials" do
expect { import_service.create_data_protection_confirmations("data_protection_directory") }
.to change(User, :count).by(1)
- data_protection_officer = User.find_by(organisation:, role: "data_protection_officer")
+ data_protection_officer = User.find_by(organisation:, is_dpo: true)
expect(data_protection_officer.email).to eq("")
end