DEV: Remove legacy tag and category routes (#10338)

* DEV: Remove client-side legacy tag and category routes

* DEV: Remove server-side legacy tag routes

* DEV: Refactor ListController#set_category

* FIX: Remove reference to discovery.parentCategory

* FIX: Refactor TagsController#set_category_from_params

* FIX: Build correct canonical URL for tags and categories

* DEV: Fix deprecation notice in Ruby 2.7

* DEV: Replace use of removed legacy tag route

* DEV: Add deprecation notices for old routes and controllers
This commit is contained in:
Dan Ungureanu 2020-11-03 16:57:58 +02:00 committed by GitHub
parent 5140ec9acf
commit 3c51647872
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
15 changed files with 79 additions and 156 deletions

View File

@ -52,6 +52,24 @@ export function buildResolver(baseName) {
return "service:app-events"; return "service:app-events";
} }
for (const [key, value] of Object.entries({
"controller:discovery.categoryWithID": "controller:discovery.category",
"controller:discovery.parentCategory": "controller:discovery.category",
"controller:tags-show": "controller:tag-show",
"controller:tags.show": "controller:tag.show",
"controller:tagsShow": "controller:tagShow",
"route:discovery.categoryWithID": "route:discovery.category",
"route:discovery.parentCategory": "route:discovery.category",
"route:tags-show": "route:tag-show",
"route:tags.show": "route:tag.show",
"route:tagsShow": "route:tagShow",
})) {
if (fullName === key) {
deprecated(`${key} was replaced with ${value}`, { since: "2.6.0" });
return value;
}
}
const split = fullName.split(":"); const split = fullName.split(":");
if (split.length > 1) { if (split.length > 1) {
const appBase = `${baseName}/${split[0]}s/`; const appBase = `${baseName}/${split[0]}s/`;

View File

@ -26,7 +26,7 @@ export default Controller.extend(ModalFunctionality, BufferedContent, {
this.send("closeModal"); this.send("closeModal");
if (result.responseJson.tag) { if (result.responseJson.tag) {
this.transitionToRoute("tags.show", result.responseJson.tag.id); this.transitionToRoute("tag.show", result.responseJson.tag.id);
} else { } else {
this.flash(extractError(result.responseJson.errors[0]), "error"); this.flash(extractError(result.responseJson.errors[0]), "error");
} }

View File

@ -122,7 +122,7 @@ NavItem.reopenClass({
if (context.tagId && Site.currentProp("filters").includes(filterType)) { if (context.tagId && Site.currentProp("filters").includes(filterType)) {
includesTagContext = true; includesTagContext = true;
path += "/tags"; path += "/tag";
} }
if (context.category) { if (context.category) {

View File

@ -1,7 +1,7 @@
import buildCategoryRoute from "discourse/routes/build-category-route"; import buildCategoryRoute from "discourse/routes/build-category-route";
import buildTopicRoute from "discourse/routes/build-topic-route"; import buildTopicRoute from "discourse/routes/build-topic-route";
import DiscoverySortableController from "discourse/controllers/discovery-sortable"; import DiscoverySortableController from "discourse/controllers/discovery-sortable";
import TagsShowRoute from "discourse/routes/tags-show"; import TagShowRoute from "discourse/routes/tag-show";
import Site from "discourse/models/site"; import Site from "discourse/models/site";
import User from "discourse/models/user"; import User from "discourse/models/user";
@ -11,20 +11,16 @@ export default {
initialize(registry, app) { initialize(registry, app) {
app.DiscoveryCategoryController = DiscoverySortableController.extend(); app.DiscoveryCategoryController = DiscoverySortableController.extend();
app.DiscoveryParentCategoryController = DiscoverySortableController.extend();
app.DiscoveryCategoryNoneController = DiscoverySortableController.extend(); app.DiscoveryCategoryNoneController = DiscoverySortableController.extend();
app.DiscoveryCategoryAllController = DiscoverySortableController.extend(); app.DiscoveryCategoryAllController = DiscoverySortableController.extend();
app.DiscoveryCategoryWithIDController = DiscoverySortableController.extend();
app.DiscoveryCategoryRoute = buildCategoryRoute("default"); app.DiscoveryCategoryRoute = buildCategoryRoute("default");
app.DiscoveryParentCategoryRoute = buildCategoryRoute("default");
app.DiscoveryCategoryNoneRoute = buildCategoryRoute("default", { app.DiscoveryCategoryNoneRoute = buildCategoryRoute("default", {
no_subcategories: true, no_subcategories: true,
}); });
app.DiscoveryCategoryAllRoute = buildCategoryRoute("default", { app.DiscoveryCategoryAllRoute = buildCategoryRoute("default", {
no_subcategories: false, no_subcategories: false,
}); });
app.DiscoveryCategoryWithIDRoute = buildCategoryRoute("default");
const site = Site.current(); const site = Site.current();
site.get("filters").forEach((filter) => { site.get("filters").forEach((filter) => {
@ -35,9 +31,6 @@ export default {
app[ app[
`Discovery${filterCapitalized}CategoryController` `Discovery${filterCapitalized}CategoryController`
] = DiscoverySortableController.extend(); ] = DiscoverySortableController.extend();
app[
`Discovery${filterCapitalized}ParentCategoryController`
] = DiscoverySortableController.extend();
app[ app[
`Discovery${filterCapitalized}CategoryNoneController` `Discovery${filterCapitalized}CategoryNoneController`
] = DiscoverySortableController.extend(); ] = DiscoverySortableController.extend();
@ -48,9 +41,6 @@ export default {
app[`Discovery${filterCapitalized}CategoryRoute`] = buildCategoryRoute( app[`Discovery${filterCapitalized}CategoryRoute`] = buildCategoryRoute(
filter filter
); );
app[
`Discovery${filterCapitalized}ParentCategoryRoute`
] = buildCategoryRoute(filter);
app[ app[
`Discovery${filterCapitalized}CategoryNoneRoute` `Discovery${filterCapitalized}CategoryNoneRoute`
] = buildCategoryRoute(filter, { no_subcategories: true }); ] = buildCategoryRoute(filter, { no_subcategories: true });
@ -74,9 +64,6 @@ export default {
app[ app[
`DiscoveryTop${periodCapitalized}CategoryController` `DiscoveryTop${periodCapitalized}CategoryController`
] = DiscoverySortableController.extend(); ] = DiscoverySortableController.extend();
app[
`DiscoveryTop${periodCapitalized}ParentCategoryController`
] = DiscoverySortableController.extend();
app[ app[
`DiscoveryTop${periodCapitalized}CategoryNoneController` `DiscoveryTop${periodCapitalized}CategoryNoneController`
] = DiscoverySortableController.extend(); ] = DiscoverySortableController.extend();
@ -86,38 +73,26 @@ export default {
app[`DiscoveryTop${periodCapitalized}CategoryRoute`] = buildCategoryRoute( app[`DiscoveryTop${periodCapitalized}CategoryRoute`] = buildCategoryRoute(
"top/" + period "top/" + period
); );
app[
`DiscoveryTop${periodCapitalized}ParentCategoryRoute`
] = buildCategoryRoute("top/" + period);
app[ app[
`DiscoveryTop${periodCapitalized}CategoryNoneRoute` `DiscoveryTop${periodCapitalized}CategoryNoneRoute`
] = buildCategoryRoute("top/" + period, { no_subcategories: true }); ] = buildCategoryRoute("top/" + period, { no_subcategories: true });
}); });
app["TagsShowCategoryRoute"] = TagsShowRoute.extend(); app["TagsShowCategoryRoute"] = TagShowRoute.extend();
app["TagsShowCategoryNoneRoute"] = TagsShowRoute.extend({ app["TagsShowCategoryNoneRoute"] = TagShowRoute.extend({
noSubcategories: true, noSubcategories: true,
}); });
app["TagsShowParentCategoryRoute"] = TagsShowRoute.extend();
app["TagShowRoute"] = TagsShowRoute;
site.get("filters").forEach(function (filter) { site.get("filters").forEach(function (filter) {
app["TagsShow" + filter.capitalize() + "Route"] = TagsShowRoute.extend({ app["TagShow" + filter.capitalize() + "Route"] = TagShowRoute.extend({
navMode: filter,
});
app["TagShow" + filter.capitalize() + "Route"] = TagsShowRoute.extend({
navMode: filter, navMode: filter,
}); });
app[ app[
"TagsShowCategory" + filter.capitalize() + "Route" "TagsShowCategory" + filter.capitalize() + "Route"
] = TagsShowRoute.extend({ navMode: filter }); ] = TagShowRoute.extend({ navMode: filter });
app[ app[
"TagsShowNoneCategory" + filter.capitalize() + "Route" "TagsShowNoneCategory" + filter.capitalize() + "Route"
] = TagsShowRoute.extend({ navMode: filter, noSubcategories: true }); ] = TagShowRoute.extend({ navMode: filter, noSubcategories: true });
app[
"TagsShowParentCategory" + filter.capitalize() + "Route"
] = TagsShowRoute.extend({ navMode: filter });
}); });
}, },
}; };

View File

@ -34,9 +34,6 @@ export default function () {
); );
this.route("discovery", { path: "/", resetNamespace: true }, function () { this.route("discovery", { path: "/", resetNamespace: true }, function () {
// legacy route
this.route("topParentCategory", { path: "/c/:slug/l/top" });
// top // top
this.route("top"); this.route("top");
this.route("topCategoryNone", { this.route("topCategoryNone", {
@ -48,9 +45,6 @@ export default function () {
Site.currentProp("periods").forEach((period) => { Site.currentProp("periods").forEach((period) => {
const top = "top" + period.capitalize(); const top = "top" + period.capitalize();
// legacy route
this.route(top + "ParentCategory", { path: "/c/:slug/l/top/" + period });
this.route(top, { path: "/top/" + period }); this.route(top, { path: "/top/" + period });
this.route(top + "CategoryNone", { this.route(top + "CategoryNone", {
path: "/c/*category_slug_path_with_id/none/l/top/" + period, path: "/c/*category_slug_path_with_id/none/l/top/" + period,
@ -62,9 +56,6 @@ export default function () {
// filters (e.g. bookmarks, posted, read, unread, latest) // filters (e.g. bookmarks, posted, read, unread, latest)
Site.currentProp("filters").forEach((filter) => { Site.currentProp("filters").forEach((filter) => {
// legacy route
this.route(filter + "ParentCategory", { path: "/c/:slug/l/" + filter });
this.route(filter, { path: "/" + filter }); this.route(filter, { path: "/" + filter });
this.route(filter + "CategoryNone", { this.route(filter + "CategoryNone", {
path: "/c/*category_slug_path_with_id/none/l/" + filter, path: "/c/*category_slug_path_with_id/none/l/" + filter,
@ -76,10 +67,6 @@ export default function () {
this.route("categories"); this.route("categories");
// legacy routes
this.route("parentCategory", { path: "/c/:slug" });
this.route("categoryWithID", { path: "/c/:parentSlug/:slug/:id" });
// default filter for a category // default filter for a category
this.route("categoryNone", { path: "/c/*category_slug_path_with_id/none" }); this.route("categoryNone", { path: "/c/*category_slug_path_with_id/none" });
this.route("categoryAll", { path: "/c/*category_slug_path_with_id/all" }); this.route("categoryAll", { path: "/c/*category_slug_path_with_id/all" });
@ -259,14 +246,6 @@ export default function () {
this.route("intersection", { this.route("intersection", {
path: "intersection/:tag_id/*additional_tags", path: "intersection/:tag_id/*additional_tags",
}); });
// legacy routes
this.route("show", { path: "/:tag_id" });
Site.currentProp("filters").forEach((filter) => {
this.route("show" + filter.capitalize(), {
path: "/:tag_id/l/" + filter,
});
});
}); });
this.route( this.route(

View File

@ -33,17 +33,9 @@ export default DiscourseRoute.extend({
} }
} }
if (Boolean(category)) { if (category) {
let route = "discovery.parentCategory"; let route = "discovery.category";
let params = { category, slug: category.slug }; let params = { category, id: category.id };
if (category.parentCategory) {
route = "discovery.category";
params = {
category,
parentSlug: category.parentCategory.slug,
slug: category.slug,
};
}
this.replaceWith(route, params).then((e) => { this.replaceWith(route, params).then((e) => {
if (this.controllerFor("navigation/category").canCreateTopic) { if (this.controllerFor("navigation/category").canCreateTopic) {

View File

@ -23,7 +23,7 @@ export default DiscourseRoute.extend(FilterModeMixin, {
queryParams, queryParams,
renderTemplate() { renderTemplate() {
const controller = this.controllerFor("tags.show"); const controller = this.controllerFor("tag.show");
this.render("tags.show", { controller }); this.render("tags.show", { controller });
}, },
@ -62,7 +62,7 @@ export default DiscourseRoute.extend(FilterModeMixin, {
}, },
afterModel(tag, transition) { afterModel(tag, transition) {
const controller = this.controllerFor("tags.show"); const controller = this.controllerFor("tag.show");
controller.setProperties({ controller.setProperties({
loading: true, loading: true,
showInfo: false, showInfo: false,
@ -122,7 +122,7 @@ export default DiscourseRoute.extend(FilterModeMixin, {
const filterText = I18n.t( const filterText = I18n.t(
`filters.${this.navMode.replace("/", ".")}.title` `filters.${this.navMode.replace("/", ".")}.title`
); );
const controller = this.controllerFor("tags.show"); const controller = this.controllerFor("tag.show");
if (controller.get("model.id")) { if (controller.get("model.id")) {
if (this.category) { if (this.category) {
@ -152,7 +152,7 @@ export default DiscourseRoute.extend(FilterModeMixin, {
}, },
setupController(controller, model) { setupController(controller, model) {
this.controllerFor("tags.show").setProperties({ this.controllerFor("tag.show").setProperties({
model, model,
tag: model, tag: model,
additionalTags: this.additionalTags, additionalTags: this.additionalTags,
@ -180,7 +180,7 @@ export default DiscourseRoute.extend(FilterModeMixin, {
}, },
createTopic() { createTopic() {
const controller = this.controllerFor("tags.show"); const controller = this.controllerFor("tag.show");
if (controller.get("list.draft")) { if (controller.get("list.draft")) {
this.openTopicDraft(controller.get("list")); this.openTopicDraft(controller.get("list"));
@ -233,7 +233,7 @@ export default DiscourseRoute.extend(FilterModeMixin, {
resetParams, resetParams,
didTransition() { didTransition() {
this.controllerFor("tags.show")._showFooter(); this.controllerFor("tag.show")._showFooter();
return true; return true;
}, },
}, },

View File

@ -1,6 +1,6 @@
import TagsShowRoute from "discourse/routes/tags-show"; import TagShowRoute from "discourse/routes/tag-show";
export default TagsShowRoute.extend({}); export default TagShowRoute.extend({});
// The tags-intersection route is exactly the same as the tags-show route, but the wildcard at the // The tags-intersection route is exactly the same as the tags-show route, but the wildcard at the
// end of the route (*additional_tags) will cause a match when query parameters are present, // end of the route (*additional_tags) will cause a match when query parameters are present,

View File

@ -336,25 +336,14 @@ class ListController < ApplicationController
end end
def set_category def set_category
parts = params.require(:category_slug_path_with_id).split('/') category_slug_path_with_id = params.require(:category_slug_path_with_id)
if !parts.empty? && parts.last =~ /\A\d+\Z/ @category = Category.find_by_slug_path_with_id(category_slug_path_with_id)
id = parts.pop.to_i if @category.nil?
end raise Discourse::NotFound.new("category not found", check_permalinks: true)
slug_path = parts unless parts.empty?
if id.present?
@category = Category.find_by_id(id)
elsif slug_path.present?
@category = Category.find_by_slug_path(slug_path)
# Legacy paths
if @category.nil? && parts.last =~ /\A\d+-category/
@category = Category.find_by_id(parts.last.to_i)
end
end end
raise Discourse::NotFound.new("category not found", check_permalinks: true) if @category.nil? params[:category] = @category.id.to_s
if !guardian.can_see?(@category) if !guardian.can_see?(@category)
if SiteSetting.detailed_404 if SiteSetting.detailed_404
@ -364,13 +353,13 @@ class ListController < ApplicationController
end end
end end
current_slug = params.require(:category_slug_path_with_id) # Check if the category slug is incorrect and redirect to a link containing
real_slug = @category.full_slug("/") # the correct one.
current_slug = category_slug_path_with_id
if SiteSetting.slug_generation_method == "encoded" if SiteSetting.slug_generation_method == "encoded"
current_slug = current_slug.split("/").map { |slug| CGI.escape(slug) }.join("/") current_slug = current_slug.split("/").map { |slug| CGI.escape(slug) }.join("/")
end end
real_slug = @category.full_slug("/")
if current_slug != real_slug if current_slug != real_slug
url = request.fullpath.gsub(current_slug, real_slug) url = request.fullpath.gsub(current_slug, real_slug)
if ActionController::Base.config.relative_url_root if ActionController::Base.config.relative_url_root
@ -380,14 +369,13 @@ class ListController < ApplicationController
return redirect_to path(url), status: 301 return redirect_to path(url), status: 301
end end
params[:category] = @category.id.to_s
@description_meta = if @category.uncategorized? @description_meta = if @category.uncategorized?
I18n.t('category.uncategorized_description', locale: SiteSetting.default_locale) I18n.t("category.uncategorized_description", locale: SiteSetting.default_locale)
else elsif @category.description_text.present?
@category.description_text @category.description_text
else
SiteSetting.site_description
end end
@description_meta = SiteSetting.site_description if @description_meta.blank?
if use_crawler_layout? if use_crawler_layout?
@subcategories = @category.subcategories.select { |c| guardian.can_see?(c) } @subcategories = @category.subcategories.select { |c| guardian.can_see?(c) }

View File

@ -18,7 +18,7 @@ class TagsController < ::ApplicationController
skip_before_action :check_xhr, only: [:tag_feed, :show, :index] skip_before_action :check_xhr, only: [:tag_feed, :show, :index]
before_action :set_category_from_params, except: [:index, :update, :destroy, before_action :set_category, except: [:index, :update, :destroy,
:tag_feed, :search, :notifications, :update_notifications, :personal_messages, :info] :tag_feed, :search, :notifications, :update_notifications, :personal_messages, :info]
before_action :fetch_tag, only: [:info, :create_synonyms, :destroy_synonym] before_action :fetch_tag, only: [:info, :create_synonyms, :destroy_synonym]
@ -92,8 +92,9 @@ class TagsController < ::ApplicationController
@description_meta = I18n.t("rss_by_tag", tag: tag_params.join(' & ')) @description_meta = I18n.t("rss_by_tag", tag: tag_params.join(' & '))
@title = @description_meta @title = @description_meta
path_name = url_method(params.slice(:category, :parent_category)) canonical_params = params.slice(:category_slug_path_with_id, :tag_id)
canonical_url "#{Discourse.base_url_no_prefix}#{public_send(path_name, *(params.slice(:parent_category, :category, :tag_id).values.map { |t| t.force_encoding("UTF-8") }))}" canonical_method = url_method(canonical_params)
canonical_url "#{Discourse.base_url_no_prefix}#{public_send(canonical_method, *(canonical_params.values.map { |t| t.force_encoding("UTF-8") }))}"
if @list.topics.size == 0 && params[:tag_id] != 'none' && !Tag.where_name(@tag_id).exists? if @list.topics.size == 0 && params[:tag_id] != 'none' && !Tag.where_name(@tag_id).exists?
raise Discourse::NotFound.new("tag not found", check_permalinks: true) raise Discourse::NotFound.new("tag not found", check_permalinks: true)
@ -347,25 +348,9 @@ class TagsController < ::ApplicationController
end.compact end.compact
end end
def set_category_from_params def set_category
if request.path_parameters.include?(:category_slug_path_with_id) if request.path_parameters.include?(:category_slug_path_with_id)
parts = params[:category_slug_path_with_id].split('/') @filter_on_category = Category.find_by_slug_path_with_id(params[:category_slug_path_with_id])
if !parts.empty? && parts.last =~ /\A\d+\Z/
id = parts.pop.to_i
end
slug_path = parts unless parts.empty?
if id.present?
@filter_on_category = Category.find_by_id(id)
elsif slug_path.present?
@filter_on_category = Category.find_by_slug_path(slug_path)
# Legacy paths
if @filter_on_category.nil? && parts.last =~ /\A\d+-category/
@filter_on_category = Category.find_by_id(parts.last.to_i)
end
end
else else
slug_or_id = params[:category] slug_or_id = params[:category]
return true if slug_or_id.nil? return true if slug_or_id.nil?
@ -373,9 +358,16 @@ class TagsController < ::ApplicationController
@filter_on_category = Category.query_category(slug_or_id, nil) @filter_on_category = Category.query_category(slug_or_id, nil)
end end
category_redirect_or_not_found && (return) if !@filter_on_category
guardian.ensure_can_see!(@filter_on_category) guardian.ensure_can_see!(@filter_on_category)
if !@filter_on_category
permalink = Permalink.find_by_url("c/#{params[:category_slug_path_with_id]}")
if permalink.present? && permalink.category_id
return redirect_to "#{Discourse::base_path}/tags#{permalink.target_url}/#{params[:tag_id]}", status: :moved_permanently
end
raise Discourse::NotFound
end
end end
def page_params def page_params
@ -409,9 +401,7 @@ class TagsController < ::ApplicationController
end end
def url_method(opts = {}) def url_method(opts = {})
if opts[:parent_category] && opts[:category] if opts[:category_slug_path_with_id]
"tag_parent_category_category_#{action_name}_path"
elsif opts[:category]
"tag_category_#{action_name}_path" "tag_category_#{action_name}_path"
else else
"tag_#{action_name}_path" "tag_#{action_name}_path"
@ -472,19 +462,6 @@ class TagsController < ::ApplicationController
options options
end end
def category_redirect_or_not_found
# automatic redirects for renamed categories
url = params[:parent_category] ? "c/#{params[:parent_category]}/#{params[:category]}" : "c/#{params[:category]}"
permalink = Permalink.find_by_url(url)
if permalink.present? && permalink.category_id
redirect_to "#{Discourse.base_path}/tags#{permalink.target_url}/#{params[:tag_id]}", status: :moved_permanently
else
# redirect to 404
raise Discourse::NotFound
end
end
def tag_params def tag_params
[@tag_id].concat(Array(@additional_tags)) [@tag_id].concat(Array(@additional_tags))
end end

View File

@ -810,6 +810,17 @@ class Category < ActiveRecord::Base
Category.find_by_id(query) Category.find_by_id(query)
end end
def self.find_by_slug_path_with_id(slug_path_with_id)
slug_path = slug_path_with_id.split("/")
if slug_path.last =~ /\A\d+\Z/
id = slug_path.pop.to_i
Category.find_by_id(id)
else
Category.find_by_slug_path(slug_path)
end
end
def self.find_by_slug(category_slug, parent_category_slug = nil) def self.find_by_slug(category_slug, parent_category_slug = nil)
return nil if category_slug.nil? return nil if category_slug.nil?

View File

@ -59,7 +59,7 @@ class TopicList < DraftableList
def top_tags def top_tags
opts = @category ? { category: @category } : {} opts = @category ? { category: @category } : {}
opts[:guardian] = Guardian.new(@current_user) opts[:guardian] = Guardian.new(@current_user)
Tag.top_tags(opts) Tag.top_tags(**opts)
end end
def preload_key def preload_key

View File

@ -921,23 +921,6 @@ Discourse::Application.routes.draw do
get '/intersection/:tag_id/*additional_tag_ids' => 'tags#show', as: 'tag_intersection' get '/intersection/:tag_id/*additional_tag_ids' => 'tags#show', as: 'tag_intersection'
end end
# legacy routes
constraints(tag_id: /[^\/]+?/, format: /json|rss/) do
get '/:tag_id.rss' => 'tags#tag_feed'
get '/:tag_id' => 'tags#show'
get '/:tag_id/info' => 'tags#info'
get '/:tag_id/notifications' => 'tags#notifications'
put '/:tag_id/notifications' => 'tags#update_notifications'
put '/:tag_id' => 'tags#update'
delete '/:tag_id' => 'tags#destroy'
post '/:tag_id/synonyms' => 'tags#create_synonyms'
delete '/:tag_id/synonyms/:synonym_id' => 'tags#destroy_synonym'
Discourse.filters.each do |filter|
get "/:tag_id/l/#{filter}" => "tags#show_#{filter}"
end
end
end end
resources :tag_groups, constraints: StaffConstraint.new, except: [:edit] do resources :tag_groups, constraints: StaffConstraint.new, except: [:edit] do

View File

@ -235,7 +235,7 @@ describe 'tags' do
end end
end end
path '/tags/{name}.json' do path '/tag/{name}.json' do
get 'Get a specific tag' do get 'Get a specific tag' do
tags 'Tags' tags 'Tags'