From b395d05c872890efa7f09f4a87d55bb8e2ee3d6d Mon Sep 17 00:00:00 2001 From: Tijmen Brommet Date: Wed, 28 Mar 2018 15:37:40 +0100 Subject: [PATCH] Always use the non-numbered contents list This removes the GuideChapterNav A/B test. We've tested this and we'll keep the B version. Except for travel advice, which keeps its navigation. https://trello.com/c/ndUiAA1y --- .../concerns/guide_nav_ab_testable.rb | 68 ---------------- app/controllers/content_items_controller.rb | 2 - app/presenters/guide_presenter.rb | 4 - app/views/content_items/guide.html.erb | 8 +- .../content_items/travel_advice.html.erb | 20 ++++- app/views/layouts/application.html.erb | 1 - app/views/shared/_parts_navigation.html.erb | 20 ----- .../content_items_controller_test.rb | 1 - ...ide_chapter_nav_ab_test_controller_test.rb | 81 ------------------- .../step_navigation_controller_test.rb | 1 - test/integration/guide_test.rb | 27 +------ test/presenters/guide_presenter_test.rb | 5 -- 12 files changed, 23 insertions(+), 215 deletions(-) delete mode 100644 app/controllers/concerns/guide_nav_ab_testable.rb delete mode 100644 app/views/shared/_parts_navigation.html.erb delete mode 100644 test/controllers/guide_chapter_nav_ab_test_controller_test.rb diff --git a/app/controllers/concerns/guide_nav_ab_testable.rb b/app/controllers/concerns/guide_nav_ab_testable.rb deleted file mode 100644 index 758428bf9..000000000 --- a/app/controllers/concerns/guide_nav_ab_testable.rb +++ /dev/null @@ -1,68 +0,0 @@ -module GuideNavAbTestable - DIMENSION = 64 - TEST_NAME = "GuideChapterNav".freeze - - def self.included(base) - base.helper_method( - :guide_test_variant, - :is_tested_guide?, - :show_alternate_guide_chapters? - ) - - base.after_action :set_guide_test_response_header - end - - def guide_test - @guide_nav_test ||= set_ab_test( - name: TEST_NAME, - dimension: DIMENSION - ) - end - - def guide_test_variant - @guide_test_variant ||= - guide_test.requested_variant(request.headers) - end - - def show_alternate_guide_chapters? - guide_test_variant.variant?('B') && is_tested_guide? - end - - def is_tested_guide? - is_guide? && !is_education? - end - - def set_guide_test_response_header - guide_test_variant.configure_response(response) if is_tested_guide? - end - -private - - def is_guide? - content_item.is_a?(GuidePresenter) - end - - def is_education? - @is_education ||= root_taxon_slugs(content_item.content_item).include?("/education") - end - - def root_taxon_slugs(content_item) - root_taxon_set = Set.new - - links = content_item["links"] - - parent_taxons = (links["parent_taxons"] || links["taxons"]) unless links.nil? - if parent_taxons.blank? - root_taxon_set << content_item["base_path"] if content_item["document_type"] == 'taxon' - else - parent_taxons.each do |parent_taxon| - root_taxon_set += root_taxon_slugs(parent_taxon) - end - end - root_taxon_set - end - - def set_ab_test(name:, dimension:) - GovukAbTesting::AbTest.new(name, dimension: dimension) - end -end diff --git a/app/controllers/content_items_controller.rb b/app/controllers/content_items_controller.rb index 04e7219b6..f328aeee8 100644 --- a/app/controllers/content_items_controller.rb +++ b/app/controllers/content_items_controller.rb @@ -1,6 +1,4 @@ class ContentItemsController < ApplicationController - include GuideNavAbTestable - rescue_from GdsApi::HTTPForbidden, with: :error_403 rescue_from GdsApi::HTTPNotFound, with: :error_notfound rescue_from GdsApi::HTTPGone, with: :error_410 diff --git a/app/presenters/guide_presenter.rb b/app/presenters/guide_presenter.rb index 9b008bbdb..17bddd81c 100644 --- a/app/presenters/guide_presenter.rb +++ b/app/presenters/guide_presenter.rb @@ -26,10 +26,6 @@ def multi_page_guide? parts.size > 1 end - def current_part_title_with_index - "#{current_part_index + 1}. #{current_part_title}" - end - def print_link "#{base_path}/print" end diff --git a/app/views/content_items/guide.html.erb b/app/views/content_items/guide.html.erb index 4d59142c0..70f6db782 100644 --- a/app/views/content_items/guide.html.erb +++ b/app/views/content_items/guide.html.erb @@ -4,7 +4,7 @@ <%= render 'govuk_component/title', { title: @content_item.title } %> <% if @content_item.multi_page_guide? %> <% end %> @@ -12,11 +12,7 @@ <% if @content_item.has_parts? %> <% if @content_item.multi_page_guide? %>

- <% if show_alternate_guide_chapters? %> - <%= @content_item.current_part_title %> - <% else %> - <%= @content_item.current_part_title_with_index %> - <% end %> + <%= @content_item.current_part_title %>

<% end %> <%= render 'govuk_component/govspeak', diff --git a/app/views/content_items/travel_advice.html.erb b/app/views/content_items/travel_advice.html.erb index 2149cdfb8..862f349c6 100644 --- a/app/views/content_items/travel_advice.html.erb +++ b/app/views/content_items/travel_advice.html.erb @@ -8,7 +8,25 @@
<%= render 'govuk_component/title', @content_item.title_and_context %>
diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 80102227f..0df1defac 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -24,7 +24,6 @@ <% end %> - <%= guide_test_variant.analytics_meta_tag.html_safe if is_tested_guide? %> <%= yield :extra_head_content %> diff --git a/app/views/shared/_parts_navigation.html.erb b/app/views/shared/_parts_navigation.html.erb deleted file mode 100644 index b05415cb3..000000000 --- a/app/views/shared/_parts_navigation.html.erb +++ /dev/null @@ -1,20 +0,0 @@ -<% if show_alternate_guide_chapters? %> - <%= render "components/contents-list", aria_label: navigation_label, contents: content_item.part_link_elements %> -<% else %> - -<% end %> diff --git a/test/controllers/content_items_controller_test.rb b/test/controllers/content_items_controller_test.rb index efc3dd7c7..ccc5e6cab 100644 --- a/test/controllers/content_items_controller_test.rb +++ b/test/controllers/content_items_controller_test.rb @@ -3,7 +3,6 @@ class ContentItemsControllerTest < ActionController::TestCase include GdsApi::TestHelpers::ContentStore include GdsApi::TestHelpers::Rummager - include GovukAbTesting::MinitestHelpers test 'routing handles paths with no format or locale' do assert_routing( diff --git a/test/controllers/guide_chapter_nav_ab_test_controller_test.rb b/test/controllers/guide_chapter_nav_ab_test_controller_test.rb deleted file mode 100644 index 962c3219c..000000000 --- a/test/controllers/guide_chapter_nav_ab_test_controller_test.rb +++ /dev/null @@ -1,81 +0,0 @@ -require 'test_helper' - -class ContentItemsControllerTest < ActionController::TestCase - include GdsApi::TestHelpers::ContentStore - include GovukAbTesting::MinitestHelpers - - %w(A B).each do |test_variant| - test "GuideChapterNav variant #{test_variant} works for guides not in the education taxon" do - #this sample guide is not part of the education taxon - content_item = content_store_has_schema_example("guide", "single-page-guide") - content_store_has_item(content_item['base_path'], content_item) - - with_variant GuideChapterNav: test_variant do - get :show, params: { path: path_for(content_item) } - assert_response 200 - - ab_test = GovukAbTesting::AbTest.new("GuideChapterNav", dimension: 64) - requested = ab_test.requested_variant(request.headers) - assert requested.variant?(test_variant) - end - end - - test "GuideChapterNav variant #{test_variant} does not affect travel advice content" do - content_item = content_store_has_schema_example("travel_advice", "full-country") - content_store_has_item(content_item['base_path'], content_item) - - setup_ab_variant("GuideChapterNav", test_variant) - - get :show, params: { path: path_for(content_item) } - assert_response 200 - assert_response_not_modified_for_ab_test("GuideChapterNav") - end - end - - test "GuideChapterNav shows the original nav on variant A" do - # The example schema is in the education taxon which would remove it from - # the test, so we'll stub it here - @controller.stubs(:is_education?).returns(false) - - content_item = content_store_has_schema_example("guide", "guide") - content_store_has_item(content_item['base_path'], content_item) - - with_variant GuideChapterNav: "A" do - get :show, params: { path: path_for(content_item) } - assert_response 200 - - #original navigation section - assert_match("part-navigation", response.body) - - #parts are still numbers - assert_match("1. Overview", response.body) - end - end - - test "GuideChapterNav shows the alternate nav on variant B" do - # The example schema is in the education taxon which would remove it from - # the test, so we'll stub it here - @controller.stubs(:is_education?).returns(false) - - content_item = content_store_has_schema_example("guide", "guide") - content_store_has_item(content_item['base_path'], content_item) - - with_variant GuideChapterNav: "B" do - get :show, params: { path: path_for(content_item) } - assert_response 200 - - #alternate navigation component - assert_match("app-c-contents-list", response.body) - - #remove numbering from parts - refute_match("1. Overview", response.body) - assert_match("Overview", response.body) - 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/controllers/step_navigation_controller_test.rb b/test/controllers/step_navigation_controller_test.rb index 179d10610..0006873b5 100644 --- a/test/controllers/step_navigation_controller_test.rb +++ b/test/controllers/step_navigation_controller_test.rb @@ -2,7 +2,6 @@ class ContentItemsControllerTest < ActionController::TestCase include GdsApi::TestHelpers::ContentStore - include GovukAbTesting::MinitestHelpers %w(guide answer publication).each do |schema_name| test "#{schema_name} shows step by step navigation where relevant" do diff --git a/test/integration/guide_test.rb b/test/integration/guide_test.rb index 432e0c738..37348cc93 100644 --- a/test/integration/guide_test.rb +++ b/test/integration/guide_test.rb @@ -11,44 +11,21 @@ class GuideTest < ActionDispatch::IntegrationTest assert page.has_css?("title", visible: false, text: @content_item['title']) assert_has_component_title(@content_item['title']) - assert page.has_css?('.part-navigation ol', count: 2) - assert page.has_css?('.part-navigation li', count: @content_item['details']['parts'].size) - - @content_item["details"]["parts"].each_with_index do |part, i| - if i.zero? - assert page.has_css?('.part-navigation li', text: part['title']) - refute page.has_css?('.part-navigation li a', text: part['title']) - else - assert page.has_css?(".part-navigation li a[href*=\"#{part['slug']}\"]", text: part['title']) - end - end - - assert page.has_css?('h1', text: "1. #{@content_item['details']['parts'].first['title']}") + assert page.has_css?('h1', text: @content_item['details']['parts'].first['title']) assert page.has_css?(shared_component_selector("previous_and_next_navigation")) assert page.has_css?('.app-c-print-link a[href$="/print"]') - - assert page.has_css?(".part-navigation[data-module='track-click']") - assert_tracking_link("category", "contentsClicked", (@content_item['details']['parts'].size - 1)) - assert_tracking_link("action", "content_item 2") - assert_tracking_link("label", "/national-curriculum/key-stage-1-and-2") - assert_tracking_link("options", { dimension29: "Key stage 1 and 2" }.to_json) end test "draft access tokens are appended to part links within navigation" do setup_and_visit_content_item('guide', '?token=some_token') - assert page.has_css?('.part-navigation a[href$="?token=some_token"]') + assert page.has_css?('.app-c-contents-list a[href$="?token=some_token"]') end test "does not show part navigation, print link or part title when only one part" do setup_and_visit_content_item('single-page-guide') refute page.has_css?('h1', text: @content_item['details']['parts'].first['title']) - refute page.has_css?('.part-navigation') refute page.has_css?('.app-c-print-link') end - - def assert_tracking_link(name, value, total = 1) - assert page.has_css?("a[data-track-#{name}='#{value}']", count: total) - end end diff --git a/test/presenters/guide_presenter_test.rb b/test/presenters/guide_presenter_test.rb index 26545d073..ce1c1107a 100644 --- a/test/presenters/guide_presenter_test.rb +++ b/test/presenters/guide_presenter_test.rb @@ -17,11 +17,6 @@ def schema_name end end - test "presents part titles with their index" do - first_part_title = schema_item['details']['parts'].first['title'] - assert_equal presented_item.current_part_title_with_index, "1. #{first_part_title}" - end - test 'presents withdrawn in the title for withdrawn content' do presented_item = presented_item(schema_name, nil, "withdrawn_notice" => { "explanation": "Withdrawn", "withdrawn_at": "2014-08-22T10:29:02+01:00" }) assert_equal "[Withdrawn] The national curriculum", presented_item.page_title