From 80fb20816cd6ae405cc0ceda883c18e86aeb85ff Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 20 May 2013 10:29:49 +1000 Subject: [PATCH] get rid of nonsense 404.html correct 404 handling for invalid pages --- app/controllers/application_controller.rb | 15 +++++++------- app/controllers/exceptions_controller.rb | 11 +++------- app/controllers/static_controller.rb | 2 +- app/controllers/topics_controller.rb | 10 ++++----- lib/topic_view.rb | 3 +++ public/404.html | 25 ----------------------- spec/components/topic_view_spec.rb | 13 ++---------- 7 files changed, 20 insertions(+), 59 deletions(-) delete mode 100644 public/404.html diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 0dda1353af6..88427ee163f 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -65,14 +65,13 @@ class ApplicationController < ActionController::Base end rescue_from Discourse::NotFound do - if !request.format || request.format.html? - # for now do a simple remap, we may look at cleaner ways of doing the render - # - # Sam: I am confused about this, we need a comment that explains why this is conditional - raise ActiveRecord::RecordNotFound - else - render file: 'public/404', formats: [:html], layout: false, status: 404 - end + + f = Topic.where(deleted_at: nil, archetype: "regular") + @latest = f.order('views desc').take(10) + @recent = f.order('created_at desc').take(10) + @slug = params[:slug].class == String ? params[:slug] : '' + @slug.gsub!('-',' ') + render status: 404, layout: 'no_js', template: '/exceptions/not_found' end rescue_from Discourse::InvalidAccess do diff --git a/app/controllers/exceptions_controller.rb b/app/controllers/exceptions_controller.rb index 9814bbbfd71..55fb96172b4 100644 --- a/app/controllers/exceptions_controller.rb +++ b/app/controllers/exceptions_controller.rb @@ -1,14 +1,9 @@ class ExceptionsController < ApplicationController skip_before_filter :check_xhr - layout 'no_js' def not_found - f = Topic.where(deleted_at: nil, archetype: "regular") - - @latest = f.order('views desc').take(10) - @recent = f.order('created_at desc').take(10) - @slug = params[:slug].class == String ? params[:slug] : '' - @slug.gsub!('-',' ') - render status: 404 + # centralize all rendering of 404 into app controller + raise Discourse::NotFound end + end diff --git a/app/controllers/static_controller.rb b/app/controllers/static_controller.rb index 87386d1ce6b..834ee57c305 100644 --- a/app/controllers/static_controller.rb +++ b/app/controllers/static_controller.rb @@ -21,7 +21,7 @@ class StaticController < ApplicationController return end - render file: 'public/404', layout: false, status: 404 + raise Discourse::NotFound end # This method just redirects to a given url. diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 8875599bdb3..8617ab91bd3 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -25,7 +25,10 @@ class TopicsController < ApplicationController caches_action :avatar, cache_path: Proc.new {|c| "#{c.params[:post_number]}-#{c.params[:topic_id]}" } def show - create_topic_view + opts = params.slice(:username_filters, :best_of, :page, :post_number, :posts_before, :posts_after, :best) + @topic_view = TopicView.new(params[:id] || params[:topic_id], current_user, opts) + + raise Discourse::NotFound unless @topic_view.posts.present? anonymous_etag(@topic_view.topic) do redirect_to_correct_topic && return if slugs_do_not_match @@ -196,11 +199,6 @@ class TopicsController < ApplicationController private - def create_topic_view - opts = params.slice(:username_filters, :best_of, :page, :post_number, :posts_before, :posts_after, :best) - @topic_view = TopicView.new(params[:id] || params[:topic_id], current_user, opts) - end - def toggle_mute(v) @topic = Topic.where(id: params[:topic_id].to_i).first guardian.ensure_can_see!(@topic) diff --git a/lib/topic_view.rb b/lib/topic_view.rb index bad4704b50a..6bad3c1a347 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -286,6 +286,9 @@ class TopicView post_count = (filtered_post_ids.length - 1) max = [max, post_count].min + + return @posts = [] if min > max + min = [[min, max].min, 0].max @index_offset = min diff --git a/public/404.html b/public/404.html deleted file mode 100644 index 4a8c7246b00..00000000000 --- a/public/404.html +++ /dev/null @@ -1,25 +0,0 @@ - - - - The resource you wanted can't be found (404) - - - - - -
-

The resource you want can't be found.

-
- - diff --git a/spec/components/topic_view_spec.rb b/spec/components/topic_view_spec.rb index 78c3a929fe1..547d1a477c5 100644 --- a/spec/components/topic_view_spec.rb +++ b/spec/components/topic_view_spec.rb @@ -227,20 +227,11 @@ describe TopicView do describe '#filter_posts_paged' do before { SiteSetting.stubs(:posts_per_page).returns(1) } - it 'returns correct posts for first page' do + it 'returns correct posts for all pages' do topic_view.filter_posts_paged(1).should == [p1, p2] - end - - it 'returns correct posts for requested page number' do topic_view.filter_posts_paged(2).should == [p2, p3] - end - - it 'returns correct posts for last page' do topic_view.filter_posts_paged(4).should == [p5] - end - - it 'returns posts for last page when page is out of range' do - topic_view.filter_posts_paged(100).should == [p5] + topic_view.filter_posts_paged(100).should == [] end end