From 28aac21fc25a76191fee4795045f0574deb63132 Mon Sep 17 00:00:00 2001 From: Karl Baker Date: Wed, 23 Jan 2019 15:51:02 +0000 Subject: [PATCH 1/4] Swap suggested_ordered_related_links for related_ordered_links when they exist and when running the B variant of the test --- app/controllers/content_items_controller.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/controllers/content_items_controller.rb b/app/controllers/content_items_controller.rb index a3d0eb42d..ac400bf56 100644 --- a/app/controllers/content_items_controller.rb +++ b/app/controllers/content_items_controller.rb @@ -56,6 +56,11 @@ def set_guide_draft_access_token def load_content_item content_item = Services.content_store.content_item(content_item_path) + + if related_links_variant.variant?('B') && content_item["links"]["suggested_ordered_related_items"] + content_item["links"]["ordered_related_items"] = content_item["links"]["suggested_ordered_related_items"] + end + @content_item = PresenterBuilder.new(content_item, content_item_path).presenter end From 6913f9760af486e007eac15d5007874064c181e4 Mon Sep 17 00:00:00 2001 From: Karl Baker Date: Thu, 24 Jan 2019 13:38:05 +0000 Subject: [PATCH 2/4] Update for new test and add tests --- app/controllers/application_controller.rb | 2 +- .../{aa_testable.rb => ab_testable.rb} | 4 ++-- app/controllers/content_items_controller.rb | 4 ++-- .../content_items_controller_test.rb | 21 +++++++++++++++++++ .../development_controller_test.rb | 4 ++-- 5 files changed, 28 insertions(+), 7 deletions(-) rename app/controllers/concerns/{aa_testable.rb => ab_testable.rb} (92%) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 1b5eb623f..ef20b6cb4 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -3,7 +3,7 @@ class ApplicationController < ActionController::Base # For APIs, you may want to use :null_session instead. protect_from_forgery except: :service_sign_in_options - include AATestable + include ABTestable private diff --git a/app/controllers/concerns/aa_testable.rb b/app/controllers/concerns/ab_testable.rb similarity index 92% rename from app/controllers/concerns/aa_testable.rb rename to app/controllers/concerns/ab_testable.rb index 032b17b92..d5695c2ef 100644 --- a/app/controllers/concerns/aa_testable.rb +++ b/app/controllers/concerns/ab_testable.rb @@ -1,4 +1,4 @@ -module AATestable +module ABTestable RELATED_LINKS_DIMENSION = 65 def self.included(base) @@ -16,7 +16,7 @@ def related_links_variant def related_links_test @related_links_test ||= GovukAbTesting::AbTest.new( - "RelatedLinksAATest", + "RelatedLinksABTest", dimension: RELATED_LINKS_DIMENSION, allowed_variants: %w(A B), control_variant: "A" diff --git a/app/controllers/content_items_controller.rb b/app/controllers/content_items_controller.rb index ac400bf56..063f14e09 100644 --- a/app/controllers/content_items_controller.rb +++ b/app/controllers/content_items_controller.rb @@ -57,8 +57,8 @@ def set_guide_draft_access_token def load_content_item content_item = Services.content_store.content_item(content_item_path) - if related_links_variant.variant?('B') && content_item["links"]["suggested_ordered_related_items"] - content_item["links"]["ordered_related_items"] = content_item["links"]["suggested_ordered_related_items"] + if related_links_variant.variant?('B') && content_item.dig('links', 'suggested_ordered_related_items') + content_item['links']['ordered_related_items'] = content_item['links']['suggested_ordered_related_items'] end @content_item = PresenterBuilder.new(content_item, content_item_path).presenter diff --git a/test/controllers/content_items_controller_test.rb b/test/controllers/content_items_controller_test.rb index a6481479d..89bb0fc6f 100644 --- a/test/controllers/content_items_controller_test.rb +++ b/test/controllers/content_items_controller_test.rb @@ -3,6 +3,7 @@ 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( @@ -129,6 +130,26 @@ class ContentItemsControllerTest < ActionController::TestCase assert_equal content_item['title'], assigns[:content_item].title end + test "gets item from the content store and keeps ordered_related_items when running RelatedLinksABTest control variant" do + with_variant RelatedLinksABTest: 'A' do + content_item = content_store_has_schema_example('case_study', 'case_study') + + get :show, params: { path: path_for(content_item) } + assert_response :success + assert_equal content_item['links']['ordered_related_items'], assigns[:content_item].content_item['links']['ordered_related_items'] + end + end + + test "gets item from the content store and replaces ordered_related_items when running RelatedLinksABTest test variant" do + with_variant RelatedLinksABTest: 'B' do + content_item = content_store_has_schema_example('case_study', 'case_study') + + get :show, params: { path: path_for(content_item) } + assert_response :success + assert_equal assigns[:content_item].content_item['links']['ordered_related_items'], assigns[:content_item].content_item['links']['suggested_ordered_related_items'] + end + end + test "sets the expiry as sent by content-store" do content_item = content_store_has_schema_example('coming_soon', 'coming_soon') content_store_has_item(content_item['base_path'], content_item, max_age: 20) diff --git a/test/controllers/development_controller_test.rb b/test/controllers/development_controller_test.rb index 5bc0164be..f2efec3b5 100644 --- a/test/controllers/development_controller_test.rb +++ b/test/controllers/development_controller_test.rb @@ -4,8 +4,8 @@ class DevelopmentControllerTest < ActionController::TestCase include GovukAbTesting::MinitestHelpers %w(A B).each do |test_variant| - test "RelatedLinksAATest works correctly for each variant (variant: #{test_variant})" do - with_variant RelatedLinksAATest: test_variant do + test "RelatedLinksABTest works correctly for each variant (variant: #{test_variant})" do + with_variant RelatedLinksABTest: test_variant do get :index ab_test = @controller.send(:related_links_test) From e600c6bc691ad04d1bdbc965e732ca7e359deb30 Mon Sep 17 00:00:00 2001 From: Karl Baker Date: Wed, 6 Feb 2019 16:44:54 +0000 Subject: [PATCH 3/4] Ensure B variant of related links shows no related links if suggested related links do not exist. Previously, a user on the B variant would have seen related links of the control variant if the test variant contained no related links, as `ordered_related_items` would not have been overridden on the content item. This commit fixes that problem, to ensure a fair test. This commit also renames `RelatedLinksABTest` to `RelatedLinksABTest1` in anticipation of multiple test iterations. Solo: @karlbaker02 --- app/controllers/concerns/ab_testable.rb | 2 +- app/controllers/content_items_controller.rb | 4 ++-- .../content_items_controller_test.rb | 18 ++++++++++++++---- .../controllers/development_controller_test.rb | 4 ++-- 4 files changed, 19 insertions(+), 9 deletions(-) diff --git a/app/controllers/concerns/ab_testable.rb b/app/controllers/concerns/ab_testable.rb index d5695c2ef..df3964fd9 100644 --- a/app/controllers/concerns/ab_testable.rb +++ b/app/controllers/concerns/ab_testable.rb @@ -16,7 +16,7 @@ def related_links_variant def related_links_test @related_links_test ||= GovukAbTesting::AbTest.new( - "RelatedLinksABTest", + "RelatedLinksABTest1", dimension: RELATED_LINKS_DIMENSION, allowed_variants: %w(A B), control_variant: "A" diff --git a/app/controllers/content_items_controller.rb b/app/controllers/content_items_controller.rb index 063f14e09..114ef2ef8 100644 --- a/app/controllers/content_items_controller.rb +++ b/app/controllers/content_items_controller.rb @@ -57,8 +57,8 @@ def set_guide_draft_access_token def load_content_item content_item = Services.content_store.content_item(content_item_path) - if related_links_variant.variant?('B') && content_item.dig('links', 'suggested_ordered_related_items') - content_item['links']['ordered_related_items'] = content_item['links']['suggested_ordered_related_items'] + if related_links_variant.variant?('B') + content_item['links']['ordered_related_items'] = content_item['links'].fetch('suggested_ordered_related_items', []) end @content_item = PresenterBuilder.new(content_item, content_item_path).presenter diff --git a/test/controllers/content_items_controller_test.rb b/test/controllers/content_items_controller_test.rb index 89bb0fc6f..bdbaf990d 100644 --- a/test/controllers/content_items_controller_test.rb +++ b/test/controllers/content_items_controller_test.rb @@ -130,8 +130,8 @@ class ContentItemsControllerTest < ActionController::TestCase assert_equal content_item['title'], assigns[:content_item].title end - test "gets item from the content store and keeps ordered_related_items when running RelatedLinksABTest control variant" do - with_variant RelatedLinksABTest: 'A' do + test "gets item from the content store and keeps ordered_related_items when running RelatedLinksABTest1 control variant" do + with_variant RelatedLinksABTest1: 'A' do content_item = content_store_has_schema_example('case_study', 'case_study') get :show, params: { path: path_for(content_item) } @@ -140,8 +140,8 @@ class ContentItemsControllerTest < ActionController::TestCase end end - test "gets item from the content store and replaces ordered_related_items when running RelatedLinksABTest test variant" do - with_variant RelatedLinksABTest: 'B' do + test "gets item from the content store and replaces ordered_related_items when running RelatedLinksABTest1 test variant" do + with_variant RelatedLinksABTest1: 'B' do content_item = content_store_has_schema_example('case_study', 'case_study') get :show, params: { path: path_for(content_item) } @@ -150,6 +150,16 @@ class ContentItemsControllerTest < ActionController::TestCase end end + test "gets item from the content store and replaces ordered_related_items when empty array when RelatedLinksABTest1 test variant has no suggestions" do + with_variant RelatedLinksABTest1: 'B' do + content_item = content_store_has_schema_example('guide', 'guide') + + get :show, params: { path: path_for(content_item) } + assert_response :success + assert_equal [], assigns[:content_item].content_item['links']['ordered_related_items'] + end + end + test "sets the expiry as sent by content-store" do content_item = content_store_has_schema_example('coming_soon', 'coming_soon') content_store_has_item(content_item['base_path'], content_item, max_age: 20) diff --git a/test/controllers/development_controller_test.rb b/test/controllers/development_controller_test.rb index f2efec3b5..2008594cf 100644 --- a/test/controllers/development_controller_test.rb +++ b/test/controllers/development_controller_test.rb @@ -4,8 +4,8 @@ class DevelopmentControllerTest < ActionController::TestCase include GovukAbTesting::MinitestHelpers %w(A B).each do |test_variant| - test "RelatedLinksABTest works correctly for each variant (variant: #{test_variant})" do - with_variant RelatedLinksABTest: test_variant do + test "RelatedLinksABTest1 works correctly for each variant (variant: #{test_variant})" do + with_variant RelatedLinksABTest1: test_variant do get :index ab_test = @controller.send(:related_links_test) From 876edc4ad5aaf2b377f092eaa7eef04a693c6ab1 Mon Sep 17 00:00:00 2001 From: Karl Baker Date: Thu, 7 Feb 2019 11:57:22 +0000 Subject: [PATCH 4/4] Ensure content item has links --- 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 114ef2ef8..0c74bb87f 100644 --- a/app/controllers/content_items_controller.rb +++ b/app/controllers/content_items_controller.rb @@ -57,7 +57,7 @@ def set_guide_draft_access_token def load_content_item content_item = Services.content_store.content_item(content_item_path) - if related_links_variant.variant?('B') + if related_links_variant.variant?('B') && content_item.dig('links') content_item['links']['ordered_related_items'] = content_item['links'].fetch('suggested_ordered_related_items', []) end