-
Notifications
You must be signed in to change notification settings - Fork 920
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
Data consent updates and privacy improvements (Fixes #14213) #14559
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14559 +/- ##
==========================================
+ Coverage 76.85% 76.90% +0.04%
==========================================
Files 156 156
Lines 8149 8166 +17
==========================================
+ Hits 6263 6280 +17
Misses 1886 1886 ☔ View full report in Codecov by Sentry. |
e1fe4ab
to
1acc725
Compare
1acc725
to
f1ebc2e
Compare
8cd7d63
to
0be5e58
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.
Issue: Banner remains visible after consent cookie is set.
Steps to reproduce:
- Land on a page which opens the banner
- Click
Cookie settings
- Configure cookies and save
- Use the back button or click previous page
Expected behaviour:
- Consent has been configured, the banner does not display
Actual behaviour:
- Consent banner is still visible.
(reloading the page acts as expected and does make it respect the new settings)
I'm open to this being non-blocking if it can be addressed in a follow-up PR, however, this is how I commonly interact with these banners so there's at least one user out there would would be confused by it.
Edit: Now that I know what's involved in fixing this I no-longer consider it launch blocking.
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.
With a consent cookie set that rejects analytics:
[ ] Glean deletion request ping IS sent.
I'm not sure what I'm looking for to confirm this.
Not at this time, but this could change in the future. The mandate thus far has been to fix issues for EU/EAA, with minimal changes for rest-of-world. From legal's perspective this is OK for now, but how we handle consent longer term for rest-of-world will likely be a followup item to address.
See: https://bedrock.readthedocs.io/en/latest/attribution/0001-analytics.html#debugging-pings. You should be able to see when a deletion request ping is sent.
What you're seeing here is bfcache, which is default browser behavior. Basically when clicking the back button, the previous page is cached by the browser rather than requiring it to be loaded over the network again. There are ways to disable this, but the obvious drawback is we lose the performance gains of bfcache. I'll take a look to see if there's anything we can do here, but my gut is to not really start messing with this stuff. |
b43d059
to
c182966
Compare
@stephaniehobson I pushed a commit containing a potential fix for the bfcache issue you spotted. It makes me a little nervous to write code that will automatically refresh a web page, so let's only use this if we feel like the fix is safe. I did add some guardrails around it so that the code will only run if a consent banner was visible on the previous page. Let me know what you think? I pushed it to the demo instance for testing - it seems to work OK? /**
* If user clicks back button from /cookie-settings/ and
* the page is in bfcache, reload the page if they have
* set a consent cookie to avoid showing the banner again
* inadvertently.
* @param {Object} e - The page navigation event.
*/
function handleBfCacheNavigation(e) {
if (e.persisted && hasConsentCookie()) {
window.location.reload();
}
}
window.addEventListener('pageshow', handleBfCacheNavigation, false); |
Glean (EU) > With GPC/DNT disabled > Clicking "Reject" closes banner > Glean deletion request ping IS sent. When I rejected cookies I saw this |
Chore: The following strings can be removed from
|
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 have tested/reviewed all the functionality and files I wanted to. I think the only merge-blocking thing is that I have not been able to verify the Glean deletion request ping. That could still be tester error though.
Removing the banner when the user goes back to the banner page after viewing cookie settings is not a launch blocker IMHO (now that I know what it would take to fix it...) but we should make sure we're both comfortable with the current state before it merges. Maybe we can clear that up on Slack tomorrow or Monday morning.
This is the expected result. The log message is worded a little oddly for sure. I asked the Glean folks about it, and they say it basically means the ping has no custom metrics attached to it other than the standard |
c182966
to
09db633
Compare
Thanks for the detailed review @stephaniehobson! I've addressed all comments so far. I think we're both comfortable enough with |
09db633
to
8b47cb6
Compare
R+ as soon as the lastupdated date is updated. Very through and thoughtful work on a massive change for the better. ✨ |
Thanks @stephaniehobson - can you please give me an r+wc and then I can update the date and merge on Monday? (this will give me extra time in my day to test things before deploying to prod). Thanks again |
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.
R+wc
Just needs the date updated.
8b47cb6
to
bbb3988
Compare
One-line summary
Note
I apologize for this PR being large, with many things to test. Whoever reviews I owe you a beverage of your choice at all-hands 🍺 🍸 🍹
This PR implements the following changes:
Significant changes and points to review
Issue / Bugzilla link
#14213
Testing
Successful test run: https://github.com/mozilla/bedrock/actions/runs/9113965630
Important
Make sure to clear cookies before / after each test.
Cookie settings page (EU):
URL: https://www-demo1.allizom.org/en-US/privacy/websites/cookie-settings/?geo=de
moz-consent-pref
consent cookie is NOT set, default form values should equal "I Do Not Agree".moz-consent-pref
consent cookie IS set.Cookie settings page (rest-of-world):
URL: https://www-demo1.allizom.org/en-US/privacy/websites/cookie-settings/
moz-consent-pref
consent cookie is NOT set, default form values should equal "I Agree".Firefox Campaign Pages (EU)
URL: https://www-demo1.allizom.org/de/firefox/challenge-the-default/?geo=de
With GPC/DNT enabled:
window.dataLayer
containscore-datalayer-loaded
andpage-id-loaded
events.moz-stub-attribution-code
andmoz-stub-attribution-sig
cookies are NOT set.With GCP/DNT disabled:
window.dataLayer
containscore-datalayer-loaded
andpage-id-loaded
events.moz-stub-attribution-code
andmoz-stub-attribution-sig
cookies ARE set.attribution_code
andattribution_sig
parameters ARE added to the Firefox auto-download URL.moz-stub-attribution-code
andmoz-stub-attribution-sig
cookies are NOT set.GTM test:
Firefox Campaign Pages (rest-of-world)
URL: https://www-demo1.allizom.org/de/firefox/challenge-the-default/
With GPC/DNT enabled:
moz-stub-attribution-code
andmoz-stub-attribution-sig
cookies are NOT set.With GPC/DNT disabled:
window.dataLayer
containscore-datalayer-loaded
andpage-id-loaded
events.moz-stub-attribution-code
andmoz-stub-attribution-sig
cookies ARE set.With a consent cookie set that rejects analytics:
moz-stub-attribution-code
andmoz-stub-attribution-sig
cookies are NOT set (you may need to clear these if they were already set previously before opt-out).With a consent cookie set that accepts analytics:
window.dataLayer
containscore-datalayer-loaded
andpage-id-loaded
events.moz-stub-attribution-code
andmoz-stub-attribution-sig
cookies ARE set.GTM test:
Firefox product pages (EU):
URL: https://www-demo1.allizom.org/en-US/firefox/new/?geo=de
moz-stub-attribution-code
andmoz-stub-attribution-sig
cookies are NOT set.Firefox product pages pages (rest-of-world)
URL: https://www-demo1.allizom.org/en-US/firefox/new/
With GPC/DNT enabled:
moz-stub-attribution-code
andmoz-stub-attribution-sig
cookies are NOT set.With GCP/DNT disabled:
window.dataLayer
containscore-datalayer-loaded
andpage-id-loaded
events.moz-stub-attribution-code
andmoz-stub-attribution-sig
cookies ARE set.attribution_code
andattribution_sig
parameters ARE added to the Firefox auto-download URL.With a consent cookie set that rejects analytics:
moz-stub-attribution-code
andmoz-stub-attribution-sig
cookies are NOT set (you may need to clear these if they were already set previously before opt-out).With a consent cookie set that accepts analytics:
window.dataLayer
containscore-datalayer-loaded
andpage-id-loaded
events.moz-stub-attribution-code
andmoz-stub-attribution-sig
cookies ARE set.attribution_code
andattribution_sig
parameters ARE added to the Firefox auto-download URL.Firefox RTAMO flow (same logic applies for EU and rest-of-world)
URL: https://www-demo1.allizom.org/en-US/firefox/download/thanks/?s=direct&utm_source=test&utm_campaign=test
With GPC/DNT enabled:
moz-stub-attribution-code
andmoz-stub-attribution-sig
cookies are NOT set.attribution_code
andattribution_sig
parameters.With GPC/DNT disabled:
moz-stub-attribution-code
andmoz-stub-attribution-sig
cookies ARE set.attribution_code
andattribution_sig
parameters.FxA form (EU)
URL: https://www-demo1.allizom.org/en-US/firefox/?geo=de&utm_source=test&utm_campaign=test
Note: since these are form fields, you might need to do a hard refresh between each test to reset the form.
utm_source=test
andutm_campaign=test
are NOT passed through to the formfxa-email-form-utm-source
andfxa-email-form-utm-campaign
hidden fields.flow_id
,device_id
) are NOT added to hidden fields.When using Firefox on desktop:
<input type="hidden" name="context" value="fx_desktop_v3">
IS added to the form.FxA form (rest-of-world)
URL: https://www-demo1.allizom.org/en-US/firefox/?utm_source=test&utm_campaign=test
Note: since these are form fields, you might need to do a hard refresh between each test to reset the form.
By default: (does not require DNT/GPC)
utm_source=test
andutm_campaign=test
ARE passed through to the formfxa-email-form-utm-source
andfxa-email-form-utm-campaign
hidden fields.flow_id
,device_id
) ARE added to hidden fields.With a consent cookie set that rejects analytics:
utm_source=test
andutm_campaign=test
are NOT passed through to the formfxa-email-form-utm-source
andfxa-email-form-utm-campaign
hidden fields.flow_id
,device_id
) are NOT added to hidden fields.With a consent cookie set that accepts analytics:
utm_source=test
andutm_campaign=test
ARE passed through to the formfxa-email-form-utm-source
andfxa-email-form-utm-campaign
hidden fields.flow_id
,device_id
) ARE added to hidden fields.When using Firefox on desktop:
<input type="hidden" name="context" value="fx_desktop_v3">
IS added to the form.VPN attribution (EU)
URL: https://www-demo1.allizom.org/en-US/products/vpn/?geo=pl&utm_source=test&utm_campaign=test
With GPC/DNT enabled:
utm_source=test&utm_campaign=test
are NOT propagated through to subscription links.flow_id
,device_id
etc) are NOT added to subscription links.With GCP/DNT disabled:
utm_source=test&utm_campaign=test
ARE propagated through to subscription links.flow_id
,device_id
etc) ARE added to subscription links.utm_source=test&utm_campaign=test
are NOT propagated through to subscription links.flow_id
,device_id
etc) are NOT added to subscription links.VPN attribution (rest-of-world)
URL: https://www-demo1.allizom.org/en-US/products/vpn/?utm_source=test&utm_campaign=test
By default: (does not require DNT/GPC)
utm_source=test&utm_campaign=test
ARE propagated through to subscription links.flow_id
,device_id
etc) ARE added to subscription links.With a consent cookie set that rejects analytics:
utm_source=test&utm_campaign=test
are NOT propagated through to subscription links.flow_id
,device_id
etc) are NOT added to subscription links.With a consent cookie set that accepts analytics:
utm_source=test&utm_campaign=test
ARE propagated through to subscription links.flow_id
,device_id
etc) ARE added to subscription links.VPN affiliate marketing flow (EU)
URL: https://www-demo1.allizom.org/en-US/products/vpn/?geo=de&cjevent=123456789
With GPC/DNT enabled:
stage.cjms.nonprod.cloudops.mozgcp.net/aic
.moz-cj-affiliate
cookie is NOT set.With GCP/DNT disabled:
stage.cjms.nonprod.cloudops.mozgcp.net/aic
.moz-cj-affiliate
cookie IS set.moz-cj-affiliate
cookie is NOT set.stage.cjms.nonprod.cloudops.mozgcp.net/aic
.VPN affiliate marketing flow (rest-of-world)
URL: https://www-demo1.allizom.org/en-US/products/vpn/?cjevent=123456789
With GPC/DNT enabled:
stage.cjms.nonprod.cloudops.mozgcp.net/aic
.moz-cj-affiliate
cookie is NOT set.With GCP/DNT disabled:
stage.cjms.nonprod.cloudops.mozgcp.net/aic
.moz-cj-affiliate
cookie IS set.With a consent cookie set that rejects analytics:
stage.cjms.nonprod.cloudops.mozgcp.net/aic
.moz-cj-affiliate
cookie is NOT set.With a consent cookie set that accepts analytics:
stage.cjms.nonprod.cloudops.mozgcp.net/aic
.moz-cj-affiliate
cookie IS set.VPN / Stripe Radar (EU)
URL: https://www-demo1.allizom.org/en-US/products/vpn/?geo=de
With GPC/DNT enabled:
js.stripe.com
are NOT set.With GCP/DNT disabled:
js.stripe.com
ARE set.js.stripe.com
are NOT set.VPN / Stripe Radar (rest-of-world)
URL: https://www-demo1.allizom.org/en-US/products/vpn/
By default:
js.stripe.com
ARE set.With a consent cookie set that rejects analytics:
js.stripe.com
are NOT set.With a consent cookie set that accepts analytics:
js.stripe.com
ARE set.Glean (EU)
URL: https://www-demo1.allizom.org/en-US/products/vpn/?geo=de
With GPC/DNT enabled:
With GPC/DNT disabled:
Glean (rest-of-world)
URL: https://www-demo1.allizom.org/en-US/products/vpn/
By default (does not require DNT/GPC):
With a consent cookie set that rejects analytics:
With a consent cookie set that accepts analytics:
Page banners (same logic applies for EU and rest-of-world)
This is not so easy to test as right now there is not a banner shown on desktop. The easiest way is to change this code locally so the firefox app store banner gets shown on desktop.
URL: http://localhost:8000/en-US/
firefox-app-store-banner
cookie IS set when clicking close (X) button on app banner.With a consent cookie set that rejects preference cookies:
firefox-app-store-banner
cookie is NOT set when clicking close (X) button on app banner.With a consent cookie set that accepts preference cookies:
firefox-app-store-banner
cookie IS set when clicking close (X) button on app banner.Traffic Cop (same logic applies for EU and rest-of-world)
URL: https://www-demo1.allizom.org/en-US/products/vpn/
With GPC/DNT enabled:
With GPC/DNT disabled:
GPC (Global Privacy Control)
URL: https://www-demo1.allizom.org/.well-known/gpc.json
application/json
content type