From d84c34ad7523c6f894315dcbfc0b215dff3b3707 Mon Sep 17 00:00:00 2001 From: Daniel Waterworth Date: Wed, 30 Oct 2019 17:22:32 +0000 Subject: [PATCH] DEV: Server-side category routing changes The routes for categories are changing. The scheme that I intend to move us to is: /c/*slug_path/(:id)/ENDPOINT /c/*slug_path/(:id) This commit adds support for the new scheme to the server side without dropping support for existing URLs. It is necessary to support existing URLs for two reasons: * This commit does not change any client side routing code, * Posts that contain category hashtags that refer to a root category are baked into URLs that do not fit this new scheme, (/c/[id]-[slug]) --- app/controllers/list_controller.rb | 92 +++++++++++++-------------- app/models/category.rb | 10 +++ config/routes.rb | 49 ++++++++------ spec/requests/list_controller_spec.rb | 14 ++-- 4 files changed, 87 insertions(+), 78 deletions(-) diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb index 681af70ec2b..bba2d3c9b65 100644 --- a/app/controllers/list_controller.rb +++ b/app/controllers/list_controller.rb @@ -11,16 +11,12 @@ class ListController < ApplicationController # filtered topics lists Discourse.filters.map { |f| :"category_#{f}" }, Discourse.filters.map { |f| :"category_none_#{f}" }, - Discourse.filters.map { |f| :"parent_category_category_#{f}" }, - Discourse.filters.map { |f| :"parent_category_category_none_#{f}" }, # top summaries :category_top, :category_none_top, - :parent_category_category_top, # top pages (ie. with a period) TopTopic.periods.map { |p| :"category_top_#{p}" }, TopTopic.periods.map { |p| :"category_none_top_#{p}" }, - TopTopic.periods.map { |p| :"parent_category_category_top_#{p}" }, # category feeds :category_feed, ].flatten @@ -34,8 +30,6 @@ class ListController < ApplicationController :category_default, Discourse.anonymous_filters.map { |f| :"category_#{f}" }, Discourse.anonymous_filters.map { |f| :"category_none_#{f}" }, - Discourse.anonymous_filters.map { |f| :"parent_category_category_#{f}" }, - Discourse.anonymous_filters.map { |f| :"parent_category_category_none_#{f}" }, # category feeds :category_feed, # user topics feed @@ -44,13 +38,11 @@ class ListController < ApplicationController :top, :category_top, :category_none_top, - :parent_category_category_top, # top pages (ie. with a period) TopTopic.periods.map { |p| :"top_#{p}" }, TopTopic.periods.map { |p| :"top_#{p}_feed" }, TopTopic.periods.map { |p| :"category_top_#{p}" }, TopTopic.periods.map { |p| :"category_none_top_#{p}" }, - TopTopic.periods.map { |p| :"parent_category_category_top_#{p}" }, :group_topics ].flatten @@ -124,15 +116,6 @@ class ListController < ApplicationController define_method("category_none_#{filter}") do self.public_send(filter, category: @category.id, no_subcategories: true) end - - define_method("parent_category_category_#{filter}") do - canonical_url "#{Discourse.base_url_no_prefix}#{@category.url}" - self.public_send(filter, category: @category.id) - end - - define_method("parent_category_category_none_#{filter}") do - self.public_send(filter, category: @category.id) - end end def category_default @@ -257,10 +240,6 @@ class ListController < ApplicationController top(category: @category.id, no_subcategories: true) end - def parent_category_category_top - top(category: @category.id) - end - TopTopic.periods.each do |period| define_method("top_#{period}") do |options = nil| top_options = build_topic_list_options @@ -296,10 +275,6 @@ class ListController < ApplicationController ) end - define_method("parent_category_category_top_#{period}") do - self.public_send("top_#{period}", category: @category.id) - end - # rss feed define_method("top_#{period}_feed") do |options = nil| discourse_expires_in 1.minute @@ -333,32 +308,42 @@ class ListController < ApplicationController def page_params route_params = { format: 'json' } - route_params[:category] = @category.slug_for_url if @category - route_params[:parent_category] = @category.parent_category.slug_for_url if @category && @category.parent_category - route_params[:username] = UrlHelper.escape_uri(params[:username]) if params[:username].present? + + if @category.present? + slug_path = @category.slug_path + + route_params[:category_slug_path_with_id] = + (slug_path + [@category.id.to_s]).join("/") + end + + route_params[:username] = UrlHelper.escape_uri(params[:username]) if params[:username].present? route_params end def set_category - slug_or_id = params.fetch(:category) - parent_slug_or_id = params[:parent_category] - id = params[:id].to_i + parts = params.require(:category_slug_path_with_id).split('/') - parent_category_id = nil - if parent_slug_or_id.present? - parent_category_id = Category.query_parent_category(parent_slug_or_id) - raise Discourse::NotFound.new("category not found", check_permalinks: true) if parent_category_id.blank? && !id + if !parts.empty? && parts.last =~ /\A\d+\Z/ + id = parts.pop.to_i + end + slug_path = parts unless parts.empty? + + if id.present? + @category = Category.find_by_id(id) + elsif slug_path.present? + if (1..2).include?(slug_path.size) + @category = Category.find_by_slug(*slug_path.reverse) + end + + # Legacy paths + if @category.nil? && parts.last =~ /\A\d+-/ + @category = Category.find_by_id(parts.last.to_i) + end end - @category = Category.query_category(slug_or_id, parent_category_id) + raise Discourse::NotFound.new("category not found", check_permalinks: true) if @category.nil? - # Redirect if we have `/c/:parent_category/:category/:id` - if params.include?(:id) - category = Category.find_by_id(id) - (redirect_to category.url, status: 301) && return if category - end - - raise Discourse::NotFound.new("category not found", check_permalinks: true) if !@category + params[:category] = @category.id.to_s @description_meta = @category.description_text if !guardian.can_see?(@category) @@ -388,12 +373,21 @@ class ListController < ApplicationController def construct_url_with(action, opts, url_prefix = nil) method = url_prefix.blank? ? "#{action_name}_path" : "#{url_prefix}_#{action_name}_path" - url = if action == :prev - public_send(method, opts.merge(prev_page_params)) - else # :next - public_send(method, opts.merge(next_page_params)) - end - url.sub('.json?', '?') + + page_params = + case action + when :prev + prev_page_params + when :next + next_page_params + else + raise "unreachable" + end + + opts = opts.dup + opts.delete(:category) if page_params.include?(:category_slug_path_with_id) + + public_send(method, opts.merge(page_params)).sub('.json?', '?') end def get_excluded_category_ids(current_category = nil) diff --git a/app/models/category.rb b/app/models/category.rb index ecec90e437c..e692de65f9b 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -808,6 +808,16 @@ class Category < ActiveRecord::Base end end + def slug_path + if self.parent_category_id.present? + slug_path = self.parent_category.slug_path + slug_path.push(self.slug_for_url) + slug_path + else + [self.slug_for_url] + end + end + private def check_permissions_compatibility(parent_permissions, child_permissions) diff --git a/config/routes.rb b/config/routes.rb index 44594aa6ac3..c00041b3e04 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -620,35 +620,49 @@ Discourse::Application.routes.draw do get '/c', to: redirect(relative_url_root + 'categories') - resources :categories, except: :show - post "category/:category_id/move" => "categories#move" + resources :categories, except: [:show, :new, :edit] post "categories/reorder" => "categories#reorder" - post "category/:category_id/notifications" => "categories#set_notifications" - put "category/:category_id/slug" => "categories#update_slug" + + scope path: 'category/:category_id' do + post "/move" => "categories#move" + post "/notifications" => "categories#set_notifications" + put "/slug" => "categories#update_slug" + end + + get "category/*path" => "categories#redirect" get "categories_and_latest" => "categories#categories_and_latest" get "categories_and_top" => "categories#categories_and_top" get "c/:id/show" => "categories#show" + get "c/:category_slug/find_by_slug" => "categories#find_by_slug" get "c/:parent_category_slug/:category_slug/find_by_slug" => "categories#find_by_slug" - get "c/:category.rss" => "list#category_feed", format: :rss - get "c/:parent_category/:category.rss" => "list#category_feed", format: :rss - get "c/:category" => "list#category_default", as: "category_default" - get "c/:category/none" => "list#category_none_latest" - get "c/:parent_category/:category/(:id)" => "list#parent_category_category_latest", constraints: { id: /\d+/ } - get "c/:category/l/top" => "list#category_top", as: "category_top" - get "c/:category/none/l/top" => "list#category_none_top", as: "category_none_top" - get "c/:parent_category/:category/l/top" => "list#parent_category_category_top", as: "parent_category_category_top" + + get "c/*category_slug_path_with_id.rss" => "list#category_feed", format: :rss + scope path: 'c/*category_slug_path_with_id' do + get "/none" => "list#category_none_latest" + get "/none/l/top" => "list#category_none_top", as: "category_none_top" + get "/l/top" => "list#category_top", as: "category_top" + + TopTopic.periods.each do |period| + get "/none/l/top/#{period}" => "list#category_none_top_#{period}", as: "category_none_top_#{period}" + get "/l/top/#{period}" => "list#category_top_#{period}", as: "category_top_#{period}" + end + + Discourse.filters.each do |filter| + get "/none/l/#{filter}" => "list#category_none_#{filter}", as: "category_none_#{filter}" + get "/l/#{filter}" => "list#category_#{filter}", as: "category_#{filter}" + end + + get "/" => "list#category_default", as: "category_default" + end get "category_hashtags/check" => "category_hashtags#check" TopTopic.periods.each do |period| get "top/#{period}.rss" => "list#top_#{period}_feed", format: :rss get "top/#{period}" => "list#top_#{period}" - get "c/:category/l/top/#{period}" => "list#category_top_#{period}", as: "category_top_#{period}" - get "c/:category/none/l/top/#{period}" => "list#category_none_top_#{period}", as: "category_none_top_#{period}" - get "c/:parent_category/:category/l/top/#{period}" => "list#parent_category_category_top_#{period}", as: "parent_category_category_top_#{period}" end Discourse.anonymous_filters.each do |filter| @@ -657,13 +671,8 @@ Discourse::Application.routes.draw do Discourse.filters.each do |filter| get "#{filter}" => "list##{filter}" - get "c/:category/l/#{filter}" => "list#category_#{filter}", as: "category_#{filter}" - get "c/:category/none/l/#{filter}" => "list#category_none_#{filter}", as: "category_none_#{filter}" - get "c/:parent_category/:category/l/#{filter}" => "list#parent_category_category_#{filter}", as: "parent_category_category_#{filter}" end - get "category/*path" => "categories#redirect" - get "top" => "list#top" get "search/query" => "search#query" get "search" => "search#show" diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb index 279acbff67c..5148a6e07d9 100644 --- a/spec/requests/list_controller_spec.rb +++ b/spec/requests/list_controller_spec.rb @@ -380,19 +380,15 @@ RSpec.describe ListController do let(:child_category) { Fabricate(:category_with_definition, parent_category: category) } context "with valid slug" do - it "redirects to the child category" do - get "/c/#{category.slug}/#{child_category.slug}/l/latest", params: { - id: child_category.id - } - expect(response).to redirect_to(child_category.url) + it "succeeds" do + get "/c/#{category.slug}/#{child_category.slug}/#{child_category.id}/l/latest" + expect(response.status).to eq(200) end end context "with invalid slug" do - it "redirects to child category" do - get "/c/random_slug/another_random_slug/l/latest", params: { - id: child_category.id - } + xit "redirects" do + get "/c/random_slug/another_random_slug/#{child_category.id}/l/latest" expect(response).to redirect_to(child_category.url) end end