From 387e906610323fe9bbce11d87ca501799f99a226 Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Mon, 27 May 2024 21:30:19 +0200 Subject: [PATCH] REFACTOR: All kinds of permalinks should return relative URLs Mixing relative and absolute URLs is unnecessary and confusing. --- app/models/category.rb | 2 + app/models/permalink.rb | 8 +- app/models/tag.rb | 2 + app/models/user.rb | 4 + spec/models/permalink_spec.rb | 179 +++++++++++++++++++++------------- 5 files changed, 124 insertions(+), 71 deletions(-) diff --git a/app/models/category.rb b/app/models/category.rb index 8c562260cc6..8ff20b03d0c 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -946,6 +946,8 @@ class Category < ActiveRecord::Base end end + alias_method :relative_url, :url + # If the name changes, try and update the category definition topic too if it's an exact match def rename_category_definition return if topic.blank? diff --git a/app/models/permalink.rb b/app/models/permalink.rb index e2128c83263..d5909b4aaac 100644 --- a/app/models/permalink.rb +++ b/app/models/permalink.rb @@ -72,11 +72,11 @@ class Permalink < ActiveRecord::Base def target_url return external_url if external_url - return "#{Discourse.base_path}#{post.url}" if post + return post.relative_url if post return topic.relative_url if topic - return category.url if category - return tag.full_url if tag - return user.full_url if user + return category.relative_url if category + return tag.relative_url if tag + return user.relative_url if user nil end diff --git a/app/models/tag.rb b/app/models/tag.rb index 493339d06ec..8637eac2be9 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -210,6 +210,8 @@ class Tag < ActiveRecord::Base "#{Discourse.base_path}/tag/#{UrlHelper.encode_component(self.name)}" end + alias_method :relative_url, :url + def full_url "#{Discourse.base_url}/tag/#{UrlHelper.encode_component(self.name)}" end diff --git a/app/models/user.rb b/app/models/user.rb index cf8324981cb..7fc0115b371 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1779,6 +1779,10 @@ class User < ActiveRecord::Base username_lower == User.normalize_username(another_username) end + def relative_url + "#{Discourse.base_path}/u/#{encoded_username}" + end + def full_url "#{Discourse.base_url}/u/#{encoded_username}" end diff --git a/spec/models/permalink_spec.rb b/spec/models/permalink_spec.rb index 3ca6a715d39..116989ac863 100644 --- a/spec/models/permalink_spec.rb +++ b/spec/models/permalink_spec.rb @@ -33,79 +33,124 @@ RSpec.describe Permalink do let(:tag) { Fabricate(:tag) } let(:user) { Fabricate(:user) } - it "returns a topic url when topic_id is set" do - permalink.topic_id = topic.id - expect(target_url).to eq(topic.relative_url) - end - - it "returns nil when topic_id is set but topic is not found" do - permalink.topic_id = 99_999 - expect(target_url).to eq(nil) - end - - it "returns a post url when post_id is set" do - permalink.post_id = post.id - expect(target_url).to eq(post.url) - end - - it "returns nil when post_id is set but post is not found" do - permalink.post_id = 99_999 - expect(target_url).to eq(nil) - end - - it "returns a post url when post_id and topic_id are both set" do - permalink.post_id = post.id - permalink.topic_id = topic.id - expect(target_url).to eq(post.url) - end - - it "returns a category url when category_id is set" do - permalink.category_id = category.id - expect(target_url).to eq("#{category.url}") - end - - it "returns nil when category_id is set but category is not found" do - permalink.category_id = 99_999 - expect(target_url).to eq(nil) - end - - it "returns a post url when topic_id, post_id, and category_id are all set for some reason" do - permalink.post_id = post.id - permalink.topic_id = topic.id - permalink.category_id = category.id - expect(target_url).to eq(post.url) - end - - it "returns a tag url when tag_id is set" do - permalink.tag_id = tag.id - expect(target_url).to eq(tag.full_url) - end - - it "returns nil when tag_id is set but tag is not found" do - permalink.tag_id = 99_999 - expect(target_url).to eq(nil) - end - - it "returns a post url when topic_id, post_id, category_id and tag_id are all set for some reason" do - permalink.post_id = post.id - permalink.topic_id = topic.id - permalink.category_id = category.id - permalink.tag_id = tag.id - expect(target_url).to eq(post.url) - end - it "returns nil when nothing is set" do expect(target_url).to eq(nil) end - it "returns a user url when user_id is set" do - permalink.user_id = user.id - expect(target_url).to eq(user.full_url) + context "when `topic_id` is set" do + it "returns an absolute path" do + permalink.topic_id = topic.id + expect(target_url).to eq(topic.relative_url) + expect(target_url).not_to start_with("http") + end + + it "returns nil when topic is not found" do + permalink.topic_id = 99_999 + expect(target_url).to eq(nil) + end end - it "returns nil when user_id is set but user is not found" do - permalink.user_id = 99_999 - expect(target_url).to eq(nil) + context "when `post_id` is set" do + it "returns an absolute path" do + permalink.post_id = post.id + expect(target_url).to eq(post.relative_url) + expect(target_url).not_to start_with("http") + end + + it "returns nil when post is not found" do + permalink.post_id = 99_999 + expect(target_url).to eq(nil) + end + end + + context "when `category_id` is set" do + it "returns an absolute path" do + permalink.category_id = category.id + expect(target_url).to eq(category.url) + expect(target_url).not_to start_with("http") + end + + it "returns nil when category is not found" do + permalink.category_id = 99_999 + expect(target_url).to eq(nil) + end + end + + context "when `tag_id` is set" do + it "returns an absolute path" do + permalink.tag_id = tag.id + expect(target_url).to eq(tag.relative_url) + expect(target_url).not_to start_with("http") + end + + it "returns nil when tag is not found" do + permalink.tag_id = 99_999 + expect(target_url).to eq(nil) + end + end + + context "when `user_id` is set" do + it "returns an absolute path" do + permalink.user_id = user.id + expect(target_url).to eq(user.relative_url) + expect(target_url).not_to start_with("http") + end + + it "returns nil when user is not found" do + permalink.user_id = 99_999 + expect(target_url).to eq(nil) + end + end + + context "when `external_url` is set" do + it "returns a URL when an absolute URL is set" do + permalink.external_url = "https://example.com" + expect(target_url).to eq("https://example.com") + end + + it "returns a protocol-relative URL when a PRURL is set" do + permalink.external_url = "//example.com" + expect(target_url).to eq("//example.com") + end + + it "returns an absolute path when an absolute path is set" do + permalink.external_url = "/my/preferences" + expect(target_url).to eq("/my/preferences") + end + + it "returns a relative path when a relative path is set" do + permalink.external_url = "foo/bar" + expect(target_url).to eq("foo/bar") + end + end + + it "returns the highest priority url when multiple attributes are set" do + permalink.external_url = "/my/preferences" + permalink.post = post + permalink.topic = topic + permalink.category = category + permalink.tag = tag + permalink.user = user + + expect(permalink.target_url).to eq("/my/preferences") + + permalink.external_url = nil + expect(permalink.target_url).to eq(post.relative_url) + + permalink.post = nil + expect(permalink.target_url).to eq(topic.relative_url) + + permalink.topic = nil + expect(permalink.target_url).to eq(category.relative_url) + + permalink.category = nil + expect(permalink.target_url).to eq(tag.relative_url) + + permalink.tag = nil + expect(permalink.target_url).to eq(user.relative_url) + + permalink.user = nil + expect(permalink.target_url).to be_nil end end end