Add Google Analytics tracking if and only if a user has clicked "Accept" on the notification #409
Add Google Analytics tracking if and only if a user has clicked "Accept" on the notification #409
Conversation
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.
+1 tested, very detailed and well thought out. Works well too! Great job.
import Snackbar from '@material-ui/core/Snackbar'; | ||
import Button from './Button'; | ||
import ShowOnly from './ShowOnly'; | ||
import COLOURS from '../util/COLOURS'; | ||
|
||
class GDPRNotification extends PureComponent { |
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 change from PureComponent
to Component
? (I'm not familiar with the subtle differences)
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.
My understanding of PureComponent is that it has some characteristics which memoize it in certain circumstances, but I don't quite know whether that's true or what those circumstances are. In the interest of being able to reason about this a little more accurately I switched it out for a regular Component.
}; | ||
acceptGDPRAlertAndDismissSnackbar = () => this.setState( | ||
state => Object.assign({}, state, { open: false }), | ||
acceptGATrackingAndStartTracking, |
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.
Excellent function name
src/app/src/util/util.ga.js
Outdated
const gaKey = env(REACT_APP_GOOGLE_ANALYTICS_KEY); | ||
const environment = env('ENVIRONMENT'); | ||
|
||
if (!environment || environment !== 'development') { |
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 don't quite understand the environment !== 'development'
clause here. Doesn't this mean that if we are in staging or production then we will always 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.
This is part of the test commit that will be dropped before merging. It's supposed to be if (environment === development) 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.
Yeah, sorry for not explaining well. I'll drop c68fabf before merging this, but it is essentially just to be able to test this in local development. For production and staging it will be set to === 'development'
-> 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.
This is a thoughtful implementation and is working well. I am not sure we need to be recording a consent action every time the page is loaded, but in the interest of getting this released I would rather get it deployed and remove it later if we decide we don't need it.
- add util.ga.js file with functions for getting and setting a value for the user's choice to accept or reject cookies and starting tracking if and only if a user has explicitly accepted the tracking code - add util.ga.tests.js file with tests for localstorage - update GDPRNotification component to use utility functions - conditionally call tracking code in GDPRNotification's componentDidMount method if and only if a user has already clicked the Accept link in the GDPRNotification snackbar - update cookie opt out Snackbar message with approved text - set link to TOS & privacy policy Connects #265 - send TRACKING_CONSENT event to Google Analytics prior to each pageview - add moment library for date parsing and comparison - store date user decided in localstorage - expire date after one year - don't track if date is expired
2e7c0ec
to
6a107e5
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.
I see that this is still conditionally and correctly sending analyitcs events.
Thanks for the feedback and help with this! |
Overview
This PR configures the application to add Google Analytics tracking if -- and only if -- a user has clicked "Accept" on the Notification. To make this easier to prove & verify, I opted to initialize the GA tracking conditionally in a utility function around which I was able to set up some testing apparatus.
If a user clicks "Reject", we set a value for
GA_TRACKING
in localStorage to"HAS_REJECTED_GA_TRACKING"
and then that sticks until the user clears localStorage.If a user clicks "Accept", we set a value for
GA_TRACKING
in localStorage to"HAS_ACCEPTED_GA_TRACKING"
and then set up tracking for that visit. On subsequent visits, the application will check for theGA_TRACKING
value and if it sees that there is a value AND that that value is"HAS_ACCEPTED_GA_TRACKING"
then tracking starts.For the following conditions, the application does not start GA Tracking at all:
window.localStorage
is unavailable for whatever reasonGA_TRACKING
has not been set at all inwindow.localStorage
GA_TRACKING
is inwindow.localStorage
but does not have a value of"HAS_ACCEPTED_GA_TRACKING"
.As an additional guard, I set an initial global opt out value per https://developers.google.com/analytics/devguides/collection/analyticsjs/user-opt-out which is only deleted when all the acceptance conditions have been met.
Connects #265
Connects #267
Demo
Notes
The second commit here is entirely for testing and I will drop it before merging this in. GA provides a helpful debug library which logs actions to the console so you can easily see when it is called.
Testing Instructions
Rejection testing
At no point prior to "Accepting" or after "Rejecting" should you see:
Acceptance testing
"anonymizeIp"
settingNo localStorage testing
about:config
Checklist
fixup!
commits have been squashed