Role can be optional

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

@@ -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

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

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
 

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

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