diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index c22570a2e..cd82742b6 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -58,7 +58,7 @@ class UsersController < ApplicationController end elsif user_params.key?("password") format_error_messages - @minimum_password_length = User.password_length.min + @minimum_password_length = Devise.password_length.min render "devise/passwords/edit", locals: { resource: @user, resource_name: "user" }, status: :unprocessable_entity else format_error_messages @@ -79,7 +79,6 @@ class UsersController < ApplicationController redirect_to created_user_redirect_path else unless @resource.errors[:organisation].empty? - @resource.errors.add(:organisation_id, message: @resource.errors[:organisation]) @resource.errors.delete(:organisation) end render :new, status: :unprocessable_entity @@ -87,7 +86,7 @@ class UsersController < ApplicationController end def edit_password - @minimum_password_length = User.password_length.min + @minimum_password_length = Devise.password_length.min render "devise/passwords/edit", locals: { resource: @user, resource_name: "user" } end @@ -114,9 +113,6 @@ 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[:role].empty? - @resource.errors.add :role, I18n.t("activerecord.errors.models.user.attributes.role.blank") - end end def format_error_messages diff --git a/app/models/user.rb b/app/models/user.rb index f9ed4a0e9..f3233bff6 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,16 +1,23 @@ class User < ApplicationRecord # Include default devise modules. Others available are: # :omniauthable - include Helpers::Email - devise :database_authenticatable, :recoverable, :rememberable, :validatable, + devise :database_authenticatable, :recoverable, :rememberable, :trackable, :lockable, :two_factor_authenticatable, :confirmable, :timeoutable - belongs_to :organisation + # Marked as optional because we validate organisation_id below instead so that + # the error message is linked to the right field on the form + belongs_to :organisation, optional: true has_many :owned_case_logs, through: :organisation, dependent: :delete_all has_many :managed_case_logs, through: :organisation - validate :validate_email - validates :name, :email, presence: true + validates :name, presence: true + validates :email, presence: true + validates :email, uniqueness: { allow_blank: true, case_sensitive: true, if: :will_save_change_to_email? } + validates :email, format: { with: Devise.email_regexp, allow_blank: true, if: :will_save_change_to_email? } + validates :password, presence: { if: :password_required? } + validates :password, confirmation: { if: :password_required? } + validates :password, length: { within: Devise.password_length, allow_blank: true } + validates :organisation_id, presence: true has_paper_trail ignore: %w[last_sign_in_at current_sign_in_at @@ -156,15 +163,12 @@ class User < ApplicationRecord super && active? end -private +protected - def validate_email - unless email_valid?(email) - if User.exists?(["email LIKE ?", "%#{email}%"]) - errors.add :email, I18n.t("activerecord.errors.models.user.attributes.email.taken") - else - errors.add :email, I18n.t("activerecord.errors.models.user.attributes.email.invalid") - end - end + # Checks whether a password is needed or not. For validations only. + # Passwords are always required if it's a new record, or if the password + # or confirmation are being set somewhere. + def password_required? + !persisted? || !password.nil? || !password_confirmation.nil? end end diff --git a/app/views/users/new.html.erb b/app/views/users/new.html.erb index cf15c06e0..cf72e49f0 100644 --- a/app/views/users/new.html.erb +++ b/app/views/users/new.html.erb @@ -41,7 +41,7 @@ options: { disabled: [""], selected: @organisation_id ? answer_options.first : "" } %> <% end %> - <% hints_for_roles = { data_provider: ["Default role for this organisation", "Can view and submit logs for this organisation"], data_coordinator: ["Can view and submit logs for this organisation and any of its managing agents", "Can manage details for this organisation", "Can manage users for this organisation"], support: nil } %> + <% hints_for_roles = { data_provider: ["Can view and submit logs for this organisation"], data_coordinator: ["Can view and submit logs for this organisation and any of its managing agents", "Can manage details for this organisation", "Can manage users for this organisation"], support: nil } %> <% roles_with_hints = current_user.assignable_roles.map { |key, _| OpenStruct.new(id: key, name: key.to_s.humanize, description: hints_for_roles[key.to_sym]) } %> @@ -52,7 +52,8 @@ lambda { |option| option.description&.map { |hint| content_tag(:li, hint) }&.reduce(:+) }, - legend: { text: "Role", size: "m" } %> + legend: { text: "Role (optional)", size: "m" }, + hint: { text: "You do not need to select a role if the user is a data protection officer only. You can tell us that this user is a data protection officer after you have invited them." } %> <%= f.govuk_submit "Continue" %> diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 83c73db72..cd1a30ee8 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -184,7 +184,7 @@ Devise.setup do |config| # Email regex used to validate email formats. It simply asserts that # one (and only one) @ exists in the given string. This is mainly # to give user feedback and not to assert the e-mail validity. - config.email_regexp = /\A[^@\s]+@[^@\s]+\z/ + config.email_regexp = URI::MailTo::EMAIL_REGEXP # ==> Configuration for :timeoutable # The time you want to timeout the user session without activity. After this diff --git a/config/locales/en.yml b/config/locales/en.yml index 4fc1c7b61..56b70d397 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -78,8 +78,8 @@ en: user: attributes: organisation_id: - blank: "Enter the existing organisation’s name" - invalid: "Enter the existing organisation’s name" + blank: "Select the user’s organisation" + invalid: "Select the user’s organisation" name: blank: "Enter a name" email: diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index caf8d03d3..5b68a736e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -225,4 +225,48 @@ RSpec.describe User, type: :model do end end end + + describe "validate" do + context "when a user does not have values for required fields" do + let(:user) { described_class.new } + + before do + user.validate + end + + it "validates name, email and organisation presence in the correct order" do + expect(user.errors.map(&:attribute).uniq).to eq(%i[name email password organisation_id]) + end + end + + context "when a too short password is entered" do + let(:password) { "123" } + let(:error_message) { "Validation failed: Password #{I18n.t('errors.messages.too_short', count: 8)}" } + + it "validates password length" do + expect { FactoryBot.create(:user, password:) } + .to raise_error(ActiveRecord::RecordInvalid, error_message) + end + end + + context "when an invalid email is entered" do + let(:invalid_email) { "not_an_email" } + let(:error_message) { "Validation failed: Email #{I18n.t('activerecord.errors.models.user.attributes.email.invalid')}" } + + it "validates email format" do + expect { FactoryBot.create(:user, email: invalid_email) } + .to raise_error(ActiveRecord::RecordInvalid, error_message) + end + end + + context "when the email entered has already been used" do + let(:user) { FactoryBot.create(:user) } + let(:error_message) { "Validation failed: Email #{I18n.t('activerecord.errors.models.user.attributes.email.taken')}" } + + it "validates email uniqueness" do + expect { FactoryBot.create(:user, email: user.email) } + .to raise_error(ActiveRecord::RecordInvalid, error_message) + end + end + end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index c5303776a..1d3b55cb0 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -930,7 +930,6 @@ RSpec.describe UsersController, type: :request do expect(response).to have_http_status(:unprocessable_entity) expect(page).to have_content(I18n.t("activerecord.errors.models.user.attributes.name.blank")) expect(page).to have_content(I18n.t("activerecord.errors.models.user.attributes.email.blank")) - expect(page).to have_content(I18n.t("activerecord.errors.models.user.attributes.role.blank")) end end end diff --git a/spec/services/imports/user_import_service_spec.rb b/spec/services/imports/user_import_service_spec.rb index 02a776ecc..66ea6eb07 100644 --- a/spec/services/imports/user_import_service_spec.rb +++ b/spec/services/imports/user_import_service_spec.rb @@ -44,7 +44,7 @@ RSpec.describe Imports::UserImportService do end it "refuses to create a user belonging to a non existing organisation" do - expect(logger).to receive(:error).with(/Organisation must exist/) + expect(logger).to receive(:error).with(/ActiveRecord::RecordInvalid/) import_service.create_users("user_directory") end