Cleaned up the User model, controller, access periods and added a UserRoleService to help with this.

app/controllers/users_controller.rb

@@ -1,78 +1,46 @@
 class UsersController < ApplicationController
   before_action :authenticate_user!
-  before_action :set_user, only: [:edit, :update, :destroy]
   before_action :require_admin
+  before_action :set_user, only: [:show, :edit, :update, :destroy]
   load_and_authorize_resource
 
-  def index
-    @users = User.all
-  end
-
-  def new
-    @user = User.new
-    @user.access_periods.build unless @user.access_periods.any?
-  end
-  
-  
-
-  def edit
-    @user = User.find(params[:id])
-    @user.access_periods.build if @user.access_periods.empty?
-  end
-  
-
   def create
     @user = User.new(user_params)
-    # Ensure an access period is built if none exist and no start_date is provided
-    if @user.access_periods.empty?
-      @user.access_periods.build(start_date: Date.today)  # Set start date automatically
-    end
-  
     if @user.save
-      update_user_roles(@user, params[:user][:roles] || ['user'])
       redirect_to users_path, notice: 'User was successfully created.'
     else
       render :new
     end
   end
-  
+
+  def edit
+    # Since @user is set by set_user, there's no need to find the user again
+  end
+
   def update
-    @user = User.find(params[:id])  # If @user is set in a before_action, this line can be removed.
-  
-    # Handling password change: if password fields are blank, they are removed from user_params to prevent updating the password to nil.
+    # Clean up password fields if they are blank
     cleaned_params = user_params
     if cleaned_params[:password].blank?
       cleaned_params.delete(:password)
       cleaned_params.delete(:password_confirmation)
     end
   
-    if @user.update(cleaned_params.except(:roles))
-      # Update roles
-      update_user_roles(@user, params[:user][:roles] || [])
-  
-      # Check and update the access revoked status and end date
-      if params[:user][:access_revoked] == "1"
-        current_period = @user.access_periods.order(:created_at).last
-        current_period.update(end_date: Date.today) unless current_period.end_date.present?
-      end
-  
+    # Attempt to update the user with the cleaned parameters
+    if @user.update(cleaned_params)
+      UserRoleService.new(@user, params[:user][:roles] || []).update_roles  # Handle roles separately
       redirect_to users_path, notice: 'User was successfully updated.'
     else
-      render :edit
+      render :edit  # If there's an error, it will render the edit view where you can display error messages
     end
   end
   
-
-  def show
-    @user = User.includes(:access_periods).find(params[:id])
-  end
   
-
   def destroy
     @user.destroy
     redirect_to users_path, notice: 'User was successfully deleted.'
   end
 
+
   private
 
   def set_user
@@ -84,46 +52,12 @@ class UsersController < ApplicationController
       :email, :password, :password_confirmation, :remember_me,
       :first_name, :last_name, :phone, :company, 
       :access_revoked, :access_start_date, :access_end_date,
-      access_periods_attributes: [:id, :start_date, :end_date, :_destroy], 
-      roles: []
+      access_periods_attributes: [:id, :start_date, :end_date, :_destroy],
     )
   end
   
 
   def require_admin
-    unless current_user.admin?
-      redirect_to root_path, alert: 'Only admins are allowed to access this section.'
-    end
+    redirect_to root_path, alert: 'Only admins are allowed to access this section.' unless current_user.admin?
   end
-
-  def assign_roles(user)
-    user.roles = []
-    params[:user][:roles].each do |role_name|
-      user.add_role(role_name) unless role_name.blank?
-    end if params[:user][:roles].present?
-  end
-  
-
-  def update_user_roles(user, roles_names)
-    # Assume roles_names should only contain one role at most
-    new_role = roles_names.reject(&:blank?).uniq.first  # Take the first valid role name
-  
-    # Clear all roles and set the new one
-    user.roles.delete_all
-    user.add_role(new_role) if new_role
-  end
-  
-  
-
-  def handle_access_revocation
-    if params[:user][:access_revoked] == "1"
-      current_period = @user.access_periods.find_or_initialize_by(end_date: nil)
-      current_period.update(end_date: Date.today) unless current_period.end_date.present?
-    elsif params[:user][:access_revoked] == "0" && @user.access_periods.last&.end_date.present?
-      @user.access_periods.build(start_date: Date.today)
-    end
-  end
-  
-  
-
 end

app/models/access_period.rb

@@ -1,6 +1,7 @@
 class AccessPeriod < ApplicationRecord
   belongs_to :user
-  before_validation :set_default_start_date, on: :create
+
+  before_create :set_default_start_date
   validates :end_date, presence: true, if: -> { user&.access_revoked? }
 
   private
@@ -9,4 +10,3 @@ class AccessPeriod < ApplicationRecord
     self.start_date ||= Date.today if self.new_record? && self.start_date.blank?
   end
 end
-

app/models/user.rb

@@ -4,6 +4,7 @@ class User < ApplicationRecord
   has_many :access_periods, dependent: :destroy
   accepts_nested_attributes_for :access_periods, allow_destroy: true
   after_create :assign_default_role
+  before_update :handle_access_revocation
 
   validate :password_complexity
 
@@ -33,6 +34,14 @@ class User < ApplicationRecord
     self.admin = has_role?(:admin)
   end
 
+  def handle_access_revocation
+    if access_revoked_changed? && access_revoked
+      access_periods.find_or_initialize_by(end_date: nil).update(end_date: Date.today)
+    elsif access_revoked_changed? && !access_revoked
+      access_periods.create(start_date: Date.today)
+    end
+  end
+
   def end_date_after_start_date
     if access_start_date.present?
       errors.add(:access_end_date, 'must be provided when access is revoked') unless access_end_date.present?

app/services/user_role_service.rb

@@ -0,0 +1,24 @@
+class UserRoleService
+    def initialize(user, role_names)
+      @user = user
+      @role_names = role_names.reject(&:blank?).uniq
+    end
+  
+    def update_roles
+      @user.roles = Role.where(name: @role_names)
+    end
+  
+    private
+  
+    def remove_unassigned_roles
+      current_roles = @user.roles.pluck(:name)
+      roles_to_remove = current_roles - @role_names
+      roles_to_remove.each { |role| @user.remove_role(role) }
+    end
+  
+    def add_new_roles
+      roles_to_add = @role_names - @user.roles.pluck(:name)
+      roles_to_add.each { |role| @user.add_role(role) }
+    end
+  end
+