CLDC-1783 Add telephone number question to the user form (#1706)

* Add telephone number question to the user form

* Extract user policy

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
 

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

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

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) } %>
 

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) } %>

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),

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"

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)

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) }
 

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

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")