-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
[amp-experiment] Allocate experiment variant based on CID. #3960
Conversation
LGTM, deferring final LGTM to @jridgewell |
return hasConsentPromise | ||
.then(hasConsent => { | ||
if (!hasConsent) { | ||
return null; |
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.
I'm not sure we want to return null
here. Imagine the user hasn't dismissed the consent notification yet, but will eventually. We're specifically not allocating this user into a variant since we can't get consent (we can't render block forever). But this is distinct from allocating the user into the null
variant below (because the publisher didn't provide a full 100% distribution of variants).
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.
Good point. This has been discussed during the design, and we decided to skip such an experiment on users first several visits until user has given the consent.
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.
👍
One comment, code LGTM. |
Thanks folks for the quick response! |
#1411