Merge pull request #1091 from maykinmedia/feature/2192-newsletter-sub…

…scribe-error-handling

✨ [#2192] Display newsletter form errors to user

src/open_inwoner/accounts/views/profile.py

@@ -331,6 +331,13 @@ def get_form_kwargs(self) -> dict[str, Any]:
     def form_valid(self, form):
         form.save(self.request)
 
+        # Display errors raised by Laposta API
+        if form.errors:
+            self.log_user_action(
+                self.request.user, _("failed to modify user newsletter subscription")
+            )
+            return self.form_invalid(form)
+
         messages.success(self.request, _("Uw wijzigingen zijn opgeslagen"))
         self.log_user_action(
             self.request.user, _("users newsletter subscriptions were modified")

src/open_inwoner/laposta/client.py

@@ -32,39 +32,40 @@ def get_lists(self) -> list[LapostaList]:
         return lists
 
     def create_subscription(self, list_id: str, user_data: UserData) -> Member | None:
-        try:
-            response = self.post(
-                "member", data={"list_id": list_id, **user_data.dict()}
-            )
-            if response.status_code == 400:
-                data = response.json()
-                # Handle scenario where a subscription exists in the API, but not locally
-                if data.get("error", {}).get("message") == "Email address exists":
-                    logger.info("Subscription already exists for user")
-                    return Member(
-                        member_id=data["error"]["member_id"],
-                        list_id=list_id,
-                        email=user_data.email,
-                        ip=user_data.ip,
-                    )
-            data = get_json_response(response)
-        except (RequestException, ClientError) as e:
-            logger.exception("exception while making request", exc_info=e)
-            return None
-
+        response = self.post("member", data={"list_id": list_id, **user_data.dict()})
+
+        if response.status_code == 400:
+            data = response.json()
+            error = data.get("error", {})
+            # Handle scenario where a subscription exists in the API, but not locally
+            if error.get("code") == 204 and error.get("parameter") == "email":
+                logger.info("Subscription already exists for user")
+                return Member(
+                    member_id=data["error"]["member_id"],
+                    list_id=list_id,
+                    email=user_data.email,
+                    ip=user_data.ip,
+                )
+
+        data = get_json_response(response)
         if not data:
             return None
 
         return Member(**data["member"])
 
     def remove_subscription(self, list_id: str, member_id: str) -> Member | None:
-        try:
-            response = self.delete(f"member/{member_id}", params={"list_id": list_id})
-            data = get_json_response(response)
-        except (RequestException, ClientError) as e:
-            logger.exception("exception while making request", exc_info=e)
-            return None
-
+        response = self.delete(f"member/{member_id}", params={"list_id": list_id})
+
+        if response.status_code == 400:
+            data = response.json()
+            error = data.get("error", {})
+            # Handle scenario where a subscription does not exists in the API,
+            # but it does exist locally
+            if error.get("code") == 203 and error.get("parameter") == "member_id":
+                logger.info("Subscription does not exist for user")
+                return None
+
+        data = get_json_response(response)
         if not data:
             return None
 

src/open_inwoner/laposta/forms.py

@@ -1,14 +1,21 @@
+import logging
+
 from django import forms
+from django.core.exceptions import ValidationError
 from django.utils.translation import gettext_lazy as _
 
 from ipware import get_client_ip
+from requests.exceptions import RequestException
 
 from open_inwoner.laposta.api_models import UserData
 from open_inwoner.laposta.models import LapostaConfig, Subscription
+from open_inwoner.utils.api import ClientError
 
 from .choices import get_list_choices
 from .client import create_laposta_client
 
+logger = logging.getLogger(__name__)
+
 
 class NewsletterSubscriptionForm(forms.Form):
     newsletters = forms.MultipleChoiceField(
@@ -38,26 +45,45 @@ def __init__(self, user=None, *args, **kwargs):
     def save(self, request, *args, **kwargs):
         newsletters = self.cleaned_data["newsletters"]
 
-        if laposta_client := create_laposta_client():
-            user_data = UserData(
-                ip=get_client_ip(request)[0],
-                email=request.user.email,
-                source_url=None,
-                custom_fields=None,
-                options=None,
+        client = create_laposta_client()
+        if not client:
+            return
+
+        list_name_mapping = dict(self.fields["newsletters"].choices)
+        user_data = UserData(
+            ip=get_client_ip(request)[0],
+            email=request.user.email,
+            source_url=None,
+            custom_fields=None,
+            options=None,
+        )
+        existing_subscriptions = set(
+            Subscription.objects.filter(user=request.user).values_list(
+                "list_id", flat=True
             )
-            existing_subscriptions = set(
-                Subscription.objects.filter(user=request.user).values_list(
-                    "list_id", flat=True
+        )
+
+        to_create = []
+        for list_id in newsletters:
+            if list_id in existing_subscriptions:
+                continue
+
+            try:
+                member = client.create_subscription(list_id, user_data)
+            except (RequestException, ClientError):
+                logger.exception(
+                    "Something went wrong while trying to create subscription"
                 )
-            )
-
-            to_create = []
-            for list_id in newsletters:
-                if list_id in existing_subscriptions:
-                    continue
-
-                member = laposta_client.create_subscription(list_id, user_data)
+                self.add_error(
+                    "newsletters",
+                    ValidationError(
+                        _(
+                            "Something went wrong while trying to subscribe "
+                            "to '{list_name}', please try again later"
+                        ).format(list_name=list_name_mapping[list_id])
+                    ),
+                )
+            else:
                 if member:
                     to_create.append(
                         Subscription(
@@ -67,17 +93,33 @@ def save(self, request, *args, **kwargs):
                         )
                     )
 
-            if to_create:
-                Subscription.objects.bulk_create(to_create)
-
-            unsubscribe_from_ids = existing_subscriptions - set(newsletters)
-            unsubscribe_from = Subscription.objects.filter(
-                user=request.user, list_id__in=unsubscribe_from_ids
-            )
-
-            for subscription in unsubscribe_from:
-                laposta_client.remove_subscription(
-                    subscription.list_id, subscription.member_id
+        if to_create:
+            Subscription.objects.bulk_create(to_create)
+
+        unsubscribe_from_ids = existing_subscriptions - set(newsletters)
+        unsubscribe_from = Subscription.objects.filter(
+            user=request.user, list_id__in=unsubscribe_from_ids
+        )
+
+        to_delete_ids = []
+        for subscription in unsubscribe_from:
+            try:
+                client.remove_subscription(subscription.list_id, subscription.member_id)
+            except (RequestException, ClientError):
+                logger.exception(
+                    "Something went wrong while trying to delete subscription"
                 )
-
-            unsubscribe_from.delete()
+                self.add_error(
+                    "newsletters",
+                    ValidationError(
+                        _(
+                            "Something went wrong while trying to unsubscribe "
+                            "from '{list_name}', please try again later"
+                        ).format(list_name=list_name_mapping[subscription.list_id])
+                    ),
+                )
+            else:
+                to_delete_ids.append(subscription.list_id)
+        Subscription.objects.filter(
+            user=request.user, list_id__in=to_delete_ids
+        ).delete()

src/open_inwoner/laposta/tests/test_forms.py

@@ -1,6 +1,7 @@
 from urllib.parse import parse_qs
 
 from django.test import RequestFactory, TestCase
+from django.utils.translation import gettext as _
 
 import requests_mock
 
@@ -186,3 +187,61 @@ def test_save_form_delete_non_existent_subscription(self, m):
         self.assertEqual(len(delete_matcher.request_history), 1)
 
         self.assertFalse(Subscription.objects.filter(user=self.user).exists())
+
+    def test_save_form_raises_errors(self, m):
+        """
+        Form should return errors if unexpected errors occur when performing API calls
+        """
+        self.setUpMocks(m)
+
+        SubscriptionFactory.create(list_id="456", member_id="member456", user=self.user)
+
+        form = NewsletterSubscriptionForm(data={"newsletters": ["789"]}, user=self.user)
+
+        self.assertTrue(form.is_valid())
+
+        post_matcher = m.post(
+            f"{LAPOSTA_API_ROOT}member",
+            json={
+                "error": {
+                    "type": "internal",
+                    "message": "Internal server error",
+                }
+            },
+            status_code=500,
+        )
+        delete_matcher = m.delete(
+            f"{LAPOSTA_API_ROOT}member/member456?list_id=456",
+            json={
+                "error": {
+                    "type": "internal",
+                    "message": "Internal server error",
+                }
+            },
+            status_code=500,
+        )
+
+        form.save(self.request)
+
+        self.assertEqual(len(post_matcher.request_history), 1)
+        self.assertEqual(len(delete_matcher.request_history), 1)
+        self.assertEqual(
+            form.errors,
+            {
+                "newsletters": [
+                    _(
+                        "Something went wrong while trying to subscribe to "
+                        "'{list_name}', please try again later"
+                    ).format(list_name="Nieuwsbrief3: baz"),
+                    _(
+                        "Something went wrong while trying to unsubscribe from "
+                        "'{list_name}', please try again later"
+                    ).format(list_name="Nieuwsbrief2: bar"),
+                ]
+            },
+        )
+
+        # Local subscription should not be deleted
+        subscription = Subscription.objects.get(user=self.user)
+
+        self.assertEqual(subscription.list_id, "456")