From 1f76768610ab145fe6becc8a5e51e23df0da1db7 Mon Sep 17 00:00:00 2001 From: Bart van der Schoor Date: Thu, 11 Apr 2024 12:31:23 +0200 Subject: [PATCH 1/3] [#2291 #2273 #2233] Use verified email for Laposta newsletters and Qmatic appointments --- .../accounts/tests/test_profile_views.py | 10 +++++ src/open_inwoner/accounts/views/profile.py | 25 +++++++---- .../cms/plugins/cms_plugins/appointments.py | 19 +++++---- .../cms/plugins/tests/test_appointments.py | 18 ++++++++ src/open_inwoner/laposta/forms.py | 18 +++++--- src/open_inwoner/laposta/tests/test_forms.py | 42 +++++++++++++++++++ src/open_inwoner/qmatic/tests/data.py | 5 ++- 7 files changed, 113 insertions(+), 24 deletions(-) diff --git a/src/open_inwoner/accounts/tests/test_profile_views.py b/src/open_inwoner/accounts/tests/test_profile_views.py index ee7d2eec97..c6f4fd03db 100644 --- a/src/open_inwoner/accounts/tests/test_profile_views.py +++ b/src/open_inwoner/accounts/tests/test_profile_views.py @@ -1165,6 +1165,7 @@ def setUp(self): super().setUp() self.data = QmaticMockData() + self.assertTrue(self.data.user.has_verified_email()) def test_do_not_render_list_if_config_is_missing(self, m): self.data.config.service = None @@ -1194,6 +1195,15 @@ def test_do_not_render_list_if_validation_error(self, m): self.assertIn(_("Geen afspraken beschikbaar"), response.text) + def test_do_not_render_list_if_email_not_verified(self, m): + self.data.user.verified_email = "" + self.data.user.save() + self.assertFalse(self.data.user.has_verified_email()) + + response = self.app.get(self.appointments_url, user=self.data.user) + + self.assertIn(_("Geen afspraken beschikbaar"), response.text) + def test_render_list_if_appointments_are_found(self, m): self.data.setUpMocks(m) diff --git a/src/open_inwoner/accounts/views/profile.py b/src/open_inwoner/accounts/views/profile.py index b0e9330b19..e8ad9c4957 100644 --- a/src/open_inwoner/accounts/views/profile.py +++ b/src/open_inwoner/accounts/views/profile.py @@ -338,22 +338,29 @@ def form_valid(self, form): class UserAppointmentsView( - LogMixin, LoginRequiredMixin, CommonPageMixin, BaseBreadcrumbMixin, TemplateView + LogMixin, + LoginRequiredMixin, + CommonPageMixin, + BaseBreadcrumbMixin, + TemplateView, ): template_name = "pages/profile/appointments.html" def get_context_data(self, **kwargs) -> dict[str, Any]: context = super().get_context_data(**kwargs) - # TODO email should be verified - try: - client = QmaticClient() - except NoServiceConfigured: - logger.exception("Error occurred while creating Qmatic client") + user: User = self.request.user + if not user.has_verified_email(): context["appointments"] = [] else: - context["appointments"] = client.list_appointments_for_customer( - self.request.user.email - ) + try: + client = QmaticClient() + except NoServiceConfigured: + logger.exception("Error occurred while creating Qmatic client") + context["appointments"] = [] + else: + context["appointments"] = client.list_appointments_for_customer( + user.verified_email + ) return context @cached_property diff --git a/src/open_inwoner/cms/plugins/cms_plugins/appointments.py b/src/open_inwoner/cms/plugins/cms_plugins/appointments.py index 9dc30457d5..1837f0b146 100644 --- a/src/open_inwoner/cms/plugins/cms_plugins/appointments.py +++ b/src/open_inwoner/cms/plugins/cms_plugins/appointments.py @@ -20,17 +20,18 @@ class UserAppointmentsPlugin(CMSPluginBase): def render(self, context, instance, placeholder): request = context["request"] - # TODO email should be verified - if not request.user.is_authenticated: - return context - - try: - client = QmaticClient() - except NoServiceConfigured: - logger.exception("Error occurred while creating Qmatic client") + if not request.user.is_authenticated or not request.user.has_verified_email(): appointments = [] else: - appointments = client.list_appointments_for_customer(request.user.email) + try: + client = QmaticClient() + except NoServiceConfigured: + logger.exception("Error occurred while creating Qmatic client") + appointments = [] + else: + appointments = client.list_appointments_for_customer( + request.user.verified_email + ) context.update( { diff --git a/src/open_inwoner/cms/plugins/tests/test_appointments.py b/src/open_inwoner/cms/plugins/tests/test_appointments.py index 1752887b44..1b6bb157eb 100644 --- a/src/open_inwoner/cms/plugins/tests/test_appointments.py +++ b/src/open_inwoner/cms/plugins/tests/test_appointments.py @@ -17,6 +17,8 @@ def test_plugin(self, m): data = QmaticMockData() data.setUpMocks(m) + self.assertTrue(data.user.has_verified_email()) + html, context = cms_tools.render_plugin( UserAppointmentsPlugin, plugin_data={}, user=data.user ) @@ -46,3 +48,19 @@ def test_plugin(self, m): action_url = items[0].attrib["href"] self.assertEqual(action_url, reverse("profile:appointments")) + + def test_plugin__email_not_verified(self, m): + data = QmaticMockData() + data.setUpMocks(m) + data.user.verified_email = "" + data.user.save() + self.assertFalse(data.user.has_verified_email()) + + html, context = cms_tools.render_plugin( + UserAppointmentsPlugin, plugin_data={}, user=data.user + ) + + appointments = context["appointments"] + + # zero results + self.assertEqual(len(appointments), 0) diff --git a/src/open_inwoner/laposta/forms.py b/src/open_inwoner/laposta/forms.py index 6f2de4b0cb..9898bdca14 100644 --- a/src/open_inwoner/laposta/forms.py +++ b/src/open_inwoner/laposta/forms.py @@ -11,6 +11,7 @@ from open_inwoner.laposta.models import LapostaConfig from open_inwoner.utils.api import ClientError +from ..accounts.models import User from .choices import get_list_choices from .client import create_laposta_client @@ -27,6 +28,9 @@ class NewsletterSubscriptionForm(forms.Form): def __init__(self, user=None, *args, **kwargs): super().__init__(**kwargs) + if not user.has_verified_email(): + return + if laposta_client := create_laposta_client(): lists = laposta_client.get_lists() self.fields["newsletters"].choices = get_list_choices(lists) @@ -40,27 +44,31 @@ def __init__(self, user=None, *args, **kwargs): self.fields[ "newsletters" ].initial = laposta_client.get_subscriptions_for_email( - limited_to, user.email + limited_to, user.verified_email ) def save(self, request, *args, **kwargs): - newsletters = self.cleaned_data["newsletters"] + user: User = request.user + if not user.has_verified_email(): + return client = create_laposta_client() if not client: return + newsletters = self.cleaned_data["newsletters"] + list_name_mapping = dict(self.fields["newsletters"].choices) user_data = UserData( ip=get_client_ip(request)[0], - email=request.user.email, + email=user.verified_email, source_url=None, custom_fields=None, options=None, ) limited_to = LapostaConfig.get_solo().limit_list_selection_to existing_subscriptions = set( - client.get_subscriptions_for_email(limited_to, request.user.email) + client.get_subscriptions_for_email(limited_to, user.verified_email) ) for list_id in newsletters: if list_id in existing_subscriptions: @@ -85,7 +93,7 @@ def save(self, request, *args, **kwargs): unsubscribe_from_ids = existing_subscriptions - set(newsletters) for list_id in unsubscribe_from_ids: try: - client.remove_subscription(list_id, request.user.email) + client.remove_subscription(list_id, user.verified_email) except (RequestException, ClientError): logger.exception( "Something went wrong while trying to delete subscription" diff --git a/src/open_inwoner/laposta/tests/test_forms.py b/src/open_inwoner/laposta/tests/test_forms.py index 3d5dc695e6..ded8718c87 100644 --- a/src/open_inwoner/laposta/tests/test_forms.py +++ b/src/open_inwoner/laposta/tests/test_forms.py @@ -1,3 +1,4 @@ +from unittest.mock import patch from urllib.parse import parse_qs from django.test import RequestFactory, TestCase @@ -21,6 +22,9 @@ def setUp(self): super().setUp() self.user = DigidUserFactory() + self.user.verified_email = self.user.email + self.user.save() + self.assertTrue(self.user.has_verified_email()) self.request = RequestFactory().get("/") self.request.user = self.user @@ -289,3 +293,41 @@ def test_save_form_raises_errors(self, m): ] }, ) + + @patch("open_inwoner.laposta.forms.create_laposta_client") + def test_form__disables_when_email_not_verified(self, m, mock_create_client): + """ + Verify that the form can create and delete subscriptions + """ + self.setUpMocks(m) + + self.user.verified_email = "" + self.user.save() + self.assertFalse(self.user.has_verified_email()) + + m.get( + f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=123", + json={ + "member": MemberFactory.build( + list_id="123", + member_id="1234567", + email=self.user.email, + custom_fields=None, + ).model_dump() + }, + ) + + form = NewsletterSubscriptionForm(data={}, user=self.user) + + # We don't get any newletters if not using verified email + self.assertEqual(form["newsletters"].initial, None) + + # Because client was never built (and no request-mock exception was thrown) + mock_create_client.assert_not_called() + + # Check forced save() doesn't do anything + form = NewsletterSubscriptionForm(data={"newsletters": ["123"]}, user=self.user) + form.save(self.request) + + # Client was never built (and no request-mock exception was thrown) + mock_create_client.assert_not_called() diff --git a/src/open_inwoner/qmatic/tests/data.py b/src/open_inwoner/qmatic/tests/data.py index f65147c1a4..9bdf484e42 100644 --- a/src/open_inwoner/qmatic/tests/data.py +++ b/src/open_inwoner/qmatic/tests/data.py @@ -11,7 +11,10 @@ class QmaticMockData: def __init__(self): self.appointments_url = reverse("profile:appointments") - self.user = DigidUserFactory() + self.user = DigidUserFactory( + email="qmatic@example.com", + verified_email="qmatic@example.com", + ) self.config = QmaticConfig.get_solo() self.config.booking_base_url = "https://qmatic.local/" From 36c27eb4f788201c3a6bde301f18e2c32641ffc4 Mon Sep 17 00:00:00 2001 From: Bart van der Schoor Date: Thu, 11 Apr 2024 13:48:01 +0200 Subject: [PATCH 2/3] [#2291 #2273 #2233] Added missing test --- .../accounts/tests/test_profile_views.py | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/src/open_inwoner/accounts/tests/test_profile_views.py b/src/open_inwoner/accounts/tests/test_profile_views.py index c6f4fd03db..681502d84b 100644 --- a/src/open_inwoner/accounts/tests/test_profile_views.py +++ b/src/open_inwoner/accounts/tests/test_profile_views.py @@ -1041,7 +1041,10 @@ def setUp(self): cms_tools.create_apphook_page(ProfileApphook) self.profile_url = reverse("profile:detail") - self.user = DigidUserFactory() + self.user = DigidUserFactory( + email="news@example.com", verified_email="news@example.com" + ) + self.assertTrue(self.user.has_verified_email()) self.config = LapostaConfig.get_solo() self.config.api_root = "https://laposta.local/api/v2/" @@ -1153,6 +1156,31 @@ def test_render_form_limit_newsletters_to_admin_selection(self, m): # Second field was excluded by `LapostaConfig.limit_list_selection_to` self.assertNotIn("Nieuwsbrief2: bar", response.text) + def test_do_not_render_form_if_email_not_verified(self, m): + self.setUpMocks(m) + + self.user.verified_email = "" + self.user.save() + self.assertFalse(self.user.has_verified_email()) + + 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() + }, + ) + response = self.app.get(self.profile_url, user=self.user) + + self.assertNotIn("newsletter-form", response.forms) + @requests_mock.Mocker() @override_settings( From 5164ae46448d69dddeb107a66804c13d8360bc66 Mon Sep 17 00:00:00 2001 From: Bart van der Schoor Date: Thu, 11 Apr 2024 16:53:48 +0200 Subject: [PATCH 3/3] Update src/open_inwoner/laposta/tests/test_forms.py Co-authored-by: Steven Bal --- src/open_inwoner/laposta/tests/test_forms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/open_inwoner/laposta/tests/test_forms.py b/src/open_inwoner/laposta/tests/test_forms.py index ded8718c87..617791b916 100644 --- a/src/open_inwoner/laposta/tests/test_forms.py +++ b/src/open_inwoner/laposta/tests/test_forms.py @@ -297,7 +297,7 @@ def test_save_form_raises_errors(self, m): @patch("open_inwoner.laposta.forms.create_laposta_client") def test_form__disables_when_email_not_verified(self, m, mock_create_client): """ - Verify that the form can create and delete subscriptions + Verify that form actions cannot be performed if the user's email is not verified """ self.setUpMocks(m)