Update NavigationLink handling (#12316)

* Update copy

* Allow relative URLs and normalize on save

* Update specs

* Move clarification from index to form

* Add data update script

* Update spec

app/models/navigation_link.rb

@@ -1,10 +1,33 @@
 class NavigationLink < ApplicationRecord
   SVG_REGEXP = /<svg .*>/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

app/views/admin/navigation_links/_form.html.erb

@@ -6,7 +6,7 @@
 <div class="form-group">
   <%= form.label :url %>
   <%= form.text_field :url, class: "form-control" %>
-  <div class="alert alert-info">A full URL for the link</div>
+  <div class="alert alert-info">A full (external) or relative (internal) URL for the link</div>
 </div>
 <div class="form-group">
   <%= form.label :icon %>

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

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 { "<svg xmlns='http://www.w3.org/2000/svg'/></svg>" }
+
+    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

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

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