[10.0][IMP] website_cookie_notice: use specific cookie "accepted_cookie" by wpichler · Pull Request #461 · OCA/website

wpichler · 2018-05-15T09:44:28Z

No description provided.

rafaelbn

Waiting for runbot to test it. Travis is 🔴 Thanks @wpichler 😄

rafaelbn · 2018-05-15T23:57:08Z

website_cookie_notice/__manifest__.py

    'category': 'Website',
    'author': "Agile Business Group, "
              "Tecnativa, "
+              "Callino"


I'm very thankful for you contribution and improvement of this module, but I'm not sure if is fair to add you as an author after 2 lines of code. Usually contributor it is.

wpichler · 2018-05-16T05:19:13Z

Sorry - i thought this is necessary...

tarteo · 2018-06-14T07:24:33Z

Hi @wpichler can you explain why this addition is needed, please?

wpichler · 2018-06-14T08:02:25Z

there has been some rare cases where the normal way did not work - so setting this extra cookie does not hurt

rafaelbn · 2018-10-07T01:13:06Z

@yajo could you please review this code changes?

yajo

I like very much the idea, and IMHO this is much better implementation.

The problem with the previous one is that, when a session is destroyed (it is expired in the server, or user logs out), the user is asked yet again to allow cookies.

With this patch, the cookie acceptance is stored in a separate cookie, unrelated to user's session, so user shouldn't care about the cookie message anymore, no matter how many times he logs in or out.

(BTW, you should be the one explaining your work, not me, @wpichler 😉 )

But if we are going to use this new method, then please remove the old one. No need to have both.

yajo · 2018-10-08T08:05:31Z

website_cookie_notice/templates/website.xml

 <template id="message" name="Cookies notice">
    <div t-if="request and
-        not request.session.get('accepted_cookies')"
+        not request.session.get('accepted_cookies') and not request.httprequest.cookies.get('accepted_cookies')"


Use only the new method to check cookie acceptance. Remove the old one please.

wpichler · 2018-10-08T08:51:13Z

Sorry for not explaining - i'll try to explain better from now on...

Removing the old method is no problem - this would include removing the controller function...

While thinking about that - i'd had another idea to improve it...

Add a new field "cookies_accepted" - Bool default false to res.partner
When the cookie gets accepted - then call the controller function only if the current session is a non public session.
The controller function then does set the field cookies_accepted on the corresponding partner record
On Template we do check for this field if it is already set or not

So registered users will get complete away from the cookie notice - also on different browsers / devices...

What do you think ?

yajo · 2018-10-08T11:07:11Z

Hmm I think that better not... 🤔 I might accept cookies in my computer but not in my phone, for instance. I think the best choice is just the specific cookie as you were doing here.

Removing the old method is no problem - this would include removing the controller function...

Please do! I ❤️ code removal 😋

wpichler · 2018-10-08T13:46:31Z

Thats a valid point - so i've removed code now - and felling happy with it ;-)

simahawk

LG, thanks for the cleanup

yajo

Thanks! I rebuilt Travis; let's see if it goes ✔️ now.

simahawk · 2018-10-10T07:00:45Z

Tested on runbot, build is green: merging!
@wpichler thanks!

pedrobaeza · 2018-10-11T16:38:44Z

Isn't this cookie notice deprecated since GDPR? If not, this should be forward-ported to v11

wpichler · 2018-10-11T18:08:28Z

No - it has become more important since gdpr

pedrobaeza · 2018-10-11T18:13:37Z

Well, I don't think so if you don't need any explicit consent. And for that, you need to modify current message. @yajo, you are our GDPR expert. Can you talk about this?

wpichler · 2018-10-11T18:17:36Z

I think it is needed - take a look here: https://eugdprcompliant.com/cookies-consent-gdpr/

wpichler · 2018-10-11T18:18:25Z

Also - this is something that the website owner has to decide - we do only provide the tools for it

pedrobaeza · 2018-10-11T18:21:05Z

I think it is needed - take a look here: https://eugdprcompliant.com/cookies-consent-gdpr/

I remember that there was a clause that if the cookie is put for making legitimately work the website (not for tracking), it's not needed

Also - this is something that the website owner has to decide - we do only provide the tools for it

Of course, but promoting something that is not useful at all is not the best way of optimizing flows. If we provide this module without more explanations on new legal topic, people will install it without knowing the real need.

wpichler · 2018-10-11T18:38:45Z

Providing a explanation on a legal topic is in my opinion dangerous - if we are not correct with our interpretation of the topic - then we have given a wrong explanation.

yajo · 2018-10-15T08:47:08Z

This is part of GDPR, Article 6:

Lawfulness of processing

Processing shall be lawful only if and to the extent that at least one of the following applies:

(a) the data subject has given consent to the processing of his or her personal data for one or more specific purposes;

(b) processing is necessary for the performance of a contract to which the data subject is party or in order to take steps at the request of the data subject prior to entering into a contract;

So, if you want to use option B, you can skip this addon; but if you want to use option A, you can use it.

OCA is not your lawyer, we just share code here. If more than 1 user in the world wants option A, it makes sense to share this addon here. However, cnsult your lawyer to know whether to use it or not.

rafaelbn added this to the 10.0 milestone May 15, 2018

rafaelbn reviewed May 15, 2018

View reviewed changes

wpichler force-pushed the 10.0-add_accepted_cookie-WP branch from 39b5c4e to 39dad22 Compare May 16, 2018 05:21

tarteo approved these changes Jun 14, 2018

View reviewed changes

rafaelbn added the needs review label Oct 7, 2018

yajo requested changes Oct 8, 2018

View reviewed changes

[IMP] Add accepted_cookie cookie

4a1b239

wpichler force-pushed the 10.0-add_accepted_cookie-WP branch from 39dad22 to 4a1b239 Compare October 8, 2018 13:45

simahawk changed the title ~~[IMP] Add accepted_cookie cookie~~ [10.0][IMP] website_cookie_notice: use specific cookie "accepted_cookie" Oct 8, 2018

simahawk approved these changes Oct 8, 2018

View reviewed changes

yajo approved these changes Oct 9, 2018

View reviewed changes

simahawk merged commit 59a6060 into OCA:10.0 Oct 10, 2018

wpichler deleted the 10.0-add_accepted_cookie-WP branch October 10, 2018 07:12

yajo mentioned this pull request Mar 25, 2019

[FIX] website_cookie_notice: max-age for cookie #582

Merged

Uh oh!

Conversation

wpichler commented May 15, 2018

Uh oh!

rafaelbn left a comment

Choose a reason for hiding this comment

Uh oh!

rafaelbn May 15, 2018

Choose a reason for hiding this comment

Uh oh!

wpichler commented May 16, 2018

Uh oh!

tarteo commented Jun 14, 2018

Uh oh!

wpichler commented Jun 14, 2018

Uh oh!

rafaelbn commented Oct 7, 2018

Uh oh!

yajo left a comment

Choose a reason for hiding this comment

Uh oh!

yajo Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

wpichler commented Oct 8, 2018

Uh oh!

yajo commented Oct 8, 2018

Uh oh!

wpichler commented Oct 8, 2018

Uh oh!

simahawk left a comment

Choose a reason for hiding this comment

Uh oh!

yajo left a comment

Choose a reason for hiding this comment

Uh oh!

simahawk commented Oct 10, 2018

Uh oh!

pedrobaeza commented Oct 11, 2018

Uh oh!

wpichler commented Oct 11, 2018

Uh oh!

pedrobaeza commented Oct 11, 2018

Uh oh!

wpichler commented Oct 11, 2018

Uh oh!

wpichler commented Oct 11, 2018

Uh oh!

pedrobaeza commented Oct 11, 2018

Uh oh!

wpichler commented Oct 11, 2018

Uh oh!

yajo commented Oct 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants