From b07716b051bfbdb181fb1d633ec0d574679a0454 Mon Sep 17 00:00:00 2001 From: Bart van der Schoor Date: Thu, 25 Jan 2024 12:29:26 +0100 Subject: [PATCH 1/4] [#2041] Added hard database constraints on User's identifier/login_type fields --- .../migrations/0071_auto_20240125_1227.py | 88 +++++++++++++ src/open_inwoner/accounts/models.py | 66 ++++++---- .../accounts/tests/test_user_constraints.py | 119 ++++++++++++++++++ 3 files changed, 246 insertions(+), 27 deletions(-) create mode 100644 src/open_inwoner/accounts/migrations/0071_auto_20240125_1227.py create mode 100644 src/open_inwoner/accounts/tests/test_user_constraints.py diff --git a/src/open_inwoner/accounts/migrations/0071_auto_20240125_1227.py b/src/open_inwoner/accounts/migrations/0071_auto_20240125_1227.py new file mode 100644 index 0000000000..9869487bb6 --- /dev/null +++ b/src/open_inwoner/accounts/migrations/0071_auto_20240125_1227.py @@ -0,0 +1,88 @@ +# Generated by Django 3.2.23 on 2024-01-25 11:27 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("accounts", "0070_auto_20231205_1657"), + ] + + operations = [ + migrations.AddConstraint( + model_name="user", + constraint=models.UniqueConstraint( + condition=models.Q(("login_type", "digid")), + fields=("bsn",), + name="unique_bsn_when_login_digid", + ), + ), + migrations.AddConstraint( + model_name="user", + constraint=models.UniqueConstraint( + condition=models.Q( + ("login_type", "eherkenning"), models.Q(("rsin", ""), _negated=True) + ), + fields=("rsin",), + name="unique_rsin_when_login_eherkenning", + ), + ), + migrations.AddConstraint( + model_name="user", + constraint=models.UniqueConstraint( + condition=models.Q( + ("login_type", "eherkenning"), models.Q(("kvk", ""), _negated=True) + ), + fields=("kvk",), + name="unique_kvk_when_login_eherkenning", + ), + ), + migrations.AddConstraint( + model_name="user", + constraint=models.UniqueConstraint( + condition=models.Q(("login_type", "oidc")), + fields=("oidc_id",), + name="unique_oidc_id_when_login_oidc", + ), + ), + migrations.AddConstraint( + model_name="user", + constraint=models.CheckConstraint( + check=models.Q(("bsn", ""), ("login_type", "digid"), _connector="OR"), + name="check_bsn_only_set_when_login_digid", + ), + ), + migrations.AddConstraint( + model_name="user", + constraint=models.CheckConstraint( + check=models.Q( + ("oidc_id", ""), ("login_type", "oidc"), _connector="OR" + ), + name="check_oidc_id_only_set_when_login_oidc", + ), + ), + migrations.AddConstraint( + model_name="user", + constraint=models.CheckConstraint( + check=models.Q( + models.Q(("kvk", ""), ("rsin", "")), + ("login_type", "eherkenning"), + _connector="OR", + ), + name="check_kvk_or_rsin_only_set_when_login_eherkenning", + ), + ), + migrations.AddConstraint( + model_name="user", + constraint=models.CheckConstraint( + check=models.Q( + models.Q(("kvk", ""), models.Q(("rsin", ""), _negated=True)), + models.Q(models.Q(("kvk", ""), _negated=True), ("rsin", "")), + models.Q(("login_type", "eherkenning"), _negated=True), + _connector="OR", + ), + name="check_kvk_or_rsin_exclusive_when_login_eherkenning", + ), + ), + ] diff --git a/src/open_inwoner/accounts/models.py b/src/open_inwoner/accounts/models.py index 37d903ac34..b12ce04515 100644 --- a/src/open_inwoner/accounts/models.py +++ b/src/open_inwoner/accounts/models.py @@ -132,8 +132,6 @@ def has_verified_email(self): date_joined = models.DateTimeField( verbose_name=_("Date joined"), default=timezone.now ) - # TODO shouldn't rsin & bsn be unique? (possibly fixed in model constraints) - # TODO fix rsin & bsn to not be both null AND blank (!) rsin = models.CharField( verbose_name=_("Rsin"), max_length=9, blank=True, default="" ) @@ -255,31 +253,45 @@ class Meta: & ~Q(login_type=LoginTypeChoices.eherkenning), name="unique_email_when_not_digid_or_eherkenning", ), - # UniqueConstraint( - # fields=["bsn"], - # condition=Q(login_type=LoginTypeChoices.digid) - # # not quite sure if we want this (bsn shouldn't be null AND blank) - # & ~(Q(bsn="") | Q(bsn__isnull=True)), - # name="unique_bsn_when_digid", - # ), - # UniqueConstraint( - # fields=["rsin"], - # condition=Q(login_type=LoginTypeChoices.eherkenning) - # # not quite sure if we want this (rsin shouldn't be null AND blank) - # & ~(Q(rsin="") | Q(rsin__isnull=True)), - # name="unique_rsin_when_eherkenning", - # ), - # UniqueConstraint( - # fields=["oidc_id"], - # condition=Q(login_type=LoginTypeChoices.oidc), - # name="unique_bsn_when_digid", - # ), - # CheckConstraint( - # # maybe this is not correct? - # check=(Q(bsn="") | Q(bsn__isnull=True)) - # | ~Q(login_type=LoginTypeChoices.digid), - # name="check_digid_bsn_required_when_digid", - # ), + UniqueConstraint( + fields=["bsn"], + condition=Q(login_type=LoginTypeChoices.digid), + name="unique_bsn_when_login_digid", + ), + UniqueConstraint( + fields=["rsin"], + condition=Q(login_type=LoginTypeChoices.eherkenning) & ~Q(rsin=""), + name="unique_rsin_when_login_eherkenning", + ), + UniqueConstraint( + fields=["kvk"], + condition=Q(login_type=LoginTypeChoices.eherkenning) & ~Q(kvk=""), + name="unique_kvk_when_login_eherkenning", + ), + UniqueConstraint( + fields=["oidc_id"], + condition=Q(login_type=LoginTypeChoices.oidc), + name="unique_oidc_id_when_login_oidc", + ), + # --- --- --- + CheckConstraint( + check=Q(bsn="") | Q(login_type=LoginTypeChoices.digid), + name="check_bsn_only_set_when_login_digid", + ), + CheckConstraint( + check=Q(oidc_id="") | Q(login_type=LoginTypeChoices.oidc), + name="check_oidc_id_only_set_when_login_oidc", + ), + CheckConstraint( + check=(Q(kvk="") & Q(rsin="")) + | Q(login_type=LoginTypeChoices.eherkenning), + name="check_kvk_or_rsin_only_set_when_login_eherkenning", + ), + CheckConstraint( + check=((Q(kvk="") & ~Q(rsin="")) | (~Q(kvk="") & Q(rsin=""))) + | ~Q(login_type=LoginTypeChoices.eherkenning), + name="check_kvk_or_rsin_exclusive_when_login_eherkenning", + ), ] def __init__(self, *args, **kwargs): diff --git a/src/open_inwoner/accounts/tests/test_user_constraints.py b/src/open_inwoner/accounts/tests/test_user_constraints.py new file mode 100644 index 0000000000..b50dd0f0c7 --- /dev/null +++ b/src/open_inwoner/accounts/tests/test_user_constraints.py @@ -0,0 +1,119 @@ +from django.db import IntegrityError, transaction +from django.test import TestCase + +from open_inwoner.accounts.choices import LoginTypeChoices + +from .factories import UserFactory + + +class UserIdentifierConstraintTests(TestCase): + def test_unique_email_when_login_default(self): + UserFactory(email="foo@example.com", login_type=LoginTypeChoices.default) + UserFactory(email="bar@example.com", login_type=LoginTypeChoices.default) + + with self.assertRaises(IntegrityError): + UserFactory(email="foo@example.com", login_type=LoginTypeChoices.default) + + def test_not_unique_email_when_login_not_default(self): + UserFactory(email="foo@example.com", login_type=LoginTypeChoices.default) + + UserFactory(email="foo@example.com", login_type=LoginTypeChoices.digid) + UserFactory(email="foo@example.com", login_type=LoginTypeChoices.eherkenning) + UserFactory(email="foo@example.com", login_type=LoginTypeChoices.oidc) + + def test_unique_bsn_when_login_digid(self): + UserFactory(bsn="123456782", login_type=LoginTypeChoices.digid) + UserFactory(bsn="123456783", login_type=LoginTypeChoices.digid) + + with self.assertRaises(IntegrityError): + UserFactory(bsn="123456782", login_type=LoginTypeChoices.digid) + + def test_unique_oidc_id_when_login_oidc(self): + UserFactory(oidc_id="1234", login_type=LoginTypeChoices.oidc) + UserFactory(oidc_id="1235", login_type=LoginTypeChoices.oidc) + + with self.assertRaises(IntegrityError): + UserFactory(oidc_id="1234", login_type=LoginTypeChoices.oidc) + + def test_unique_kvk_when_login_eherkenning(self): + UserFactory(kvk="12345678", login_type=LoginTypeChoices.eherkenning) + UserFactory(kvk="12345679", login_type=LoginTypeChoices.eherkenning) + + with self.assertRaises(IntegrityError): + UserFactory(kvk="12345678", login_type=LoginTypeChoices.eherkenning) + + def test_unique_rsin_when_login_herkenning(self): + UserFactory(rsin="12345678", login_type=LoginTypeChoices.eherkenning) + UserFactory(rsin="12345679", login_type=LoginTypeChoices.eherkenning) + + with self.assertRaises(IntegrityError): + UserFactory(rsin="12345678", login_type=LoginTypeChoices.eherkenning) + + def test_not_both_kvk_and_rsin_when_login_eherkenning(self): + with self.assertRaises(IntegrityError): + UserFactory( + kvk="12345678", rsin="12345678", login_type=LoginTypeChoices.eherkenning + ) + + def test_not_bsn_when_login_not_digid(self): + for login_type in [ + LoginTypeChoices.default, + LoginTypeChoices.eherkenning, + LoginTypeChoices.oidc, + ]: + with self.subTest(login_type): + # start transaction block because we're testing multiple IntegrityErrors + with transaction.atomic(): + with self.assertRaises(IntegrityError): + UserFactory(bsn="123456782", login_type=login_type) + + def test_not_rsin_when_login_not_eherkenning(self): + for login_type in [ + LoginTypeChoices.default, + LoginTypeChoices.digid, + LoginTypeChoices.oidc, + ]: + with self.subTest(login_type): + # start transaction block because we're testing multiple IntegrityErrors + with transaction.atomic(): + with self.assertRaises(IntegrityError): + UserFactory(rsin="12345678", login_type=login_type) + + def test_not_kvk_when_login_not_eherkenning(self): + for login_type in [ + LoginTypeChoices.default, + LoginTypeChoices.digid, + LoginTypeChoices.oidc, + ]: + with self.subTest(login_type): + # start transaction block because we're testing multiple IntegrityErrors + with transaction.atomic(): + with self.assertRaises(IntegrityError): + UserFactory(kvk="12345678", login_type=login_type) + + def test_not_oidc_id_when_login_not_oidc(self): + for login_type in [ + LoginTypeChoices.default, + LoginTypeChoices.digid, + LoginTypeChoices.eherkenning, + ]: + with self.subTest(login_type): + # start transaction block because we're testing multiple IntegrityErrors + with transaction.atomic(): + with self.assertRaises(IntegrityError): + UserFactory(oidc_id="12345678", login_type=login_type) + + def test_login_types_not_changed(self): + """ + The constraints and tests depend on a known set of login_types, + so make sure this suite fails if we ever change them. + """ + self.assertEqual( + LoginTypeChoices.values, + [ + LoginTypeChoices.default, + LoginTypeChoices.digid, + LoginTypeChoices.eherkenning, + LoginTypeChoices.oidc, + ], + ) From ec550ab7840650226e8ec86c3db51a94fa0b5f1d Mon Sep 17 00:00:00 2001 From: Steven Bal Date: Fri, 23 Feb 2024 16:43:36 +0100 Subject: [PATCH 2/4] :twisted_rightwards_arrows: [#2041] Fixes after rebase --- ...user_unique_bsn_when_login_digid_and_more.py} | 16 ++-------------- src/open_inwoner/accounts/models.py | 12 ++++++------ 2 files changed, 8 insertions(+), 20 deletions(-) rename src/open_inwoner/accounts/migrations/{0071_auto_20240125_1227.py => 0076_user_unique_bsn_when_login_digid_and_more.py} (79%) diff --git a/src/open_inwoner/accounts/migrations/0071_auto_20240125_1227.py b/src/open_inwoner/accounts/migrations/0076_user_unique_bsn_when_login_digid_and_more.py similarity index 79% rename from src/open_inwoner/accounts/migrations/0071_auto_20240125_1227.py rename to src/open_inwoner/accounts/migrations/0076_user_unique_bsn_when_login_digid_and_more.py index 9869487bb6..27478ecc2a 100644 --- a/src/open_inwoner/accounts/migrations/0071_auto_20240125_1227.py +++ b/src/open_inwoner/accounts/migrations/0076_user_unique_bsn_when_login_digid_and_more.py @@ -1,4 +1,4 @@ -# Generated by Django 3.2.23 on 2024-01-25 11:27 +# Generated by Django 4.2.11 on 2024-04-15 13:28 from django.db import migrations, models @@ -6,7 +6,7 @@ class Migration(migrations.Migration): dependencies = [ - ("accounts", "0070_auto_20231205_1657"), + ("accounts", "0075_user_verified_email"), ] operations = [ @@ -73,16 +73,4 @@ class Migration(migrations.Migration): name="check_kvk_or_rsin_only_set_when_login_eherkenning", ), ), - migrations.AddConstraint( - model_name="user", - constraint=models.CheckConstraint( - check=models.Q( - models.Q(("kvk", ""), models.Q(("rsin", ""), _negated=True)), - models.Q(models.Q(("kvk", ""), _negated=True), ("rsin", "")), - models.Q(("login_type", "eherkenning"), _negated=True), - _connector="OR", - ), - name="check_kvk_or_rsin_exclusive_when_login_eherkenning", - ), - ), ] diff --git a/src/open_inwoner/accounts/models.py b/src/open_inwoner/accounts/models.py index b12ce04515..3e05bae451 100644 --- a/src/open_inwoner/accounts/models.py +++ b/src/open_inwoner/accounts/models.py @@ -7,7 +7,7 @@ from django.contrib.contenttypes.fields import GenericRelation from django.core.exceptions import ValidationError from django.db import models -from django.db.models import Q, UniqueConstraint +from django.db.models import CheckConstraint, Q, UniqueConstraint from django.urls import reverse from django.utils import timezone from django.utils.crypto import get_random_string @@ -287,11 +287,11 @@ class Meta: | Q(login_type=LoginTypeChoices.eherkenning), name="check_kvk_or_rsin_only_set_when_login_eherkenning", ), - CheckConstraint( - check=((Q(kvk="") & ~Q(rsin="")) | (~Q(kvk="") & Q(rsin=""))) - | ~Q(login_type=LoginTypeChoices.eherkenning), - name="check_kvk_or_rsin_exclusive_when_login_eherkenning", - ), + # CheckConstraint( + # check=((Q(kvk="") & ~Q(rsin="")) | (~Q(kvk="") & Q(rsin=""))) + # | ~Q(login_type=LoginTypeChoices.eherkenning), + # name="check_kvk_or_rsin_exclusive_when_login_eherkenning", + # ), ] def __init__(self, *args, **kwargs): From ed82dc7c7f1cd15570b39de0febe59cfc839a907 Mon Sep 17 00:00:00 2001 From: Steven Bal Date: Fri, 23 Feb 2024 17:06:30 +0100 Subject: [PATCH 3/4] :white_check_mark: [#2041] Fix tests for user DB constraints --- src/open_inwoner/accounts/tests/test_contact_views.py | 2 +- src/open_inwoner/accounts/tests/test_profile_views.py | 1 + .../accounts/tests/test_user_constraints.py | 6 ++++-- src/open_inwoner/cms/cases/tests/test_contactform.py | 6 +++++- src/open_inwoner/haalcentraal/tests/test_signal.py | 10 +++++++--- src/open_inwoner/kvk/tests/test_signals.py | 11 +++++++---- src/open_inwoner/openklant/tests/data.py | 8 ++++++++ src/open_inwoner/pdc/tests/test_views.py | 3 +-- 8 files changed, 34 insertions(+), 13 deletions(-) diff --git a/src/open_inwoner/accounts/tests/test_contact_views.py b/src/open_inwoner/accounts/tests/test_contact_views.py index 8efc92627d..3a3dd432a8 100644 --- a/src/open_inwoner/accounts/tests/test_contact_views.py +++ b/src/open_inwoner/accounts/tests/test_contact_views.py @@ -173,7 +173,7 @@ def test_existing_user_contact(self): self.assertEqual(existing_user, pending_invitation) def test_existing_user_contact_with_case_sensitive_email(self): - existing_user = UserFactory(email="user@example.com", bsn="111111111") + existing_user = DigidUserFactory(email="user@example.com", bsn="111111111") response = self.app.get(self.create_url, user=self.user) form = response.forms["contact-form"] diff --git a/src/open_inwoner/accounts/tests/test_profile_views.py b/src/open_inwoner/accounts/tests/test_profile_views.py index 681502d84b..7f5dc51f09 100644 --- a/src/open_inwoner/accounts/tests/test_profile_views.py +++ b/src/open_inwoner/accounts/tests/test_profile_views.py @@ -213,6 +213,7 @@ def test_info_eherkenning_user(self): housenumber="42", postcode="1234 XY", city="The good place", + kvk="87654321", ) response = self.app.get(self.url, user=user) diff --git a/src/open_inwoner/accounts/tests/test_user_constraints.py b/src/open_inwoner/accounts/tests/test_user_constraints.py index b50dd0f0c7..cb88f77092 100644 --- a/src/open_inwoner/accounts/tests/test_user_constraints.py +++ b/src/open_inwoner/accounts/tests/test_user_constraints.py @@ -1,3 +1,5 @@ +from unittest import skip + from django.db import IntegrityError, transaction from django.test import TestCase @@ -14,12 +16,11 @@ def test_unique_email_when_login_default(self): with self.assertRaises(IntegrityError): UserFactory(email="foo@example.com", login_type=LoginTypeChoices.default) - def test_not_unique_email_when_login_not_default(self): + def test_not_unique_email_when_login_digid_or_eherkenning(self): UserFactory(email="foo@example.com", login_type=LoginTypeChoices.default) UserFactory(email="foo@example.com", login_type=LoginTypeChoices.digid) UserFactory(email="foo@example.com", login_type=LoginTypeChoices.eherkenning) - UserFactory(email="foo@example.com", login_type=LoginTypeChoices.oidc) def test_unique_bsn_when_login_digid(self): UserFactory(bsn="123456782", login_type=LoginTypeChoices.digid) @@ -49,6 +50,7 @@ def test_unique_rsin_when_login_herkenning(self): with self.assertRaises(IntegrityError): UserFactory(rsin="12345678", login_type=LoginTypeChoices.eherkenning) + @skip("Not yet implemented") def test_not_both_kvk_and_rsin_when_login_eherkenning(self): with self.assertRaises(IntegrityError): UserFactory( diff --git a/src/open_inwoner/cms/cases/tests/test_contactform.py b/src/open_inwoner/cms/cases/tests/test_contactform.py index b2d24b9133..e15233fb0b 100644 --- a/src/open_inwoner/cms/cases/tests/test_contactform.py +++ b/src/open_inwoner/cms/cases/tests/test_contactform.py @@ -15,6 +15,7 @@ ) from zgw_consumers.constants import APITypes +from open_inwoner.accounts.models import User from open_inwoner.accounts.tests.factories import ( DigidUserFactory, eHerkenningUserFactory, @@ -459,6 +460,9 @@ def test_form_success_with_api_eherkenning_user(self, m, contactmoment_mock): with self.subTest( use_rsin_for_innNnpId_query_parameter=use_rsin_for_innNnpId_query_parameter ): + # FIXME for some reason creating the user outside of the loop + # makes the second iteration fail? + User.objects.filter(kvk="12345678").delete() eherkenning_user = eHerkenningUserFactory( kvk="12345678", rsin="000000000" ) @@ -477,7 +481,7 @@ def test_form_success_with_api_eherkenning_user(self, m, contactmoment_mock): m.get( f"{KLANTEN_ROOT}klanten?subjectNietNatuurlijkPersoon__innNnpId={identifier}", json=paginated_response([self.klant]), - ), + ) response = self.app.get(self.case_detail_url, user=eherkenning_user) diff --git a/src/open_inwoner/haalcentraal/tests/test_signal.py b/src/open_inwoner/haalcentraal/tests/test_signal.py index df61235656..510f7e0a08 100644 --- a/src/open_inwoner/haalcentraal/tests/test_signal.py +++ b/src/open_inwoner/haalcentraal/tests/test_signal.py @@ -1,6 +1,7 @@ import logging -from django.test import TestCase, override_settings +from django.contrib.auth import user_logged_in +from django.test import RequestFactory, TestCase, override_settings from django.utils.translation import gettext as _ import requests_mock @@ -8,6 +9,7 @@ from timeline_logger.models import TimelineLog from open_inwoner.accounts.choices import LoginTypeChoices +from open_inwoner.accounts.models import User from open_inwoner.accounts.tests.factories import UserFactory from open_inwoner.utils.tests.helpers import AssertTimelineLogMixin @@ -106,8 +108,10 @@ def test_user_is_not_updated_when_not_logged_in_via_digid(self, m): user = UserFactory( first_name="", infix="", last_name="", login_type=LoginTypeChoices.default ) - user.bsn = "999993847" - user.save() + + request = RequestFactory().get("/dummy") + request.user = user + user_logged_in.send(User, user=user, request=request) user.refresh_from_db() diff --git a/src/open_inwoner/kvk/tests/test_signals.py b/src/open_inwoner/kvk/tests/test_signals.py index 74f48f9c77..57c505572e 100644 --- a/src/open_inwoner/kvk/tests/test_signals.py +++ b/src/open_inwoner/kvk/tests/test_signals.py @@ -1,7 +1,7 @@ import logging -from django.contrib.auth import get_user_model -from django.test import TestCase +from django.contrib.auth import get_user_model, user_logged_in +from django.test import RequestFactory, TestCase from django.utils.translation import gettext as _ import requests_mock @@ -9,6 +9,7 @@ from timeline_logger.models import TimelineLog from open_inwoner.accounts.choices import LoginTypeChoices +from open_inwoner.accounts.models import User from open_inwoner.accounts.tests.factories import UserFactory from open_inwoner.utils.logentry import LOG_ACTIONS from open_inwoner.utils.test import ClearCachesMixin @@ -53,8 +54,10 @@ def test_user_is_not_updated_when_not_logged_in_via_eherkenning(self, m): user = UserFactory( first_name="", infix="", last_name="", login_type=LoginTypeChoices.default ) - user.bsn = "69599084" - user.save() + + request = RequestFactory().get("/dummy") + request.user = user + user_logged_in.send(User, user=user, request=request) user.refresh_from_db() diff --git a/src/open_inwoner/openklant/tests/data.py b/src/open_inwoner/openklant/tests/data.py index 57080d6972..65c0c4f312 100644 --- a/src/open_inwoner/openklant/tests/data.py +++ b/src/open_inwoner/openklant/tests/data.py @@ -1,6 +1,7 @@ from requests.exceptions import RequestException from zgw_consumers.constants import APITypes +from open_inwoner.accounts.models import User from open_inwoner.accounts.tests.factories import ( DigidUserFactory, eHerkenningUserFactory, @@ -38,10 +39,13 @@ def setUpServices(cls): class MockAPIReadPatchData(MockAPIData): def __init__(self): + User.objects.filter(email="old@example.com").delete() self.user = DigidUserFactory( email="old@example.com", phonenumber="0100000000", ) + + User.objects.filter(kvk="12345678").delete() self.eherkenning_user = eHerkenningUserFactory( email="old2@example.com", kvk="12345678", @@ -130,9 +134,11 @@ def install_mocks_eherkenning(self, m, use_rsin=True) -> "MockAPIReadPatchData": class MockAPIReadData(MockAPIData): def __init__(self): + User.objects.filter(bsn="100000001").delete() self.user = DigidUserFactory( bsn="100000001", ) + User.objects.filter(kvk="12345678").delete() self.eherkenning_user = eHerkenningUserFactory( kvk="12345678", rsin="000000000", @@ -347,9 +353,11 @@ def install_mocks(self, m, link_objectcontactmomenten=False) -> "MockAPIReadData class MockAPICreateData(MockAPIData): def __init__(self): + User.objects.filter(bsn="100000001").delete() self.user = DigidUserFactory( bsn="100000001", ) + User.objects.filter(kvk="12345678").delete() self.eherkenning_user = eHerkenningUserFactory( kvk="12345678", rsin="000000000", diff --git a/src/open_inwoner/pdc/tests/test_views.py b/src/open_inwoner/pdc/tests/test_views.py index 9e9382fd7f..64bd364f30 100644 --- a/src/open_inwoner/pdc/tests/test_views.py +++ b/src/open_inwoner/pdc/tests/test_views.py @@ -202,8 +202,7 @@ def test_category_detail_view_access_restricted_for_digid_user(self): description="A descriptive description", visible_for_citizens=False, ) - user = DigidUserFactory() - self.client.force_login(user) + self.client.force_login(self.user) url = reverse("products:category_detail", kwargs={"slug": category.slug}) From 2b8b97c37269b00f7177620daed8e393dacebb09 Mon Sep 17 00:00:00 2001 From: Steven Bal Date: Fri, 15 Mar 2024 09:41:28 +0100 Subject: [PATCH 4/4] :ok_hand: [#2041] PR feedback --- src/open_inwoner/accounts/models.py | 8 +- src/open_inwoner/accounts/tests/test_auth.py | 2 +- .../accounts/tests/test_oidc_views.py | 2 +- .../accounts/tests/test_profile_views.py | 117 +++++++++++------- .../accounts/tests/test_user_constraints.py | 12 +- .../cms/cases/tests/test_contactform.py | 14 +-- src/open_inwoner/openklant/tests/data.py | 9 +- .../openklant/tests/test_contactform.py | 8 +- .../openklant/tests/test_views.py | 8 +- 9 files changed, 96 insertions(+), 84 deletions(-) diff --git a/src/open_inwoner/accounts/models.py b/src/open_inwoner/accounts/models.py index 3e05bae451..16b2b7f538 100644 --- a/src/open_inwoner/accounts/models.py +++ b/src/open_inwoner/accounts/models.py @@ -287,11 +287,9 @@ class Meta: | Q(login_type=LoginTypeChoices.eherkenning), name="check_kvk_or_rsin_only_set_when_login_eherkenning", ), - # CheckConstraint( - # check=((Q(kvk="") & ~Q(rsin="")) | (~Q(kvk="") & Q(rsin=""))) - # | ~Q(login_type=LoginTypeChoices.eherkenning), - # name="check_kvk_or_rsin_exclusive_when_login_eherkenning", - # ), + # NOTE: we do not need a constraint that enforces exclusivity between + # `KVK` and `RSIN`, because companies have both of these attributes and we + # need both of them to fetch cases for these companies ] def __init__(self, *args, **kwargs): diff --git a/src/open_inwoner/accounts/tests/test_auth.py b/src/open_inwoner/accounts/tests/test_auth.py index 2bc07869ad..b949b6ce38 100644 --- a/src/open_inwoner/accounts/tests/test_auth.py +++ b/src/open_inwoner/accounts/tests/test_auth.py @@ -1134,7 +1134,7 @@ def test_eherkenning_user_success(self, mock_kvk, mock_retrieve_rsin_with_kvk): test_user = eHerkenningUserFactory.create( email="test@localhost", kvk="64819772", - rsin="123456789", + rsin="123456782", ) url = reverse("eherkenning-mock:password") diff --git a/src/open_inwoner/accounts/tests/test_oidc_views.py b/src/open_inwoner/accounts/tests/test_oidc_views.py index c788e040ca..f26c362126 100644 --- a/src/open_inwoner/accounts/tests/test_oidc_views.py +++ b/src/open_inwoner/accounts/tests/test_oidc_views.py @@ -1055,7 +1055,7 @@ def test_new_user_is_created_when_new_kvk( # set up a user with a non existing email address mock_get_userinfo.return_value = {"sub": "00000000"} eHerkenningUserFactory.create( - kvk="12345678", rsin="123456789", email="existing_user@example.com" + kvk="12345678", rsin="123456782", email="existing_user@example.com" ) session = self.client.session session["oidc_states"] = {"mock": {"nonce": "nonce"}} diff --git a/src/open_inwoner/accounts/tests/test_profile_views.py b/src/open_inwoner/accounts/tests/test_profile_views.py index 7f5dc51f09..b5c50ef37a 100644 --- a/src/open_inwoner/accounts/tests/test_profile_views.py +++ b/src/open_inwoner/accounts/tests/test_profile_views.py @@ -512,7 +512,6 @@ def test_modify_phone_and_email_updates_klant_api(self, m): response = self.app.get(self.url, user=data.user) # reset noise from signals - m.reset_mock() self.clearTimelineLogs() form = response.forms["profile-edit"] @@ -537,52 +536,76 @@ def test_modify_phone_and_email_updates_klant_api(self, m): ) @requests_mock.Mocker() - def test_eherkenning_user_updates_klant_api(self, m): + def test_eherkenning_user_updates_klant_api_with_rsin(self, m): MockAPIReadPatchData.setUpServices() + data = MockAPIReadPatchData() + + matchers = data.install_mocks_eherkenning(m, use_rsin=True).matchers + + config = OpenKlantConfig.get_solo() + config.use_rsin_for_innNnpId_query_parameter = True + config.save() + + response = self.app.get(self.url, user=data.eherkenning_user) + + # reset noise from signals + self.clearTimelineLogs() - for use_rsin_for_innNnpId_query_parameter in [True, False]: - with self.subTest( - use_rsin_for_innNnpId_query_parameter=use_rsin_for_innNnpId_query_parameter - ): - # NOTE Explicitly creating a new Mocker object here, because for some reason - # `m` is overridden somewhere, which causes issues when `MockAPIReadPatchData.install_mocks` - # is run for the second time - with requests_mock.Mocker() as m: - data = MockAPIReadPatchData().install_mocks_eherkenning( - m, use_rsin=use_rsin_for_innNnpId_query_parameter - ) - - config = OpenKlantConfig.get_solo() - config.use_rsin_for_innNnpId_query_parameter = ( - use_rsin_for_innNnpId_query_parameter - ) - config.save() - - response = self.app.get(self.url, user=data.eherkenning_user) - - # reset noise from signals - m.reset_mock() - self.clearTimelineLogs() - - form = response.forms["profile-edit"] - form["email"] = "new@example.com" - form["phonenumber"] = "0612345678" - form.submit() - - # user data tested in other cases - self.assertTrue(data.matchers[0].called) - klant_patch_data = data.matchers[1].request_history[0].json() - self.assertEqual( - klant_patch_data, - { - "emailadres": "new@example.com", - "telefoonnummer": "0612345678", - }, - ) - self.assertTimelineLog("retrieved klant for user") - self.assertTimelineLog( - "patched klant from user profile edit with fields: emailadres, telefoonnummer" - ) + form = response.forms["profile-edit"] + form["email"] = "new@example.com" + form["phonenumber"] = "0612345678" + form.submit() + + # user data tested in other cases + self.assertTrue(matchers[0].called) + klant_patch_data = matchers[1].request_history[0].json() + self.assertEqual( + klant_patch_data, + { + "emailadres": "new@example.com", + "telefoonnummer": "0612345678", + }, + ) + self.assertTimelineLog("retrieved klant for user") + self.assertTimelineLog( + "patched klant from user profile edit with fields: emailadres, telefoonnummer" + ) + + @requests_mock.Mocker() + def test_eherkenning_user_updates_klant_api_with_kvk(self, m): + MockAPIReadPatchData.setUpServices() + data = MockAPIReadPatchData() + + matchers = data.install_mocks_eherkenning(m, use_rsin=False).matchers + + config = OpenKlantConfig.get_solo() + config.use_rsin_for_innNnpId_query_parameter = False + config.save() + + response = self.app.get(self.url, user=data.eherkenning_user) + + # reset noise from signals + self.clearTimelineLogs() + + form = response.forms["profile-edit"] + form["email"] = "new@example.com" + form["phonenumber"] = "0612345678" + form.submit() + + # user data tested in other cases + self.assertTrue(matchers[0].called) + klant_patch_data = matchers[1].request_history[0].json() + self.assertEqual( + klant_patch_data, + { + "emailadres": "new@example.com", + "telefoonnummer": "0612345678", + }, + ) + self.assertTimelineLog("retrieved klant for user") + self.assertTimelineLog( + "patched klant from user profile edit with fields: emailadres, telefoonnummer" + ) @requests_mock.Mocker() def test_modify_phone_updates_klant_api_but_skips_unchanged(self, m): @@ -611,7 +634,7 @@ def test_modify_phone_updates_klant_api_but_skip_unchanged_email(self, m): response = self.app.get(self.url, user=data.user) # reset noise from signals - m.reset_mock() + # m.reset_mock() self.clearTimelineLogs() form = response.forms["profile-edit"] @@ -641,7 +664,7 @@ def test_modify_phone_updates_klant_api_but_skip_unchanged_phone(self, m): response = self.app.get(self.url, user=data.user) # reset noise from signals - m.reset_mock() + # m.reset_mock() self.clearTimelineLogs() form = response.forms["profile-edit"] diff --git a/src/open_inwoner/accounts/tests/test_user_constraints.py b/src/open_inwoner/accounts/tests/test_user_constraints.py index cb88f77092..c4b849c90d 100644 --- a/src/open_inwoner/accounts/tests/test_user_constraints.py +++ b/src/open_inwoner/accounts/tests/test_user_constraints.py @@ -1,5 +1,3 @@ -from unittest import skip - from django.db import IntegrityError, transaction from django.test import TestCase @@ -50,12 +48,10 @@ def test_unique_rsin_when_login_herkenning(self): with self.assertRaises(IntegrityError): UserFactory(rsin="12345678", login_type=LoginTypeChoices.eherkenning) - @skip("Not yet implemented") - def test_not_both_kvk_and_rsin_when_login_eherkenning(self): - with self.assertRaises(IntegrityError): - UserFactory( - kvk="12345678", rsin="12345678", login_type=LoginTypeChoices.eherkenning - ) + def test_both_kvk_and_rsin_when_login_eherkenning(self): + UserFactory( + kvk="12345678", rsin="12345678", login_type=LoginTypeChoices.eherkenning + ) def test_not_bsn_when_login_not_digid(self): for login_type in [ diff --git a/src/open_inwoner/cms/cases/tests/test_contactform.py b/src/open_inwoner/cms/cases/tests/test_contactform.py index e15233fb0b..4292ee29f9 100644 --- a/src/open_inwoner/cms/cases/tests/test_contactform.py +++ b/src/open_inwoner/cms/cases/tests/test_contactform.py @@ -15,7 +15,6 @@ ) from zgw_consumers.constants import APITypes -from open_inwoner.accounts.models import User from open_inwoner.accounts.tests.factories import ( DigidUserFactory, eHerkenningUserFactory, @@ -455,18 +454,15 @@ def test_form_success_with_api(self, m, contactmoment_mock): def test_form_success_with_api_eherkenning_user(self, m, contactmoment_mock): self._setUpMocks(m) self._setUpExtraMocks(m) - + eherkenning_user = eHerkenningUserFactory.create( + kvk="12345678", + rsin="000000000", + email=self.klant["emailadres"], + ) for use_rsin_for_innNnpId_query_parameter in [True, False]: with self.subTest( use_rsin_for_innNnpId_query_parameter=use_rsin_for_innNnpId_query_parameter ): - # FIXME for some reason creating the user outside of the loop - # makes the second iteration fail? - User.objects.filter(kvk="12345678").delete() - eherkenning_user = eHerkenningUserFactory( - kvk="12345678", rsin="000000000" - ) - config = OpenKlantConfig.get_solo() config.use_rsin_for_innNnpId_query_parameter = ( use_rsin_for_innNnpId_query_parameter diff --git a/src/open_inwoner/openklant/tests/data.py b/src/open_inwoner/openklant/tests/data.py index 65c0c4f312..fd5fee89c1 100644 --- a/src/open_inwoner/openklant/tests/data.py +++ b/src/open_inwoner/openklant/tests/data.py @@ -1,7 +1,6 @@ from requests.exceptions import RequestException from zgw_consumers.constants import APITypes -from open_inwoner.accounts.models import User from open_inwoner.accounts.tests.factories import ( DigidUserFactory, eHerkenningUserFactory, @@ -39,13 +38,11 @@ def setUpServices(cls): class MockAPIReadPatchData(MockAPIData): def __init__(self): - User.objects.filter(email="old@example.com").delete() self.user = DigidUserFactory( email="old@example.com", phonenumber="0100000000", ) - User.objects.filter(kvk="12345678").delete() self.eherkenning_user = eHerkenningUserFactory( email="old2@example.com", kvk="12345678", @@ -134,14 +131,14 @@ def install_mocks_eherkenning(self, m, use_rsin=True) -> "MockAPIReadPatchData": class MockAPIReadData(MockAPIData): def __init__(self): - User.objects.filter(bsn="100000001").delete() self.user = DigidUserFactory( bsn="100000001", ) - User.objects.filter(kvk="12345678").delete() + self.eherkenning_user = eHerkenningUserFactory( kvk="12345678", rsin="000000000", + email="foo@bar.com", ) self.klant_bsn = generate_oas_component_cached( @@ -353,11 +350,9 @@ def install_mocks(self, m, link_objectcontactmomenten=False) -> "MockAPIReadData class MockAPICreateData(MockAPIData): def __init__(self): - User.objects.filter(bsn="100000001").delete() self.user = DigidUserFactory( bsn="100000001", ) - User.objects.filter(kvk="12345678").delete() self.eherkenning_user = eHerkenningUserFactory( kvk="12345678", rsin="000000000", diff --git a/src/open_inwoner/openklant/tests/test_contactform.py b/src/open_inwoner/openklant/tests/test_contactform.py index 512f0416f5..4c706006b4 100644 --- a/src/open_inwoner/openklant/tests/test_contactform.py +++ b/src/open_inwoner/openklant/tests/test_contactform.py @@ -434,6 +434,8 @@ def test_submit_and_register_bsn_user_via_api(self, m): def test_submit_and_register_kvk_or_rsin_user_via_api(self, _m): MockAPICreateData.setUpServices() + data = MockAPICreateData() + original_phonenumber = data.eherkenning_user.phonenumber config = OpenKlantConfig.get_solo() config.register_contact_moment = True @@ -455,7 +457,9 @@ def test_submit_and_register_kvk_or_rsin_user_via_api(self, _m): ) config.save() - data = MockAPICreateData() + data.eherkenning_user.phonenumber = original_phonenumber + data.eherkenning_user.save() + data.install_mocks_eherkenning( m, use_rsin=use_rsin_for_innNnpId_query_parameter ) @@ -603,6 +607,7 @@ def test_submit_and_register_bsn_user_via_api_and_update_klant(self, m): def test_submit_and_register_kvk_or_rsin_user_via_api_and_update_klant(self, _m): MockAPICreateData.setUpServices() + data = MockAPICreateData() config = OpenKlantConfig.get_solo() config.register_contact_moment = True @@ -624,7 +629,6 @@ def test_submit_and_register_kvk_or_rsin_user_via_api_and_update_klant(self, _m) ) config.save() - data = MockAPICreateData() data.install_mocks_eherkenning_missing_contact_info( m, use_rsin=use_rsin_for_innNnpId_query_parameter ) diff --git a/src/open_inwoner/openklant/tests/test_views.py b/src/open_inwoner/openklant/tests/test_views.py index d143564c12..f464e36120 100644 --- a/src/open_inwoner/openklant/tests/test_views.py +++ b/src/open_inwoner/openklant/tests/test_views.py @@ -151,6 +151,8 @@ def test_list_for_bsn_new_answer_available(self, m, mock_get_kcm_answer_mapping) self.assertIn(_("Nieuw antwoord beschikbaar"), response.text) def test_list_for_kvk_or_rsin(self, m, mock_get_kcm_answer_mapping): + data = MockAPIReadData().install_mocks(m) + for use_rsin_for_innNnpId_query_parameter in [True, False]: with self.subTest( use_rsin_for_innNnpId_query_parameter=use_rsin_for_innNnpId_query_parameter @@ -161,8 +163,6 @@ def test_list_for_kvk_or_rsin(self, m, mock_get_kcm_answer_mapping): ) config.save() - data = MockAPIReadData().install_mocks(m) - detail_url = reverse( "cases:contactmoment_detail", kwargs={ @@ -448,6 +448,8 @@ def test_display_contactmoment_subject_no_mapping_fallback( ) def test_show_detail_for_kvk_or_rsin(self, m, mock_get_kcm_answer_mapping): + data = MockAPIReadData().install_mocks(m) + for use_rsin_for_innNnpId_query_parameter in [True, False]: with self.subTest( use_rsin_for_innNnpId_query_parameter=use_rsin_for_innNnpId_query_parameter @@ -461,8 +463,6 @@ def test_show_detail_for_kvk_or_rsin(self, m, mock_get_kcm_answer_mapping): ) config.save() - data = MockAPIReadData().install_mocks(m) - detail_url = reverse( "cases:contactmoment_detail", kwargs={