From 2efd3e61c73be812367e6584e846ef0b30c4ca20 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Tue, 26 Mar 2013 11:58:49 -0400 Subject: [PATCH] TopicView respects `sort_order` and better specs --- lib/topic_view.rb | 160 +++++------ spec/components/topic_view_spec.rb | 447 +++++++++++++---------------- 2 files changed, 280 insertions(+), 327 deletions(-) diff --git a/lib/topic_view.rb b/lib/topic_view.rb index fc1f07ab39f..c6ef6e6c2a7 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -4,7 +4,7 @@ require_dependency 'summarize' class TopicView - attr_accessor :topic, :min, :max, :draft, :draft_key, :draft_sequence, :posts + attr_accessor :topic, :draft, :draft_key, :draft_sequence, :posts def initialize(topic_id, user=nil, options={}) @topic = find_topic(topic_id) @@ -17,24 +17,20 @@ class TopicView end Guardian.new(user).ensure_can_see!(@topic) - @min, @max = 1, SiteSetting.posts_per_page @post_number, @page = options[:post_number], options[:page] - @posts = @topic.posts - @posts = @posts.with_deleted if user.try(:admin?) - @posts = @posts.best_of if options[:best_of].present? + @filtered_posts = @topic.posts + @filtered_posts = @filtered_posts.with_deleted if user.try(:admin?) + @filtered_posts = @filtered_posts.best_of if options[:best_of].present? if options[:username_filters].present? usernames = options[:username_filters].map{|u| u.downcase} - @posts = @posts.where('post_number = 1 or user_id in (select u.id from users u where username_lower in (?))', usernames) + @filtered_posts = @filtered_posts.where('post_number = 1 or user_id in (select u.id from users u where username_lower in (?))', usernames) end @user = user @initial_load = true - @all_posts = @posts - - filter_posts(options) @draft_key = @topic.draft_key @@ -53,9 +49,9 @@ class TopicView end def next_page - last_post = @posts.last + last_post = @filtered_posts.last if last_post.present? && (@topic.highest_post_number > last_post.post_number) - (@posts[0].post_number / SiteSetting.posts_per_page) + 1 + (@filtered_posts[0].post_number / SiteSetting.posts_per_page) + 1 end end @@ -86,64 +82,80 @@ class TopicView end def filter_posts(opts = {}) - if opts[:post_number].present? - # Get posts near a post - filter_posts_near(opts[:post_number].to_i) - elsif opts[:posts_before].present? - filter_posts_before(opts[:posts_before].to_i) - elsif opts[:posts_after].present? - filter_posts_after(opts[:posts_after].to_i) - else - # No filter? Consider it a paged view, default to page 0 which is the first segment - filter_posts_paged(opts[:page].to_i) - end + return filter_posts_near(opts[:post_number].to_i) if opts[:post_number].present? + return filter_posts_before(opts[:posts_before].to_i) if opts[:posts_before].present? + return filter_posts_after(opts[:posts_after].to_i) if opts[:posts_after].present? + filter_posts_paged(opts[:page].to_i) + end + + # Find the sort order for a post in the topic + def sort_order_for_post_number(post_number) + Post.where(topic_id: @topic.id, post_number: post_number) + .with_deleted + .first + .try(:sort_order) end # Filter to all posts near a particular post number def filter_posts_near(post_number) - @min, @max = post_range(post_number) - filter_posts_in_range(@min, @max) + + # Find the closest number we have + closest_post_id = @filtered_posts.order("@(post_number - #{post_number})").first.try(:id) + return nil if closest_post_id.blank? + + closest_index = filtered_post_ids.index(closest_post_id) + return nil if closest_index.blank? + + # Make sure to get at least one post before, even with rounding + posts_before = (SiteSetting.posts_per_page.to_f / 4).floor + posts_before = 1 if posts_before == 0 + + min_idx = closest_index - posts_before + min_idx = 0 if min_idx < 0 + max_idx = min_idx + (SiteSetting.posts_per_page - 1) + + # Get a full page even if at the end + upper_limit = (filtered_post_ids.length - 1) + if max_idx >= upper_limit + max_idx = upper_limit + min_idx = (upper_limit - SiteSetting.posts_per_page) + 1 + end + + filter_posts_in_range(min_idx, max_idx) end - def post_numbers - @post_numbers ||= @posts.order(:post_number).pluck(:post_number) + def filtered_post_ids + @filtered_post_ids ||= @filtered_posts.order(:sort_order).pluck(:id) end def filter_posts_paged(page) page ||= 0 min = (SiteSetting.posts_per_page * page) max = min + SiteSetting.posts_per_page - - max_val = (post_numbers.length - 1) - - # If we're off the charts, return nil - return nil if min > max_val - - # Pin max to the last post - max = max_val if max > max_val - - filter_posts_in_range(post_numbers[min], post_numbers[max]) + filter_posts_in_range(min, max) end # Filter to all posts before a particular post number def filter_posts_before(post_number) @initial_load = false - @max = post_number - 1 - @posts = @posts.reverse_order.where("post_number < ?", post_number) + sort_order = sort_order_for_post_number(post_number) + return nil unless sort_order + # Find posts before the `sort_order` + @posts = @filtered_posts.order('sort_order desc').where("sort_order < ?", sort_order) @posts = @posts.includes(:reply_to_user).includes(:topic).joins(:user).limit(SiteSetting.posts_per_page) - @min = @max - @posts.size - @min = 1 if @min < 1 end # Filter to all posts after a particular post number def filter_posts_after(post_number) @initial_load = false - @min = post_number - @posts = @posts.regular_order.where("post_number > ?", post_number) + + sort_order = sort_order_for_post_number(post_number) + return nil unless sort_order + + @posts = @filtered_posts.order('sort_order').where("sort_order > ?", sort_order) @posts = @posts.includes(:reply_to_user).includes(:topic).joins(:user).limit(SiteSetting.posts_per_page) - @max = @min + @posts.size end def read?(post_number) @@ -202,35 +214,6 @@ class TopicView @link_counts ||= TopicLinkClick.counts_for(@topic, posts) end - # Binary search for closest value - def self.closest(array, target, min, max) - return min if max <= min - return max if (max - min) == 1 - - middle_idx = ((min + max) / 2).floor - middle_val = array[middle_idx] - - return middle_idx if target == middle_val - return closest(array, target, min, middle_idx) if middle_val > target - return closest(array, target, middle_idx, max) - end - - # Find a range of posts, allowing for gaps by deleted posts. - def post_range(post_number) - closest_index = TopicView.closest(post_numbers, post_number, 0, post_numbers.size - 1) - - min_idx = closest_index - (SiteSetting.posts_per_page.to_f / 4).floor - min_idx = 0 if min_idx < 0 - max_idx = min_idx + (SiteSetting.posts_per_page - 1) - if max_idx > (post_numbers.length - 1) - max_idx = post_numbers.length - 1 - min_idx = max_idx - SiteSetting.posts_per_page - min_idx = 0 if min_idx < 0 - end - - [post_numbers[min_idx], post_numbers[max_idx]] - end - # Are we the initial page load? If so, we can return extra information like # user post counts, etc. def initial_load? @@ -248,11 +231,11 @@ class TopicView # the end of the stream (for mods), nor is it correct for filtered # streams def highest_post_number - @highest_post_number ||= @all_posts.maximum(:post_number) + @highest_post_number ||= @filtered_posts.maximum(:post_number) end def recent_posts - @all_posts.by_newest.with_user.first(25) + @filtered_posts.by_newest.with_user.first(25) end protected @@ -263,12 +246,12 @@ class TopicView return result unless @user.present? return result unless topic_user.present? - posts_max = @max > (topic_user.last_read_post_number || 1 ) ? (topic_user.last_read_post_number || 1) : @max + post_numbers = PostTiming.select(:post_number) + .where(topic_id: @topic.id, user_id: @user.id) + .where(post_number: @posts.pluck(:post_number)) + .pluck(:post_number) - PostTiming.select(:post_number) - .where("topic_id = ? AND user_id = ? AND post_number BETWEEN ? AND ?", - @topic.id, @user.id, @min, posts_max) - .each {|t| result << t.post_number} + post_numbers.each {|pn| result << pn} result end end @@ -276,8 +259,23 @@ class TopicView private def filter_posts_in_range(min, max) - @min, @max = min, max - @posts = @posts.where("post_number between ? and ?", @min, @max).includes(:user).includes(:reply_to_user).regular_order + max_index = (filtered_post_ids.length - 1) + + # If we're off the charts, return nil + return nil if min > max_index + + # Pin max to the last post + max = max_index if max > max_index + min = 0 if min < 0 + + # TODO: Sort might be off + @posts = Post.where(id: filtered_post_ids[min..max]) + .includes(:user) + .includes(:reply_to_user) + .order('sort_order') + @posts = @posts.with_deleted if @user.try(:admin?) + + @posts end def find_topic(topic_id) diff --git a/spec/components/topic_view_spec.rb b/spec/components/topic_view_spec.rb index 40ae99720fc..b591c032378 100644 --- a/spec/components/topic_view_spec.rb +++ b/spec/components/topic_view_spec.rb @@ -6,9 +6,7 @@ describe TopicView do let(:topic) { Fabricate(:topic) } let(:coding_horror) { Fabricate(:coding_horror) } let(:first_poster) { topic.user } - let!(:p1) { Fabricate(:post, topic: topic, user: first_poster )} - let!(:p2) { Fabricate(:post, topic: topic, user: coding_horror )} - let!(:p3) { Fabricate(:post, topic: topic, user: first_poster )} + let(:topic_view) { TopicView.new(topic.id, coding_horror) } @@ -21,293 +19,250 @@ describe TopicView do lambda { topic_view }.should raise_error(Discourse::InvalidAccess) end + context "with a few sample posts" do + let!(:p1) { Fabricate(:post, topic: topic, user: first_poster )} + let!(:p2) { Fabricate(:post, topic: topic, user: coding_horror )} + let!(:p3) { Fabricate(:post, topic: topic, user: first_poster )} + + it "raises NotLoggedIn if the user isn't logged in and is trying to view a private message" do Topic.any_instance.expects(:private_message?).returns(true) lambda { TopicView.new(topic.id, nil) }.should raise_error(Discourse::NotLoggedIn) end - it "provides an absolute url" do - topic_view.absolute_url.should be_present - end - - it "provides a summary of the first post" do - topic_view.summary.should be_present - end - - describe "#get_canonical_path" do - let(:user) { Fabricate(:user) } - let(:topic) { Fabricate(:topic) } - let(:path) { "/1234" } - - before do - topic.expects(:relative_url).returns(path) - described_class.any_instance.expects(:find_topic).with(1234).returns(topic) + it "provides an absolute url" do + topic_view.absolute_url.should be_present end - context "without a post number" do - context "without a page" do - it "generates a canonical path for a topic" do - described_class.new(1234, user).canonical_path.should eql(path) - end + it "provides a summary of the first post" do + topic_view.summary.should be_present + end + + describe "#get_canonical_path" do + let(:user) { Fabricate(:user) } + let(:topic) { Fabricate(:topic) } + let(:path) { "/1234" } + + before do + topic.expects(:relative_url).returns(path) + described_class.any_instance.expects(:find_topic).with(1234).returns(topic) end - context "with a page" do - let(:path_with_page) { "/1234?page=5" } + context "without a post number" do + context "without a page" do + it "generates a canonical path for a topic" do + described_class.new(1234, user).canonical_path.should eql(path) + end + end + + context "with a page" do + let(:path_with_page) { "/1234?page=5" } + + it "generates a canonical path for a topic" do + described_class.new(1234, user, page: 5).canonical_path.should eql(path_with_page) + end + end + end + context "with a post number" do + let(:path_with_page) { "/1234?page=10" } + before { SiteSetting.stubs(:posts_per_page).returns(5) } it "generates a canonical path for a topic" do - described_class.new(1234, user, page: 5).canonical_path.should eql(path_with_page) + described_class.new(1234, user, post_number: 50).canonical_path.should eql(path_with_page) end end end - context "with a post number" do - let(:path_with_page) { "/1234?page=10" } - before { SiteSetting.stubs(:posts_per_page).returns(5) } - it "generates a canonical path for a topic" do - described_class.new(1234, user, post_number: 50).canonical_path.should eql(path_with_page) + describe "#next_page" do + let(:posts) { [stub(post_number: 1), stub(post_number: 2)] } + let(:topic) do + topic = Fabricate(:topic) + topic.stubs(:posts).returns(posts) + topic.stubs(:highest_post_number).returns(5) + topic + end + let(:user) { Fabricate(:user) } + + before do + described_class.any_instance.expects(:find_topic).with(1234).returns(topic) + described_class.any_instance.stubs(:filter_posts) + SiteSetting.stubs(:posts_per_page).returns(2) + end + + it "should return the next page" do + described_class.new(1234, user).next_page.should eql(1) end end - end - describe "#next_page" do - let(:posts) { [stub(post_number: 1), stub(post_number: 2)] } - let(:topic) do - topic = Fabricate(:topic) - topic.stubs(:posts).returns(posts) - topic.stubs(:highest_post_number).returns(5) - topic - end - let(:user) { Fabricate(:user) } - - before do - described_class.any_instance.expects(:find_topic).with(1234).returns(topic) - described_class.any_instance.stubs(:filter_posts) - SiteSetting.stubs(:posts_per_page).returns(2) + context '.posts_count' do + it 'returns the two posters with their counts' do + topic_view.posts_count.to_a.should =~ [[first_poster.id, 2], [coding_horror.id, 1]] + end end - it "should return the next page" do - described_class.new(1234, user).next_page.should eql(1) - end - end - - context '.posts_count' do - it 'returns the two posters with their counts' do - topic_view.posts_count.to_a.should =~ [[first_poster.id, 2], [coding_horror.id, 1]] - end - end - - context '.participants' do - it 'returns the two participants hashed by id' do - topic_view.participants.to_a.should =~ [[first_poster.id, first_poster], [coding_horror.id, coding_horror]] - end - end - - context '.all_post_actions' do - it 'is blank at first' do - topic_view.all_post_actions.should be_blank + context '.participants' do + it 'returns the two participants hashed by id' do + topic_view.participants.to_a.should =~ [[first_poster.id, first_poster], [coding_horror.id, coding_horror]] + end end - it 'returns the like' do - PostAction.act(coding_horror, p1, PostActionType.types[:like]) - topic_view.all_post_actions[p1.id][PostActionType.types[:like]].should be_present - end - end + context '.all_post_actions' do + it 'is blank at first' do + topic_view.all_post_actions.should be_blank + end - it 'allows admins to see deleted posts' do - post_number = p3.post_number - p3.destroy - admin = Fabricate(:admin) - topic_view = TopicView.new(topic.id, admin) - topic_view.posts.count.should == 3 - topic_view.highest_post_number.should == post_number - end - - it 'does not allow non admins to see deleted posts' do - p3.destroy - topic_view.posts.count.should == 2 - end - - # Sam: disabled for now, we only need this for polls, if we do, roll it into topic - # having to walk every post action is not really a good idea - # - # context '.voted_in_topic?' do - # it "is false when the user hasn't voted" do - # topic_view.voted_in_topic?.should be_false - # end - - # it "is true when the user has voted for a post" do - # PostAction.act(coding_horror, p1, PostActionType.types[:vote]) - # topic_view.voted_in_topic?.should be_true - # end - # end - - context '.post_action_visibility' do - - it "is allows users to see likes" do - topic_view.post_action_visibility.include?(PostActionType.types[:like]).should be_true + it 'returns the like' do + PostAction.act(coding_horror, p1, PostActionType.types[:like]) + topic_view.all_post_actions[p1.id][PostActionType.types[:like]].should be_present + end end - end - - context '.read?' do - it 'is unread with no logged in user' do - TopicView.new(topic.id).read?(1).should be_false + context '.post_action_visibility' do + it "is allows users to see likes" do + topic_view.post_action_visibility.include?(PostActionType.types[:like]).should be_true + end end - it 'makes posts as unread by default' do - topic_view.read?(1).should be_false + context '.read?' do + it 'is unread with no logged in user' do + TopicView.new(topic.id).read?(1).should be_false + end + + it 'makes posts as unread by default' do + topic_view.read?(1).should be_false + end + + it 'knows a post is read when it has a PostTiming' do + PostTiming.create(topic: topic, user: coding_horror, post_number: 1, msecs: 1000) + topic_view.read?(1).should be_true + end end - it 'knows a post is read when it has a PostTiming' do - PostTiming.create(topic: topic, user: coding_horror, post_number: 1, msecs: 1000) - topic_view.read?(1).should be_true - end - end + context '.topic_user' do + it 'returns nil when there is no user' do + TopicView.new(topic.id, nil).topic_user.should be_blank + end - context '.topic_user' do - it 'returns nil when there is no user' do - TopicView.new(topic.id, nil).topic_user.should be_blank + it 'returns a record once the user has some data' do + TopicView.new(topic.id, coding_horror).topic_user.should be_present + end end - it 'returns a record once the user has some data' do - TopicView.new(topic.id, coding_horror).topic_user.should be_present + context '#recent_posts' do + before do + 24.times do # our let()s have already created 3 + Fabricate(:post, topic: topic, user: first_poster) + end + end + it 'returns at most 25 recent posts ordered newest first' do + recent_posts = topic_view.recent_posts + + # count + recent_posts.count.should == 25 + + # ordering + recent_posts.include?(p1).should be_false + recent_posts.include?(p3).should be_true + recent_posts.first.created_at.should > recent_posts.last.created_at + end end + end context '.posts' do - context 'near a post_number' do - let (:near_topic_view) { TopicView.new(topic.id, coding_horror, post_number: p2.post_number) } + # Create the posts in a different order than the sort_order + let!(:p5) { Fabricate(:post, topic: topic, user: coding_horror)} + let!(:p2) { Fabricate(:post, topic: topic, user: coding_horror)} + let!(:p4) { Fabricate(:post, topic: topic, user: coding_horror, deleted_at: Time.now)} + let!(:p1) { Fabricate(:post, topic: topic, user: first_poster)} + let!(:p3) { Fabricate(:post, topic: topic, user: first_poster)} - it 'returns posts around a post number' do - near_topic_view.posts.should == [p1, p2, p3] - end - - it 'has a min of the 1st post number' do - near_topic_view.min.should == p1.post_number - end - - it 'has a max of the 3rd post number' do - near_topic_view.max.should == p3.post_number - end - - it 'is the inital load' do - near_topic_view.should be_initial_load - end - - end - - context 'before a post_number' do - before do - topic_view.filter_posts_before(p3.post_number) - end - - it 'returns posts before a post number' do - topic_view.posts.should == [p2, p1] - end - - it 'has a min of the 1st post number' do - topic_view.min.should == p1.post_number - end - - it 'has a max of the 2nd post number' do - topic_view.max.should == p2.post_number - end - - it "isn't the inital load" do - topic_view.should_not be_initial_load - end - - end - - context 'after a post_number' do - before do - topic_view.filter_posts_after(p1.post_number) - end - - it 'returns posts after a post number' do - topic_view.posts.should == [p2, p3] - end - - it 'has a min of the 1st post number' do - topic_view.min.should == p1.post_number - end - - it 'has a max of 3' do - topic_view.max.should == 3 - end - - it "isn't the inital load" do - topic_view.should_not be_initial_load - end - end - end - - context 'post range' do - context 'without gaps' do - before do - SiteSetting.stubs(:posts_per_page).returns(20) - TopicView.any_instance.stubs(:post_numbers).returns((1..50).to_a) - end - - it 'returns the first a full page if the post number is 1' do - topic_view.post_range(1).should == [1, 20] - end - - it 'returns 4 posts above and 16 below' do - topic_view.post_range(20).should == [15, 34] - end - - it "returns 20 previous results if we ask for the last post" do - topic_view.post_range(50).should == [30, 50] - end - - it "returns 20 previous results we would overlap the last post" do - topic_view.post_range(40).should == [30, 50] - end - end - - context 'with gaps' do - before do - SiteSetting.stubs(:posts_per_page).returns(20) - - post_numbers = ((1..20).to_a << [100, 105] << (110..150).to_a).flatten - TopicView.any_instance.stubs(:post_numbers).returns(post_numbers) - end - - it "will return posts even if the post required is missing" do - topic_view.post_range(80).should == [16, 122] - end - - it "works at the end of gapped post numbers" do - topic_view.post_range(140).should == [130, 150] - end - - it "works well past the end of the post numbers" do - topic_view.post_range(2000).should == [130, 150] - end - - end - - end - - context '#recent_posts' do before do - 24.times do # our let()s have already created 3 - Fabricate(:post, topic: topic, user: first_poster) + SiteSetting.stubs(:posts_per_page).returns(3) + + # Update them to the sort order we're checking for + [p1, p2, p3, p4, p5].each_with_index do |p, idx| + p.sort_order = idx + 1 + p.save end end - it 'returns at most 25 recent posts ordered newest first' do - recent_posts = topic_view.recent_posts - # count - recent_posts.count.should == 25 + describe "filter_posts_after" do + it "returns undeleted posts after a post" do + topic_view.filter_posts_after(p1.post_number).should == [p2, p3, p5] + topic_view.should_not be_initial_load + end - # ordering - recent_posts.include?(p1).should be_false - recent_posts.include?(p3).should be_true - recent_posts.first.created_at.should > recent_posts.last.created_at + it "clips to the end boundary" do + topic_view.filter_posts_after(p2.post_number).should == [p3, p5] + end + + it "returns nothing after the last post" do + topic_view.filter_posts_after(p5.post_number).should be_blank + end + + it "returns nothing after an invalid post number" do + topic_view.filter_posts_after(1000).should be_blank + end + + it "returns deleted posts to an admin" do + coding_horror.admin = true + topic_view.filter_posts_after(p1.post_number).should == [p2, p3, p4] + end end + + describe "fitler_posts_before" do + it "returns undeleted posts before a post" do + topic_view.filter_posts_before(p5.post_number).should == [p3, p2, p1] + topic_view.should_not be_initial_load + end + + it "clips to the beginning boundary" do + topic_view.filter_posts_before(p3.post_number).should == [p2, p1] + end + + it "returns nothing before the first post" do + topic_view.filter_posts_before(p1.post_number).should be_blank + end + + it "returns nothing before an invalid post number" do + topic_view.filter_posts_before(-10).should be_blank + topic_view.filter_posts_before(1000).should be_blank + end + + it "returns deleted posts to an admin" do + coding_horror.admin = true + topic_view.filter_posts_before(p5.post_number).should == [p4, p3, p2] + end + end + + describe "filter_posts_near" do + + def topic_view_posts_near(post) + TopicView.new(topic.id, coding_horror, post_number: post.post_number).posts + end + + it "snaps to the lower boundary" do + topic_view_posts_near(p1).should == [p1, p2, p3] + end + + it "snaps to the upper boundary" do + topic_view_posts_near(p5).should == [p2, p3, p5] + end + + it "returns the posts in the middle" do + topic_view_posts_near(p2).should == [p1, p2, p3] + end + + it "returns deleted posts to an admin" do + coding_horror.admin = true + topic_view_posts_near(p3).should == [p2, p3, p4] + end + + end + + end end