[#939] Process PR feedback

src/open_inwoner/accounts/migrations/0074_merge_20240228_1544.py

@@ -0,0 +1,13 @@
+# Generated by Django 4.2.10 on 2024-02-28 14:44
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("accounts", "0073_alter_user_user_contacts"),
+        ("accounts", "0073_user_selected_categories"),
+    ]
+
+    operations = []

src/open_inwoner/accounts/tests/test_commands.py

@@ -2,13 +2,14 @@
 from django.test import TestCase
 
 from freezegun import freeze_time
-from timeline_logger.models import TimelineLog
+
+from open_inwoner.utils.tests.helpers import AssertTimelineLogMixin
 
 from ..models import Invite
 from .factories import InviteFactory, UserFactory
 
 
-class DeleteContactInvitationsTest(TestCase):
+class DeleteContactInvitationsTest(AssertTimelineLogMixin, TestCase):
     @freeze_time("2023-09-26", as_arg=True)
     def test_delete_expired_invitations(frozen_time, self):
         user = UserFactory(
@@ -34,19 +35,18 @@ def test_delete_expired_invitations(frozen_time, self):
         self.assertEqual(len(invitations), 1)
         self.assertEqual(invitations[0].invitee_email, invite4.invitee_email)
 
-        # check logs
-        logs = TimelineLog.objects.all()
-        self.assertEqual(len(logs), 2)
+        # check logs (use string dump, don't rely on order of logs)
+        log_dump = self.getTimelineLogDump()
 
-        self.assertEqual(
-            logs[0].extra_data["deleted_invitations"][0],
-            f"{invite1.invitee_email} (invited on {invite1.created_on.strftime('%Y-%m-%d')})",
-        )
-        self.assertEqual(
-            logs[0].extra_data["deleted_invitations"][1],
-            f"{invite2.invitee_email} (invited on {invite2.created_on.strftime('%Y-%m-%d')})",
+        self.assertIn("total 2 timelinelogs", log_dump)
+
+        self.assertIn(
+            f'deleted_invitations=["{invite1.invitee_email} (invited on 2023-09-26)", "{invite2.invitee_email} (invited on 2023-09-26)"',
+            log_dump,
         )
-        self.assertEqual(
-            logs[1].extra_data["deleted_invitations"][0],
-            f"{invite3.invitee_email} (invited on {invite3.created_on.strftime('%Y-%m-%d')})",
+        self.assertIn(
+            f'deleted_invitations=["{invite3.invitee_email} (invited on 2023-09-26)"',
+            log_dump,
         )
+
+        self.assertNotIn(f"{invite4.invitee_email}", log_dump)

src/open_inwoner/accounts/views/inbox.py

@@ -105,7 +105,7 @@ def get_other_user(self, conversations: dict) -> Optional[User]:
 
         return get_object_or_404(User, uuid=other_user_uuid)
 
-    def get_messages(self, other_user: User) -> List[MessageQuerySet]:
+    def get_messages(self, other_user: User) -> List[Message]:
         """
         Returns the messages (MessageType) of the current conversation.
         """

src/open_inwoner/cms/tests/cms_tools.py

@@ -2,9 +2,6 @@
 
 from django.conf import settings
 from django.contrib.auth.models import AnonymousUser
-from django.contrib.sessions.middleware import (
-    SessionMiddleware as DefaultSessionMiddleWare,
-)
 from django.test import RequestFactory
 from django.utils.module_loading import import_string
 
@@ -15,15 +12,7 @@
 from cms.plugin_rendering import ContentRenderer
 
 from open_inwoner.cms.extensions.models import CommonExtension
-
-
-class SessionMiddleware(DefaultSessionMiddleWare):
-    """
-    The `SessionMiddleware` __init__ expects a `get_response` argument in Django 4.2
-    """
-
-    def __init__(self, *args, **kwargs):
-        super().__init__(get_response=lambda x: "dummy")
+from open_inwoner.utils.test import SessionMiddleware
 
 
 def create_homepage():

src/open_inwoner/configurations/tests/test_oidc.py

@@ -24,7 +24,7 @@ def setUpTestData(cls):
         openid_config.enabled = True
         openid_config.save()
 
-    def test_admin_only_enlabled(self):
+    def test_admin_only_enabled(self):
         """Assert that the OIDC login option is only displayed for login via admin"""
 
         config = SiteConfiguration.get_solo()
@@ -64,7 +64,7 @@ def test_admin_only_disabled(self):
         # admin login
         response = self.client.get(reverse("admin:login"))
 
-        self.assertNotContains(response, _("Log in met een organisatieaccount"))
+        self.assertNotContains(response, _("Login with organization account"))
 
     def test_oidc_config_validation(self):
         """

src/open_inwoner/haalcentraal/signals.py

@@ -5,6 +5,7 @@
 
 from open_inwoner.accounts.choices import LoginTypeChoices
 from open_inwoner.accounts.models import User
+from open_inwoner.utils.logentry import system_action
 
 from .utils import update_brp_data_in_db
 
@@ -18,6 +19,14 @@ def on_bsn_change(instance, **kwargs):
         and instance.is_prepopulated is False
         and instance.login_type == LoginTypeChoices.digid
     ):
-        logger.info("Retrieving data for %s from haal centraal based on BSN", instance)
+        try:
+            system_action(
+                "Retrieving data for %s from haal centraal based on BSN",
+                content_object=instance,
+            )
+        except ValueError:
+            logger.info(
+                "Retrieving data for %s from haal centraal based on BSN", instance
+            )
 
         update_brp_data_in_db(instance)

src/open_inwoner/haalcentraal/tests/test_signal.py

@@ -1,10 +1,13 @@
+import logging
+
 from django.test import TestCase, override_settings
 
 import requests_mock
 from freezegun import freeze_time
 
 from open_inwoner.accounts.choices import LoginTypeChoices
 from open_inwoner.accounts.tests.factories import UserFactory
+from open_inwoner.utils.tests.helpers import AssertTimelineLogMixin
 
 from ...utils.test import ClearCachesMixin
 from ..models import HaalCentraalConfig
@@ -14,7 +17,7 @@
 
 @requests_mock.Mocker()
 class TestPreSaveSignal(ClearCachesMixin, HaalCentraalMixin, TestCase):
-    def test_signal_updates_users_data_when_logged_in_via_digid_v_2(self, m):
+    def test_signal_updates_users_data_when_logged_in_via_digid_v_3(self, m):
         self._setUpMocks_v_2(m)
         self._setUpService()
 
@@ -243,7 +246,7 @@ def test_user_is_not_updated_when_http_500(self, m):
         self.assertFalse(user.is_prepopulated)
 
 
-class TestLogging(HaalCentraalMixin, TestCase):
+class TestLogging(HaalCentraalMixin, AssertTimelineLogMixin, TestCase):
     @freeze_time("2021-10-18 13:00:00")
     @requests_mock.Mocker()
     def test_signal_updates_logging(self, m):
@@ -270,9 +273,12 @@ def test_signal_updates_logging(self, m):
             first_name="", last_name="", login_type=LoginTypeChoices.digid
         )
         user.bsn = "999993847"
+        user.save()
 
-        with self.assertLogs("open_inwoner.haalcentraal.signals"):
-            user.save()
+        self.assertTimelineLog(
+            "Retrieving data for %s from haal centraal based on BSN",
+            level=logging.INFO,
+        )
 
     @requests_mock.Mocker()
     def test_single_entry_is_logged_when_there_is_an_error(self, m):
@@ -295,9 +301,14 @@ def test_single_entry_is_logged_when_there_is_an_error(self, m):
         config.save()
 
         user = UserFactory(
-            first_name="", last_name="", login_type=LoginTypeChoices.digid
+            first_name="",
+            last_name="",
+            login_type=LoginTypeChoices.digid,
         )
         user.bsn = "999993847"
+        user.save()
 
-        with self.assertLogs("open_inwoner.haalcentraal.signals"):
-            user.save()
+        self.assertTimelineLog(
+            "Retrieving data for %s from haal centraal based on BSN",
+            level=logging.INFO,
+        )

src/open_inwoner/questionnaire/tests/test_views.py

@@ -1,15 +1,14 @@
 from unittest.mock import patch
 
-from django.contrib.sessions.middleware import (
-    SessionMiddleware as DefaultSessionMiddleWare,
-)
 from django.http import Http404
 from django.test import RequestFactory, TestCase, override_settings
 from django.urls import reverse
 from django.views.generic import FormView
 
 from django_webtest import WebTest
 
+from open_inwoner.utils.test import SessionMiddleware
+
 from ...accounts.tests.factories import UserFactory
 from ...cms.profile.cms_appconfig import ProfileConfig
 from ...cms.profile.cms_apps import ProfileApphook
@@ -22,15 +21,6 @@
 from .factories import QuestionnaireStepFactory, QuestionnaireStepFileFactory
 
 
-class SessionMiddleware(DefaultSessionMiddleWare):
-    """
-    The `SessionMiddleware` __init__ expects a `get_response` argument in Django 4.2
-    """
-
-    def __init__(self, *args, **kwargs):
-        super().__init__(get_response=lambda x: "dummy")
-
-
 @override_settings(ROOT_URLCONF="open_inwoner.cms.tests.urls")
 class QuestionnaireResetViewTestCase(WebTest):
     def test_clears_session(self):

src/open_inwoner/utils/test.py

@@ -2,6 +2,9 @@
 import tempfile
 from typing import Any, Dict, List
 
+from django.contrib.sessions.middleware import (
+    SessionMiddleware as DefaultSessionMiddleWare,
+)
 from django.core.cache import caches
 from django.test import override_settings
 
@@ -72,3 +75,12 @@ def wrapper(self, *args, **kwargs):
         return wrapper
 
     return decorator
+
+
+class SessionMiddleware(DefaultSessionMiddleWare):
+    """
+    `SessionMiddleware` __init__ expects a `get_response` argument in Django 4.2
+    """
+
+    def __init__(self, *args, **kwargs):
+        super().__init__(get_response=lambda x: "dummy")