From c9f7c9247c901e77f52d5af881a437272c538da5 Mon Sep 17 00:00:00 2001 From: eddgrant Date: Mon, 30 Oct 2023 13:52:58 +0000 Subject: [PATCH] TRG-1096: Calculate redirects as relative paths when site is configured to use relative links. (cherry picked from commit 177153a647a3b27f021fcfbd9fac953e6e280514) --- lib/govuk_tech_docs/path_helpers.rb | 24 +++++---- lib/govuk_tech_docs/redirects.rb | 7 ++- spec/govuk_tech_docs/pages_spec.rb | 4 +- spec/govuk_tech_docs/path_helpers_spec.rb | 4 +- spec/govuk_tech_docs/redirects_spec.rb | 63 +++++++++++++++++++++++ spec/table_of_contents/helpers_spec.rb | 19 ++++--- 6 files changed, 95 insertions(+), 26 deletions(-) create mode 100644 spec/govuk_tech_docs/redirects_spec.rb diff --git a/lib/govuk_tech_docs/path_helpers.rb b/lib/govuk_tech_docs/path_helpers.rb index 3619f298..762a211e 100644 --- a/lib/govuk_tech_docs/path_helpers.rb +++ b/lib/govuk_tech_docs/path_helpers.rb @@ -1,26 +1,28 @@ require "uri" module GovukTechDocs module PathHelpers - # Some useful notes from https://www.rubydoc.info/github/middleman/middleman/Middleman/Sitemap/Resource#url-instance_method : - # 'resource.path' is "The source path of this resource (relative to the source directory, without template extensions)." - # 'resource.destination_path', which is: "The output path in the build directory for this resource." - # 'resource.url' is based on 'resource.destination_path', but is further tweaked to optionally strip the index file and prefixed with any :http_prefix. - - # Calculates the path to the sought resource, taking in to account whether the site has been configured + # Calculates the path to the sought #resource, taking in to account whether the site has been configured # to generate relative or absolute links. # Identifies whether the sought resource is an internal or external target: External targets are returned untouched. Path calculation is performed for internal targets. # Works for both "Middleman::Sitemap::Resource" resources and plain strings (which may be passed from the site configuration when generating header links). # - # @param [Object] config - # @param [Object] resource - # @param [Object] current_page + # Some useful notes from label[https://www.rubydoc.info/github/middleman/middleman/Middleman/Sitemap/Resource#url-instance_method] : + # * 'resource.path' is "The source path of this resource (relative to the source directory, without template extensions)." + # * 'resource.destination_path', which is: "The output path in the build directory for this resource." + # * 'resource.url' is based on 'resource.destination_path', but is further tweaked to optionally strip the index file and prefixed with any :http_prefix. + # + # @param [Object] config from Middleman::ConfigContext + # @param [Middleman::Sitemap::Resource, String] resource + # @param [Middleman::Sitemap::Resource, String] current_page def get_path_to_resource(config, resource, current_page) + current_page_path = current_page.is_a?(Middleman::Sitemap::Resource) ? current_page.path : current_page + if resource.is_a?(Middleman::Sitemap::Resource) - config[:relative_links] ? get_resource_path_relative_to_current_page(config, current_page.path, resource.path) : resource.url + config[:relative_links] ? get_resource_path_relative_to_current_page(config, current_page_path, resource.path) : resource.url elsif external_url?(resource) resource elsif config[:relative_links] - get_resource_path_relative_to_current_page(config, current_page.path, resource) + get_resource_path_relative_to_current_page(config, current_page_path, resource) else resource end diff --git a/lib/govuk_tech_docs/redirects.rb b/lib/govuk_tech_docs/redirects.rb index d2248e16..7788bf18 100644 --- a/lib/govuk_tech_docs/redirects.rb +++ b/lib/govuk_tech_docs/redirects.rb @@ -1,5 +1,10 @@ +require "govuk_tech_docs/path_helpers" +require "ostruct" + module GovukTechDocs class Redirects + include GovukTechDocs::PathHelpers + LEADING_SLASH = %r{\A/}.freeze def initialize(context) @@ -11,7 +16,7 @@ def redirects all_redirects.map do |from, to| # Middleman needs paths without leading slashes - [from.sub(LEADING_SLASH, ""), { to: to.sub(LEADING_SLASH, "") }] + [from.sub(LEADING_SLASH, ""), { to: get_path_to_resource(@context.config, to.sub(LEADING_SLASH, ""), from.sub(LEADING_SLASH, "")) }] end end diff --git a/spec/govuk_tech_docs/pages_spec.rb b/spec/govuk_tech_docs/pages_spec.rb index 0dfa591d..2d481cd1 100644 --- a/spec/govuk_tech_docs/pages_spec.rb +++ b/spec/govuk_tech_docs/pages_spec.rb @@ -3,7 +3,7 @@ RSpec.describe GovukTechDocs::Pages do describe "#to_json" do it "returns the pages as JSON when using absolute links" do - current_page = double(path: "/api/pages.json") + current_page = create_resource_double(path: "/api/pages.json") sitemap = double(resources: [ create_resource_double(url: "/a.html", data: double(title: "A thing", owner_slack: "#2ndline", last_reviewed_on: Date.yesterday, review_in: "0 days")), create_resource_double(url: "/b.html", data: double(title: "B thing", owner_slack: "#2ndline", last_reviewed_on: Date.yesterday, review_in: "2 days")), @@ -18,7 +18,7 @@ end it "returns the pages as JSON when using relative links" do - current_page = double(path: "/api/pages.json") + current_page = create_resource_double(path: "/api/pages.json") sitemap = double(resources: [ create_resource_double(url: "/a.html", path: "/a.html", data: double(title: "A thing", owner_slack: "#2ndline", last_reviewed_on: Date.yesterday, review_in: "0 days")), create_resource_double(url: "/b/c.html", path: "/b/c.html", data: double(title: "B thing", owner_slack: "#2ndline", last_reviewed_on: Date.yesterday, review_in: "2 days")), diff --git a/spec/govuk_tech_docs/path_helpers_spec.rb b/spec/govuk_tech_docs/path_helpers_spec.rb index 72d4f85d..40394513 100644 --- a/spec/govuk_tech_docs/path_helpers_spec.rb +++ b/spec/govuk_tech_docs/path_helpers_spec.rb @@ -22,7 +22,7 @@ } current_page_path = "/documentation/introduction/section-one/index.html" - current_page = double("current_page", path: current_page_path) + current_page = create_resource_double(url: current_page_path, path: current_page_path) resource = create_resource_double(url: resource_url, path: resource_url) resource_path = get_path_to_resource(config, resource, current_page) @@ -49,7 +49,7 @@ } url = "/documentation/introduction/index.html" - current_page = double("current_page", path: current_page_path) + current_page = create_resource_double(url: current_page_path, path: current_page_path) resource_path = get_path_to_resource(config, url, current_page) expect(resource_path).to eql("../../../documentation/introduction/index.html") diff --git a/spec/govuk_tech_docs/redirects_spec.rb b/spec/govuk_tech_docs/redirects_spec.rb new file mode 100644 index 00000000..c50860a7 --- /dev/null +++ b/spec/govuk_tech_docs/redirects_spec.rb @@ -0,0 +1,63 @@ +require "govuk_tech_docs/doubles" + +RSpec.describe GovukTechDocs::Redirects do + describe "#redirects" do + it "calculates redirects as a relative path from the site root" do + config = { + relative_links: true, + tech_docs: { + redirects: { + # A page has been renamed in an existing folder + "documentation/some-location/old-page.html" => "documentation/some-location/new-page.html", + # A page has been moved between sibling folders + "documentation/old-location/index.html" => "documentation/new-location/index.html", + # A page has been moved and has changed its position in the folder hierarchy + "documentation/some-path/old-location/index.html" => "documentation/new-location/index.html", + # A page has been moved but within some shared common folder hierarchy + "documentation/some-common-path/old-location/index.html" => "documentation/some-common-path/new-location/index.html", + }, + }, + } + context = double("Middleman::ConfigContext") + allow(context).to receive(:config).and_return config + allow(context).to receive_message_chain(:sitemap, :resources).and_return [] + + under_test = described_class.new(context) + redirects = under_test.redirects + + expect(redirects[0][0]).to eql("documentation/some-location/old-page.html") + expect(redirects[0][1][:to]).to eql("../../documentation/some-location/new-page.html") + + expect(redirects[1][0]).to eql("documentation/old-location/index.html") + expect(redirects[1][1][:to]).to eql("../../documentation/new-location/index.html") + + expect(redirects[2][0]).to eql("documentation/some-path/old-location/index.html") + expect(redirects[2][1][:to]).to eql("../../../documentation/new-location/index.html") + + expect(redirects[3][0]).to eql("documentation/some-common-path/old-location/index.html") + expect(redirects[3][1][:to]).to eql("../../../documentation/some-common-path/new-location/index.html") + end + + it "performs no manipulation of redirects when the site is not using relative links" do + old_location = "documentation/old-location/index.html" + new_location = "documentation/new-location/index.html" + config = { + relative_links: false, + tech_docs: { + redirects: { + old_location => new_location, + }, + }, + } + context = double("Middleman::ConfigContext") + allow(context).to receive(:config).and_return config + allow(context).to receive_message_chain(:sitemap, :resources).and_return [] + + under_test = described_class.new(context) + redirects = under_test.redirects + + expect(redirects[0][0]).to eql(old_location) + expect(redirects[0][1][:to]).to eql(new_location) + end + end +end diff --git a/spec/table_of_contents/helpers_spec.rb b/spec/table_of_contents/helpers_spec.rb index 195a95e1..8741010f 100644 --- a/spec/table_of_contents/helpers_spec.rb +++ b/spec/table_of_contents/helpers_spec.rb @@ -1,3 +1,4 @@ +require "govuk_tech_docs/doubles" require "spec_helper" describe GovukTechDocs::TableOfContents::Helpers do @@ -224,11 +225,10 @@ def is_a?(klass) resources[5] = FakeResource.new("/1/5/6/e.html", '

Heading one

Heading two

', 60, "Sub page A", resources[0]) resources[0].add_children [resources[1], resources[2], resources[3], resources[4], resources[5]] - current_page = double("current_page", - data: double("page_frontmatter", description: "The description.", title: "The Title"), - url: "/1/2/3/index.html", - path: "/1/2/3/index.html", - metadata: { locals: {} }) + current_page = create_resource_double(data: double("page_frontmatter", description: "The description.", title: "The Title"), + url: "/1/2/3/index.html", + path: "/1/2/3/index.html", + metadata: { locals: {} }) current_page_html = '

Heading one

Heading two

' @@ -299,11 +299,10 @@ def is_a?(klass) resources[2] = FakeResource.new("/prefix/b.html", '

Heading one

Heading two

', 20, "Sub page B", resources[0]) resources[0].add_children [resources[1], resources[2]] - current_page = double("current_page", - data: double("page_frontmatter", description: "The description.", title: "The Title"), - url: "/prefix/index.html", - path: "/prefix/index.html", - metadata: { locals: {} }) + current_page = create_resource_double(data: double("page_frontmatter", description: "The description.", title: "The Title"), + url: "/prefix/index.html", + path: "/prefix/index.html", + metadata: { locals: {} }) current_page_html = '

Heading one

Heading two

'