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

add option for disabling Matomo cookies #314

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

egpbos
Copy link

@egpbos egpbos commented Dec 17, 2024

In RSQKit we want to disable Matomo cookies for now. This makes the site more easily GDPR compliant (without cookie wall). It only takes a single added line of JS, which this commit adds.

In RSQKit we want to disable Matomo cookies for now. This makes the site more easily GDPR compliant (without cookie wall). It only takes a single added line of JS, which this commit adds.
egpbos added a commit to EVERSE-ResearchSoftware/RSQKit that referenced this pull request Dec 17, 2024
The head.html file was copied from the current ETT version (3.2.0). Hopefully, ETT will adopt the PR to disable Matomo cookies (ELIXIR-Belgium/elixir-toolkit-theme#314). When they do, the head.html file in this repo can be removed again and we can simply configure the disabling of Matomo cookies via _config.yml.

This commit also pins the ETT version to 3.2.0 to make sure we update it manually later.
Copy link
Member

@bedroesb bedroesb left a comment

Choose a reason for hiding this comment

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

Thanks for proposing this! This is a very useful feature. I just proposed some changes related to existing instances that expect user tracking to work (cookies), and a new version update would with the current proposal break this.

@@ -29,6 +29,8 @@ matomo:
# Matomo domain where Matomo is running
matomo_id:
# Integer indicating the Matomo Website ID
matomo_cookies: false
Copy link
Member

Choose a reason for hiding this comment

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

I personally would leave the default behavior to the current behavior, which is matomo_cookies: true or not having this setting at all. Please also update https://elixir-belgium.github.io/elixir-toolkit-theme/configuring_theme

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense!

I wasn't sure what would happen if existing sites start using this functionality without adding matomo_cookies to their _config.yml. I.e. what is the default value of an undefined variable? What I gathered from my Googlings was that an empty value would be truthy (I read somewhere that only false and NIL are falsy), so then when users update ETT without adding matomo_cookies it would indeed be evaluated as true and hence the unless site.matomo_cookies clause would not trigger.

But it's very well possible I am mistaken! In any case, I will change the default value.

Copy link
Author

Choose a reason for hiding this comment

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

Applied suggestion in b05d71f.

Copy link
Author

Choose a reason for hiding this comment

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

Or is NIL actually what a non-defined value is? In that case I now understand your second more explicit if false suggestion! :D

@@ -124,6 +124,9 @@
<script>
var _paq = window._paq = window._paq || [];
/* tracker methods like "setCustomDimension" should be called before "trackPageView" */
{%- unless site.matomo_cookies %}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{%- unless site.matomo_cookies %}
{%- if site.matomo_cookies == false %}

I prefer it reverted. So if people don't want cookies, they have to explicitly say false.

Copy link
Author

Choose a reason for hiding this comment

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

I think these two statements are equivalent, no? But perhaps the more explicit if version is better indeed.

Copy link
Author

Choose a reason for hiding this comment

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

Applied suggestion in b05d71f.

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.

2 participants