From 20a3498781d73c5a2f5c797506c901a19756b776 Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Tue, 25 Sep 2018 18:40:24 +0100 Subject: [PATCH 1/3] Link to people instead of ministers Ministers is now deprecated [1] as it serves the same purpose as people links and these contain the same data. This allows widening up to links to people with other role appointments. As people can not have a base_path this also removes links that don't have pages. [1]: https://github.com/alphagov/govuk-content-schemas/blob/76fe9441a2b9ec6f030735f012347dbe8eb6e331/formats/news_article.jsonnet#L47 --- app/presenters/content_item/linkable.rb | 8 ++-- test/presenters/content_item/linkable_test.rb | 44 +++++++++++++++++++ test/presenters/publication_presenter_test.rb | 4 +- 3 files changed, 50 insertions(+), 6 deletions(-) create mode 100644 test/presenters/content_item/linkable_test.rb diff --git a/app/presenters/content_item/linkable.rb b/app/presenters/content_item/linkable.rb index 6766b63e6..945d1e829 100644 --- a/app/presenters/content_item/linkable.rb +++ b/app/presenters/content_item/linkable.rb @@ -3,7 +3,7 @@ module Linkable include ActionView::Helpers::UrlHelper def from - organisations_ordered_by_importance + links_group(%w{worldwide_organisations ministers speaker}) + organisations_ordered_by_importance + links_group(%w{worldwide_organisations people speaker}) end def part_of @@ -21,9 +21,9 @@ def part_of private def links(type) - expanded_links_from_content_item(type).map do |link| - link_for_type(type, link) - end + expanded_links_from_content_item(type) + .select { |link| link["base_path"] || type == "world_locations" } + .map { |link| link_for_type(type, link) } end def expanded_links_from_content_item(type) diff --git a/test/presenters/content_item/linkable_test.rb b/test/presenters/content_item/linkable_test.rb new file mode 100644 index 000000000..24d0bcffc --- /dev/null +++ b/test/presenters/content_item/linkable_test.rb @@ -0,0 +1,44 @@ +require 'test_helper' + +class ContentItemLinkableTest < ActiveSupport::TestCase + class DummyContentItem + include ContentItem::Linkable + attr_accessor :content_item, :title + + def initialize + @content_item = { + "base_path" => "/a/base/path", + "links" => {}, + } + @title = "A Title" + end + end + + test "when people links have base_paths they are linked to" do + item = DummyContentItem.new + item.content_item["links"]["people"] = [ + { + "title" => "Winston Churchill", + "base_path" => "/government/people/winston-churchill", + } + ] + + expected_from_links = [ + %{Winston Churchill} + ] + assert_equal expected_from_links, item.from + end + + test "when people links don't have base_paths they are skipped" do + item = DummyContentItem.new + item.content_item["links"]["people"] = [ + { + "title" => "Winston Churchill", + "base_path" => nil, + } + ] + + expected_from_links = [] + assert_equal expected_from_links, item.from + end +end diff --git a/test/presenters/publication_presenter_test.rb b/test/presenters/publication_presenter_test.rb index f59349c50..df24aee5e 100644 --- a/test/presenters/publication_presenter_test.rb +++ b/test/presenters/publication_presenter_test.rb @@ -35,8 +35,8 @@ def schema_name assert presented.historically_political? end - test '#from presents ministers' do - minister = schema_item["links"]["ministers"][0] + test '#from presents people' do + minister = schema_item["links"]["people"][0] assert presented_item.from.include?("#{minister['title']}") end From fd4fb006b569ce55a6cc54d136f19b283aa37574 Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Tue, 25 Sep 2018 19:28:46 +0100 Subject: [PATCH 2/3] Move dummy class inside test class This namespaces the DummyContentItem class inside the ContentItemShareableTest class. This is done so that it doesn't use the global namesapce of DummyContentItem which restricted this pattern to only this single test (other tests that used it would break depending on load order). --- test/presenters/content_item/shareable_test.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/presenters/content_item/shareable_test.rb b/test/presenters/content_item/shareable_test.rb index 596e76a20..f653fc2af 100644 --- a/test/presenters/content_item/shareable_test.rb +++ b/test/presenters/content_item/shareable_test.rb @@ -1,18 +1,18 @@ require 'test_helper' include ERB::Util -class DummyContentItem - include ContentItem::Shareable - attr_accessor :content_item, :title +class ContentItemShareableTest < ActiveSupport::TestCase + class DummyContentItem + include ContentItem::Shareable + attr_accessor :content_item, :title - def initialize - @content_item = {} - @content_item["base_path"] = "/a/base/path" - @title = "A Title" + def initialize + @content_item = {} + @content_item["base_path"] = "/a/base/path" + @title = "A Title" + end end -end -class ContentItemShareableTest < ActiveSupport::TestCase def expected_path url_encode(Plek.current.website_root + "/a/base/path") end From 2b3ddea040d3e9cfe0efb4fca3e595df27719b05 Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Wed, 26 Sep 2018 17:54:17 +0100 Subject: [PATCH 3/3] Add test for weird world locations behaviour Added in to explain a bit of the oddity here since it could get lost in the annals of time. --- test/presenters/content_item/linkable_test.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/presenters/content_item/linkable_test.rb b/test/presenters/content_item/linkable_test.rb index 24d0bcffc..156166ff1 100644 --- a/test/presenters/content_item/linkable_test.rb +++ b/test/presenters/content_item/linkable_test.rb @@ -41,4 +41,22 @@ def initialize expected_from_links = [] assert_equal expected_from_links, item.from end + + # World locations don't have links in the Publishing API payload + # This weird situation is explained here: + # - https://github.com/alphagov/government-frontend/pull/386 + test "when a world location is linked to" do + item = DummyContentItem.new + item.content_item["links"]["world_locations"] = [ + { + "title" => "Germany", + "base_path" => nil, + } + ] + + expected_from_links = [ + %{Germany} + ] + assert_equal expected_from_links, item.part_of + end end