refactor Topic

* move finding by username/email to User
* make SiteSetting return a range of possible post title lengths
* remove unnecessary conditions

app/controllers/admin/impersonate_controller.rb

@@ -3,10 +3,8 @@ class Admin::ImpersonateController < Admin::AdminController
   def create
     requires_parameters(:username_or_email)
 
-    user = User.where(['username_lower = lower(?) or lower(email) = lower(?) or lower(name) = lower(?)',
+    user = User.find_by_username_or_email(params[:username_or_email]).first
-                        params[:username_or_email],
+
-                        params[:username_or_email],
-                        params[:username_or_email]]).first
     raise Discourse::NotFound if user.blank?
 
     guardian.ensure_can_impersonate!(user)

app/models/site_setting.rb

@@ -144,4 +144,8 @@ class SiteSetting < ActiveRecord::Base
     self.enforce_global_nicknames? and self.discourse_org_access_key.present?
   end
 
+  def self.topic_title_length
+    min_topic_title_length..max_topic_title_length
+  end
+
 end

app/models/topic.rb

@@ -22,7 +22,7 @@ class Topic < ActiveRecord::Base
 
   validate :title_quality
   validates_presence_of :title
-  validates :title, length: { in: SiteSetting.min_topic_title_length..SiteSetting.max_topic_title_length }
+  validates :title, length: { in: SiteSetting.topic_title_length }
 
   serialize :meta_data, ActiveRecord::Coders::Hstore
 
@@ -137,14 +137,12 @@ class Topic < ActiveRecord::Base
       # It's possible the sentinel has cleaned up the title a bit
       self.title = sentinel.text
     else
-      errors.add(:title, I18n.t(:is_invalid)) unless sentinel.valid?
+      errors.add(:title, I18n.t(:is_invalid))
     end
   end
 
   def new_version_required?
-    return true if title_changed?
+    title_changed? || category_id_changed?
-    return true if category_id_changed?
-    false
   end
 
   # Returns new topics since a date for display in email digest.
@@ -332,7 +330,7 @@ class Topic < ActiveRecord::Base
   def invite(invited_by, username_or_email)
     if private_message?
       # If the user exists, add them to the topic.
-      user = User.where("lower(username) = :user OR lower(email) = :user", user: username_or_email.downcase).first
+      user = User.find_by_username_or_email(username_or_email).first
       if user.present?
         if topic_allowed_users.create!(user_id: user.id)
           # Notify the user they've been invited
@@ -485,7 +483,6 @@ class Topic < ActiveRecord::Base
 
   # Enable/disable the star on the topic
   def toggle_star(user, starred)
-
     Topic.transaction do
       TopicUser.change(user, self.id, starred: starred, starred_at: starred ? DateTime.now : nil)
 
@@ -511,9 +508,7 @@ class Topic < ActiveRecord::Base
   end
 
   def slug
-    result = Slug.for(title)
+    Slug.for(title).presence || "topic"
-    return "topic" if result.blank?
-    result
   end
 
   def last_post_url

app/models/user.rb

@@ -166,10 +166,14 @@ class User < ActiveRecord::Base
   def self.find_by_temporary_key(key)
     user_id = $redis.get("temporary_key:#{key}")
     if user_id.present?
-      User.where(id: user_id.to_i).first
+      where(id: user_id.to_i).first
     end
   end
 
+  def self.find_by_username_or_email(username_or_email)
+    where("username_lower = :user or lower(username) = :user or lower(email) = :user or lower(name) = :user", user: username_or_email.downcase)
+  end
+
   # tricky, we need our bus to be subscribed from the right spot
   def sync_notification_channel_position
     @unread_notifications_by_type = nil

spec/models/site_setting_spec.rb

@@ -107,4 +107,13 @@ describe SiteSetting do
     end
   end
 
+  describe 'topic_title_length' do
+    it 'returns a range of min/max topic title length' do
+      SiteSetting.min_topic_title_length = 1
+      SiteSetting.max_topic_title_length = 2
+
+      SiteSetting.topic_title_length.should == (1..2)
+    end
+  end
+
 end