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.

Sign up for GitHub

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

Render featured attachments directly #1709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6287f68
Move publication partial inline
Mar 25, 2020
c6ed5d8
Extract attachment rendering into partial
Mar 25, 2020
a097b81
Render featured attachments directly
Mar 25, 2020
bf24c71
Fix usages of aria-labelledby
Mar 26, 2020
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
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.attachment {
margin-bottom: govuk-spacing(6);
}
40 changes: 15 additions & 25 deletions app/presenters/consultation_presenter.rb
View file
Open in desktop
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 @@ -50,20 +51,20 @@ def final_outcome_detail
content_item["details"]["final_outcome_detail"]
end

def final_outcome_documents?
final_outcome_documents_list.any?
end

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

def public_feedback_documents?
public_feedback_documents_list.any?
def final_outcome_attachments
content_item["details"]["final_outcome_attachments"].to_a
end

def public_feedback_documents
public_feedback_documents_list.join("")
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
Expand All @@ -78,12 +79,12 @@ def held_on_another_website_url
content_item["details"]["held_on_another_website_url"]
end

def documents?
documents_list.any?
def documents
content_item["details"]["documents"].to_a.join("")
end

def documents
documents_list.join("")
def featured_attachments
content_item["details"]["featured_attachments"].to_a
end

def ways_to_respond?
Expand Down Expand Up @@ -111,7 +112,8 @@ def attachment_url
end

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

private
Expand All @@ -133,16 +135,4 @@ def display_date_and_time(date, rollback_midnight = false)
def ways_to_respond
content_item["details"]["ways_to_respond"]
end

def final_outcome_documents_list
content_item["details"]["final_outcome_documents"] || []
end

def public_feedback_documents_list
content_item["details"]["public_feedback_documents"] || []
end

def documents_list
content_item["details"]["documents"] || []
end
end
9 changes: 9 additions & 0 deletions app/presenters/content_item/attachments.rb
View file
Open in desktop
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
13 changes: 4 additions & 9 deletions app/presenters/publication_presenter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,18 @@ class PublicationPresenter < ContentItemPresenter
include ContentItem::NationalApplicability
include ContentItem::NationalStatisticsLogo
include ContentItem::Political
include ContentItem::Attachments

def details
content_item["details"]["body"]
end

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

def documents_count
documents_list.size
def featured_attachments
content_item["details"]["featured_attachments"].to_a
end

def national_statistics?
Expand All @@ -23,10 +24,4 @@ def national_statistics?
def dataset?
%(national_statistics official_statistics transparency).include? document_type
end

private

def documents_list
content_item["details"]["documents"]
end
end
20 changes: 20 additions & 0 deletions app/views/content_items/_attachments.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<% if legacy_pre_rendered_documents.present? || attachments.any? %>
<section id="<%= title.parameterize %>">
<%= render 'govuk_publishing_components/components/heading',
text: title,
mobile_top_margin: true %>

<% 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 %>
</section>
<% end %>
21 changes: 0 additions & 21 deletions app/views/content_items/_publication_inline_body.html.erb
View file
Open in desktop

This file was deleted.

36 changes: 12 additions & 24 deletions app/views/content_items/consultation.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,10 @@
<% elsif @content_item.final_outcome? %>
<%= render 'govuk_publishing_components/components/notice', title: 'This consultation has concluded' %>

<% if @content_item.final_outcome_documents? %>
<%= render 'govuk_publishing_components/components/heading', text: "Download the full outcome", mobile_top_margin: true %>
<div class="consultation-outcome">
<%= render 'govuk_publishing_components/components/govspeak',
content: @content_item.final_outcome_documents.html_safe,
direction: page_text_direction %>
</div>
<% end %>
<%= render "attachments",
title: "Download the full outcome",
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 @@ -57,14 +53,10 @@
</div>
<% end %>

<% if @content_item.public_feedback_documents? %>
<%= render 'govuk_publishing_components/components/heading', text: "Feedback received", mobile_top_margin: true %>
<div class="consultation-feedback-documents">
<%= render 'govuk_publishing_components/components/govspeak',
content: @content_item.public_feedback_documents.html_safe,
direction: page_text_direction %>
</div >
<% end %>
<%= render "attachments",
title: "Feedback received",
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 @@ -115,14 +107,10 @@
<%= render 'govuk_publishing_components/components/heading', text: "Consultation description", mobile_top_margin: true %>
<%= render 'govuk_publishing_components/components/govspeak', @content_item.govspeak_body %>

<% if @content_item.documents? %>
<%= render 'govuk_publishing_components/components/heading', text: "Documents", mobile_top_margin: true %>
<div class="consultation-documents">
<%= render 'govuk_publishing_components/components/govspeak',
content: @content_item.documents.html_safe,
direction: page_text_direction %>
</div>
<% end %>
<%= render "attachments",
title: "Documents",
legacy_pre_rendered_documents: @content_item.documents,
attachments: @content_item.featured_attachments %>
</div>

<% if @content_item.ways_to_respond? %>
Expand Down
16 changes: 15 additions & 1 deletion app/views/content_items/publication.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,21 @@
<div class="govuk-grid-column-two-thirds responsive-bottom-margin">
<%= render 'components/important-metadata', items: @content_item.important_metadata %>
<div class="responsive-bottom-margin">
<%= render 'publication_inline_body' %>
<%= render "attachments",
title: t("publication.documents", count: 5), # This should always be pluralised.
legacy_pre_rendered_documents: @content_item.documents,
attachments: @content_item.featured_attachments %>


<section id="details">
<%= render 'govuk_publishing_components/components/heading',
text: t("publication.details"),
mobile_top_margin: true %>

<%= render 'govuk_publishing_components/components/govspeak',
content: @content_item.details.html_safe,
direction: page_text_direction %>
</section>
</div>

<%= render 'components/published-dates', {
Expand Down
35 changes: 29 additions & 6 deletions test/integration/consultation_test.rb
View file
Open in desktop
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")

within ".consultation-documents" do
assert page.has_text?("Documents")
within "#documents" 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 "#documents" 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,19 +95,34 @@ 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")

within ".consultation-outcome" do
assert page.has_text?("Download the full outcome")
within "#download-the-full-outcome" 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 "#download-the-full-outcome" 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 ".consultation-feedback-documents" do
within "#feedback-received" 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 "#feedback-received" 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
Expand Down
12 changes: 9 additions & 3 deletions test/integration/publication_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class PublicationTest < ActionDispatch::IntegrationTest
assert_has_component_title(@content_item["title"])
assert page.has_text?(@content_item["description"])

within '[aria-labelledby="details-title"]' do
within "#details" do
assert page.has_text?("Installation name: Leeming Biogas Facility")
end
end
Expand All @@ -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
within "#documents" do
assert page.has_text?("Permit: Veolia ES (UK) Limited")
end

setup_and_visit_content_item("publication-with-featured-attachments")
within "#documents" 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
3 changes: 0 additions & 3 deletions test/presenters/consultation_presenter_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,23 +66,20 @@ def schema_name
presented = presented_item("closed_consultation")
schema = schema_item("closed_consultation")

assert presented.documents?
assert_equal schema["details"]["documents"].join(""), presented.documents
end

test "presents final outcome documents" do
presented = presented_item("consultation_outcome")
schema = schema_item("consultation_outcome")

assert presented.final_outcome_documents?
assert_equal schema["details"]["final_outcome_documents"].join(""), presented.final_outcome_documents
end

test "presents public feedback documents" do
presented = presented_item("consultation_outcome_with_feedback")
schema = schema_item("consultation_outcome_with_feedback")

assert presented.public_feedback_documents?
assert_equal schema["details"]["public_feedback_documents"].join(""), presented.public_feedback_documents
end

Expand Down
4 changes: 0 additions & 4 deletions test/presenters/publication_presenter_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@ def schema_name
assert_equal "3 May 2016", presented_item.published
end

test "counts documents attached to publication" do
assert_equal 2, presented_item.documents_count
end

test "presents the title of the publishing government" do
assert_equal schema_item["links"]["government"][0]["title"], presented_item.publishing_government
end
Expand Down
Loading