diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index f9a86055d..a9344f0f7 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -5,7 +5,7 @@ class UsersController < ApplicationController include Modules::SearchFilter before_action :authenticate_user! - before_action :find_resource, except: %i[new create] + before_action :find_user, except: %i[new create] before_action :authenticate_scope!, except: %i[new] before_action :session_filters, if: :current_user, only: %i[index] before_action -> { filter_manager.serialize_filters_to_session }, if: :current_user, only: %i[index] @@ -49,7 +49,8 @@ class UsersController < ApplicationController end def update - if @user.update(user_params) + validate_attributes + if @user.errors.empty? && @user.update(user_params) if @user == current_user bypass_sign_in @user flash[:notice] = I18n.t("devise.passwords.updated") if user_params.key?("password") @@ -83,18 +84,18 @@ class UsersController < ApplicationController def new @organisation_id = params["organisation_id"] - @resource = User.new + @user = User.new end def create - @resource = User.new(user_params.merge(org_params).merge(password_params)) + @user = User.new(user_params.merge(org_params).merge(password_params)) validate_attributes - if @resource.errors.empty? && @resource.save + if @user.errors.empty? && @user.save redirect_to created_user_redirect_path else - unless @resource.errors[:organisation].empty? - @resource.errors.delete(:organisation) + unless @user.errors[:organisation].empty? + @user.errors.delete(:organisation) end render :new, status: :unprocessable_entity end @@ -124,15 +125,15 @@ class UsersController < ApplicationController private def validate_attributes - @resource.validate + @user.validate if user_params[:role].present? && !current_user.assignable_roles.key?(user_params[:role].to_sym) - @resource.errors.add :role, I18n.t("validations.role.invalid") + @user.errors.add :role, I18n.t("validations.role.invalid") end - if user_params[:phone].blank? - @resource.errors.add :phone, :blank - elsif !valid_phone_number?(user_params[:phone]) - @resource.errors.add :phone + if !user_params[:phone].nil? && user_params[:phone].blank? + @user.errors.add :phone, :blank + elsif !user_params[:phone].nil? && !valid_phone_number?(user_params[:phone]) + @user.errors.add :phone end end @@ -188,7 +189,7 @@ private end end - def find_resource + def find_user @user = User.find_by(id: params[:user_id]) || User.find_by(id: params[:id]) || current_user end diff --git a/app/views/users/new.html.erb b/app/views/users/new.html.erb index fc66bcad8..0465f67bb 100644 --- a/app/views/users/new.html.erb +++ b/app/views/users/new.html.erb @@ -4,7 +4,7 @@ <%= govuk_back_link(href: :back) %> <% end %> -<%= form_for(@resource, as: :user, html: { method: :post }) do |f| %> +<%= form_for(@user, as: :user, html: { method: :post }) do |f| %>
<%= f.govuk_error_summary %> @@ -21,13 +21,13 @@ label: { text: "Email address", size: "m" }, autocomplete: "email", spellcheck: "false", - value: @resource.email %> + value: @user.email %> <%= f.govuk_phone_field :phone, label: { text: "Telephone number", size: "m" }, autocomplete: "phone", spellcheck: "false", - value: @resource.phone %> + value: @user.phone %> <% if current_user.support? %> <% null_option = [OpenStruct.new(id: "", name: "Select an option")] %> diff --git a/spec/factories/user.rb b/spec/factories/user.rb index 7f9b45932..935bd30f6 100644 --- a/spec/factories/user.rb +++ b/spec/factories/user.rb @@ -5,6 +5,7 @@ FactoryBot.define do password { "pAssword1" } organisation role { "data_provider" } + phone { "1234512345123" } trait :data_coordinator do role { "data_coordinator" } end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index dbdde21b8..958f2187a 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -894,6 +894,64 @@ RSpec.describe UsersController, type: :request do expect(response).to have_http_status(:unprocessable_entity) end end + + context "when updating telephone numbers" do + let(:params) do + { + "user": { + phone:, + }, + } + end + + before do + sign_in user + patch "/users/#{user.id}", headers:, params: + end + + context "when telephone number is not given" do + let(:phone) { "" } + + it "validates telephone number" do + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content(I18n.t("activerecord.errors.models.user.attributes.phone.blank")) + end + end + + context "when telephone number is not numeric" do + let(:phone) { "randomstring" } + + it "validates telephone number" do + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content(I18n.t("activerecord.errors.models.user.attributes.phone.invalid")) + end + end + + context "when telephone number is shorter than 11 digits" do + let(:phone) { "123" } + + it "validates telephone number" do + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content(I18n.t("activerecord.errors.models.user.attributes.phone.invalid")) + end + end + + context "when telephone number is in correct format" do + let(:phone) { "012345678919" } + + it "validates telephone number" do + expect(page).not_to have_content(I18n.t("activerecord.errors.models.user.attributes.phone.invalid")) + end + end + + context "when telephone number is in correct format and includes +" do + let(:phone) { "+12345678919" } + + it "validates telephone number" do + expect(page).not_to have_content(I18n.t("activerecord.errors.models.user.attributes.phone.invalid")) + end + end + end end describe "#create" do