Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add content navigation multivariate test #526

Merged
merged 5 commits into from Nov 9, 2017
Merged

Add content navigation multivariate test #526

merged 5 commits into from Nov 9, 2017

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Nov 7, 2017

First pass at adding the content navigation multivariate test. This adds the logic to respond to test bucket, without any of the markup changes or variations.

Start with 4 variants matching https://github.com/alphagov/cdn-configs/pull/17
original, no nav, taxonomy nav, mainstream nav

  • Follow “concern” pattern setup in #513 for task list AB test
  • Only content with the document types listed are part of the test
  • The content must also be tagged to a taxon
  • Use a “universal_navigation” variant for all multivariate tests
    except original – they are largely the same, significant differences
    with original shouldn’t be repeated

Part of:
https://trello.com/c/sVQSGE2e/16-create-multivariant-test-for-content-pages
https://trello.com/c/1caGTIJn/77-create-no-navigation-test-variant-based-on-first-universal-design

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-526 Nov 7, 2017 Inactive
@fofr fofr requested review from tijmenb and Davidslv Nov 7, 2017
@@ -0,0 +1,87 @@
module ContentNavigationABTestable
# TODO: Use actual dimension
CONTENT_NAVIGATION_DIMENSION = 66

This comment has been minimized.

@steventux

steventux Nov 8, 2017
Contributor

sooo close! 67 is the confirmed dimension 👍

GUIDANCE_DOCUMENT_TYPES.include? @content_item.document_type
end

def content_navigation_test_original_variant?

This comment has been minimized.

@steventux

steventux Nov 8, 2017
Contributor

Not for this PR but I think we could find a way to reduce the constant + method boilerplate for these helper methods and make something generic enough to give us all the help we need for a given collection of variant names.

@fofr fofr force-pushed the content-navigation-test branch from afcb248 to 4efe167 Nov 8, 2017
@fofr fofr temporarily deployed to government-frontend-pr-526 Nov 8, 2017 Inactive
@fofr fofr temporarily deployed to government-frontend-pr-526 Nov 8, 2017 Inactive
@fofr fofr temporarily deployed to government-frontend-pr-526 Nov 8, 2017 Inactive
if @navigation.should_present_taxonomy_navigation?
request.variant = :taxonomy_navigation
if should_present_universal_navigation?
request.variant = :universal_navigation

This comment has been minimized.

@tijmenb

tijmenb Nov 8, 2017
Contributor

Are we sure about using variants? I would've expected that the universal navigation is a single template, not variants on the existing per-format templates.

This comment has been minimized.

@fofr

fofr Nov 8, 2017
Author Contributor

That might be what we end up with. For now there's still a few too many unknowns. While content will all be in two columns, there will be differences eg Guides with their pagination and page links at top vs Publications that have metadata, vs Answers which have neither.

This comment has been minimized.

@tijmenb

tijmenb Nov 8, 2017
Contributor

Cool. I think the dream would be a government-frontend that doesn't know anything about schema_name and document_type and determines layout dependent on the features it detects (not sure if that's 100% possible though).

This comment has been minimized.

@fofr

fofr Nov 9, 2017
Author Contributor

Strongly agree about content itself determining features, been wanting that for a while.

Keen not to focus on global layout and template though, prefer adaptable components that can handle whatever is given to them. End result ought to be similar.

@@ -0,0 +1,86 @@
module ContentNavigationABTestable
CONTENT_NAVIGATION_TEST_NAME = "ContentNavigation".freeze
CONTENT_NAVIGATION_DIMENSION = 67

This comment has been minimized.

@Davidslv

Davidslv Nov 9, 2017
Contributor

You could freeze this one too... although ruby will disappoint you:

CONTENT_NAVIGATION_DIMENSION = 1
=> 1
irb(main):002:0> CONTENT_NAVIGATION_DIMENSION = 2
(irb):2: warning: already initialized constant CONTENT_NAVIGATION_DIMENSION
(irb):1: warning: previous definition of CONTENT_NAVIGATION_DIMENSION was here
=> 2
irb(main):003:0> CONTENT_NAVIGATION_DIMENSION
=> 2
irb(main):004:0> CONTENT_NAVIGATION_DIMENSION = 2.freeze
(irb):4: warning: already initialized constant CONTENT_NAVIGATION_DIMENSION
(irb):2: warning: previous definition of CONTENT_NAVIGATION_DIMENSION was here
=> 2
irb(main):005:0> CONTENT_NAVIGATION_DIMENSION = 1
(irb):5: warning: already initialized constant CONTENT_NAVIGATION_DIMENSION
(irb):4: warning: previous definition of CONTENT_NAVIGATION_DIMENSION was here
=> 1
irb(main):006:0> CONTENT_NAVIGATION_DIMENSION
=> 1

This comment has been minimized.

@tijmenb

tijmenb Nov 9, 2017
Contributor

I think this confuses constants and frozen objects.

Constants aren't actually constants in Ruby, and you can change them (albeit with the warning you're seeing).

Unrelatedly objects in Ruby can also be frozen, which means you can't mutate them.

 @tijmenb ~ $ irb
irb(main):001:0> foo = "bar"
=> "bar"
irb(main):002:0> foo.upcase!
=> "BAR"
irb(main):003:0> foo
=> "BAR"
irb(main):004:0> foo.freeze
=> "BAR"
irb(main):005:0> foo.downcase!
RuntimeError: can't modify frozen String
	from (irb):5:in `downcase!'
	from (irb):5
	from /Users/tijmenbrommet/.rbenv/versions/2.3.1/bin/irb:11:in `<main>'

Since an integer like 67 can never be mutated, calling .freeze on it won't do anything.

Copy link
Contributor

@Davidslv Davidslv left a comment

👍 This looks good to me.

@fofr fofr changed the title [Do not merge] Add content navigation multivariate test Add content navigation multivariate test Nov 9, 2017
@fofr fofr force-pushed the content-navigation-test branch from a96f941 to 32b6634 Nov 9, 2017
@fofr fofr temporarily deployed to government-frontend-pr-526 Nov 9, 2017 Inactive
fofr added 4 commits Nov 7, 2017
Guides and detailed guides form part of the upcoming multivariate
content navigation test. The criteria for that test is “tagged to a
taxon” and a restricted guidance subset.

* Switch to document collection to test logic and avoid falling into
test (document collection is classified as guidance for the taxonomy
but isn’t in the stricter set used for content nav test)
Start with 4 variants:
original, no nav, taxonomy nav, mainstream nav

* Follow “concern” pattern setup in #513 for task list AB test
* Only content with the document types listed are part of the test
* The content must also be tagged to a taxon
* Use a “universal_navigation” variant for all multivariate tests
except original – they are largely the same, significant differences
with original shouldn’t be repeated
The AB testing gem defaults to ‘A’ when there’s no control variant set.
In this test there is no A. Explicitly set to the Original variant.
The helper methods for checking the current variant (eg
`universal_navigation_without_nav?`) were not checking whether the AB
test applied or not.

If the CDN set the response header and this method was used on a page
that doesn’t qualify for the test, then it would give a false positive.

* Add test to confirm expected behaviour
* Update helpers to check AB test applies before checking variant
@fofr fofr force-pushed the content-navigation-test branch from 32b6634 to d3c8e3f Nov 9, 2017
@fofr fofr temporarily deployed to government-frontend-pr-526 Nov 9, 2017 Inactive
@fofr fofr merged commit 4540801 into master Nov 9, 2017
2 checks passed
2 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/publishing-e2e-tests Publishing end-to-end tests succeeded on Jenkins
Details
@fofr fofr deleted the content-navigation-test branch Nov 9, 2017
@fofr fofr mentioned this pull request Nov 13, 2017
1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.