-
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
AB tests for Self Assessment videos #3025
Conversation
dd16358
to
bf49721
Compare
bf49721
to
edb190c
Compare
edb190c
to
4829a59
Compare
18de032
to
2b5eb32
Compare
2b5eb32
to
547ed99
Compare
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.
Great work David and Anna! This looks good to me. I checked the pages when you published the placeholder strings on integration and used the chrome extension to toggle variants - it all seemed to be working fine.
Before merging, please could you either combine the commits into a single one, or create one commit per AB test - whichever you prefer.
Because I've pushed a commit to fix your failing tests, you might want to get another person to give the work a once over before merging
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.
Mostly looks good to me - it's tempting to suggest refactoring out the duplication (e.g. by pulling the placeholders / test names / base paths / dimensions from a config file), but that's a step towards normalising this kind of work, which I'm still reluctant to take. I prefer the more brutalist approach of just repeating the code (for now at least).
The one thing I'm not sure about is whether it's okay to reuse the same "dimension" (47) for all of the tests. I'm told this doesn't make any difference in the GA4 world anyway, but I'd appreciate a steer from a Performance Analyst on whether it would make their lives easier if we used different dimensions for different tests.
|
||
stub_content_store_has_item(content_item["base_path"], content_item) | ||
|
||
request.headers["HTTP_GOVUK_ABTEST_SAVIDEORETURN1"] = "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.
Turns out we've created a with_variant
helper method which does this in a neater way - we could consider using it here
https://github.com/alphagov/govuk_ab_testing/blob/main/README.md#minitest
Hello @richardTowers We discussed the dimension earlier - see this thread on Slack, so it seems that it is fine to use the same value for all the tests. |
- log-in-file-self-assessment-tax-return - pay-self-assessment-tax-bill/pay-weekly-monthly - stop-being-self-employed
547ed99
to
dccc08c
Compare
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.
Thanks folks! Happy for this to be merged now - because of the if ... @content_item.current_part_body.include?(placeholder)
conditional it won't do anything until the placeholder is in place, so we can deploy it everywhere and test in lower environments by updating the content.
Trello
This PR adds temporary A/B Tests to 3 pages on government-frontend:
N.B. The tests for the pay-weekly-monthly one are currently failing and need more work.