From a7a5bdc6b11effc15cad516743a690f3b081adad Mon Sep 17 00:00:00 2001 From: Simon Hughesdon Date: Mon, 16 Jul 2018 09:56:22 +0100 Subject: [PATCH 1/7] Add content pages AB test logic This adds the AB testing logic we will hook into in subsequent commits. We will be showing taxonomy-based navigation in variant B. Rules for the AB test: - Exclude HTML publications as these don't yet use the universal layout - Included pages must be tagged to at least one live taxon. --- .../concerns/content_pages_nav_ab_testable.rb | 51 +++++++++++++ app/controllers/content_items_controller.rb | 7 ++ app/views/layouts/application.html.erb | 1 + .../content_pages_nav_ab_testable_test.rb | 71 ++++++++++++++++++ .../content_pages_navigation_test.rb | 71 ++++++++++++++++++ test/support/content_pages_nav_test_helper.rb | 75 +++++++++++++++++++ 6 files changed, 276 insertions(+) create mode 100644 app/controllers/concerns/content_pages_nav_ab_testable.rb create mode 100644 test/controllers/content_pages_nav_ab_testable_test.rb create mode 100644 test/integration/content_pages_navigation_test.rb create mode 100644 test/support/content_pages_nav_test_helper.rb diff --git a/app/controllers/concerns/content_pages_nav_ab_testable.rb b/app/controllers/concerns/content_pages_nav_ab_testable.rb new file mode 100644 index 000000000..715b86327 --- /dev/null +++ b/app/controllers/concerns/content_pages_nav_ab_testable.rb @@ -0,0 +1,51 @@ +module ContentPagesNavAbTestable + CUSTOM_DIMENSION = 65 + + def self.included(base) + base.helper_method( + :content_pages_nav_test_variant, + :show_new_navigation?, + :page_in_scope? + ) + base.after_action :set_test_response_header + end + + def content_pages_nav_test + @content_pages_nav_test ||= GovukAbTesting::AbTest.new( + "ContentPagesNav", + dimension: CUSTOM_DIMENSION, + allowed_variants: %w(A B), + control_variant: "A" + ) + end + + def content_pages_nav_test_variant + @content_pages_nav_test_variant ||= content_pages_nav_test.requested_variant(request.headers) + end + + def set_test_response_header + content_pages_nav_test_variant.configure_response(response) if page_in_scope? + end + + def show_new_navigation? + !content_pages_nav_test_variant.variant?("B") && page_in_scope? + end + + def page_in_scope? + # Pages are in scope if: + # - They are not an out-of-scope format, e.g: html publications + # - They are tagged to the taxonomy + if @content_item + not_an_html_publication? && has_a_live_taxon? + end + end + + def not_an_html_publication? + !@content_item.document_type.eql?("html_publication") + end + + def has_a_live_taxon? + @content_item.taxons.present? && + @content_item.taxons.detect { |taxon| taxon["phase"] == "live" } + end +end diff --git a/app/controllers/content_items_controller.rb b/app/controllers/content_items_controller.rb index 155fe7e3f..0d0fa3836 100644 --- a/app/controllers/content_items_controller.rb +++ b/app/controllers/content_items_controller.rb @@ -1,4 +1,5 @@ class ContentItemsController < ApplicationController + include ContentPagesNavAbTestable rescue_from GdsApi::HTTPForbidden, with: :error_403 rescue_from GdsApi::HTTPNotFound, with: :error_notfound @@ -14,6 +15,8 @@ class ContentItemsController < ApplicationController def show load_content_item + load_taxonomy_navigation if show_new_navigation? + set_expiry set_access_control_allow_origin_header if request.format.atom? set_guide_draft_access_token if @content_item.is_a?(GuidePresenter) @@ -60,6 +63,10 @@ def load_content_item @content_item = PresenterBuilder.new(content_item, content_item_path).presenter end + def load_taxonomy_navigation + # to be fleshed out with the content pages topic taxon navigation + end + def content_item_template @content_item.schema_name end diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index b5a22e462..3800f29dd 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -24,6 +24,7 @@ <% if @content_item.description %> <% end %> + <%= content_pages_nav_test_variant.analytics_meta_tag.html_safe if page_in_scope? %> <%= yield :extra_head_content %> diff --git a/test/controllers/content_pages_nav_ab_testable_test.rb b/test/controllers/content_pages_nav_ab_testable_test.rb new file mode 100644 index 000000000..2a6c4d8c6 --- /dev/null +++ b/test/controllers/content_pages_nav_ab_testable_test.rb @@ -0,0 +1,71 @@ +require 'test_helper' + +class ContentItemsControllerTest < ActionController::TestCase + include GdsApi::TestHelpers::ContentStore + include GovukAbTesting::MinitestHelpers + + %w(A B).each do |test_variant| + test "ContentPagesNav variant #{test_variant} works with a valid taxon" do + content_item = content_store_has_schema_example("guide", "single-page-guide").tap do |item| + item["links"]["taxons"] = SINGLE_TAXON + end + + ensure_ab_test_is_correctly_setup(test_variant, content_item) + end + + test "ContentPagesNav variant #{test_variant} works without a valid taxon" do + content_item = content_store_has_schema_example("guide", "single-page-guide") + content_store_has_item(content_item['base_path'], content_item) + + @controller.stubs(:page_in_scope?).returns(true) + ensure_ab_test_is_correctly_setup(test_variant, content_item) + end + + # HTML publications don't use the universal layout so we'll exclude them for now + test "ContentPagesNav test is inactive for HTML publications (variant #{test_variant})" do + content_item = content_store_has_schema_example("html_publication", "published").tap do |item| + item["links"]["taxons"] = SINGLE_TAXON + end + + setup_ab_variant("ContentPagesNav", test_variant) + + get :show, params: { path: path_for(content_item) } + + assert_response_not_modified_for_ab_test("ContentPagesNav") + end + + test "ContentPagesNav test is inactive when content is not tagged to a live taxon (variant #{test_variant})" do + content_item = content_store_has_schema_example("guide", "single-page-guide").tap do |item| + item["links"]["taxons"] = SINGLE_NON_LIVE_TAXON + end + + setup_ab_variant("ContentPagesNav", test_variant) + + get :show, params: { path: path_for(content_item) } + + assert_response_not_modified_for_ab_test("ContentPagesNav") + end + end + + def ensure_ab_test_is_correctly_setup(test_variant, content_item) + content_store_has_item(content_item['base_path'], content_item) + + @controller.stubs(:page_in_scope?).returns(true) + + with_variant ContentPagesNav: test_variant do + get :show, params: { path: path_for(content_item) } + assert_response 200 + + ab_test = GovukAbTesting::AbTest.new("ContentPagesNav", dimension: 65) + requested = ab_test.requested_variant(request.headers) + assert requested.variant?(test_variant) + end + end + end + + def path_for(content_item, locale = nil) + base_path = content_item['base_path'].sub(/^\//, '') + base_path.gsub!(/\.#{locale}$/, '') if locale + base_path + end +end diff --git a/test/integration/content_pages_navigation_test.rb b/test/integration/content_pages_navigation_test.rb new file mode 100644 index 000000000..46cedabf0 --- /dev/null +++ b/test/integration/content_pages_navigation_test.rb @@ -0,0 +1,71 @@ +require 'test_helper' + +class ContentPagesNavigationTest < ActionDispatch::IntegrationTest + include ContentPagesNavTestHelper + include GdsApi::TestHelpers::Rummager + + test "ContentPagesNav variant A does not show taxonomy navigation for single taxon" do + setup_variant_a + + setup_and_visit_content_item_with_taxons('guide', SINGLE_TAXON) + + refute page.has_css?('.taxonomy-navigation') + end + + test "ContentPagesNav variant B does not show taxonomy navigation for content not tagged to a taxon" do + setup_variant_b + + setup_and_visit_content_item_with_taxons('guide', []) + + refute page.has_css?('.taxonomy-navigation') + end + + test "ContentPagesNav variant B shows taxonomy navigation for single taxon" do + stub_rummager + setup_variant_b + + setup_and_visit_content_item_with_taxons('guide', SINGLE_TAXON) + + assert page.has_css?('.taxonomy-navigation h2', text: 'Becoming an apprentice') + assert page.has_css?('.gem-c-highlight-boxes__title', text: 'Apprenticeship agreement: template') + end + + test "ContentPagesNav variant B renders many taxons nicely" do + stub_rummager + setup_variant_b + + setup_and_visit_content_item_with_taxons('guide', THREE_TAXONS) + + assert page.has_css?('.taxonomy-navigation h2', text: 'Becoming an apprentice') + assert page.has_css?('.taxonomy-navigation h2', text: 'Becoming a wizard') + assert page.has_css?('.taxonomy-navigation h2', text: 'Becoming the sorceror supreme') + + assert page.has_css?('.gem-c-highlight-boxes__title', text: 'Apprenticeship agreement: template') + end + + test "ContentPagesNav variant B only includes live taxons" do + stub_rummager + setup_variant_b + + taxons = SINGLE_TAXON + SINGLE_NON_LIVE_TAXON + + setup_and_visit_content_item_with_taxons('guide', taxons) + + assert page.has_css?('.taxonomy-navigation h2', text: 'Becoming an apprentice') + refute page.has_css?('.taxonomy-navigation h2', text: 'Becoming a ghostbuster') + + assert page.has_css?('.gem-c-highlight-boxes__title', text: 'Apprenticeship agreement: template') + end + + def setup_variant_a + ContentItemsController.any_instance.stubs(:show_new_navigation?).returns(false) + end + + def setup_variant_b + ContentItemsController.any_instance.stubs(:show_new_navigation?).returns(true) + end + + def schema_type + "guide" + end +end diff --git a/test/support/content_pages_nav_test_helper.rb b/test/support/content_pages_nav_test_helper.rb new file mode 100644 index 000000000..81cb4a471 --- /dev/null +++ b/test/support/content_pages_nav_test_helper.rb @@ -0,0 +1,75 @@ +module ContentPagesNavTestHelper + def stub_rummager + stub_any_rummager_search + .to_return( + body: { + "results": [ + { + "content_store_document_type": "form", + "description": "This agreement must be signed by the apprentice and the employer at the start of the apprenticeship.", + "link": "/government/publications/apprenticeship-agreement-template", + "public_timestamp": "2012-08-28T00:00:00.000+01:00", + "title": "Apprenticeship agreement: template", + "index": "government", + "_id": "/government/publications/apprenticeship-agreement-template", + "elasticsearch_type": "edition", + "document_type": "edition" + } + ], + "total": 1, + "start": 0, + "aggregates": {}, + "suggested_queries": [] + }.to_json + ) + end + + SINGLE_TAXON = [ + { + "base_path" => "/education/becoming-an-apprentice", + "content_id" => "ff0e8e1f-4dea-42ff-b1d5-f1ae37807af2", + "description" => "Pay and conditions, how to apply, become a higher or degree apprentice. Apprenticeship levels, training, find an apprenticeship.", + "schema_name" => "taxon", + "title" => "Becoming an apprentice", + "phase" => "live", + } + ].freeze + + THREE_TAXONS = [ + { + "base_path" => "/education/becoming-an-apprentice", + "content_id" => "ff0e8e1f-4dea-42ff-b1d5-f1ae37807af2", + "description" => "Pay and conditions, how to apply, become a higher or degree apprentice. Apprenticeship levels, training, find an apprenticeship.", + "schema_name" => "taxon", + "title" => "Becoming an apprentice", + "phase" => "live", + }, + { + "base_path" => "/education/becoming-a-wizard", + "content_id" => "ff0e8e1f-4dea-42ff-b1d5-f1ae37807af3", + "description" => "Pay and conditions, how to apply, become a higher or degree wizard. Wizard levels, training, find a wizard placement.", + "schema_name" => "taxon", + "title" => "Becoming a wizard", + "phase" => "live", + }, + { + "base_path" => "/education/becoming-the-sorceror-supreme", + "content_id" => "ff0e8e1f-4dea-42ff-b1d5-f1ae37807af4", + "description" => "Pay and conditions, how to apply. The astral plane, the mirror dimension, infinity stones.", + "schema_name" => "taxon", + "title" => "Becoming the sorceror supreme", + "phase" => "live", + } + ].freeze + + SINGLE_NON_LIVE_TAXON = [ + { + "base_path" => "/education/becoming-a-ghostbuster", + "content_id" => "ff0e8e1f-4dea-42ff-b1d5-f1ae37807af5", + "description" => "Pay and conditions, how to apply, use of a proton accelerator.", + "schema_name" => "taxon", + "title" => "Becoming a ghostbuster", + "phase" => "ethereal", + } + ].freeze +end From eaa4428b4e71d5ac96ad2d0e11483f63c81d4253 Mon Sep 17 00:00:00 2001 From: Simon Hughesdon Date: Thu, 19 Jul 2018 14:43:11 +0100 Subject: [PATCH 2/7] Add supergroup and rummager logic Puts in place the supergroup (services) and rummager search logic in place ready to retrieve the most popular services. We are also stubbing the rummager requests in the tests. The test helpers in gds-api-adapters are all set to stub rummager responses. This was done in https://github.com/alphagov/gds-api-adapters/commit/ddae94d1656f59f7778ca794a0cce356fc75e7e2 as an explicit choice. --- app/lib/services.rb | 5 ++ app/presenters/supergroups/services.rb | 7 ++ app/services/most_popular_content.rb | 40 +++++++++ app/services/rummager_fields.rb | 14 +++ startup.sh | 2 + test/integration/specialist_document_test.rb | 6 ++ test/services/most_popular_content_test.rb | 94 ++++++++++++++++++++ 7 files changed, 168 insertions(+) create mode 100644 app/presenters/supergroups/services.rb create mode 100644 app/services/most_popular_content.rb create mode 100644 app/services/rummager_fields.rb create mode 100644 test/services/most_popular_content_test.rb diff --git a/app/lib/services.rb b/app/lib/services.rb index 7c1b2fa84..11a57fd00 100644 --- a/app/lib/services.rb +++ b/app/lib/services.rb @@ -1,4 +1,5 @@ require 'gds_api/content_store' +require 'gds_api/rummager' module Services def self.content_store @@ -10,4 +11,8 @@ def self.content_store disable_cache: true, ) end + + def self.rummager + @rummager ||= GdsApi::Rummager.new(Plek.new.find("rummager")) + end end diff --git a/app/presenters/supergroups/services.rb b/app/presenters/supergroups/services.rb new file mode 100644 index 000000000..8033450c5 --- /dev/null +++ b/app/presenters/supergroups/services.rb @@ -0,0 +1,7 @@ +module Supergroups + class Services + def tagged_content(taxon_id) + @content = MostPopularContent.fetch(content_id: taxon_id, filter_content_purpose_supergroup: "services") + end + end +end diff --git a/app/services/most_popular_content.rb b/app/services/most_popular_content.rb new file mode 100644 index 000000000..0a386e74b --- /dev/null +++ b/app/services/most_popular_content.rb @@ -0,0 +1,40 @@ +require 'gds_api/rummager' + +class MostPopularContent + include RummagerFields + + attr_reader :content_id, :filter_content_purpose_supergroup, :number_of_links + + def initialize(content_id:, filter_content_purpose_supergroup:, number_of_links: 5) + @content_id = content_id + @filter_content_purpose_supergroup = filter_content_purpose_supergroup + @number_of_links = number_of_links + end + + def self.fetch(content_id:, filter_content_purpose_supergroup:) + new(content_id: content_id, filter_content_purpose_supergroup: filter_content_purpose_supergroup).fetch + end + + def fetch + search_response + end + +private + + def search_response + params = { + start: 0, + count: number_of_links, + fields: RummagerFields::TAXON_SEARCH_FIELDS, + filter_part_of_taxonomy_tree: Array(content_id), + order: '-popularity', + } + params[:filter_content_purpose_supergroup] = filter_content_purpose_supergroup if filter_content_purpose_supergroup.present? + + search_result(params) + end + + def search_result(params) + @_search_result ||= Services.rummager.search(params) + end +end diff --git a/app/services/rummager_fields.rb b/app/services/rummager_fields.rb new file mode 100644 index 000000000..97fa8651d --- /dev/null +++ b/app/services/rummager_fields.rb @@ -0,0 +1,14 @@ +module RummagerFields + TAXON_SEARCH_FIELDS = %w(title + link + description + content_store_document_type + public_timestamp + organisations).freeze + + FEED_SEARCH_FIELDS = %w(title + link + description + display_type + public_timestamp).freeze +end diff --git a/startup.sh b/startup.sh index 421990265..10c628858 100755 --- a/startup.sh +++ b/startup.sh @@ -8,6 +8,7 @@ if [[ $1 == "--live" ]] ; then PLEK_SERVICE_CONTENT_STORE_URI=${PLEK_SERVICE_CONTENT_STORE_URI-https://www.gov.uk/api} \ PLEK_SERVICE_STATIC_URI=${PLEK_SERVICE_STATIC_URI-assets.publishing.service.gov.uk} \ PLEK_SERVICE_RUMMAGER_URI=${PLEK_SERVICE_RUMMAGER_URI-https://www.gov.uk/api} \ + PLEK_SERVICE_SEARCH_URI=${PLEK_SERVICE_SEARCH_URI-https://www.gov.uk/api} \ bundle exec rails s -p 3090 elif [[ $1 == "--dummy" ]] ; then GOVUK_APP_DOMAIN=www.gov.uk \ @@ -15,6 +16,7 @@ elif [[ $1 == "--dummy" ]] ; then PLEK_SERVICE_CONTENT_STORE_URI=${PLEK_SERVICE_CONTENT_STORE_URI-https://govuk-content-store-examples.herokuapp.com/api} \ PLEK_SERVICE_STATIC_URI=${PLEK_SERVICE_STATIC_URI-assets.publishing.service.gov.uk} \ PLEK_SERVICE_RUMMAGER_URI=${PLEK_SERVICE_RUMMAGER_URI-https://www.gov.uk/api} \ + PLEK_SERVICE_SEARCH_URI=${PLEK_SERVICE_SEARCH_URI-https://www.gov.uk/api} \ bundle exec rails s -p 3090 else bundle exec rails s -p 3090 diff --git a/test/integration/specialist_document_test.rb b/test/integration/specialist_document_test.rb index 8dbfb9a3f..09a369e27 100644 --- a/test/integration/specialist_document_test.rb +++ b/test/integration/specialist_document_test.rb @@ -1,6 +1,12 @@ require 'test_helper' class SpecialistDocumentTest < ActionDispatch::IntegrationTest + include GdsApi::TestHelpers::Rummager + + setup do + stub_any_rummager_search + end + test "random but valid specialist documents do not error" do setup_and_visit_random_content_item(document_type: 'aaib_report') setup_and_visit_random_content_item(document_type: 'raib_report') diff --git a/test/services/most_popular_content_test.rb b/test/services/most_popular_content_test.rb new file mode 100644 index 000000000..c7409d4f1 --- /dev/null +++ b/test/services/most_popular_content_test.rb @@ -0,0 +1,94 @@ +require 'test_helper' + +class MostPopularContentTest < ActiveSupport::TestCase + include RummagerFields + + def most_popular_content + @most_popular_content ||= MostPopularContent.new( + content_id: taxon_content_id, + filter_content_purpose_supergroup: 'guidance_and_regulation' + ) + end + + def taxon_content_id + 'c3c860fc-a271-4114-b512-1c48c0f82564' + end + + test 'returns the results from search' do + search_results = { + 'results' => [ + { 'title' => 'Doc 1' }, + { 'title' => 'A Doc 2' }, + ] + } + + Services.rummager.stubs(:search).returns(search_results) + + results = most_popular_content.fetch + assert_equal(results.count, 2) + end + + test 'starts from the first page' do + assert_includes_params(start: 0) do + most_popular_content.fetch + end + end + + test 'requests five results by default' do + assert_includes_params(count: 5) do + most_popular_content.fetch + end + end + + test 'requests a limited number of fields' do + fields = RummagerFields::TAXON_SEARCH_FIELDS + + assert_includes_params(fields: fields) do + most_popular_content.fetch + end + end + + test 'orders the results by popularity in descending order' do + assert_includes_params(order: '-popularity') do + most_popular_content.fetch + end + end + + test 'scopes the results to the current taxon' do + assert_includes_params(filter_part_of_taxonomy_tree: Array(taxon_content_id)) do + most_popular_content.fetch + end + end + + test 'filters content by the requested filter_content_purpose_supergroup only' do + assert_includes_params(filter_content_purpose_supergroup: 'guidance_and_regulation') do + most_popular_content.fetch + end + end + + def assert_includes_params(expected_params) + search_results = { + 'results' => [ + { + 'title' => 'Doc 1' + }, + { + 'title' => 'Doc 2' + } + ] + } + + Services. + rummager. + stubs(:search). + with { |params| params.including?(expected_params) }. + returns(search_results) + + results = yield + + assert_equal(results.count, 2) + + assert_equal(results.first.title, 'Doc 1') + assert_equal(results.last.title, 'Doc 2') + end +end From c5896874ab831a72d75ab65c7c649e4fa7b94fcc Mon Sep 17 00:00:00 2001 From: Vanita Barrett Date: Wed, 18 Jul 2018 13:22:50 +0000 Subject: [PATCH 3/7] Retrieve most popular services for taxon Retrieves the most popular services for the first taxon that content item is tagged to. TODO: handle tagging to multiple taxons --- app/presenters/content_item_presenter.rb | 4 +++- app/presenters/supergroups/services.rb | 26 ++++++++++++++++++++++-- app/services/most_popular_content.rb | 4 ++-- 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/app/presenters/content_item_presenter.rb b/app/presenters/content_item_presenter.rb index b3fa5def7..daf799266 100644 --- a/app/presenters/content_item_presenter.rb +++ b/app/presenters/content_item_presenter.rb @@ -10,7 +10,8 @@ class ContentItemPresenter :locale, :phase, :part_slug, - :document_type + :document_type, + :taxons def initialize(content_item, requested_content_item_path = nil) @content_item = content_item @@ -22,6 +23,7 @@ def initialize(content_item, requested_content_item_path = nil) @locale = content_item["locale"] || "en" @phase = content_item["phase"] @document_type = content_item["document_type"] + @taxons = content_item["links"]["taxons"] if content_item["links"] @part_slug = requesting_a_part? ? requested_content_item_path.split('/').last : nil end diff --git a/app/presenters/supergroups/services.rb b/app/presenters/supergroups/services.rb index 8033450c5..7d534ff3b 100644 --- a/app/presenters/supergroups/services.rb +++ b/app/presenters/supergroups/services.rb @@ -1,7 +1,29 @@ module Supergroups class Services - def tagged_content(taxon_id) - @content = MostPopularContent.fetch(content_id: taxon_id, filter_content_purpose_supergroup: "services") + attr_reader :content + + def initialize(taxon_id) + @taxon_id = taxon_id + end + + def tagged_content + @content = MostPopularContent.fetch(content_id: @taxon_id, filter_content_purpose_supergroup: "services") + format_document_data(@content) + end + + private + + def format_document_data(documents) + documents&.map do |document| + data = { + link: { + text: document["title"], + path: document["link"] + } + } + + data + end end end end diff --git a/app/services/most_popular_content.rb b/app/services/most_popular_content.rb index 0a386e74b..a3a53f883 100644 --- a/app/services/most_popular_content.rb +++ b/app/services/most_popular_content.rb @@ -16,7 +16,7 @@ def self.fetch(content_id:, filter_content_purpose_supergroup:) end def fetch - search_response + search_response["results"] end private @@ -35,6 +35,6 @@ def search_response end def search_result(params) - @_search_result ||= Services.rummager.search(params) + Services.rummager.search(params) end end From 39916bd5da3adeb91ac3985095c7aaf44406cf40 Mon Sep 17 00:00:00 2001 From: Vanita Barrett Date: Wed, 18 Jul 2018 13:50:24 +0000 Subject: [PATCH 4/7] Render services section in taxonomy navigation --- app/assets/stylesheets/application.scss | 1 + .../helpers/_taxonomy-navigation.scss | 19 +++++++++++++++ app/controllers/content_items_controller.rb | 21 +++++++++++++++-- app/views/layouts/application.html.erb | 3 +++ .../shared/_taxonomy_navigation.html.erb | 23 +++++++++++++++++++ config/locales/en.yml | 3 +++ .../content_items_controller_test.rb | 2 ++ .../step_navigation_controller_test.rb | 4 ++++ 8 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 app/assets/stylesheets/helpers/_taxonomy-navigation.scss create mode 100644 app/views/shared/_taxonomy_navigation.html.erb diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index f9643040a..6c2d683df 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -17,6 +17,7 @@ @import 'helpers/add-title-margin'; @import "helpers/content-bottom-margin"; @import "helpers/publisher-metadata-with-logo"; +@import "helpers/taxonomy-navigation"; // Components from this application @import 'components/*'; diff --git a/app/assets/stylesheets/helpers/_taxonomy-navigation.scss b/app/assets/stylesheets/helpers/_taxonomy-navigation.scss new file mode 100644 index 000000000..971ea8b8a --- /dev/null +++ b/app/assets/stylesheets/helpers/_taxonomy-navigation.scss @@ -0,0 +1,19 @@ +.taxonomy-navigation { + border-top: 2px solid $govuk-blue; + padding-top: $gutter-half; +} + +.taxonomy-navigation__title-link { + @include bold-24; + display: block; +} + +.taxonomy-navigation__section { + padding: $gutter-one-third 0 $gutter-two-thirds 0; + margin-top: $gutter-half; + border-top: 1px solid $grey-2; +} + +.taxonomy-navigation__section:first-of-type { + border-top: 0; +} diff --git a/app/controllers/content_items_controller.rb b/app/controllers/content_items_controller.rb index 0d0fa3836..314dc2179 100644 --- a/app/controllers/content_items_controller.rb +++ b/app/controllers/content_items_controller.rb @@ -10,7 +10,7 @@ class ContentItemsController < ApplicationController rescue_from PresenterBuilder::RedirectRouteReturned, with: :error_redirect rescue_from PresenterBuilder::SpecialRouteReturned, with: :error_notfound - attr_accessor :content_item + attr_accessor :content_item, :taxonomy_navigation def show load_content_item @@ -64,7 +64,24 @@ def load_content_item end def load_taxonomy_navigation - # to be fleshed out with the content pages topic taxon navigation + # For now, just get the first tagged taxon. + # TODO: handle content tagged to multiple taxons + if @content_item.taxons + taxon = @content_item.taxons.first + taxon_id = taxon["content_id"] + + services = Supergroups::Services.new(taxon_id) + + @taxonomy_navigation = { + services: services.tagged_content, + } + + @tagged_taxon = { + taxon_id: taxon_id, + taxon_name: taxon["title"], + taxon_link: taxon["base_path"], + } + end end def content_item_template diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 3800f29dd..1e1f45cf2 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -44,6 +44,9 @@
<%= yield %> + <% if show_new_navigation? && @taxonomy_navigation %> + <%= render 'shared/taxonomy_navigation', taxonomy_navigation: @taxonomy_navigation, tagged_taxon: @tagged_taxon %> + <% end %>
<%= render 'govuk_publishing_components/components/feedback' %> diff --git a/app/views/shared/_taxonomy_navigation.html.erb b/app/views/shared/_taxonomy_navigation.html.erb new file mode 100644 index 000000000..7ccdb7861 --- /dev/null +++ b/app/views/shared/_taxonomy_navigation.html.erb @@ -0,0 +1,23 @@ +<% if taxonomy_navigation.values().flatten.any? %> +
+

More in + <%= tagged_taxon[:taxon_name] %> +

+ + <% if taxonomy_navigation[:services].any? %> +
+ <%= render "govuk_publishing_components/components/heading", { + text: t('supergroups.services'), + heading_level: 3, + font_size: 24, + margin_bottom: 2 + } %> + + <%= render "govuk_publishing_components/components/highlight_boxes", { + inverse: true, + items: taxonomy_navigation[:services] + } %> +
+ <% end %> +
+<% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 248484c68..5103cd6c8 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -43,6 +43,9 @@ en: last_updated: "Last updated %{date}" published: "Published %{date}" see_all_updates: "see all updates" + supergroups: + services: "Services" + see_more_supergroups: "See more %{supergroup}" language_names: ar: Arabic de: German diff --git a/test/controllers/content_items_controller_test.rb b/test/controllers/content_items_controller_test.rb index 1d49783ab..a6481479d 100644 --- a/test/controllers/content_items_controller_test.rb +++ b/test/controllers/content_items_controller_test.rb @@ -78,6 +78,8 @@ class ContentItemsControllerTest < ActionController::TestCase stub_request(:get, %r{#{invalid_part_path}}).to_return(status: 200, body: content_item.to_json, headers: {}) + @controller.stubs(:page_in_scope?).returns(false) + get :show, params: { path: invalid_part_path } assert_response :redirect diff --git a/test/controllers/step_navigation_controller_test.rb b/test/controllers/step_navigation_controller_test.rb index 0006873b5..295b4fab0 100644 --- a/test/controllers/step_navigation_controller_test.rb +++ b/test/controllers/step_navigation_controller_test.rb @@ -11,6 +11,8 @@ class ContentItemsControllerTest < ActionController::TestCase content_store_has_item(content_item['base_path'], content_item) + @controller.stubs(:page_in_scope?).returns(false) + get :show, params: { path: path } assert_response 200 @@ -24,6 +26,8 @@ class ContentItemsControllerTest < ActionController::TestCase content_store_has_item(content_item['base_path'], content_item) + @controller.stubs(:page_in_scope?).returns(false) + get :show, params: { path: path } assert_response 200 From 0818dcd24f423184abce301a09b2cf8b4eea4f2c Mon Sep 17 00:00:00 2001 From: Simon Hughesdon Date: Thu, 19 Jul 2018 14:55:21 +0100 Subject: [PATCH 5/7] Add initial testing for content pages navigation tests This checks that the ab test works for content which is tagged to a single taxon. --- .../concerns/content_pages_nav_ab_testable.rb | 2 +- .../content_pages_nav_ab_testable_test.rb | 20 +++++++++++-------- test/test_helper.rb | 10 +++++++++- 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/app/controllers/concerns/content_pages_nav_ab_testable.rb b/app/controllers/concerns/content_pages_nav_ab_testable.rb index 715b86327..3b6f26d27 100644 --- a/app/controllers/concerns/content_pages_nav_ab_testable.rb +++ b/app/controllers/concerns/content_pages_nav_ab_testable.rb @@ -28,7 +28,7 @@ def set_test_response_header end def show_new_navigation? - !content_pages_nav_test_variant.variant?("B") && page_in_scope? + content_pages_nav_test_variant.variant?("B") && page_in_scope? end def page_in_scope? diff --git a/test/controllers/content_pages_nav_ab_testable_test.rb b/test/controllers/content_pages_nav_ab_testable_test.rb index 2a6c4d8c6..552a456f7 100644 --- a/test/controllers/content_pages_nav_ab_testable_test.rb +++ b/test/controllers/content_pages_nav_ab_testable_test.rb @@ -1,9 +1,15 @@ require 'test_helper' class ContentItemsControllerTest < ActionController::TestCase + include ContentPagesNavTestHelper include GdsApi::TestHelpers::ContentStore + include GdsApi::TestHelpers::Rummager include GovukAbTesting::MinitestHelpers + setup do + stub_rummager + end + %w(A B).each do |test_variant| test "ContentPagesNav variant #{test_variant} works with a valid taxon" do content_item = content_store_has_schema_example("guide", "single-page-guide").tap do |item| @@ -15,7 +21,6 @@ class ContentItemsControllerTest < ActionController::TestCase test "ContentPagesNav variant #{test_variant} works without a valid taxon" do content_item = content_store_has_schema_example("guide", "single-page-guide") - content_store_has_item(content_item['base_path'], content_item) @controller.stubs(:page_in_scope?).returns(true) ensure_ab_test_is_correctly_setup(test_variant, content_item) @@ -52,14 +57,13 @@ def ensure_ab_test_is_correctly_setup(test_variant, content_item) @controller.stubs(:page_in_scope?).returns(true) - with_variant ContentPagesNav: test_variant do - get :show, params: { path: path_for(content_item) } - assert_response 200 + with_variant ContentPagesNav: test_variant do + get :show, params: { path: path_for(content_item) } + assert_response 200 - ab_test = GovukAbTesting::AbTest.new("ContentPagesNav", dimension: 65) - requested = ab_test.requested_variant(request.headers) - assert requested.variant?(test_variant) - end + ab_test = GovukAbTesting::AbTest.new("ContentPagesNav", dimension: 65) + requested = ab_test.requested_variant(request.headers) + assert requested.variant?(test_variant) end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 9315ca672..ceffdf730 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -131,7 +131,15 @@ def assert_footer_has_published_dates(published = nil, last_updated = nil, histo def setup_and_visit_content_item(name, parameter_string = '') @content_item = get_content_example(name).tap do |item| content_store_has_item(item["base_path"], item.to_json) - visit("#{item['base_path']}#{parameter_string}") + visit_with_cachebust("#{item['base_path']}#{parameter_string}") + end + end + + def setup_and_visit_content_item_with_taxons(name, taxons) + @content_item = get_content_example(name).tap do |item| + item["links"]["taxons"] = taxons + content_store_has_item(item["base_path"], item.to_json) + visit_with_cachebust(item['base_path']) end end From 55a81c8a15c54b725ddd0269bdff4d3045657a7c Mon Sep 17 00:00:00 2001 From: Simon Hughesdon Date: Fri, 20 Jul 2018 15:42:12 +0100 Subject: [PATCH 6/7] Allow multiple taxons Where content is tagged to more than one taxon, get the tagged content for all those taxons. Note that this doesn't put any limit on the number of taxons allowed. We may want to set an arbitrary limit on this just to be on the safe side. --- app/controllers/content_items_controller.rb | 22 +++++------ app/presenters/supergroups/services.rb | 6 +-- app/services/most_popular_content.rb | 12 +++--- app/views/layouts/application.html.erb | 2 +- .../shared/_taxonomy_navigation.html.erb | 4 +- test/services/most_popular_content_test.rb | 37 +++++++++++++------ 6 files changed, 49 insertions(+), 34 deletions(-) diff --git a/app/controllers/content_items_controller.rb b/app/controllers/content_items_controller.rb index 314dc2179..f2fc94dea 100644 --- a/app/controllers/content_items_controller.rb +++ b/app/controllers/content_items_controller.rb @@ -64,23 +64,23 @@ def load_content_item end def load_taxonomy_navigation - # For now, just get the first tagged taxon. - # TODO: handle content tagged to multiple taxons - if @content_item.taxons - taxon = @content_item.taxons.first - taxon_id = taxon["content_id"] + if @content_item.taxons.present? + taxons = @content_item.taxons - services = Supergroups::Services.new(taxon_id) + taxon_ids = taxons.map { |taxon| taxon["content_id"] } + services = Supergroups::Services.new(taxon_ids) @taxonomy_navigation = { services: services.tagged_content, } - @tagged_taxon = { - taxon_id: taxon_id, - taxon_name: taxon["title"], - taxon_link: taxon["base_path"], - } + @tagged_taxons = taxons.map do |taxon| + { + taxon_id: taxon["content_id"], + taxon_name: taxon["title"], + taxon_link: taxon["base_path"], + } + end end end diff --git a/app/presenters/supergroups/services.rb b/app/presenters/supergroups/services.rb index 7d534ff3b..456c3cdaa 100644 --- a/app/presenters/supergroups/services.rb +++ b/app/presenters/supergroups/services.rb @@ -2,12 +2,12 @@ module Supergroups class Services attr_reader :content - def initialize(taxon_id) - @taxon_id = taxon_id + def initialize(taxon_ids) + @taxon_ids = taxon_ids end def tagged_content - @content = MostPopularContent.fetch(content_id: @taxon_id, filter_content_purpose_supergroup: "services") + @content = MostPopularContent.fetch(content_ids: @taxon_ids, filter_content_purpose_supergroup: "services") format_document_data(@content) end diff --git a/app/services/most_popular_content.rb b/app/services/most_popular_content.rb index a3a53f883..f8fa1bd3c 100644 --- a/app/services/most_popular_content.rb +++ b/app/services/most_popular_content.rb @@ -3,16 +3,16 @@ class MostPopularContent include RummagerFields - attr_reader :content_id, :filter_content_purpose_supergroup, :number_of_links + attr_reader :content_ids, :filter_content_purpose_supergroup, :number_of_links - def initialize(content_id:, filter_content_purpose_supergroup:, number_of_links: 5) - @content_id = content_id + def initialize(content_ids:, filter_content_purpose_supergroup:, number_of_links: 5) + @content_ids = content_ids @filter_content_purpose_supergroup = filter_content_purpose_supergroup @number_of_links = number_of_links end - def self.fetch(content_id:, filter_content_purpose_supergroup:) - new(content_id: content_id, filter_content_purpose_supergroup: filter_content_purpose_supergroup).fetch + def self.fetch(content_ids:, filter_content_purpose_supergroup:) + new(content_ids: content_ids, filter_content_purpose_supergroup: filter_content_purpose_supergroup).fetch end def fetch @@ -26,7 +26,7 @@ def search_response start: 0, count: number_of_links, fields: RummagerFields::TAXON_SEARCH_FIELDS, - filter_part_of_taxonomy_tree: Array(content_id), + filter_part_of_taxonomy_tree: content_ids, order: '-popularity', } params[:filter_content_purpose_supergroup] = filter_content_purpose_supergroup if filter_content_purpose_supergroup.present? diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 1e1f45cf2..1f23ab34e 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -45,7 +45,7 @@
<%= yield %> <% if show_new_navigation? && @taxonomy_navigation %> - <%= render 'shared/taxonomy_navigation', taxonomy_navigation: @taxonomy_navigation, tagged_taxon: @tagged_taxon %> + <%= render 'shared/taxonomy_navigation', taxonomy_navigation: @taxonomy_navigation, tagged_taxons: @tagged_taxons %> <% end %>
<%= render 'govuk_publishing_components/components/feedback' %> diff --git a/app/views/shared/_taxonomy_navigation.html.erb b/app/views/shared/_taxonomy_navigation.html.erb index 7ccdb7861..efd49e1fe 100644 --- a/app/views/shared/_taxonomy_navigation.html.erb +++ b/app/views/shared/_taxonomy_navigation.html.erb @@ -1,7 +1,9 @@ <% if taxonomy_navigation.values().flatten.any? %>

More in - <%= tagged_taxon[:taxon_name] %> + <% tagged_taxons.each do |tagged_taxon| %> + <%= tagged_taxon[:taxon_name] %> + <% end %>

<% if taxonomy_navigation[:services].any? %> diff --git a/test/services/most_popular_content_test.rb b/test/services/most_popular_content_test.rb index c7409d4f1..e251f9876 100644 --- a/test/services/most_popular_content_test.rb +++ b/test/services/most_popular_content_test.rb @@ -2,27 +2,30 @@ class MostPopularContentTest < ActiveSupport::TestCase include RummagerFields + include GdsApi::TestHelpers::Rummager def most_popular_content @most_popular_content ||= MostPopularContent.new( - content_id: taxon_content_id, + content_ids: taxon_content_ids, filter_content_purpose_supergroup: 'guidance_and_regulation' ) end - def taxon_content_id - 'c3c860fc-a271-4114-b512-1c48c0f82564' + def taxon_content_ids + ['something-id-like', 'some-other-id'] end test 'returns the results from search' do search_results = { - 'results' => [ - { 'title' => 'Doc 1' }, - { 'title' => 'A Doc 2' }, - ] + body: { + 'results' => [ + { 'title' => 'Doc 1' }, + { 'title' => 'A Doc 2' }, + ] + }.to_json } - Services.rummager.stubs(:search).returns(search_results) + stub_any_rummager_search.to_return(search_results) results = most_popular_content.fetch assert_equal(results.count, 2) @@ -55,7 +58,7 @@ def taxon_content_id end test 'scopes the results to the current taxon' do - assert_includes_params(filter_part_of_taxonomy_tree: Array(taxon_content_id)) do + assert_includes_params(filter_part_of_taxonomy_tree: taxon_content_ids) do most_popular_content.fetch end end @@ -81,14 +84,24 @@ def assert_includes_params(expected_params) Services. rummager. stubs(:search). - with { |params| params.including?(expected_params) }. + with { |params| assert_includes_subhash(expected_params, params) }. returns(search_results) results = yield assert_equal(results.count, 2) - assert_equal(results.first.title, 'Doc 1') - assert_equal(results.last.title, 'Doc 2') + assert_equal(results.first['title'], 'Doc 1') + assert_equal(results.last['title'], 'Doc 2') + end + + def assert_includes_subhash(expected_sub_hash, hash) + expected_sub_hash.each do |key, value| + assert_equal( + value, + hash[key], + "Expected #{hash} to include #{key} => #{value}" + ) + end end end From 73d1fe0d907659bdba22b1dd175ef16c4d2a4134 Mon Sep 17 00:00:00 2001 From: Simon Hughesdon Date: Fri, 20 Jul 2018 17:02:48 +0100 Subject: [PATCH 7/7] Ensure we only use live taxons --- app/controllers/content_items_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/content_items_controller.rb b/app/controllers/content_items_controller.rb index f2fc94dea..357812a23 100644 --- a/app/controllers/content_items_controller.rb +++ b/app/controllers/content_items_controller.rb @@ -65,7 +65,7 @@ def load_content_item def load_taxonomy_navigation if @content_item.taxons.present? - taxons = @content_item.taxons + taxons = @content_item.taxons.select { |taxon| taxon["phase"] == "live" } taxon_ids = taxons.map { |taxon| taxon["content_id"] } services = Supergroups::Services.new(taxon_ids)