Skip to content
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

Merged
merged 1 commit into from Mar 4, 2024

Conversation

AshGDS
Copy link
Contributor

@AshGDS AshGDS commented Feb 29, 2024

What

  • Currently, if a user has usage cookies set to true, 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.)
  • I have fixed this by setting a boolean called window.GOVUK.stopSendingAnalytics to true when they disable usage cookies. This boolean is then checked in Universal Analytics and GA4 before they send tracking data.
  • I've set 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.
  • I'm assuming the only tracking stuff we have on our site is UA, GA4, and LUX. I'm going by this page: https://www.gov.uk/help/cookie-details

Why

  • Fixes a bug that has existed or years on the cookie settings page.
  • I'm not checking the consent cookie directly to stop sending analytics, (i.e. if usage = false, dont send analytics) because this caused 200+ of our tests to start failing.
    • I assume this is because a lot of our tests send tracking data without worrying what our usage cookie is set to, and if I used the usage cookie as a guard at the time we send tracking data, those 200 tests would then stop working. Therefore, using this boolean was a lot simpler.
    • I also assume using the consent cookie might have impact on the single consent work, as trying to get those 200 tests to work would probably cause a massive merge conflict for the consent PR.
    • I also thought that since UA will be removed soon, it would be a lot of effort to try and fix all the tests if they'll just be removed eventually.
  • I thought about just setting window.GOVUK.analytics and window.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.
  • Trello card: https://trello.com/c/vAI4kvX5/795-tracking-is-still-active-on-cookie-settings-page-after-rejecting-consent

How to test

  • Run static locally, pointing to this branch of the gem
  • Run frontend locally, pointing to your local static, and this branch of the gem
  • Go to the cookie settings page, http://127.0.0.1:3005/help/cookies with your usage cookie set to true.
  • If you expand/collapse our header menu, you will see UA events in Omnibug, and GA4 events in the dataLayer
  • If you then set the usage cookie to false via the cookie settings form, you will no longer get any additional Omnibug events or dataLayer events if you interact with the page, for example expanding the header menu again.

Visual Changes

None.

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3893 February 29, 2024 16:25 Inactive
@AshGDS AshGDS force-pushed the stop-tracking-on-cookie-settings branch from 3578a3f to dc8c796 Compare February 29, 2024 16:27
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3893 February 29, 2024 16:28 Inactive
@AshGDS AshGDS self-assigned this Feb 29, 2024
Copy link
Contributor

@andysellick andysellick left a 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'
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor

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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

@AshGDS AshGDS Mar 1, 2024

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/

Copy link
Contributor

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 👍

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3893 March 1, 2024 13:55 Inactive
@AshGDS AshGDS force-pushed the stop-tracking-on-cookie-settings branch from 986bffa to e8bd4a7 Compare March 1, 2024 14:07
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3893 March 1, 2024 14:07 Inactive
Copy link
Contributor

@andysellick andysellick left a 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).

@AshGDS AshGDS force-pushed the stop-tracking-on-cookie-settings branch from e8bd4a7 to 0d78fca Compare March 1, 2024 15:45
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3893 March 1, 2024 15:49 Inactive
@AshGDS AshGDS merged commit 7393258 into main Mar 4, 2024
12 checks passed
@AshGDS AshGDS deleted the stop-tracking-on-cookie-settings branch March 4, 2024 11:37
@andysellick andysellick mentioned this pull request Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants