-
Notifications
You must be signed in to change notification settings - Fork 17
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
Education navigation ab testing #256
Conversation
@@ -137,15 +137,13 @@ class ContentItemsControllerTest < ActionController::TestCase | |||
|
|||
content_store_has_item(content_item['base_path'], content_item) | |||
|
|||
with_variant EducationNavigation: "A" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the issue with using with_variant
here? Is it something we should change in the A/B gem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's just that I incorrectly added the meta tag and response headers before, where they shouldn't be applied if we've not got content tagged to a taxon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, you could include the assert_response_not_modified_for_ab_test
assertion, which will check that the meta tag and response headers haven't been added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything that you want to be reviewed in particular in the templates? Are they worth reviewing in depth, or are they very similar to some other version?
get :show, params: { path: path_for(content_item) } | ||
assert_equal [], @request.variant | ||
end | ||
setup_ab_variant('educationnavigation', 'A') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be EducationNavigation
to match the casing elsewhere.
get :show, params: { path: path_for(content_item) } | ||
assert_equal [], @request.variant | ||
end | ||
setup_ab_variant('educationnavigation', 'B') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
e9bff05
to
6b6e732
Compare
@suzannehamilton the templates are probably worth reviewing in case I missed anything. Sorry the diffing isn't easier. |
@@ -0,0 +1,3 @@ | |||
<div class="national-statistics-logo"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this logo need a new_navigation
file? Is it different in the A and B versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - it was in a one-third-column
div
6b6e732
to
54c9487
Compare
This extends the styling changes made in #237 to also encompass the following content item schemas:
document_collection
publication
The changes are to:
meta
tag to these documents to indicate that they are in an A/B test (when their content is tagged to a taxon)Vary
header on responses from these documentsTrello
https://trello.com/c/jd56fcgd/338-changes-to-education-related-content-pages-as-part-of-the-new-navigation-in-government-frontend-part-2