diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index f36d9d311..c4062f6d5 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -7,8 +7,10 @@ class UsersController < ApplicationController def update if @user.update(user_params) - bypass_sign_in @user - flash[:notice] = I18n.t("devise.passwords.updated") if user_params.key?("password") + if @user == current_user + bypass_sign_in @user + flash[:notice] = I18n.t("devise.passwords.updated") if user_params.key?("password") + end redirect_to user_path(@user) elsif user_params.key?("password") format_error_messages @@ -74,9 +76,9 @@ private def user_params if @user == current_user - params.require(:user).permit(:email, :name, :password, :password_confirmation, :role) + params.require(:user).permit(:email, :name, :password, :password_confirmation, :role, :is_dpo) else - params.require(:user).permit(:email, :name, :role) + params.require(:user).permit(:email, :name, :role, :is_dpo) end end diff --git a/app/views/users/edit.html.erb b/app/views/users/edit.html.erb index a214c3242..1c26021f8 100644 --- a/app/views/users/edit.html.erb +++ b/app/views/users/edit.html.erb @@ -1,4 +1,4 @@ -<% content_for :title, current_user == @user ? "Change your personal details" : "Change #{@user.name}'s personal details" %> +<% content_for :title, current_user == @user ? "Change your personal details" : "Change #{@user.name}’s personal details" %> <% content_for :before_content do %> <%= govuk_back_link( @@ -7,7 +7,7 @@ ) %> <% end %> -<%= form_for(current_user, as: :user, html: { method: :patch }) do |f| %> +<%= form_for(@user, as: :user, html: { method: :patch }) do |f| %>
<%= f.govuk_error_summary %> @@ -26,6 +26,10 @@ spellcheck: "false" %> + <%= f.govuk_check_boxes_fieldset :is_dpo, multiple: false, legend: { text: "Are they a data protection officer?", size: "m" } do %> + <%= f.govuk_check_box :is_dpo, 1, 0, multiple: false, link_errors: true, label: { text: "Yes" } %> + <% end %> + <%= f.govuk_submit "Save changes" %>
diff --git a/app/views/users/new.html.erb b/app/views/users/new.html.erb index 53e64eafa..4cc66da03 100644 --- a/app/views/users/new.html.erb +++ b/app/views/users/new.html.erb @@ -31,6 +31,10 @@ f.govuk_collection_radio_buttons :role, roles, :id, :name, legend: { text: "Role", size: "m" } %> + <%= f.govuk_check_boxes_fieldset :is_dpo, legend: { text: "Are they a data protection officer?", size: "m" } do %> + <%= f.govuk_check_box :is_dpo, :is_dpo, label: { text: "Yes" } %> + <% end %> + <%= f.govuk_submit "Continue" %> diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index 545dca1af..96c7f06d5 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -1,4 +1,4 @@ -<% content_for :title, current_user == @user ? "Your account" : "#{@user.name}'s 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 they a data protection officer?', href: edit_user_path, html_attributes: { 'data-qa': 'change-are-they-a-data-protection-officer' }) + end %> <% end %>
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 0e04b9352..b517186e9 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -5,8 +5,9 @@ RSpec.describe UsersController, type: :request do 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, }, } @@ -199,7 +200,7 @@ RSpec.describe UsersController, type: :request do it "updates the user" do user.reload - expect(user.name).to eq(new_value) + expect(user.name).to eq(new_name) end it "tracks who updated the record" do @@ -208,6 +209,16 @@ RSpec.describe UsersController, type: :request do 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: "1" } } } + + 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 context "when the update fails to persist" do @@ -224,7 +235,7 @@ RSpec.describe UsersController, type: :request do end context "when the current user does not matches the user ID" do - let(:params) { { id: other_user.id, user: { name: new_value } } } + let(:params) { { id: other_user.id, user: { name: new_name } } } before do sign_in user @@ -239,7 +250,7 @@ RSpec.describe UsersController, type: :request do context "when we update the user password" 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 @@ -284,7 +295,7 @@ RSpec.describe UsersController, type: :request do end it "shows the user details page" do - expect(page).to have_content("#{other_user.name}'s account") + expect(page).to have_content("#{other_user.name}’s account") end end @@ -326,7 +337,7 @@ RSpec.describe UsersController, type: :request do end it "shows the user details page" do - expect(page).to have_content("Change #{other_user.name}'s personal details") + expect(page).to have_content("Change #{other_user.name}’s personal details") end end @@ -377,7 +388,7 @@ RSpec.describe UsersController, type: :request do it "updates the user" do user.reload - expect(user.name).to eq(new_value) + expect(user.name).to eq(new_name) end it "tracks who updated the record" do @@ -387,10 +398,20 @@ 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: "1" } } } + + 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_value, password_confirmation: "something_else" } + id: user.id, user: { password: new_name, password_confirmation: "something_else" } } end @@ -414,7 +435,7 @@ RSpec.describe UsersController, type: :request do 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_value) + .to change { other_user.reload.name }.from(other_user.name).to(new_name) end it "tracks who updated the record" do @@ -422,10 +443,28 @@ 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: "1" } } } + + 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_value, password_confirmation: new_value, name: "new name" } + id: user.id, user: { password: new_name, password_confirmation: new_name, name: "new name" } } end @@ -444,7 +483,7 @@ RSpec.describe UsersController, type: :request do 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_value } } } + let(:params) { { id: other_user.id, user: { name: new_name } } } before do sign_in user