New Cookies banner #2411

luisramos0 · 2018-06-27T16:23:59Z

What? Why?

Closes:

[Cookies] Create cookie policy page #2401 and [Cookies] Add footer link to access the cookie policy page #2402 (cookies policy page and link in footer)
[Cookies] Build coookie banner and add a toggle to power it on-off #2407 (cookies banner)
[Cookies] Add privacy policy URL and display in footer #2406 (privacy URL)

Added footer link and cookies policy page.
New cookies banner toggle and new cookies banner with accept cookies button working.
Privacy URL BO setting and respective link on footer also added as part of this PR: issue 2406.
New darkswarm folder cookie_policy with directive, controller, service, template and styling.

What should we test?

Cookies Policy Modal

The footer link to the cookies policy page should always open the modal.
The modal should describe cookies and show the correct instance domain name.
On clicking the X or pressing Escape key in the cookies policy modal, the modal should close and user should be back to the page she was.
Also,

URL http://<instance_domain>/#/policies/cookies should load homepage with open cookies policy modal, if modal closed here, the user should be left on the homepage
URL http://<instance_domain>/shops/#/policies/cookies should load shops page with open cookies policy modal, on modal close, if modal closed here, user should be left on the shops page

Same behaviour on mobile, all cookie policy parts should be readable.

Cookies Banner

New cookie banner with "accept cookies" button and link to cookie policy page.
New footer entry with link to privacy policy URL.
New legal settings in BO with cookie banner toggle and privacy URL.

This new banner also needs to be tested on mobile.

Modals

This change affects the max height of all modals. The registration process should be tested while changing the browser window size to see if it looks okay with different sizes.

Release notes

New cookies policy link in footer and new cookies policy modal with details about the cookies used on the website.
New cookie banner with "accept cookies" button and link to cookie policy page.
New footer entry with link to privacy policy URL.
New legal settings in BO with cookie banner toggle and privacy URL.

mkllnk

Good work in bringing it all together. There are a few issues though. Here are a couple of screens.

mkllnk · 2018-07-04T00:10:29Z

app/assets/stylesheets/darkswarm/modals.css.scss

@@ -28,7 +28,7 @@ dialog, .reveal-modal {
  // Medium and up - when modal IS NOT full screen
  @media only screen and (min-width: 641px) {
    top: 10%;
-    max-height: 120%;
+    max-height: 100%;


Can you explain why you are doing this change? And does it open #1766 fixed in #2055 again?

It's good practice to explain the why in the commit message.

@mkllnk sorry, I forgot to explain this one!

I changed it to 100% so that the modal is never outside the viewport. Allowing a height over 100% makes content go outside the viewport without scrollbars...

Thanks for the reference to #2055! I have just tested the registration process with this new value and it looks good according to my tests.

@HugsDaniel does this make sense to you? I see your comment here: #2055 (comment)
Why would you ever let max-height be bigger than 100%? That means you remove scroll bars and the content will not show. I think 100% is always the correct value, so that if the content doesn't fit, you get the scroll bars. That's how all modals should work. Is there an example where you think this 100% value will not work?
Thanks!

I thought that the modal may be bigger than the screen, but then there will be scrollbars for the whole screen, not the model. And you can still scroll down. That avoids scroll bars within the modal which could be a double up (scroll screen + scroll modal) and look odd. But I'm actually not that familiar with the issue. There definitely needs to be a way of scrolling.

I see. Well, I have retested the registration process and it looks good with max-height 100%: it makes the modal scroll, not the screen. I think this is fine. But lets wait for someone else's feedback.
Thanks!

We'll see when we test but my experience of cookies banner is that you can scroll the page behind it but it doesn't scroll the modal. But for the registration process on modal you will definitely need to scroll as some modal pages have too many info to fit on small mobile screens... anyway I don't get the technical side so happy to see when we test what it looks like :-)

Referencing PR #2544.

In summary, I discovered that there were issues with the JS modal library (angular-bootstrap) and the styles we are using:

The 10px space at the top is actually intended to be 10% in the styles. The JS library is just treating it as px, I don't know if by design.

There is a race condition between the -25% animation of the modal and focus() in the JS library which causes the page to scroll up (if still possible).

Offset from the top plus the height of the modal (on any screen size) should not exceed 100%, or else some content could be hidden. The change of @luisramos0 decreasing max-height for large screens from 120% to 100% is a big improvement. Fix top position, height, and scrolling with modals #2544 also addresses the lack of max-height for small screens.

I see that this is already merged. Maybe we can move discussion of the modal dimensions to #2544? I've just rebased it upon the updated master. I'll see how it affects the cookies policy modal, and also check #1766.

mkllnk · 2018-07-04T00:12:58Z

app/assets/javascripts/darkswarm/cookies_policy/cookies_policy.html.haml

@@ -0,0 +1,89 @@
+%h2


When you introduce new user interfaces, it's helpful to add some screenshots to your pull request.

Yes, I have now added links to the screenshots as a comment to this PR.

yes, that makes sense. I have added links to all the screenshots below as a comment to this PR.

Okay, I added that to the test steps of your PR description.

mkllnk · 2018-07-04T00:24:30Z

config/locales/en.yml

@@ -1270,8 +1270,33 @@ en:
  footer_legal_tos: "Terms and conditions"
  footer_legal_visit: "Find us on"
  footer_legal_text_html: "Open Food Network is a free and open source software platform. Our content is licensed with %{content_license} and our code with %{code_license}."
-
+  footer_legal_data_text: "We take good care of your data." 
+  footer_legal_data_call:  "See our"


I see why you split the text, but it makes it hard for translators to get the context. Depending on the grammar of the language, the link may also not be at the end of the sentence. I would do it like footer_legal_text_html:

footer_legal_data_statement_html: "We take good care of your data. See our %{cookies_policy}."

It would also be nice to refactor these translations to use lazy lookup. I know, you went with the existing structure and that's fine. But it's even better when we can do a bit of refactoring on the way and improve the code that touching the new things we want to build.

I did it this way because, on the same sentence, there is a conditional. We have to display privacy URL if it is configured in the BO.
See footer_legal_data_privacy_policy here: 1e3b67d
With this in mind, I am not sure I can do what you suggest (in the requirements the privacy policy URL comes before the cookies policy in the sentence). Do you understand?

In terms of lazy lookup, do you mean to move all footer translations (the ones from line 1223 on en.yml to line 1254) into the "shared" subfolder on en.yml (line 1077)? I think that's what you mean. Please confirm I'll do it.

Maybe you could have both sentences in full as separate keys, and show either one based on the conditional instead of a split sentence.

Yes, I think having both versions in full sentences is the best option.

thanks for your feedback!
Agreed, I have made it 2 versions with the full sentence. Please have a look at the new version.

mkllnk · 2018-07-04T00:27:17Z

config/locales/en.yml

+      cookie_stripe_desc: "Data collected by our payment processor Stripe for fraud detection https://stripe.com/cookies-policy/legal. Not all shops use Stripe as a payment method but it is a good practice to prevent fraud to apply it to all pages. Stripe probably build a picture of which of our pages usually interact with their API and then flag anything unusual. So setting the Stripe cookie has a broader function than simply the provision of a payment method to a user. Removing it could affect the security of the service itself. You can learn more about Stripe and read its privacy policy at https://stripe.com/privacy."
+      disabling_cookies_header: "Warning on disabling cookies"
+      disabling_cookies_desc: "As a user you can always allow, block or delete Open Food Network’s or any other website cookies whenever you want to through your browser’s setting control. Each browser has a different operative. Here are the links:"
+      disabling_cookies_firefox_link: "http://support.mozilla.org/es/kb/habilitar-y-deshabilitar-cookies-que-los-sitios-we"


These are Spanish links in an English translation file?

I have copied those from the feature specification... I have changes the urls to english ones now.

mkllnk · 2018-07-04T00:32:49Z

app/views/shared/_footer.html.haml

@@ -143,7 +143,11 @@
          %div
            = t :footer_legal_data_text
            = t :footer_legal_data_call
-            %a{'cookies-policy-modal'=> true, 'cookies-banner'=> @show_cookies_banner}
+            - if "#{Spree::Config.privacy_policy_url}" != ""


Why not - if Spree::Config.privacy_policy_url.present??

Yes, fixed. Excuse my Ruby :-D

mkllnk · 2018-07-04T00:33:29Z

app/views/shared/_footer.html.haml

@@ -143,7 +143,11 @@
          %div
            = t :footer_legal_data_text
            = t :footer_legal_data_call
-            %a{'cookies-policy-modal'=> true, 'cookies-banner'=> @show_cookies_banner}
+            - if "#{Spree::Config.privacy_policy_url}" != ""
+              %a{href: "#{Spree::Config.privacy_policy_url}"}


Same here: %a{href: Spree::Config.privacy_policy_url} should work.

Yes, fixed.

mkllnk · 2018-07-04T00:33:56Z

app/views/shared/_footer.html.haml

+              %a{href: "#{Spree::Config.privacy_policy_url}"}
+                = t :footer_legal_data_privacy_policy
+              = t :footer_legal_data_call_and
+            %a{'cookies-policy-modal'=> true, 'cookies-banner'=> @show_cookies_banner && ("true" == "#{Spree::Config.cookies_consent_banner_toggle}")}


Minor style note: missing spaces before =>.

mkllnk · 2018-07-04T00:39:32Z

app/overrides/spree/admin/general_settings/edit/legal_settings.html.haml.deface

+    = preference_field_tag(:cookies_consent_banner_toggle, Spree::Config[:cookies_consent_banner_toggle], :type => Spree::Config.preference_type(:cookies_consent_banner_toggle))
+    = label_tag(:cookies_consent_banner_toggle, t(:cookies_consent_banner_toggle)) + tag(:br)
+  .field
+    = label_tag(:privacy_policy_url, t(':privacy_policy_url')) + tag(:br)


Copy and paste error in t(':privacy_policy_url'), additional :.

mkllnk · 2018-07-04T00:48:19Z

app/assets/javascripts/darkswarm/cookies_banner/cookies_banner.html.haml

+.row
+  .large-9.columns
+    %p
+      {{ 'legal.cookies_banner.cookies_usage' | t}}


These translations are actually under legal.cookies_policy. It would be better to use the lazy lookup and move the translations to the right place.

These translations are not in legal.cookies_policy, they are in legal.cookies_banner, which is different. Or I may be missing something.
These are javascript translations in angular templates where I don't think we can use lazy lookup. Or I may be missing something... (if this is correct, we can't use lazy lookup in angular templates, I can add this detail to the i18n page).

You are right. I just checked and this is correct. And since this is an Angular template, it needs to be translated through the t filter. Somehow my computer is not re-compiling the locale for Javascript and I get the missing translations. But that's not your fault. ;-)

luisramos0 · 2018-07-20T16:37:17Z

Please see screenshots here (cookies banner), here (BO legal settings) and here (footer and cookies policy page).

luisramos0 · 2018-07-20T22:13:32Z

Hey @mkllnk , thanks a lot for your review! I have fixed or commented on every point you mentioned. Please have a look. Thanks!

mkllnk

Much better. Now I found some more things that you could improve. :-D

mkllnk · 2018-07-23T01:39:31Z

app/assets/stylesheets/darkswarm/modals.css.scss

@@ -28,7 +28,7 @@ dialog, .reveal-modal {
  // Medium and up - when modal IS NOT full screen
  @media only screen and (min-width: 641px) {
    top: 10%;
-    max-height: 120%;
+    max-height: 100%;


I thought that the modal may be bigger than the screen, but then there will be scrollbars for the whole screen, not the model. And you can still scroll down. That avoids scroll bars within the modal which could be a double up (scroll screen + scroll modal) and look odd. But I'm actually not that familiar with the issue. There definitely needs to be a way of scrolling.

mkllnk · 2018-07-23T01:49:06Z

app/views/shared/_footer.html.haml

@@ -139,6 +139,12 @@
          %a{href:"https://github.com/openfoodfoundation/openfoodnetwork", target: "_blank"} GitHub
        %p.text-small
          = t :footer_legal_text_html, {content_license: link_to('CC BY-SA 3.0', 'https://creativecommons.org/licenses/by-sa/3.0/'), code_license: link_to('AGPL 3', 'https://tldrlegal.com/license/gnu-affero-general-public-license-v3-(agpl-3.0)')}
+        %p.text-small
+          %div
+            = t :footer_legal_data_text


These should be lazy lookup keys. They should be in en.shared.footer and referenced as:

= t '.legal_data_text'

👍 I have moved all translations in footer to shared/footer. please have a look at the new version. It feels good to clean up!

mkllnk · 2018-07-23T01:49:21Z

app/views/shared/_footer.html.haml

+          %div
+            = t :footer_legal_data_text
+            = t :footer_legal_data_call
+            %a{'cookies-policy-modal'=> true}


Missing space.

👍 fixed in the new version.

mkllnk · 2018-07-23T01:50:17Z

config/locales/en.yml

@@ -1270,8 +1270,33 @@ en:
  footer_legal_tos: "Terms and conditions"
  footer_legal_visit: "Find us on"
  footer_legal_text_html: "Open Food Network is a free and open source software platform. Our content is licensed with %{content_license} and our code with %{code_license}."
-
+  footer_legal_data_text: "We take good care of your data." 
+  footer_legal_data_call:  "See our"


Yes, I think having both versions in full sentences is the best option.

mkllnk · 2018-07-23T02:39:59Z

app/controllers/application_controller.rb

@@ -5,6 +5,7 @@ class ApplicationController < ActionController::Base

  prepend_before_filter :restrict_iframes
  before_filter :set_cache_headers # Issue #1213, prevent cart emptying via cache when using back button
+  before_filter :set_show_cookie_banner


Can we move this into a helper instead of adding code to the application controller? I don't think you need to persist the state and introduce a new variable.

Do you mean we should move the method set_show_cookie_banner into a separate file app/helpers/cookies_banner_helper.rb?
In the ApplicationController we would still need the following 2 lines:

helper CookiesBannerHelper

before_filter :set_show_cookie_banner or helper_method :set_show_cookie_banner
Is this what you mean?

I am not sure what you mean with the "need to persist". Do you mean I should access request.cookies from the view directly? I don't think that would be good.

Thanks!

Sorry for not being clear. I meant to remove all your additions from the ApplicationController and add a helper like this:

def show_cookie_banner? request.cookies['cookies_consent'].nil? end

I find that a lot simpler than callbacks and an instance variable. But I'm also not the biggest rails pro. Are there other opinions, @Matt-Yorkley @sauloperez?

Personally, I'm not a fan of callbacks either and I dislike having ivars in views. Both lead to a global state that is very hard to manage in the long run.

I would go with @mkllnk suggestion and call that show_cookie_banner? straight from the view.

awesome, thanks for the advise guys. I have removed everything from applicationcontroller and also removed a few things from api/cookies_consent_controller and added the cookies logic to the new and beautiful cookies_consent_helper.

It feels this helper is ok like this BUT it's quite obvious this should not be a helper, it should be a Service. Where would our service layer live? In this specific case, for example, where would a service like CookiesConsentService be in terms of folders?
Thanks!

There is currently the choice between app/services and lib. We haven't agreed yet which approach is better. And while you are right that application logic should go into a service object, I find it okay to leave a one-liner in the helpers. You need the helper anyway to call the service object and pass on the request variable.

mkllnk · 2018-07-23T02:42:35Z

app/views/shared/_footer.html.haml

              = t :footer_legal_data_cookies_policy
+        %p.text-small
+          %a{ng: { controller:'CookiesBannerCtrl', click: 'deleteCookiesConsent()' }}
+            = 'delete consent cookie (for manual testing only)'


This doesn't look like something that should be in production code. Did you forget to remove it?

I didn't forget about this. I just thought we would need this for manual testing. So that manual testers could delete the consent_cookie. BUT I just realised it's extremelly easy to delete a cookie in chromes dev tools on the "Application" tab. I have deleted this code now.

I also find it extremely useful to open a private window, test something and then close it again. All cookies of the private window are gone after closing it. You can even do that on mobile.

mkllnk · 2018-07-23T02:45:26Z

app/overrides/spree/admin/general_settings/edit/legal_settings.html.haml.deface

+/ insert_after "fieldset.security"
+
+%fieldset.legal.no-border-bottom
+  %legend{:align => "center"}= t(:legal_settings)


Have you tried to use lazy lookup in deface files? It would be good to know if that works. I would just do t('.legal_settings') and then check the error message to see where it expects the key to be. We can then move all these strings into the right scope.

yes, it works. Thanks for the hint!
I have added it to "add translations' here: https://github.com/openfoodfoundation/openfoodnetwork/wiki/Internationalisation-(i18n)#development

I am now using lazy loading for these translations of the legal settings 👍

mkllnk · 2018-07-23T02:56:37Z

app/assets/javascripts/darkswarm/cookies_banner/cookies_banner.html.haml

+.row
+  .large-9.columns
+    %p
+      {{ 'legal.cookies_banner.cookies_usage' | t}}


You are right. I just checked and this is correct. And since this is an Angular template, it needs to be translated through the t filter. Somehow my computer is not re-compiling the locale for Javascript and I get the missing translations. But that's not your fault. ;-)

mkllnk · 2018-07-23T05:59:52Z

I found that the layout is not ideal for every window size. It is possible that the accept button is not visible and I can't scroll.

luisramos0 · 2018-07-23T18:38:39Z

Hi @mkllnk thanks again for you re-review :-D

I have fixed the layout issue by improving the media queries.

sauloperez · 2018-07-23T18:05:15Z

app/assets/stylesheets/darkswarm/cookies_banner.css.scss

+//   @media only screen and (max-width: 400px) {
+//    overflow: hidden !important;
+//  }
+//}


Any reason to keep this comments? I'd remove them.

sauloperez · 2018-07-24T09:25:36Z

app/controllers/api/cookies_consent_controller.rb

+    end
+
+    def show
+      render json: { cookie_consent: !cookies[@cookie_name].nil? }


the key? method might be a bit easier to understand than nil? plus a negation.

Yes, thanks. Done. #learning_ruby

Of course! and Ruby has plenty of methods like this one in its standard library.

sauloperez · 2018-07-24T09:29:14Z

app/controllers/api/cookies_consent_controller.rb

+    def initialize
+      super
+      @cookie_name = 'cookies_consent'
+    end


I wouldn't override the constructor to avoid coupling with Rails internals. Keep in mind it belongs to the framework and it might change in future versions and receive any kind of low-level optimizations that could break this controller.

Also, I think what you're trying to achieve is better suited as a constant. Like

COOKIE_NAME = 'cookies_consent'.freeze

Yes, thanks. Done. #learning_ruby

sauloperez · 2018-07-24T09:34:45Z

app/controllers/application_controller.rb

@@ -5,6 +5,7 @@ class ApplicationController < ActionController::Base

  prepend_before_filter :restrict_iframes
  before_filter :set_cache_headers # Issue #1213, prevent cart emptying via cache when using back button
+  before_filter :set_show_cookie_banner


Personally, I'm not a fan of callbacks either and I dislike having ivars in views. Both lead to a global state that is very hard to manage in the long run.

I would go with @mkllnk suggestion and call that show_cookie_banner? straight from the view.

luisramos0 · 2018-07-24T19:46:55Z

So... in conversation with Pau, I thought that this would be a wonderful opportunity to introduce the Rails Engines strategy to modularize OFN (see here).

In the context of this PR, that means we would see the birth of the first OFN engine (or OFN module) called Web (see discourse above for names of the modules) that would contain, for now, some of the cookies logic required for this feature.
The current branch includes the new Rails Engine Web.

@mkllnk @Matt-Yorkley @sauloperez @enricostano @HugsDaniel
I think this would be super awesome progress in the right direction. If this works, I could move most of the code of this PR to the new module already, including views, controllers and javascript! But I need someone's help to make it work :-D

Thanks in advance for your help!

UPDATE:
In this branch, for now, I moved the cookiesBanner (angular code) and cookiesConsent (route and api controller) to the new Web Engine. This is where they will be. For now I left the cookiesBanner and cookiesConsent in the main app but RENAMED to cookYsBanner and cookYsConsent.
There are 3 parts that need to work with the new engine:

the footer.html is now using the working cookYsBanner directive but WILL use the cookiesBanner directive defined in the Web engine (we need to fix the packaging of the Web engine angular module) - TODO - it's not working right now
the footer is using the working CookYsConsent service but WILL use the CookiesConsent service on the engine - TODO - it's not working right now
the api route on the main app "/api/cookys/consent" is working but the one on the engine is not "/api/cookies/consent" - TODO - the routes mount isn't working

…age, cookies banner and privacy policy link

…page to improve UX on mobile

…es from the server, instead of precompiled assets. This was used to fix a problem with the cookies policy page template that was using a Spree configuration

This avoids asset compile errors like this one: Sass::SyntaxError: Undefined variable: "$disabled-light".

mkllnk · 2018-08-14T00:08:08Z

Staged on https://staging1.openfood.com.au/.

myriamboure · 2018-08-15T09:45:23Z

Ok, so I couldn't prevent myself from testing that even if I'm on holidays ;-)
https://docs.google.com/document/d/1Bz9nnK0raTLi-51O6IgrKkNT8nPInPpMexv62xov2Qc/edit#

I saw 4 things to improve before we can consider the feature as done, but none of them for me prevents merging, so I propose to merge and open issues for the 4 following issues:
1- On desktop, the size of the cookie banner text is pretty small, doesn't seem to adapt to the size of the screen. It should take more space in the grey banner. (to review and agree with UX person before moving forward).
2- More annoying, the mobile experience has a bug: when I accept cookie policy by clicking on "accept cookies", then click on cookie policy modal page, when I close the page the banner displays again and I have to accept again. This happens only on mobile.
3- Another issue on mobile experience: the cookie policy modal page is both hard to scroll (strange feeling of having to scroll twice for the scrolling to be reactive, and scrolling happens only in half screen, all that is really strange), and the table inside is unreadable. We need to replace the modal page by a real page that opens in a new tab to make the experience good, AND we need to review the table inside, present it differently so that it can be responsive and readable on mobile.
4- The GA/Matomo conditional text on cookie policy page needs to be updated. GA text has not been updated to latest version, and for Matomo we had agreed first to put it in Essential cookies. No objection to put it in Statistics cookies section, but we need to review the text, say that data are anonymized, etc. I will describe precisely what is to changed when I open the issue to improve that.

I propose to merge this, and when I'm back from holidays I will good deeper in each things to improve and open the issues so that we finish the feature. Meanwhile @luisramos0 now that the Matomo setup and this are going to be merged you can add the opt-out option from Matomo ;-)

Can we make sure that default behavior is that checkbox "display cookie banner" is disabled before merging? I wouldn't want the Australian to have a banner popping up that they don't want... @mkllnk @luisramos0 @Matt-Yorkley I had a doublt while testing as by default I could see the banner first time I landed on staging. Please check before merging 🙏

luisramos0 · 2018-08-15T16:55:41Z

Awesome, thanks a lot @myriamboure

1- yes, we can increase font size
2- I can see it on desktop as well. my bug. It's a very easy fix (Ive fixed it locally now).
3- I see the modal is not taking up all screen height. I can make it full height, it's very easy to do it, I just need to test the other modals: registration and sign up. I am not sure how a normal page will make it better in terms of ux apart from making it full screen.
re the table, I didnt want to change it too much because it was in the spec. In mobile this should be a paragraph for each cookie, isnt it? Anyway, not the best solution for sure but the table is horizontally scrollable :-/ btw, making the modal a normal page will not fix this table issue, the screen will still be the same size.
4- yes, I think I wrote the Matomo text. Sorry, I should have asked for details before. Yes, we should move the conditional Matomo text to the essential section. Please let me know how you want it and I will change.

re default value, good point :-D
I checked the code and default is false - disabled. Maikel must have changed it on the BO after staging. Right @mkllnk ?

mkllnk · 2018-08-16T01:10:51Z

app/controllers/api/cookies_consent_controller.rb

+      render json: { cookies_consent: cookies_consent.exists? }
+    end
+
+    def create


@luisramos0 Why do we request the server to set a cookie? Can't we just do it via Javascript? https://docs.angularjs.org/api/ngCookies/service/$cookies That could be a lot simpler and faster.

An e-commerce website should only use httponly cookies that cannot be set by the browser. It's a simple security measure to avoid a million types of XSS attacks. That's why you should always go around to the server like here.
BUT in the end, I forgot to actually set it to httponly, which is as simple as adding a line like this to the cookie setting statement:
:secure => true

Maybe I should create a new issue to make this a secure cookie? What do you think?

Wow, thank you for that insight. Even though it's not crucial for this cookie, we should definitely use HttpOnly cookies everywhere. Go ahead and create that issue.

Issue #2554 with PR #2555 in code review.

mkllnk · 2018-08-16T01:13:06Z

Maikel must have changed it on the BO after staging.

I didn't, but someone must have done it. I just re-staged which is resetting the database and I can't see a banner. Should be all good!

mkllnk · 2018-08-16T01:21:12Z

I just deployed that to Aus production. No cookie banner visible. :-)

sigmundpetersen · 2018-08-16T06:54:14Z

So can we close off all of #2401 #2402 #2406 #2407 then? Or are there more to any of them left undone? @luisramos0 @mkllnk

luisramos0 · 2018-08-16T20:13:17Z

Hurray, live!
Thanks @mkllnk for the support and apologies for the giant PR...

@sigmundpetersen That's it. They should all be done and closed (apart from the points mentioned by Myriam that will be handled on separate new issues). I'd let Myriam confirm and close those issues when she is back.

kristinalim · 2018-08-19T06:37:21Z

app/assets/stylesheets/darkswarm/cookies_banner.css.scss

+  background: $dark-grey;
+  bottom: 0;
+  position: fixed;
+  top: 20vh !important;


What browser versions do we aim to support? I happened to check this recently: https://caniuse.com/#feat=viewport-units

good point. I used it because it was already being used somewhere else!
Maybe we switch to %? https://github.com/openfoodfoundation/openfoodnetwork/wiki/Code-Conventions
Feel free to add stuff to this page above. You can add rules you think we should follow and then we discuss/validate.

I don't think we have a set of supported browsers. It would be good to create that list though. I'll post something in Slack.

myriamboure · 2018-08-28T07:06:47Z

@luisramos0

2- I can see it on desktop as well. my bug. It's a very easy fix (Ive fixed it locally now).

Did you fix it already? I was not sure while reading it. Let me know I'll open an issue if not fixed yet.

myriamboure · 2018-08-28T09:07:00Z

Ok I opened the remaining issues, just didn't table modal as it seems pretty ok actually when I retested, l et's see after Kristina has finished the work she is doing on modal how it looks like for the cookie policy page ;-)

luisramos0 · 2018-08-28T16:24:34Z

ok, thanks @myriamboure
1 - handled in #2597
2 - handled in #2599
3 - re the modal on mobile, I agree, making the modal 100% height will make a big difference. And also removing the table (or making it a single column) with #2601 will improve it a lot.
4 - handled on #2601

We also have:

the matomo optout part in [Cookies] Add Matomo opt out iframe to the cookies policy page #2518
and finally user and admin documentation in [Cookies] Document technical choices in wiki #2427 and [Cookies] Document feature use in super admin handbook gitbook #2428

Can we close this PR and move the overall conversation, if any, to the epic #2242?

myriamboure · 2018-08-28T16:36:39Z

It is merged already, so let's discuss on epic now yes ;-) The list you made is on the epic btw, if you look at all issues attached below main thread ;-)

luisramos0 · 2018-08-28T18:58:38Z

yes, zenhub is nice :-) I like to do these summaries for clarity.

luisramos0 self-assigned this Jun 27, 2018

luisramos0 force-pushed the cookies_banner branch 6 times, most recently from 9b45b42 to 195ba5e Compare June 28, 2018 15:29

mkllnk requested changes Jul 4, 2018

View reviewed changes

luisramos0 force-pushed the cookies_banner branch from 09eb341 to 160bfb9 Compare July 20, 2018 15:22

luisramos0 force-pushed the cookies_banner branch from 160bfb9 to fda9018 Compare July 20, 2018 17:15

luisramos0 force-pushed the cookies_banner branch 3 times, most recently from 07df781 to 83f5d8d Compare July 20, 2018 22:24

This was referenced Jul 20, 2018

added cookies policy link to footer and the cookies policy modal #2405

Closed

EU Cookie compliance #2242

Closed

luisramos0 force-pushed the cookies_banner branch from 83f5d8d to b6d463b Compare July 20, 2018 23:26

mkllnk reviewed Jul 23, 2018

View reviewed changes

luisramos0 force-pushed the cookies_banner branch 4 times, most recently from 9ab5a51 to 3e6ff42 Compare July 23, 2018 18:32

sauloperez suggested changes Jul 24, 2018

View reviewed changes

luisramos0 force-pushed the cookies_banner branch 2 times, most recently from 278670b to 5d4b7f5 Compare July 24, 2018 19:22

luisramos0 force-pushed the cookies_banner branch from 5d4b7f5 to d6be91e Compare July 25, 2018 11:04

luisramos0 and others added 5 commits August 14, 2018 10:01

Added functional tests for cookies related features: cookies policy p…

bff5581

…age, cookies banner and privacy policy link

Added a button to close the cookies policy page at the bottom of the …

1c20967

…page to improve UX on mobile

Added angular templates route and controller to serve angular templat…

ccd5147

…es from the server, instead of precompiled assets. This was used to fix a problem with the cookies policy page template that was using a Spree configuration

Fixed CodeClimate scss feedback on cookies banner and policy page css

8681618

Import required colour definitions in SCSS

c986253

This avoids asset compile errors like this one: Sass::SyntaxError: Undefined variable: "$disabled-light".

mkllnk force-pushed the cookies_banner branch from dfac7a7 to c986253 Compare August 14, 2018 00:02

mkllnk added the pr-staged-au staging.openfoodnetwork.org.au label Aug 14, 2018

myriamboure assigned myriamboure and unassigned RachL Aug 15, 2018

myriamboure removed their assignment Aug 15, 2018

mkllnk reviewed Aug 16, 2018

View reviewed changes

oeoeaio merged commit c986253 into openfoodfoundation:master Aug 16, 2018

luisramos0 deleted the cookies_banner branch August 16, 2018 20:14

daniellemoorhead mentioned this pull request Aug 17, 2018

[Cookies] Create cookie policy page #2401

Closed

mkllnk removed the pr-staged-au staging.openfoodnetwork.org.au label Aug 17, 2018

mkllnk mentioned this pull request Aug 17, 2018

[Cookies] Intermittently failing cookies spec #2553

Closed

luisramos0 mentioned this pull request Aug 17, 2018

[Cookie] Use secure HttpOnly cookies for cookie_consent cookie #2554

Closed

kristinalim reviewed Aug 19, 2018

View reviewed changes

New Cookies banner #2411

New Cookies banner #2411

Conversation

luisramos0 commented Jun 27, 2018 • edited by mkllnk Loading

What? Why?

What should we test?

Cookies Policy Modal

Cookies Banner

Modals

Release notes

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisramos0 Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisramos0 Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisramos0 Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisramos0 Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisramos0 commented Jul 20, 2018 • edited Loading

luisramos0 commented Jul 20, 2018

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkllnk commented Jul 23, 2018

luisramos0 commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisramos0 commented Jul 24, 2018 • edited Loading

luisramos0 commented Jun 27, 2018 •

edited by mkllnk

Loading

luisramos0 Jul 20, 2018 •

edited

Loading

luisramos0 Jul 20, 2018 •

edited

Loading

luisramos0 Jul 20, 2018 •

edited

Loading

luisramos0 Jul 20, 2018 •

edited

Loading

luisramos0 commented Jul 20, 2018 •

edited

Loading

luisramos0 commented Jul 24, 2018 •

edited

Loading