0
\$\begingroup\$

I've refactored this model about 50 times according to codeclimate, trying to get the code smell down. With all of the associations, I feel like this is as bare bones as I can get it while still keeping the necessary associations. I wanted to throw it up on here and see what people thought or if anyone had any suggestions on what I could do differently. It's pretty basic, has some custom validators for email and passwords, delegates, carrierwave upload mounter and some concerns included.

class User < ActiveRecord::Base
  self.table_name = "users_standard"

  include Scopeable::Clientable, Searchable, Auditable
  attr_accessor :password, :password_confirmation

  belongs_to :role
  has_many :discussion_posts, dependent: :destroy
  has_many :passtokens
  has_many :attendees
  has_many :authorized_ipads
  has_many :bookjobs
  with_options foreign_key: "user_id" do |user|
    user.has_and_belongs_to_many :questions,    join_table: "questions_users"
    user.has_and_belongs_to_many :chatposts,    join_table: "chatusers"
    user.has_and_belongs_to_many :permissions,  join_table: "permissions_users"
    user.has_and_belongs_to_many :committees,   join_table: "committees_users"
    user.has_and_belongs_to_many :bookfolders,  join_table: "bookfolders_users"
    user.has_and_belongs_to_many :efolders,     join_table: "efolders_users"
    user.has_and_belongs_to_many :surveys,      join_table: "surveys_users"
    user.has_and_belongs_to_many :tasks,        join_table: "tasks_users"
  end

  with_options uniqueness: { scope: :client_id } do |user| 
    user.validates :email, email: true, allow_blank: true
    user.validates :companyemail, email: true, allow_blank: true
    user.validates :login, presence: true
  end
  validates :firstname, :lastname, presence: true
  validates :password, presence: true, confirmation: { message: "does not match" }, password: true, allow_blank: true

  before_save do |u|
    if u.password.present?
      self.password_hash = SecurePasswordService.new(self.password).secure
      self.p2            = true
    end
  end

  accepts_nested_attributes_for :permissions, allow_destroy: true

  delegate :title, :hostname, :forge_email_sender, :email_from, :email_new_account, :profile_update_email, :remove_email_footer, :disclaimer, :disclaimer_expiration_period, :password_change_days, :ftlogin, :user_lockout, to: :client, prefix: true

  mount_uploader :photo, UserPhotoUploader

  def self.active
    where('active = 1 and (system = 0 or system is null) and global_spawn != 1').order('lastname asc')
  end

  def self.private_directory_list(user)
    joins(:committees).where('committees.id in (?)', user.committees.map(&:id))
  end

  def endpoint_safe
    { user: self.attributes.with_indifferent_access.except!(:client_id) }
  end

  def is_committee_member?(committee)
    (committee.is_a?(Committee) && self.committees.include?(committee)) or (!committee.is_a?(Committee) && committee.map{ |c| self.committees.include?(c) })
  end    
end
\$\endgroup\$
1
\$\begingroup\$

A partial (an really short) answer:

for User.active method you have the following:

system = 0 or system is null

I think you should change the condition order to

system is null or system = 0

This because when (and depending of dbms) the database compare null with a value (system = 0 could be null = 0) it can invalidate the condition.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.