Fix sitemap generating correct root urls for default and additional languages

Author: Marc Antwertinger

app/helpers/alchemy/url_helper.rb

@@ -8,20 +8,12 @@ module Alchemy
   module UrlHelper
     # Returns the path for rendering an alchemy page
     def show_alchemy_page_path(page, optional_params = {})
-      alchemy.show_page_path(show_page_path_params(page, optional_params))
+      page.url_path(optional_params)
     end
 
     # Returns the url for rendering an alchemy page
     def show_alchemy_page_url(page, optional_params = {})
-      alchemy.show_page_url(show_page_path_params(page, optional_params))
-    end
-
-    # Returns the correct params-hash for passing to show_page_path
-    def show_page_path_params(page, optional_params = {})
-      raise ArgumentError, "Page is nil" if page.nil?
-
-      url_params = {urlname: page.urlname}.update(optional_params)
-      prefix_locale?(page.language_code) ? url_params.update(locale: page.language_code) : url_params
+      url_for(page.url_path(optional_params))
     end
 
     # Returns the path for downloading an alchemy attachment

app/models/alchemy/page.rb

@@ -301,8 +301,8 @@ def find_elements(options = {})
     # = The url_path for this page
     #
     # @see Alchemy::Page::UrlPath#call
-    def url_path
-      self.class.url_path_class.new(self).call
+    def url_path(optional_params = {})
+      self.class.url_path_class.new(self, optional_params).call
     end
 
     # The page's view partial is dependent from its page layout

app/models/alchemy/page/url_path.rb

@@ -20,20 +20,29 @@ class Page
     #     link_to page.url
     #
     class UrlPath
-      def initialize(page)
+      def initialize(page, optional_params = {})
         @page = page
         @language = @page.language
         @site = @language.site
+        @optional_params = optional_params
       end
 
       def call
-        if @page.language_root?
+        path = if @page.language_root?
           language_root_path
         elsif @site.languages.count(&:public?) > 1
           page_path_with_language_prefix
         else
           page_path_with_leading_slash
         end
+
+        if @optional_params.present?
+          uri = URI(path)
+          uri.query = @optional_params.to_query
+          uri.to_s
+        else
+          path
+        end
       end
 
       private

app/views/alchemy/language_links/_language.html.erb

@@ -1,8 +1,7 @@
 <%= link_to(
   content_tag(:span, language.label(options[:linkname])).html_safe,
   show_alchemy_page_path(
-    language.pages.language_roots.first,
-    locale: prefix_locale?(language.code) ? language.code : nil
+    language.pages.language_roots.first
   ),
   class: [
     language.code,

spec/helpers/alchemy/pages_helper_spec.rb

@@ -239,8 +239,7 @@ module Alchemy
 
           it "should render language links referring to their language root page" do
             code = klingon_language_root.language_code
-            urlname = klingon_language_root.urlname
-            expect(helper.language_links).to have_selector("a.#{code}[href='/#{code}/#{urlname}']")
+            expect(helper.language_links).to have_selector("a.#{code}[href='/#{code}']")
           end
 
           context "with options[:linkname]" do

spec/helpers/alchemy/url_helper_spec.rb

@@ -9,60 +9,11 @@ module Alchemy
     let(:page) { mock_model(Page, urlname: "testpage", language_code: "en") }
 
     context "page path helpers" do
-      describe "#show_page_path_params" do
-        subject(:show_page_path_params) { helper.show_page_path_params(page) }
-
-        context "if prefix_locale? is false" do
-          before do
-            expect(helper).to receive(:prefix_locale?).with(page.language_code) { false }
-          end
-
-          it "returns a Hash with urlname and no locale parameter" do
-            expect(show_page_path_params).to include(urlname: "testpage")
-            expect(show_page_path_params).to_not include(locale: "en")
-          end
-
-          context "with additional parameters" do
-            subject(:show_page_path_params) do
-              helper.show_page_path_params(page, {query: "test"})
-            end
-
-            it "returns a Hash with urlname, no locale and query parameter" do
-              expect(show_page_path_params).to \
-                include(urlname: "testpage", query: "test")
-              expect(show_page_path_params).to_not \
-                include(locale: "en")
-            end
-          end
-        end
-
-        context "if prefix_locale? is false" do
-          before do
-            expect(helper).to receive(:prefix_locale?).with(page.language_code) { true }
-          end
-
-          it "returns a Hash with urlname and locale parameter" do
-            expect(show_page_path_params).to \
-              include(urlname: "testpage", locale: "en")
-          end
-
-          context "with additional parameters" do
-            subject(:show_page_path_params) do
-              helper.show_page_path_params(page, {query: "test"})
-            end
-
-            it "returns a Hash with urlname, locale and query parameter" do
-              expect(show_page_path_params).to \
-                include(urlname: "testpage", locale: "en", query: "test")
-            end
-          end
-        end
-      end
-
       describe "#show_alchemy_page_path" do
         context "when prefix_locale? set to true" do
           before do
-            expect(helper).to receive(:prefix_locale?).with(page.language_code) { true }
+            allow(page).to receive(:url_path).with({}).and_return("/#{page.language_code}/testpage")
+            allow(page).to receive(:url_path).with({query: "test"}).and_return("/#{page.language_code}/testpage?query=test")
           end
 
           it "should return the correct relative path string" do
@@ -77,7 +28,8 @@ module Alchemy
 
         context "when prefix_locale? set to false" do
           before do
-            expect(helper).to receive(:prefix_locale?).with(page.language_code) { false }
+            allow(page).to receive(:url_path).with({}).and_return("/testpage")
+            allow(page).to receive(:url_path).with({query: "test"}).and_return("/testpage?query=test")
           end
 
           it "should return the correct relative path string" do
@@ -94,33 +46,35 @@ module Alchemy
       describe "#show_alchemy_page_url" do
         context "when prefix_locale? set to true" do
           before do
-            expect(helper).to receive(:prefix_locale?).with(page.language_code) { true }
+            allow(page).to receive(:url_path).with({}).and_return("/#{page.language_code}/testpage")
+            allow(page).to receive(:url_path).with({query: "test"}).and_return("/#{page.language_code}/testpage?query=test")
           end
 
           it "should return the correct url string" do
             expect(helper.show_alchemy_page_url(page)).to \
-              eq("http://#{helper.request.host}/#{page.language_code}/testpage")
+              eq("/#{page.language_code}/testpage")
           end
 
           it "should return the correct url string with additional parameters" do
             expect(helper.show_alchemy_page_url(page, {query: "test"})).to \
-              eq("http://#{helper.request.host}/#{page.language_code}/testpage?query=test")
+              eq("/#{page.language_code}/testpage?query=test")
           end
         end
 
         context "when prefix_locale? set to false" do
           before do
-            expect(helper).to receive(:prefix_locale?).with(page.language_code) { false }
+            allow(page).to receive(:url_path).with({}).and_return("/testpage")
+            allow(page).to receive(:url_path).with({query: "test"}).and_return("/testpage?query=test")
           end
 
           it "should return the correct url string" do
             expect(helper.show_alchemy_page_url(page)).to \
-              eq("http://#{helper.request.host}/testpage")
+              eq("/testpage")
           end
 
           it "should return the correct url string with additional parameter" do
             expect(helper.show_alchemy_page_url(page, {query: "test"})).to \
-              eq("http://#{helper.request.host}/testpage?query=test")
+              eq("/testpage?query=test")
           end
         end
       end

spec/models/alchemy/page/url_path_spec.rb

@@ -66,6 +66,16 @@
     end
   end
 
+  context "with optional params" do
+    subject(:url) { described_class.new(page, {foo: "bar"}).call }
+
+    let(:page) { create(:alchemy_page) }
+
+    it "appends params as query string" do
+      is_expected.to eq("/#{page.urlname}?foo=bar")
+    end
+  end
+
   context "mounted on a non-root path" do
     let(:page) do
       create(:alchemy_page)

spec/requests/alchemy/sitemap_spec.rb

@@ -24,34 +24,22 @@
   end
 
   context "in multi language mode" do
-    let!(:root) { page.parent }
-    let!(:pages) { [root, page] }
-
-    before do
-      allow_any_instance_of(Alchemy::BaseController).to receive("prefix_locale?") { true }
-    end
+    let!(:klingon) { create(:alchemy_language, :klingon) }
+    let!(:klingon_page) { create(:alchemy_page, :public, sitemap: true, language: klingon) }
 
     it "links in sitemap has locale code included" do
       get "/sitemap.xml"
       xml_doc = Nokogiri::XML(response.body)
-      xml_doc.css("urlset url").each_with_index do |node, i|
-        page = pages[i]
-        expect(node.css("loc").text).to match(/\/#{page.language_code}\//)
-      end
+      klingon_loc = xml_doc.css("urlset url loc").map(&:text).find { |loc| loc.include?(klingon_page.urlname) }
+      expect(klingon_loc).to match(/\/#{klingon.language_code}\//)
     end
 
     context "if the default locale is the page locale" do
-      before do
-        allow_any_instance_of(Alchemy::BaseController).to receive("prefix_locale?") { false }
-      end
-
       it "links in sitemap has no locale code included" do
         get "/sitemap.xml"
         xml_doc = Nokogiri::XML(response.body)
-        xml_doc.css("urlset url").each_with_index do |node, i|
-          page = pages[i]
-          expect(node.css("loc").text).to_not match(/\/#{page.language_code}\//)
-        end
+        default_loc = xml_doc.css("urlset url loc").map(&:text).find { |loc| loc.include?(page.urlname) }
+        expect(default_loc).to_not match(/\/#{page.language_code}\//)
       end
     end
   end