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 66695d1cd..0e304653c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -4,14 +4,20 @@ class User < ApplicationRecord 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 validates :name, presence: true - validates :email, presence: true, uniqueness: true - validates :email, format: { with: Devise.email_regexp } + 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 @@ -154,4 +160,13 @@ class User < ApplicationRecord def valid_for_authentication? super && active? end + +protected + + # 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..bfe04305c 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: "This does not need to be selected if the user is a data protection officer only" } %> <%= f.govuk_submit "Continue" %> diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e9fb14283..5b68a736e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -227,17 +227,15 @@ RSpec.describe User, type: :model do end describe "validate" do - let(:organisation) { FactoryBot.create(:organisation) } - context "when a user does not have values for required fields" do - let(:user) { described_class.new(organisation:) } + let(:user) { described_class.new } before do user.validate end - it "validates name presence before email presence" do - expect(user.errors.map(&:attribute).uniq).to eq(%i[name email]) + 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 diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index a99d1e6c3..b82093d7e 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -922,7 +922,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