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
1 Answer 1
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.