From 6d49406a77e12133b0f912e310935ebcb667fe74 Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Wed, 29 Apr 2020 20:29:15 +0100 Subject: [PATCH 1/3] Use presenter helps across all presenter tests This adds a new method in, create_presenter, which has default test arguments set for presenters. This then updates all of the presenter tests so that they all use this helper. The reason for this change is that I intend to add a third argument soon to presenter classes which would mean an increase in test verbosity. I also think this improves consistency of these tests. --- test/presenter_test_helper.rb | 11 +++-- test/presenters/case_study_presenter_test.rb | 2 +- .../presenters/content_item_presenter_test.rb | 45 +++++++++++-------- test/presenters/gone_presenter_test.rb | 19 ++++---- test/presenters/guide_presenter_test.rb | 7 +-- .../html_publication_presenter_test.rb | 3 +- .../choose_sign_in_presenter_test.rb | 11 ++--- .../create_new_account_presenter_test.rb | 15 ++----- .../travel_advice_presenter_test.rb | 7 +-- .../presenters/unpublishing_presenter_test.rb | 25 +++++------ 10 files changed, 73 insertions(+), 72 deletions(-) diff --git a/test/presenter_test_helper.rb b/test/presenter_test_helper.rb index bb767c957..413130781 100644 --- a/test/presenter_test_helper.rb +++ b/test/presenter_test_helper.rb @@ -1,19 +1,24 @@ require "test_helper" class PresenterTestCase < ActiveSupport::TestCase + def create_presenter(presenter_class, + content_item: schema_item("case_study"), + requested_path: "/test-content-item") + presenter_class.new(content_item, requested_path) + end + def schema_name raise NotImplementedError, "Override this method in your test class" end -private - def presented_item(type = schema_name, overrides = {}) example = schema_item(type) present_example(example.merge(overrides)) end def present_example(example) - "#{schema_name.classify}Presenter".safe_constantize.new(example) + create_presenter("#{schema_name.classify}Presenter".safe_constantize, + content_item: example) end def schema_item(type = schema_name) diff --git a/test/presenters/case_study_presenter_test.rb b/test/presenters/case_study_presenter_test.rb index 24c319cca..fe5cb2224 100644 --- a/test/presenters/case_study_presenter_test.rb +++ b/test/presenters/case_study_presenter_test.rb @@ -82,7 +82,7 @@ def schema_name test "#history returns an empty array if the content item is not published" do never_published = schema_item never_published["details"].delete("first_public_at") - presented = CaseStudyPresenter.new(never_published) + presented = create_presenter(CaseStudyPresenter, content_item: never_published) assert_equal [], presented.history end diff --git a/test/presenters/content_item_presenter_test.rb b/test/presenters/content_item_presenter_test.rb index 6e983c3b1..3bbc5fc37 100644 --- a/test/presenters/content_item_presenter_test.rb +++ b/test/presenters/content_item_presenter_test.rb @@ -1,56 +1,65 @@ -require "test_helper" +require "presenter_test_helper" -class ContentItemPresenterTest < ActiveSupport::TestCase +class ContentItemPresenterTest < PresenterTestCase test "#title" do - assert_equal "Title", ContentItemPresenter.new("title" => "Title").title + presenter = create_presenter(ContentItemPresenter, content_item: { "title" => "Title" }) + assert_equal "Title", presenter.title end test "#description" do - assert_equal "Description", ContentItemPresenter.new("description" => "Description").description + presenter = create_presenter(ContentItemPresenter, content_item: { "description" => "Description" }) + assert_equal "Description", presenter.description end test "#schema_name" do - assert_equal "Schema name", ContentItemPresenter.new("schema_name" => "Schema name").schema_name + presenter = create_presenter(ContentItemPresenter, content_item: { "schema_name" => "Schema name" }) + assert_equal "Schema name", presenter.schema_name end test "#locale" do - assert_equal "ar", ContentItemPresenter.new("locale" => "ar").locale + presenter = create_presenter(ContentItemPresenter, content_item: { "locale" => "ar" }) + assert_equal "ar", presenter.locale end test "#document_type" do - assert_equal "Type", ContentItemPresenter.new("document_type" => "Type").document_type + presenter = create_presenter(ContentItemPresenter, content_item: { "document_type" => "Type" }) + assert_equal "Type", presenter.document_type end test "#canonical_url without a part" do - assert_equal "https://www.test.gov.uk/test", ContentItemPresenter.new("base_path" => "/test").canonical_url + presenter = create_presenter(ContentItemPresenter, content_item: { "base_path" => "/test" }) + assert_equal "https://www.test.gov.uk/test", presenter.canonical_url end test "#canonical_url with a part" do example_with_parts = govuk_content_schema_example("travel_advice", "full-country") request_path = example_with_parts["base_path"] + "/safety-and-security" - presented_example = TravelAdvicePresenter.new(example_with_parts, request_path) + presenter = create_presenter(TravelAdvicePresenter, + content_item: example_with_parts, + requested_path: request_path) - assert_equal "https://www.test.gov.uk/foreign-travel-advice/albania/safety-and-security", presented_example.canonical_url + assert_equal "https://www.test.gov.uk/foreign-travel-advice/albania/safety-and-security", presenter.canonical_url end test "available_translations sorts languages by locale with English first" do translated = govuk_content_schema_example("case_study", "translated") - locales = ContentItemPresenter.new(translated).available_translations - assert_equal %w[en ar es], (locales.map { |t| t[:locale] }) + presenter = create_presenter(ContentItemPresenter, content_item: translated) + assert_equal %w[en ar es], (presenter.available_translations.map { |t| t[:locale] }) end test "available_translations returns native locale names using native_language_name_for" do translated = govuk_content_schema_example("case_study", "translated") - locales = ContentItemPresenter.new(translated).available_translations - - assert_equal %w[English العربية Español], (locales.map { |t| t[:text] }) + presenter = create_presenter(ContentItemPresenter, content_item: translated) + assert_equal %w[English العربية Español], (presenter.available_translations.map { |t| t[:text] }) end test "part slug is nil when requesting a content item without parts" do example_without_parts = govuk_content_schema_example("case_study", "translated") - presented_example = ContentItemPresenter.new(example_without_parts, example_without_parts["base_path"]) + presenter = create_presenter(TravelAdvicePresenter, + content_item: example_without_parts, + requested_path: example_without_parts["base_path"]) - assert_not presented_example.requesting_a_part? - assert presented_example.part_slug.nil? + assert_not presenter.requesting_a_part? + assert presenter.part_slug.nil? end end diff --git a/test/presenters/gone_presenter_test.rb b/test/presenters/gone_presenter_test.rb index bc69509bd..495ba41e4 100644 --- a/test/presenters/gone_presenter_test.rb +++ b/test/presenters/gone_presenter_test.rb @@ -1,16 +1,13 @@ -require "test_helper" +require "presenter_test_helper" -class GonePresenterTest < ActiveSupport::TestCase - test "presents the basic details required to display an gone" do - assert_equal gone["details"]["explanation"], presented_gone.explanation - assert_equal gone["details"]["alternative_path"], presented_gone.alternative_path - end - - def gone - govuk_content_schema_example("gone", "gone") +class GonePresenterTest < PresenterTestCase + def schema_name + "gone" end - def presented_gone - GonePresenter.new(gone) + test "presents the basic details required to display an gone" do + gone = schema_item("gone") + assert_equal gone["details"]["explanation"], presented_item.explanation + assert_equal gone["details"]["alternative_path"], presented_item.alternative_path end end diff --git a/test/presenters/guide_presenter_test.rb b/test/presenters/guide_presenter_test.rb index 37c24494a..ae7826b99 100644 --- a/test/presenters/guide_presenter_test.rb +++ b/test/presenters/guide_presenter_test.rb @@ -112,9 +112,10 @@ def presented_item(type = schema_name, part_slug = nil, overrides = {}) schema_example_content_item = schema_item(type) part_slug = "/#{part_slug}" if part_slug - GuidePresenter.new( - schema_example_content_item.merge(overrides), - "#{schema_example_content_item['base_path']}#{part_slug}", + create_presenter( + GuidePresenter, + content_item: schema_example_content_item.merge(overrides), + requested_path: "#{schema_example_content_item['base_path']}#{part_slug}", ) end end diff --git a/test/presenters/html_publication_presenter_test.rb b/test/presenters/html_publication_presenter_test.rb index 58760f666..ec5de568a 100644 --- a/test/presenters/html_publication_presenter_test.rb +++ b/test/presenters/html_publication_presenter_test.rb @@ -33,7 +33,8 @@ def schema_name multiple_organisations_html_publication = schema_item("multiple_organisations") organisation_titles = multiple_organisations_html_publication["links"]["organisations"].map { |o| o["title"] } - presented_unordered_html_publication = HtmlPublicationPresenter.new(multiple_organisations_html_publication) + presented_unordered_html_publication = create_presenter(HtmlPublicationPresenter, + content_item: multiple_organisations_html_publication) presented_organisations = presented_unordered_html_publication.organisations.map { |o| o["title"] } assert_equal organisation_titles, presented_organisations diff --git a/test/presenters/service_sign_in/choose_sign_in_presenter_test.rb b/test/presenters/service_sign_in/choose_sign_in_presenter_test.rb index dda641b7b..4395e8850 100644 --- a/test/presenters/service_sign_in/choose_sign_in_presenter_test.rb +++ b/test/presenters/service_sign_in/choose_sign_in_presenter_test.rb @@ -1,13 +1,14 @@ -require "test_helper" +require "presenter_test_helper" class ServiceSignInPresenterTest - class ChooseSignIn < ActiveSupport::TestCase + class ChooseSignIn < PresenterTestCase def schema_name "service_sign_in" end def setup - @presented_item = present_example(schema_item) + @presented_item = create_presenter(ServiceSignIn::ChooseSignInPresenter, + content_item: schema_item) @choose_sign_in = schema_item["details"]["choose_sign_in"] end @@ -15,10 +16,6 @@ def present_example(example) ServiceSignIn::ChooseSignInPresenter.new(example) end - def schema_item - @schema_item ||= govuk_content_schema_example(schema_name, schema_name) - end - test "presents the schema_name" do assert_equal schema_item["schema_name"], @presented_item.schema_name end diff --git a/test/presenters/service_sign_in/create_new_account_presenter_test.rb b/test/presenters/service_sign_in/create_new_account_presenter_test.rb index 91d71f832..65a553c90 100644 --- a/test/presenters/service_sign_in/create_new_account_presenter_test.rb +++ b/test/presenters/service_sign_in/create_new_account_presenter_test.rb @@ -1,24 +1,17 @@ -require "test_helper" +require "presenter_test_helper" class ServiceSignInPresenterTest - class CreateNewAccount < ActiveSupport::TestCase + class CreateNewAccount < PresenterTestCase def schema_name "service_sign_in" end def setup - @presented_item = present_example(schema_item) + @presented_item = create_presenter(ServiceSignIn::CreateNewAccountPresenter, + content_item: schema_item) @create_new_account = schema_item["details"]["create_new_account"] end - def present_example(example) - ServiceSignIn::CreateNewAccountPresenter.new(example) - end - - def schema_item - @schema_item ||= govuk_content_schema_example(schema_name, schema_name) - end - def parent_slug schema_item["details"]["choose_sign_in"]["slug"] end diff --git a/test/presenters/travel_advice_presenter_test.rb b/test/presenters/travel_advice_presenter_test.rb index 096809471..acbb79d89 100644 --- a/test/presenters/travel_advice_presenter_test.rb +++ b/test/presenters/travel_advice_presenter_test.rb @@ -299,9 +299,10 @@ def presented_item(type = schema_name, part_slug = nil, overrides = {}) schema_example_content_item = schema_item(type) part_slug = "/#{part_slug}" if part_slug - TravelAdvicePresenter.new( - schema_example_content_item.merge(overrides), - "#{schema_example_content_item['base_path']}#{part_slug}", + create_presenter( + TravelAdvicePresenter, + content_item: schema_example_content_item.merge(overrides), + requested_path: "#{schema_example_content_item['base_path']}#{part_slug}", ) end end diff --git a/test/presenters/unpublishing_presenter_test.rb b/test/presenters/unpublishing_presenter_test.rb index 85ba1a612..53127553e 100644 --- a/test/presenters/unpublishing_presenter_test.rb +++ b/test/presenters/unpublishing_presenter_test.rb @@ -1,20 +1,17 @@ -require "test_helper" +require "presenter_test_helper" -class UnpublishingPresenterTest < ActiveSupport::TestCase - test "presents the basic details required to display an unpublishing" do - assert_equal unpublishing["schema_name"], presented_unpublishing.schema_name - assert_equal unpublishing["locale"], presented_unpublishing.locale - assert_equal unpublishing["details"]["explanation"], presented_unpublishing.explanation - - assert_nil unpublishing["details"]["alternative_url"] - assert_nil presented_unpublishing.alternative_url +class UnpublishingPresenterTest < PresenterTestCase + def schema_name + "unpublishing" end - def unpublishing - govuk_content_schema_example("unpublishing", "unpublishing") - end + test "presents the basic details required to display an unpublishing" do + unpublishing = schema_item("unpublishing") + assert_equal unpublishing["schema_name"], presented_item.schema_name + assert_equal unpublishing["locale"], presented_item.locale + assert_equal unpublishing["details"]["explanation"], presented_item.explanation - def presented_unpublishing - UnpublishingPresenter.new(unpublishing) + assert_nil unpublishing["details"]["alternative_url"] + assert_nil presented_item.alternative_url end end From 286b22463c591c89b0225b5571ee9220c4eb0733 Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Wed, 29 Apr 2020 21:30:00 +0100 Subject: [PATCH 2/3] Use requested_path rather than requested_content_item_path This shortens this variable name and I believe it still signifies the same intent as the previous name. The optional nature of it is also removed since every time this class is initialised in app this attribute is set, I imagine this was set as optional for test verbosity but this is no longer an issue. This also fixes a couple of ivars that weren't necessary due to methods existing. --- app/presenters/coming_soon_presenter.rb | 2 +- app/presenters/content_item/parts.rb | 2 +- app/presenters/content_item_presenter.rb | 12 ++++++------ app/presenters/gone_presenter.rb | 2 +- app/presenters/service_sign_in/paths.rb | 2 +- app/presenters/unpublishing_presenter.rb | 2 +- test/presenters/content_item/parts_test.rb | 18 +++++++++--------- .../content_items/attachments.html.erb_test.rb | 9 ++++----- 8 files changed, 24 insertions(+), 25 deletions(-) diff --git a/app/presenters/coming_soon_presenter.rb b/app/presenters/coming_soon_presenter.rb index 8e68554ce..91789d73f 100644 --- a/app/presenters/coming_soon_presenter.rb +++ b/app/presenters/coming_soon_presenter.rb @@ -1,7 +1,7 @@ class ComingSoonPresenter < ContentItemPresenter attr_reader :publish_time - def initialize(content_item, requested_content_item_path = nil) + def initialize(content_item, requested_path = nil) super @publish_time = content_item["details"]["publish_time"] end diff --git a/app/presenters/content_item/parts.rb b/app/presenters/content_item/parts.rb index 818994e48..9e40b94cd 100644 --- a/app/presenters/content_item/parts.rb +++ b/app/presenters/content_item/parts.rb @@ -14,7 +14,7 @@ def parts # with a base path that's different to the one requested. That content # item contains all the parts for that document. def requesting_a_part? - parts.any? && requested_content_item_path && requested_content_item_path != base_path + parts.any? && requested_path && requested_path != base_path end def has_valid_part? diff --git a/app/presenters/content_item_presenter.rb b/app/presenters/content_item_presenter.rb index 82ce6f8dc..041a401a0 100644 --- a/app/presenters/content_item_presenter.rb +++ b/app/presenters/content_item_presenter.rb @@ -4,7 +4,7 @@ class ContentItemPresenter include ApplicationHelper attr_reader :content_item, - :requested_content_item_path, + :requested_path, :base_path, :slug, :title, @@ -19,11 +19,11 @@ class ContentItemPresenter attr_accessor :include_collections_in_other_publisher_metadata - def initialize(content_item, requested_content_item_path = nil) + def initialize(content_item, requested_path) @content_item = content_item - @requested_content_item_path = requested_content_item_path + @requested_path = requested_path @base_path = content_item["base_path"] - @slug = @base_path.delete_prefix("/") if @base_path + @slug = base_path.delete_prefix("/") if base_path @title = content_item["title"] @description = content_item["description"] @schema_name = content_item["schema_name"] @@ -32,7 +32,7 @@ def initialize(content_item, requested_content_item_path = nil) @document_type = content_item["document_type"] @taxons = content_item["links"]["taxons"] if content_item["links"] @step_by_steps = content_item["links"]["part_of_step_navs"] if content_item["links"] - @part_slug = requesting_a_part? ? requested_content_item_path.split("/").last : nil + @part_slug = requesting_a_part? ? requested_path.split("/").last : nil end def requesting_a_part? @@ -44,7 +44,7 @@ def requesting_a_service_sign_in_page? end def available_translations - translations = @content_item["links"]["available_translations"] || [] + translations = content_item["links"]["available_translations"] || [] mapped_locales(sorted_locales(translations)) end diff --git a/app/presenters/gone_presenter.rb b/app/presenters/gone_presenter.rb index 1aac28389..c690f03d1 100644 --- a/app/presenters/gone_presenter.rb +++ b/app/presenters/gone_presenter.rb @@ -1,7 +1,7 @@ class GonePresenter < ContentItemPresenter attr_reader :alternative_path, :explanation - def initialize(content_item, requested_content_item_path = nil) + def initialize(content_item, requested_path = nil) super @explanation = content_item["details"]["explanation"] @alternative_path = content_item["details"]["alternative_path"] diff --git a/app/presenters/service_sign_in/paths.rb b/app/presenters/service_sign_in/paths.rb index 5f3cb02e7..4a4b4b254 100644 --- a/app/presenters/service_sign_in/paths.rb +++ b/app/presenters/service_sign_in/paths.rb @@ -5,7 +5,7 @@ def path end def has_valid_path? - path == @requested_content_item_path + path == requested_path end def requesting_a_service_sign_in_page? diff --git a/app/presenters/unpublishing_presenter.rb b/app/presenters/unpublishing_presenter.rb index 2f7591fb4..eb8eb0640 100644 --- a/app/presenters/unpublishing_presenter.rb +++ b/app/presenters/unpublishing_presenter.rb @@ -1,7 +1,7 @@ class UnpublishingPresenter < ContentItemPresenter attr_reader :alternative_url, :explanation - def initialize(content_item, requested_content_item_path = nil) + def initialize(content_item, requested_path = nil) super @explanation = content_item["details"]["explanation"] @alternative_url = content_item["details"]["alternative_url"] diff --git a/test/presenters/content_item/parts_test.rb b/test/presenters/content_item/parts_test.rb index 5b0d45a08..b8de16575 100644 --- a/test/presenters/content_item/parts_test.rb +++ b/test/presenters/content_item/parts_test.rb @@ -30,7 +30,7 @@ def part_slug nil end - def requested_content_item_path + def requested_path base_path end end @@ -40,7 +40,7 @@ def part_slug "second-slug" end - def requested_content_item_path + def requested_path base_path end end @@ -70,9 +70,9 @@ def content_item assert_not @parts.requesting_a_part? end - test "is not requesting a part when parts exist and base_path matches requested_content_item_path" do + test "is not requesting a part when parts exist and base_path matches requested_path" do class << @parts - def requested_content_item_path + def requested_path base_path end end @@ -80,13 +80,13 @@ def requested_content_item_path assert_not @parts.requesting_a_part? end - test "is requesting a part when part exists and base_path is different to requested_content_item_path" do + test "is requesting a part when part exists and base_path is different to requested_path" do class << @parts def part_slug "second-slug" end - def requested_content_item_path + def requested_path base_path + "/second-slug" end end @@ -100,7 +100,7 @@ def part_slug "second-slug" end - def requested_content_item_path + def requested_path base_path + "/" + part_slug end end @@ -116,7 +116,7 @@ def part_slug "not-a-valid-slug" end - def requested_content_item_path + def requested_path base_path + "/" + part_slug end end @@ -131,7 +131,7 @@ def part_slug "first-slug" end - def requested_content_item_path + def requested_path base_path + "/" + part_slug end end diff --git a/test/views/content_items/attachments.html.erb_test.rb b/test/views/content_items/attachments.html.erb_test.rb index 107543bda..00b4f7648 100644 --- a/test/views/content_items/attachments.html.erb_test.rb +++ b/test/views/content_items/attachments.html.erb_test.rb @@ -12,11 +12,10 @@ class ContentItemsAttachmentsTest < ActionView::TestCase test "can render attachments using their metadata" do @content_item = PublicationPresenter.new( - "details" => { - "attachments" => [{ "id" => "attachment_id", - "title" => "Some title", - "url" => "some/url" }], - }, + { "details" => { "attachments" => [{ "id" => "attachment_id", + "title" => "Some title", + "url" => "some/url" }] } }, + "/publication", ) render(partial: "content_items/attachments", From c57012c5bbb963640daf6c8c135810c20a21bf88 Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Wed, 29 Apr 2020 21:52:19 +0100 Subject: [PATCH 3/3] Share view context with presenters This changes the way presenters work in this application so that they have the equivalent of a view object available to call helpers on. This removes the need to import specific helpers into individual presenters. The reason for adding this in is to fix a number of subtle issues that can occur when it comes to generating URLs without any request context. This problem affected me when I went to looking at adjusting the asset host in order to make progress towards RFC-115 [1]. I found that we'd experience different behaviours on asset url methods depending on the GOVUK_ASSET_ROOT env var. When this env var is not set, as per [2], any calls for an asset_url would return a path rather than URL like we requested; when a GOVUK_ASSET_ROOT env var is set as a hostname without protocol, as per [3], any calls for asset urls would raise a "undefined method `protocol` for nil:NilClass" exception at [4]. These problems don't affect production since GOVUK_ASSET_ROOT is set as a full URL. Anyway, I came to the conclusion that these presenters are effectively just extensions of the view - evidenced by the high proliferation of view helpers embedded - and I felt that it'd be easier to have a view_context object available to call any view helpers we might want. This helps us out when it comes to rendering asset_urls as this view context object has the request context available and can determine the URL based on the request. Were we to use Rails route helpers for navigating the app we'd also be able to drop the various usages of Plek.current.website_root and instead rely on view context to generate URLs. [1]: https://github.com/alphagov/govuk-rfcs/blob/master/rfc-115-enabling-http2-on-govuk.md [2]: https://github.com/alphagov/govuk-docker/blob/4b40e479799ea304709276efb29c49898a8b8c9f/projects/government-frontend/docker-compose.yml#L48-L57 [3]: https://github.com/alphagov/govuk-docker/blob/4b40e479799ea304709276efb29c49898a8b8c9f/projects/government-frontend/docker-compose.yml#L20-L30 [4]: https://github.com/rails/rails/blob/92d03850f3bb4c44103d0b06b43e14d6e270e646/actionview/lib/action_view/helpers/asset_url_helper.rb#L303 --- app/controllers/content_items_controller.rb | 4 +++- app/presenters/coming_soon_presenter.rb | 2 +- app/presenters/content_item/contents_list.rb | 5 +---- .../corporate_information_groups.rb | 14 +++++++----- app/presenters/content_item/linkable.rb | 8 +++---- .../content_item/national_applicability.rb | 2 +- app/presenters/content_item/no_deal_notice.rb | 6 +---- app/presenters/content_item/parts.rb | 22 +++++++++---------- .../content_item/title_and_context.rb | 2 +- app/presenters/content_item/withdrawable.rb | 6 ++--- app/presenters/content_item_presenter.rb | 5 +++-- .../document_collection_presenter.rb | 5 ++++- app/presenters/fatality_notice_presenter.rb | 4 ++-- app/presenters/gone_presenter.rb | 2 +- .../guide_faq_page_schema_presenter.rb | 11 ++++------ .../specialist_document_presenter.rb | 6 +++-- app/presenters/speech_presenter.rb | 2 +- app/presenters/travel_advice_presenter.rb | 8 +++---- app/presenters/unpublishing_presenter.rb | 2 +- app/services/presenter_builder.rb | 9 +++++--- app/views/content_items/guide.html.erb | 2 +- test/presenter_test_helper.rb | 5 +++-- .../content_item/contents_list_test.rb | 2 ++ test/presenters/content_item/linkable_test.rb | 3 ++- test/presenters/content_item/parts_test.rb | 4 ++++ .../content_item/withdrawable_test.rb | 2 ++ .../attachments.html.erb_test.rb | 1 + 27 files changed, 79 insertions(+), 65 deletions(-) diff --git a/app/controllers/content_items_controller.rb b/app/controllers/content_items_controller.rb index 796028460..df15bac2e 100644 --- a/app/controllers/content_items_controller.rb +++ b/app/controllers/content_items_controller.rb @@ -63,7 +63,9 @@ def load_content_item content_item["links"]["ordered_related_items"] = content_item["links"].fetch("suggested_ordered_related_items", []) end - @content_item = PresenterBuilder.new(content_item, content_item_path).presenter + @content_item = PresenterBuilder.new(content_item, + content_item_path, + view_context).presenter end def format_banner_links(links, type) diff --git a/app/presenters/coming_soon_presenter.rb b/app/presenters/coming_soon_presenter.rb index 91789d73f..40cf84160 100644 --- a/app/presenters/coming_soon_presenter.rb +++ b/app/presenters/coming_soon_presenter.rb @@ -1,7 +1,7 @@ class ComingSoonPresenter < ContentItemPresenter attr_reader :publish_time - def initialize(content_item, requested_path = nil) + def initialize(*args) super @publish_time = content_item["details"]["publish_time"] end diff --git a/app/presenters/content_item/contents_list.rb b/app/presenters/content_item/contents_list.rb index 35fc8c057..c420fa877 100644 --- a/app/presenters/content_item/contents_list.rb +++ b/app/presenters/content_item/contents_list.rb @@ -1,8 +1,5 @@ module ContentItem module ContentsList - include ActionView::Helpers::UrlHelper - include TypographyHelper - CHARACTER_LIMIT = 415 CHARACTER_LIMIT_WITH_IMAGE = 224 TABLE_ROW_LIMIT = 13 @@ -37,7 +34,7 @@ def show_contents_list? def extract_headings_with_ids headings = parsed_body.css("h2").map do |heading| id = heading.attribute("id") - { text: strip_trailing_colons(heading.text), id: id.value } if id + { text: view_context.strip_trailing_colons(heading.text), id: id.value } if id end headings.compact end diff --git a/app/presenters/content_item/corporate_information_groups.rb b/app/presenters/content_item/corporate_information_groups.rb index 4576de47d..ece349134 100644 --- a/app/presenters/content_item/corporate_information_groups.rb +++ b/app/presenters/content_item/corporate_information_groups.rb @@ -9,14 +9,18 @@ def corporate_information? def corporate_information corporate_information_groups.map do |group| { - heading: content_tag(:h3, group["name"], id: group_title_id(group["name"])), + heading: view_context.content_tag(:h3, + group["name"], + id: group_title_id(group["name"])), links: normalised_group_links(group), } end end def corporate_information_heading_tag - content_tag(:h2, corporate_information_heading[:text], id: corporate_information_heading[:id]) + view_context.content_tag(:h2, + corporate_information_heading[:text], + id: corporate_information_heading[:id]) end def further_information @@ -33,7 +37,7 @@ def further_information def further_information_link(type) link = corporate_information_page_links.find { |l| l["document_type"] == type } - link_to(link["title"], link["base_path"]) if link + view_context.link_to(link["title"], link["base_path"]) if link end def further_information_about(type) @@ -63,9 +67,9 @@ def normalised_group_links(group) def normalised_group_item_link(group_item) if group_item.is_a?(String) group_item_link = corporate_information_page_links.find { |l| l["content_id"] == group_item } - link_to(group_item_link["title"], group_item_link["base_path"]) + view_context.link_to(group_item_link["title"], group_item_link["base_path"]) else - link_to(group_item["title"], group_item["path"] || group_item["url"]) + view_context.link_to(group_item["title"], group_item["path"] || group_item["url"]) end end diff --git a/app/presenters/content_item/linkable.rb b/app/presenters/content_item/linkable.rb index 07df49d10..ad2751857 100644 --- a/app/presenters/content_item/linkable.rb +++ b/app/presenters/content_item/linkable.rb @@ -1,7 +1,5 @@ module ContentItem module Linkable - include ActionView::Helpers::UrlHelper - def from organisations_ordered_by_importance + links_group(%w[worldwide_organisations people speaker]) end @@ -38,7 +36,7 @@ def links_group(types) def organisations_ordered_by_importance organisations_with_emphasised_first.map do |link| - link_to(link["title"], link["base_path"], class: "govuk-link") + view_context.link_to(link["title"], link["base_path"], class: "govuk-link") end end @@ -56,12 +54,12 @@ def emphasised_organisations def link_for_type(type, link) return link_for_world_location(link) if type == "world_locations" - link_to(link["title"], link["base_path"], class: "govuk-link") + view_context.link_to(link["title"], link["base_path"], class: "govuk-link") end def link_for_world_location(link) base_path = WorldLocationBasePath.for(link) - link_to(link["title"], base_path, class: "govuk-link") + view_context.link_to(link["title"], base_path, class: "govuk-link") end end end diff --git a/app/presenters/content_item/national_applicability.rb b/app/presenters/content_item/national_applicability.rb index 21816f4df..2aa9f28dd 100644 --- a/app/presenters/content_item/national_applicability.rb +++ b/app/presenters/content_item/national_applicability.rb @@ -15,7 +15,7 @@ def applies_to nations_with_alt_urls = inapplicable_nations.select { |n| n["alternative_url"].present? } if nations_with_alt_urls.any? alternate_links = nations_with_alt_urls - .map { |n| link_to(n["label"], n["alternative_url"], rel: :external, class: "govuk-link app-link") } + .map { |n| view_context.link_to(n["label"], n["alternative_url"], rel: :external, class: "govuk-link app-link") } .to_sentence applies_to += " (see #{translated_schema_name(nations_with_alt_urls.count)} for #{alternate_links})" diff --git a/app/presenters/content_item/no_deal_notice.rb b/app/presenters/content_item/no_deal_notice.rb index 9ac9ea340..c671e5899 100644 --- a/app/presenters/content_item/no_deal_notice.rb +++ b/app/presenters/content_item/no_deal_notice.rb @@ -1,9 +1,5 @@ module ContentItem module NoDealNotice - include ActionView::Helpers::TagHelper - include ActionView::Helpers::UrlHelper - include ActionView::Context - def has_no_deal_notice? content_item.dig("details").has_key?("brexit_no_deal_notice") end @@ -46,7 +42,7 @@ def no_deal_landing_page_cta "track-label": "the transition period", } - featured_link = link_to("the transition period", "/transition", data: data_attributes, class: "govuk-link") + featured_link = view_context.link_to("the transition period", "/transition", data: data_attributes, class: "govuk-link") featured_link_intro = no_deal_notice_links.any? ? "You can also read about" : "You can read about" (featured_link_intro + " " + featured_link + ".").html_safe diff --git a/app/presenters/content_item/parts.rb b/app/presenters/content_item/parts.rb index 9e40b94cd..3a7aab1b9 100644 --- a/app/presenters/content_item/parts.rb +++ b/app/presenters/content_item/parts.rb @@ -1,7 +1,5 @@ module ContentItem module Parts - include ActionView::Helpers::UrlHelper - def parts raw_parts.each_with_index.map do |part, i| # Link to base_path for first part @@ -86,15 +84,17 @@ def current_part def part_links parts.map.with_index(1) do |part, position| if part["slug"] != current_part["slug"] - link_to part["title"], part["full_path"], class: "govuk-link", - data: { - track_category: "contentsClicked", - track_action: "content_item #{position}", - track_label: part["full_path"], - track_options: { - dimension29: part["title"], - }, - } + view_context.link_to( + part["title"], + part["full_path"], + class: "govuk-link", + data: { + track_category: "contentsClicked", + track_action: "content_item #{position}", + track_label: part["full_path"], + track_options: { dimension29: part["title"] }, + }, + ) else part["title"] end diff --git a/app/presenters/content_item/title_and_context.rb b/app/presenters/content_item/title_and_context.rb index ae5fed83b..44f17a4bc 100644 --- a/app/presenters/content_item/title_and_context.rb +++ b/app/presenters/content_item/title_and_context.rb @@ -4,7 +4,7 @@ def title_and_context { title: title, context: I18n.t("content_item.schema_name.#{document_type}", count: 1), - context_locale: t_locale_fallback("content_item.schema_name.#{document_type}", count: 1), + context_locale: view_context.t_locale_fallback("content_item.schema_name.#{document_type}", count: 1), average_title_length: "long", } end diff --git a/app/presenters/content_item/withdrawable.rb b/app/presenters/content_item/withdrawable.rb index e3e6855ae..98d73a82d 100644 --- a/app/presenters/content_item/withdrawable.rb +++ b/app/presenters/content_item/withdrawable.rb @@ -1,7 +1,5 @@ module ContentItem module Withdrawable - include ActionView::Helpers::TagHelper - def withdrawn? withdrawal_notice.present? end @@ -35,7 +33,9 @@ def withdrawal_notice_context end def withdrawal_notice_time - content_tag(:time, english_display_date(withdrawal_notice["withdrawn_at"]), datetime: withdrawal_notice["withdrawn_at"]) + view_context.content_tag(:time, + english_display_date(withdrawal_notice["withdrawn_at"]), + datetime: withdrawal_notice["withdrawn_at"]) end def english_display_date(timestamp, format = "%-d %B %Y") diff --git a/app/presenters/content_item_presenter.rb b/app/presenters/content_item_presenter.rb index 041a401a0..f51ef5384 100644 --- a/app/presenters/content_item_presenter.rb +++ b/app/presenters/content_item_presenter.rb @@ -1,10 +1,10 @@ class ContentItemPresenter include ContentItem::Withdrawable include ContentItem::NoDealNotice - include ApplicationHelper attr_reader :content_item, :requested_path, + :view_context, :base_path, :slug, :title, @@ -19,9 +19,10 @@ class ContentItemPresenter attr_accessor :include_collections_in_other_publisher_metadata - def initialize(content_item, requested_path) + def initialize(content_item, requested_path, view_context) @content_item = content_item @requested_path = requested_path + @view_context = view_context @base_path = content_item["base_path"] @slug = base_path.delete_prefix("/") if base_path @title = content_item["title"] diff --git a/app/presenters/document_collection_presenter.rb b/app/presenters/document_collection_presenter.rb index 30c3295fe..ef5a70c73 100644 --- a/app/presenters/document_collection_presenter.rb +++ b/app/presenters/document_collection_presenter.rb @@ -52,7 +52,10 @@ def group_document_links(group, group_index) def group_heading(group) title = group["title"] heading_level = show_contents_list? ? :h3 : :h2 - content_tag heading_level, title, class: "group-title", id: group_title_id(title) + view_context.content_tag(heading_level, + title, + class: "group-title", + id: group_title_id(title)) end private diff --git a/app/presenters/fatality_notice_presenter.rb b/app/presenters/fatality_notice_presenter.rb index 712ef8a3b..cd2fc9e61 100644 --- a/app/presenters/fatality_notice_presenter.rb +++ b/app/presenters/fatality_notice_presenter.rb @@ -12,13 +12,13 @@ def field_of_operation end def image - { "url" => ActionController::Base.helpers.asset_url("ministry-of-defence-crest.png"), "alt_text" => "Ministry of Defence crest" } + { "url" => view_context.image_url("ministry-of-defence-crest.png"), "alt_text" => "Ministry of Defence crest" } end def important_metadata super.tap do |m| if field_of_operation - m.merge!("Field of operation" => link_to(field_of_operation.title, field_of_operation.path, class: "govuk-link app-link")) + m.merge!("Field of operation" => view_context.link_to(field_of_operation.title, field_of_operation.path, class: "govuk-link app-link")) end end end diff --git a/app/presenters/gone_presenter.rb b/app/presenters/gone_presenter.rb index c690f03d1..f92611b50 100644 --- a/app/presenters/gone_presenter.rb +++ b/app/presenters/gone_presenter.rb @@ -1,7 +1,7 @@ class GonePresenter < ContentItemPresenter attr_reader :alternative_path, :explanation - def initialize(content_item, requested_path = nil) + def initialize(*args) super @explanation = content_item["details"]["explanation"] @alternative_path = content_item["details"]["alternative_path"] diff --git a/app/presenters/machine_readable/guide_faq_page_schema_presenter.rb b/app/presenters/machine_readable/guide_faq_page_schema_presenter.rb index 64c7bec6f..d740c3faf 100644 --- a/app/presenters/machine_readable/guide_faq_page_schema_presenter.rb +++ b/app/presenters/machine_readable/guide_faq_page_schema_presenter.rb @@ -1,9 +1,10 @@ module MachineReadable class GuideFaqPageSchemaPresenter - attr_reader :guide + attr_reader :guide, :view_context - def initialize(guide) + def initialize(guide, view_context) @guide = guide + @view_context = view_context end def structured_data @@ -64,11 +65,7 @@ def guide_url end def logo_url - image_url("govuk_publishing_components/govuk-logo.png") - end - - def image_url(image_file) - ActionController::Base.helpers.asset_url(image_file, type: :image) + view_context.image_url("govuk_publishing_components/govuk-logo.png") end end end diff --git a/app/presenters/specialist_document_presenter.rb b/app/presenters/specialist_document_presenter.rb index 6dfe91165..b8aeb78b2 100644 --- a/app/presenters/specialist_document_presenter.rb +++ b/app/presenters/specialist_document_presenter.rb @@ -48,7 +48,7 @@ def will_continue_on def finder_link if finder && statutory_instrument? - link_to "See all #{finder['title']}", finder["base_path"] + view_context.link_to("See all #{finder['title']}", finder["base_path"]) end end @@ -170,7 +170,9 @@ def facet_block(facet, allowed_value) def facet_link(label, value, key) finder_base_path = finder["base_path"] - link_to(label, "#{finder_base_path}?#{key}%5B%5D=#{value}", class: "govuk-link app-link") + view_context.link_to(label, + "#{finder_base_path}?#{key}%5B%5D=#{value}", + class: "govuk-link app-link") end def first_published_at_facet_key diff --git a/app/presenters/speech_presenter.rb b/app/presenters/speech_presenter.rb index e88d0b6f0..6aa091eeb 100644 --- a/app/presenters/speech_presenter.rb +++ b/app/presenters/speech_presenter.rb @@ -39,7 +39,7 @@ def location def delivered_on delivered_on_date = content_item["details"]["delivered_on"] - content_tag(:time, display_date(delivered_on_date), datetime: delivered_on_date) + view_context.content_tag(:time, display_date(delivered_on_date), datetime: delivered_on_date) end def speech_type_explanation diff --git a/app/presenters/travel_advice_presenter.rb b/app/presenters/travel_advice_presenter.rb index 5965f9de2..f4a794013 100644 --- a/app/presenters/travel_advice_presenter.rb +++ b/app/presenters/travel_advice_presenter.rb @@ -1,6 +1,5 @@ class TravelAdvicePresenter < ContentItemPresenter include ContentItem::Parts - include ActionView::Helpers::TextHelper ATOM_CACHE_CONTROL_MAX_AGE = 300 @@ -21,7 +20,7 @@ def metadata "Updated" => display_date(reviewed_at || updated_at), } - other["Latest update"] = simple_format(latest_update) if latest_update.present? + other["Latest update"] = view_context.simple_format(latest_update) if latest_update.present? { other: other, @@ -77,7 +76,8 @@ def alert_status alert_statuses = content_item["details"]["alert_status"] || [] alert_statuses = alert_statuses.map do |alert| if allowed_statuses.include?(alert) - content_tag(:p, I18n.t("travel_advice.alert_status.#{alert}").html_safe) + copy = I18n.t("travel_advice.alert_status.#{alert}").html_safe + view_context.content_tag(:p, copy) end end @@ -85,7 +85,7 @@ def alert_status end def atom_change_description - simple_format(HTMLEntities.new.encode(change_description, :basic, :decimal)) + view_context.simple_format(HTMLEntities.new.encode(change_description, :basic, :decimal)) end def atom_public_updated_at diff --git a/app/presenters/unpublishing_presenter.rb b/app/presenters/unpublishing_presenter.rb index eb8eb0640..d12bbc24e 100644 --- a/app/presenters/unpublishing_presenter.rb +++ b/app/presenters/unpublishing_presenter.rb @@ -1,7 +1,7 @@ class UnpublishingPresenter < ContentItemPresenter attr_reader :alternative_url, :explanation - def initialize(content_item, requested_path = nil) + def initialize(*args) super @explanation = content_item["details"]["explanation"] @alternative_url = content_item["details"]["alternative_url"] diff --git a/app/services/presenter_builder.rb b/app/services/presenter_builder.rb index 89b66b992..7e3b1dd82 100644 --- a/app/services/presenter_builder.rb +++ b/app/services/presenter_builder.rb @@ -1,9 +1,10 @@ class PresenterBuilder - attr_reader :content_item, :content_item_path + attr_reader :content_item, :content_item_path, :view_context - def initialize(content_item, content_item_path) + def initialize(content_item, content_item_path, view_context) @content_item = content_item @content_item_path = content_item_path + @view_context = view_context end def presenter @@ -11,7 +12,9 @@ def presenter raise RedirectRouteReturned, content_item if redirect_route? raise GovernmentReturned if government_content_item? - presenter_name.constantize.new(content_item, content_item_path) + presenter_name.constantize.new(content_item, + content_item_path, + view_context) end private diff --git a/app/views/content_items/guide.html.erb b/app/views/content_items/guide.html.erb index db132ad49..9deac1ad2 100644 --- a/app/views/content_items/guide.html.erb +++ b/app/views/content_items/guide.html.erb @@ -8,7 +8,7 @@ <% unless @content_item.requesting_a_part? %> <% end %> diff --git a/test/presenter_test_helper.rb b/test/presenter_test_helper.rb index 413130781..e741af029 100644 --- a/test/presenter_test_helper.rb +++ b/test/presenter_test_helper.rb @@ -3,8 +3,9 @@ class PresenterTestCase < ActiveSupport::TestCase def create_presenter(presenter_class, content_item: schema_item("case_study"), - requested_path: "/test-content-item") - presenter_class.new(content_item, requested_path) + requested_path: "/test-content-item", + view_context: ApplicationController.new.view_context) + presenter_class.new(content_item, requested_path, view_context) end def schema_name diff --git a/test/presenters/content_item/contents_list_test.rb b/test/presenters/content_item/contents_list_test.rb index b432d58ba..a0633fe9d 100644 --- a/test/presenters/content_item/contents_list_test.rb +++ b/test/presenters/content_item/contents_list_test.rb @@ -3,6 +3,8 @@ class ContentItemContentsListTest < ActiveSupport::TestCase def setup @contents_list = Object.new + @contents_list.stubs(:view_context) + .returns(ApplicationController.new.view_context) @contents_list.extend(ContentItem::ContentsList) end diff --git a/test/presenters/content_item/linkable_test.rb b/test/presenters/content_item/linkable_test.rb index 0e66f6bba..1f7fd84c4 100644 --- a/test/presenters/content_item/linkable_test.rb +++ b/test/presenters/content_item/linkable_test.rb @@ -3,7 +3,7 @@ class ContentItemLinkableTest < ActiveSupport::TestCase class DummyContentItem include ContentItem::Linkable - attr_accessor :content_item, :title + attr_accessor :content_item, :title, :view_context def initialize @content_item = { @@ -11,6 +11,7 @@ def initialize "links" => {}, } @title = "A Title" + @view_context = ApplicationController.new.view_context end end diff --git a/test/presenters/content_item/parts_test.rb b/test/presenters/content_item/parts_test.rb index b8de16575..2701efc2d 100644 --- a/test/presenters/content_item/parts_test.rb +++ b/test/presenters/content_item/parts_test.rb @@ -23,6 +23,10 @@ def content_item }, } end + + def view_context + ApplicationController.new.view_context + end end module PresentingFirstPartInContentItem diff --git a/test/presenters/content_item/withdrawable_test.rb b/test/presenters/content_item/withdrawable_test.rb index 8e4ecb435..8935465c9 100644 --- a/test/presenters/content_item/withdrawable_test.rb +++ b/test/presenters/content_item/withdrawable_test.rb @@ -3,6 +3,8 @@ class ContentItemWithdrawableTest < ActiveSupport::TestCase def setup @withdrawable = Object.new + @withdrawable.stubs(:view_context) + .returns(ApplicationController.new.view_context) @withdrawable.extend(ContentItem::Withdrawable) I18n.locale = :cy end diff --git a/test/views/content_items/attachments.html.erb_test.rb b/test/views/content_items/attachments.html.erb_test.rb index 00b4f7648..20cba6f8a 100644 --- a/test/views/content_items/attachments.html.erb_test.rb +++ b/test/views/content_items/attachments.html.erb_test.rb @@ -16,6 +16,7 @@ class ContentItemsAttachmentsTest < ActionView::TestCase "title" => "Some title", "url" => "some/url" }] } }, "/publication", + ApplicationController.new.view_context, ) render(partial: "content_items/attachments",