From 6287f6847acf2fb6ef9024fd3fce5042186bedb8 Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Wed, 25 Mar 2020 12:06:32 +0000
Subject: [PATCH 1/4] Move publication partial inline

Previously the main content of a publication was extracted into a
single-use partial, which made the view harder to visualise in code.
This inlines the partial into the main view.
---
 .../_publication_inline_body.html.erb         | 21 ------------------
 app/views/content_items/publication.html.erb  | 22 ++++++++++++++++++-
 2 files changed, 21 insertions(+), 22 deletions(-)
 delete mode 100644 app/views/content_items/_publication_inline_body.html.erb

diff --git a/app/views/content_items/_publication_inline_body.html.erb b/app/views/content_items/_publication_inline_body.html.erb
deleted file mode 100644
index 864ae4b7a..000000000
--- a/app/views/content_items/_publication_inline_body.html.erb
+++ /dev/null
@@ -1,21 +0,0 @@
-<%= render 'govuk_publishing_components/components/heading',
-  text: t("publication.documents", count: 5), # This should always be pluralised.
-  id: "documents-title",
-  mobile_top_margin: true %>
-
-<div aria-labelledby="documents-title">
-  <%= render 'govuk_publishing_components/components/govspeak',
-             content: @content_item.documents.html_safe,
-             direction: page_text_direction %>
-</div>
-
-<%= render 'govuk_publishing_components/components/heading',
-  text: t("publication.details"),
-  id: "details-title",
-  mobile_top_margin: true %>
-
-<div aria-labelledby="details-title">
-  <%= render 'govuk_publishing_components/components/govspeak',
-             content: @content_item.details.html_safe,
-             direction: page_text_direction %>
-</div>
diff --git a/app/views/content_items/publication.html.erb b/app/views/content_items/publication.html.erb
index 2b7d4638a..dba4564ab 100644
--- a/app/views/content_items/publication.html.erb
+++ b/app/views/content_items/publication.html.erb
@@ -29,7 +29,27 @@
   <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 'govuk_publishing_components/components/heading',
+        text: t("publication.documents", count: 5), # This should always be pluralised.
+        id: "documents-title",
+        mobile_top_margin: true %>
+
+      <div aria-labelledby="documents-title">
+        <%= render 'govuk_publishing_components/components/govspeak',
+                   content: @content_item.documents.html_safe,
+                   direction: page_text_direction %>
+      </div>
+
+      <%= render 'govuk_publishing_components/components/heading',
+        text: t("publication.details"),
+        id: "details-title",
+        mobile_top_margin: true %>
+
+      <div aria-labelledby="details-title">
+        <%= render 'govuk_publishing_components/components/govspeak',
+                   content: @content_item.details.html_safe,
+                   direction: page_text_direction %>
+      </div>
     </div>
 
     <%= render 'components/published-dates', {

From c6ed5d876786adaf9a9b5b4547518dd55ec83742 Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Wed, 25 Mar 2020 13:19:11 +0000
Subject: [PATCH 2/4] Extract attachment rendering into partial

Previously publication and consultation attachments were rendered
inline. This extracts the rendering into a new 'attachments' partial,
which has the nice side effect of improving the aria markup for
consultations, as well as making the testing approach consistent.

This also replaces the 'documents' question methods in each presenter
with explicit calls in the views. Since the view needs to be aware of
the type of data, it makes sense to ask questions like 'present?' and
'any?' in the view directly, instead of bloating the presenters.
---
 app/presenters/consultation_presenter.rb      | 33 +++----------------
 app/presenters/publication_presenter.rb       | 12 +------
 app/views/content_items/_attachments.html.erb | 14 ++++++++
 app/views/content_items/consultation.html.erb | 33 +++++--------------
 app/views/content_items/publication.html.erb  | 13 ++------
 test/integration/consultation_test.rb         |  6 ++--
 .../presenters/consultation_presenter_test.rb |  3 --
 test/presenters/publication_presenter_test.rb |  4 ---
 8 files changed, 35 insertions(+), 83 deletions(-)
 create mode 100644 app/views/content_items/_attachments.html.erb

diff --git a/app/presenters/consultation_presenter.rb b/app/presenters/consultation_presenter.rb
index 26393817a..319eb5fad 100644
--- a/app/presenters/consultation_presenter.rb
+++ b/app/presenters/consultation_presenter.rb
@@ -50,20 +50,12 @@ 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("")
-  end
-
-  def public_feedback_documents?
-    public_feedback_documents_list.any?
+    content_item["details"]["final_outcome_documents"].to_a.join("")
   end
 
   def public_feedback_documents
-    public_feedback_documents_list.join("")
+    content_item["details"]["public_feedback_documents"].to_a.join("")
   end
 
   def public_feedback_detail
@@ -78,12 +70,8 @@ def held_on_another_website_url
     content_item["details"]["held_on_another_website_url"]
   end
 
-  def documents?
-    documents_list.any?
-  end
-
   def documents
-    documents_list.join("")
+    content_item["details"]["documents"].to_a.join("")
   end
 
   def ways_to_respond?
@@ -111,7 +99,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?
   end
 
 private
@@ -133,16 +122,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
diff --git a/app/presenters/publication_presenter.rb b/app/presenters/publication_presenter.rb
index 97032933c..08d974c29 100644
--- a/app/presenters/publication_presenter.rb
+++ b/app/presenters/publication_presenter.rb
@@ -9,11 +9,7 @@ def details
   end
 
   def documents
-    documents_list.join("")
-  end
-
-  def documents_count
-    documents_list.size
+    content_item["details"]["documents"].to_a.join("")
   end
 
   def national_statistics?
@@ -23,10 +19,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
diff --git a/app/views/content_items/_attachments.html.erb b/app/views/content_items/_attachments.html.erb
new file mode 100644
index 000000000..9388b1a59
--- /dev/null
+++ b/app/views/content_items/_attachments.html.erb
@@ -0,0 +1,14 @@
+<% attachment_title_id = "#{title.parameterize}-title" %>
+
+<% if legacy_pre_rendered_documents.present? %>
+  <%= 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 %>
+  </div>
+<% end %>
diff --git a/app/views/content_items/consultation.html.erb b/app/views/content_items/consultation.html.erb
index 4db3e95f3..e0e82db46 100644
--- a/app/views/content_items/consultation.html.erb
+++ b/app/views/content_items/consultation.html.erb
@@ -40,14 +40,9 @@
     <% 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 %>
 
       <%= render 'govuk_publishing_components/components/heading', text: "Detail of outcome", mobile_top_margin: true %>
       <div class="consultation-outcome-detail">
@@ -57,14 +52,9 @@
       </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 %>
 
     <% if @content_item.public_feedback_detail %>
       <%= render 'govuk_publishing_components/components/heading', text: "Detail of feedback received", mobile_top_margin: true %>
@@ -115,14 +105,9 @@
       <%= 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 %>
     </div>
 
     <% if @content_item.ways_to_respond? %>
diff --git a/app/views/content_items/publication.html.erb b/app/views/content_items/publication.html.erb
index dba4564ab..667137772 100644
--- a/app/views/content_items/publication.html.erb
+++ b/app/views/content_items/publication.html.erb
@@ -29,16 +29,9 @@
   <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 'govuk_publishing_components/components/heading',
-        text: t("publication.documents", count: 5), # This should always be pluralised.
-        id: "documents-title",
-        mobile_top_margin: true %>
-
-      <div aria-labelledby="documents-title">
-        <%= render 'govuk_publishing_components/components/govspeak',
-                   content: @content_item.documents.html_safe,
-                   direction: page_text_direction %>
-      </div>
+      <%= render "attachments",
+          title: t("publication.documents", count: 5), # This should always be pluralised.
+          legacy_pre_rendered_documents: @content_item.documents %>
 
       <%= render 'govuk_publishing_components/components/heading',
         text: t("publication.details"),
diff --git a/test/integration/consultation_test.rb b/test/integration/consultation_test.rb
index 9e9f8b95b..83974fc82 100644
--- a/test/integration/consultation_test.rb
+++ b/test/integration/consultation_test.rb
@@ -25,7 +25,7 @@ class ConsultationTest < ActionDispatch::IntegrationTest
   test "consultation documents render" do
     setup_and_visit_content_item("closed_consultation")
 
-    within ".consultation-documents" do
+    within '[aria-labelledby="documents-title"]' do
       assert page.has_text?("Museums Review Terms of Reference")
     end
   end
@@ -90,7 +90,7 @@ class ConsultationTest < ActionDispatch::IntegrationTest
   test "consultation outcome documents render" do
     setup_and_visit_content_item("consultation_outcome")
 
-    within ".consultation-outcome" do
+    within '[aria-labelledby="download-the-full-outcome-title"]' do
       assert page.has_text?("Employee Share Schemes: NIC elections - consulation response")
     end
   end
@@ -99,7 +99,7 @@ class ConsultationTest < ActionDispatch::IntegrationTest
     setup_and_visit_content_item("consultation_outcome_with_feedback")
 
     assert page.has_text?("Feedback received")
-    within ".consultation-feedback-documents" do
+    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
diff --git a/test/presenters/consultation_presenter_test.rb b/test/presenters/consultation_presenter_test.rb
index 22d8d8926..995241b7d 100644
--- a/test/presenters/consultation_presenter_test.rb
+++ b/test/presenters/consultation_presenter_test.rb
@@ -66,7 +66,6 @@ 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
 
@@ -74,7 +73,6 @@ def schema_name
       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
 
@@ -82,7 +80,6 @@ def schema_name
       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
 
diff --git a/test/presenters/publication_presenter_test.rb b/test/presenters/publication_presenter_test.rb
index f9b076df8..12c9446fa 100644
--- a/test/presenters/publication_presenter_test.rb
+++ b/test/presenters/publication_presenter_test.rb
@@ -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

From a097b81108d4cf666ea768aab62ac7316a071408 Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Wed, 25 Mar 2020 14:02:51 +0000
Subject: [PATCH 3/4] Render featured attachments directly

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
---
 app/assets/stylesheets/application.scss       |  1 +
 .../stylesheets/helpers/_attachments.scss     |  3 ++
 app/presenters/consultation_presenter.rb      | 15 ++++++-
 app/presenters/content_item/attachments.rb    |  9 +++++
 app/presenters/publication_presenter.rb       |  5 +++
 app/views/content_items/_attachments.html.erb | 17 ++++++--
 app/views/content_items/consultation.html.erb |  9 +++--
 app/views/content_items/publication.html.erb  |  3 +-
 test/integration/consultation_test.rb         | 29 ++++++++++++--
 test/integration/publication_test.rb          |  8 +++-
 .../attachments.html.erb_test.rb              | 40 +++++++++++++++++++
 11 files changed, 126 insertions(+), 13 deletions(-)
 create mode 100644 app/assets/stylesheets/helpers/_attachments.scss
 create mode 100644 app/presenters/content_item/attachments.rb
 create mode 100644 test/views/content_items/attachments.html.erb_test.rb

diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss
index 759af8678..34358ea9a 100644
--- a/app/assets/stylesheets/application.scss
+++ b/app/assets/stylesheets/application.scss
@@ -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/*';
diff --git a/app/assets/stylesheets/helpers/_attachments.scss b/app/assets/stylesheets/helpers/_attachments.scss
new file mode 100644
index 000000000..7aed5f99c
--- /dev/null
+++ b/app/assets/stylesheets/helpers/_attachments.scss
@@ -0,0 +1,3 @@
+.attachment {
+  margin-bottom: govuk-spacing(6);
+}
diff --git a/app/presenters/consultation_presenter.rb b/app/presenters/consultation_presenter.rb
index 319eb5fad..55e3a49c4 100644
--- a/app/presenters/consultation_presenter.rb
+++ b/app/presenters/consultation_presenter.rb
@@ -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"]
@@ -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
@@ -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
@@ -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
diff --git a/app/presenters/content_item/attachments.rb b/app/presenters/content_item/attachments.rb
new file mode 100644
index 000000000..628e8d625
--- /dev/null
+++ b/app/presenters/content_item/attachments.rb
@@ -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
diff --git a/app/presenters/publication_presenter.rb b/app/presenters/publication_presenter.rb
index 08d974c29..c2553fb29 100644
--- a/app/presenters/publication_presenter.rb
+++ b/app/presenters/publication_presenter.rb
@@ -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"]
@@ -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
diff --git a/app/views/content_items/_attachments.html.erb b/app/views/content_items/_attachments.html.erb
index 9388b1a59..57c2e12a2 100644
--- a/app/views/content_items/_attachments.html.erb
+++ b/app/views/content_items/_attachments.html.erb
@@ -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 %>
diff --git a/app/views/content_items/consultation.html.erb b/app/views/content_items/consultation.html.erb
index e0e82db46..803f7cf69 100644
--- a/app/views/content_items/consultation.html.erb
+++ b/app/views/content_items/consultation.html.erb
@@ -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">
@@ -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 %>
@@ -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? %>
diff --git a/app/views/content_items/publication.html.erb b/app/views/content_items/publication.html.erb
index 667137772..4dc0d1932 100644
--- a/app/views/content_items/publication.html.erb
+++ b/app/views/content_items/publication.html.erb
@@ -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"),
diff --git a/test/integration/consultation_test.rb b/test/integration/consultation_test.rb
index 83974fc82..e0a81a0d2 100644
--- a/test/integration/consultation_test.rb
+++ b/test/integration/consultation_test.rb
@@ -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
@@ -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
diff --git a/test/integration/publication_test.rb b/test/integration/publication_test.rb
index 53c8060f8..63d497b66 100644
--- a/test/integration/publication_test.rb
+++ b/test/integration/publication_test.rb
@@ -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
diff --git a/test/views/content_items/attachments.html.erb_test.rb b/test/views/content_items/attachments.html.erb_test.rb
new file mode 100644
index 000000000..6df55e025
--- /dev/null
+++ b/test/views/content_items/attachments.html.erb_test.rb
@@ -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

From bf24c71fdabd11cfe6e489882638f2eac75bc0cf Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Thu, 26 Mar 2020 14:10:17 +0000
Subject: [PATCH 4/4] Fix usages of aria-labelledby

Previously the div of attachments used 'aria-labelledby' to
refer to an outer heading element, which is incorrect: the
label should refer to something inside the 'div'. The 'div'
should also set the appropriate 'role="section"' attribute [1],
or we can just replace the 'div' with a 'section' element [2].

This corrects the structure of the attachments partial so
that the sections properly self-describe. Each section now has
an ID, so they continue to be easy to grab hold of in tests.

[1] https://www.w3.org/TR/wai-aria/#region

[2] https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/Region_role
---
 app/views/content_items/_attachments.html.erb | 13 +++++--------
 app/views/content_items/publication.html.erb  | 12 ++++++------
 test/integration/consultation_test.rb         | 12 ++++++------
 test/integration/publication_test.rb          |  6 +++---
 4 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/app/views/content_items/_attachments.html.erb b/app/views/content_items/_attachments.html.erb
index 57c2e12a2..81c191f83 100644
--- a/app/views/content_items/_attachments.html.erb
+++ b/app/views/content_items/_attachments.html.erb
@@ -1,12 +1,9 @@
-<% attachment_title_id = "#{title.parameterize}-title" %>
-
 <% if legacy_pre_rendered_documents.present? || attachments.any? %>
-  <%= render 'govuk_publishing_components/components/heading',
-      id: attachment_title_id,
-      text: title,
-      mobile_top_margin: true %>
+  <section id="<%= title.parameterize %>">
+    <%= render 'govuk_publishing_components/components/heading',
+        text: title,
+        mobile_top_margin: true %>
 
-  <div aria-labelledby="<%= attachment_title_id %>">
     <% if legacy_pre_rendered_documents.present? %>
       <%= render 'govuk_publishing_components/components/govspeak',
                  content: legacy_pre_rendered_documents.html_safe,
@@ -19,5 +16,5 @@
         </div>
       <% end %>
     <% end %>
-  </div>
+  </section>
 <% end %>
diff --git a/app/views/content_items/publication.html.erb b/app/views/content_items/publication.html.erb
index 4dc0d1932..100bed212 100644
--- a/app/views/content_items/publication.html.erb
+++ b/app/views/content_items/publication.html.erb
@@ -34,16 +34,16 @@
           legacy_pre_rendered_documents: @content_item.documents,
           attachments: @content_item.featured_attachments %>
 
-      <%= render 'govuk_publishing_components/components/heading',
-        text: t("publication.details"),
-        id: "details-title",
-        mobile_top_margin: true %>
 
-      <div aria-labelledby="details-title">
+      <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 %>
-      </div>
+      </section>
     </div>
 
     <%= render 'components/published-dates', {
diff --git a/test/integration/consultation_test.rb b/test/integration/consultation_test.rb
index e0a81a0d2..5821ee55b 100644
--- a/test/integration/consultation_test.rb
+++ b/test/integration/consultation_test.rb
@@ -26,14 +26,14 @@ class ConsultationTest < ActionDispatch::IntegrationTest
     setup_and_visit_content_item("closed_consultation")
 
     assert page.has_text?("Documents")
-    within '[aria-labelledby="documents-title"]' do
+    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 '[aria-labelledby="documents-title"]' do
+    within "#documents" do
       assert page.has_text?("Setting the grade standards of new GCSEs in England – part 2")
     end
   end
@@ -99,14 +99,14 @@ class ConsultationTest < ActionDispatch::IntegrationTest
     setup_and_visit_content_item("consultation_outcome")
 
     assert page.has_text?("Download the full outcome")
-    within '[aria-labelledby="download-the-full-outcome-title"]' do
+    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")
 
     assert page.has_text?("Download the full outcome")
-    within '[aria-labelledby="download-the-full-outcome-title"]' do
+    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
@@ -115,14 +115,14 @@ class ConsultationTest < ActionDispatch::IntegrationTest
     setup_and_visit_content_item("consultation_outcome_with_feedback")
 
     assert page.has_text?("Feedback received")
-    within '[aria-labelledby="feedback-received-title"]' 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 '[aria-labelledby="feedback-received-title"]' 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
   end
diff --git a/test/integration/publication_test.rb b/test/integration/publication_test.rb
index 63d497b66..5a17dd8c3 100644
--- a/test/integration/publication_test.rb
+++ b/test/integration/publication_test.rb
@@ -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
@@ -36,12 +36,12 @@ class PublicationTest < ActionDispatch::IntegrationTest
 
   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 '[aria-labelledby="documents-title"]' do
+    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