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

✨ [#1537] Adding cookie-consent banner #681

Merged
merged 5 commits into from
Aug 1, 2023

Conversation

jiromaykin
Copy link
Contributor

@jiromaykin jiromaykin commented Jun 19, 2023

Cookie-banner that respects the GDPR privacy laws, and gives the user the chance to dismiss the non-essential cookies only.
https://taiga.maykinmedia.nl/project/open-inwoner/task/1537

IF the Admin does NOT want to use the cookiebanner, THEN analytics cookies need to be set to true.
IF Admin wants to use the banner, THEN analytics need to be set to false when cookies are rejected by the local user, and to true when approved. + Analytics must be on by default, unless there is a cookiebanner set.

  • style modal pop-up to be open onLoad
  • make cookiebanner optional in config (not visible)
  • add default Admin configuration texts (Scroll down to Cookie consent in http://127.0.0.1:8000/admin/configurations/siteconfiguration/)
  • reject all or accept all approve/dismiss
  • remember user choice across context/all pages for 90days(?) - this means the cookiebanner won't show for 90 days when it is clicked, but it will come back again due to the expirationTime in JS
  • make reject script actually reject ALL from Google/Matomo/Siteimprove
  • make the cookiebanner work for failing tests (error "intercepts pointer events... retrying click action" => ERROR: test_action_status (failing tests: 1. file: open_inwoner/accounts/tests/test_action_views.py, 2. test: open_inwoner/accounts/tests/test_action_views.py 'test_action_status' 3. file: open_inwoner/cms/cases/tests/test_htmx.py 4. htmx: test_cases, 5. file open_inwoner/plans/tests/test_views.py, 6. test "test_action_status")
  • add migrations

Challenge: (not in this issue) not making the cookie-banner appear on the Privacy policy page? Right now you first have to click it it in order to be able to read it; (if the cookie-banner is shown, it has to block all other interactions).

@jiromaykin jiromaykin force-pushed the feature/1537-add-cookie-consent-banner branch 2 times, most recently from bd1209d to 900d9dd Compare June 22, 2023 08:47
@codecov-commenter
Copy link

codecov-commenter commented Jun 25, 2023

Codecov Report

Merging #681 (34607a6) into develop (5ea47fc) will decrease coverage by 0.04%.
The diff coverage is 67.74%.

@@             Coverage Diff             @@
##           develop     #681      +/-   ##
===========================================
- Coverage    96.20%   96.16%   -0.04%     
===========================================
  Files          642      643       +1     
  Lines        22934    22959      +25     
===========================================
+ Hits         22064    22079      +15     
- Misses         870      880      +10     
Impacted Files Coverage Δ
...c/open_inwoner/accounts/tests/test_action_views.py 86.84% <0.00%> (-1.95%) ⬇️
src/open_inwoner/cms/cases/tests/test_htmx.py 55.72% <0.00%> (-2.22%) ⬇️
src/open_inwoner/configurations/admin.py 100.00% <ø> (ø)
src/open_inwoner/utils/context_processors.py 90.90% <ø> (ø)
...igrations/0046_siteconfiguration_cookie_consent.py 100.00% <100.00%> (ø)
src/open_inwoner/configurations/models.py 97.72% <100.00%> (+0.10%) ⬆️
...pen_inwoner/configurations/tests/test_analytics.py 100.00% <100.00%> (ø)
src/open_inwoner/utils/templatetags/utils.py 90.90% <100.00%> (+0.90%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jiromaykin jiromaykin force-pushed the feature/1537-add-cookie-consent-banner branch 7 times, most recently from eb7551f to f367f91 Compare June 29, 2023 15:45
@jiromaykin jiromaykin added the help wanted Extra attention is needed label Jul 3, 2023
@jiromaykin jiromaykin added wip Work in progress and removed help wanted Extra attention is needed labels Jul 6, 2023
@jiromaykin jiromaykin force-pushed the feature/1537-add-cookie-consent-banner branch 2 times, most recently from 0d811dc to 4148b91 Compare July 10, 2023 07:41
@jiromaykin jiromaykin removed the wip Work in progress label Jul 10, 2023
@jiromaykin jiromaykin marked this pull request as ready for review July 10, 2023 07:42
@jiromaykin
Copy link
Contributor Author

jiromaykin commented Jul 10, 2023

@Bartvaderkin and @pi-sigma I don't know too much about writing tests yet (so a review of what I did in accounts/tests/test_action_views.py and in cms/cases/tests/test_htmx.py is much appreciated) + and I am not sure if my way of adding context-processors (in order to configure the banner from the admin) is the correct way to do it.... so feel free to leave feedback. for improvement.

@jiromaykin jiromaykin force-pushed the feature/1537-add-cookie-consent-banner branch from d0f4a70 to 34607a6 Compare July 18, 2023 07:49
cookiebanner_rejectbutton.click()
# check cookie-banner state
expect(cookiebanner_banner).to_be_hidden()

# find action element and the dropdown widget
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. The test is for the action view, which has nothing specifically to do with cookies. This would need to be a separate test.
  2. If we're going to test the hiding of the cookie banner, we should also test that it's hidden when the 'accept' button is clicked.

However, I'm not sure this is even needed. We can confirm manually that the banner is hidden on click. What we do need to check automatically is that the analytics scripts are not rendered when the cookie is not set. You should modify the test in src/open_inwoner/configurations/tests/test_analytics.py (I should have done this myself when I changed the test, but didn't think of it back then).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pi-sigma I didn't know what to do here, because this test was failing when I added the cookiebanner, because the cookiebanner is blocking all other clicks when it pops up so I add something-that-clicks-something but I definitely don't know what I am doing here (I would prefer to set " self.cookiebanner_enabled" to False but that didn't work.)

cookiebanner_rejectbutton.click()
# check cookie-banner state
expect(cookiebanner_banner).to_be_hidden()

# expected anchors
Copy link
Contributor

Choose a reason for hiding this comment

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

See previous comment

@alextreme
Copy link
Member

Discussed with Jiro and Paul:

  • test needs to be added to check that the analytics aren't used if the cookies aren't accepted
  • test_action_views needs to be modified, expect-changes about the cookiebanner should be removed (it should work without or be mocked)

@jiromaykin jiromaykin force-pushed the feature/1537-add-cookie-consent-banner branch 3 times, most recently from b2ff3d0 to 52c400b Compare July 25, 2023 13:55
def setUp(self):
self.config = SiteConfiguration.get_solo()
cms_tools.create_homepage()
self.config.cookie_info_text = ""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pi-sigma I've added the new tests here. Question: I am assuming that if cookie_info_text is set to a string with content here, instead of empty, the tests below should actually be failing since analytics should then be off by default?
All other behaviour behaves conforming to the assertions.

@jiromaykin jiromaykin added the help wanted Extra attention is needed label Jul 27, 2023
@jiromaykin jiromaykin force-pushed the feature/1537-add-cookie-consent-banner branch from 52c400b to ec1e266 Compare August 1, 2023 08:18
@alextreme
Copy link
Member

3 failing tests:

2023-08-01T09:02:21.1027134Z ERROR: test_action_status (open_inwoner.accounts.tests.test_action_views.ActionsPlaywrightTests)
2023-08-01T09:02:21.1236333Z ERROR: test_cases (open_inwoner.cms.cases.tests.test_htmx.CasesPlaywrightTests)
2023-08-01T09:02:21.1397757Z ERROR: test_action_status (open_inwoner.plans.tests.test_views.PlanActionStatusPlaywrightTests)

@jiromaykin jiromaykin force-pushed the feature/1537-add-cookie-consent-banner branch 2 times, most recently from 41d01a2 to a4264ed Compare August 1, 2023 09:46
@pi-sigma pi-sigma force-pushed the feature/1537-add-cookie-consent-banner branch from a4264ed to 958298b Compare August 1, 2023 14:16
@@ -325,7 +336,9 @@ def setUp(self) -> None:
)
self.action_list_url = reverse("profile:action_list")

def test_action_status(self):
def test_action_status(self, mock_solo):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! @pi-sigma I had no idea... now this is ready for review again.

@jiromaykin jiromaykin removed the help wanted Extra attention is needed label Aug 1, 2023
@alextreme alextreme self-requested a review August 1, 2023 16:13
@alextreme alextreme merged commit 0492c68 into develop Aug 1, 2023
@alextreme alextreme deleted the feature/1537-add-cookie-consent-banner branch August 1, 2023 16:14
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.

4 participants