Merge pull request #121 from jazzband/feature/105-add-unique-constraint

Add missing CookieGroup.varname unique constraint

cookie_consent/cache.py

@@ -1,10 +1,11 @@
 # -*- coding: utf-8 -*-
 from django.core.cache import caches
 
-from cookie_consent.conf import settings
+from .conf import settings
+from .models import CookieGroup
 
 CACHE_KEY = "cookie_consent_cache"
-CACHE_TIMEOUT = 60 * 60
+CACHE_TIMEOUT = 60 * 60  # 60 minutes
 
 
 def _get_cache():
@@ -23,20 +24,22 @@ def delete_cache():
     cache.delete(CACHE_KEY)
 
 
+def _get_cookie_groups_from_db():
+    qs = CookieGroup.objects.filter(is_required=False).prefetch_related("cookie_set")
+    return qs.in_bulk(field_name="varname")
+
+
 def all_cookie_groups():
+    """
+    Get all cookie groups that are optional.
+
+    Reads from the cache where possible, sets the value in the cache if there's a
+    cache miss.
+    """
     cache = _get_cache()
-    items = cache.get(CACHE_KEY)
-    if items is None:
-        from cookie_consent.models import CookieGroup
-
-        qs = CookieGroup.objects.filter(is_required=False)
-        qs = qs.prefetch_related("cookie_set")
-        # items = qs.in_bulk(field_name="varname")
-        # FIXME -> doesn't work because varname is not a unique fieldl, we need to
-        # make this unique
-        items = {group.varname: group for group in qs}
-        cache.set(CACHE_KEY, items, CACHE_TIMEOUT)
-    return items
+    return cache.get_or_set(
+        CACHE_KEY, _get_cookie_groups_from_db, timeout=CACHE_TIMEOUT
+    )
 
 
 def get_cookie_group(varname):

cookie_consent/migrations/0003_alter_cookiegroup_varname.py

@@ -0,0 +1,32 @@
+# Generated by Django 4.2.13 on 2024-05-09 19:01
+
+import re
+
+import django.core.validators
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("cookie_consent", "0002_auto__add_logitem"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="cookiegroup",
+            name="varname",
+            field=models.CharField(
+                max_length=32,
+                unique=True,
+                validators=[
+                    django.core.validators.RegexValidator(
+                        re.compile("^[-_a-zA-Z0-9]+$"),
+                        "Enter a valid 'varname' consisting of letters, numbers, underscores or hyphens.",
+                        "invalid",
+                    )
+                ],
+                verbose_name="Variable name",
+            ),
+        ),
+    ]

cookie_consent/models.py

@@ -6,8 +6,6 @@
 from django.db import models
 from django.utils.translation import gettext_lazy as _
 
-from cookie_consent.cache import delete_cache
-
 COOKIE_NAME_RE = re.compile(r"^[-_a-zA-Z0-9]+$")
 validate_cookie_name = RegexValidator(
     COOKIE_NAME_RE,
@@ -21,6 +19,8 @@
 
 def clear_cache_after(func):
     def wrapper(*args, **kwargs):
+        from .cache import delete_cache
+
         return_value = func(*args, **kwargs)
         delete_cache()
         return return_value
@@ -50,7 +50,10 @@ def update(self, **kwargs):
 
 class CookieGroup(models.Model):
     varname = models.CharField(
-        _("Variable name"), max_length=32, validators=[validate_cookie_name]
+        _("Variable name"),
+        max_length=32,
+        unique=True,
+        validators=[validate_cookie_name],
     )
     name = models.CharField(_("Name"), max_length=100, blank=True)
     description = models.TextField(_("Description"), blank=True)

docs/changelog.rst

@@ -2,6 +2,15 @@
 Changelog
 =========
 
+0.6.0 (unreleased)
+------------------
+
+💥 This feature release has a potential breaking change. The ``CookieGroup.varname``
+field now has a unique constraint on it. If you have duplicate values, this migration
+will crash.
+
+* ...
+
 0.5.0b0 (2023-09-24)
 --------------------
 

testapp/templates/show-cookie-bar-script.html

@@ -21,6 +21,8 @@
       },
       onDecline: () => document.querySelector('body').classList.remove('with-cookie-bar'),
     });
+
+    document.getElementById('loading-marker').style.display = 'inline';
 </script>
 
 <template id="show-share-button">

testapp/templates/test_page.html

@@ -10,6 +10,8 @@
   <body>
     <h1>Test page</h1>
 
+    <span id="loading-marker" style="display:none">page-done-loading</span>
+
     <h2>Social cookies</h2>
     <p>
         sharing button is displayed below only if "Social" cookies are accepted.

tests/test_javascript_cookiebar.py

@@ -22,6 +22,8 @@
 def before_each_after_each(live_server, page: Page, load_testapp_fixture):
     test_page_url = f"{live_server.url}{reverse('test_page')}"
     page.goto(test_page_url)
+    marker = page.get_by_text("page-done-loading")
+    expect(marker).to_be_visible()
     yield
 
 
@@ -72,6 +74,8 @@ def test_on_accept_handler_runs_on_load(page: Page, live_server):
 
     test_page_url = f"{live_server.url}{reverse('test_page')}"
     page.goto(test_page_url)
+    marker = page.get_by_text("page-done-loading")
+    expect(marker).to_be_visible()
 
     share_button = page.get_by_role("button", name="SHARE")
     expect(share_button).to_be_visible()

tests/test_models.py

@@ -1,14 +1,39 @@
 # -*- coding: utf-8 -*-
+from copy import deepcopy
 from unittest import mock
 
+from django.conf import settings
+from django.core.cache import caches
 from django.core.exceptions import ValidationError
-from django.test import TestCase
+from django.test import TestCase, override_settings
 
-from cookie_consent.cache import delete_cache
+from cookie_consent.cache import CACHE_KEY, delete_cache
 from cookie_consent.models import Cookie, CookieGroup, validate_cookie_name
 
+patch_caches = override_settings(
+    COOKIE_CONSENT_CACHE_BACKEND="tests",
+    CACHES={
+        **deepcopy(settings.CACHES),
+        "tests": {
+            "BACKEND": "django.core.cache.backends.locmem.LocMemCache",
+        },
+    },
+)
 
-class CookieGroupTest(TestCase):
+
+class CacheMixin:
+    def populateCache(self):
+        cache = caches["tests"]
+        cache.set(CACHE_KEY, {}, timeout=3600)
+
+    def assertCacheNotPopulated(self):
+        cache = caches["tests"]
+        has_key = cache.has_key(CACHE_KEY)
+        self.assertFalse(has_key)
+
+
+@patch_caches
+class CookieGroupTest(CacheMixin, TestCase):
     def setUp(self):
         self.addCleanup(delete_cache)
 
@@ -26,27 +51,30 @@ def test_get_version(self):
             self.cookie_group.get_version(), self.cookie.created.isoformat()
         )
 
-    @mock.patch("cookie_consent.models.delete_cache")
-    def test_bulk_delete(self, mock_delete_cache):
+    def test_bulk_delete(self):
+        self.populateCache()
+
         deleted_objs_count, _ = CookieGroup.objects.filter(
             id=self.cookie_group.id
         ).delete()
 
         # Deleting a CookieGroup also deletes the associated Cookies, that's why we expect a count of 2.
         self.assertEqual(deleted_objs_count, 2)
-        self.assertEqual(mock_delete_cache.call_count, 1)
+        self.assertCacheNotPopulated()
+
+    def test_bulk_update(self):
+        self.populateCache()
 
-    @mock.patch("cookie_consent.models.delete_cache")
-    def test_bulk_update(self, mock_delete_cache):
         updated_objs_count = CookieGroup.objects.filter(id=self.cookie_group.id).update(
             name="Optional2"
         )
 
         self.assertEqual(updated_objs_count, 1)
-        self.assertEqual(mock_delete_cache.call_count, 1)
+        self.assertCacheNotPopulated()
 
 
-class CookieTest(TestCase):
+@patch_caches
+class CookieTest(CacheMixin, TestCase):
     def setUp(self):
         self.addCleanup(delete_cache)
 
@@ -63,21 +91,23 @@ def setUp(self):
     def test_varname(self):
         self.assertEqual(self.cookie.varname, "optional=foo:.example.com")
 
-    @mock.patch("cookie_consent.models.delete_cache")
-    def test_bulk_delete(self, mock_delete_cache):
+    def test_bulk_delete(self):
+        self.populateCache()
+
         deleted_objs_count, _ = Cookie.objects.filter(id=self.cookie.id).delete()
 
         self.assertEqual(deleted_objs_count, 1)
-        self.assertEqual(mock_delete_cache.call_count, 1)
+        self.assertCacheNotPopulated()
+
+    def test_bulk_update(self):
+        self.populateCache()
 
-    @mock.patch("cookie_consent.models.delete_cache")
-    def test_bulk_update(self, mock_delete_cache):
         updated_objs_count = Cookie.objects.filter(id=self.cookie.id).update(
             name="foo2"
         )
 
         self.assertEqual(updated_objs_count, 1)
-        self.assertEqual(mock_delete_cache.call_count, 1)
+        self.assertCacheNotPopulated()
 
 
 class ValidateCookieNameTest(TestCase):