Navigation AB Test #237

alecgibson · 2017-01-24T17:28:13Z

Add functionality to check for A/B Testing, and display content according to which "bucket" the user has been added to.

This WIP PR adds a helper method to the controller to check for A/B bucket (with associated tests).

As a proof-of-concept, it also updates two of the content_item views to show either an A or B view. Currently the only difference between the two is the display of "normal" breadcrumbs (view A) or "taxon" breadcrumbs (view B).

If we continue in this style, we should add A/B views for all Content Items. Is this the best approach? If so, should we be grouping them in subfolders under content_items to reduce clutter? Is there any scope for commonising, or is that a waste of time, given that - if testing is successful - we'd just want to delete all the "A" views anyway in the long run.

NickColley · 2017-01-24T17:31:14Z

If we're only testing the breadcrumbs wouldnt only want to A/B that component itself?

alecgibson · 2017-01-24T17:33:54Z

@NickColley we'll eventually be shuffling actual HTML around. For example, in Detailed Guide, we'll need to make space on the right-hand-side for a Related Links box.

carvil · 2017-01-24T17:34:44Z

We will also be removing "Part of" as well, so this will be a bit more comprehensive than just breadcrumbs.

carvil · 2017-01-24T17:35:16Z

app/controllers/content_items_controller.rb

    with_locale do
      render content_item_template
    end
  end

+  def should_present_b_view?
+    (ab_testing_group == "B") && new_navigation_enabled? && content_is_tagged_to_a_taxon?


Do we have the new chrome extension ready so we can add ourselves to the B group in the A/B test on integration?

Not sure. @suzannehamilton ? At the moment you can manually manipulate your own headers with a Chrome extension.

That's currently on master, we'll release soon.

alecgibson · 2017-01-24T17:35:25Z

Yes, this is just a small change to start with to keep the PR manageable and get early feedback.

carvil · 2017-01-24T17:36:19Z

app/controllers/content_items_controller.rb

+    (ab_testing_group == "B") && new_navigation_enabled? && content_is_tagged_to_a_taxon?
+  end
+
+  helper_method :should_present_b_view?


What do you think of changing this to should_present_new_content_view? It could be hard to understand what the B version is without more context.

carvil · 2017-01-24T17:37:10Z

app/views/content_items/_case_study_a.html.erb

@@ -0,0 +1,9 @@
+<%= content_for :title, "#{@content_item.page_title} - #{t("content_item.format.#{@content_item.document_type}", count: 1)}" %>


Like the comment above, I think we could try to rename these partials to something other than a/b, so it's clearer what they render. What do you think?

What do you propose? Is "New"/"Old" any better than "A"/"B"?

What do you think of:

_case_study.html.erb

for the A version and:

_case_study_for_new_navigation.html.erb

for the B version?

fofr · 2017-01-25T10:31:23Z

app/controllers/content_items_controller.rb

@@ -56,4 +63,16 @@ def error_403(exception)
  def error_notfound
    render plain: 'Not found', status: :not_found
  end
+
+  def ab_testing_group


Could the logic for generic A/B testing be collected in a separate file?
Can we have a further separate place for the logic specific to each test?

Could do, although we'd still need a method here to check if the content is tagged to a taxon. Where's the best place for such a method to live?

It could be included as a module.

fofr · 2017-01-25T10:31:37Z

app/controllers/content_items_controller.rb

+    ENV['ENABLE_NEW_NAVIGATION'] == 'yes'
+  end
+
+  def content_is_tagged_to_a_taxon?


What's a taxon?

It's a new document type that defines the new navigation structure. A taxonomy is a tree of taxons. Content is tagged to these taxon nodes essentially as leaf nodes.

fofr · 2017-01-25T10:33:00Z

app/presenters/content_item_presenter.rb

@@ -31,6 +29,22 @@ def content_id
    content_item.parsed_content["content_id"]
  end

+  def taxons


Breadcrumbs aren't included on every format.
We use a mixin pattern to include features when they're needed.
Should these methods be in a mixin and included on just the formats that need them?

This will prevent the single content_item_presenter from getting too large.

Sure sounds good to me.

Related: #238

fofr · 2017-01-25T10:35:43Z

app/views/content_items/_detailed_guide_b.html.erb

@@ -0,0 +1,54 @@
+<%= content_for :title, @content_item.page_title %>


It's not clear what the differences between a.html.erb and b.html.erb are. Rather than two templates, could the variations be provided by picking different partials? This reduces duplication and makes the template variations clearer.

eg

if should_present_b_view? render "shared/taxon_breadcrumbs else: render "shared/breadcrumbs

I've not dived into it yet, but I think the HTML is going to be moving around. For example, on Detailed Guides, we need to shift the content to the left two thirds to make room for a Related Items box on the right. This would include shifting the contents and some metadata out of the way (presumably to the top of the document?). I'm not sure that just picking different partials will really cut it in this case.

Or the logic could be in the partial itself, if it affects all breadcrumbs on all formats with breadcrumbs.

The reason for not putting the switch in the partial itself is that I didn't want two levels of switching - it's clear coming to Detailed Guide that there's a big switch on the template. It'd be less clear if you came to a single Detailed Guide that the partials have switches buried in them, making it harder still to track down differences between views.

To someone editing a detailed guide or case study, it is not clear that they need to make the change in two places.

Why does it need to be clear that there is a big switch?

I think this way you're making it explicit by saying "This is what the old content looks like", and "This is what the new content looks like."

It's not clear what the differences between a.html.erb and b.html.erb are.

I think this needs to be solved, whichever approach is taken.

@fofr I'm not sure I understand what you're proposing. Surely the only way to be able to have arbitrarily different HTML views is to have two different templates (or variants)?

tijmenb · 2017-01-25T12:31:59Z

Just remembered that Rails has a "variant" feature. Would that work for us here?

http://edgeguides.rubyonrails.org/4_1_release_notes.html#action-pack-variants

fofr · 2017-01-25T14:29:39Z

test/controllers/content_items_controller_test.rb

@@ -75,6 +75,43 @@ class ContentItemsControllerTest < ActionController::TestCase
    assert_response :forbidden
  end

+  test "honours AB Testing cookie" do
+    assert_equal "yes", ENV['ENABLE_NEW_NAVIGATION']


Should the testing of the AB feature be more generic, ie not be tied to this specific test?

What would you propose?

Mock the environment variable and mock the headers?

Then add tests specific to this AB test as well as the generic ones.

I'm not sure what good generic tests would be? The checks for AB testing aren't (and don't need to be?) generic.

Maybe just your test that needs re-phrasing then, "honours AB Testing cookie" sounds generic.

NickColley · 2017-01-25T15:17:29Z

Variants looking good! 👍 Tiny typo case_stude -> case_study

alecgibson · 2017-02-10T12:17:38Z

@carvil this is finally ready for re-review (cc @mobaig )

carvil

🚢

malcolmbaig · 2017-02-10T17:11:20Z

LG 👍

alecgibson · 2017-02-13T10:46:57Z

@carvil I've now bumped the versions of both govuk_navigation_helpers and govuk_ab_testing

carvil · 2017-02-13T10:52:24Z

@alecgibson could you please git cherry-pick b7f62d3 to this PR? That way we can ship everything in one go.

alecgibson · 2017-02-13T11:00:19Z

@carvil done

tijmenb · 2017-02-13T11:56:43Z

🎈

NickColley · 2017-02-13T11:59:34Z

🎉

alecgibson · 2017-10-11T13:30:48Z

Belatedly adding this Trello card for future context: https://trello.com/c/yLGE64pa/1013-changes-to-education-related-content-pages-as-part-of-the-new-navigation-in-government-frontend

boffbowsh temporarily deployed to government-frontend-pr-237 January 24, 2017 17:28 Inactive

carvil reviewed Jan 24, 2017

View reviewed changes

carvil suggested changes Jan 24, 2017

View reviewed changes

fofr reviewed Jan 25, 2017

View reviewed changes

alecgibson force-pushed the navigation-ab-test branch from ebda259 to a528d41 Compare January 25, 2017 11:22

boffbowsh temporarily deployed to government-frontend-pr-237 January 25, 2017 11:23 Inactive

alecgibson force-pushed the navigation-ab-test branch from a528d41 to 63cbdd9 Compare January 25, 2017 11:34

boffbowsh temporarily deployed to government-frontend-pr-237 January 25, 2017 11:34 Inactive

tijmenb mentioned this pull request Jan 25, 2017

Show breadcrumbs consistently #238

Merged

boffbowsh temporarily deployed to government-frontend-pr-237 January 25, 2017 13:06 Inactive

fofr reviewed Jan 25, 2017

View reviewed changes

alecgibson force-pushed the navigation-ab-test branch from f953fc1 to df2baa4 Compare January 25, 2017 14:47

boffbowsh temporarily deployed to government-frontend-pr-237 January 25, 2017 14:47 Inactive

boffbowsh temporarily deployed to government-frontend-pr-237 January 25, 2017 15:00 Inactive

boffbowsh temporarily deployed to government-frontend-pr-237 January 25, 2017 15:05 Inactive

boffbowsh temporarily deployed to government-frontend-pr-237 January 25, 2017 15:12 Inactive

alecgibson force-pushed the navigation-ab-test branch from 416703e to 0275d77 Compare January 25, 2017 15:20

boffbowsh temporarily deployed to government-frontend-pr-237 January 25, 2017 15:20 Inactive

boffbowsh temporarily deployed to government-frontend-pr-237 January 25, 2017 15:31 Inactive

alecgibson force-pushed the navigation-ab-test branch from 118cab3 to c5130a5 Compare January 25, 2017 16:46

boffbowsh temporarily deployed to government-frontend-pr-237 January 25, 2017 16:46 Inactive

alecgibson force-pushed the navigation-ab-test branch from ee4cc5f to 3d48702 Compare February 9, 2017 17:25

boffbowsh temporarily deployed to government-frontend-pr-237 February 9, 2017 17:26 Inactive

malcolmbaig mentioned this pull request Feb 9, 2017

[DO NOT MERGE] Add beta label to pages showing new taxonomy navigation #247

Closed

alecgibson force-pushed the navigation-ab-test branch from 3d48702 to 70dd2b9 Compare February 9, 2017 17:34

boffbowsh temporarily deployed to government-frontend-pr-237 February 9, 2017 17:34 Inactive

alecgibson force-pushed the navigation-ab-test branch from 70dd2b9 to df7d3c2 Compare February 10, 2017 12:06

boffbowsh temporarily deployed to government-frontend-pr-237 February 10, 2017 12:06 Inactive

Display new navigation for Education A/B testing

9ca52bf

alecgibson force-pushed the navigation-ab-test branch from df7d3c2 to 736c0d2 Compare February 10, 2017 12:13

boffbowsh temporarily deployed to government-frontend-pr-237 February 10, 2017 12:13 Inactive

alecgibson changed the title ~~[Do not merge] Navigation AB Test~~ Navigation AB Test Feb 10, 2017

carvil approved these changes Feb 10, 2017

View reviewed changes

alecgibson force-pushed the navigation-ab-test branch from 736c0d2 to b8ebb46 Compare February 13, 2017 10:09

boffbowsh temporarily deployed to government-frontend-pr-237 February 13, 2017 10:09 Inactive

Use AB Testing Gem

7911461

alecgibson force-pushed the navigation-ab-test branch from b8ebb46 to 7911461 Compare February 13, 2017 10:27

boffbowsh temporarily deployed to government-frontend-pr-237 February 13, 2017 10:27 Inactive

Add beta label to pages showing new taxonomy navigation

caea485

boffbowsh deployed to government-frontend-pr-237 February 13, 2017 10:55 View deployment

carvil merged commit 3f1780d into master Feb 13, 2017

carvil deleted the navigation-ab-test branch February 13, 2017 11:43

alecgibson mentioned this pull request Feb 23, 2017

Education navigation ab testing #256

Merged

		@@ -0,0 +1,9 @@
		<%= content_for :title, "#{@content_item.page_title} - #{t("content_item.format.#{@content_item.document_type}", count: 1)}" %>

		@@ -0,0 +1,54 @@
		<%= content_for :title, @content_item.page_title %>

Navigation AB Test #237

Navigation AB Test #237

Conversation

alecgibson commented Jan 24, 2017

NickColley commented Jan 24, 2017

alecgibson commented Jan 24, 2017

carvil commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecgibson commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fofr Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fofr Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tijmenb commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fofr Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickColley commented Jan 25, 2017

alecgibson commented Feb 10, 2017

carvil left a comment

Choose a reason for hiding this comment

malcolmbaig commented Feb 10, 2017

alecgibson commented Feb 13, 2017

carvil commented Feb 13, 2017

alecgibson commented Feb 13, 2017

tijmenb commented Feb 13, 2017

NickColley commented Feb 13, 2017

alecgibson commented Oct 11, 2017

fofr Jan 25, 2017 •

edited

Loading

fofr Jan 25, 2017 •

edited

Loading

fofr Jan 25, 2017 •

edited

Loading