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.

Sign up for GitHub

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

Jump to bottom

Start button AB test For July 2016 #2655

Merged
merged 2 commits into from
Jul 28, 2016
Merged

Start button AB test For July 2016 #2655

merged 2 commits into from
Jul 28, 2016

Conversation

ikennaokpala
Copy link
Contributor

@ikennaokpala ikennaokpala commented Jul 22, 2016 •
edited
Loading

Trello card

Description

This PR sets up AB test for the following smart answers:

Cohorts across the aforementioned smart answers fall under the following.

  • Start now
  • Continue
  • Next

Currently being tested in the Integration environment.

Also as a result of the recent update to the front-end toolkit.

This PR also contains a bump to the front-end toolkit from 4.8.0 to 4.14.0

_NB:_

This branch has already been tested on the integration server by @vp2015 and Dan.
See on Integration (Deploy App Job):

  • 5238
  • 5208
  • 5200
  • 5183
  • 5182
  • 5181

Heroku Preview Links:

This was referenced Jul 22, 2016
Merged
Merged
Merged
@ikennaokpala ikennaokpala force-pushed the start-button-ab-test-july-2016 branch 8 times, most recently from 5e7839e to 150f0c1 Compare July 22, 2016 16:21
@pmanrubia pmanrubia added needs content review Waiting for a content designer to approve Ready for code review labels Jul 26, 2016
@pmanrubia
Copy link
Contributor

I have added the labels Needs factcheck and Ready for code review

@pmanrubia
Copy link
Contributor

Could you please update the commit notes of 4fd1053 and describe why you are confident it is not breaking anything in smart-answers.
It feels like a big update: from 4.8 to 4.14 and I am not sure if the rest of the app will continue working as expected.

pmanrubia
pmanrubia reviewed Jul 26, 2016
View reviewed changes
app/assets/javascripts/smart-answers.js
function browserSupportsHtml5HistoryApi() {
return !! (history && history.replaceState && history.pushState);
}

function isExpectedPath(slug) { // Used mostly during A/B testing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not clear to me what this method is doing, because the argument is not a path, it is a smart-answer name. It might make be better to rename it to something similar to:

SmartAnswer.isStartPage(‘overseas-passports’)

If there are any particular reasons for doing split("/").join("") it would be good to explain it in the commit notes.

I am not sure either about having this function isolated. I would prefer so see it enclosed within an object / module. I will facilitate maintenance and testing.

Copy link
Contributor Author

@ikennaokpala ikennaokpala Jul 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your suggestion is for defining SmartAnswer.isStartPage(‘overseas-passports’) this way is also possible.

@ikennaokpala ikennaokpala force-pushed the start-button-ab-test-july-2016 branch 2 times, most recently from 657c607 to e208b4c Compare July 26, 2016 16:32
@ikennaokpala
Copy link
Contributor Author

ikennaokpala commented Jul 27, 2016 •
edited
Loading

@pmanrubia Great point and thanks for pointing this out.

I have examined closely the diff from v4.14.0 to v4.8.0

I haven't see anything that is of concern.. @selfthinker can you take a quick look.

I am also reviewing it again

@selfthinker
Copy link
Contributor

selfthinker commented Jul 27, 2016 •
edited by ikennaokpala
Loading

I've looked at all the changes Ikenna listed in the bump commit (ebe9cab):

Stylesheets:

  • _typography.scss e05ca517 , 70f53d27c
  • _colors.scss 11798945a , 3853a726c , 0052024b4b9 , 984d53292 , 2180a72e4

And I can confirm that none of them would have an effect on Smart Answers as far as I can tell.

@ikennaokpala ikennaokpala force-pushed the start-button-ab-test-july-2016 branch 4 times, most recently from 80f9a10 to 1e6ac98 Compare July 27, 2016 16:05
@pmanrubia
Copy link
Contributor

@ikennaokpala is there any reason for this commit: 4fdd4d5?

@ikennaokpala ikennaokpala force-pushed the start-button-ab-test-july-2016 branch 2 times, most recently from 1605f63 to 7fda193 Compare July 28, 2016 09:06
@ikennaokpala
Copy link
Contributor Author

@pmanrubia I have rebased the branch to sort that out..

@pmanrubia
Copy link
Contributor

Thank you @ikennaokpala and @selfthinker for reviewing the gem upgrade.
Great job @ikennaokpala!

LGTM

@ikennaokpala
Copy link
Contributor Author

@pmanrubia Thank you for pointing out the none obvious points.

@ikennaokpala
Bump govuk_frontend_toolkit gem to 4.14.0
8abc663 
This commit bumps the front-end toolkit from 4.8.0 to 4.14.0 as a result
of the recent update to the front-end toolkit.

* alphagov/govuk_frontend_toolkit#296
* alphagov/govuk_frontend_toolkit#294
* alphagov/govuk_frontend_toolkit#292

SmartAnswers makes use of the following files from the frontend_toolkit:
Stylesheets:
- _conditionals.scss
- _typography.scss
- _colours.scss
- _shims.scss
- design-patterns/_buttons.scss
- _grid_layout.scss

Javascript:
- govuk/multivariate-test

Having reviewed the diff between 4.8.0 and 4.14.0 alphagov/govuk_frontend_toolkit@0cfc216...37d23c1.

This diff composes of 82 commits, 34 Merge Commits and 48 concrete commits.

I gone through each tracing the possible areas that this might affect smart answers. To the best of my knowledge I have found only cosmetic changes to the following files.

Stylesheets:
- _typography.scss e05ca517 , 70f53d27c
- _colors.scss 11798945a , 3853a726c , 0052024b4b9 , 984d53292 , 2180a72e4

UI examination was carried by Anika and myself and we found nothing of concern.

Already existing unit, regression and integration tests passed, giving assurance
that the aforementioned commit/changes have had no (interfering) affect to the
transit route or flow or other routes within check uk visa or other smart answers.
@ikennaokpala ikennaokpala force-pushed the start-button-ab-test-july-2016 branch from 7fda193 to dcf9ceb Compare July 28, 2016 12:03
@ikennaokpala ikennaokpala removed the needs content review Waiting for a content designer to approve label Jul 28, 2016
@ikennaokpala
Add AB testing setup for July 2016 session
e471dae 
This commit sets up AB test for the following smart answers:

* overseas-passport
* calculate-your-child-maintenance
* marriage-abroad

Cohorts across the aforementioned smart answers fall under the following.

* Start now
* Continue
* Next

The isStartPage checks that the current path matches the slug of the smart answers that has been identified for A/B testing.

It splits the pathname with the forward slash delimiter and joins all.

This in effect returns a combination of all the characters in the path for the current page, returning true for only paths that equal the defined slug.
@ikennaokpala ikennaokpala force-pushed the start-button-ab-test-july-2016 branch from dcf9ceb to e471dae Compare July 28, 2016 12:38
@ikennaokpala ikennaokpala merged commit 8005e31 into master Jul 28, 2016
@ikennaokpala ikennaokpala deleted the start-button-ab-test-july-2016 branch July 28, 2016 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ikennaokpala @pmanrubia @selfthinker