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`

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?
 

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)
-      if !has_visit_record?(date)
-        adding_visit_record(date)
-        User.increment_counter(:days_visited, 1)
-      end
+    unless seen?(date) || has_visit_record?(date)
+      adding_visit_record(date)
+      User.increment_counter(:days_visited, 1)
     end
   end
 
   def update_ip_address!(new_ip_address)
-    if (ip_address != new_ip_address) and new_ip_address.present?
-      ip_address = new_ip_address
+    unless ip_address == new_ip_address && new_ip_address.blank?
       update_column(:ip_address, new_ip_address)
     end
   end
@@ -317,12 +305,10 @@ class User < ActiveRecord::Base
       # Keep track of our last visit
       if seen_before? && (self.last_seen_at < (now - SiteSetting.previous_visit_timeout_hours.hours))
         previous_visit_at = last_seen_at
-        update_column(:previous_visit_at, previous_visit_at )
+        update_column(:previous_visit_at, previous_visit_at)
       end
-      update_column(:last_seen_at,  now )
-
+      update_column(:last_seen_at,  now)
     end
-
   end
 
   def self.avatar_template(email)
@@ -334,14 +320,12 @@ class User < ActiveRecord::Base
 
   # return null for local avatars, a template for gravatar
   def avatar_template
-    # robohash = CGI.escape("http://robohash.org/size_") << "{size}x{size}" << CGI.escape("/#{email_hash}.png")
-    "http://www.gravatar.com/avatar/#{email_hash}.png?s={size}&r=pg&d=identicon"
+    User.avatar_template(email)
   end
 
 
   # Updates the denormalized view counts for all users
   def self.update_view_counts
-
     # Update denormalized topics_entered
     exec_sql "UPDATE users SET topics_entered = x.c
              FROM
@@ -360,13 +344,12 @@ class User < ActiveRecord::Base
                FROM post_timings AS pt
                GROUP BY pt.user_id) AS X
                WHERE x.user_id = users.id"
-
   end
 
   # The following count methods are somewhat slow - definitely don't use them in a loop.
   # They might need to be denormialzied
   def like_count
-    UserAction.where(user_id: self.id, action_type: UserAction::WAS_LIKED).count
+    UserAction.where(user_id: id, action_type: UserAction::WAS_LIKED).count
   end
 
   def post_count
@@ -374,7 +357,7 @@ class User < ActiveRecord::Base
   end
 
   def flags_given_count
-    PostAction.where(user_id: self.id, post_action_type_id: PostActionType.FlagTypes).count
+    PostAction.where(user_id: id, post_action_type_id: PostActionType.FlagTypes).count
   end
 
   def flags_received_count
@@ -402,23 +385,18 @@ class User < ActiveRecord::Base
   end
 
   def is_banned?
-    !banned_till.nil? && banned_till > DateTime.now
+    banned_till && banned_till > DateTime.now
   end
 
   # Use this helper to determine if the user has a particular trust level.
   # Takes into account admin, etc.
   def has_trust_level?(level)
-    raise "Invalid trust level #{level}" unless TrustLevel.Levels.has_key?(level)
-
-    # Admins can do everything
-    return true if admin?
-
-    # Otherwise compare levels
-    (self.trust_level || TrustLevel.Levels[:new]) >= TrustLevel.Levels[level]
+    raise "Invalid trust level #{level}" unless TrustLevel.valid_level?(level)
+    admin? || TrustLevel.compare(trust_level, level)
   end
 
   def change_trust_level(level)
-    raise "Invalid trust level #{level}" unless TrustLevel.Levels.has_key?(level)
+    raise "Invalid trust level #{level}" unless TrustLevel.valid_level?(level)
     self.trust_level = TrustLevel.Levels[level]
   end
 
@@ -434,7 +412,7 @@ class User < ActiveRecord::Base
   end
 
   def email_confirmed?
-    email_tokens.where(email: self.email, confirmed: true).present? or email_tokens.count == 0
+    email_tokens.where(email: email, confirmed: true).present? || email_tokens.empty?
   end
 
   def treat_as_new_topic_start_date
@@ -474,7 +452,7 @@ class User < ActiveRecord::Base
   protected
 
     def cook
-      if self.bio_raw.present?
+      if bio_raw.present?
         self.bio_cooked = PrettyText.cook(bio_raw) if bio_raw_changed?
       else
         self.bio_cooked = nil
@@ -482,31 +460,31 @@ class User < ActiveRecord::Base
     end
 
     def update_tracked_topics
-      if self.auto_track_topics_after_msecs_changed?
+      if auto_track_topics_after_msecs_changed?
 
         if auto_track_topics_after_msecs < 0
 
           User.exec_sql('update topic_users set notification_level = ?
                          where notifications_reason_id is null and
-                           user_id = ?' , TopicUser::NotificationLevel::REGULAR , self.id)
+                           user_id = ?' , TopicUser::NotificationLevel::REGULAR , id)
         else
 
           User.exec_sql('update topic_users set notification_level = ?
                          where notifications_reason_id is null and
                            user_id = ? and
-                           total_msecs_viewed < ?' , TopicUser::NotificationLevel::REGULAR , self.id, auto_track_topics_after_msecs)
+                           total_msecs_viewed < ?' , TopicUser::NotificationLevel::REGULAR , id, auto_track_topics_after_msecs)
 
           User.exec_sql('update topic_users set notification_level = ?
                          where notifications_reason_id is null and
                            user_id = ? and
-                           total_msecs_viewed >= ?' , TopicUser::NotificationLevel::TRACKING , self.id, auto_track_topics_after_msecs)
+                           total_msecs_viewed >= ?' , TopicUser::NotificationLevel::TRACKING , id, auto_track_topics_after_msecs)
         end
       end
     end
 
 
     def create_email_token
-      email_tokens.create(email: self.email)
+      email_tokens.create(email: email)
     end
 
     def ensure_password_is_hashed
@@ -517,7 +495,7 @@ class User < ActiveRecord::Base
     end
 
     def hash_password(password, salt)
-      PBKDF2.new(:password => password, :salt => salt, :iterations => Rails.configuration.pbkdf2_iterations).hex_string
+      PBKDF2.new(password: password, salt: salt, iterations: Rails.configuration.pbkdf2_iterations).hex_string
     end
 
     def add_trust_level
@@ -534,7 +512,7 @@ class User < ActiveRecord::Base
       username_format_validator || begin
         lower = username.downcase
         if username_changed? && User.where(username_lower: lower).exists?
-          return errors.add(:username, I18n.t(:'user.username.unique'))
+          errors.add(:username, I18n.t(:'user.username.unique'))
         end
       end
     end
@@ -544,15 +522,14 @@ class User < ActiveRecord::Base
         domains = setting.gsub('.', '\.')
         regexp = Regexp.new("@(#{domains})", true)
         if self.email =~ regexp
-          return errors.add(:email, I18n.t(:'user.email.not_allowed'))
+          errors.add(:email, I18n.t(:'user.email.not_allowed'))
         end
       end
     end
 
     def password_validator
-      if (@raw_password and @raw_password.length < 6) or (@password_required and !@raw_password)
-        return errors.add(:password, "must be 6 letters or longer")
+      if (@raw_password and @raw_password.length < 6) || (@password_required && !@raw_password)
+        errors.add(:password, "must be 6 letters or longer")
       end
     end
-
 end

db/structure.sql

@@ -1239,7 +1239,7 @@ CREATE TABLE categories (
 --
 
 CREATE SEQUENCE categories_id_seq
-    START WITH 1
+    START WITH 5
     INCREMENT BY 1
     NO MINVALUE
     NO MAXVALUE
@@ -1324,7 +1324,7 @@ CREATE TABLE draft_sequences (
 --
 
 CREATE SEQUENCE draft_sequences_id_seq
-    START WITH 1
+    START WITH 20
     INCREMENT BY 1
     NO MINVALUE
     NO MAXVALUE
@@ -1358,7 +1358,7 @@ CREATE TABLE drafts (
 --
 
 CREATE SEQUENCE drafts_id_seq
-    START WITH 1
+    START WITH 2
     INCREMENT BY 1
     NO MINVALUE
     NO MAXVALUE
@@ -1391,7 +1391,7 @@ CREATE TABLE email_logs (
 --
 
 CREATE SEQUENCE email_logs_id_seq
-    START WITH 1
+    START WITH 3
     INCREMENT BY 1
     NO MINVALUE
     NO MAXVALUE
@@ -1426,7 +1426,7 @@ CREATE TABLE email_tokens (
 --
 
 CREATE SEQUENCE email_tokens_id_seq
-    START WITH 1
+    START WITH 3
     INCREMENT BY 1
     NO MINVALUE
     NO MAXVALUE
@@ -1672,7 +1672,7 @@ CREATE TABLE onebox_renders (
 --
 
 CREATE SEQUENCE onebox_renders_id_seq
-    START WITH 1
+    START WITH 2
     INCREMENT BY 1
     NO MINVALUE
     NO MAXVALUE
@@ -1706,7 +1706,7 @@ CREATE TABLE post_action_types (
 --
 
 CREATE SEQUENCE post_action_types_id_seq
-    START WITH 1
+    START WITH 6
     INCREMENT BY 1
     NO MINVALUE
     NO MAXVALUE
@@ -1837,7 +1837,7 @@ CREATE TABLE posts (
 --
 
 CREATE SEQUENCE posts_id_seq
-    START WITH 1
+    START WITH 16
     INCREMENT BY 1
     NO MINVALUE
     NO MAXVALUE
@@ -1928,7 +1928,7 @@ CREATE TABLE site_settings (
 --
 
 CREATE SEQUENCE site_settings_id_seq
-    START WITH 1
+    START WITH 4
     INCREMENT BY 1
     NO MINVALUE
     NO MAXVALUE
@@ -1960,7 +1960,7 @@ CREATE TABLE topic_allowed_users (
 --
 
 CREATE SEQUENCE topic_allowed_users_id_seq
-    START WITH 1
+    START WITH 3
     INCREMENT BY 1
     NO MINVALUE
     NO MAXVALUE
@@ -2152,7 +2152,7 @@ CREATE TABLE topics (
 --
 
 CREATE SEQUENCE topics_id_seq
-    START WITH 1
+    START WITH 16
     INCREMENT BY 1
     NO MINVALUE
     NO MAXVALUE
@@ -2258,7 +2258,7 @@ CREATE TABLE user_actions (
 --
 
 CREATE SEQUENCE user_actions_id_seq
-    START WITH 1
+    START WITH 40
     INCREMENT BY 1
     NO MINVALUE
     NO MAXVALUE
@@ -2322,7 +2322,7 @@ CREATE TABLE user_visits (
 --
 
 CREATE SEQUENCE user_visits_id_seq
-    START WITH 1
+    START WITH 4
     INCREMENT BY 1
     NO MINVALUE
     NO MAXVALUE
@@ -2389,7 +2389,7 @@ CREATE TABLE users (
 --
 
 CREATE SEQUENCE users_id_seq
-    START WITH 1
+    START WITH 3
     INCREMENT BY 1
     NO MINVALUE
     NO MAXVALUE

lib/promotion.rb

@@ -13,7 +13,7 @@ class Promotion
     # nil users are never promoted
     return false if @user.blank?
 
-    trust_key = TrustLevel.Levels.invert[@user.trust_level]
+    trust_key = TrustLevel.level_key(@user.trust_level)
 
     review_method = :"review_#{trust_key.to_s}"
     return send(review_method) if respond_to?(review_method)

lib/trust_level.rb

@@ -2,18 +2,33 @@ class TrustLevel
 
   attr_reader :id, :name
 
-  def self.Levels
-    {:new => 0,
-     :basic => 1,
-     :regular => 2,
-     :experienced => 3,
-     :advanced => 4,
-     :moderator => 5}
-  end
+  class << self
+    def levels
+      { new: 0,
+        basic: 1,
+        regular: 2,
+        experienced: 3,
+        advanced: 4,
+        moderator: 5 }
+    end
+    alias_method :Levels, :levels
 
-  def self.all
-    self.Levels.map do |name_key, id|
-      TrustLevel.new(name_key, id)
+    def all
+      levels.map do |name_key, id|
+        TrustLevel.new(name_key, id)
+      end
+    end
+
+    def valid_level?(level)
+      levels.has_key?(level)
+    end
+
+    def compare(current_level, level)
+      (current_level || levels[:new]) >= levels[level]
+    end
+
+    def level_key(level)
+      levels.invert[level]
     end
   end
 
@@ -23,6 +38,6 @@ class TrustLevel
   end
 
   def serializable_hash
-    {id: @id, name: @name}
+    { id: @id, name: @name }
   end
 end

spec/models/user_spec.rb

@@ -87,7 +87,7 @@ describe User do
 
   end
 
-  describe '.approve!' do
+  describe '.approve' do
     let(:user) { Fabricate(:user) }
     let(:admin) { Fabricate(:admin) }
 
@@ -231,7 +231,7 @@ describe User do
       User.new.trust_level.should == TrustLevel.Levels[:advanced]
     end
 
-    describe 'has_trust_level' do
+    describe 'has_trust_level?' do
 
       it "raises an error with an invalid level" do
         lambda { user.has_trust_level?(:wat) }.should raise_error