-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Do Not Track support #4046
Do Not Track support #4046
Changes from all commits
d0cc92e
b038d9d
c20e6c2
3f74168
a62ff17
f963cdf
781ccaa
39d1663
7b7d742
c642625
fd3a886
4784272
f186dc1
66e5e2f
df22053
e01d94c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,33 +2,40 @@ | |
// https://docs.readthedocs.io/en/latest/advertising-details.html#analytics | ||
|
||
|
||
// RTD Analytics Code | ||
// Skip analytics for users with Do Not Track enabled | ||
if (navigator.doNotTrack === '1') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the JS respect the setting as well? Probably needs to be added to the context manager. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand this comment. Can you elaborate? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't add this JS opt out unless the DO_NOT_TRACK_ENABLED setting is True There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally, yes we want to only respect DNT if people running RTD have the setting enabled. However, the use case we are supporting by changing that is to help people taking the readthedocs.org code base and don't want to support DNT. To do this, we would need to pass the DNT setting through the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, that makes sense for user docs. I guess I was thinking for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's the only tidbit I'd add before it's deployable -- just so that all our users dashboards don't get DNT'd automatically. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to add this to |
||
console.log('Respecting DNT with respect to analytics...'); | ||
} else { | ||
// RTD Analytics Code | ||
(function(i,s,o,g,r,a,m){i['GoogleAnalyticsObject']=r;i[r]=i[r]||function(){ | ||
(i[r].q=i[r].q||[]).push(arguments)},i[r].l=1*new Date();a=s.createElement(o), | ||
m=s.getElementsByTagName(o)[0];a.async=1;a.src=g;m.parentNode.insertBefore(a,m) | ||
})(window,document,'script','https://www.google-analytics.com/analytics.js','ga'); | ||
|
||
(function(i,s,o,g,r,a,m){i['GoogleAnalyticsObject']=r;i[r]=i[r]||function(){ | ||
(i[r].q=i[r].q||[]).push(arguments)},i[r].l=1*new Date();a=s.createElement(o), | ||
m=s.getElementsByTagName(o)[0];a.async=1;a.src=g;m.parentNode.insertBefore(a,m) | ||
})(window,document,'script','https://www.google-analytics.com/analytics.js','ga'); | ||
if (typeof READTHEDOCS_DATA !== 'undefined') { | ||
if (READTHEDOCS_DATA.global_analytics_code) { | ||
ga('create', READTHEDOCS_DATA.global_analytics_code, 'auto', 'rtfd', { | ||
'cookieExpires': 30 * 24 * 60 * 60 | ||
}); | ||
ga('rtfd.set', 'dimension1', READTHEDOCS_DATA.project); | ||
ga('rtfd.set', 'dimension2', READTHEDOCS_DATA.version); | ||
ga('rtfd.set', 'dimension3', READTHEDOCS_DATA.language); | ||
ga('rtfd.set', 'dimension4', READTHEDOCS_DATA.theme); | ||
ga('rtfd.set', 'dimension5', READTHEDOCS_DATA.programming_language); | ||
ga('rtfd.set', 'dimension6', READTHEDOCS_DATA.builder); | ||
ga('rtfd.set', 'anonymizeIp', true); | ||
ga('rtfd.send', 'pageview'); | ||
} | ||
|
||
if (typeof READTHEDOCS_DATA !== 'undefined') { | ||
if (READTHEDOCS_DATA.global_analytics_code) { | ||
ga('create', READTHEDOCS_DATA.global_analytics_code, 'auto', 'rtfd'); | ||
ga('rtfd.set', 'dimension1', READTHEDOCS_DATA.project); | ||
ga('rtfd.set', 'dimension2', READTHEDOCS_DATA.version); | ||
ga('rtfd.set', 'dimension3', READTHEDOCS_DATA.language); | ||
ga('rtfd.set', 'dimension4', READTHEDOCS_DATA.theme); | ||
ga('rtfd.set', 'dimension5', READTHEDOCS_DATA.programming_language); | ||
ga('rtfd.set', 'dimension6', READTHEDOCS_DATA.builder); | ||
ga('rtfd.set', 'anonymizeIp', true); | ||
ga('rtfd.send', 'pageview'); | ||
// User Analytics Code | ||
if (READTHEDOCS_DATA.user_analytics_code) { | ||
ga('create', READTHEDOCS_DATA.user_analytics_code, 'auto', 'user', { | ||
'cookieExpires': 30 * 24 * 60 * 60 | ||
}); | ||
ga('user.set', 'anonymizeIp', true); | ||
ga('user.send', 'pageview'); | ||
} | ||
// End User Analytics Code | ||
} | ||
|
||
// User Analytics Code | ||
if (READTHEDOCS_DATA.user_analytics_code) { | ||
ga('create', READTHEDOCS_DATA.user_analytics_code, 'auto', 'user'); | ||
ga('user.set', 'anonymizeIp', true); | ||
ga('user.send', 'pageview'); | ||
} | ||
// End User Analytics Code | ||
// end RTD Analytics Code | ||
} | ||
|
||
// end RTD Analytics Code |
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.
you can use something like this script to check if
do not track
is enabledThere 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.
That entire script is basically to handle an IE10 bug. I'm not sure it's worth the effort.
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 think it should be in a script or a function that we can call from any scipt. Regarding IE, we may still support IE10.
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.
IE10 is not supported by Microsoft with a couple exceptions and it is a tiny fraction of our users (sub-0.1%). I don't think it is unreasonable to not support a privacy feature for users who are using a browser unsupported by the vendor. In addition, the "support" that the linked script offers is mostly just to mark IE10 as "unspecified" for DNT which for our purpose would be off.
I lean toward simplicity here.
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.
Understand. then we can keep
window.doNotTrack === '1' || navigator.doNotTrack === '1')
in a function and call it from everywhere!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.
Actually, after testing this I'm reconsidering. It looks like IE11 on Windows 7 and Windows 8 set the DNT default to on.
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.
Considering that to use this script would mean marking IE11 and IE10 as having DNT as "unspecified" I'm leaning toward maybe just checking
navigator.doNotTrack === '1'
and that's it. This would mean that no versions of IE can opt-out of tracking. It would be supported in MS Edge, however.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.
Not very easily actually.
readthedocs-analytics.js
is loaded on the docs pages and should not have any outside dependencies apart fromREADTHEDOCS_DATA
.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 updated the PR to just check
navigator.doNotTrack === '1'
.