diff --git a/app/models/navigation_link.rb b/app/models/navigation_link.rb index ea41d9911..45f54860a 100644 --- a/app/models/navigation_link.rb +++ b/app/models/navigation_link.rb @@ -1,10 +1,33 @@ class NavigationLink < ApplicationRecord SVG_REGEXP = //i.freeze + before_validation :allow_relative_url, if: :url? + before_save :strip_local_hostname, if: :url? + validates :name, :url, :icon, presence: true validates :url, url: { schemes: %w[https http] }, uniqueness: { scope: :name } validates :icon, format: SVG_REGEXP validates :display_only_when_signed_in, inclusion: { in: [true, false] } scope :ordered, -> { order(position: :asc, name: :asc) } + + private + + # We want to allow relative URLs (e.g. /contact) for navigation links while + # still going through the normal validation process. + def allow_relative_url + parsed_url = URI.parse(url) + return unless parsed_url.relative? && url.starts_with?("/") + + self.url = URI.parse(URL.url).merge(parsed_url).to_s + end + + # When persisting to the database we store local links as relative URLs which + # makes it easier to switch from a forem.cloud subdomain to the live domain. + def strip_local_hostname + parsed_url = URI.parse(url) + return unless url.match?(/^#{URL.url}/i) + + self.url = parsed_url.path + end end diff --git a/app/views/admin/navigation_links/_form.html.erb b/app/views/admin/navigation_links/_form.html.erb index 26202a594..ec067886b 100644 --- a/app/views/admin/navigation_links/_form.html.erb +++ b/app/views/admin/navigation_links/_form.html.erb @@ -6,7 +6,7 @@
<%= form.label :url %> <%= form.text_field :url, class: "form-control" %> -
A full URL for the link
+
A full (external) or relative (internal) URL for the link
<%= form.label :icon %> diff --git a/lib/data_update_scripts/20210119060219_make_local_navigation_links_relative.rb b/lib/data_update_scripts/20210119060219_make_local_navigation_links_relative.rb new file mode 100644 index 000000000..5a431ac78 --- /dev/null +++ b/lib/data_update_scripts/20210119060219_make_local_navigation_links_relative.rb @@ -0,0 +1,12 @@ +module DataUpdateScripts + class MakeLocalNavigationLinksRelative + def run + NavigationLink.find_each do |navigation_link| + parsed_url = URI.parse(navigation_link.url) + next if parsed_url.relative? + + navigation_link.save + end + end + end +end diff --git a/spec/factories/navigation_links.rb b/spec/factories/navigation_links.rb index 771c748a6..7170d970f 100644 --- a/spec/factories/navigation_links.rb +++ b/spec/factories/navigation_links.rb @@ -3,5 +3,15 @@ FactoryBot.define do name { "#{Faker::Book.title} #{rand(1000)}" } url { "#{Faker::Internet.url}/#{rand(1000)}" } icon { "" } + + trait :without_url_normalization do + after(:build) do |navigation_link| + navigation_link.class.skip_callback :save, :before, :strip_local_hostname + end + + after(:create) do |navigation_link| + navigation_link.class.set_callback :save, :before, :strip_local_hostname + end + end end end diff --git a/spec/lib/data_update_scripts/make_local_navigation_links_relative_spec.rb b/spec/lib/data_update_scripts/make_local_navigation_links_relative_spec.rb new file mode 100644 index 000000000..65976b4ea --- /dev/null +++ b/spec/lib/data_update_scripts/make_local_navigation_links_relative_spec.rb @@ -0,0 +1,28 @@ +require "rails_helper" +require Rails.root.join( + "lib/data_update_scripts/20210119060219_make_local_navigation_links_relative.rb", +) + +describe DataUpdateScripts::MakeLocalNavigationLinksRelative do + let(:base_url) { "https://testforem.com" } + + before { allow(URL).to receive(:url).and_return(base_url) } + + it "makes local navigation links relative" do + absolute_url = "#{base_url}/test" + navigation_link = create(:navigation_link, :without_url_normalization, url: absolute_url) + + expect do + described_class.new.run + end.to change { navigation_link.reload.url }.from(absolute_url).to("/test") + end + + it "leaves external navigation links unchanged" do + absolute_url = "https://example.com/test" + navigation_link = create(:navigation_link, url: absolute_url) + + expect do + described_class.new.run + end.not_to change { navigation_link.reload.url } + end +end diff --git a/spec/models/navigation_link_spec.rb b/spec/models/navigation_link_spec.rb index caec0edc3..83f2d33e7 100644 --- a/spec/models/navigation_link_spec.rb +++ b/spec/models/navigation_link_spec.rb @@ -1,6 +1,8 @@ require "rails_helper" RSpec.describe NavigationLink, type: :model do + let(:navigation_link) { create(:navigation_link) } + describe "validations" do describe "presence validations" do it { is_expected.to validate_presence_of(:name) } @@ -8,18 +10,46 @@ RSpec.describe NavigationLink, type: :model do it { is_expected.to validate_presence_of(:icon) } end - describe "regex validations" do - let(:navigation_link) { create(:navigation_link) } + it "validates the icon" do + navigation_link.icon = "test.png" + expect(navigation_link).not_to be_valid + end - it "vaidates the url" do + context "when validating the URL" do + it "does not allow invalid URLs" do navigation_link.url = "test" expect(navigation_link).not_to be_valid end - it "vaidates the icon" do - navigation_link.icon = "test.png" - expect(navigation_link).not_to be_valid + it "does allow relative URLs" do + navigation_link.url = "/test" + expect(navigation_link).to be_valid end end end + + describe "callbacks" do + let(:base_url) { "https://testforem.com" } + + before { allow(URL).to receive(:url).and_return(base_url) } + + it "normalizes local URLs to relative URLs on save" do + navigation_link.url = "#{base_url}/test" + navigation_link.save + expect(navigation_link.url).to eq "/test" + end + + it "persists relative URLs unchanged" do + navigation_link.url = "/test" + navigation_link.save + expect(navigation_link.url).to eq "/test" + end + + it "persists external URLs unchanged" do + url = "https://example.com/test" + navigation_link.url = url + navigation_link.save + expect(navigation_link.url).to eq url + end + end end