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 Cookie consent banner #125

Closed
wants to merge 11 commits into from
Closed

Add Cookie consent banner #125

wants to merge 11 commits into from

Conversation

vincanger
Copy link
Collaborator

the cookie consent banner only adds the Google Analytics script to the page when the user accepts the cookie.

Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

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

Nice start, but it seems to me it is a bit overly hard to understand at the moment so let's discuss that!

So I actually don't know much about the whole cookie consent thing, and I guess many others also don't. So just a bit of education here might go a long way.

So what are we doing here: we are adding the banner where they agree to cookies. Ok. How is that connected to our analytics? So if the agree, we will activate the analytics, is that it? Otherwise, analytics are not collected.

What about any cookies that we might use for other stuff? I don't think we use any though hm.

We might use them for Auth in the future, but right now we use only LocalStorage if I am correct. If we did use them for Auth, would that also fall under this?

What about having some docs for this? Shouldn't those also be part of this PR?

What about e2e test for this, would that be interesting?

It feels a bit confusing to me that they have the place to put analytics in head, but then we also have cookies, and they have to delete one or the other hm. Would be awesome if they could choose what they want, but I guess that would be too much right now.
But if we had docs we could call it out there, make it very clear it is a choice and what they have to do.

@@ -0,0 +1,197 @@
import type { CookieConsentConfig } from 'vanilla-cookieconsent';
Copy link
Member

Choose a reason for hiding this comment

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

Uff a lot of stuff happening here!

How should I think about this? I guess as a user I should be checking out the library to understand any of these options?

THis default config, anything I should know about it? What about uncommented stuff, that is for me to use if I want?

I see google analytics below -> that is only if I do use google analytics?

Seems quite complex all together! A bit too complex even hm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. i've started the process of simplifying the config based on Filip's "accept all / reject all" suggestion. Removing the individual preferences makes the cookie banner config more straightforward. Also added more links to the respective docs

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I would put this config under CookieConsent.tsx, to keep them together.
Or, group them together like this:

cookie-consent/
  Banner.tsx
  Config.ts

Or would it be CookieConsent/? I am not sure which convention we use for naming directories.

@vincanger
Copy link
Collaborator Author

@Martinsos here are some answers to your Qs:

So what are we doing here: we are adding the banner where they agree to cookies. Ok. How is that connected to our analytics? So if the agree, we will activate the analytics, is that it? Otherwise, analytics are not collected.

Exactly!

What about any cookies that we might use for other stuff? I don't think we use any though hm.

Nope. But currently, by putting analytics scripts in the head section of the config file causes them to load or be reloaded on every page load/refresh. Therefore, when using a consent banner, we can't add our analytics scripts in the expected Wasp manner.

What about having some docs for this? Shouldn't those also be part of this PR?

Definitely. Docs are on a separate branch though. I would add them after this PR gets merged.

What about e2e test for this, would that be interesting?

Yeah, possibly. I guess checking to see if the Google Analytics cookie (e.g. ga_...) is present or not is enough.

It feels a bit confusing to me that they have the place to put analytics in head, but then we also have cookies, and they have to delete one or the other hm. Would be awesome if they could choose what they want, but I guess that would be too much right now. But if we had docs we could call it out there, make it very clear it is a choice and what they have to do.

Perhaps we could add the code in the docs only, and if they want to opt-in to a using a cookie banner, they can add the component + config, and not add the script to head. How does that sound?

@sodic
Copy link
Collaborator

sodic commented May 16, 2024

Oh man, this brings on some PTSD 😄

Anyway, it's not as simple as it seems.

To answer some of the questions.

@Martinsos wanted to know what falls under GDPR and cookie consent.

In short, there's no such thing as a "Cookie law."
It's a very confusing unofficial term that trips people up all the time.

GDPR is a "Data regulation law". It never mentions or cares about cookies, local storage, session storage, or any kind of technology sites may use to achieve data storage and collection.

In short, all you need to know is:

Under the GDPR you can only store personal data that is necessary to provide the service you are providing. For everything else, you need opt-in consent.

It doesn't matter at all how you store this data (cookies, local storage, Google Analytics, etc.). It only matters whether you store it or not.

To complicate things, even though it's enough to offer only "reject all unnecessary" and "accept all," most companies prefer to divide the non-necessary data into subcategories (analytics, marketing, etc.).

This is because some users might be okay with sharing their data for analytics but not for marketing. Companies want to get as much data as possible, so offering subcategories might help them collect just a little more.

I haven't seen the banner Vinny implemented. However, if it does offer subcategories, we must make sure they are honored (e.g., we can have them accept "analytics only" and then go on to collect data used for marketing).

As far as I can see, the current setup we use for GA can also be used for GTM (Google uses GTM's setup for setting up GA), and GTM can do all kinds of stuff our banner might not expect it to. Therefore, I recommend we stick with the "accept or reject everything banner" and don't offer subcategories (that might be exactly what it does, I haven't run it).

Also, we should make sure not to show the banner if they're not collecting any extra data (i.e., using analytics).

@Martinsos
Copy link
Member

@sodic thanks for the nice explanation!

Ok, I agree with the logic of by default having "reject" or "accept all", that makes sense. Further than that is up to developers, they can customize it further how they like. What we are giving them is an easy way to get going with cookies.

@vincanger

  • e2e tests -> if it is too complicated we can skip them, but having them ensures it all keeps working, so up to you, I think you will best be able to decide what makes sense.
  • adding code to the docs only -> yeah hm, that is an "issue" we have recently, with features that are not super core, how to best have them -> added to the code, or as part of the docs. If they are part of code, we can do e2e tests on them. But some stuff is just too much.
    We could do though a separate open-saas app we are e2e-testing that is not the same thing as template, and there we can put everything, but I guess that would be too much at the moment. It would be cool if we had a mechanism for choosing what you want: I want cookies, I don't want them. I want analytics, I don't want them. I am not sure what is best at the moment I have to admit. Either we put them in and it is quite a bit of stuff then to remove, or we don't and we don't have a good way to test it. I will leave it to your intuition of what is best for open-saas's users. But we should start thinking about ways to offer people programmatic choice when creating an app, that would up our game quite a bit. Should we open an issue for that?
  • docs on separate branch -> ah yes, I remember something like that! Why is that so? Because on main is template, and on release is what is actually published as docs + demo app, so it is not the same thing? Could you shortly explain the reasoning behind the split, how it works, here, even if slightly off topic?

@vincanger
Copy link
Collaborator Author

@Martinsos

  • added e2e tests for accepting and rejecting cookies
  • I decided to leave the code in template. Because it is a separate component, it's easy enough to remove. And if they're serious about building a SaaS, it's nice to have that whole thing figured out for you already.
  • The "docs on a separate branch" issue we're discussing in the other issue, so I'll leave that for there.

Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

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

Approved! I did left some comments though so please check those out, but I think the main idea is fine. Once you take care of those, feel free to merge.

So next are docs?

app/src/client/components/cookie-consent/Config.ts Outdated Show resolved Hide resolved
app/src/client/components/cookie-consent/Config.ts Outdated Show resolved Hide resolved
root: 'body',
autoShow: true,
disablePageInteraction: false,
hideFromBots: import.meta.env.PROD ? true : false, // Set this to false for dev/headless tests otherwise the modal will not be visible
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
hideFromBots: import.meta.env.PROD ? true : false, // Set this to false for dev/headless tests otherwise the modal will not be visible
hideFromBots: import.meta.env.PROD ? true : false, // Set this to false for dev/headless tests otherwise the modal will not be visible.

mode: 'opt-in',
revision: 0,

// Default configuration for the cookie
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
// Default configuration for the cookie
// Default configuration for the cookie.

Comment on lines +46 to +51
{
name: /^_ga/, // regex: match all cookies starting with '_ga'
},
{
name: '_gid', // string: exact cookie name
},
Copy link
Member

Choose a reason for hiding this comment

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

Why, is this specifically tailored for Google Analytics here? Whta if I am using plausible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Plausible doesn't use cookies (privacy-conscious analytics)

label: 'Google Analytics',
onAccept: () => {
try {
const GA_MEASUREMENT_ID = import.meta.env.REACT_APP_GOOGLE_ANALYTICS_ID;
Copy link
Member

Choose a reason for hiding this comment

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

Why do you call it GA_MEASUEREMENT_ID when it is GOOGLE_ANALYTICS_ID?

gtag('js', new Date());
gtag('config', GA_MEASUREMENT_ID);

// Adding the script tag dynamically to the DOM
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
// Adding the script tag dynamically to the DOM
// Adding the script tag dynamically to the DOM.

Comment on lines 99 to 100
<a href="#" target="_blank">Privacy Policy</a>
<a href="#" target="_blank">Terms and Conditions</a>
Copy link
Member

Choose a reason for hiding this comment

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

Should they input these? We should indicate it somehow so they don't miss it.


// https://cookieconsent.orestbida.com/reference/configuration-reference.html#category-services
services: {
ga: {
Copy link
Member

Choose a reason for hiding this comment

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

So we have it only for ga here, not for plausible? How is that?

@vincanger
Copy link
Collaborator Author

vincanger commented Jun 3, 2024

Approved! I did left some comments though so please check those out, but I think the main idea is fine. Once you take care of those, feel free to merge.

So next are docs?

Next would be docs and updating landing-page to let people know we have e2e-tests #88

@vincanger vincanger closed this Jun 6, 2024
@vincanger vincanger deleted the cookie-consent branch June 6, 2024 14:28
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