diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 476cc29cd..e6adfa53a 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -15,86 +15,21 @@ class TopicsController < ApplicationController :unmute, :set_notifications, :move_posts] + before_filter :consider_user_for_promotion, only: :show skip_before_filter :check_xhr, only: [:avatar, :show] caches_action :avatar, :cache_path => Proc.new {|c| "#{c.params[:post_number]}-#{c.params[:topic_id]}" } + def show - - # Consider the user for a promotion if they're new - if current_user.present? - Promotion.new(current_user).review if current_user.trust_level == TrustLevel.Levels[:new] - end - - @topic_view = TopicView.new(params[:id] || params[:topic_id], - current_user, - username_filters: params[:username_filters], - best_of: params[:best_of], - page: params[:page]) + create_topic_view anonymous_etag(@topic_view.topic) do - # force the correct slug - if params[:slug] && @topic_view.topic.slug != params[:slug] - fullpath = request.fullpath - - split = fullpath.split('/') - split[2] = @topic_view.topic.slug - - redirect_to split.join('/'), status: 301 - return - end - - # Figure out what we're filter on - if params[:post_number].present? - # Get posts near a post - @topic_view.filter_posts_near(params[:post_number].to_i) - elsif params[:posts_before].present? - @topic_view.filter_posts_before(params[:posts_before].to_i) - elsif params[:posts_after].present? - @topic_view.filter_posts_after(params[:posts_after].to_i) - else - # No filter? Consider it a paged view, default to page 0 which is the first segment - @topic_view.filter_posts_paged(params[:page].to_i) - end + redirect_to_correct_topic and return if slugs_do_not_match View.create_for(@topic_view.topic, request.remote_ip, current_user) - - @topic_view.draft_key = @topic_view.topic.draft_key - @topic_view.draft_sequence = DraftSequence.current(current_user, @topic_view.draft_key) - - if (!request.xhr? || params[:track_visit]) && current_user - TopicUser.track_visit! @topic_view.topic, current_user - @topic_view.draft = Draft.get(current_user, @topic_view.draft_key, @topic_view.draft_sequence) - end - - topic_view_serializer = TopicViewSerializer.new(@topic_view, scope: guardian, root: false) - - respond_to do |format| - format.html do - @canonical = "#{request.protocol}#{request.host_with_port}" + @topic_view.topic.relative_url - - if params[:post_number] - @post = @topic_view.posts.select{|p| p.post_number == params[:post_number].to_i}.first - page = ((params[:post_number].to_i - 1) / SiteSetting.posts_per_page) + 1 - @canonical << "?page=#{page}" if page > 1 - else - @canonical << "?page=#{params[:page]}" if params[:page] && params[:page].to_i > 1 - end - - last_post = @topic_view.posts[-1] - if last_post.present? and (@topic_view.topic.highest_post_number > last_post.post_number) - @next_page = (@topic_view.posts[0].post_number / SiteSetting.posts_per_page) + 1 - end - - store_preloaded("topic_#{@topic_view.topic.id}", MultiJson.dump(topic_view_serializer)) - end - - format.json do - render_json_dump(topic_view_serializer) - end - - end + track_visit_to_topic + perform_show_response end - end def destroy_timings @@ -241,12 +176,56 @@ class TopicsController < ApplicationController private - def toggle_mute(v) - @topic = Topic.where(id: params[:topic_id].to_i).first - guardian.ensure_can_see!(@topic) + def create_topic_view + opts = params.slice(:username_filters, :best_of, :page, :post_number, :posts_before, :posts_after) + @topic_view = TopicView.new(params[:id] || params[:topic_id], current_user, opts) + end - @topic.toggle_mute(current_user, v) - render nothing: true + def toggle_mute(v) + @topic = Topic.where(id: params[:topic_id].to_i).first + guardian.ensure_can_see!(@topic) + + @topic.toggle_mute(current_user, v) + render nothing: true + end + + def consider_user_for_promotion + Promotion.new(current_user).review if current_user.present? + end + + def slugs_do_not_match + params[:slug] && @topic_view.topic.slug != params[:slug] + end + + def redirect_to_correct_topic + fullpath = request.fullpath + + split = fullpath.split('/') + split[2] = @topic_view.topic.slug + + redirect_to split.join('/'), status: 301 + end + + def track_visit_to_topic + return unless should_track_visit_to_topic? + TopicUser.track_visit! @topic_view.topic, current_user + @topic_view.draft = Draft.get(current_user, @topic_view.draft_key, @topic_view.draft_sequence) + end + + def should_track_visit_to_topic? + (!request.xhr? || params[:track_visit]) && current_user + end + + def perform_show_response + topic_view_serializer = TopicViewSerializer.new(@topic_view, scope: guardian, root: false) + respond_to do |format| + format.html do + store_preloaded("topic_#{@topic_view.topic.id}", MultiJson.dump(topic_view_serializer)) + end + + format.json do + render_json_dump(topic_view_serializer) + end end - + end end diff --git a/app/views/topics/show.html.erb b/app/views/topics/show.html.erb index 869cd21c0..f43444c8b 100644 --- a/app/views/topics/show.html.erb +++ b/app/views/topics/show.html.erb @@ -1,8 +1,8 @@ <h2> - <%= render_topic_title(@topic_view.topic) %> + <a href="<%= @topic_view.relative_url %>"><%= @topic_view.title %></a> </h2> -<% (@post ? [@post] : @topic_view.posts).each do |post| %> +<% @topic_view.posts.each do |post| %> <div class='creator'> #<%=post.post_number%> By: <b><%= post.user.name %></b>, <%= post.created_at.to_formatted_s(:long_ordinal) %> </div> @@ -11,10 +11,12 @@ </div> <% end %> -<% if @next_page%> +<% if @topic_view.next_page %> <p> - <b><%= render_topic_next_page_link(@topic_view.topic, @next_page) %></b> + <b><a href="<%= @topic_view.next_page_path %>">next page</a></b> </p> <% end %> -<%- content_for :canonical do %><%= @canonical %><%- end %> +<%- content_for :canonical do %> + <%= "#{request.protocol}#{request.host_with_port}#{@topic_view.canonical_path}" %> +<%- end %> diff --git a/lib/topic_view.rb b/lib/topic_view.rb index cd4cbea21..03d563969 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -6,7 +6,7 @@ class TopicView attr_accessor :topic, :min, :max, :draft, :draft_key, :draft_sequence def initialize(topic_id, user=nil, options={}) - @topic = Topic.where(id: topic_id).includes(:category).first + @topic = find_topic(topic_id) raise Discourse::NotFound if @topic.blank? # Special case: If the topic is private and the user isn't logged in, ask them @@ -15,11 +15,11 @@ class TopicView raise Discourse::NotLoggedIn.new end - Guardian.new(user).ensure_can_see!(@topic) + 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? @@ -31,20 +31,62 @@ class TopicView @user = user @initial_load = true + filter_posts(options) + + @draft_key = @topic.draft_key + @draft_sequence = DraftSequence.current(user, @draft_key) + end + + def canonical_path + path = @topic.relative_url + path << if @post_number + page = ((@post_number.to_i - 1) / SiteSetting.posts_per_page) + 1 + (page > 1) ? "?page=#{page}" : "" + else + (@page && @page.to_i > 1) ? "?page=#{@page}" : "" + end + path + end + + def next_page + last_post = @posts.last + if last_post.present? and (@topic.highest_post_number > last_post.post_number) + (@posts[0].post_number / SiteSetting.posts_per_page) + 1 + end + end + + def next_page_path + "#{@topic.relative_url}?page=#{next_page}" + end + + def relative_url + @topic.relative_url + end + + def title + @topic.title + 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 end # Filter to all posts near a particular post number def filter_posts_near(post_number) - @min, @max = post_range(post_number) + @min, @max = post_range(post_number) filter_posts_in_range(@min, @max) end - def filter_posts_in_range(min, max) - @min, @max = min, max - @posts = @posts.where("post_number between ? and ?", @min, @max).includes(:user).regular_order - end - - def post_numbers @post_numbers ||= @posts.order(:post_number).pluck(:post_number) end @@ -55,7 +97,7 @@ class TopicView 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 @@ -71,9 +113,9 @@ class TopicView @max = post_number - 1 @posts = @posts.reverse_order.where("post_number < ?", post_number) - @posts = @posts.includes(:topic).joins(:user).limit(SiteSetting.posts_per_page) + @posts = @posts.includes(:topic).joins(:user).limit(SiteSetting.posts_per_page) @min = @max - @posts.size - @min = 1 if @min < 1 + @min = 1 if @min < 1 end # Filter to all posts after a particular post number @@ -81,14 +123,19 @@ class TopicView @initial_load = false @min = post_number @posts = @posts.regular_order.where("post_number > ?", post_number) - @posts = @posts.includes(:topic).joins(:user).limit(SiteSetting.posts_per_page) - @max = @min + @posts.size + @posts = @posts.includes(:topic).joins(:user).limit(SiteSetting.posts_per_page) + @max = @min + @posts.size end def posts - @posts + @post_number.present? ? find_post_by_post_number : @posts end + def find_post_by_post_number + @posts.select {|post| post.post_number == @post_number.to_i } + end + + def read?(post_number) read_posts_set.include?(post_number) end @@ -117,7 +164,7 @@ class TopicView end def voted_in_topic? - return false + return false # all post_actions is not the way to do this, cut down on the query, roll it up into topic if we need it @@ -171,7 +218,7 @@ class TopicView min_idx = 0 if min_idx < 0 end - [post_numbers[min_idx], post_numbers[max_idx]] + [post_numbers[min_idx], post_numbers[max_idx]] end # Are we the initial page load? If so, we can return extra information like @@ -187,20 +234,30 @@ class TopicView protected - def read_posts_set - @read_posts_set ||= begin - result = Set.new - return result unless @user.present? - return result unless topic_user.present? + def read_posts_set + @read_posts_set ||= begin + result = Set.new + 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 + posts_max = @max > (topic_user.last_read_post_number || 1 ) ? (topic_user.last_read_post_number || 1) : @max - 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} - result - end + 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} + result end + end + private + + def filter_posts_in_range(min, max) + @min, @max = min, max + @posts = @posts.where("post_number between ? and ?", @min, @max).includes(:user).regular_order + end + + def find_topic(topic_id) + Topic.where(id: topic_id).includes(:category).first + end end diff --git a/spec/components/topic_view_spec.rb b/spec/components/topic_view_spec.rb index c32ec01fc..b46142561 100644 --- a/spec/components/topic_view_spec.rb +++ b/spec/components/topic_view_spec.rb @@ -26,6 +26,62 @@ describe TopicView do lambda { TopicView.new(topic.id, nil) }.should raise_error(Discourse::NotLoggedIn) 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 "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, post_number: 50).canonical_path.should eql(path_with_page) + 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) + 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]] diff --git a/spec/controllers/topics_controller_spec.rb b/spec/controllers/topics_controller_spec.rb index f6bac266f..d4c4b38d6 100644 --- a/spec/controllers/topics_controller_spec.rb +++ b/spec/controllers/topics_controller_spec.rb @@ -267,15 +267,9 @@ describe TopicsController do it "reviews the user for a promotion if they're new" do user.update_column(:trust_level, TrustLevel.Levels[:new]) - promotion.expects(:review) + Promotion.any_instance.expects(:review) get :show, id: topic.id end - - it "doesn't reviews the user for a promotion if they're basic" do - promotion.expects(:review).never - get :show, id: topic.id - end - end context 'filters' do