From d21a08c284b4e7d30227499f7a0d682b37f51cf2 Mon Sep 17 00:00:00 2001 From: Dan Ungureanu Date: Thu, 18 Jun 2020 11:32:14 +0300 Subject: [PATCH] DEV: Deprecate Category#url_with_id in favor of Category#url (#9972) --- .../category_hashtags_controller.rb | 2 +- app/models/category.rb | 9 +++-- app/models/permalink.rb | 2 +- app/models/topic_list.rb | 2 +- lib/pretty_text/helpers.rb | 4 +-- spec/components/pretty_text_spec.rb | 4 +-- spec/models/category_spec.rb | 35 +++---------------- spec/models/permalink_spec.rb | 2 +- spec/requests/categories_controller_spec.rb | 2 +- .../category_hashtags_controller_spec.rb | 4 +-- 10 files changed, 21 insertions(+), 45 deletions(-) diff --git a/app/controllers/category_hashtags_controller.rb b/app/controllers/category_hashtags_controller.rb index b08d7f23ae9..b5a971fa805 100644 --- a/app/controllers/category_hashtags_controller.rb +++ b/app/controllers/category_hashtags_controller.rb @@ -9,7 +9,7 @@ class CategoryHashtagsController < ApplicationController ids = category_slugs.map { |category_slug| Category.query_from_hashtag_slug(category_slug).try(:id) } valid_categories = Category.secured(guardian).where(id: ids).map do |category| - { slug: category.hashtag_slug, url: category.url_with_id } + { slug: category.hashtag_slug, url: category.url } end.compact render json: { valid: valid_categories } diff --git a/app/models/category.rb b/app/models/category.rb index d6375eb7144..cb51a03cc48 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -721,11 +721,13 @@ class Category < ActiveRecord::Base end def url - @@url_cache[self.id] ||= "#{Discourse.base_uri}/c/#{slug_path.join('/')}" + @@url_cache[self.id] ||= "#{Discourse.base_uri}/c/#{slug_path.join('/')}/#{self.id}" end def url_with_id - self.parent_category ? "#{url}/#{self.id}" : "#{Discourse.base_uri}/c/#{self.slug}/#{self.id}" + Discourse.deprecate("Category#url_with_id is deprecated. Use `Category#url` instead.", output_in_test: true) + + url end # If the name changes, try and update the category definition topic too if it's an exact match @@ -739,9 +741,10 @@ class Category < ActiveRecord::Base def create_category_permalink old_slug = saved_changes.transform_values(&:first)["slug"] + url = +"#{Discourse.base_uri}/c" url << "/#{parent_category.slug_path.join('/')}" if parent_category_id - url << "/#{old_slug}" + url << "/#{old_slug}/#{id}" url = Permalink.normalize_url(url) if Permalink.where(url: url).exists? diff --git a/app/models/permalink.rb b/app/models/permalink.rb index 9e1e2d1a4e2..ea317839e51 100644 --- a/app/models/permalink.rb +++ b/app/models/permalink.rb @@ -80,7 +80,7 @@ class Permalink < ActiveRecord::Base return external_url if external_url return "#{Discourse::base_uri}#{post.url}" if post return topic.relative_url if topic - return "#{category.url}/#{category.id}" if category + return category.url if category return tag.full_url if tag nil end diff --git a/app/models/topic_list.rb b/app/models/topic_list.rb index d51c0f153f3..52b0ee00708 100644 --- a/app/models/topic_list.rb +++ b/app/models/topic_list.rb @@ -67,7 +67,7 @@ class TopicList def preload_key if @category - "topic_list_#{@category.url.sub(/^\//, '')}/#{@category.id}/l/#{@filter}" + "topic_list_#{@category.url.sub(/^\//, '')}/l/#{@filter}" else "topic_list_#{@filter}" end diff --git a/lib/pretty_text/helpers.rb b/lib/pretty_text/helpers.rb index d207e01eed6..9633a4e8eaf 100644 --- a/lib/pretty_text/helpers.rb +++ b/lib/pretty_text/helpers.rb @@ -41,7 +41,7 @@ module PrettyText def category_hashtag_lookup(category_slug) if category = Category.query_from_hashtag_slug(category_slug) - [category.url_with_id, category_slug] + [category.url, category_slug] else nil end @@ -106,7 +106,7 @@ module PrettyText is_tag = text =~ /#{tag_postfix}$/ if !is_tag && category = Category.query_from_hashtag_slug(text) - [category.url_with_id, text] + [category.url, text] elsif (!is_tag && tag = Tag.find_by(name: text)) || (is_tag && tag = Tag.find_by(name: text.gsub!("#{tag_postfix}", ''))) ["#{Discourse.base_url}/tag/#{tag.name}", text] diff --git a/spec/components/pretty_text_spec.rb b/spec/components/pretty_text_spec.rb index 49a3ac90baa..823fb8900c2 100644 --- a/spec/components/pretty_text_spec.rb +++ b/spec/components/pretty_text_spec.rb @@ -1106,9 +1106,9 @@ describe PrettyText do [ "#unknown::tag", - "#known", + "#known", "#known", - "#testing" + "#testing" ].each do |element| expect(cooked).to include(element) diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb index 7b550a9c1d3..f122673c0a9 100644 --- a/spec/models/category_spec.rb +++ b/spec/models/category_spec.rb @@ -430,7 +430,7 @@ describe Category do end it "reuses existing permalink when category slug is changed" do - permalink = Permalink.create!(url: "c/#{@category.slug}", category_id: 42) + permalink = Permalink.create!(url: "c/#{@category.slug}/#{@category.id}", category_id: 42) expect { @category.update(slug: 'new-slug') }.to_not change { Permalink.count } expect(permalink.reload.category_id).to eq(@category.id) @@ -695,47 +695,20 @@ describe Category do describe "for normal categories" do it "builds a url" do - expect(category.url).to eq("/c/root") + expect(category.url).to eq("/c/root/#{category.id}") end end describe "for subcategories" do it "builds a url" do - expect(sub_category.url).to eq("/c/root/child") + expect(sub_category.url).to eq("/c/root/child/#{sub_category.id}") end end describe "for sub-sub-categories" do it "builds a url" do expect(sub_sub_category.url) - .to eq("/c/root/child/child-of-child") - end - end - end - - describe "#url_with_id" do - fab!(:category) do - Fabricate( - :category_with_definition, - name: 'cats', - ) - end - - it "includes the id in the URL" do - expect(category.url_with_id).to eq("/c/cats/#{category.id}") - end - - context "child category" do - fab!(:child_category) do - Fabricate( - :category, - parent_category_id: category.id, - name: 'dogs', - ) - end - - it "includes the id in the URL" do - expect(child_category.url_with_id).to eq("/c/cats/dogs/#{child_category.id}") + .to eq("/c/root/child/child-of-child/#{sub_sub_category.id}") end end end diff --git a/spec/models/permalink_spec.rb b/spec/models/permalink_spec.rb index 869d38fba5f..709768e5d03 100644 --- a/spec/models/permalink_spec.rb +++ b/spec/models/permalink_spec.rb @@ -63,7 +63,7 @@ describe Permalink do it "returns a category url when category_id is set" do permalink.category_id = category.id - expect(target_url).to eq("#{category.url}/#{category.id}") + expect(target_url).to eq("#{category.url}") end it "returns nil when category_id is set but category is not found" do diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb index a4825fb101a..d920723b145 100644 --- a/spec/requests/categories_controller_spec.rb +++ b/spec/requests/categories_controller_spec.rb @@ -13,7 +13,7 @@ describe CategoriesController do get '/categories', headers: { 'HTTP_USER_AGENT' => 'Googlebot' } html = Nokogiri::HTML5(response.body) expect(html.css('body.crawler')).to be_present - expect(html.css("a[href=\"/forum/c/#{category.slug}\"]")).to be_present + expect(html.css("a[href=\"/forum/c/#{category.slug}/#{category.id}\"]")).to be_present end it "properly preloads topic list" do diff --git a/spec/requests/category_hashtags_controller_spec.rb b/spec/requests/category_hashtags_controller_spec.rb index f2f62a82ea1..7676ba56423 100644 --- a/spec/requests/category_hashtags_controller_spec.rb +++ b/spec/requests/category_hashtags_controller_spec.rb @@ -18,7 +18,7 @@ describe CategoryHashtagsController do expect(response.status).to eq(200) expect(response.parsed_body).to eq( - "valid" => [{ "slug" => category.hashtag_slug, "url" => category.url_with_id }] + "valid" => [{ "slug" => category.hashtag_slug, "url" => category.url }] ) end @@ -45,7 +45,7 @@ describe CategoryHashtagsController do expect(response.status).to eq(200) expect(response.parsed_body).to eq( - "valid" => [{ "slug" => private_category.hashtag_slug, "url" => private_category.url_with_id }] + "valid" => [{ "slug" => private_category.hashtag_slug, "url" => private_category.url }] ) end end