From 5017492f6000232aae4c08ffde1c1a701c0c7fac Mon Sep 17 00:00:00 2001 From: Paul Schilling Date: Tue, 13 Feb 2024 16:20:55 +0100 Subject: [PATCH] [#939] Fix tests * refactor logging, move logic from DocumentDeleteView.delete() to form_valid() * normalize data structure and correct type annotation in `InboxView` (confusion between queryset and list) * refactor logging in the context of DigiD user change/creation since db logging caused `ValueError: save() prohibited to prevent data loss due to unsaved related object 'content_object'` --- .../0073_alter_user_user_contacts.py | 24 +++++++++++ src/open_inwoner/accounts/tests/test_admin.py | 2 +- src/open_inwoner/accounts/views/documents.py | 10 ++--- src/open_inwoner/accounts/views/inbox.py | 12 +++--- ...lter_bannerimage_cmsplugin_ptr_and_more.py | 41 +++++++++++++++++++ ...4_alter_userfeed_cmsplugin_ptr_and_more.py | 41 +++++++++++++++++++ src/open_inwoner/cms/tests/cms_tools.py | 13 +++++- src/open_inwoner/configurations/emails.py | 3 +- .../configurations/tests/test_emails.py | 21 ++-------- src/open_inwoner/haalcentraal/signals.py | 5 +-- .../haalcentraal/tests/test_signal.py | 25 +++-------- src/open_inwoner/haalcentraal/utils.py | 4 +- .../pdc/tests/test_product_admin.py | 2 +- src/open_inwoner/plans/tests/test_views.py | 2 +- .../questionnaire/tests/test_views.py | 13 +++++- .../tests/hooks/test_case_document.py | 2 +- .../userfeed/tests/hooks/test_case_status.py | 2 +- .../userfeed/tests/hooks/test_plan.py | 2 +- .../utils/tests/test_validators.py | 2 +- 19 files changed, 157 insertions(+), 69 deletions(-) create mode 100644 src/open_inwoner/accounts/migrations/0073_alter_user_user_contacts.py create mode 100644 src/open_inwoner/cms/banner/migrations/0004_alter_bannerimage_cmsplugin_ptr_and_more.py create mode 100644 src/open_inwoner/cms/plugins/migrations/0004_alter_userfeed_cmsplugin_ptr_and_more.py diff --git a/src/open_inwoner/accounts/migrations/0073_alter_user_user_contacts.py b/src/open_inwoner/accounts/migrations/0073_alter_user_user_contacts.py new file mode 100644 index 0000000000..883472a74d --- /dev/null +++ b/src/open_inwoner/accounts/migrations/0073_alter_user_user_contacts.py @@ -0,0 +1,24 @@ +# Generated by Django 4.2.10 on 2024-02-13 13:58 + +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("accounts", "0072_merge_20240129_1610"), + ] + + operations = [ + migrations.AlterField( + model_name="user", + name="user_contacts", + field=models.ManyToManyField( + blank=True, + help_text="The contacts of the specific user", + to=settings.AUTH_USER_MODEL, + verbose_name="Contacts", + ), + ), + ] diff --git a/src/open_inwoner/accounts/tests/test_admin.py b/src/open_inwoner/accounts/tests/test_admin.py index d52e0cca8d..88df024a31 100644 --- a/src/open_inwoner/accounts/tests/test_admin.py +++ b/src/open_inwoner/accounts/tests/test_admin.py @@ -1,5 +1,5 @@ from django.urls import reverse -from django.utils.translation import ugettext as _ +from django.utils.translation import gettext as _ from django_webtest import WebTest from maykin_2fa.test import disable_admin_mfa diff --git a/src/open_inwoner/accounts/views/documents.py b/src/open_inwoner/accounts/views/documents.py index d7e09b83fe..4977306046 100644 --- a/src/open_inwoner/accounts/views/documents.py +++ b/src/open_inwoner/accounts/views/documents.py @@ -1,5 +1,4 @@ from django.contrib.auth.mixins import LoginRequiredMixin -from django.http.response import HttpResponseRedirect from django.urls.base import reverse_lazy from django.utils.translation import gettext as _ from django.views.generic import DeleteView @@ -20,9 +19,6 @@ def get_queryset(self): base_qs = super().get_queryset() return base_qs.filter(owner=self.request.user) - def delete(self, request, *args, **kwargs): - object = self.get_object() - super().delete(request, *args, **kwargs) - - self.log_deletion(object, _("file was deleted")) - return HttpResponseRedirect(self.success_url) + def form_valid(self, form): + self.log_deletion(self.object, _("file was deleted")) + return super().form_valid(form) diff --git a/src/open_inwoner/accounts/views/inbox.py b/src/open_inwoner/accounts/views/inbox.py index 298bfa6c8b..f1d7475db1 100644 --- a/src/open_inwoner/accounts/views/inbox.py +++ b/src/open_inwoner/accounts/views/inbox.py @@ -1,5 +1,5 @@ import logging -from typing import Optional +from typing import List, Optional from urllib.parse import unquote from django.contrib.auth.mixins import LoginRequiredMixin @@ -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) -> MessageQuerySet: + def get_messages(self, other_user: User) -> List[MessageQuerySet]: """ Returns the messages (MessageType) of the current conversation. """ @@ -122,12 +122,10 @@ def get_status(self, messages: MessageQuerySet) -> str: """ Returns the status string of the conversation. """ - try: - return f"{_('Laatste bericht ontvangen op')} {formats.date_format(messages[-1].created_on)}" - except IndexError: - return "" - except AssertionError: + messages = list(messages) + if not messages: return "" + return f"{_('Laatste bericht ontvangen op')} {formats.date_format(messages[-1].created_on)}" def mark_messages_seen(self, other_user: Optional[User]): if not other_user: diff --git a/src/open_inwoner/cms/banner/migrations/0004_alter_bannerimage_cmsplugin_ptr_and_more.py b/src/open_inwoner/cms/banner/migrations/0004_alter_bannerimage_cmsplugin_ptr_and_more.py new file mode 100644 index 0000000000..97042161f7 --- /dev/null +++ b/src/open_inwoner/cms/banner/migrations/0004_alter_bannerimage_cmsplugin_ptr_and_more.py @@ -0,0 +1,41 @@ +# Generated by Django 4.2.10 on 2024-02-13 13:58 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("cms", "0022_auto_20180620_1551"), + ("banner", "0003_auto_20230511_1125"), + ] + + operations = [ + migrations.AlterField( + model_name="bannerimage", + name="cmsplugin_ptr", + field=models.OneToOneField( + auto_created=True, + on_delete=django.db.models.deletion.CASCADE, + parent_link=True, + primary_key=True, + related_name="%(app_label)s_%(class)s", + serialize=False, + to="cms.cmsplugin", + ), + ), + migrations.AlterField( + model_name="bannertext", + name="cmsplugin_ptr", + field=models.OneToOneField( + auto_created=True, + on_delete=django.db.models.deletion.CASCADE, + parent_link=True, + primary_key=True, + related_name="%(app_label)s_%(class)s", + serialize=False, + to="cms.cmsplugin", + ), + ), + ] diff --git a/src/open_inwoner/cms/plugins/migrations/0004_alter_userfeed_cmsplugin_ptr_and_more.py b/src/open_inwoner/cms/plugins/migrations/0004_alter_userfeed_cmsplugin_ptr_and_more.py new file mode 100644 index 0000000000..56c4b19510 --- /dev/null +++ b/src/open_inwoner/cms/plugins/migrations/0004_alter_userfeed_cmsplugin_ptr_and_more.py @@ -0,0 +1,41 @@ +# Generated by Django 4.2.10 on 2024-02-13 13:58 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("cms", "0022_auto_20180620_1551"), + ("plugins", "0003_userfeed_title"), + ] + + operations = [ + migrations.AlterField( + model_name="userfeed", + name="cmsplugin_ptr", + field=models.OneToOneField( + auto_created=True, + on_delete=django.db.models.deletion.CASCADE, + parent_link=True, + primary_key=True, + related_name="%(app_label)s_%(class)s", + serialize=False, + to="cms.cmsplugin", + ), + ), + migrations.AlterField( + model_name="videoplayer", + name="cmsplugin_ptr", + field=models.OneToOneField( + auto_created=True, + on_delete=django.db.models.deletion.CASCADE, + parent_link=True, + primary_key=True, + related_name="%(app_label)s_%(class)s", + serialize=False, + to="cms.cmsplugin", + ), + ), + ] diff --git a/src/open_inwoner/cms/tests/cms_tools.py b/src/open_inwoner/cms/tests/cms_tools.py index dafbcb7250..b118493972 100644 --- a/src/open_inwoner/cms/tests/cms_tools.py +++ b/src/open_inwoner/cms/tests/cms_tools.py @@ -2,7 +2,9 @@ from django.conf import settings from django.contrib.auth.models import AnonymousUser -from django.contrib.sessions.middleware import SessionMiddleware +from django.contrib.sessions.middleware import ( + SessionMiddleware as DefaultSessionMiddleWare, +) from django.test import RequestFactory from django.utils.module_loading import import_string @@ -15,6 +17,15 @@ 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") + + def create_homepage(): """ helper to create an empty, published homepage diff --git a/src/open_inwoner/configurations/emails.py b/src/open_inwoner/configurations/emails.py index e3d5abe131..28d368d56e 100644 --- a/src/open_inwoner/configurations/emails.py +++ b/src/open_inwoner/configurations/emails.py @@ -3,7 +3,6 @@ from django.db.models import F from django.utils import timezone -from django_yubin.constants import RESULT_FAILED from django_yubin.models import Log from mail_editor.helpers import find_template @@ -21,7 +20,7 @@ def inform_admins_about_failing_emails(): period_start = now - timedelta(days=1) failed_email_logs = ( - Log.objects.filter(date__gt=period_start, result=RESULT_FAILED) + Log.objects.filter(date__gt=period_start) .annotate(subject=F("message__subject"), recipient=F("message__to_address")) .values("subject", "recipient", "date") ) diff --git a/src/open_inwoner/configurations/tests/test_emails.py b/src/open_inwoner/configurations/tests/test_emails.py index 1260cfd17a..ca2b42cc01 100644 --- a/src/open_inwoner/configurations/tests/test_emails.py +++ b/src/open_inwoner/configurations/tests/test_emails.py @@ -2,7 +2,6 @@ from django.core.management import call_command from django.test import TestCase -from django_yubin.constants import RESULT_FAILED from django_yubin.models import Log, Message from freezegun import freeze_time @@ -20,11 +19,8 @@ def test_no_recipients_configured(self): subject="Some subject", from_address="from@example.com", to_address="to@example.com", - encoded_message="foo", - ) - Log.objects.create( - message=message, result=RESULT_FAILED, log_message="Some " ) + Log.objects.create(message=message, log_message="Some ") with freeze_time("2024-01-02T00:00:00"): call_command("send_failed_mail_digest") @@ -41,11 +37,8 @@ def test_no_failing_emails_in_past_24_hours(self): subject="Some subject", from_address="from@example.com", to_address="to@example.com", - encoded_message="foo", - ) - Log.objects.create( - message=message, result=RESULT_FAILED, log_message="Some msg" ) + Log.objects.create(message=message, log_message="Some msg") with freeze_time("2024-01-02T00:00:00"): call_command("send_failed_mail_digest") @@ -62,22 +55,16 @@ def test_send_daily_failing_email_digest(self): subject="Should not show up in email", from_address="from@example.com", to_address="to@example.com", - encoded_message="foo", - ) - Log.objects.create( - message=message, result=RESULT_FAILED, log_message="Some msg" ) + Log.objects.create(message=message, log_message="Some msg") with freeze_time("2024-01-01T12:00:00"): message = Message.objects.create( subject="Should show up in email", from_address="from@example.com", to_address="to@example.com", - encoded_message="bar", - ) - Log.objects.create( - message=message, result=RESULT_FAILED, log_message="Some msg" ) + Log.objects.create(message=message, log_message="Some msg") with freeze_time("2024-01-02T00:00:00"): call_command("send_failed_mail_digest") diff --git a/src/open_inwoner/haalcentraal/signals.py b/src/open_inwoner/haalcentraal/signals.py index 31cdababe3..405dc8951e 100644 --- a/src/open_inwoner/haalcentraal/signals.py +++ b/src/open_inwoner/haalcentraal/signals.py @@ -5,7 +5,6 @@ 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 @@ -19,8 +18,6 @@ def on_bsn_change(instance, **kwargs): and instance.is_prepopulated is False and instance.login_type == LoginTypeChoices.digid ): - system_action( - "Retrieving data from haal centraal based on BSN", content_object=instance - ) + 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 d37f0c9226..b5a6cc6e22 100644 --- a/src/open_inwoner/haalcentraal/tests/test_signal.py +++ b/src/open_inwoner/haalcentraal/tests/test_signal.py @@ -1,4 +1,5 @@ import logging +from unittest.mock import patch from django.test import TestCase, override_settings from django.utils.translation import gettext as _ @@ -275,23 +276,9 @@ def test_signal_updates_logging(self, m): first_name="", last_name="", login_type=LoginTypeChoices.digid ) user.bsn = "999993847" - user.save() - log_entry = TimelineLog.objects.filter(object_id=user.id)[1] - - self.assertEquals( - log_entry.timestamp.strftime("%m/%d/%Y, %H:%M:%S"), "10/18/2021, 13:00:00" - ) - self.assertEquals(log_entry.object_id, str(user.id)) - self.assertEquals( - log_entry.extra_data, - { - "message": _("data was retrieved from haal centraal"), - "log_level": logging.INFO, - "action_flag": list(LOG_ACTIONS[5]), - "content_object_repr": str(user), - }, - ) + with self.assertLogs("open_inwoner.haalcentraal.signals"): + user.save() @requests_mock.Mocker() def test_single_entry_is_logged_when_there_is_an_error(self, m): @@ -317,8 +304,6 @@ def test_single_entry_is_logged_when_there_is_an_error(self, m): first_name="", last_name="", login_type=LoginTypeChoices.digid ) user.bsn = "999993847" - user.save() - - log_entries = TimelineLog.objects.count() - self.assertEqual(log_entries, 1) + with self.assertLogs("open_inwoner.haalcentraal.signals"): + user.save() diff --git a/src/open_inwoner/haalcentraal/utils.py b/src/open_inwoner/haalcentraal/utils.py index f643fd3ed8..d685b3994b 100644 --- a/src/open_inwoner/haalcentraal/utils.py +++ b/src/open_inwoner/haalcentraal/utils.py @@ -2,11 +2,9 @@ from typing import Optional from django.conf import settings -from django.utils.translation import gettext as _ from open_inwoner.haalcentraal.api import BRP_1_3, BRP_2_1, BRPAPI from open_inwoner.haalcentraal.api_models import BRPData -from open_inwoner.utils.logentry import system_action logger = logging.getLogger(__name__) @@ -41,4 +39,4 @@ def update_brp_data_in_db(user, initial=True): if initial is False: user.save() - system_action(_("data was retrieved from haal centraal"), content_object=user) + logger.info("Retrieving data for %s from haal centraal based on BSN", user) diff --git a/src/open_inwoner/pdc/tests/test_product_admin.py b/src/open_inwoner/pdc/tests/test_product_admin.py index fde73b1cca..f10316981d 100644 --- a/src/open_inwoner/pdc/tests/test_product_admin.py +++ b/src/open_inwoner/pdc/tests/test_product_admin.py @@ -1,6 +1,6 @@ from django.contrib.auth.models import Permission from django.urls import reverse -from django.utils.translation import ugettext as _ +from django.utils.translation import gettext as _ from django_webtest import WebTest from maykin_2fa.test import disable_admin_mfa diff --git a/src/open_inwoner/plans/tests/test_views.py b/src/open_inwoner/plans/tests/test_views.py index 0049b92067..9ef5b1b73f 100644 --- a/src/open_inwoner/plans/tests/test_views.py +++ b/src/open_inwoner/plans/tests/test_views.py @@ -5,7 +5,7 @@ from django.core import mail from django.test import override_settings, tag from django.urls import reverse -from django.utils.translation import ugettext as _ +from django.utils.translation import gettext as _ from django_webtest import WebTest from freezegun import freeze_time diff --git a/src/open_inwoner/questionnaire/tests/test_views.py b/src/open_inwoner/questionnaire/tests/test_views.py index 2b38f23879..8b08e87eaa 100644 --- a/src/open_inwoner/questionnaire/tests/test_views.py +++ b/src/open_inwoner/questionnaire/tests/test_views.py @@ -1,6 +1,8 @@ from unittest.mock import patch -from django.contrib.sessions.middleware import SessionMiddleware +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 @@ -20,6 +22,15 @@ 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/userfeed/tests/hooks/test_case_document.py b/src/open_inwoner/userfeed/tests/hooks/test_case_document.py index 0f6bae22ec..7864c63e8a 100644 --- a/src/open_inwoner/userfeed/tests/hooks/test_case_document.py +++ b/src/open_inwoner/userfeed/tests/hooks/test_case_document.py @@ -2,7 +2,7 @@ from django.test import TestCase, override_settings from django.urls import reverse -from django.utils.translation import ugettext as _ +from django.utils.translation import gettext as _ from zgw_consumers.api_models.base import factory diff --git a/src/open_inwoner/userfeed/tests/hooks/test_case_status.py b/src/open_inwoner/userfeed/tests/hooks/test_case_status.py index a830b3278f..669b218608 100644 --- a/src/open_inwoner/userfeed/tests/hooks/test_case_status.py +++ b/src/open_inwoner/userfeed/tests/hooks/test_case_status.py @@ -3,7 +3,7 @@ from django.test import TestCase, override_settings from django.urls import reverse from django.utils.html import escape, strip_tags -from django.utils.translation import ngettext, ugettext as _ +from django.utils.translation import gettext as _, ngettext from zgw_consumers.api_models.base import factory diff --git a/src/open_inwoner/userfeed/tests/hooks/test_plan.py b/src/open_inwoner/userfeed/tests/hooks/test_plan.py index 8cd26950ed..2fc20f0afc 100644 --- a/src/open_inwoner/userfeed/tests/hooks/test_plan.py +++ b/src/open_inwoner/userfeed/tests/hooks/test_plan.py @@ -3,7 +3,7 @@ from django.test import TestCase, override_settings from django.urls import reverse -from django.utils.translation import ugettext as _ +from django.utils.translation import gettext as _ from open_inwoner.accounts.tests.factories import UserFactory from open_inwoner.plans.tests.factories import PlanFactory diff --git a/src/open_inwoner/utils/tests/test_validators.py b/src/open_inwoner/utils/tests/test_validators.py index 2ec6798b70..0dc187967b 100644 --- a/src/open_inwoner/utils/tests/test_validators.py +++ b/src/open_inwoner/utils/tests/test_validators.py @@ -1,6 +1,6 @@ from django.core.exceptions import ValidationError from django.test import TestCase -from django.utils.translation import ugettext as _ +from django.utils.translation import gettext as _ from ..validators import ( DutchPhoneNumberValidator,