From 333d4f9a98a7dddddd5830928e36a2c9014a26d9 Mon Sep 17 00:00:00 2001 From: Dan Ungureanu Date: Tue, 7 Jul 2020 15:25:36 +0300 Subject: [PATCH] FIX: Redirect user to the URL with the correct category slug (#10167) This commit implements a category redirect similar to the one for topic URLs with wrong slug. --- app/controllers/list_controller.rb | 23 +++++++---- spec/requests/api/categories_spec.rb | 6 ++- spec/requests/list_controller_spec.rb | 59 ++++++++++++++++++--------- 3 files changed, 58 insertions(+), 30 deletions(-) diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb index 5f43378b5b7..f9220b489b8 100644 --- a/app/controllers/list_controller.rb +++ b/app/controllers/list_controller.rb @@ -338,6 +338,21 @@ class ListController < ApplicationController raise Discourse::NotFound.new("category not found", check_permalinks: true) if @category.nil? + if !guardian.can_see?(@category) + if SiteSetting.detailed_404 + raise Discourse::InvalidAccess + else + raise Discourse::NotFound + end + end + + current_slug = params.require(:category_slug_path_with_id) + real_slug = @category.full_slug("/") + if current_slug != real_slug + url = request.fullpath.gsub(current_slug, real_slug) + return redirect_to path(url), status: 301 + end + params[:category] = @category.id.to_s @description_meta = if @category.uncategorized? @@ -347,14 +362,6 @@ class ListController < ApplicationController end @description_meta = SiteSetting.site_description if @description_meta.blank? - if !guardian.can_see?(@category) - if SiteSetting.detailed_404 - raise Discourse::InvalidAccess - else - raise Discourse::NotFound - end - end - if use_crawler_layout? @subcategories = @category.subcategories.select { |c| guardian.can_see?(c) } end diff --git a/spec/requests/api/categories_spec.rb b/spec/requests/api/categories_spec.rb index 4e3e2bac512..f14335f1ecc 100644 --- a/spec/requests/api/categories_spec.rb +++ b/spec/requests/api/categories_spec.rb @@ -243,12 +243,13 @@ describe 'categories' do end end - path '/c/{category_id}.json' do + path '/c/{category_slug}/{category_id}.json' do get 'List topics' do tags 'Categories' produces 'application/json' - parameter name: :category_id, in: :path, schema: { type: :string } + parameter name: :category_slug, in: :path, schema: { type: :string } + parameter name: :category_id, in: :path, schema: { type: :integer } response '200', 'response' do schema type: :object, properties: { @@ -329,6 +330,7 @@ describe 'categories' do } } let(:category_id) { category.id } + let(:category_slug) { category.slug_path.join("/") } run_test! end end diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb index 2fea625c058..7afe5423a81 100644 --- a/spec/requests/list_controller_spec.rb +++ b/spec/requests/list_controller_spec.rb @@ -363,14 +363,7 @@ RSpec.describe ListController do context 'with access to see the category' do it "succeeds" do - get "/c/#{category.slug}/l/latest" - expect(response.status).to eq(200) - end - end - - context 'with a link that includes an id' do - it "succeeds" do - get "/c/#{category.id}-category/l/latest" + get "/c/#{category.slug}/#{category.id}/l/latest" expect(response.status).to eq(200) end end @@ -403,7 +396,7 @@ RSpec.describe ListController do } it 'uses the correct category' do - get "/c/#{other_category.slug}/l/latest.json" + get "/c/#{other_category.slug}/#{other_category.id}/l/latest.json" expect(response.status).to eq(200) body = response.parsed_body expect(body["topic_list"]["topics"].first["category_id"]) @@ -416,7 +409,7 @@ RSpec.describe ListController do context 'when parent and child are requested' do it "succeeds" do - get "/c/#{category.slug}/#{sub_category.slug}/l/latest" + get "/c/#{category.slug}/#{sub_category.slug}/#{sub_category.id}/l/latest" expect(response.status).to eq(200) end end @@ -431,14 +424,14 @@ RSpec.describe ListController do describe 'feed' do it 'renders RSS' do - get "/c/#{category.slug}.rss" + get "/c/#{category.slug}/#{category.id}.rss" expect(response.status).to eq(200) expect(response.media_type).to eq('application/rss+xml') end it "renders RSS in subfolder correctly" do set_subfolder "/forum" - get "/c/#{category.slug}.rss" + get "/c/#{category.slug}/#{category.id}.rss" expect(response.status).to eq(200) expect(response.body).to_not include("/forum/forum") expect(response.body).to include("http://test.localhost/forum/c/#{category.slug}") @@ -448,7 +441,7 @@ RSpec.describe ListController do describe "category default views" do it "has a top default view" do category.update!(default_view: 'top', default_top_period: 'monthly') - get "/c/#{category.slug}.json" + get "/c/#{category.slug}/#{category.id}.json" expect(response.status).to eq(200) json = response.parsed_body expect(json["topic_list"]["for_period"]).to eq("monthly") @@ -456,7 +449,7 @@ RSpec.describe ListController do it "has a default view of nil" do category.update!(default_view: nil) - get "/c/#{category.slug}.json" + get "/c/#{category.slug}/#{category.id}.json" expect(response.status).to eq(200) json = response.parsed_body expect(json["topic_list"]["for_period"]).to be_blank @@ -464,7 +457,7 @@ RSpec.describe ListController do it "has a default view of ''" do category.update!(default_view: '') - get "/c/#{category.slug}.json" + get "/c/#{category.slug}/#{category.id}.json" expect(response.status).to eq(200) json = response.parsed_body expect(json["topic_list"]["for_period"]).to be_blank @@ -472,7 +465,7 @@ RSpec.describe ListController do it "has a default view of latest" do category.update!(default_view: 'latest') - get "/c/#{category.slug}.json" + get "/c/#{category.slug}/#{category.id}.json" expect(response.status).to eq(200) json = response.parsed_body expect(json["topic_list"]["for_period"]).to be_blank @@ -481,13 +474,13 @@ RSpec.describe ListController do describe "renders canonical tag" do it 'for category default view' do - get "/c/#{category.slug}" + get "/c/#{category.slug}/#{category.id}" expect(response.status).to eq(200) expect(css_select("link[rel=canonical]").length).to eq(1) end it 'for category latest view' do - get "/c/#{category.slug}/l/latest" + get "/c/#{category.slug}/#{category.id}/l/latest" expect(response.status).to eq(200) expect(css_select("link[rel=canonical]").length).to eq(1) end @@ -497,14 +490,14 @@ RSpec.describe ListController do let!(:amazing_category) { Fabricate(:category_with_definition, name: "Amazing Category") } it 'for category default view' do - get "/c/#{amazing_category.slug}" + get "/c/#{amazing_category.slug}/#{amazing_category.id}" expect(response.body).to have_tag "title", text: "Amazing Category - Discourse" end it 'for category latest view' do SiteSetting.short_site_description = "Best community" - get "/c/#{amazing_category.slug}/l/latest" + get "/c/#{amazing_category.slug}/#{amazing_category.id}/l/latest" expect(response.body).to have_tag "title", text: "Amazing Category - Discourse" end @@ -646,4 +639,30 @@ RSpec.describe ListController do expect(response.status).to eq(404) end end + + describe "set_category" do + let(:category) { Fabricate(:category_with_definition) } + let(:subcategory) { Fabricate(:category_with_definition, parent_category_id: category.id) } + let(:subsubcategory) { Fabricate(:category_with_definition, parent_category_id: subcategory.id) } + + before do + SiteSetting.max_category_nesting = 3 + end + + it "redirects to URL with the updated slug" do + get "/c/hello/world/bye/#{subsubcategory.id}" + expect(response.status).to eq(301) + expect(response).to redirect_to("/c/#{category.slug}/#{subcategory.slug}/#{subsubcategory.slug}/#{subsubcategory.id}") + end + + context "with subfolder" do + it "redirects to URL containing the updated slug" do + set_subfolder "/forum" + get "/c/hello/world/bye/#{subsubcategory.id}" + + expect(response.status).to eq(301) + expect(response).to redirect_to("/forum/c/#{category.slug}/#{subcategory.slug}/#{subsubcategory.slug}/#{subsubcategory.id}") + end + end + end end