From 43c6395fdc5aee7acbd6514c3c548f68aed75d22 Mon Sep 17 00:00:00 2001 From: Paul Schilling Date: Fri, 23 Feb 2024 14:58:25 +0100 Subject: [PATCH] [#939] Process PR feedback --- .../migrations/0074_merge_20240228_1544.py | 13 ++++++++ .../accounts/tests/test_commands.py | 30 +++++++++---------- src/open_inwoner/accounts/views/inbox.py | 2 +- src/open_inwoner/cms/tests/cms_tools.py | 13 +------- .../configurations/tests/test_oidc.py | 4 +-- src/open_inwoner/haalcentraal/signals.py | 11 ++++++- .../haalcentraal/tests/test_signal.py | 25 +++++++++++----- .../questionnaire/tests/test_views.py | 14 ++------- src/open_inwoner/utils/test.py | 12 ++++++++ 9 files changed, 74 insertions(+), 50 deletions(-) create mode 100644 src/open_inwoner/accounts/migrations/0074_merge_20240228_1544.py diff --git a/src/open_inwoner/accounts/migrations/0074_merge_20240228_1544.py b/src/open_inwoner/accounts/migrations/0074_merge_20240228_1544.py new file mode 100644 index 0000000000..b85729bd39 --- /dev/null +++ b/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 = [] diff --git a/src/open_inwoner/accounts/tests/test_commands.py b/src/open_inwoner/accounts/tests/test_commands.py index 658c15845b..ff01a3c612 100644 --- a/src/open_inwoner/accounts/tests/test_commands.py +++ b/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) diff --git a/src/open_inwoner/accounts/views/inbox.py b/src/open_inwoner/accounts/views/inbox.py index f1d7475db1..875ab14dbf 100644 --- a/src/open_inwoner/accounts/views/inbox.py +++ b/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. """ diff --git a/src/open_inwoner/cms/tests/cms_tools.py b/src/open_inwoner/cms/tests/cms_tools.py index b118493972..98ab0691ed 100644 --- a/src/open_inwoner/cms/tests/cms_tools.py +++ b/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(): diff --git a/src/open_inwoner/configurations/tests/test_oidc.py b/src/open_inwoner/configurations/tests/test_oidc.py index 2e5659d942..4ee990c058 100644 --- a/src/open_inwoner/configurations/tests/test_oidc.py +++ b/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): """ diff --git a/src/open_inwoner/haalcentraal/signals.py b/src/open_inwoner/haalcentraal/signals.py index 405dc8951e..8e4bedc1e9 100644 --- a/src/open_inwoner/haalcentraal/signals.py +++ b/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) diff --git a/src/open_inwoner/haalcentraal/tests/test_signal.py b/src/open_inwoner/haalcentraal/tests/test_signal.py index b654e17dfc..66eb125105 100644 --- a/src/open_inwoner/haalcentraal/tests/test_signal.py +++ b/src/open_inwoner/haalcentraal/tests/test_signal.py @@ -1,3 +1,5 @@ +import logging + from django.test import TestCase, override_settings import requests_mock @@ -5,6 +7,7 @@ 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, + ) diff --git a/src/open_inwoner/questionnaire/tests/test_views.py b/src/open_inwoner/questionnaire/tests/test_views.py index 8b08e87eaa..7a821a7b43 100644 --- a/src/open_inwoner/questionnaire/tests/test_views.py +++ b/src/open_inwoner/questionnaire/tests/test_views.py @@ -1,8 +1,5 @@ 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 @@ -10,6 +7,8 @@ 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): diff --git a/src/open_inwoner/utils/test.py b/src/open_inwoner/utils/test.py index ea4e3e6322..f36b07fbba 100644 --- a/src/open_inwoner/utils/test.py +++ b/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")