[#2718] Migrate cookie consent from Klaro to Orejime #3199
Conversation
|
Hi @AndreaBarbasso, I tried to test the PR but was unsuccessfull. Maybe there is something I missed. I test through Docker. When I click on "Cookie settings" in the footer, the pop up is not display and the page "frozes" (cannot be scrolled anymore), and an error occurs: |
|
Hi @AndreaBarbasso, |
There was a problem hiding this comment.
@AndreaBarbasso : We gave this a group code review in today's Developers Meeting. Overall, the changes here look reasonable. The PR needs a rebase though as it's still using yarn and we've recently switched to npm (see #3173 )...so the yarn.lock no longer exists.
I think this may need more testing though to see what happens if a site sets enablePrivacyStatement: false in their config.yml. It appears this code is now ignoring that setting...which seems like it could be a bug (as the privacy statement itself will return a 404 when that is set to false). We may need to determine if there's a way to still support enablePrivacyStatement: false with Orejime.
| */ | ||
| initialize() { | ||
| if (!environment.info.enablePrivacyStatement) { | ||
| delete this.klaroConfig.privacyPolicy; | ||
| this.klaroConfig.translations.zz.consentNotice.description = 'cookies.consent.content-notice.description.no-privacy'; |
There was a problem hiding this comment.
This seems to be removing the ability to disable the Privacy Statement in DSpace. See the docs at https://wiki.lyrasis.org/display/DSDOC7x/User+Interface+Configuration#UserInterfaceConfiguration-Toggleend-useragreementandprivacypolicy
What happens if someone tries to use this PR when enablePrivacyStatement: false is set in your config.prod.yml?
| "cookies.consent.app.disable-all.description": "Use this switch to enable or disable all services.", | ||
|
|
||
| "cookies.consent.app.disable-all.title": "Enable or disable all services", | ||
|
|
There was a problem hiding this comment.
Just a note for other developers. Initially, the removal of all these keys from json5 files looked confusing to me. But, I see that you are removing keys that are no longer used in Orejime. So, I think this cleanup is correct.
this was happening because of hidden apps not being removed from category arrays.
|
@pilasou, I managed to solve the problem you encountered. It was happening if a cookie category referenced (by name) at least one app that had been removed from the app list. Thanks! @tdonohue, I managed to keep the current behavior regarding the Right now, we have another situation going on. Quoting the Orejime docs:
So, in a case like the one we can see on the sandbox (where all cookies are mandatory), Orejime will not show any cookie notice on the bottom right corner of the page. What I can see as possible options, right now, are:
Any other idea on how to proceed on this issue? |
|
Hi @AndreaBarbasso, just want to mentionned that I retested the cookies consent and it works as described. The cookie setting window does not show by default as all default cookies are mandatory. I can display the window by clicking the Cookies setting link in the page footer as shown below: All toogles are on by default and cannot be changed, |
|
@AndreaBarbasso : We discussed this in today's Developer Meeting. Those in attendance believe that Orejime's default behavior should be GDPR compliant. In other words, users need to only be notified if there are non-mandatory cookies. So, this PR's current behavior sounds like it should be OK. One final note. It appears this PR is currently failing some tests/specs |
|
Hi @AndreaBarbasso, |
# Conflicts: # package-lock.json # package.json # src/assets/i18n/ar.json5
|
Hi @AndreaBarbasso, |
# Conflicts: # package-lock.json
There was a problem hiding this comment.
@AndreaBarbasso : Thanks again for this PR! Overall this is looking good and it works well. However, I've run into a few minor issues noted inline below. The main issue is that the anonymous cookie is currently named orejime and not orejime-anonymous. But, it might just be a configuration issue with Orejime (see comments below)?
The other issue I've run into is that, with this PR installed, if I run npm run clean followed by npm install, then my package-lock.json is modified slightly. This implies to me that this PR hasn't properly updated package-lock.json...so you may want to see if it also changes on your end when your reinstall all dependencies (if so, then you need to commit those changes to this PR).
Beyond that, I've verified that this is working properly, and that it also works correctly when Google Analytics and/or reCaptcha are enabled (or disabled). Also tested that it works correctly if the privacy statement is disabled (or enabled).
So, overall, I'm +1, but the minor bugs still need to be solved before this can be merged.
| --button-text-color-cookie: #333; // This variable represents the text color for buttons in the Klaro cookie banner | ||
|
|
||
| --green1: #1FB300; // This variable represents the success color for the Orejime cookie banner | ||
| --button-text-color-cookie: #fff; // This variable represents the text color for buttons in the Orejime cookie banner |
There was a problem hiding this comment.
This change to using white text for the button results in an accessibility error for color contrast. The background of the "Save" button is #1fb300 and the text is #ffffff, which gives it a color contrast ratio of only 2.79:1. The required ratio to pass is 4.5:1
I think this would be solved if you revert this change and use #333333 instead.
The other option, if you wanted to keep the white text is to make the green1 darker. It just needs to have better contrast, so that it won't fail accessibility checks.
(NOTE: I was able to see this failure by opening the "Cookie settings" on the homepage and running the Chrome "axe DevTools" plugin on the page. It returned that color contrast error on the "Save" button.)
cypress/support/e2e.ts
Outdated
| // This just ensures it doesn't get in the way of matching other objects in the page. | ||
| cy.setCookie('klaro-anonymous', '{%22authentication%22:true%2C%22preferences%22:true%2C%22acknowledgement%22:true%2C%22google-analytics%22:true%2C%22google-recaptcha%22:true}'); | ||
| cy.setCookie('orejime-anonymous', '{%22authentication%22:true%2C%22preferences%22:true%2C%22acknowledgement%22:true%2C%22google-analytics%22:true%2C%22google-recaptcha%22:true}'); |
There was a problem hiding this comment.
I believe the anonymous orejime cookie is just named "orejime" (at least that's what I'm seeing in my cookies locally).
So, this line may need to replace 'orejime-anonymous' with 'orejime'. That might also fix the tests failures you are seeing, as I think these e2e tests are setting the wrong cookie, and the test failures are caused by the Orejime cookie popup getting in front of the page content.
| @@ -247,8 +247,8 @@ export class RegisterEmailFormComponent implements OnDestroy, OnInit { | |||
| * Return true if the user has accepted the required cookies for reCaptcha | |||
| */ | |||
| isRecaptchaCookieAccepted(): boolean { | |||
| const klaroAnonymousCookie = this.cookieService.get('klaro-anonymous'); | |||
| return isNotEmpty(klaroAnonymousCookie) ? klaroAnonymousCookie[CAPTCHA_NAME] : false; | |||
| const orejimeAnonymousCookie = this.cookieService.get('orejime-anonymous'); | |||
There was a problem hiding this comment.
Same here, the anonymous cookie seems to be named orejime.
|
|
||
| export function getOrejimeConfiguration(_window: NativeWindowRef): any { | ||
| return { | ||
| storageName: ANONYMOUS_STORAGE_NAME_OREJIME, |
There was a problem hiding this comment.
Based on the comments above, I don't believe this setting is working. It's also not listed in the Orejime config reference, so it may not be valid. https://github.com/empreinte-digitale/orejime?tab=readme-ov-file#configuration
Maybe it should be removed?
UPDATE: It looks like Orejime renamed this to be cookieName. That might be why it's not working. You could try changing this to be cookieName, as that might also result in the anonymous cookie being named orejime-anonymous. But, we need to also verify that when you login the cookie remains named orejime-[eperson-uuid]
There was a problem hiding this comment.
@tdonohue, good catch! With that I can see both orejime-anonymous and orejime-!
But tests are still failing, I'm on them.
There was a problem hiding this comment.
@AndreaBarbasso : Thanks for the updates! Overall, this is looking better now, but I've stumbled upon an odd bug. When I'm logged in, I cannot seem to ever update my saved cookie settings.
I'm not sure if this is something on my end, or if it can be reproduced, but this is what I'm seeing:
- First, I'm starting with both Google Analytics & Google ReCaptcha enabled, so that there are (optional) settings in Orejime that can be turned off.
- In an incognito window, I'm accessing the site as an anonymous user & accepting all cookies. This creates an
orejime-anonymouscookie with all settings saved as "true". - Now, if I login as a user, this creates an
orejime-[uuid]cookie with my current saved preferences (in my case, they are all set to "true" again). - While I'm still logged in, I click on "Cookie settings" in the footer and change my settings for Orejime so that Google Analytics is not allowed. This is where the bug occurs, because my (authenticated)
orejime-[uuid]cookie does NOT update. However, theorijime-anonymouscookie DOES UPDATE (and shows Google Analytics set to "false")- Since I'm logged in, I should no longer be using the
orijime-anonymouscookie, and instead I should be using theorijime-[uuid]cookie.
- Since I'm logged in, I should no longer be using the
I'm not sure if there's a bug in our code, or perhaps the Orejime configuration, or maybe Orejime acts differently and always tries to save your updates to the Cookie specified in the cookieName setting (which we updated to be orijime-anonymous)?
But, I've verified this behavior does NOT occur with Klaro. If I try the same actions using Klaro, then updating my "Cookie settings" while logged in will result in the klaro-[uuid] cookie being updated correctly.
Overall, I think this PR is looking great. But, I feel this behavior seems buggy. I haven't figured out what could be causing it though.
|
@tdonohue, I fixed the issue you encountered and another issue that was correlated to it - regarding the encoding of characters in the cookie - that caused errors in the |
There was a problem hiding this comment.
👍 Thanks @AndreaBarbasso ! Retested this today, and it appears to all be working well. It looks like the behavior is the same as Klaro (found no obvious differences).
Merging immediately (as this was tested/approved above as well). If we find later bugs we can resolve them in a follow-up PR.
References
Description
Migrated the cookie consent functionality from Klaro to Orejime, because of better accessibility and maintenance.
Instructions for Reviewers
All references to Klaro have been renamed to Orejime. Klaro has been removed from the
package.jsonfile and its config has been modified to be accepted by Orejime (which needs a similar, but not quite the same, config file).Since Orejime needs a URL for the privacy policy, this is now mandatory.
Orejime does not allow raw HTML as text, so the i18n files have been changed accordingly.
Orejime has less strings than Klaro, so a lot of i18n strings are now obsolete and have been removed.
List of changes in this PR:
To review this PR. try to use the Orejime's cookie banner as if it was the Klaro's one. Remember to add a cookie to the configuration in order to test the banner when there is a new cookie to accept/decline.
You can also see that the focus is now starting on the cookie consent toast when using a screen reader.
screen_reader_orejime_video.mp4
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).yarn lintyarn check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.