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
Ensure analytics stops firing if a user disables usage cookies on our cookie settings page #3893
Conversation
3578a3f
to
dc8c796
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.
Looks good, sensible approach. Just a couple of questions that probably have obvious answers but my brain is struggling right now 😁
@@ -39,6 +39,11 @@ window.GOVUK.analyticsGa4 = window.GOVUK.analyticsGa4 || {}; | |||
}, | |||
|
|||
sendData: function (data) { | |||
// Allows us to stop sending tracking at the moment a user sets their usage cookies to "false" on the cookie settings page. | |||
if (window.GOVUK.stopSendingAnalytics) { | |||
return 'analytics disabled' |
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.
Why return this instead of just false
? Is it being used somewhere?
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.
@andysellick Happy to change it to false
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.
@andysellick Done, should be ready for re-review. Thanks
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.
Can you change it in ga4-core
as well?
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.
Sorry, my suggestion has broken some of your tests - they're expecting analytics disabled
instead of false
now.
@@ -49,7 +54,7 @@ | |||
Analytics.prototype.trackPageview = function (path, title, options) { | |||
arguments[0] = arguments[0] || this.defaultPathForTrackPageview(window.location) | |||
if (arguments.length === 0) { arguments.length = 1 } | |||
this.sendToTrackers('trackPageview', this.pii.stripPII(arguments)) | |||
return this.sendToTrackers('trackPageview', this.pii.stripPII(arguments)) |
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.
Why this change?
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.
@andysellick sendToTrackers
isn't a public function, so I can't access it in the tests. Therefore I return the value of it in the functions that are publicly accessible trackPageview / trackEvent / trackShare
so that I can verify in the tests that analytics is disabled.
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.
Just to check, this doesn't impact the outcome? As in, does returning this.sendToTracker
instead of executing it still call that function?
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.
@andysellick It's still being executed, the result of the execution is what is returned. If it didn't have ('trackPageview', this.pii.stripPII(arguments))
on the function, then the function would be being returned instead of executed.
Example here if you want to play around with it: https://jsfiddle.net/spLfjez5/2/
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.
Cool, no worries, just wanted to be sure 👍
986bffa
to
e8bd4a7
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.
Looks like you've addressed my comments, so happy to approve (remember to squash your commits).
e8bd4a7
to
0d78fca
Compare
What
usage
cookies set totrue
, and then disables them on/help/cookies
, our tracking still fires on the cookie settings page until they refresh/navigate away. (This doesn't affect other pages on GOV.UK as our JS will reinitialise every time they go to another page, which then checks their consent status.)window.GOVUK.stopSendingAnalytics
totrue
when they disable usage cookies. This boolean is then checked in Universal Analytics and GA4 before they send tracking data.window.GOVUK.LUX
to an empty object ({}
) when they disable the usage cookie as well, but I'm not sure how to test if this actually doing anything.Why
if usage = false, dont send analytics
) because this caused 200+ of our tests to start failing.window.GOVUK.analytics
andwindow.GOVUK.analyticsGa4
to empty objects once the usage cookie is false as well, but this also caused issues with the tests - even when I tried to revert the UA/GA4 objects after the tests, the tests were acting strangely. Doing this would have also caused console errors on the cookie settings page, as the existing tracking event listeners would try and run tracking functions that no longer exist.How to test
static
locally, pointing to this branch of the gemfrontend
locally, pointing to your localstatic
, and this branch of the gemhttp://127.0.0.1:3005/help/cookies
with your usage cookie set totrue
.Omnibug
, and GA4 events in thedataLayer
Omnibug
events ordataLayer
events if you interact with the page, for example expanding the header menu again.Visual Changes
None.