Merge pull request #1170 from maykinmedia/fix/2303-laposta-issues

✨ [#2303] Improvements/fixes for Laposta

src/open_inwoner/accounts/tests/test_profile_views.py

@@ -4,7 +4,7 @@
 
 from django.conf import settings
 from django.template.defaultfilters import date as django_date
-from django.test import override_settings
+from django.test import override_settings, tag
 from django.urls import reverse, reverse_lazy
 from django.utils.translation import gettext as _
 
@@ -1038,6 +1038,7 @@ def test_collaborate_notifications_display(self):
         self.assertIn("plans_notifications", form.fields)
 
 
+@tag("laposta")
 @requests_mock.Mocker()
 @override_settings(
     ROOT_URLCONF="open_inwoner.cms.tests.urls", MIDDLEWARE=PATCHED_MIDDLEWARE
@@ -1131,10 +1132,10 @@ def test_render_form_if_newsletters_are_found(self, m):
         self.assertIn("Nieuwsbrief1", response.text)
         self.assertIn("Nieuwsbrief2", response.text)
 
-    def test_render_form_limit_newsletters_to_admin_selection(self, m):
+    def test_save_form_with_errors(self, m):
         self.setUpMocks(m)
 
-        self.config.limit_list_selection_to = ["list1"]
+        self.config.limit_list_selection_to = ["list1", "list2"]
         self.config.save()
 
         m.get(
@@ -1164,8 +1165,52 @@ def test_render_form_limit_newsletters_to_admin_selection(self, m):
         self.assertTrue(form.fields["newsletters"][0].checked)
         self.assertIn("Nieuwsbrief1", response.text)
 
-        # Second field was excluded by `LapostaConfig.limit_list_selection_to`
-        self.assertNotIn("Nieuwsbrief2", response.text)
+        post_matcher = m.post(
+            f"{self.config.api_root}member",
+            json={
+                "error": {
+                    "type": "internal",
+                    "message": "Internal server error",
+                }
+            },
+            status_code=500,
+        )
+        delete_matcher = m.delete(
+            f"{self.config.api_root}member/{self.user.email}?list_id=list1",
+            json={
+                "error": {
+                    "type": "internal",
+                    "message": "Internal server error",
+                }
+            },
+            status_code=500,
+        )
+
+        form["newsletters"] = ["list2"]
+        response = form.submit("newsletter-submit")
+
+        subscribe_error, unsubscribe_error = response.pyquery(
+            ".notifications__errors .notification__content"
+        )
+
+        self.assertEqual(
+            PQ(subscribe_error).text(),
+            _(
+                "Something went wrong while trying to subscribe to '{list_name}', please try again later"
+            ).format(list_name="Nieuwsbrief2"),
+        )
+        self.assertEqual(
+            PQ(unsubscribe_error).text(),
+            _(
+                "Something went wrong while trying to unsubscribe from '{list_name}', please try again later"
+            ).format(list_name="Nieuwsbrief1"),
+        )
+
+        form = response.forms["newsletter-form"]
+
+        # The initial data should be kept the same as the last POST
+        self.assertFalse(form.fields["newsletters"][0].checked)
+        self.assertTrue(form.fields["newsletters"][1].checked)
 
     def test_do_not_render_form_if_email_not_verified(self, m):
         self.setUpMocks(m)
@@ -1192,6 +1237,42 @@ def test_do_not_render_form_if_email_not_verified(self, m):
 
         self.assertNotIn("newsletter-form", response.forms)
 
+    def test_render_form_limit_newsletters_to_admin_selection(self, m):
+        self.setUpMocks(m)
+
+        self.config.limit_list_selection_to = ["list1"]
+        self.config.save()
+
+        m.get(
+            f"{self.config.api_root}member/{self.user.email}?list_id=list1",
+            json={
+                "member": MemberFactory.build(
+                    list_id="list1",
+                    member_id="1234567",
+                    email=self.user.email,
+                    custom_fields=None,
+                ).model_dump()
+            },
+        )
+        m.get(
+            f"{self.config.api_root}member/{self.user.email}?list_id=list2",
+            status_code=400,
+        )
+
+        response = self.app.get(self.profile_url, user=self.user)
+
+        self.assertIn(_("Nieuwsbrieven"), response.text)
+        self.assertIn("newsletter-form", response.forms)
+
+        form = response.forms["newsletter-form"]
+
+        # First checkbox should be checked, because the user is already subscribed
+        self.assertTrue(form.fields["newsletters"][0].checked)
+        self.assertIn("Nieuwsbrief1", response.text)
+
+        # Second field was excluded by `LapostaConfig.limit_list_selection_to`
+        self.assertNotIn("Nieuwsbrief2", response.text)
+
 
 @requests_mock.Mocker()
 @override_settings(

src/open_inwoner/accounts/views/profile.py

@@ -55,7 +55,7 @@ def get_success_url(self) -> str:
 
     def get_form_kwargs(self) -> dict[str, Any]:
         kwargs = super().get_form_kwargs()
-        kwargs["user"] = self.request.user
+        kwargs["request"] = self.request
         return kwargs
 
     def form_valid(self, form):

src/open_inwoner/laposta/client.py

@@ -43,7 +43,7 @@ def get_lists(self) -> list[LapostaList]:
 
     def create_subscription(self, list_id: str, user_data: UserData) -> Member | None:
         response = self.post(
-            "member", data={"list_id": list_id, **user_data.model_dump()}
+            "member", json={"list_id": list_id, **user_data.model_dump()}
         )
 
         if response.status_code == 400:

src/open_inwoner/laposta/forms.py

@@ -1,6 +1,7 @@
 import logging
 
 from django import forms
+from django.contrib import messages
 from django.core.exceptions import ValidationError
 from django.utils.translation import gettext_lazy as _
 
@@ -25,10 +26,10 @@ class NewsletterSubscriptionForm(forms.Form):
         widget=forms.widgets.CheckboxSelectMultiple,
     )
 
-    def __init__(self, user=None, *args, **kwargs):
+    def __init__(self, request=None, *args, **kwargs):
         super().__init__(**kwargs)
 
-        if not user.has_verified_email():
+        if not request.user.has_verified_email():
             return
 
         if laposta_client := create_laposta_client():
@@ -42,11 +43,15 @@ def __init__(self, user=None, *args, **kwargs):
                     if choice[0] in limited_to
                 ]
 
-            self.fields[
-                "newsletters"
-            ].initial = laposta_client.get_subscriptions_for_email(
-                limited_to, user.verified_email
-            )
+            # In case of errors, we want to keep the same data selected
+            if "newsletters" in request.POST:
+                initial_data = request.POST.getlist("newsletters")
+            else:
+                initial_data = laposta_client.get_subscriptions_for_email(
+                    limited_to, request.user.verified_email
+                )
+
+            self.fields["newsletters"].initial = initial_data
 
     def save(self, request, *args, **kwargs):
         user: User = request.user
@@ -58,13 +63,14 @@ def save(self, request, *args, **kwargs):
             return
 
         newsletters = self.cleaned_data["newsletters"]
+        has_errors = False
 
         list_name_mapping = dict(self.fields["newsletters"].choices)
         user_data = UserData(
             ip=get_client_ip(request)[0],
             email=user.verified_email,
             source_url=None,
-            custom_fields=None,
+            custom_fields={"toestemming": "Ja, ik wil de nieuwsbrief ontvangen"},
             options=None,
         )
         limited_to = LapostaConfig.get_solo().limit_list_selection_to
@@ -81,6 +87,7 @@ def save(self, request, *args, **kwargs):
                 logger.exception(
                     "Something went wrong while trying to create subscription"
                 )
+                has_errors = True
                 self.add_error(
                     "newsletters",
                     ValidationError(
@@ -99,6 +106,7 @@ def save(self, request, *args, **kwargs):
                 logger.exception(
                     "Something went wrong while trying to delete subscription"
                 )
+                has_errors = True
                 self.add_error(
                     "newsletters",
                     ValidationError(
@@ -108,3 +116,8 @@ def save(self, request, *args, **kwargs):
                         ).format(list_name=list_name_mapping[list_id])
                     ),
                 )
+
+        if has_errors:
+            messages.warning(
+                request, _("Er ging iets mis bij het opslaan van je voorkeuren")
+            )

src/open_inwoner/laposta/migrations/0005_alter_lapostaconfig_limit_list_selection_to.py

@@ -0,0 +1,26 @@
+# Generated by Django 4.2.11 on 2024-04-22 14:05
+
+import django.contrib.postgres.fields
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("laposta", "0004_delete_subscription"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="lapostaconfig",
+            name="limit_list_selection_to",
+            field=django.contrib.postgres.fields.ArrayField(
+                base_field=models.CharField(max_length=255),
+                blank=True,
+                default=list,
+                help_text="If configured, users will only be able to choose from this selection of lists to subscribe to. Note: the list of newsletters is cached, so it may take up to 15 minutes before newly added newsletters show up here.",
+                size=None,
+                verbose_name="Limit list selection to",
+            ),
+        ),
+    ]

src/open_inwoner/laposta/models.py

@@ -33,7 +33,8 @@ class LapostaConfig(SingletonModel):
         blank=True,
         help_text=_(
             "If configured, users will only be able to choose from this selection of "
-            "lists to subscribe to"
+            "lists to subscribe to. Note: the list of newsletters is cached, so it may take "
+            "up to 15 minutes before newly added newsletters show up here."
         ),
     )
 

src/open_inwoner/laposta/tests/test_forms.py

@@ -1,12 +1,13 @@
 from unittest.mock import patch
-from urllib.parse import parse_qs
 
-from django.test import RequestFactory, TestCase
+from django.contrib.messages.middleware import MessageMiddleware
+from django.test import TestCase, tag
 from django.utils.translation import gettext as _
 
 import requests_mock
 
 from open_inwoner.accounts.tests.factories import DigidUserFactory
+from open_inwoner.cms.tests.cms_tools import get_request
 from open_inwoner.utils.test import ClearCachesMixin
 
 from ..forms import NewsletterSubscriptionForm
@@ -16,6 +17,7 @@
 LAPOSTA_API_ROOT = "https://laposta.local/api/v2/"
 
 
+@tag("laposta")
 @requests_mock.Mocker()
 class NewsletterSubscriptionFormTestCase(ClearCachesMixin, TestCase):
     def setUp(self):
@@ -26,8 +28,8 @@ def setUp(self):
         self.user.save()
         self.assertTrue(self.user.has_verified_email())
 
-        self.request = RequestFactory().get("/")
-        self.request.user = self.user
+        self.request = get_request(user=self.user)
+        MessageMiddleware(lambda x: x).process_request(self.request)
 
         self.config = LapostaConfig.get_solo()
         self.config.api_root = LAPOSTA_API_ROOT
@@ -90,13 +92,13 @@ def test_save_form(self, m):
             f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=789", status_code=400
         )
 
-        form = NewsletterSubscriptionForm(data={}, user=self.user)
+        form = NewsletterSubscriptionForm(data={}, request=self.request)
 
         # User already has a subscription for the first two newsletters
         self.assertEqual(form["newsletters"].initial, ["123", "456"])
 
         form = NewsletterSubscriptionForm(
-            data={"newsletters": ["456", "789"]}, user=self.user
+            data={"newsletters": ["456", "789"]}, request=self.request
         )
 
         self.assertTrue(form.is_valid())
@@ -127,8 +129,15 @@ def test_save_form(self, m):
         [post_request] = post_matcher.request_history
 
         self.assertEqual(
-            parse_qs(post_request.body),
-            {"list_id": ["789"], "ip": ["127.0.0.1"], "email": [self.user.email]},
+            post_request.json(),
+            {
+                "list_id": "789",
+                "ip": "127.0.0.1",
+                "email": self.user.email,
+                "custom_fields": {"toestemming": "Ja, ik wil de nieuwsbrief ontvangen"},
+                "source_url": None,
+                "options": None,
+            },
         )
 
         # Because list_id 123 was present in the
@@ -155,7 +164,9 @@ def test_save_form_create_duplicate_subscription(self, m):
             f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=789", status_code=400
         )
 
-        form = NewsletterSubscriptionForm(data={"newsletters": ["789"]}, user=self.user)
+        form = NewsletterSubscriptionForm(
+            data={"newsletters": ["789"]}, request=self.request
+        )
 
         self.assertTrue(form.is_valid())
 
@@ -204,7 +215,9 @@ def test_save_form_delete_non_existent_subscription(self, m):
             },
         )
 
-        form = NewsletterSubscriptionForm(data={"newsletters": []}, user=self.user)
+        form = NewsletterSubscriptionForm(
+            data={"newsletters": []}, request=self.request
+        )
 
         self.assertTrue(form.is_valid())
 
@@ -249,7 +262,9 @@ def test_save_form_raises_errors(self, m):
             f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=789", status_code=400
         )
 
-        form = NewsletterSubscriptionForm(data={"newsletters": ["789"]}, user=self.user)
+        form = NewsletterSubscriptionForm(
+            data={"newsletters": ["789"]}, request=self.request
+        )
 
         self.assertTrue(form.is_valid())
 
@@ -317,7 +332,7 @@ def test_form__disables_when_email_not_verified(self, m, mock_create_client):
             },
         )
 
-        form = NewsletterSubscriptionForm(data={}, user=self.user)
+        form = NewsletterSubscriptionForm(data={}, request=self.request)
 
         # We don't get any newletters if not using verified email
         self.assertEqual(form["newsletters"].initial, None)
@@ -326,7 +341,9 @@ def test_form__disables_when_email_not_verified(self, m, mock_create_client):
         mock_create_client.assert_not_called()
 
         # Check forced save() doesn't do anything
-        form = NewsletterSubscriptionForm(data={"newsletters": ["123"]}, user=self.user)
+        form = NewsletterSubscriptionForm(
+            data={"newsletters": ["123"]}, request=self.request
+        )
         form.save(self.request)
 
         # Client was never built (and no request-mock exception was thrown)