-
Notifications
You must be signed in to change notification settings - Fork 0
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 banner and Google Tag Manager snippet #19
Conversation
Hi @kacperpONS. I think you might find (as I did for the prototype) that using https://www.ons.gov.uk/cookies as the management URL will produce some strange inconsistencies, especially in environments that aren't using the On the prototype, we had to add our own version of the https://www.ons.gov.uk/cookies page, following the advice in the design system. You might find that you need the URL and view changes from ONSdigital/dis-wagtail-alpha#112 too. |
Replying to @ababic, we've decided that for the time being the cookies page will not reside in Wagtail and we'll point to the existing Cookies page on the ONS domain. This is noted on the card in Jira CMS-189 The risk that Andy mentions is known and that has been confirmed at today's stand up meeting. 👍 |
A small comment to the above commit: Now that we take the settings for the Cookie banner as env variables rather than a model in the db we won't be able to dynamically change the app settings to test that custom values are being picked up correctly. The app settings are loaded on app startup from the settings file and per Django docs you shouldn’t alter settings in your applications at runtime. I've therefore removed the test checking that the custom values of the env variables have been picked up, and only left the one that checks that sensible/expected default values are set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work @kacperpONS.
Left a few notes and suggestions
^ Comment to the above commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One non-blocking suggestion from me.
Otherwise LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulled down the branch, tested the features locally and the tests all passes. LGTM!
Minor: The branch is out of date with main.
@sanjeevz3009 @zerolab can I get a re-review plz 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Standard_pages app creation * Django models for information page updated and the template for the information page tweaked * Replace `pytest` with Django's built-in runner (#18) * Release calendar pages (#16) * Replace Docker Desktop setup with Colima (#24) * Add Cookie banner and Google Tag Manager snippet (#19) * Analysis page and series (#20) * Addition of equation partials * Repeated code remove, /media added to .gitignore, media file removed * Related pages InlinePanel added. Add Info Template updated. --------- Co-authored-by: Neha <[email protected]> Co-authored-by: Sanjeev <[email protected]>
What is the context of this PR?
We wanted to add the DS cookies banner as part of our base template.
Things that were changed and require some explanation:
Logic for handling the language of the banner
Even though multi-language isn't yet supported, I added placeholder logic for displaying the Banner in Welsh if the LANGUAGE_CODE is set to cy and in English for all other lang codes.
Removed the
Tracking
site setting model and added the migrationAfter the changes in this PR we pass the settings as env variables and don't use the Tracking model anymore to store the values. After consultation with @zerolab, we've decided to remove the model and migrate the database.
A side effect of this change is that when we'll want to change the value of any of the env variables, an app reload will be required (which might require some downtime depending on how we go about it on the infrastructure level)
Old version of Design System
This project is using release 70.0.17 of the Design System, the November prototype is using 72.0.0 and the latest version is 72.0.1.
As a result I had to use an old name of a template variable, that does not appear on the Design System website, so there's currently a mismatch that needs to be noted because it will break the banner when we update the DS version.
In release 72.0.0 of the DS the variable
settingsLinkTextURL
has been renamed tosettingsLinkURL
. In this PR I'm using the old version(See: Update param name settingsLinkTextURL to settingsLinkURL in Cookies Banner component design-system#3300)
How to review
Describe the steps required to test the changes (include screenshots if appropriate).
LANGUAGE_CODE
in settings to "cy" and confirm that the banner is displayed in WelshTesting GTM connection
You can also test that the GTM snippet works correctly by going to https://tagmanager.google.com/, creating an account and retrieving a GTM Container ID and passing it in the Docker-compose file.
As part of this card I've tested this too, see the screenshots below to know what to expect to see in the GTM console.
Follow-up Actions
Link to the Jira card
https://jira.ons.gov.uk/browse/CMS-189