Skip to content

Commit

Permalink
Render featured attachments directly
Browse files Browse the repository at this point in the history
This extends the attachments partial to render attachments directly
using the new 'attachments' metadata in the payload (see RFC 116) [1].
Since both kinds of data may be present in the payload, I've added a
view test to ensure we don't change the way attachments are rendered
until the publishing app stops sending pre-rendered attachments.

Publications have one set of featured attachments, while consultations
can have up to three. For the tests, I decided to test both the legacy
and the new behaviours in the same test block, noting that there's no
other way to group these related tests together.

The new 'attachment' CSS class replicates the 30px bottom margin that
legacy pre-rendered attachments have in Govspeak [2].

[1] https://github.com/alphagov/govuk-rfcs/blob/master/rfc-116-store-attachment-data-in-content-items.md

[2] https://github.com/alphagov/govuk_publishing_components/blob/6ee40d5f1c2b3d81d98db0923ab96cf454d34ff0/app/assets/stylesheets/govuk_publishing_components/components/govspeak/_attachment.scss#L22
  • Loading branch information
Ben Thorner committed Mar 26, 2020
1 parent c6ed5d8 commit 999f56c
Show file tree
Hide file tree
Showing 11 changed files with 126 additions and 13 deletions.
1 change: 1 addition & 0 deletions app/assets/stylesheets/application.scss
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ $govuk-use-legacy-palette: false;
@import 'helpers/add-title-margin';
@import "helpers/content-bottom-margin";
@import "helpers/publisher-metadata-with-logo";
@import "helpers/attachments";

// Components from this application
@import 'components/*';
Expand Down
3 changes: 3 additions & 0 deletions app/assets/stylesheets/helpers/_attachments.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.attachment {
margin-bottom: govuk-spacing(6);
}
15 changes: 14 additions & 1 deletion app/presenters/consultation_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ class ConsultationPresenter < ContentItemPresenter
include ContentItem::Political
include ContentItem::Shareable
include ContentItem::TitleAndContext
include ContentItem::Attachments

def opening_date_time
content_item["details"]["opening_date"]
Expand Down Expand Up @@ -54,10 +55,18 @@ def final_outcome_documents
content_item["details"]["final_outcome_documents"].to_a.join("")
end

def final_outcome_attachments
content_item["details"]["final_outcome_attachments"].to_a
end

def public_feedback_documents
content_item["details"]["public_feedback_documents"].to_a.join("")
end

def public_feedback_attachments
content_item["details"]["public_feedback_attachments"].to_a
end

def public_feedback_detail
content_item["details"]["public_feedback_detail"]
end
Expand All @@ -74,6 +83,10 @@ def documents
content_item["details"]["documents"].to_a.join("")
end

def featured_attachments
content_item["details"]["featured_attachments"].to_a
end

def ways_to_respond?
open? && ways_to_respond && (respond_online_url || email || postal_address)
end
Expand All @@ -100,7 +113,7 @@ def attachment_url

def add_margin?
final_outcome? || public_feedback_detail ||
public_feedback_documents.present?
public_feedback_documents.present? || public_feedback_attachments.any?
end

private
Expand Down
9 changes: 9 additions & 0 deletions app/presenters/content_item/attachments.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module ContentItem
module Attachments
def attachment_details(attachment_id)
content_item["details"]["attachments"].find do |attachment|
attachment["id"] == attachment_id
end
end
end
end
5 changes: 5 additions & 0 deletions app/presenters/publication_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ class PublicationPresenter < ContentItemPresenter
include ContentItem::NationalApplicability
include ContentItem::NationalStatisticsLogo
include ContentItem::Political
include ContentItem::Attachments

def details
content_item["details"]["body"]
Expand All @@ -12,6 +13,10 @@ def documents
content_item["details"]["documents"].to_a.join("")
end

def featured_attachments
content_item["details"]["featured_attachments"].to_a
end

def national_statistics?
document_type == "national_statistics"
end
Expand Down
17 changes: 13 additions & 4 deletions app/views/content_items/_attachments.html.erb
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
<% attachment_title_id = "#{title.parameterize}-title" %>

<% if legacy_pre_rendered_documents.present? %>
<% if legacy_pre_rendered_documents.present? || attachments.any? %>
<%= render 'govuk_publishing_components/components/heading',
id: attachment_title_id,
text: title,
mobile_top_margin: true %>

<div aria-labelledby="<%= attachment_title_id %>">
<%= render 'govuk_publishing_components/components/govspeak',
content: legacy_pre_rendered_documents.html_safe,
direction: page_text_direction %>
<% if legacy_pre_rendered_documents.present? %>
<%= render 'govuk_publishing_components/components/govspeak',
content: legacy_pre_rendered_documents.html_safe,
direction: page_text_direction %>
<% else %>
<% attachments.each do |attachment_id| %>
<div class="attachment">
<%= render 'govuk_publishing_components/components/attachment',
attachment: @content_item.attachment_details(attachment_id) %>
</div>
<% end %>
<% end %>
</div>
<% end %>
9 changes: 6 additions & 3 deletions app/views/content_items/consultation.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@

<%= render "attachments",
title: "Download the full outcome",
legacy_pre_rendered_documents: @content_item.final_outcome_documents %>
legacy_pre_rendered_documents: @content_item.final_outcome_documents,
attachments: @content_item.final_outcome_attachments %>

<%= render 'govuk_publishing_components/components/heading', text: "Detail of outcome", mobile_top_margin: true %>
<div class="consultation-outcome-detail">
Expand All @@ -54,7 +55,8 @@

<%= render "attachments",
title: "Feedback received",
legacy_pre_rendered_documents: @content_item.public_feedback_documents %>
legacy_pre_rendered_documents: @content_item.public_feedback_documents,
attachments: @content_item.public_feedback_attachments %>

<% if @content_item.public_feedback_detail %>
<%= render 'govuk_publishing_components/components/heading', text: "Detail of feedback received", mobile_top_margin: true %>
Expand Down Expand Up @@ -107,7 +109,8 @@

<%= render "attachments",
title: "Documents",
legacy_pre_rendered_documents: @content_item.documents %>
legacy_pre_rendered_documents: @content_item.documents,
attachments: @content_item.featured_attachments %>
</div>

<% if @content_item.ways_to_respond? %>
Expand Down
3 changes: 2 additions & 1 deletion app/views/content_items/publication.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@
<div class="responsive-bottom-margin">
<%= render "attachments",
title: t("publication.documents", count: 5), # This should always be pluralised.
legacy_pre_rendered_documents: @content_item.documents %>
legacy_pre_rendered_documents: @content_item.documents,
attachments: @content_item.featured_attachments %>

<%= render 'govuk_publishing_components/components/heading',
text: t("publication.details"),
Expand Down
29 changes: 26 additions & 3 deletions test/integration/consultation_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,20 @@ class ConsultationTest < ActionDispatch::IntegrationTest
end
end

test "consultation documents render" do
test "renders document attachments (as-is and directly)" do
setup_and_visit_content_item("closed_consultation")

assert page.has_text?("Documents")
within '[aria-labelledby="documents-title"]' do
assert page.has_text?("Museums Review Terms of Reference")
end

setup_and_visit_content_item("consultation_outcome_with_featured_attachments")

assert page.has_text?("Documents")
within '[aria-labelledby="documents-title"]' do
assert page.has_text?("Setting the grade standards of new GCSEs in England – part 2")
end
end

test "link to external consultations" do
Expand Down Expand Up @@ -87,21 +95,36 @@ class ConsultationTest < ActionDispatch::IntegrationTest
end
end

test "consultation outcome documents render" do
test "renders consultation outcome attachments (as-is and directly)" do
setup_and_visit_content_item("consultation_outcome")

assert page.has_text?("Download the full outcome")
within '[aria-labelledby="download-the-full-outcome-title"]' do
assert page.has_text?("Employee Share Schemes: NIC elections - consulation response")
end

setup_and_visit_content_item("consultation_outcome_with_featured_attachments")

assert page.has_text?("Download the full outcome")
within '[aria-labelledby="download-the-full-outcome-title"]' do
assert page.has_text?("Equalities impact assessment: setting the grade standards of new GCSEs in England – part 2")
end
end

test "public feedback documents render" do
test "renders public feedback attachments (as-is and directly)" do
setup_and_visit_content_item("consultation_outcome_with_feedback")

assert page.has_text?("Feedback received")
within '[aria-labelledby="feedback-received-title"]' do
assert page.has_text?("Analysis of responses to our consultation on setting the grade standards of new GCSEs in England – part 2")
end

setup_and_visit_content_item("consultation_outcome_with_featured_attachments")

assert page.has_text?("Feedback received")
within '[aria-labelledby="feedback-received-title"]' do
assert page.has_text?("Analysis of responses to our consultation on setting the grade standards of new GCSEs in England – part 2")
end
end

test "consultation that only applies to a set of nations" do
Expand Down
8 changes: 7 additions & 1 deletion test/integration/publication_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,17 @@ class PublicationTest < ActionDispatch::IntegrationTest
assert_footer_has_published_dates("Published 3 May 2016")
end

test "renders a govspeak block for attachments" do
test "renders document attachments (as-is and directly)" do
setup_and_visit_content_item("publication")
within '[aria-labelledby="documents-title"]' do
assert page.has_text?("Permit: Veolia ES (UK) Limited")
end

setup_and_visit_content_item("publication-with-featured-attachments")
within '[aria-labelledby="documents-title"]' do
assert page.has_text?("Number of ex-regular service personnel now part of FR20")
assert page.has_css?(".gem-c-attachment")
end
end

test "withdrawn publication" do
Expand Down
40 changes: 40 additions & 0 deletions test/views/content_items/attachments.html.erb_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
require "test_helper"

class ContentItemsAttachmentsTest < ActionView::TestCase
test "it shows pre-rendered attachments by default" do
render(partial: "content_items/attachments",
locals: { title: "Documents",
legacy_pre_rendered_documents: "some html" })

assert_includes rendered, "gem-c-govspeak"
assert_includes rendered, "some html"
end

test "can render attachments using their metadata" do
@content_item = PublicationPresenter.new(
"details" => {
"attachments" => [{ "id" => "attachment_id",
"title" => "Some title",
"url" => "some/url" }]
}
)

render(partial: "content_items/attachments",
locals: { title: "Documents",
legacy_pre_rendered_documents: "",
attachments: %w(attachment_id) })

assert_includes rendered, "gem-c-attachment"
assert_includes rendered, "Some title"
end

test "it prioritises pre-rendered attachments" do
render(partial: "content_items/attachments",
locals: { title: "Documents",
legacy_pre_rendered_documents: "some html",
attachments: %w(attachment_id) })

assert_includes rendered, "gem-c-govspeak"
assert_includes rendered, "some html"
end
end

0 comments on commit 999f56c

Please sign in to comment.