From 0f30c1218f306e8d8a967e4a5c137e9e8d2b5bd5 Mon Sep 17 00:00:00 2001 From: Richard Towers Date: Wed, 18 Oct 2023 10:18:19 +0100 Subject: [PATCH] More robust replacement of AB test content As suggested by @hannako, we can make the AB test replacement less fragile by looking for a specific replacement placeholder in the content item, instead of replacing a particular bit of content. I've gone for a [mustache](https://mustache.github.io/) style replacement string of {{ab_test_find_utr_number_video_links}}. There's a possible future thought of doing other kinds of templating, so I think starting to socialise this syntax is a good idea. I've checked in the govspeak preview app, and the `{{...}}` syntax is ignored by govspeak so there should be no issue entering it in the publishing app. I've pulled the replacement strings out into the translation file so that they're easier to review / keep up to date. I've also switched to three A / B / Z variants, so we can do splits like 5% / 5% / 90% where we're measuring the difference between the two 5% samples and ignoring the 90% sample. This test is probably going to be 50% / 50%, so not strictly necessary, but this seems to be the idiomatic way to do it. --- app/controllers/content_items_controller.rb | 27 ++++++++----------- config/locales/en.yml | 5 ++++ .../content_items_controller_test.rb | 25 ++++++++++++++--- 3 files changed, 37 insertions(+), 20 deletions(-) diff --git a/app/controllers/content_items_controller.rb b/app/controllers/content_items_controller.rb index 7d3032b04..06ed6dd2e 100644 --- a/app/controllers/content_items_controller.rb +++ b/app/controllers/content_items_controller.rb @@ -30,26 +30,21 @@ def show ab_test = GovukAbTesting::AbTest.new( "find_utr_number_video_links", dimension: 300, # TODO: which dimension should we use? - allowed_variants: %w[HelpText VideoLink], - control_variant: "HelpText", + allowed_variants: %w[A B Z], + control_variant: "Z", ) @requested_variant = ab_test.requested_variant(request.headers) @requested_variant.configure_response(response) - if @requested_variant.variant? "VideoLink" - # NOTE: this is a fragile way of doing an AB test on content. - # - # Any change to the base content, or to the way the content is rendered - # could potentially break the B variant of the test, and result in both - # variants being the same. - # We're aware of this risk, and we're going to be careful in this one off - # situation. This is not a sustainable way of doing AB tests in the - # future. - @content_item.body.sub!( - /
  • \s*in\ the\s+HMRC<\/abbr>\s+app<\/a>\s*<\/li>/, - '
  • in the HMRC app - watch a video about finding your UTR number in the app
    • ', - ) - end + replacement = case @requested_variant.variant_name + when "A" + I18n.t("ab_tests.find_utr_number_video_links.A") + when "B" + I18n.t("ab_tests.find_utr_number_video_links.B") + else + I18n.t("ab_tests.find_utr_number_video_links.Z") + end + @content_item.body.sub!("{{ab_test_find_utr_number_video_links}}", replacement) end # /TEMPORARY diff --git a/config/locales/en.yml b/config/locales/en.yml index 465ebc43c..6103d76c3 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1,5 +1,10 @@ --- en: + ab_tests: + find_utr_number_video_links: + A: 'in the HMRC app' + B: 'in the HMRC app - watch a video about finding your UTR number in the app' + Z: 'in the HMRC app' call_for_evidence: and: and another_website_html: This call for evidence %{closed} held on another website diff --git a/test/controllers/content_items_controller_test.rb b/test/controllers/content_items_controller_test.rb index 7f7908b20..b138cdd43 100644 --- a/test/controllers/content_items_controller_test.rb +++ b/test/controllers/content_items_controller_test.rb @@ -364,19 +364,36 @@ class ContentItemsControllerTest < ActionController::TestCase assert_equal "true", @response.headers[Slimmer::Headers::REMOVE_SEARCH_HEADER] end - test "AB test replaces content on the find-utr-number page" do + test "AB test replaces content on the find-utr-number page with default" do content_item = content_store_has_schema_example("answer", "answer") content_item["base_path"] = "/find-utr-number" - content_item["details"]["body"] = "
  • in the HMRC app\n
    • " + content_item["details"]["body"] = "
  • {{ab_test_find_utr_number_video_links}}
    • " content_item["locale"] = "en" stub_content_store_has_item(content_item["base_path"], content_item) - request.headers["HTTP_GOVUK_ABTEST_FIND_UTR_NUMBER_VIDEO_LINKS"] = "VideoLink" + request.headers["HTTP_GOVUK_ABTEST_FIND_UTR_NUMBER_VIDEO_LINKS"] = nil get :show, params: { path: path_for(content_item) } assert_response :success - assert_match 'watch a video about finding your UTR number in the app', response.body + assert_no_match "{{ab_test_find_utr_number_video_links}}", response.body + assert_match "
  • #{I18n.t('ab_tests.find_utr_number_video_links.Z')}
    • ", response.body + end + + test "AB test replaces content on the find-utr-number page with variant B" do + content_item = content_store_has_schema_example("answer", "answer") + content_item["base_path"] = "/find-utr-number" + content_item["details"]["body"] = "
  • {{ab_test_find_utr_number_video_links}}
    • " + content_item["locale"] = "en" + + stub_content_store_has_item(content_item["base_path"], content_item) + + request.headers["HTTP_GOVUK_ABTEST_FIND_UTR_NUMBER_VIDEO_LINKS"] = "B" + + get :show, params: { path: path_for(content_item) } + assert_response :success + assert_no_match "{{ab_test_find_utr_number_video_links}}", response.body + assert_match "
  • #{I18n.t('ab_tests.find_utr_number_video_links.B')}
    • ", response.body end def path_for(content_item, locale = nil)