refactor User and TrustLevel a bit

refactor User and TrustLevel a bit

  • rename User#password_required to User#password_required!
  • emails with “i” @ something are a special case as well
  • get rid of self. and returns where possible
  • prefer “unless a” instead of “if !a”
  • unread_notifications without manually iterating
  • introduce User#moderator?
  • introduce TrustLevel#valid_key?, TrustLevel#compare, and TrustLevel#level_key
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index dc5156d..058ee3b 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -143,7 +143,7 @@ class UsersController < ApplicationController
     if auth && auth[:email] == params[:email] && auth[:email_valid]
       user.active = true
     end
-    user.password_required unless auth
+    user.password_required! unless auth
 
     DiscourseHub.register_nickname( user.username, user.email ) if user.valid? and SiteSetting.call_discourse_hub?
 
diff --git a/app/models/user.rb b/app/models/user.rb
index a9b7212..e6e1ce8 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -2,7 +2,6 @@ require_dependency 'email_token'
 require_dependency 'trust_level'
 
 class User < ActiveRecord::Base
-
   attr_accessible :name, :username, :password, :email, :bio_raw, :website
 
   has_many :posts
@@ -28,7 +27,7 @@ class User < ActiveRecord::Base
   validates_presence_of :email
   validates_uniqueness_of :email
   validate :username_validator
-  validate :email_validator, :if => :email_changed?
+  validate :email_validator, if: :email_changed?
   validate :password_validator
 
   before_save :cook
@@ -56,15 +55,14 @@ class User < ActiveRecord::Base
   end
 
   def self.suggest_username(name)
-
     return nil unless name.present?
 
     # If it's an email
     if name =~ /([^@]+)@([^\.]+)/
       name = Regexp.last_match[1]
 
-      # Special case, if it's me @ something, take the something.
-      name = Regexp.last_match[2] if name == 'me'
+      # Special case, if it's "me" or "i" @ something, take the something.
+      name = Regexp.last_match[2] if ['i', 'me'].include?(name)
     end
 
     name.gsub!(/^[^A-Za-z0-9]+/, "")
@@ -82,9 +80,9 @@ class User < ActiveRecord::Base
     attempt = name
     while !username_available?(attempt)
       suffix = i.to_s
-      max_length = User.username_length.end - 1 - suffix.length
+      max_length = User.username_length.end - suffix.length - 1
       attempt = "#{name[0..max_length]}#{suffix}"
-      i+=1
+      i += 1
     end
     attempt
   end
@@ -94,8 +92,8 @@ class User < ActiveRecord::Base
 
     if SiteSetting.call_discourse_hub?
       begin
-        match, available, suggestion = DiscourseHub.nickname_match?( username, email )
-        username = suggestion unless match or available
+        match, available, suggestion = DiscourseHub.nickname_match?(username, email)
+        username = suggestion unless match || available
       rescue => e
         Rails.logger.error e.message + "\n" + e.backtrace.join("\n")
       end
@@ -107,7 +105,7 @@ class User < ActiveRecord::Base
 
     if SiteSetting.call_discourse_hub?
       begin
-        DiscourseHub.register_nickname( username, email )
+        DiscourseHub.register_nickname(username, email)
       rescue => e
         Rails.logger.error e.message + "\n" + e.backtrace.join("\n")
       end
@@ -118,42 +116,41 @@ class User < ActiveRecord::Base
 
   def self.username_available?(username)
     lower = username.downcase
-    !User.where(username_lower: lower).exists?
+    User.where(username_lower: lower).blank?
   end
 
   def self.username_valid?(username)
     u = User.new(username: username)
     u.username_format_validator
-    !u.errors[:username].present?
+    u.errors[:username].blank?
   end
 
   def enqueue_welcome_message(message_type)
     return unless SiteSetting.send_welcome_message?
-    Jobs.enqueue(:send_system_message, user_id: self.id, message_type: message_type)
+    Jobs.enqueue(:send_system_message, user_id: id, message_type: message_type)
   end
 
   def self.suggest_name(email)
     return "" unless email
     name = email.split(/[@\+]/)[0]
-    name = name.sub(".", "  ")
-    name.split(" ").collect{|word| word[0] = word[0].upcase; word}.join(" ")
+    name = name.gsub(".", " ")
+    name.titleize
   end
 
   def change_username(new_username)
-    current_username = self.username
-    self.username = new_username
+    current_username, self.username = username, new_username
 
-    if SiteSetting.call_discourse_hub? and self.valid?
+    if SiteSetting.call_discourse_hub? && valid?
       begin
-        DiscourseHub.change_nickname( current_username, new_username )
+        DiscourseHub.change_nickname(current_username, new_username)
       rescue DiscourseHub::NicknameUnavailable
-        return false
+        false
       rescue => e
         Rails.logger.error e.message + "\n" + e.backtrace.join("\n")
       end
     end
 
-    self.save
+    save
   end
 
   # Use a temporary key to find this user, store it in redis with an expiry
@@ -183,8 +180,7 @@ class User < ActiveRecord::Base
 
   def invited_by
     used_invite = invites.where("redeemed_at is not null").includes(:invited_by).first
-    return nil unless used_invite.present?
-    used_invite.invited_by
+    used_invite.try(:invited_by)
   end
 
   # Approve this user
@@ -200,7 +196,7 @@ class User < ActiveRecord::Base
   end
 
   def email_hash
-    User.email_hash(self.email)
+    User.email_hash(email)
   end
 
   def unread_notifications_by_type
@@ -214,16 +210,11 @@ class User < ActiveRecord::Base
 
 
   def unread_private_messages
-    return 0 if unread_notifications_by_type.blank?
-    return unread_notifications_by_type[Notification.Types[:private_message]] || 0
+    unread_notifications_by_type[Notification.Types[:private_message]] || 0
   end
 
   def unread_notifications
-    result = 0
-    unread_notifications_by_type.each do |k,v|
-      result += v unless k == Notification.Types[:private_message]
-    end
-    result
+    unread_notifications_by_type.except(Notification.Types[:private_message]).values.sum
   end
 
   def saw_notification_id(notification_id)
@@ -231,10 +222,10 @@ class User < ActiveRecord::Base
   end
 
   def publish_notifications_state
-    MessageBus.publish("/notification/#{self.id}",
-        {unread_notifications: self.unread_notifications,
-         unread_private_messages: self.unread_private_messages},
-        user_ids: [self.id] # only publish the notification to this user
+    MessageBus.publish("/notification/#{id}",
+        { unread_notifications: unread_notifications,
+          unread_private_messages: unread_private_messages },
+        user_ids: [id] # only publish the notification to this user
       )
   end
 
@@ -243,31 +234,31 @@ class User < ActiveRecord::Base
     User.select(:username).order('last_posted_at desc').limit(20)
   end
 
+  def moderator?
+    has_trust_level?(:moderator)
+  end
+
   def regular?
-    (not admin?) and (not has_trust_level?(:moderator))
+    !(admin? || moderator?)
   end
 
   def password=(password)
     # special case for passwordless accounts
-    unless password.blank?
-      @raw_password = password
-    end
+    @raw_password = password unless password.blank?
   end
 
   # Indicate that this is NOT a passwordless account for the purposes of validation
-  def password_required
+  def password_required!
     @password_required = true
   end
 
   def confirm_password?(password)
-    return false unless self.password_hash && self.salt
-    self.password_hash == hash_password(password,self.salt)
+    return false unless password_hash && salt
+    self.password_hash == hash_password(password, salt)
   end
 
   def seen?(date)
-    if last_seen_at.present?
-      !(last_seen_at.to_date < date)
-    end
+    last_seen_at.to_date >= date if last_seen_at.present?
   end
 
   def seen_before?
@@ -275,30 +266,27 @@ class User < ActiveRecord::Base
   end
 
   def has_visit_record?(date)
-    user_visits.where(["visited_at =? ", date ]).first
+    user_visits.where(["visited_at =? ", date]).first
   end
 
   def adding_visit_record(date)
-    user_visits.create!(visited_at: date )
+    user_visits.create!(visited_at: date)
   end
 
   def update_visit_record!(date)
-    if !seen_before?
+    unless seen_before?
       adding_visit_record(date)
       update_column(:days_visited, 1)
     end
 
-    if !seen?(date)

[... diff too long, it was truncated ...]

GitHub sha: d2f3c829

1 Like

@eviltrout we need to be more careful about accepting patches that bork structure.sql, perhaps we should not even allow people to check it in and auto generate.

1 Like

Weird I just got a notification about this comment… from 6 years ago!!!

3 Likes

@eviltrout, The topic may say that it’s 6 years old, but going by the id, I would say that it was created recently (the plugin uses github’s created_at time). I suspect it was created because it was referenced by a pull request, but I’m not sure why the PR topic was created.

2 Likes