From 12d664a610cfaddbb3ab73ec8da35b15b337ac73 Mon Sep 17 00:00:00 2001
From: Gosha Arinich <me@goshakkk.name>
Date: Tue, 26 Feb 2013 19:27:59 +0300
Subject: [PATCH] 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 |  6 ++----
 app/models/site_setting.rb                      |  4 ++++
 app/models/topic.rb                             | 15 +++++----------
 app/models/user.rb                              |  6 +++++-
 spec/models/site_setting_spec.rb                |  9 +++++++++
 5 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/app/controllers/admin/impersonate_controller.rb b/app/controllers/admin/impersonate_controller.rb
index 0ebe1d12a..6cdea0077 100644
--- a/app/controllers/admin/impersonate_controller.rb
+++ b/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(?)',
-                        params[:username_or_email],
-                        params[:username_or_email],
-                        params[:username_or_email]]).first
+    user = User.find_by_username_or_email(params[:username_or_email]).first
+
     raise Discourse::NotFound if user.blank?
 
     guardian.ensure_can_impersonate!(user)
diff --git a/app/models/site_setting.rb b/app/models/site_setting.rb
index d4090f04f..d4e3437b8 100644
--- a/app/models/site_setting.rb
+++ b/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
diff --git a/app/models/topic.rb b/app/models/topic.rb
index 3fc6b7ead..ab100244e 100644
--- a/app/models/topic.rb
+++ b/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?
-    return true if category_id_changed?
-    false
+    title_changed? || category_id_changed?
   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)
-    return "topic" if result.blank?
-    result
+    Slug.for(title).presence || "topic"
   end
 
   def last_post_url
diff --git a/app/models/user.rb b/app/models/user.rb
index 9626cc338..16391ed43 100644
--- a/app/models/user.rb
+++ b/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
diff --git a/spec/models/site_setting_spec.rb b/spec/models/site_setting_spec.rb
index e909866b9..600999a94 100644
--- a/spec/models/site_setting_spec.rb
+++ b/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