diff --git a/app/assets/javascripts/admin/addon/components/admin-permalink-form.gjs b/app/assets/javascripts/admin/addon/components/admin-permalink-form.gjs index f5e517b88ee..f0e4f0a925d 100644 --- a/app/assets/javascripts/admin/addon/components/admin-permalink-form.gjs +++ b/app/assets/javascripts/admin/addon/components/admin-permalink-form.gjs @@ -45,7 +45,7 @@ export default class AdminFlagsForm extends Component { } else if (!isEmpty(this.args.permalink.category_id)) { permalinkType = "category"; permalinkValue = this.args.permalink.category_id; - } else if (!isEmpty(this.args.permalink.tag_name)) { + } else if (!isEmpty(this.args.permalink.tag_id)) { permalinkType = "tag"; permalinkValue = this.args.permalink.tag_name; } else if (!isEmpty(this.args.permalink.external_url)) { diff --git a/app/controllers/admin/permalinks_controller.rb b/app/controllers/admin/permalinks_controller.rb index d774abe86ba..3e2461ecab5 100644 --- a/app/controllers/admin/permalinks_controller.rb +++ b/app/controllers/admin/permalinks_controller.rb @@ -20,24 +20,14 @@ class Admin::PermalinksController < Admin::AdminController end def create - permalink = - Permalink.create!( - url: permalink_params[:url], - permalink_type: permalink_params[:permalink_type], - permalink_type_value: permalink_params[:permalink_type_value], - ) + permalink = Permalink.create!(permalink_params) render_serialized(permalink, PermalinkSerializer) rescue ActiveRecord::RecordInvalid => e render_json_error(e.record.errors.full_messages) end def update - @permalink.update!( - url: permalink_params[:url], - permalink_type: permalink_params[:permalink_type], - permalink_type_value: permalink_params[:permalink_type_value], - ) - + @permalink.update!(permalink_params) render_serialized(@permalink, PermalinkSerializer) rescue ActiveRecord::RecordInvalid => e render_json_error(e.record.errors.full_messages) @@ -55,6 +45,24 @@ class Admin::PermalinksController < Admin::AdminController end def permalink_params - params.require(:permalink).permit(:url, :permalink_type, :permalink_type_value) + permitted_params = + params.require(:permalink).permit(:url, :permalink_type, :permalink_type_value) + + { + url: permitted_params[:url], + topic_id: extract_param(permitted_params, "topic"), + post_id: extract_param(permitted_params, "post"), + category_id: extract_param(permitted_params, "category"), + tag_id: + extract_param(permitted_params, "tag").then do |tag_name| + (Tag.where(name: tag_name).pluck(:id).first || -1) if tag_name + end, + user_id: extract_param(permitted_params, "user"), + external_url: extract_param(permitted_params, "external_url"), + } + end + + def extract_param(permitted_params, type) + permitted_params[:permalink_type] == type ? permitted_params[:permalink_type_value] : nil end end diff --git a/app/models/permalink.rb b/app/models/permalink.rb index 0f22c1ae33e..da3465f19f8 100644 --- a/app/models/permalink.rb +++ b/app/models/permalink.rb @@ -1,30 +1,16 @@ # frozen_string_literal: true class Permalink < ActiveRecord::Base - attr_accessor :permalink_type, :permalink_type_value - belongs_to :topic belongs_to :post belongs_to :category belongs_to :tag belongs_to :user - before_validation :clear_associations before_validation :normalize_url, :encode_url - before_validation :set_association_value validates :url, uniqueness: true - - validates :topic_id, presence: true, if: Proc.new { |permalink| permalink.topic_type? } - validates :post_id, presence: true, if: Proc.new { |permalink| permalink.post_type? } - validates :category_id, presence: true, if: Proc.new { |permalink| permalink.category_type? } - validates :tag_id, presence: true, if: Proc.new { |permalink| permalink.tag_type? } - validates :user_id, presence: true, if: Proc.new { |permalink| permalink.user_type? } - validates :external_url, presence: true, if: Proc.new { |permalink| permalink.external_url_type? } - - %i[topic post category tag user external_url].each do |association| - define_method("#{association}_type?") { self.permalink_type == association.to_s } - end + validate :exactly_one_association class Normalizer attr_reader :source @@ -114,22 +100,14 @@ class Permalink < ActiveRecord::Base external_url.match?(%r{\A/[^/]}) ? "#{Discourse.base_path}#{external_url}" : external_url end - def clear_associations - self.topic_id = nil - self.post_id = nil - self.category_id = nil - self.user_id = nil - self.tag_id = nil - self.external_url = nil - end - - def set_association_value - self.topic_id = self.permalink_type_value if self.topic_type? - self.post_id = self.permalink_type_value if self.post_type? - self.user_id = self.permalink_type_value if self.user_type? - self.category_id = self.permalink_type_value if self.category_type? - self.external_url = self.permalink_type_value if self.external_url_type? - self.tag_id = Tag.where(name: self.permalink_type_value).first&.id if self.tag_type? + def exactly_one_association + associations = [topic_id, post_id, category_id, tag_id, user_id, external_url] + if associations.compact.size != 1 + errors.add( + :base, + "Exactly one of topic_id, post_id, category_id, tag_id, user_id, or external_url must be set", + ) + end end end diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb index d1d5d9f6d7e..6d6f9eb1d2c 100644 --- a/spec/models/category_spec.rb +++ b/spec/models/category_spec.rb @@ -474,13 +474,13 @@ RSpec.describe Category do end it "deletes permalink when category slug is reused" do - Fabricate(:permalink, url: "/c/bikeshed-category") + Fabricate(:permalink, url: "/c/bikeshed-category", category_id: 42) Fabricate(:category_with_definition, slug: "bikeshed-category") expect(Permalink.count).to eq(0) end it "deletes permalink when sub category slug is reused" do - Fabricate(:permalink, url: "/c/main-category/sub-category") + Fabricate(:permalink, url: "/c/main-category/sub-category", category_id: 42) main_category = Fabricate(:category_with_definition, slug: "main-category") Fabricate( :category_with_definition, diff --git a/spec/models/permalink_spec.rb b/spec/models/permalink_spec.rb index 7f68cd6078c..848c0d35c8c 100644 --- a/spec/models/permalink_spec.rb +++ b/spec/models/permalink_spec.rb @@ -1,6 +1,12 @@ # frozen_string_literal: true RSpec.describe Permalink do + let(:topic) { Fabricate(:topic) } + let(:post) { Fabricate(:post, topic: topic) } + let(:category) { Fabricate(:category) } + let(:tag) { Fabricate(:tag) } + let(:user) { Fabricate(:user) } + describe "normalization" do it "correctly normalizes" do normalizer = Permalink::Normalizer.new("/(\\/hello.*)\\?.*/\\1|/(\\/bye.*)\\?.*/\\1") @@ -13,92 +19,48 @@ RSpec.describe Permalink do describe "new record" do it "strips blanks" do - permalink = described_class.create!(url: " my/old/url ") + permalink = described_class.create!(url: " my/old/url ", topic_id: topic.id) expect(permalink.url).to eq("my/old/url") end it "removes leading slash" do - permalink = described_class.create!(url: "/my/old/url") + permalink = described_class.create!(url: "/my/old/url", topic_id: topic.id) expect(permalink.url).to eq("my/old/url") end it "checks for unique URL" do - permalink = described_class.create(url: "/my/old/url") + permalink = described_class.create(url: "/my/old/url", topic_id: topic.id) expect(permalink.errors[:url]).to be_empty - permalink = described_class.create(url: "/my/old/url") + permalink = described_class.create(url: "/my/old/url", topic_id: topic.id) expect(permalink.errors[:url]).to be_present + permalink = described_class.create(url: "my/old/url", topic_id: topic.id) + expect(permalink.errors[:url]).to be_present + end + + it "ensures that exactly one associated value is set" do permalink = described_class.create(url: "my/old/url") - expect(permalink.errors[:url]).to be_present - end + expect(permalink.errors[:base]).to be_present - it "validates association" do - permalink = described_class.create(url: "/my/old/url", permalink_type: "topic") - expect(permalink.errors[:topic_id]).to be_present + permalink = described_class.create(url: "my/old/url", topic_id: topic.id, post_id: post.id) + expect(permalink.errors[:base]).to be_present - permalink = described_class.create(url: "/my/old/url", permalink_type: "post") - expect(permalink.errors[:post_id]).to be_present - - permalink = described_class.create(url: "/my/old/url", permalink_type: "category") - expect(permalink.errors[:category_id]).to be_present - - permalink = described_class.create(url: "/my/old/url", permalink_type: "user") - expect(permalink.errors[:user_id]).to be_present - - permalink = described_class.create(url: "/my/old/url", permalink_type: "external_url") - expect(permalink.errors[:external_url]).to be_present - - permalink = described_class.create(url: "/my/old/url", permalink_type: "tag") - expect(permalink.errors[:tag_id]).to be_present - end - - it "clears associations when permalink_type changes" do - permalink = described_class.create!(url: " my/old/url ") - - permalink.update!(permalink_type_value: 1, permalink_type: "topic") - expect(permalink.topic_id).to eq(1) - - permalink.update!(permalink_type_value: 1, permalink_type: "post") - expect(permalink.topic_id).to be_nil - expect(permalink.post_id).to eq(1) - - permalink.update!(permalink_type_value: 1, permalink_type: "category") - expect(permalink.post_id).to be_nil - expect(permalink.category_id).to eq(1) - - permalink.update!(permalink_type_value: 1, permalink_type: "user") - expect(permalink.category_id).to be_nil - expect(permalink.user_id).to eq(1) - - permalink.update!( - permalink_type_value: "https://discourse.org", - permalink_type: "external_url", - ) - expect(permalink.user_id).to be_nil - expect(permalink.external_url).to eq("https://discourse.org") - - tag = Fabricate(:tag, name: "art") - permalink.update!(permalink_type_value: "art", permalink_type: "tag") - expect(permalink.external_url).to be_nil - expect(permalink.tag_id).to eq(tag.id) - - permalink.update!(permalink_type_value: 1, permalink_type: "topic") - expect(permalink.tag_id).to be_nil - expect(permalink.topic_id).to eq(1) + permalink = described_class.create(url: "my/old/url", post_id: post.id) + expect(permalink.errors[:base]).to be_empty end context "with special characters in URL" do it "percent encodes any special character" do - permalink = described_class.create!(url: "/2022/10/03/привет-sam") + permalink = described_class.create!(url: "/2022/10/03/привет-sam", topic_id: topic.id) expect(permalink.url).to eq("2022/10/03/%D0%BF%D1%80%D0%B8%D0%B2%D0%B5%D1%82-sam") end it "checks for unique URL" do - permalink = described_class.create(url: "/2022/10/03/привет-sam") + permalink = described_class.create(url: "/2022/10/03/привет-sam", topic_id: topic.id) expect(permalink.errors[:url]).to be_empty - permalink = described_class.create(url: "/2022/10/03/привет-sam") + permalink = described_class.create(url: "/2022/10/03/привет-sam", topic_id: topic.id) expect(permalink.errors[:url]).to be_present end end @@ -108,11 +70,6 @@ RSpec.describe Permalink do subject(:target_url) { permalink.target_url } let(:permalink) { Fabricate.build(:permalink) } - let(:topic) { Fabricate(:topic) } - let(:post) { Fabricate(:post, topic: topic) } - let(:category) { Fabricate(:category) } - let(:tag) { Fabricate(:tag) } - let(:user) { Fabricate(:user) } it "returns nil when nothing is set" do expect(target_url).to eq(nil) diff --git a/spec/requests/admin/permalinks_controller_spec.rb b/spec/requests/admin/permalinks_controller_spec.rb index 3c15a1c8cff..4961aa9b23b 100644 --- a/spec/requests/admin/permalinks_controller_spec.rb +++ b/spec/requests/admin/permalinks_controller_spec.rb @@ -10,20 +10,10 @@ RSpec.describe Admin::PermalinksController do before { sign_in(admin) } it "filters url" do - Fabricate(:permalink, url: "/forum/23", permalink_type_value: "1", permalink_type: "topic") - Fabricate(:permalink, url: "/forum/98", permalink_type_value: "1", permalink_type: "topic") - Fabricate( - :permalink, - url: "/discuss/topic/45", - permalink_type_value: "1", - permalink_type: "topic", - ) - Fabricate( - :permalink, - url: "/discuss/topic/76", - permalink_type_value: "1", - permalink_type: "topic", - ) + Fabricate(:permalink, url: "/forum/23", topic_id: 1) + Fabricate(:permalink, url: "/forum/98", topic_id: 1) + Fabricate(:permalink, url: "/discuss/topic/45", topic_id: 1) + Fabricate(:permalink, url: "/discuss/topic/76", topic_id: 1) get "/admin/permalinks.json", params: { filter: "topic" } @@ -33,26 +23,10 @@ RSpec.describe Admin::PermalinksController do end it "filters external url" do - Fabricate( - :permalink, - permalink_type_value: "http://google.com", - permalink_type: "external_url", - ) - Fabricate( - :permalink, - permalink_type_value: "http://wikipedia.org", - permalink_type: "external_url", - ) - Fabricate( - :permalink, - permalink_type_value: "http://www.discourse.org", - permalink_type: "external_url", - ) - Fabricate( - :permalink, - permalink_type_value: "http://try.discourse.org", - permalink_type: "external_url", - ) + Fabricate(:permalink, external_url: "http://google.com") + Fabricate(:permalink, external_url: "http://wikipedia.org") + Fabricate(:permalink, external_url: "http://www.discourse.org") + Fabricate(:permalink, external_url: "http://try.discourse.org") get "/admin/permalinks.json", params: { filter: "discourse" } @@ -62,30 +36,10 @@ RSpec.describe Admin::PermalinksController do end it "filters url and external url both" do - Fabricate( - :permalink, - url: "/forum/23", - permalink_type_value: "http://google.com", - permalink_type: "external_url", - ) - Fabricate( - :permalink, - url: "/discourse/98", - permalink_type_value: "http://wikipedia.org", - permalink_type: "external_url", - ) - Fabricate( - :permalink, - url: "/discuss/topic/45", - permalink_type_value: "http://discourse.org", - permalink_type: "external_url", - ) - Fabricate( - :permalink, - url: "/discuss/topic/76", - permalink_type_value: "http://try.discourse.org", - permalink_type: "external_url", - ) + Fabricate(:permalink, url: "/forum/23", external_url: "http://google.com") + Fabricate(:permalink, url: "/discourse/98", external_url: "http://wikipedia.org") + Fabricate(:permalink, url: "/discuss/topic/45", external_url: "http://discourse.org") + Fabricate(:permalink, url: "/discuss/topic/76", external_url: "http://try.discourse.org") get "/admin/permalinks.json", params: { filter: "discourse" } diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index f039c957ca1..eb1dc518efe 100644 --- a/spec/requests/application_controller_spec.rb +++ b/spec/requests/application_controller_spec.rb @@ -345,11 +345,7 @@ RSpec.describe ApplicationController do describe "topic not found" do it "should not redirect to permalink if topic/category does not exist" do topic = create_post.topic - Permalink.create!( - url: topic.relative_url, - permalink_type_value: topic.id + 1, - permalink_type: "topic", - ) + Permalink.create!(url: topic.relative_url, topic_id: topic.id + 1) topic.trash! SiteSetting.detailed_404 = false @@ -364,11 +360,7 @@ RSpec.describe ApplicationController do it "should return permalink for deleted topics" do topic = create_post.topic external_url = "https://somewhere.over.rainbow" - Permalink.create!( - url: topic.relative_url, - permalink_type_value: external_url, - permalink_type: "external_url", - ) + Permalink.create!(url: topic.relative_url, external_url: external_url) topic.trash! get topic.relative_url @@ -390,12 +382,7 @@ RSpec.describe ApplicationController do trashed_topic = create_post.topic trashed_topic.trash! new_topic = create_post.topic - permalink = - Permalink.create!( - url: trashed_topic.relative_url, - permalink_type_value: new_topic.id, - permalink_type: "topic", - ) + permalink = Permalink.create!(url: trashed_topic.relative_url, topic_id: new_topic.id) # no subfolder because router doesn't know about subfolder in this test get "/t/#{trashed_topic.slug}/#{trashed_topic.id}" @@ -404,23 +391,14 @@ RSpec.describe ApplicationController do permalink.destroy category = Fabricate(:category) - permalink = - Permalink.create!( - url: trashed_topic.relative_url, - permalink_type_value: category.id, - permalink_type: "category", - ) + permalink = Permalink.create!(url: trashed_topic.relative_url, category_id: category.id) get "/t/#{trashed_topic.slug}/#{trashed_topic.id}" expect(response.status).to eq(301) expect(response).to redirect_to("/forum/c/#{category.slug}/#{category.id}") permalink.destroy permalink = - Permalink.create!( - url: trashed_topic.relative_url, - permalink_type_value: new_topic.posts.last.id, - permalink_type: "post", - ) + Permalink.create!(url: trashed_topic.relative_url, post_id: new_topic.posts.last.id) get "/t/#{trashed_topic.slug}/#{trashed_topic.id}" expect(response.status).to eq(301) expect(response).to redirect_to( diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb index 95d642ef061..1d5e37d80ab 100644 --- a/spec/requests/categories_controller_spec.rb +++ b/spec/requests/categories_controller_spec.rb @@ -44,12 +44,7 @@ RSpec.describe CategoriesController do end it "respects permalinks before redirecting /category paths to /c paths" do - _perm = - Permalink.create!( - url: "category/something", - permalink_type_value: category.id, - permalink_type: "category", - ) + _perm = Permalink.create!(url: "category/something", category_id: category.id) get "/category/something" expect(response).to have_http_status(:moved_permanently) diff --git a/spec/requests/permalinks_controller_spec.rb b/spec/requests/permalinks_controller_spec.rb index 6aa6bc22353..367c209b417 100644 --- a/spec/requests/permalinks_controller_spec.rb +++ b/spec/requests/permalinks_controller_spec.rb @@ -2,12 +2,10 @@ RSpec.describe PermalinksController do fab!(:topic) - fab!(:permalink) { Fabricate(:permalink, url: "deadroute/topic/546") } + fab!(:permalink) { Fabricate(:permalink, url: "deadroute/topic/546", topic_id: topic.id) } describe "show" do it "should redirect to a permalink's target_url with status 301" do - permalink.update!(permalink_type_value: topic.id, permalink_type: "topic") - get "/#{permalink.url}" expect(response).to redirect_to(topic.relative_url) @@ -15,7 +13,6 @@ RSpec.describe PermalinksController do end it "should work for subfolder installs too" do - permalink.update!(permalink_type_value: topic.id, permalink_type: "topic") set_subfolder "/forum" get "/#{permalink.url}" @@ -25,7 +22,7 @@ RSpec.describe PermalinksController do end it "should apply normalizations" do - permalink.update!(permalink_type_value: "/topic/100", permalink_type: "external_url") + permalink.update!(external_url: "/topic/100", topic_id: nil) SiteSetting.permalink_normalizations = "/(.*)\\?.*/\\1" get "/#{permalink.url}", params: { test: "hello" } @@ -46,14 +43,9 @@ RSpec.describe PermalinksController do end context "when permalink's target_url is an external URL" do - before do - permalink.update!( - permalink_type_value: "https://github.com/discourse/discourse", - permalink_type: "external_url", - ) - end - it "redirects to it properly" do + permalink.update!(external_url: "https://github.com/discourse/discourse", topic_id: nil) + get "/#{permalink.url}" expect(response).to redirect_to(permalink.external_url) end