Skip to content

Commit

Permalink
Share view context with presenters
Browse files Browse the repository at this point in the history
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
  • Loading branch information
kevindew committed May 1, 2020
1 parent 286b224 commit c57012c
Show file tree
Hide file tree
Showing 27 changed files with 79 additions and 65 deletions.
4 changes: 3 additions & 1 deletion app/controllers/content_items_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion app/presenters/coming_soon_presenter.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down
5 changes: 1 addition & 4 deletions app/presenters/content_item/contents_list.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand Down
14 changes: 9 additions & 5 deletions app/presenters/content_item/corporate_information_groups.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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

Expand Down
8 changes: 3 additions & 5 deletions app/presenters/content_item/linkable.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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
2 changes: 1 addition & 1 deletion app/presenters/content_item/national_applicability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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})"
Expand Down
6 changes: 1 addition & 5 deletions app/presenters/content_item/no_deal_notice.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand Down
22 changes: 11 additions & 11 deletions app/presenters/content_item/parts.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/presenters/content_item/title_and_context.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions app/presenters/content_item/withdrawable.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
module ContentItem
module Withdrawable
include ActionView::Helpers::TagHelper

def withdrawn?
withdrawal_notice.present?
end
Expand Down Expand Up @@ -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")
Expand Down
5 changes: 3 additions & 2 deletions app/presenters/content_item_presenter.rb
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -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"]
Expand Down
5 changes: 4 additions & 1 deletion app/presenters/document_collection_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions app/presenters/fatality_notice_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/presenters/gone_presenter.rb
Original file line number Diff line number Diff line change
@@ -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"]
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
6 changes: 4 additions & 2 deletions app/presenters/specialist_document_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/presenters/speech_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions app/presenters/travel_advice_presenter.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
class TravelAdvicePresenter < ContentItemPresenter
include ContentItem::Parts
include ActionView::Helpers::TextHelper

ATOM_CACHE_CONTROL_MAX_AGE = 300

Expand All @@ -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,
Expand Down Expand Up @@ -77,15 +76,16 @@ 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

alert_statuses.join("").html_safe
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
Expand Down
2 changes: 1 addition & 1 deletion app/presenters/unpublishing_presenter.rb
Original file line number Diff line number Diff line change
@@ -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"]
Expand Down
9 changes: 6 additions & 3 deletions app/services/presenter_builder.rb
Original file line number Diff line number Diff line change
@@ -1,17 +1,20 @@
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
raise SpecialRouteReturned if special_route?
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
Expand Down
2 changes: 1 addition & 1 deletion app/views/content_items/guide.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

<% unless @content_item.requesting_a_part? %>
<script type="application/ld+json">
<%= raw MachineReadable::GuideFaqPageSchemaPresenter.new(@content_item).structured_data.to_json %>
<%= raw MachineReadable::GuideFaqPageSchemaPresenter.new(@content_item, self).structured_data.to_json %>
</script>
<% end %>

Expand Down
Loading

0 comments on commit c57012c

Please sign in to comment.