👌 [#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)

Author: stevenbal

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)

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):

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)

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)

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 _
 
@@ -87,6 +88,7 @@ def test_search(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