TopicView respects sort_order and better specs

This commit is contained in:
Robin Ward 2013-03-26 11:58:49 -04:00
parent bd4fcfa412
commit 2efd3e61c7
2 changed files with 280 additions and 327 deletions

View file

@ -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)

View file

@ -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