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

Update publication pages #2302

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
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
29 changes: 28 additions & 1 deletion app/presenters/content_item/withdrawable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,15 @@ def withdrawn?
end

def page_title
withdrawn? ? "[Withdrawn] #{title}" : title
if withdrawn? && context_title? && publication_overview?
"[Withdrawn] #{context_title}: #{title}"
elsif withdrawn?
"[Withdrawn] #{title}"
elsif context_title? && publication_overview?
"#{context_title}: #{title}"
else
title
end
end

def withdrawal_notice_component
Expand All @@ -21,6 +29,19 @@ def withdrawal_notice_component

private

def context_title?
context_title.present?
end

def publication_overview?
publication_overview.present?
end

def publication_overview
overview = (I18n.exists?("content_item.#{schema_name}") == "publication" || "statistical_data_set")
Copy link
Contributor

@maxgds maxgds Dec 1, 2021

Choose a reason for hiding this comment

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

Could you expand a little on what's going on here to help me understand? I have two questions:

  1. Why is statistical_data_set relevant? It's not a sub type of publication so we never expect to apply this overview/context title behaviour to it.
  2. Why are you looking up whether there is a translation for the schema_name ? Don't we want to always apply overview if it's a publication?

It feels like this could be as simple as line 32 - 34 being

    def context_title?
      content_item.#{schema_name} == "publication"
    end

and lines 36-44 being removed, but I expect I'm missing something. Or multiple somethings.

overview.presence
end

def withdrawal_notice
content_item["withdrawn_notice"]
end
Expand All @@ -29,6 +50,12 @@ def withdrawal_notice_title
"This #{withdrawal_notice_context.downcase} was withdrawn on #{withdrawal_notice_time}".html_safe
end

def context_title
if I18n.exists?("content_item.schema_name.#{document_type}", count: 1, locale: :en)
I18n.t("content_item.schema_name.#{document_type}", count: 1, locale: :en)
end
end

def withdrawal_notice_context
I18n.t("content_item.schema_name.#{schema_name}", count: 1, locale: :en)
end
Expand Down
25 changes: 3 additions & 22 deletions app/views/content_items/_context_and_title.html.erb
Original file line number Diff line number Diff line change
@@ -1,26 +1,7 @@
<%
context_string = t("content_item.schema_name.#{@content_item.document_type}", count: 1);
context_inside = false;
%>

<% @content_item&.featured_attachments.each do |fa| %>
<% return if !@content_item.attachment_details(fa).present? %>
<% if @content_item.attachment_details(fa)['title'] == @content_item.title %>
<% content_for :title do %>
<%= t("content_item.schema_name.#{@content_item.document_type}.overview", count: 1) %>: <%= @content_item.title %>
<% end %>
<%
context_string = t("content_item.schema_name.#{@content_item.document_type}.overview", count: 1) << ":"
context_inside = true
%>
<% break %>
<% end %>
<% end %>

<%= render 'govuk_publishing_components/components/title',
chris-gds marked this conversation as resolved.
Show resolved Hide resolved
context: context_string,
context_locale: t_locale_fallback("content_item.schema_name.#{@content_item.document_type}", count: 1),
context_inside: context_inside,
context: t("content_item.schema_name.#{@content_item.document_type}", count: 1) << ":",
context_locale: t_locale_fallback("content_item.schema_name.#{@content_item.document_type}", count: 1) ,
context_inside: true,
title: @content_item.title,
average_title_length: "long"
%>
2 changes: 1 addition & 1 deletion test/integration/fatality_notice_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class FatalityNoticeTest < ActionDispatch::IntegrationTest
setup_and_visit_content_item("withdrawn_fatality_notice")

assert page.has_title?(
"[Withdrawn] Sir George Pomeroy Colley killed in Boer War - Fatality notice - GOV.UK",
"[Withdrawn] Fatality notice: Sir George Pomeroy Colley killed in Boer War - Fatality notice - GOV.UK",
)

within ".gem-c-notice" do
Expand Down
11 changes: 10 additions & 1 deletion test/presenters/content_item/withdrawable_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,26 @@ def title
content_item["title"]
end

def schema_name
content_item["schema_name"]
end

def document_type
content_item["document_type"]
end

def content_item
{
"title" => "Proportion of residents who do any walking or cycling (at local authority level) (CW010)",
"withdrawn_notice" => {
"withdrawn_at" => "2016-07-12T09:47:15Z",
},
"document_type" => "statistical_data_set",
}
end
end

assert_equal @withdrawable.page_title, "[Withdrawn] Proportion of residents who do any walking or cycling (at local authority level) (CW010)"
assert_equal @withdrawable.page_title, "[Withdrawn] Statistical data set: Proportion of residents who do any walking or cycling (at local authority level) (CW010)"
end

test "notice title and description are generated correctly" do
Expand Down
19 changes: 0 additions & 19 deletions test/views/content_items/attachments.html.erb_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,23 +43,4 @@ class ContentItemsAttachmentsTest < ActionView::TestCase
assert_includes rendered, "gem-c-govspeak"
assert_includes rendered, "some html"
end

test "renders overview title when attachment title matches parent" do
@content_item = PublicationPresenter.new(
{ "document_type" => "correspondence",
"title" => "Matching",
"details" => { "attachments" => [{ "id" => "attachment_id",
"title" => "Matching",
"url" => "some/url" }],
"featured_attachments" => %w[attachment_id] } },
"/publication",
ApplicationController.new.view_context,
)
render(
partial: "content_items/context_and_title",
)

assert_includes rendered, "Correspondence overview:"
assert_select "h1 span"
end
end