Refactoring user controller and model class

app/controllers/users_controller.rb

@@ -106,13 +106,14 @@ def show_if_authorized
   end
 
   def show
+    # check whether current user is authorized to edit the user being searched, call show if true
     if params[:id].nil? || ((current_user_is_a? 'Student') && (!current_user_has_id? params[:id]))
       redirect_to(action: AuthHelper.get_home_action(session[:user]), controller: AuthHelper.get_home_controller(session[:user]))
     else
       @user = User.find(params[:id])
       @role = @user.role
       @assignment_participant_num = AssignmentParticipant.where(user_id: @user.id).count
-      @maps = ResponseMap.where('reviewee_id = ? or reviewer_id = ?', params[:id], params[:id])
+      @maps = ResponseMap.where('reviewee_id = :id OR reviewer_id = :id', id: params[:id])
       @total_user_num = User.count
     end
   end
@@ -212,7 +213,7 @@ def foreign
     # when a new user joins or an existing user updates his/her profile they will get to choose
     # from all the roles available
     role = Role.find(session[:user].role_id)
-    @all_roles = Role.where('id in (?) or id = ?', role.get_available_roles, role.id)
+    @all_roles = Role.where('id in  or id = ?', role.get_available_roles, role.id)
   end
 
   private

app/models/user.rb

@@ -19,31 +19,22 @@ class User < ApplicationRecord
   has_many :track_notifications, dependent: :destroy
   belongs_to :parent, class_name: 'User'
   belongs_to :role
-  validates :name, presence: true
-  validates :name, uniqueness: true
-  validates :name, format: { without: /\s/ }
-
-  validates :email, presence: { message: "can't be blank" }
-  validates :email, format: { with: /\A[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,4}\z/i, allow_blank: true }
 
+  validates :name, presence: true, uniqueness: true, format: { without: /\s/ }
+  validates :email,
+            presence: { message: "can't be blank" },
+            format: { with: /\A[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,4}\z/i, allow_blank: true }
   validates :fullname, presence: true
 
   before_validation :randomize_password, if: ->(user) { user.new_record? && user.password.blank? } # AuthLogic
 
-  scope :superadministrators, -> { where role_id: Role.superadministrator }
-  scope :superadmins, -> { superadministrators }
-  scope :administrators, -> { where role_id: Role.administrator }
-  scope :admins, -> { administrators }
+  scope :admins, -> { where role_id: Role.administrator }
   scope :instructors, -> { where role_id: Role.instructor }
   scope :tas, -> { where role_id: Role.ta }
   scope :students, -> { where role_id: Role.student }
 
   has_paper_trail
 
-  def salt_first?
-    true
-  end
-
   def list_mine(object_type, user_id)
     object_type.where(['instructor_id = ?', user_id])
   end
@@ -56,9 +47,10 @@ def get_available_users(name)
   end
 
   def can_impersonate?(user)
-    return true if role.super_admin?
-    return true if teaching_assistant_for?(user)
-    return true if recursively_parent_of(user)
+    # Takes a user object and returns true if the current user has permission to impersonate the provided user.
+    role.super_admin? ||
+      teaching_assistant_for?(user) ||
+      recursively_parent_of(user)
 
     false
   end