CLDC-1171: User validation order (#817)

* User validation order

* Rubocop

* Remove redundant method

* Clearer error message

* Role can be optional

* Hint text content

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

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,
-  devise :database_authenticatable, :recoverable, :rememberable, :validatable,
          :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, presence: true
-  validates :name, :email, 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
+  # Checks whether a password is needed or not. For validations only.
-    unless email_valid?(email)
+  # Passwords are always required if it's a new record, or if the password
-      if User.exists?(["email LIKE ?", "%#{email}%"])
+  # or confirmation are being set somewhere.
-        errors.add :email, I18n.t("activerecord.errors.models.user.attributes.email.taken")
+  def password_required?
-      else
+    !persisted? || !password.nil? || !password_confirmation.nil?
-        errors.add :email, I18n.t("activerecord.errors.models.user.attributes.email.invalid")
-      end
-    end
   end
 end

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" %>
     </div>

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

config/locales/en.yml

@@ -78,8 +78,8 @@ en:
         user:
           attributes:
             organisation_id:
-              blank: "Enter the existing organisation’s name"
+              blank: "Select the user’s organisation"
-              invalid: "Enter the existing organisation’s name"
+              invalid: "Select the user’s organisation"
             name:
               blank: "Enter a name"
             email:

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

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

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