From 81921a5d3eb930613346234f34c473d3cfb7bc76 Mon Sep 17 00:00:00 2001 From: Pete Goddard Date: Mon, 8 Jan 2024 15:26:07 +0000 Subject: [PATCH 1/8] Extract method for siteimprove lookup --- app/controllers/metrics_controller.rb | 39 ++++++++++++++++----------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/app/controllers/metrics_controller.rb b/app/controllers/metrics_controller.rb index 6561d8c2..e52d5098 100644 --- a/app/controllers/metrics_controller.rb +++ b/app/controllers/metrics_controller.rb @@ -1,7 +1,6 @@ class MetricsController < ApplicationController def show time_period = params[:date_range] || "past-30-days" - base_path = params[:base_path] curr_period = DateRange.new(time_period) prev_period = curr_period.previous @@ -15,24 +14,32 @@ def show curr_period, ) - if current_user.view_siteimprove? - begin - @summary_info = Siteimprove::FetchSummary.call(url: "https://www.gov.uk/#{params[:base_path]}") - - raw_policy_issues = Siteimprove::FetchPolicyIssues.call(url: "https://www.gov.uk/#{params[:base_path]}") - @policy_issues = Siteimprove::PolicyIssuesPresenter.new(raw_policy_issues, @summary_info) - raw_quality_assurance_issues = Siteimprove::FetchQualityAssuranceIssues.call(url: "https://www.gov.uk/#{params[:base_path]}") - @quality_assurance_issues = Siteimprove::QualityAssuranceIssuesPresenter.new(raw_quality_assurance_issues, @summary_info) - @has_accessibility_info = @policy_issues.any? || @quality_assurance_issues.any? - rescue Siteimprove::BaseError - @has_accessibility_info = false - end - else - @has_accessibility_info = false - end + setup_siteimprove end rescue_from GdsApi::HTTPNotFound do render "errors/404", status: :not_found end + +private + + + def base_path + params[:base_path] + end + + def setup_siteimprove + return unless current_user.view_siteimprove? + + begin + @summary_info = Siteimprove::FetchSummary.call(url: "https://www.gov.uk/#{base_path}") + raw_policy_issues = Siteimprove::FetchPolicyIssues.call(url: "https://www.gov.uk/#{base_path}") + @policy_issues = Siteimprove::PolicyIssuesPresenter.new(raw_policy_issues, @summary_info) + raw_quality_assurance_issues = Siteimprove::FetchQualityAssuranceIssues.call(url: "https://www.gov.uk/#{base_path}") + @quality_assurance_issues = Siteimprove::QualityAssuranceIssuesPresenter.new(raw_quality_assurance_issues, @summary_info) + @has_accessibility_info = @policy_issues.any? || @quality_assurance_issues.any? + rescue Siteimprove::BaseError + @has_accessibility_info = false + end + end end From 5d26d617140cadc5fbd9628f82984ff4dc52cfe6 Mon Sep 17 00:00:00 2001 From: Pete Goddard Date: Mon, 8 Jan 2024 15:58:09 +0000 Subject: [PATCH 2/8] Add view_email_subs? permission to User and factory --- app/models/user.rb | 4 ++++ spec/factories/users.rb | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/app/models/user.rb b/app/models/user.rb index f073750d..ab57dde6 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -4,4 +4,8 @@ class User < ApplicationRecord def view_siteimprove? permissions.include?("view_siteimprove") end + + def view_email_subs? + permissions.include?("view_email_subs") + end end diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 14255383..3f622d8c 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -14,4 +14,8 @@ factory :view_siteimprove_user, parent: :user do permissions { %i[view_siteimprove] } end + + factory :view_email_subs_user, parent: :user do + permissions { %i[view_email_subs] } + end end From 0862456f56d04946c354a64effddaa02f4dfa3f4 Mon Sep 17 00:00:00 2001 From: Pete Goddard Date: Mon, 8 Jan 2024 15:59:30 +0000 Subject: [PATCH 3/8] Conditionally show Email subscriptions section --- app/controllers/metrics_controller.rb | 12 ++++++++- .../metrics/_email_subscriptions.html.erb | 1 + app/views/metrics/show.html.erb | 4 +++ config/locales/defaults/en.yml | 2 ++ .../single_content_item_email_subs_spec.rb | 27 +++++++++++++++++++ 5 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 app/views/metrics/_email_subscriptions.html.erb create mode 100644 spec/features/single_content_item_email_subs_spec.rb diff --git a/app/controllers/metrics_controller.rb b/app/controllers/metrics_controller.rb index e52d5098..b3aab292 100644 --- a/app/controllers/metrics_controller.rb +++ b/app/controllers/metrics_controller.rb @@ -15,6 +15,7 @@ def show ) setup_siteimprove + setup_email_subscriptions end rescue_from GdsApi::HTTPNotFound do @@ -23,11 +24,20 @@ def show private - def base_path params[:base_path] end + def setup_email_subscriptions + return unless current_user.view_email_subs? + + @show_email_subs_section = true + @email_subscriptions = { + subscriber_list_count: 12, + all_notify_count: 26, + } + end + def setup_siteimprove return unless current_user.view_siteimprove? diff --git a/app/views/metrics/_email_subscriptions.html.erb b/app/views/metrics/_email_subscriptions.html.erb new file mode 100644 index 00000000..7b81c089 --- /dev/null +++ b/app/views/metrics/_email_subscriptions.html.erb @@ -0,0 +1 @@ +

<%= t("metrics.email_subscriptions.title") %>

diff --git a/app/views/metrics/show.html.erb b/app/views/metrics/show.html.erb index c897ce39..a020a0b7 100644 --- a/app/views/metrics/show.html.erb +++ b/app/views/metrics/show.html.erb @@ -151,6 +151,10 @@
<%= render "siteimprove_issues" %> <% end %> +<% if @show_email_subs_section %> +
+ <%= render "email_subscriptions" %> +<% end %> diff --git a/config/locales/defaults/en.yml b/config/locales/defaults/en.yml index 44da2b5f..ca3408e7 100644 --- a/config/locales/defaults/en.yml +++ b/config/locales/defaults/en.yml @@ -9,6 +9,8 @@ en: feedback_explorer: 'Feedback Explorer' none: false metrics: + email_subscriptions: + title: 'Email subscriptions' upviews: title: 'Unique page views' short_title: 'Unique page views' diff --git a/spec/features/single_content_item_email_subs_spec.rb b/spec/features/single_content_item_email_subs_spec.rb new file mode 100644 index 00000000..283c8513 --- /dev/null +++ b/spec/features/single_content_item_email_subs_spec.rb @@ -0,0 +1,27 @@ +RSpec.describe "/metrics/base/path email subscription details", type: :feature do + include RequestStubs + + context "logged in as another department" do + before do + GDS::SSO.test_user = build(:user) + stub_metrics_page(base_path: "base/path", time_period: :past_30_days) + end + + it "does not show the Email Subscriptions title" do + visit "/metrics/base/path" + expect(page).not_to have_content("Email subscriptions") + end + end + + context "logged in as GDS" do + before do + GDS::SSO.test_user = build(:view_email_subs_user) + stub_metrics_page(base_path: "base/path", time_period: :past_30_days) + end + + it "shows the Email subscriptions section" do + visit "/metrics/base/path" + expect(page).to have_content("Email subscriptions") + end + end +end From 0a3ca9b19b83f9d32c50779c12931a3e59a5cecb Mon Sep 17 00:00:00 2001 From: Pete Goddard Date: Tue, 9 Jan 2024 12:29:40 +0000 Subject: [PATCH 4/8] Update gds-api-adapters to 92.1.0 --- Gemfile | 3 ++- Gemfile.lock | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/Gemfile b/Gemfile index 39fbcb6d..505b6414 100644 --- a/Gemfile +++ b/Gemfile @@ -5,7 +5,8 @@ gem "rails", "7.1.2" gem "bootsnap", require: false gem "chartkick" gem "fog-aws" -gem "gds-api-adapters" +gem "gds-api-adapters", "~> 92.1.0" + gem "gds-sso" gem "govspeak" gem "govuk_app_config" diff --git a/Gemfile.lock b/Gemfile.lock index 2ffe016c..7732dd92 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -111,7 +111,7 @@ GEM debug_inspector (1.1.0) diff-lcs (1.5.0) docile (1.4.0) - domain_name (0.6.20231109) + domain_name (0.6.20240107) drb (2.2.0) ruby2_keywords erubi (1.12.0) @@ -145,7 +145,7 @@ GEM fog-core nokogiri (>= 1.5.11, < 2.0.0) formatador (1.1.0) - gds-api-adapters (92.0.0) + gds-api-adapters (92.1.0) addressable link_header null_logger @@ -716,7 +716,7 @@ DEPENDENCIES chartkick factory_bot_rails fog-aws - gds-api-adapters + gds-api-adapters (~> 92.1.0) gds-sso govspeak govuk_app_config From 6bc3d0d27af1c23b0bff0b990a32734b09d851f1 Mon Sep 17 00:00:00 2001 From: Pete Goddard Date: Tue, 9 Jan 2024 15:04:08 +0000 Subject: [PATCH 5/8] Introduce client to retrieve metrics --- .../email_api/page_subscriptions_client.rb | 18 +++++++++++ .../email_api/page_subscriptions_response.rb | 10 ++++++ .../page_subscriptions_client_spec.rb | 31 +++++++++++++++++++ 3 files changed, 59 insertions(+) create mode 100644 app/services/email_api/page_subscriptions_client.rb create mode 100644 app/services/email_api/page_subscriptions_response.rb create mode 100644 spec/services/email_api/page_subscriptions_client_spec.rb diff --git a/app/services/email_api/page_subscriptions_client.rb b/app/services/email_api/page_subscriptions_client.rb new file mode 100644 index 00000000..e73cfe5b --- /dev/null +++ b/app/services/email_api/page_subscriptions_client.rb @@ -0,0 +1,18 @@ +module EmailApi + class PageSubscriptionsClient + def initialize + @email_alert_api = GdsApi::EmailAlertApi.new(Plek.find("email-alert-api")) + end + + def fetch(path:) + response = email_alert_api.get_subscriber_list_metrics(path:) + EmailApi::PageSubscriptionsResponse.new(**response.to_h.symbolize_keys) + rescue GdsApi::HTTPNotFound + nil + end + + private + + attr_reader :email_alert_api + end +end diff --git a/app/services/email_api/page_subscriptions_response.rb b/app/services/email_api/page_subscriptions_response.rb new file mode 100644 index 00000000..8759e175 --- /dev/null +++ b/app/services/email_api/page_subscriptions_response.rb @@ -0,0 +1,10 @@ +module EmailApi + class PageSubscriptionsResponse + def initialize(all_notify_count:, subscriber_list_count:) + @all_notify_count = all_notify_count + @subscriber_list_count = subscriber_list_count + end + + attr_reader :all_notify_count, :subscriber_list_count + end +end diff --git a/spec/services/email_api/page_subscriptions_client_spec.rb b/spec/services/email_api/page_subscriptions_client_spec.rb new file mode 100644 index 00000000..7ddca15d --- /dev/null +++ b/spec/services/email_api/page_subscriptions_client_spec.rb @@ -0,0 +1,31 @@ +require "rails_helper" +require "gds_api/test_helpers/email_alert_api" + +RSpec.describe EmailApi::PageSubscriptionsClient do + include GdsApi::TestHelpers::EmailAlertApi + subject { described_class.new } + + context "when subscription metrics exist" do + before do + json = { subscriber_list_count: 3, all_notify_count: 10 }.to_json + stub_get_subscriber_list_metrics(path: "/some/path", response: json) + end + + it "show details for the metrics" do + metrics = subject.fetch(path: "/some/path") + assert_equal 3, metrics.subscriber_list_count + assert_equal 10, metrics.all_notify_count + end + end + + context "when subscription metrics 404" do + before do + stub_get_subscriber_list_metrics_not_found(path: "/some/path") + end + + it "show details for the metrics" do + metrics = subject.fetch(path: "/some/path") + assert_nil metrics + end + end +end From a8b51c20b360edc18d678580bbfb9637a61fd3e9 Mon Sep 17 00:00:00 2001 From: Pete Goddard Date: Tue, 9 Jan 2024 15:23:24 +0000 Subject: [PATCH 6/8] Hook up metric to email api data --- app/controllers/metrics_controller.rb | 5 +---- .../email_api/page_subscriptions_client.rb | 2 +- app/views/metrics/_email_subscriptions.html.erb | 2 ++ .../single_content_item_email_subs_spec.rb | 15 ++++++++++++--- 4 files changed, 16 insertions(+), 8 deletions(-) diff --git a/app/controllers/metrics_controller.rb b/app/controllers/metrics_controller.rb index b3aab292..1d12aa69 100644 --- a/app/controllers/metrics_controller.rb +++ b/app/controllers/metrics_controller.rb @@ -32,10 +32,7 @@ def setup_email_subscriptions return unless current_user.view_email_subs? @show_email_subs_section = true - @email_subscriptions = { - subscriber_list_count: 12, - all_notify_count: 26, - } + @email_subscriptions = EmailApi::PageSubscriptionsClient.new.fetch(path: "/#{base_path}") end def setup_siteimprove diff --git a/app/services/email_api/page_subscriptions_client.rb b/app/services/email_api/page_subscriptions_client.rb index e73cfe5b..c1eb839e 100644 --- a/app/services/email_api/page_subscriptions_client.rb +++ b/app/services/email_api/page_subscriptions_client.rb @@ -1,7 +1,7 @@ module EmailApi class PageSubscriptionsClient def initialize - @email_alert_api = GdsApi::EmailAlertApi.new(Plek.find("email-alert-api")) + @email_alert_api = GdsApi.email_alert_api end def fetch(path:) diff --git a/app/views/metrics/_email_subscriptions.html.erb b/app/views/metrics/_email_subscriptions.html.erb index 7b81c089..0f3ca2e7 100644 --- a/app/views/metrics/_email_subscriptions.html.erb +++ b/app/views/metrics/_email_subscriptions.html.erb @@ -1 +1,3 @@

<%= t("metrics.email_subscriptions.title") %>

+

Subscribers: <%= @email_subscriptions.subscriber_list_count %>

+

All notification subscribers: <%=@email_subscriptions.all_notify_count %>

diff --git a/spec/features/single_content_item_email_subs_spec.rb b/spec/features/single_content_item_email_subs_spec.rb index 283c8513..60e5a862 100644 --- a/spec/features/single_content_item_email_subs_spec.rb +++ b/spec/features/single_content_item_email_subs_spec.rb @@ -1,5 +1,8 @@ +require "gds_api/test_helpers/email_alert_api" + RSpec.describe "/metrics/base/path email subscription details", type: :feature do include RequestStubs + include GdsApi::TestHelpers::EmailAlertApi context "logged in as another department" do before do @@ -17,11 +20,17 @@ before do GDS::SSO.test_user = build(:view_email_subs_user) stub_metrics_page(base_path: "base/path", time_period: :past_30_days) + json = { subscriber_list_count: 3, all_notify_count: 10 }.to_json + stub_get_subscriber_list_metrics(path: "/base/path", response: json) end - it "shows the Email subscriptions section" do - visit "/metrics/base/path" - expect(page).to have_content("Email subscriptions") + context "when there are subscriptions" do + it "shows the Email subscriptions section" do + visit "/metrics/base/path" + expect(page).to have_content("Email subscriptions") + expect(page).to have_content("Subscribers: 3") + expect(page).to have_content("All notification subscribers: 10") + end end end end From 1371f196180a2aa2709b00a3a32f32b9092c12ac Mon Sep 17 00:00:00 2001 From: Pete Goddard Date: Tue, 9 Jan 2024 15:32:06 +0000 Subject: [PATCH 7/8] Add a message when there is a 404 from email-alert-api --- .../metrics/_email_subscriptions.html.erb | 8 +++++-- .../single_content_item_email_subs_spec.rb | 22 +++++++++++++++++-- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/app/views/metrics/_email_subscriptions.html.erb b/app/views/metrics/_email_subscriptions.html.erb index 0f3ca2e7..47e6fe29 100644 --- a/app/views/metrics/_email_subscriptions.html.erb +++ b/app/views/metrics/_email_subscriptions.html.erb @@ -1,3 +1,7 @@

<%= t("metrics.email_subscriptions.title") %>

-

Subscribers: <%= @email_subscriptions.subscriber_list_count %>

-

All notification subscribers: <%=@email_subscriptions.all_notify_count %>

+<% if @email_subscriptions.present? %> +

Subscribers: <%= @email_subscriptions.subscriber_list_count %>

+

All notification subscribers: <%=@email_subscriptions.all_notify_count %>

+<% else %> +

No subscription information found

+<% end %> diff --git a/spec/features/single_content_item_email_subs_spec.rb b/spec/features/single_content_item_email_subs_spec.rb index 60e5a862..d6be6359 100644 --- a/spec/features/single_content_item_email_subs_spec.rb +++ b/spec/features/single_content_item_email_subs_spec.rb @@ -20,16 +20,34 @@ before do GDS::SSO.test_user = build(:view_email_subs_user) stub_metrics_page(base_path: "base/path", time_period: :past_30_days) - json = { subscriber_list_count: 3, all_notify_count: 10 }.to_json - stub_get_subscriber_list_metrics(path: "/base/path", response: json) end context "when there are subscriptions" do + before do + json = { subscriber_list_count: 3, all_notify_count: 10 }.to_json + stub_get_subscriber_list_metrics(path: "/base/path", response: json) + end + it "shows the Email subscriptions section" do visit "/metrics/base/path" expect(page).to have_content("Email subscriptions") expect(page).to have_content("Subscribers: 3") expect(page).to have_content("All notification subscribers: 10") + expect(page).not_to have_content("No subscription information found") + end + end + + context "when there is a 404 from email-alert-api" do + before do + stub_get_subscriber_list_metrics_not_found(path: "/base/path") + end + + it "shows the no subscription info section" do + visit "/metrics/base/path" + expect(page).to have_content("Email subscriptions") + expect(page).to have_content("No subscription information found") + expect(page).not_to have_content("Subscribers:") + expect(page).not_to have_content("All notification subscribers:") end end end From 43576f9c10913b50df3ab2bc8cfb97432865149c Mon Sep 17 00:00:00 2001 From: Pete Goddard Date: Thu, 11 Jan 2024 15:11:28 +0000 Subject: [PATCH 8/8] Use en.yml for text and render an info section --- .../metrics/_email_subscriptions.html.erb | 18 +++++++++++++++--- config/locales/defaults/en.yml | 6 ++++++ .../single_content_item_email_subs_spec.rb | 18 ++++++++++++++---- 3 files changed, 35 insertions(+), 7 deletions(-) diff --git a/app/views/metrics/_email_subscriptions.html.erb b/app/views/metrics/_email_subscriptions.html.erb index 47e6fe29..ecffe450 100644 --- a/app/views/metrics/_email_subscriptions.html.erb +++ b/app/views/metrics/_email_subscriptions.html.erb @@ -1,7 +1,19 @@

<%= t("metrics.email_subscriptions.title") %>

<% if @email_subscriptions.present? %> -

Subscribers: <%= @email_subscriptions.subscriber_list_count %>

-

All notification subscribers: <%=@email_subscriptions.all_notify_count %>

+

<%= t("metrics.email_subscriptions.active_title") %>: <%= @email_subscriptions.subscriber_list_count %>

+

<%= t("metrics.email_subscriptions.total_notify_title") %>: <%=@email_subscriptions.all_notify_count %>

<% else %> -

No subscription information found

+

<%= t("metrics.email_subscriptions.no_information") %>

+<% end %> + +<%= render "govuk_publishing_components/components/details", { + title: t("metrics.email_subscriptions.about_title") +} do %> + +

<%= t("metrics.email_subscriptions.active_title") %>

+

<%= t("metrics.email_subscriptions.active_description") %>

+ +

<%= t("metrics.email_subscriptions.total_notify_title") %>

+

<%= t("metrics.email_subscriptions.total_notify_description") %>

+ <% end %> diff --git a/config/locales/defaults/en.yml b/config/locales/defaults/en.yml index ca3408e7..63e7ed92 100644 --- a/config/locales/defaults/en.yml +++ b/config/locales/defaults/en.yml @@ -11,6 +11,12 @@ en: metrics: email_subscriptions: title: 'Email subscriptions' + about_title: 'About Email subscriptions' + active_title: 'Active subscribers' + active_description: 'Active subscribers is the number of people who have clicked the Get Emails button on this page and signed up to be subscribed to valid email alerts either to this page or to pages related to this one. Note that some pages can be subscribed to, but do not generate email alerts themselves (in these cases the subscriber list will be notified if pages related to this one are updated)' + total_notify_title: 'Number of subscribers notified by change' + total_notify_description: 'Number of subscribers notified by change is the number of people who will receive emails if this page receives a major update. Note that this may be zero even if the page has active subscribers (if the page does not generate email alerts), or may be positive even if the page has no active subscribers (as there are lists that subscribe to all emails, or all emails on a certain topic)' + no_information: 'No subscription information found' upviews: title: 'Unique page views' short_title: 'Unique page views' diff --git a/spec/features/single_content_item_email_subs_spec.rb b/spec/features/single_content_item_email_subs_spec.rb index d6be6359..8d5b9605 100644 --- a/spec/features/single_content_item_email_subs_spec.rb +++ b/spec/features/single_content_item_email_subs_spec.rb @@ -31,10 +31,20 @@ it "shows the Email subscriptions section" do visit "/metrics/base/path" expect(page).to have_content("Email subscriptions") - expect(page).to have_content("Subscribers: 3") - expect(page).to have_content("All notification subscribers: 10") + expect(page).to have_content("Active subscribers: 3") + expect(page).to have_content("Number of subscribers notified by change: 10") expect(page).not_to have_content("No subscription information found") end + + it "shows information on the email metrics" do + visit "/metrics/base/path" + [ + "Active subscribers is the number of people", + "Number of subscribers notified by change is the number of people", + ].each do |txt| + expect(page).to have_content(txt) + end + end end context "when there is a 404 from email-alert-api" do @@ -46,8 +56,8 @@ visit "/metrics/base/path" expect(page).to have_content("Email subscriptions") expect(page).to have_content("No subscription information found") - expect(page).not_to have_content("Subscribers:") - expect(page).not_to have_content("All notification subscribers:") + expect(page).not_to have_content("Active subscribers:") + expect(page).not_to have_content("Number of subscribers notified by change:") end end end