From 10071f53275af7a879338a5523e127ae1032e858 Mon Sep 17 00:00:00 2001 From: hannako Date: Fri, 4 Nov 2022 13:05:44 +0000 Subject: [PATCH 1/4] Rename var storing govuk_account_session --- app/controllers/content_items_controller.rb | 2 +- .../shared/_published_dates_with_notification_button.html.erb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/content_items_controller.rb b/app/controllers/content_items_controller.rb index bacd7024d..7f8218580 100644 --- a/app/controllers/content_items_controller.rb +++ b/app/controllers/content_items_controller.rb @@ -138,7 +138,7 @@ def render_template # use these and `@content_item.base_path` in the template @notification_button_visible = @content_item.has_single_page_notifications? - @include_single_page_notification_button_js = account_session_header.present? + @has_govuk_account = account_session_header.present? request.variant = :print if params[:variant] == "print" diff --git a/app/views/shared/_published_dates_with_notification_button.html.erb b/app/views/shared/_published_dates_with_notification_button.html.erb index 29572cf53..6a00ab0a0 100644 --- a/app/views/shared/_published_dates_with_notification_button.html.erb +++ b/app/views/shared/_published_dates_with_notification_button.html.erb @@ -6,7 +6,7 @@ } %> <%= render 'govuk_publishing_components/components/single_page_notification_button', { base_path: @content_item.base_path, - js_enhancement: @include_single_page_notification_button_js, + js_enhancement: @has_govuk_account, button_location: "bottom", margin_bottom: 0, } if @notification_button_visible %> From 8b6f312e3efe29f39b97e894b3fd78298b25673d Mon Sep 17 00:00:00 2001 From: hannako Date: Fri, 4 Nov 2022 13:39:14 +0000 Subject: [PATCH 2/4] Move button html into its own partial --- app/views/content_items/consultation.html.erb | 1 + app/views/content_items/detailed_guide.html.erb | 1 + app/views/content_items/publication.html.erb | 1 + app/views/shared/_publisher_metadata_with_logo.html.erb | 7 ------- app/views/shared/_single_page_notification_button.html.erb | 6 ++++++ 5 files changed, 9 insertions(+), 7 deletions(-) create mode 100644 app/views/shared/_single_page_notification_button.html.erb diff --git a/app/views/content_items/consultation.html.erb b/app/views/content_items/consultation.html.erb index c9887d3ae..0b35c3e59 100644 --- a/app/views/content_items/consultation.html.erb +++ b/app/views/content_items/consultation.html.erb @@ -14,6 +14,7 @@ <%= render 'shared/publisher_metadata_with_logo', content_item: @content_item %> +<%= render 'shared/single_page_notification_button', content_item: @content_item %> <%= render 'shared/history_notice', content_item: @content_item %> <%= render 'govuk_publishing_components/components/notice', @content_item.withdrawal_notice_component %> diff --git a/app/views/content_items/detailed_guide.html.erb b/app/views/content_items/detailed_guide.html.erb index c5a0d5821..a7cc522b3 100644 --- a/app/views/content_items/detailed_guide.html.erb +++ b/app/views/content_items/detailed_guide.html.erb @@ -68,6 +68,7 @@ <%= render 'shared/publisher_metadata_with_logo' %> +<%= render 'shared/single_page_notification_button', content_item: @content_item %> <%= render 'shared/history_notice', content_item: @content_item %> <%= render 'govuk_publishing_components/components/notice', @content_item.withdrawal_notice_component %> diff --git a/app/views/content_items/publication.html.erb b/app/views/content_items/publication.html.erb index 7c36ca386..0f06a9803 100644 --- a/app/views/content_items/publication.html.erb +++ b/app/views/content_items/publication.html.erb @@ -22,6 +22,7 @@ <%= render 'shared/publisher_metadata_with_logo' %> +<%= render 'shared/single_page_notification_button', content_item: @content_item %> <%= render 'shared/history_notice', content_item: @content_item %> diff --git a/app/views/shared/_publisher_metadata_with_logo.html.erb b/app/views/shared/_publisher_metadata_with_logo.html.erb index 207a01b1b..be4175dcb 100644 --- a/app/views/shared/_publisher_metadata_with_logo.html.erb +++ b/app/views/shared/_publisher_metadata_with_logo.html.erb @@ -16,10 +16,3 @@ - -<%= render 'govuk_publishing_components/components/single_page_notification_button', { - base_path: @content_item.base_path, - js_enhancement: @include_single_page_notification_button_js, - margin_bottom: 6, - button_location: "top", -} if @notification_button_visible %> diff --git a/app/views/shared/_single_page_notification_button.html.erb b/app/views/shared/_single_page_notification_button.html.erb new file mode 100644 index 000000000..5f52b7562 --- /dev/null +++ b/app/views/shared/_single_page_notification_button.html.erb @@ -0,0 +1,6 @@ +<%= render 'govuk_publishing_components/components/single_page_notification_button', { + base_path: @content_item.base_path, + js_enhancement: @has_govuk_account, + margin_bottom: 6, + button_location: "top", +} if @notification_button_visible %> From 07cb5d3432499e635f85e06fa3cdf77cd41dddb4 Mon Sep 17 00:00:00 2001 From: hannako Date: Fri, 4 Nov 2022 14:13:37 +0000 Subject: [PATCH 3/4] Remove superflous instance variable from controller - Rather than set an environment variable in the controller which is available to all the views, we should control whether or not we show the button template in the presenter. - I've removed the ALLOWED_DOCUMENT_TYPES because if we only include the ContentItem::SinglePageNotificationButton module in the presenter classes of content types which we want to show the button, the effect is the same. It's also a bit misleading having an allow list because it infers there's a reason some types can't support email subscriptions. This is not the case - email alert API doesn't care. - This pattern of including a module into the document type specific presenter is more consistent with this repo, and other frontend repos. --- app/controllers/content_items_controller.rb | 3 +-- app/presenters/consultation_presenter.rb | 1 + .../content_item/single_page_notification_button.rb | 8 +------- app/presenters/detailed_guide_presenter.rb | 1 + app/presenters/publication_presenter.rb | 1 + .../_published_dates_with_notification_button.html.erb | 2 +- .../shared/_single_page_notification_button.html.erb | 2 +- test/presenters/consultation_presenter_test.rb | 7 +++++++ test/presenters/detailed_guide_presenter_test.rb | 5 +++++ test/presenters/publication_presenter_test.rb | 5 +++++ 10 files changed, 24 insertions(+), 11 deletions(-) diff --git a/app/controllers/content_items_controller.rb b/app/controllers/content_items_controller.rb index 7f8218580..67537ed1b 100644 --- a/app/controllers/content_items_controller.rb +++ b/app/controllers/content_items_controller.rb @@ -136,8 +136,7 @@ def render_template return end - # use these and `@content_item.base_path` in the template - @notification_button_visible = @content_item.has_single_page_notifications? + # use this and `@content_item.base_path` in the template @has_govuk_account = account_session_header.present? request.variant = :print if params[:variant] == "print" diff --git a/app/presenters/consultation_presenter.rb b/app/presenters/consultation_presenter.rb index ff2085926..3dfa6c7fa 100644 --- a/app/presenters/consultation_presenter.rb +++ b/app/presenters/consultation_presenter.rb @@ -6,6 +6,7 @@ class ConsultationPresenter < ContentItemPresenter include ContentItem::Shareable include ContentItem::TitleAndContext include ContentItem::Attachments + include ContentItem::SinglePageNotificationButton def opening_date_time content_item["details"]["opening_date"] diff --git a/app/presenters/content_item/single_page_notification_button.rb b/app/presenters/content_item/single_page_notification_button.rb index 4918b2530..a048e1f8c 100644 --- a/app/presenters/content_item/single_page_notification_button.rb +++ b/app/presenters/content_item/single_page_notification_button.rb @@ -8,14 +8,8 @@ module SinglePageNotificationButton 5f9c6c15-7631-11e4-a3cb-005056011aef ].freeze - ALLOWED_DOCUMENT_TYPES = %w[ - publication - detailed_guide - consultation - ].freeze - def has_single_page_notifications? - (!EXEMPTION_LIST.include? content_item["content_id"]) && (ALLOWED_DOCUMENT_TYPES.include? content_item["schema_name"]) + !EXEMPTION_LIST.include? content_item["content_id"] end end end diff --git a/app/presenters/detailed_guide_presenter.rb b/app/presenters/detailed_guide_presenter.rb index 42b94f979..24125ec06 100644 --- a/app/presenters/detailed_guide_presenter.rb +++ b/app/presenters/detailed_guide_presenter.rb @@ -5,6 +5,7 @@ class DetailedGuidePresenter < ContentItemPresenter include ContentItem::NationalApplicability include ContentItem::Political include ContentItem::TitleAndContext + include ContentItem::SinglePageNotificationButton def title_and_context super.tap do |t| diff --git a/app/presenters/publication_presenter.rb b/app/presenters/publication_presenter.rb index c2553fb29..7514029e9 100644 --- a/app/presenters/publication_presenter.rb +++ b/app/presenters/publication_presenter.rb @@ -4,6 +4,7 @@ class PublicationPresenter < ContentItemPresenter include ContentItem::NationalStatisticsLogo include ContentItem::Political include ContentItem::Attachments + include ContentItem::SinglePageNotificationButton def details content_item["details"]["body"] diff --git a/app/views/shared/_published_dates_with_notification_button.html.erb b/app/views/shared/_published_dates_with_notification_button.html.erb index 6a00ab0a0..e8cb63071 100644 --- a/app/views/shared/_published_dates_with_notification_button.html.erb +++ b/app/views/shared/_published_dates_with_notification_button.html.erb @@ -9,4 +9,4 @@ js_enhancement: @has_govuk_account, button_location: "bottom", margin_bottom: 0, -} if @notification_button_visible %> +} if @content_item.has_single_page_notifications? %> diff --git a/app/views/shared/_single_page_notification_button.html.erb b/app/views/shared/_single_page_notification_button.html.erb index 5f52b7562..ed4b23575 100644 --- a/app/views/shared/_single_page_notification_button.html.erb +++ b/app/views/shared/_single_page_notification_button.html.erb @@ -3,4 +3,4 @@ js_enhancement: @has_govuk_account, margin_bottom: 6, button_location: "top", -} if @notification_button_visible %> +} if @content_item.has_single_page_notifications? %> diff --git a/test/presenters/consultation_presenter_test.rb b/test/presenters/consultation_presenter_test.rb index 4bdc140cf..0152d85c7 100644 --- a/test/presenters/consultation_presenter_test.rb +++ b/test/presenters/consultation_presenter_test.rb @@ -149,5 +149,12 @@ def schema_name assert_equal "https://www.facebook.com/sharer/sharer.php?u=https%3A%2F%2Fwww.test.gov.uk%2Fgovernment%2Fconsultations%2Fpostgraduate-doctoral-loans", presented_item("open_consultation").share_links[0][:href] assert_equal "https://twitter.com/share?url=https%3A%2F%2Fwww.test.gov.uk%2Fgovernment%2Fconsultations%2Fpostgraduate-doctoral-loans&text=Postgraduate%20doctoral%20loans", presented_item("open_consultation").share_links[1][:href] end + + test "presents the single page notification button" do + schema = schema_item("open_consultation") + presented = presented_item("open_consultation", schema) + + assert presented.has_single_page_notifications? + end end end diff --git a/test/presenters/detailed_guide_presenter_test.rb b/test/presenters/detailed_guide_presenter_test.rb index 99d18ac0d..439e6dd66 100644 --- a/test/presenters/detailed_guide_presenter_test.rb +++ b/test/presenters/detailed_guide_presenter_test.rb @@ -83,4 +83,9 @@ def schema_name } assert_equal presented.logo, expected end + + test "presents the single page notification button" do + presented = presented_item("national_applicability_detailed_guide") + assert presented.has_single_page_notifications? + end end diff --git a/test/presenters/publication_presenter_test.rb b/test/presenters/publication_presenter_test.rb index b5cc1b306..ad11cba5d 100644 --- a/test/presenters/publication_presenter_test.rb +++ b/test/presenters/publication_presenter_test.rb @@ -74,4 +74,9 @@ def schema_name assert_equal(presented.national_applicability[:wales][:applicable], false) assert_equal(presented.national_applicability[:wales][:alternative_url], "http://wales.gov.uk/topics/statistics/headlines/housing2012/121025/?lang=en") end + + test "presents the single page notification button" do + presented = presented_item("statistics_publication") + assert presented.has_single_page_notifications? + end end From 4228bb803dc2178121abed98e143574c505608de Mon Sep 17 00:00:00 2001 From: hannako Date: Fri, 4 Nov 2022 15:48:03 +0000 Subject: [PATCH 4/4] Remove module from parent class Currently we are requiring the single page notification button module in all the subclasses inheriting from ContentItemPresenter. We will be adding more into this module in coming work, so it's a good idea to remove unecessary coupling now. --- app/presenters/content_item_presenter.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/presenters/content_item_presenter.rb b/app/presenters/content_item_presenter.rb index 741e9438c..682241201 100644 --- a/app/presenters/content_item_presenter.rb +++ b/app/presenters/content_item_presenter.rb @@ -1,6 +1,5 @@ class ContentItemPresenter include ContentItem::Withdrawable - include ContentItem::SinglePageNotificationButton attr_reader :content_item, :requested_path, :view_context, @@ -47,6 +46,10 @@ def requesting_a_service_sign_in_page? false end + def has_single_page_notifications? + false + end + def available_translations translations = content_item["links"]["available_translations"] || []