TopicQuery cleanup in advance of custom sorting:

- Move SQL method constants into a module
- Removed unused count methods
- Moved methods that don't return a TopicList into Topic
- Replaced some confusing method signatures

app/controllers/application_controller.rb

@@ -274,8 +274,8 @@ class ApplicationController < ActionController::Base
     end
 
     def build_not_found_page(status=404, layout=false)
-      @top_viewed = TopicQuery.top_viewed(10)
+      @top_viewed = Topic.top_viewed(10)
-      @recent = TopicQuery.recent(10)
+      @recent = Topic.recent(10)
       @slug =  params[:slug].class == String ? params[:slug] : ''
       @slug =  (params[:id].class == String ? params[:id] : '') if @slug.blank?
       @slug.gsub!('-',' ')

app/models/topic.rb

@@ -184,6 +184,14 @@ class Topic < ActiveRecord::Base
     end
   end
 
+  def self.top_viewed(max = 10)
+    Topic.listable_topics.visible.secured.order('views desc').limit(max)
+  end
+
+  def self.recent(max = 10)
+    Topic.listable_topics.visible.secured.order('created_at desc').limit(max)
+  end
+
   def self.count_exceeds_minimum?
     count > SiteSetting.minimum_topics_similar
   end

lib/topic_query.rb

@@ -4,6 +4,7 @@
 #
 require_dependency 'topic_list'
 require_dependency 'suggested_topics_builder'
+require_dependency 'topic_query_sql'
 
 class TopicQuery
   # Could be rewritten to %i if Ruby 1.9 is no longer supported
@@ -18,71 +19,6 @@ class TopicQuery
                      sort_order
                      sort_descending).map(&:to_sym)
 
-  class << self
-    # use the constants in conjuction with COALESCE to determine the order with regard to pinned
-    # topics that have been cleared by the user. There
-    # might be a cleaner way to do this.
-    def lowest_date
-      "2010-01-01"
-    end
-
-    def highest_date
-      "3000-01-01"
-    end
-
-    # If you've clearned the pin, use bumped_at, otherwise put it at the top
-    def order_with_pinned_sql
-      "CASE
-        WHEN (COALESCE(topics.pinned_at, '#{lowest_date}') > COALESCE(tu.cleared_pinned_at, '#{lowest_date}'))
-          THEN '#{highest_date}'
-        ELSE topics.bumped_at
-       END DESC"
-    end
-
-    def order_hotness(user)
-      if user
-        # When logged in take into accounts what pins you've closed
-        "CASE
-          WHEN (COALESCE(topics.pinned_at, '#{lowest_date}') > COALESCE(tu.cleared_pinned_at, '#{lowest_date}'))
-            THEN 100
-          ELSE hot_topics.score + (COALESCE(categories.hotness, 5.0) / 11.0)
-         END DESC"
-      else
-        # When anonymous, don't use topic_user
-        "CASE
-          WHEN topics.pinned_at IS NOT NULL THEN 100
-          ELSE hot_topics.score + (COALESCE(categories.hotness, 5.0) / 11.0)
-        END DESC"
-      end
-    end
-
-    # If you've clearned the pin, use bumped_at, otherwise put it at the top
-    def order_nocategory_with_pinned_sql
-      "CASE
-        WHEN topics.category_id = #{SiteSetting.uncategorized_category_id.to_i} and (COALESCE(topics.pinned_at, '#{lowest_date}') > COALESCE(tu.cleared_pinned_at, '#{lowest_date}'))
-          THEN '#{highest_date}'
-        ELSE topics.bumped_at
-       END DESC"
-    end
-
-    # For anonymous users
-    def order_nocategory_basic_bumped
-      "CASE WHEN topics.category_id = #{SiteSetting.uncategorized_category_id.to_i} and (topics.pinned_at IS NOT NULL) THEN 0 ELSE 1 END, topics.bumped_at DESC"
-    end
-
-    def order_basic_bumped
-      "CASE WHEN (topics.pinned_at IS NOT NULL) THEN 0 ELSE 1 END, topics.bumped_at DESC"
-    end
-
-    def top_viewed(max = 10)
-      Topic.listable_topics.visible.secured.order('views desc').limit(max)
-    end
-
-    def recent(max = 10)
-      Topic.listable_topics.visible.secured.order('created_at desc').limit(max)
-    end
-  end
-
   def initialize(user=nil, options={})
     options.assert_valid_keys(VALID_OPTIONS)
 
@@ -122,16 +58,16 @@ class TopicQuery
 
   def list_hot
     create_list(:hot, unordered: true) do |topics|
-      topics.joins(:hot_topic).order(TopicQuery.order_hotness(@user))
+      topics.joins(:hot_topic).order(TopicQuerySQL.order_hotness(@user))
     end
   end
 
   def list_new
-    create_list(:new, {}, new_results)
+    TopicList.new(:new, @user, new_results)
   end
 
   def list_unread
-    create_list(:unread, {}, unread_results)
+    TopicList.new(:new, @user, unread_results)
   end
 
   def list_posted
@@ -165,9 +101,9 @@ class TopicQuery
     create_list(:category, unordered: true) do |list|
       list = list.where(category_id: category.id)
       if @user
-        list.order(TopicQuery.order_with_pinned_sql)
+        list.order(TopicQuerySQL.order_with_pinned_sql)
       else
-        list.order(TopicQuery.order_basic_bumped)
+        list.order(TopicQuerySQL.order_basic_bumped)
       end
     end
   end
@@ -187,14 +123,6 @@ class TopicQuery
         .where("COALESCE(tu.notification_level, :regular) >= :tracking", regular: TopicUser.notification_levels[:regular], tracking: TopicUser.notification_levels[:tracking])
   end
 
-  def unread_count
-    unread_results(limit: false).count
-  end
-
-  def new_count
-    new_results(limit: false).count
-  end
-
   protected
 
     def create_list(filter, options={}, topics = nil)
@@ -209,9 +137,9 @@ class TopicQuery
 
       # Start with a list of all topics
       result = Topic.where(id: TopicAllowedUser.where(user_id: user.id).pluck(:topic_id))
-      result = result.joins("LEFT OUTER JOIN topic_users AS tu ON (topics.id = tu.topic_id AND tu.user_id = #{user.id.to_i})")
+                    .joins("LEFT OUTER JOIN topic_users AS tu ON (topics.id = tu.topic_id AND tu.user_id = #{user.id.to_i})")
-      result = result.order(TopicQuery.order_nocategory_basic_bumped)
+                    .order(TopicQuerySQL.order_nocategory_basic_bumped)
-      result = result.private_messages
+                    .private_messages
 
       result = result.limit(options[:per_page]) unless options[:limit] == false
       result = result.visible if options[:visible] || @user.nil? || @user.regular?
@@ -245,10 +173,10 @@ class TopicQuery
       unless options[:unordered]
         # If we're logged in, we have to pay attention to our pinned settings
         if @user
-          result = category_id.nil? ? result.order(TopicQuery.order_nocategory_with_pinned_sql) :
+          result = category_id.nil? ? result.order(TopicQuerySQL.order_nocategory_with_pinned_sql) :
-                                      result.order(TopicQuery.order_with_pinned_sql)
+                                      result.order(TopicQuerySQL.order_with_pinned_sql)
         else
-          result = result.order(TopicQuery.order_nocategory_basic_bumped)
+          result = result.order(TopicQuerySQL.order_nocategory_basic_bumped)
         end
       end
 
@@ -310,6 +238,6 @@ class TopicQuery
         result = result.order("CASE WHEN topics.category_id = #{options[:topic].category_id.to_i} THEN 0 ELSE 1 END")
       end
 
-      result.order(TopicQuery.order_nocategory_with_pinned_sql)
+      result.order(TopicQuerySQL.order_nocategory_with_pinned_sql)
     end
 end

lib/topic_query_sql.rb

@@ -0,0 +1,72 @@
+#
+#  SQL fragments used when querying a list of topics.
+#
+module TopicQuerySQL
+
+  class << self
+
+    # use the constants in conjuction with COALESCE to determine the order with regard to pinned
+    # topics that have been cleared by the user. There
+    # might be a cleaner way to do this.
+    def lowest_date
+      "2010-01-01"
+    end
+
+    def highest_date
+      "3000-01-01"
+    end
+
+    # If you've clearned the pin, use bumped_at, otherwise put it at the top
+    def order_with_pinned_sql
+      "CASE
+        WHEN (COALESCE(topics.pinned_at, '#{lowest_date}') > COALESCE(tu.cleared_pinned_at, '#{lowest_date}'))
+          THEN '#{highest_date}'
+        ELSE topics.bumped_at
+       END DESC"
+    end
+
+    def order_hotness(user)
+      if user
+        # When logged in take into accounts what pins you've closed
+        "CASE
+          WHEN (COALESCE(topics.pinned_at, '#{lowest_date}') > COALESCE(tu.cleared_pinned_at, '#{lowest_date}'))
+            THEN 100
+          ELSE hot_topics.score + (COALESCE(categories.hotness, 5.0) / 11.0)
+         END DESC"
+      else
+        # When anonymous, don't use topic_user
+        "CASE
+          WHEN topics.pinned_at IS NOT NULL THEN 100
+          ELSE hot_topics.score + (COALESCE(categories.hotness, 5.0) / 11.0)
+        END DESC"
+      end
+    end
+
+    # If you've clearned the pin, use bumped_at, otherwise put it at the top
+    def order_nocategory_with_pinned_sql
+      "CASE
+        WHEN topics.category_id = #{SiteSetting.uncategorized_category_id.to_i} and (COALESCE(topics.pinned_at, '#{lowest_date}') > COALESCE(tu.cleared_pinned_at, '#{lowest_date}'))
+          THEN '#{highest_date}'
+        ELSE topics.bumped_at
+       END DESC"
+    end
+
+    # For anonymous users
+    def order_nocategory_basic_bumped
+      "CASE WHEN topics.category_id = #{SiteSetting.uncategorized_category_id.to_i} and (topics.pinned_at IS NOT NULL) THEN 0 ELSE 1 END, topics.bumped_at DESC"
+    end
+
+    def order_basic_bumped
+      "CASE WHEN (topics.pinned_at IS NOT NULL) THEN 0 ELSE 1 END, topics.bumped_at DESC"
+    end
+
+    def order_with_pinned_sql
+      "CASE
+        WHEN (COALESCE(topics.pinned_at, '#{lowest_date}') > COALESCE(tu.cleared_pinned_at, '#{lowest_date}'))
+          THEN '#{highest_date}'
+        ELSE topics.bumped_at
+       END DESC"
+    end
+
+  end
+end

spec/components/topic_query_spec.rb

@@ -24,8 +24,8 @@ describe TopicQuery do
       TopicQuery.new(nil).list_latest.topics.count.should == 0
       TopicQuery.new(user).list_latest.topics.count.should == 0
 
-      TopicQuery.top_viewed(10).count.should == 0
+      Topic.top_viewed(10).count.should == 0
-      TopicQuery.recent(10).count.should == 0
+      Topic.recent(10).count.should == 0
 
       # mods can see every group and hidden topics
       TopicQuery.new(moderator).list_latest.topics.count.should == 3
@@ -119,7 +119,6 @@ describe TopicQuery do
     context 'with no data' do
       it "has no unread topics" do
         topic_query.list_unread.topics.should be_blank
-        topic_query.unread_count.should == 0
       end
     end
 
@@ -135,7 +134,6 @@ describe TopicQuery do
       context 'list_unread' do
         it 'contains no topics' do
           topic_query.list_unread.topics.should == []
-          topic_query.unread_count.should == 0
         end
       end
 
@@ -148,10 +146,6 @@ describe TopicQuery do
         it 'only contains the partially read topic' do
           topic_query.list_unread.topics.should == [partially_read]
         end
-
-        it "returns 1 as the unread count" do
-          topic_query.unread_count.should == 1
-        end
       end
 
       context 'list_read' do
@@ -187,10 +181,6 @@ describe TopicQuery do
   context 'list_new' do
 
     context 'without a new topic' do
-      it "has an new_count of 0" do
-        topic_query.new_count.should == 0
-      end
-
       it "has no new topics" do
         topic_query.list_new.topics.should be_blank
       end