From 62041da7e048415d68066f02b154a2b8880a35d6 Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Thu, 6 Jun 2013 14:41:27 -0400 Subject: [PATCH] Handle /t/only-the-slug urls by trying to find the topic by slug (second try) --- app/controllers/application_controller.rb | 1 + app/controllers/topics_controller.rb | 17 ++++++++--------- spec/controllers/topics_controller_spec.rb | 20 +++++++++++++++----- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index b0a6b3469b8..edd94b5625e 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -282,6 +282,7 @@ class ApplicationController < ActionController::Base @latest = f.order('views desc').take(10) @recent = f.order('created_at desc').take(10) @slug = params[:slug].class == String ? params[:slug] : '' + @slug = (params[:id].class == String ? params[:id] : '') if @slug.blank? @slug.gsub!('-',' ') render status: status, layout: 'no_js', template: '/exceptions/not_found' end diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 284b8175f6a..c694909cb11 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -21,19 +21,18 @@ class TopicsController < ApplicationController before_filter :consider_user_for_promotion, only: :show - skip_before_filter :check_xhr, only: [:avatar, :show, :feed, :redirect_to_show] + skip_before_filter :check_xhr, only: [:avatar, :show, :feed] caches_action :avatar, cache_path: Proc.new {|c| "#{c.params[:post_number]}-#{c.params[:topic_id]}" } - def redirect_to_show - topic_query = ((num = params[:id].to_i) > 0 and num.to_s == params[:id].to_s) ? Topic.where(id: num) : Topic.where(slug: params[:id]) - topic = topic_query.includes(:category).first - raise Discourse::NotFound unless topic - redirect_to topic.relative_url - end - def show 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) + begin + @topic_view = TopicView.new(params[:id] || params[:topic_id], current_user, opts) + rescue Discourse::NotFound + topic = Topic.where(slug: params[:id]).first + raise Discourse::NotFound unless topic + return redirect_to(topic.relative_url) + end raise Discourse::NotFound if @topic_view.posts.blank? && !(opts[:best].to_i > 0) diff --git a/spec/controllers/topics_controller_spec.rb b/spec/controllers/topics_controller_spec.rb index 1247eeaa96c..6beae2019d1 100644 --- a/spec/controllers/topics_controller_spec.rb +++ b/spec/controllers/topics_controller_spec.rb @@ -13,7 +13,7 @@ describe TopicsController do let(:topic) { p1.topic } it "raises an error without postIds" do - lambda { xhr :post, :move_posts, topic_id: topic.id, title: 'blah' }.should raise_error(ActionController::ParameterMissing) + lambda { xhr :post, :move_posts, topic_id: topic.id, title: 'blah' }.should raise_error(ActionController::ParameterMissing) end it "raises an error when the user doesn't have permission to move the posts" do @@ -106,7 +106,7 @@ describe TopicsController do let(:topic) { p1.topic } it "raises an error without destination_topic_id" do - lambda { xhr :post, :merge_topic, topic_id: topic.id }.should raise_error(ActionController::ParameterMissing) + lambda { xhr :post, :merge_topic, topic_id: topic.id }.should raise_error(ActionController::ParameterMissing) end it "raises an error when the user doesn't have permission to merge" do @@ -218,11 +218,11 @@ describe TopicsController do end it 'requires the status parameter' do - lambda { xhr :put, :status, topic_id: @topic.id, enabled: true }.should raise_error(ActionController::ParameterMissing) + lambda { xhr :put, :status, topic_id: @topic.id, enabled: true }.should raise_error(ActionController::ParameterMissing) end it 'requires the enabled parameter' do - lambda { xhr :put, :status, topic_id: @topic.id, status: 'visible' }.should raise_error(ActionController::ParameterMissing) + lambda { xhr :put, :status, topic_id: @topic.id, status: 'visible' }.should raise_error(ActionController::ParameterMissing) end it 'raises an error with a status not in the whitelist' do @@ -373,6 +373,16 @@ describe TopicsController do response.should be_success end + it 'can find a topic given a slug in the id param' do + xhr :get, :show, id: topic.slug + expect(response).to redirect_to(topic.relative_url) + end + + it 'returns 404 when an invalid slug is given and no id' do + xhr :get, :show, id: 'nope-nope' + expect(response.status).to eq(404) + end + it 'records a view' do lambda { xhr :get, :show, topic_id: topic.id, slug: topic.slug }.should change(View, :count).by(1) end @@ -526,7 +536,7 @@ describe TopicsController do end it 'requires an email parameter' do - lambda { xhr :post, :invite, topic_id: @topic.id }.should raise_error(ActionController::ParameterMissing) + lambda { xhr :post, :invite, topic_id: @topic.id }.should raise_error(ActionController::ParameterMissing) end describe 'without permission' do