diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 9e6491554..3f91fa659 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -120,6 +120,14 @@ private if user_params[:role].present? && !current_user.assignable_roles.key?(user_params[:role].to_sym) @resource.errors.add :role, I18n.t("validations.role.invalid") end + + if user_params[:phone].present? && !valid_phone_number?(user_params[:phone]) + @resource.errors.add :phone + end + end + + def valid_phone_number?(number) + number.to_i.to_s == number && number.length >= 11 end def format_error_messages @@ -151,14 +159,14 @@ private def user_params if @user == current_user if current_user.data_coordinator? || current_user.support? - params.require(:user).permit(:email, :name, :password, :password_confirmation, :role, :is_dpo, :is_key_contact, :initial_confirmation_sent) + params.require(:user).permit(:email, :phone, :name, :password, :password_confirmation, :role, :is_dpo, :is_key_contact, :initial_confirmation_sent) else - params.require(:user).permit(:email, :name, :password, :password_confirmation, :initial_confirmation_sent) + params.require(:user).permit(:email, :phone, :name, :password, :password_confirmation, :initial_confirmation_sent) end elsif current_user.data_coordinator? - params.require(:user).permit(:email, :name, :role, :is_dpo, :is_key_contact, :active, :initial_confirmation_sent) + params.require(:user).permit(:email, :phone, :name, :role, :is_dpo, :is_key_contact, :active, :initial_confirmation_sent) elsif current_user.support? - params.require(:user).permit(:email, :name, :role, :is_dpo, :is_key_contact, :organisation_id, :active, :initial_confirmation_sent) + params.require(:user).permit(:email, :phone, :name, :role, :is_dpo, :is_key_contact, :organisation_id, :active, :initial_confirmation_sent) end end diff --git a/app/helpers/user_helper.rb b/app/helpers/user_helper.rb index 957e2dc2c..76fb78f57 100644 --- a/app/helpers/user_helper.rb +++ b/app/helpers/user_helper.rb @@ -7,30 +7,6 @@ module UserHelper current_user == user ? "Are you" : "Is this person" end - def can_edit_names?(user, current_user) - (current_user == user || current_user.data_coordinator? || current_user.support?) && user.active? - end - - def can_edit_emails?(user, current_user) - (current_user == user || current_user.data_coordinator? || current_user.support?) && user.active? - end - - def can_edit_password?(user, current_user) - current_user == user - end - - def can_edit_roles?(user, current_user) - (current_user.data_coordinator? || current_user.support?) && user.active? - end - - def can_edit_dpo?(user, current_user) - (current_user.data_coordinator? || current_user.support?) && user.active? - end - - def can_edit_key_contact?(user, current_user) - (current_user.data_coordinator? || current_user.support?) && user.active? - end - def can_edit_org?(current_user) current_user.data_coordinator? || current_user.support? end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb new file mode 100644 index 000000000..a4b1a3d5c --- /dev/null +++ b/app/policies/user_policy.rb @@ -0,0 +1,36 @@ +class UserPolicy + attr_reader :current_user, :user + + def initialize(current_user, user) + @current_user = current_user + @user = user + end + + def edit_password? + @current_user == @user + end + + def edit_roles? + (@current_user.data_coordinator? || @current_user.support?) && @user.active? + end + + %w[ + edit_roles? + edit_dpo? + edit_key_contact? + ].each do |method_name| + define_method method_name do + (@current_user.data_coordinator? || @current_user.support?) && @user.active? + end + end + + %w[ + edit_emails? + edit_telephone_numbers? + edit_names? + ].each do |method_name| + define_method method_name do + (@current_user == @user || @current_user.data_coordinator? || @current_user.support?) && @user.active? + end + end +end diff --git a/app/views/users/edit.html.erb b/app/views/users/edit.html.erb index fb7b10755..3f7259bc3 100644 --- a/app/views/users/edit.html.erb +++ b/app/views/users/edit.html.erb @@ -22,6 +22,11 @@ autocomplete: "email", spellcheck: "false" %> + <%= f.govuk_phone_field :phone, + label: { text: "Telephone number", size: "m" }, + autocomplete: "phone", + spellcheck: "false" %> + <% if current_user.data_coordinator? || current_user.support? %> <% roles = current_user.assignable_roles.map { |key, _| OpenStruct.new(id: key, name: key.to_s.humanize) } %> diff --git a/app/views/users/new.html.erb b/app/views/users/new.html.erb index b3311bcfb..fc66bcad8 100644 --- a/app/views/users/new.html.erb +++ b/app/views/users/new.html.erb @@ -23,6 +23,12 @@ spellcheck: "false", value: @resource.email %> + <%= f.govuk_phone_field :phone, + label: { text: "Telephone number", size: "m" }, + autocomplete: "phone", + spellcheck: "false", + value: @resource.phone %> + <% if current_user.support? %> <% null_option = [OpenStruct.new(id: "", name: "Select an option")] %> <% organisations = Organisation.all.map { |org| OpenStruct.new(id: org.id, name: org.name) } %> diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index 845e6ebba..844a0c965 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -13,7 +13,7 @@ <%= summary_list.row do |row| row.key { "Name" } row.value { @user.name } - if can_edit_names?(@user, current_user) + if UserPolicy.new(current_user, @user).edit_names? row.action(visually_hidden_text: "name", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-name" }) else row.action @@ -23,17 +23,27 @@ <%= summary_list.row do |row| row.key { "Email address" } row.value { @user.email } - if can_edit_emails?(@user, current_user) + if UserPolicy.new(current_user, @user).edit_emails? row.action(visually_hidden_text: "email address", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-email-address" }) else row.action end end %> + <%= summary_list.row do |row| + row.key { "Telephone number" } + row.value { @user.phone } + if UserPolicy.new(current_user, @user).edit_telephone_numbers? + row.action(visually_hidden_text: "telephone number", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-telephone-number" }) + else + row.action + end + end %> + <%= summary_list.row do |row| row.key { "Password" } row.value { "••••••••" } - if can_edit_password?(@user, current_user) + if UserPolicy.new(current_user, @user).edit_password? row.action( visually_hidden_text: "password", href: edit_password_account_path, @@ -53,7 +63,7 @@ <%= summary_list.row do |row| row.key { "Role" } row.value { @user.role&.humanize } - if can_edit_roles?(@user, current_user) + if UserPolicy.new(current_user, @user).edit_roles? row.action( visually_hidden_text: "role", href: aliased_user_edit(@user, current_user), @@ -67,7 +77,7 @@ <%= summary_list.row do |row| row.key { "Data protection officer" } row.value { @user.is_data_protection_officer? ? "Yes" : "No" } - if can_edit_dpo?(@user, current_user) + if UserPolicy.new(current_user, @user).edit_dpo? row.action( visually_hidden_text: "if data protection officer", href: user_edit_dpo_path(@user), @@ -81,7 +91,7 @@ <%= summary_list.row do |row| row.key { "Key contact" } row.value { @user.is_key_contact? ? "Yes" : "No" } - if can_edit_key_contact?(@user, current_user) + if UserPolicy.new(current_user, @user).edit_key_contact? row.action( visually_hidden_text: "if a key contact", href: user_edit_key_contact_path(@user), diff --git a/config/locales/en.yml b/config/locales/en.yml index 59fa8ef32..73cf91d0a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -154,6 +154,8 @@ en: invalid: "Enter an email address in the correct format, like name@example.com" blank: "Enter an email address" taken: "Enter an email address that hasn’t already been used to sign up" + phone: + invalid: "Enter a telephone number in the correct format" role: invalid: "Role must be data accessor, data provider or data coordinator" blank: "Select role" diff --git a/spec/features/user_spec.rb b/spec/features/user_spec.rb index d4e1a6052..222d2ff04 100644 --- a/spec/features/user_spec.rb +++ b/spec/features/user_spec.rb @@ -322,16 +322,42 @@ RSpec.describe "User Features" do expect(page).to have_title("Error") end + it "validates telephone number is numeric" do + visit("users/new") + fill_in("user[name]", with: "New User") + fill_in("user[email]", with: "newuser@example.com") + fill_in("user[phone]", with: "randomstring") + click_button("Continue") + expect(page).to have_selector("#error-summary-title") + expect(page).to have_selector("#user-phone-field-error") + expect(page).to have_content(/Enter a telephone number in the correct format/) + expect(page).to have_title("Error") + end + + it "validates telephone number is longer than 11 digits" do + visit("users/new") + fill_in("user[name]", with: "New User") + fill_in("user[email]", with: "newuser@example.com") + fill_in("user[phone]", with: "123") + click_button("Continue") + expect(page).to have_selector("#error-summary-title") + expect(page).to have_selector("#user-phone-field-error") + expect(page).to have_content(/Enter a telephone number in the correct format/) + 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") + fill_in("user[phone]", with: "12345678910") choose("user-role-data-provider-field") click_button("Continue") expect(User.find_by( name: "New User", email: "newuser@example.com", role: "data_provider", + phone: "12345678910", is_dpo: false, is_key_contact: false, )).to be_a(User) diff --git a/spec/helpers/user_helper_spec.rb b/spec/helpers/user_helper_spec.rb index e49c7d6a2..829195f6c 100644 --- a/spec/helpers/user_helper_spec.rb +++ b/spec/helpers/user_helper_spec.rb @@ -37,105 +37,6 @@ RSpec.describe UserHelper do end describe "change button permissions" do - context "when the user is a data provider viewing their own details" do - let(:current_user) { FactoryBot.create(:user, :data_provider) } - let(:user) { current_user } - - it "allows changing name" do - expect(can_edit_names?(user, current_user)).to be true - end - - it "allows changing email" do - expect(can_edit_emails?(user, current_user)).to be true - end - - it "allows changing password" do - expect(can_edit_password?(user, current_user)).to be true - end - - it "does not allow changing roles" do - expect(can_edit_roles?(user, current_user)).to be false - end - - it "does not allow changing dpo" do - expect(can_edit_dpo?(user, current_user)).to be false - end - - it "does not allow changing key contact" do - expect(can_edit_key_contact?(user, current_user)).to be false - end - end - - context "when the user is a data coordinator viewing another user's details" do - it "allows changing name" do - expect(can_edit_names?(user, current_user)).to be true - end - - it "allows changing email" do - expect(can_edit_emails?(user, current_user)).to be true - end - - it "allows changing password" do - expect(can_edit_password?(user, current_user)).to be false - end - - it "does not allow changing roles" do - expect(can_edit_roles?(user, current_user)).to be true - end - - it "does not allow changing dpo" do - expect(can_edit_dpo?(user, current_user)).to be true - end - - it "does not allow changing key contact" do - expect(can_edit_key_contact?(user, current_user)).to be true - end - - context "when the user is a data coordinator viewing their own details" do - let(:user) { current_user } - - it "allows changing password" do - expect(can_edit_password?(user, current_user)).to be true - end - end - end - - context "when the user is a support user viewing another user's details" do - let(:current_user) { FactoryBot.create(:user, :support) } - - it "allows changing name" do - expect(can_edit_names?(user, current_user)).to be true - end - - it "allows changing email" do - expect(can_edit_emails?(user, current_user)).to be true - end - - it "allows changing password" do - expect(can_edit_password?(user, current_user)).to be false - end - - it "does not allow changing roles" do - expect(can_edit_roles?(user, current_user)).to be true - end - - it "does not allow changing dpo" do - expect(can_edit_dpo?(user, current_user)).to be true - end - - it "does not allow changing key contact" do - expect(can_edit_key_contact?(user, current_user)).to be true - end - - context "when the user is a support user viewing their own details" do - let(:user) { current_user } - - it "allows changing password" do - expect(can_edit_password?(user, current_user)).to be true - end - end - end - context "when the user is a data provider viewing organisation details" do let(:current_user) { FactoryBot.create(:user, :data_provider) } diff --git a/spec/policies/user_policy_spec.rb b/spec/policies/user_policy_spec.rb new file mode 100644 index 000000000..ec84fbceb --- /dev/null +++ b/spec/policies/user_policy_spec.rb @@ -0,0 +1,103 @@ +require "rails_helper" +# rubocop:disable RSpec/RepeatedExample + +RSpec.describe UserPolicy do + subject(:policy) { described_class } + + let(:data_provider) { FactoryBot.create(:user, :data_provider) } + let(:data_coordinator) { FactoryBot.create(:user, :data_coordinator) } + let(:support) { FactoryBot.create(:user, :support) } + + permissions :edit_names? do + it "allows changing their own name" do + expect(policy).to permit(data_provider, data_provider) + end + + it "as a coordinator it allows changing other user's name" do + expect(policy).to permit(data_coordinator, data_provider) + end + + it "as a support user it allows changing other user's name" do + expect(policy).to permit(support, data_provider) + end + end + + permissions :edit_emails? do + it "allows changing their own email" do + expect(policy).to permit(data_provider, data_provider) + end + + it "as a coordinator it allows changing other user's email" do + expect(policy).to permit(data_coordinator, data_provider) + end + + it "as a support user it allows changing other user's email" do + expect(policy).to permit(support, data_provider) + end + end + + permissions :edit_password? do + it "as a provider it allows changing their own password" do + expect(policy).to permit(data_provider, data_provider) + end + + it "as a coordinator it allows changing their own password" do + expect(policy).to permit(data_coordinator, data_coordinator) + end + + it "as a support user it allows changing their own password" do + expect(policy).to permit(support, support) + end + + it "as a coordinator it does not allow changing other user's password" do + expect(policy).not_to permit(data_coordinator, data_provider) + end + + it "as a support user it does not allow changing other user's password" do + expect(policy).not_to permit(support, data_provider) + end + end + + permissions :edit_roles? do + it "as a provider it does not allow changing roles" do + expect(policy).not_to permit(data_provider, data_provider) + end + + it "as a coordinator allows changing other user's roles" do + expect(policy).to permit(data_coordinator, data_provider) + end + + it "as a support user allows changing other user's roles" do + expect(policy).to permit(support, data_provider) + end + end + + permissions :edit_dpo? do + it "as a provider it does not allow changing dpo" do + expect(policy).not_to permit(data_provider, data_provider) + end + + it "as a coordinator allows changing other user's dpo" do + expect(policy).to permit(data_coordinator, data_provider) + end + + it "as a support user allows changing other user's dpo" do + expect(policy).to permit(support, data_provider) + end + end + + permissions :edit_key_contact? do + it "as a provider it does not allow changing key_contact" do + expect(policy).not_to permit(data_provider, data_provider) + end + + it "as a coordinator allows changing other user's key_contact" do + expect(policy).to permit(data_coordinator, data_provider) + end + + it "as a support user allows changing other user's key_contact" do + expect(policy).to permit(support, data_provider) + end + end +end +# rubocop:enable RSpec/RepeatedExample diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index ecba12933..564167354 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -120,6 +120,7 @@ RSpec.describe UsersController, type: :request do it "allows changing name, email and password" do expect(page).to have_link("Change", text: "name") expect(page).to have_link("Change", text: "email address") + expect(page).to have_link("Change", text: "telephone number") expect(page).to have_link("Change", text: "password") expect(page).not_to have_link("Change", text: "role") expect(page).not_to have_link("Change", text: "if data protection officer") @@ -180,6 +181,7 @@ RSpec.describe UsersController, type: :request do it "does not have edit links" do expect(page).not_to have_link("Change", text: "name") expect(page).not_to have_link("Change", text: "email address") + expect(page).not_to have_link("Change", text: "telephone number") expect(page).not_to have_link("Change", text: "password") expect(page).not_to have_link("Change", text: "role") expect(page).not_to have_link("Change", text: "if data protection officer") @@ -499,6 +501,7 @@ RSpec.describe UsersController, type: :request do it "allows changing name, email, password, role, dpo and key contact" do expect(page).to have_link("Change", text: "name") expect(page).to have_link("Change", text: "email address") + expect(page).to have_link("Change", text: "telephone number") expect(page).to have_link("Change", text: "password") expect(page).to have_link("Change", text: "role") expect(page).to have_link("Change", text: "if data protection officer") @@ -543,6 +546,7 @@ RSpec.describe UsersController, type: :request do it "allows changing name, email, role, dpo and key contact" do expect(page).to have_link("Change", text: "name") expect(page).to have_link("Change", text: "email address") + expect(page).to have_link("Change", text: "telephone number") expect(page).not_to have_link("Change", text: "password") expect(page).to have_link("Change", text: "role") expect(page).to have_link("Change", text: "if data protection officer") @@ -1169,6 +1173,7 @@ RSpec.describe UsersController, type: :request do it "allows changing name, email, password, role, dpo and key contact" do expect(page).to have_link("Change", text: "name") expect(page).to have_link("Change", text: "email address") + expect(page).to have_link("Change", text: "telephone number") expect(page).to have_link("Change", text: "password") expect(page).to have_link("Change", text: "role") expect(page).to have_link("Change", text: "if data protection officer") @@ -1198,6 +1203,7 @@ RSpec.describe UsersController, type: :request do it "allows changing name, email, role, dpo and key contact" do expect(page).to have_link("Change", text: "name") expect(page).to have_link("Change", text: "email address") + expect(page).to have_link("Change", text: "telephone number") expect(page).not_to have_link("Change", text: "password") expect(page).to have_link("Change", text: "role") expect(page).to have_link("Change", text: "if data protection officer") @@ -1242,6 +1248,7 @@ RSpec.describe UsersController, type: :request do it "allows changing name, email, role, dpo and key contact" do expect(page).to have_link("Change", text: "name") expect(page).to have_link("Change", text: "email address") + expect(page).to have_link("Change", text: "telephone number") expect(page).not_to have_link("Change", text: "password") expect(page).to have_link("Change", text: "role") expect(page).to have_link("Change", text: "if data protection officer")