From 5e1ee415882e86e3b9857a4e249afa6ce1d6049d Mon Sep 17 00:00:00 2001 From: Steven Bal Date: Fri, 25 Oct 2024 10:22:54 +0200 Subject: [PATCH] :ok_hand: [#234] PR feedback * add explicit API test to check if no validation is applied if soort != email * remove re-raising of validationerrors for email validation in case of API context * move retrieval of soort_digitaal_adres to caller code (admin/serializer.validate) --- .../klantinteracties/admin/digitaal_adres.py | 6 ++--- .../api/serializers/digitaal_adres.py | 13 +++++++++- .../api/tests/test_digitaal_adres.py | 20 ++++++++++++++++ .../klantinteracties/api/validators.py | 24 ++++--------------- .../klantinteracties/tests/test_admin.py | 6 +++-- 5 files changed, 44 insertions(+), 25 deletions(-) diff --git a/src/openklant/components/klantinteracties/admin/digitaal_adres.py b/src/openklant/components/klantinteracties/admin/digitaal_adres.py index 034a5dc4..04516b50 100644 --- a/src/openklant/components/klantinteracties/admin/digitaal_adres.py +++ b/src/openklant/components/klantinteracties/admin/digitaal_adres.py @@ -10,10 +10,10 @@ class Meta: model = DigitaalAdres fields = "__all__" - def clean(self): + def clean_adres(self): data = self.cleaned_data - OptionalEmailValidator()(data, None) - return data + OptionalEmailValidator()(data["adres"], data.get("soort_digitaal_adres")) + return data["adres"] @admin.register(DigitaalAdres) diff --git a/src/openklant/components/klantinteracties/api/serializers/digitaal_adres.py b/src/openklant/components/klantinteracties/api/serializers/digitaal_adres.py index 758487a3..868a7b3d 100644 --- a/src/openklant/components/klantinteracties/api/serializers/digitaal_adres.py +++ b/src/openklant/components/klantinteracties/api/serializers/digitaal_adres.py @@ -13,6 +13,7 @@ from openklant.components.klantinteracties.models.digitaal_adres import DigitaalAdres from openklant.components.klantinteracties.models.klantcontacten import Betrokkene from openklant.components.klantinteracties.models.partijen import Partij +from openklant.utils.serializers import get_field_value class DigitaalAdresForeignKeySerializer(serializers.HyperlinkedModelSerializer): @@ -88,7 +89,17 @@ class Meta: "help_text": _("De unieke URL van dit digitaal adres binnen deze API."), }, } - validators = [OptionalEmailValidator()] + + def validate_adres(self, adres): + """ + Define the validator here, to avoid DRF spectacular marking the format for + `adres` as `email` + """ + soort_digitaal_adres = get_field_value( + self, self.initial_data, "soort_digitaal_adres" + ) + OptionalEmailValidator()(adres, soort_digitaal_adres) + return adres @transaction.atomic def update(self, instance, validated_data): diff --git a/src/openklant/components/klantinteracties/api/tests/test_digitaal_adres.py b/src/openklant/components/klantinteracties/api/tests/test_digitaal_adres.py index 333a90b1..a4fb7925 100644 --- a/src/openklant/components/klantinteracties/api/tests/test_digitaal_adres.py +++ b/src/openklant/components/klantinteracties/api/tests/test_digitaal_adres.py @@ -1,3 +1,4 @@ +from django.test import tag from django.utils.translation import gettext as _ from rest_framework import status @@ -92,6 +93,7 @@ def test_create_digitaal_adres(self): self.assertEqual(data["adres"], "foobar@example.com") self.assertEqual(data["omschrijving"], "omschrijving") + @tag("gh-234") def test_create_digitaal_adres_email_validation(self): list_url = reverse("klantinteracties:digitaaladres-list") data = { @@ -140,6 +142,24 @@ def test_create_digitaal_adres_email_validation(self): ], ) + with self.subTest("no validation applied if soort is not email"): + response = self.client.patch( + detail_url, + { + "soortDigitaalAdres": SoortDigitaalAdres.telefoonnummer, + "adres": "0612345678", + }, + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + + digitaal_adres.refresh_from_db() + + self.assertEqual( + digitaal_adres.soort_digitaal_adres, SoortDigitaalAdres.telefoonnummer + ) + self.assertEqual(digitaal_adres.adres, "0612345678") + def test_update_digitaal_adres(self): betrokkene, betrokkene2 = BetrokkeneFactory.create_batch(2) partij, partij2 = PartijFactory.create_batch(2) diff --git a/src/openklant/components/klantinteracties/api/validators.py b/src/openklant/components/klantinteracties/api/validators.py index ba1e1e49..66cdc5b4 100644 --- a/src/openklant/components/klantinteracties/api/validators.py +++ b/src/openklant/components/klantinteracties/api/validators.py @@ -1,9 +1,8 @@ -from django.core.exceptions import ValidationError from django.core.validators import EmailValidator from django.db import models from django.utils.translation import gettext_lazy as _ -from rest_framework import exceptions, serializers +from rest_framework import serializers from rest_framework.validators import UniqueTogetherValidator, qs_filter from openklant.components.klantinteracties.constants import SoortDigitaalAdres @@ -26,7 +25,6 @@ PartijIdentificator, ) from openklant.components.klantinteracties.models.rekeningnummers import Rekeningnummer -from openklant.utils.serializers import get_field_value class FKUniqueTogetherValidator(UniqueTogetherValidator): @@ -175,21 +173,9 @@ def Rekeningnummer_exists(value): class OptionalEmailValidator(EmailValidator): """ EmailValidator for SoortDigitaalAdres that only attempts to validate if - `SoortDigitaalAdres` is `email + `SoortDigitaalAdres` is `email` """ - requires_context = True - - def __call__(self, attrs: dict, serializer): - if ( - get_field_value(serializer, attrs, "soort_digitaal_adres") - == SoortDigitaalAdres.email - ): - try: - super().__call__(get_field_value(serializer, attrs, "adres")) - except ValidationError as e: - # re-raise as field error - if serializer: - raise exceptions.ValidationError({"adres": e.message}) - else: - raise ValidationError({"adres": e}) + def __call__(self, value: str, soort_digitaal_adres: str): + if soort_digitaal_adres == SoortDigitaalAdres.email: + super().__call__(value) diff --git a/src/openklant/components/klantinteracties/tests/test_admin.py b/src/openklant/components/klantinteracties/tests/test_admin.py index 8adc08bc..3c531b34 100644 --- a/src/openklant/components/klantinteracties/tests/test_admin.py +++ b/src/openklant/components/klantinteracties/tests/test_admin.py @@ -1,5 +1,6 @@ from uuid import uuid4 +from django.test import tag from django.urls import reverse from django.utils.translation import gettext as _ @@ -94,8 +95,8 @@ def test_digitaal_adres_inline(self): betrokkene should be optional """ - SuperUserFactory(username="admin", password="admin") - self._login_user(username="admin", password="admin") + user = SuperUserFactory(username="admin", password="admin") + self.app.set_user(user) partij = PartijFactory(soort_partij="persoon", voorkeurs_digitaal_adres=None) PersoonFactory(partij=partij) @@ -121,6 +122,7 @@ def test_digitaal_adres_inline(self): @disable_admin_mfa() class DigitaalAdresAdminTests(WebTest): + @tag("gh-234") def test_email_validation(self): """ Check that the admin applies conditional email validation on `adres`, only if