Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Render featured attachments directly #1709

Merged
merged 4 commits into from
Mar 27, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 %>">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this have the effect of reading the heading out twice (once on the heading itself, and once immediately afterwards)? (NB, the examples I've seen on MDN tend to show the heading inside the div):

<div aria-labelledby="<%= attachment_title_id %>">

  <%= render 'govuk_publishing_components/components/heading',
      id: attachment_title_id,
      text: title,
      mobile_top_margin: true %>

I'm sure Alex has already looked at this, I'm just not immediately clear on how this is an improvement 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is a case of me assuming we were doing a good thing in the first place 🤦‍♂. This is replicating a behaviour that previously applied only to publication documents, and applying it to all lists of attachments (with a title).

Apparently it was introduced here, but with no explanation. I've added another commit so it conforms to the spec. Removing it makes the tests harder, as there's nothing to 'grab hold of', so I've gone with fixing it, instead.

How does it look now?

<%= 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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each of these test blocks seem like they should be split into two test blocks - one for as-is, and one for directly. I appreciate there's no other way of grouping them together, save for a comment in between every two blocks, but it would make it easier to delete the as-is blocks when we come to deprecate that behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree they could be split up, I think that would decrease the readability of the rest of the test file, as there is indeed no way of grouping them, and they are effectively testing the same thing. One of the commit messages says this, somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I would find it that much harder to delete the lines within the test, when we (finally) disable the legacy functionality.


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