Merge pull request #106 from gammons/master

Refactored TopicsController#show into something that is much more maintainable

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
-
+    create_topic_view
-    # 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])
 
     anonymous_etag(@topic_view.topic) do
-      # force the correct slug
+      redirect_to_correct_topic and return if slugs_do_not_match
-      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
       View.create_for(@topic_view.topic, request.remote_ip, current_user)
-
+      track_visit_to_topic
-      @topic_view.draft_key = @topic_view.topic.draft_key
+      perform_show_response
-      @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
     end
-
   end
 
   def destroy_timings
@@ -241,12 +176,56 @@ class TopicsController < ApplicationController
 
   private
 
-    def toggle_mute(v)
+  def create_topic_view
-      @topic = Topic.where(id: params[:topic_id].to_i).first
+    opts = params.slice(:username_filters, :best_of, :page, :post_number, :posts_before, :posts_after)
-      guardian.ensure_can_see!(@topic)
+    @topic_view = TopicView.new(params[:id] || params[:topic_id], current_user, opts)
+  end
 
-      @topic.toggle_mute(current_user, v)
+  def toggle_mute(v)
-      render nothing: true
+    @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

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

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)      
+    @posts = @posts.includes(:topic).joins(:user).limit(SiteSetting.posts_per_page)
-    @max = @min + @posts.size      
+    @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
+  def read_posts_set
-      @read_posts_set ||= begin
+    @read_posts_set ||= begin
-        result = Set.new
+      result = Set.new
-        return result unless @user.present?
+      return result unless @user.present?
-        return result unless topic_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)
+      PostTiming.select(:post_number)
-                  .where("topic_id = ? AND user_id = ? AND post_number BETWEEN ? AND ?", 
+                .where("topic_id = ? AND user_id = ? AND post_number BETWEEN ? AND ?", 
-                         @topic.id, @user.id, @min, posts_max)
+                       @topic.id, @user.id, @min, posts_max)
-                  .each {|t| result << t.post_number}      
+                .each {|t| result << t.post_number}
-        result        
+      result
-      end
     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

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

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