From 77174e2a82024bbb19a98ae2b747c69950a94bd5 Mon Sep 17 00:00:00 2001 From: Kat Date: Fri, 16 Jun 2023 12:50:19 +0100 Subject: [PATCH] Add telephone number question to the user form --- app/controllers/users_controller.rb | 16 ++++++++++++---- app/helpers/user_helper.rb | 4 ++++ app/views/users/edit.html.erb | 5 +++++ app/views/users/new.html.erb | 6 ++++++ app/views/users/show.html.erb | 10 ++++++++++ config/locales/en.yml | 2 ++ spec/features/user_spec.rb | 26 ++++++++++++++++++++++++++ spec/requests/users_controller_spec.rb | 7 +++++++ 8 files changed, 72 insertions(+), 4 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 0c343b8c3..da51f51d5 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -114,6 +114,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 @@ -145,14 +153,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..d6eebafee 100644 --- a/app/helpers/user_helper.rb +++ b/app/helpers/user_helper.rb @@ -34,4 +34,8 @@ module UserHelper def can_edit_org?(current_user) current_user.data_coordinator? || current_user.support? end + + def can_edit_telephone_numbers?(user, current_user) + (current_user == user || current_user.data_coordinator? || current_user.support?) && user.active? + 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 e6cb9fd3e..20f2e807f 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -41,6 +41,16 @@ end end %> + <%= summary_list.row do |row| + row.key { "Telephone number" } + row.value { @user.phone } + if can_edit_telephone_numbers?(@user, current_user) + 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 { "••••••••" } 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 0c7f6c2c6..8ec487d35 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/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index a97086bb2..4c574c609 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -113,6 +113,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") @@ -165,6 +166,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") @@ -480,6 +482,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") @@ -520,6 +523,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") @@ -1138,6 +1142,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") @@ -1167,6 +1172,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") @@ -1207,6 +1213,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")