-
Notifications
You must be signed in to change notification settings - Fork 198
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
Growth Experiment #5 - "Join the Party" Share Link #1797
Conversation
@@ -36,9 +36,10 @@ | |||
<h2 class="pref-headline">{{ getString "email-addresses-title" }}</h2> | |||
{{> dashboards/manage-email-link variableClass="hide-mobile"}} | |||
</div> | |||
<div class="email-cards flx flx-col jst-cntr"> | |||
<div class="email-cards flx flx-col jst-cntr {{#if experimentFlags.treatmentBranch}}experiment{{/if}}"> |
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.
Note - I wasn't able / didn't see the value in persisting the experimentFlags
data all the way down to each verifiedEmail
object. This solution still prints it out, but it is hidden (via CSS) by default.
b83272e
to
b3c0a22
Compare
const { changePWLinks } = require("../lib/changePWLinks"); | ||
const { getAllEmailsAndBreaches } = require("./user"); | ||
|
||
const EXPERIMENTS_ENABLED = (AppConstants.EXPERIMENT_ACTIVE === "1"); | ||
const { getExperimentFlags } = require("./utils"); |
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.
Adding experiment flags to the breach details template to show/hide the Share CTA in page.
if (req.session.redirectHome) { | ||
req.session.redirectHome = false; | ||
return res.redirect("/"); | ||
} |
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.
Before this flag, if a user went to /share/{non-breach-identifier}
, it would leave that as the URL. This puts them back to the homepage, proper like.
Note that if you share a breach detail link (Ex: share/Zynga
), it keeps the /share/
pattern.
getExperimentBranch(req, false, ["en"], { | ||
"va": 50, | ||
"vb": 50, | ||
}); | ||
} |
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.
Enroll 50% of ALL English variant traffic in this experiment.
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 function returns assignedCohort
but we're not assigning that to anything here? If this function is mainly to set the experiment for the session, the function should probably be re-named to setExperimentBranch
?
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.
Filed Issue #1884
@@ -229,7 +229,9 @@ async function getDashboard(req, res) { | |||
const user = req.user; | |||
const allBreaches = req.app.locals.breaches; | |||
const { verifiedEmails, unverifiedEmails } = await getAllEmailsAndBreaches(user, allBreaches); | |||
const utmOverrides = getUTMContents(req); |
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.
Setting the /share/
link catcher on this page too, in case the person who uses it is already signed into Monitor.
middleware.js
Outdated
const featuredBreach = HIBP.getBreachByName(allBreaches, breachName); | ||
|
||
if (featuredBreach) { | ||
req.session.utmOverrides.campaignTerm = featuredBreach.Name; |
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.
Verify that user is actually requesting a valid Breach to be featured.
|
||
const router = express.Router(); | ||
const csrfProtection = csrf(); | ||
|
||
router.get("/", csrfProtection, home); | ||
router.get("/share/orange", csrfProtection, getShareUTMs, home); |
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.
Added middleware function to catch /share/
link requests and set the UTMs accordingly
@@ -28,6 +33,21 @@ function getFooterLinks(args) { | |||
}, | |||
]; | |||
|
|||
// Growth Experiment: Only add the footer share line if user is on VB branch | |||
const experimentFlags = getExperimentFlags(args.data.root.req, EXPERIMENTS_ENABLED); | |||
if (EXPERIMENTS_ENABLED && experimentFlags.treatmentBranch) { |
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 appends the share link to the footer (same in the header too) without breaking the current loop-build flow to hard-code this entry in.
string.stringId =LocaleUtils.fluentFormat(locales, stringId); | ||
// Growth Experiment: Catch EN* Experimnal Strings | ||
if (stringId === "share-monitor") { | ||
string.stringId = "Share Monitor"; |
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.
Work-around to not have the L10N team localize this experiment string.
…n on treatment branch
…padding, input height, btn color)
…elying on session data
…o that users' session
…e Copy button is clicked
…riment. This will keep the share-traffic cohort sepereate from the experiment cohort.
…match the comp Fixed #1856 - Adjusted modal headline size on modal to better match the comp Adjusted nbsp/word wrapping issue
- Fixed indentation - Corrected comment typos - Removed unneeded whitespace - Changed order of require statements
8b80a88
to
2fd1c68
Compare
Related issues:
Outstanding issues:
-exp
SemVer taggingTesting Steps
There are four share elements that show up in the treatment branch. Below describes the steps to test each:
Requirements:
General / Footer
General / Avatar Menu (Must be logged in)
General / User Dashboard (Must be logged in)
Breach-specific / Breach Details Page
Expected behavior about the Modal: