Breach Results FxA Experiment (Growth Exp 3) #1659
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maxxcrawford
force-pushed
the
1652-breach-results-fxa-experiment
branch
from
869c1b5
to
d388fec
Compare
maxxcrawford
force-pushed
the
1652-breach-results-fxa-experiment
branch
2 times, most recently
from
06e6bf6
to
8e30527
Compare
maxxcrawford
requested review from
groovecoder and
lesleyjanenorton
and removed request for
groovecoder
maxxcrawford
force-pushed
the
1652-breach-results-fxa-experiment
branch
from
a0d2f3b
to
624f98a
Compare
maxxcrawford
commented
May 6, 2020
Comment on lines
-20
to
-32
| let experimentBranch = null; | ||
| let isUserInExperiment = null; | ||
| let experimentBranchB = null; | ||
|
|
||
|
|
||
| if (EXPERIMENTS_ENABLED) { | ||
| const coinFlipNumber = Math.floor(Math.random() * 100); | ||
| experimentBranch = getExperimentBranch(req, coinFlipNumber); | ||
| // req.session.excludeFromExperiment is set to remember that the user has been excluded from the experiment. | ||
| if (!experimentBranch) { req.session.excludeFromExperiment = true; } | ||
| req.session.experimentBranch = experimentBranch; | ||
| isUserInExperiment = (experimentBranch === "vb"); | ||
| experimentBranchB = (experimentBranch === "vb" && isUserInExperiment); |
There was a problem hiding this comment.
As the experiment doesn't split until the scans page, this has been removed.
maxxcrawford
commented
May 6, 2020
| @@ -230,6 +232,20 @@ async function getDashboard(req, res) { | |||
| const allBreaches = req.app.locals.breaches; | |||
| const { verifiedEmails, unverifiedEmails } = await getAllEmailsAndBreaches(user, allBreaches); | |||
|
|
|||
| let experimentBranch = null; | |||
| let isUserInExperiment = null; | |||
There was a problem hiding this comment.
This adds the experiment branch/meta UTM info to the /user/dashboard page.
maxxcrawford
commented
May 6, 2020
Comment on lines
-43
to
-57
| // If we cannot parse req.headers["accept-language"], we should not | ||
| // enroll users in the experiment. | ||
| if (!req.headers || !req.headers["accept-language"]){ | ||
| log.debug("No headers or accept-language information present."); | ||
| return false; | ||
| } | ||
|
|
||
| // If the user doesn't have an English variant langauge selected as their primary language, | ||
| // we do not enroll them in the experiment. | ||
| const lang = req.headers["accept-language"].split(","); | ||
| if (!lang[0].includes("en")) { | ||
| log.debug("Preferred language is not English variant: ", lang[0]); | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Because all the assets in this experiment are already localized, this experiment does not have any language qualifies during cohort assignment.
groovecoder
requested changes
May 6, 2020
controllers/user.js
Outdated
|
|
||
| if (EXPERIMENTS_ENABLED) { | ||
| const coinFlipNumber = Math.floor(Math.random() * 100); | ||
| experimentBranch = getExperimentBranch(req, coinFlipNumber); |
There was a problem hiding this comment.
This looks like if someone never does a scan, but gets straight to their dashboard, they'll be put into an experiment?
|
Spot-checked scan, sign-up, sign-in, and add additional emails. All work fine. Just the 1 question/comment about the experiment logic being in the dashboard, when the experiment only affects the scan results page. |
|
@groovecoder I updated the user controller per your request. Thanks! |
lesleyjanenorton
approved these changes
May 7, 2020
groovecoder
approved these changes
May 7, 2020
…ent options as it was breaking analytics reporting in Chrome environments.
…nality, except for when a user passes a URL param.
…ly set meta info on the page if the user has already been sorted into the experiment.
maxxcrawford
force-pushed
the
1652-breach-results-fxa-experiment
branch
from
7eff601
to
bf8a6e3
Compare
|
Rebased. Merging after checks pass one final time. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Meta Issue: #1652
Steps to Verify:
Control
name@example.comTreatment
name@example.com