Merge pull request #1185 from novemberkilo/master

Reduce complexity of TopicView

lib/filter_best_posts.rb

@@ -0,0 +1,62 @@
+class FilterBestPosts
+
+  attr_accessor :filtered_posts, :posts
+
+  def initialize(topic, filtered_posts, limit, options={})
+    @filtered_posts = filtered_posts
+    @topic = topic
+    @limit = limit
+    options.each do |key, value|
+        self.instance_variable_set("@#{key}".to_sym, value)
+    end
+    filter
+  end
+
+  def filter
+    @posts =  if @min_replies && @topic.posts_count < @min_replies + 1
+                []
+              else
+                filter_posts_liked_by_moderators
+                setup_posts
+                filter_posts_based_on_trust_level
+                filter_posts_based_on_score
+                sort_posts
+              end
+  end
+
+  private
+
+  def filter_posts_liked_by_moderators
+    return unless @only_moderator_liked
+    liked_by_moderators = PostAction.where(post_id: @filtered_posts.pluck(:id), post_action_type_id: PostActionType.types[:like])
+    liked_by_moderators = liked_by_moderators.joins(:user).where('users.moderator').pluck(:post_id)
+    @filtered_posts = @filtered_posts.where(id: liked_by_moderators)
+  end
+
+  def setup_posts
+    @posts = @filtered_posts.order('percent_rank asc, sort_order asc').where("post_number > 1")
+    @posts = @posts.includes(:reply_to_user).includes(:topic).joins(:user).limit(@limit)
+  end
+
+  def filter_posts_based_on_trust_level
+    return unless @min_trust_level.try('>',0)
+      @posts = if @bypass_trust_level_score.try('>',0)
+                  @posts.where('COALESCE(users.trust_level,0) >= ? OR posts.score >= ?',
+                              @min_trust_level,
+                              @bypass_trust_level_score)
+                else
+                  @posts.where('COALESCE(users.trust_level,0) >= ?', @min_trust_level)
+                end
+  end
+
+  def filter_posts_based_on_score
+    return unless @min_score.try('>',0)
+    @posts = @posts.where('posts.score >= ?', @min_score)
+  end
+
+  def sort_posts
+    @posts.to_a.sort!{|a,b| a.post_number <=> b.post_number}
+  end
+
+end
+

lib/topic_view.rb

@@ -1,5 +1,6 @@
 require_dependency 'guardian'
 require_dependency 'topic_query'
+require_dependency 'filter_best_posts'
 require_dependency 'summarize'
 
 class TopicView
@@ -10,32 +11,17 @@ class TopicView
   def initialize(topic_id, user=nil, options={})
     @user = user
     @topic = find_topic(topic_id)
-
-    raise Discourse::NotFound if @topic.blank?
-
     @guardian = Guardian.new(@user)
-
+    check_and_raise_exceptions
-    # Special case: If the topic is private and the user isn't logged in, ask them
-    # to log in!
-    if @topic.present? && @topic.private_message? && @user.blank?
-      raise Discourse::NotLoggedIn.new
-    end
-    guardian.ensure_can_see!(@topic)
 
     @post_number, @page = options[:post_number], options[:page].to_i
     @page = 1 if @page == 0
 
     @limit = options[:limit] || SiteSetting.posts_per_page;
-
+    @username_filters = options[:username_filters]
-    @filtered_posts = @topic.posts
+    @best = options[:best]
-    @filtered_posts = @filtered_posts.with_deleted.without_nuked_users if user.try(:staff?)
+    @filter = options[:filter]
-    @filtered_posts = @filtered_posts.best_of if options[:filter] == 'best_of'
+    setup_filtered_posts
-    @filtered_posts = @filtered_posts.where('posts.post_type <> ?', Post.types[:moderator_action]) if options[:best].present?
-
-    if options[:username_filters].present?
-      usernames = options[:username_filters].map{|u| u.downcase}
-      @filtered_posts = @filtered_posts.where('post_number = 1 or user_id in (select u.id from users u where username_lower in (?))', usernames)
-    end
 
     @initial_load = true
     @index_reverse = false
@@ -165,44 +151,9 @@ class TopicView
 
 
   def filter_best(max, opts={})
-    if opts[:min_replies] && @topic.posts_count < opts[:min_replies] + 1
+    filter = FilterBestPosts.new(@topic, @filtered_posts, max, opts)
-      @posts = []
+    @posts = filter.posts
-      return
+    @filtered_posts = filter.filtered_posts
-    end
-
-
-    if opts[:only_moderator_liked]
-      liked_by_moderators = PostAction.where(post_id: @filtered_posts.pluck(:id), post_action_type_id: PostActionType.types[:like])
-      liked_by_moderators = liked_by_moderators.joins(:user).where('users.moderator').pluck(:post_id)
-      @filtered_posts = @filtered_posts.where(id: liked_by_moderators)
-    end
-
-    @posts = @filtered_posts.order('percent_rank asc, sort_order asc').where("post_number > 1")
-    @posts = @posts.includes(:reply_to_user).includes(:topic).joins(:user).limit(max)
-
-    min_trust_level = opts[:min_trust_level]
-    if min_trust_level && min_trust_level > 0
-
-      bypass_trust_level_score = opts[:bypass_trust_level_score]
-
-      if bypass_trust_level_score && bypass_trust_level_score > 0
-        @posts = @posts.where('COALESCE(users.trust_level,0) >= ? OR posts.score >= ?',
-                    min_trust_level,
-                    bypass_trust_level_score
-                 )
-      else
-        @posts = @posts.where('COALESCE(users.trust_level,0) >= ?', min_trust_level)
-      end
-    end
-
-    min_score = opts[:min_score]
-    if min_score && min_score > 0
-      @posts = @posts.where('posts.score >= ?', min_score)
-    end
-
-    @posts = @posts.to_a
-    @posts.sort!{|a,b| a.post_number <=> b.post_number}
-    @posts
   end
 
   def read?(post_number)
@@ -321,4 +272,25 @@ class TopicView
     finder = finder.with_deleted if @user.try(:staff?)
     finder.first
   end
+
+  def setup_filtered_posts
+    @filtered_posts = @topic.posts
+    @filtered_posts = @filtered_posts.with_deleted.without_nuked_users if @user.try(:staff?)
+    @filtered_posts = @filtered_posts.best_of if @filter == 'best_of'
+    @filtered_posts = @filtered_posts.where('posts.post_type <> ?', Post.types[:moderator_action]) if @best.present?
+    return unless @username_filters.present?
+    usernames = @username_filters.map{|u| u.downcase}
+    @filtered_posts = @filtered_posts.where('post_number = 1 or user_id in (select u.id from users u where username_lower in (?))', usernames)
+  end
+
+  def check_and_raise_exceptions
+    raise Discourse::NotFound if @topic.blank?
+    # Special case: If the topic is private and the user isn't logged in, ask them
+    # to log in!
+    if @topic.present? && @topic.private_message? && @user.blank?
+      raise Discourse::NotLoggedIn.new
+    end
+    guardian.ensure_can_see!(@topic)
+  end
+
 end

spec/components/filter_best_posts_spec.rb

@@ -0,0 +1,92 @@
+require 'spec_helper'
+require 'filter_best_posts'
+require 'topic_view'
+
+describe FilterBestPosts do
+
+  let(:topic) { Fabricate(:topic) }
+  let(:coding_horror) { Fabricate(:coding_horror) }
+  let(:first_poster) { topic.user }
+
+  let(:topic_view) { TopicView.new(topic.id, coding_horror) }
+
+  let!(:p1) { Fabricate(:post, topic: topic, user: first_poster, percent_rank: 1 )}
+  let!(:p2) { Fabricate(:post, topic: topic, user: coding_horror, percent_rank: 0.5 )}
+  let!(:p3) { Fabricate(:post, topic: topic, user: first_poster, percent_rank: 0 )}
+
+  let(:moderator) { Fabricate(:moderator) }
+  let(:admin) { Fabricate(:admin) }
+
+  it "can find the best responses" do
+
+    filtered_posts = TopicView.new(topic.id, coding_horror, best: 2).filtered_posts
+    best2 = FilterBestPosts.new(topic, filtered_posts, 2)
+    best2.posts.count.should == 2
+    best2.posts[0].id.should == p2.id
+    best2.posts[1].id.should == p3.id
+
+    topic.update_status('closed', true, Fabricate(:admin))
+    topic.posts.count.should == 4
+  end
+
+  describe "processing options" do
+    before { @filtered_posts = TopicView.new(topic.id, nil, best: 99).filtered_posts }
+
+    it "should not get the status post" do
+
+      best = FilterBestPosts.new(topic, @filtered_posts, 99)
+      best.filtered_posts.size.should == 3
+      best.posts.map(&:id).should =~ [p2.id, p3.id]
+
+    end
+
+    it "should get no results for trust level too low" do
+
+      best = FilterBestPosts.new(topic, @filtered_posts, 99, min_trust_level: coding_horror.trust_level + 1)
+      best.posts.count.should == 0
+    end
+
+
+    it "should filter out the posts with a score that is too low" do
+
+      best = FilterBestPosts.new(topic, @filtered_posts, 99, min_score: 99)
+      best.posts.count.should == 0
+    end
+
+
+    it "should filter out everything if min replies not met" do
+      best = FilterBestPosts.new(topic, @filtered_posts, 99, min_replies: 99)
+      best.posts.count.should == 0
+    end
+
+    it "should punch through posts if the score is high enough" do
+      p2.update_column(:score, 100)
+
+      best = FilterBestPosts.new(topic, @filtered_posts, 99, bypass_trust_level_score: 100, min_trust_level: coding_horror.trust_level + 1)
+      best.posts.count.should == 1
+    end
+
+    it "should bypass trust level score" do
+      best = FilterBestPosts.new(topic, @filtered_posts, 99, bypass_trust_level_score: 0, min_trust_level: coding_horror.trust_level + 1)
+      best.posts.count.should == 0
+    end
+
+    it "should return none if restricted to posts a moderator liked" do
+      best = FilterBestPosts.new(topic, @filtered_posts, 99, only_moderator_liked: true)
+      best.posts.count.should == 0
+    end
+
+    it "doesn't count likes from admins" do
+      PostAction.act(admin, p3, PostActionType.types[:like])
+      best = FilterBestPosts.new(topic, @filtered_posts, 99, only_moderator_liked: true)
+      best.posts.count.should == 0
+    end
+
+    it "should find the post liked by the moderator" do
+      PostAction.act(moderator, p2, PostActionType.types[:like])
+      best = FilterBestPosts.new(topic, @filtered_posts, 99, only_moderator_liked: true)
+      best.posts.count.should == 1
+    end
+
+  end
+end