diff --git a/src/open_inwoner/accounts/tests/test_profile_views.py b/src/open_inwoner/accounts/tests/test_profile_views.py index ba5793346d..153a3260e1 100644 --- a/src/open_inwoner/accounts/tests/test_profile_views.py +++ b/src/open_inwoner/accounts/tests/test_profile_views.py @@ -17,7 +17,7 @@ from open_inwoner.cms.profile.cms_appconfig import ProfileConfig from open_inwoner.haalcentraal.tests.mixins import HaalCentraalMixin from open_inwoner.laposta.models import LapostaConfig -from open_inwoner.laposta.tests.factories import LapostaListFactory, SubscriptionFactory +from open_inwoner.laposta.tests.factories import LapostaListFactory, MemberFactory from open_inwoner.openklant.models import OpenKlantConfig from open_inwoner.pdc.tests.factories import CategoryFactory from open_inwoner.plans.tests.factories import PlanFactory @@ -1077,7 +1077,24 @@ def test_do_not_render_form_if_no_newsletters_are_found(self, m): def test_render_form_if_newsletters_are_found(self, m): self.setUpMocks(m) - SubscriptionFactory.create(list_id=self.list1.list_id, user=self.user) + self.config.limit_list_selection_to = ["list1", "list2"] + 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) @@ -1098,7 +1115,21 @@ def test_render_form_limit_newsletters_to_admin_selection(self, m): self.config.limit_list_selection_to = ["list1"] self.config.save() - SubscriptionFactory.create(list_id=self.list1.list_id, user=self.user) + 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) diff --git a/src/open_inwoner/laposta/admin.py b/src/open_inwoner/laposta/admin.py index 08dce0a0cf..1465fdb8c6 100644 --- a/src/open_inwoner/laposta/admin.py +++ b/src/open_inwoner/laposta/admin.py @@ -6,7 +6,7 @@ from .choices import get_list_choices from .client import create_laposta_client -from .models import LapostaConfig, Subscription +from .models import LapostaConfig class LapostaConfigForm(forms.ModelForm): @@ -49,10 +49,3 @@ class LapostaConfigAdmin(SingletonModelAdmin): {"fields": ("limit_list_selection_to",)}, ), ) - - -@admin.register(Subscription) -class SubscriptionAdmin(admin.ModelAdmin): - list_display = ["list_id", "user", "member_id"] - list_filter = ["list_id", "user"] - search_fields = ("user__first_name", "user__last_name", "user__email") diff --git a/src/open_inwoner/laposta/client.py b/src/open_inwoner/laposta/client.py index e94f5eb2a6..050ea58f5d 100644 --- a/src/open_inwoner/laposta/client.py +++ b/src/open_inwoner/laposta/client.py @@ -1,6 +1,8 @@ import logging +from urllib.parse import quote from django.conf import settings +from django.core.cache import cache from ape_pie.client import APIClient from requests.exceptions import RequestException @@ -32,7 +34,11 @@ def get_lists(self) -> list[LapostaList]: return lists def create_subscription(self, list_id: str, user_data: UserData) -> Member | None: - response = self.post("member", data={"list_id": list_id, **user_data.dict()}) + cache.delete(f"laposta_list_subscriptions:{user_data.email}") + + response = self.post( + "member", data={"list_id": list_id, **user_data.model_dump()} + ) if response.status_code == 400: data = response.json() @@ -53,9 +59,10 @@ def create_subscription(self, list_id: str, user_data: UserData) -> Member | Non return Member(**data["member"]) - def remove_subscription(self, list_id: str, member_id: str) -> Member | None: - response = self.delete(f"member/{member_id}", params={"list_id": list_id}) + def remove_subscription(self, list_id: str, email: str) -> Member | None: + cache.delete(f"laposta_list_subscriptions:{email}") + response = self.delete(f"member/{quote(email)}", params={"list_id": list_id}) if response.status_code == 400: data = response.json() error = data.get("error", {}) @@ -71,6 +78,17 @@ def remove_subscription(self, list_id: str, member_id: str) -> Member | None: return Member(**data["member"]) + @cache_result( + "laposta_list_subscriptions:{email}", timeout=settings.CACHE_LAPOSTA_API_TIMEOUT + ) + def get_subscriptions_for_email(self, list_ids: list[str], email: str) -> list[str]: + subscribed_to = [] + for list_id in list_ids: + response = self.get(f"member/{quote(email)}", params={"list_id": list_id}) + if response.status_code == 200: + subscribed_to.append(list_id) + return subscribed_to + def create_laposta_client() -> LapostaClient | None: config = LapostaConfig.get_solo() diff --git a/src/open_inwoner/laposta/forms.py b/src/open_inwoner/laposta/forms.py index 8f4acd988d..7c4feddf8d 100644 --- a/src/open_inwoner/laposta/forms.py +++ b/src/open_inwoner/laposta/forms.py @@ -8,7 +8,7 @@ from requests.exceptions import RequestException from open_inwoner.laposta.api_models import UserData -from open_inwoner.laposta.models import LapostaConfig, Subscription +from open_inwoner.laposta.models import LapostaConfig from open_inwoner.utils.api import ClientError from .choices import get_list_choices @@ -37,10 +37,13 @@ def __init__(self, user=None, *args, **kwargs): if choice[0] in limited_to ] - self.fields["newsletters"].initial = [ - subscription.list_id - for subscription in Subscription.objects.filter(user=user) - ] + # TODO get list_ids of lists the user is subscribed to + # self.existing_subscriptions = laposta_client.get_subscriptions_for_email(limited_to, user.email) + self.fields[ + "newsletters" + ].initial = laposta_client.get_subscriptions_for_email( + limited_to, user.email + ) def save(self, request, *args, **kwargs): newsletters = self.cleaned_data["newsletters"] @@ -57,19 +60,16 @@ def save(self, request, *args, **kwargs): custom_fields=None, options=None, ) + limited_to = LapostaConfig.get_solo().limit_list_selection_to existing_subscriptions = set( - Subscription.objects.filter(user=request.user).values_list( - "list_id", flat=True - ) + client.get_subscriptions_for_email(limited_to, request.user.email) ) - - to_create = [] for list_id in newsletters: if list_id in existing_subscriptions: continue try: - member = client.create_subscription(list_id, user_data) + client.create_subscription(list_id, user_data) except (RequestException, ClientError): logger.exception( "Something went wrong while trying to create subscription" @@ -83,28 +83,11 @@ def save(self, request, *args, **kwargs): ).format(list_name=list_name_mapping[list_id]) ), ) - else: - if member: - to_create.append( - Subscription( - list_id=member.list_id, - member_id=member.member_id, - user=request.user, - ) - ) - - 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: + for list_id in unsubscribe_from_ids: try: - client.remove_subscription(subscription.list_id, subscription.member_id) + client.remove_subscription(list_id, request.user.email) except (RequestException, ClientError): logger.exception( "Something went wrong while trying to delete subscription" @@ -115,11 +98,6 @@ def save(self, request, *args, **kwargs): _( "Something went wrong while trying to unsubscribe " "from '{list_name}', please try again later" - ).format(list_name=list_name_mapping[subscription.list_id]) + ).format(list_name=list_name_mapping[list_id]) ), ) - else: - to_delete_ids.append(subscription.list_id) - Subscription.objects.filter( - user=request.user, list_id__in=to_delete_ids - ).delete() diff --git a/src/open_inwoner/laposta/migrations/0004_delete_subscription.py b/src/open_inwoner/laposta/migrations/0004_delete_subscription.py new file mode 100644 index 0000000000..d41977058b --- /dev/null +++ b/src/open_inwoner/laposta/migrations/0004_delete_subscription.py @@ -0,0 +1,16 @@ +# Generated by Django 4.2.11 on 2024-04-05 09:30 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("laposta", "0003_lapostaconfig_limit_list_selection_to"), + ] + + operations = [ + migrations.DeleteModel( + name="Subscription", + ), + ] diff --git a/src/open_inwoner/laposta/models.py b/src/open_inwoner/laposta/models.py index 5a0e0de9ab..9384a35458 100644 --- a/src/open_inwoner/laposta/models.py +++ b/src/open_inwoner/laposta/models.py @@ -51,22 +51,3 @@ def get_client_session_kwargs(self) -> dict[str, HTTPBasicAuth]: return { "auth": HTTPBasicAuth(self.basic_auth_username, self.basic_auth_password) } - - -class Subscription(models.Model): - list_id = models.CharField( - verbose_name=_("List ID"), - help_text=_("Unique ID of the list in the API"), - max_length=50, - ) - member_id = models.CharField( - verbose_name=_("Member ID"), - help_text=_("Unique ID of the member for this list in the API"), - max_length=50, - ) - user = models.ForeignKey( - "accounts.User", verbose_name=_("User"), on_delete=models.CASCADE - ) - - class Meta: - unique_together = [["list_id", "member_id"]] diff --git a/src/open_inwoner/laposta/tests/factories.py b/src/open_inwoner/laposta/tests/factories.py index bbb7b0f3fe..83d66fd914 100644 --- a/src/open_inwoner/laposta/tests/factories.py +++ b/src/open_inwoner/laposta/tests/factories.py @@ -1,10 +1,6 @@ -import factory from polyfactory.factories.pydantic_factory import ModelFactory -from open_inwoner.accounts.tests.factories import UserFactory - from ..api_models import LapostaList, Member -from ..models import Subscription class LapostaListFactory(ModelFactory[LapostaList]): @@ -13,10 +9,3 @@ class LapostaListFactory(ModelFactory[LapostaList]): class MemberFactory(ModelFactory[Member]): __model__ = Member - - -class SubscriptionFactory(factory.django.DjangoModelFactory): - class Meta: - model = Subscription - - user = factory.SubFactory(UserFactory) diff --git a/src/open_inwoner/laposta/tests/test_forms.py b/src/open_inwoner/laposta/tests/test_forms.py index f8aa74cfe1..3d5dc695e6 100644 --- a/src/open_inwoner/laposta/tests/test_forms.py +++ b/src/open_inwoner/laposta/tests/test_forms.py @@ -9,8 +9,8 @@ from open_inwoner.utils.test import ClearCachesMixin from ..forms import NewsletterSubscriptionForm -from ..models import LapostaConfig, Subscription -from .factories import LapostaListFactory, MemberFactory, SubscriptionFactory +from ..models import LapostaConfig +from .factories import LapostaListFactory, MemberFactory LAPOSTA_API_ROOT = "https://laposta.local/api/v2/" @@ -29,6 +29,7 @@ def setUp(self): self.config.api_root = LAPOSTA_API_ROOT self.config.basic_auth_username = "username" self.config.basic_auth_password = "password" + self.config.limit_list_selection_to = ["123", "456", "789"] self.config.save() self.list1 = LapostaListFactory.build( @@ -46,9 +47,9 @@ def setUpMocks(self, m): f"{LAPOSTA_API_ROOT}list", json={ "data": [ - {"list": self.list1.dict()}, - {"list": self.list2.dict()}, - {"list": self.list3.dict()}, + {"list": self.list1.model_dump()}, + {"list": self.list2.model_dump()}, + {"list": self.list3.model_dump()}, ] }, ) @@ -59,8 +60,31 @@ def test_save_form(self, m): """ self.setUpMocks(m) - SubscriptionFactory.create(list_id="123", member_id="member123", user=self.user) - SubscriptionFactory.create(list_id="456", member_id="member456", user=self.user) + 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() + }, + ) + m.get( + f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=456", + json={ + "member": MemberFactory.build( + list_id="456", + member_id="8765433", + email=self.user.email, + custom_fields=None, + ).model_dump() + }, + ) + m.get( + f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=789", status_code=400 + ) form = NewsletterSubscriptionForm(data={}, user=self.user) @@ -81,10 +105,12 @@ def test_save_form(self, m): member_id="member789", email=self.user.email, custom_fields=None, - ).dict() + ).model_dump() }, ) - delete_matcher = m.delete(f"{LAPOSTA_API_ROOT}member/member123?list_id=123") + delete_matcher = m.delete( + f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=123" + ) form.save(self.request) @@ -108,28 +134,28 @@ def test_save_form(self, m): "Unsubscribe from list if not present in the form data", ) - subscriptions = Subscription.objects.filter(user=self.user) - - self.assertEqual(subscriptions.count(), 2) - - subscription1, subscription2 = subscriptions - - self.assertEqual(subscription1.list_id, "456") - self.assertEqual(subscription1.member_id, "member456") - self.assertEqual(subscription2.list_id, "789") - self.assertEqual(subscription2.member_id, "member789") - def test_save_form_create_duplicate_subscription(self, m): """ Verify that the client properly handles the scenario where the user is a member - of the list in the API, but no Subscription exists locally and tries to create one + of the list in the API, but the user tries to create a new subscription for it """ self.setUpMocks(m) + m.get( + f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=123", status_code=400 + ) + m.get( + f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=456", status_code=400 + ) + m.get( + f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=789", status_code=400 + ) + form = NewsletterSubscriptionForm(data={"newsletters": ["789"]}, user=self.user) self.assertTrue(form.is_valid()) + # The subscription could have been created somewhere else in the meantime post_matcher = m.post( f"{LAPOSTA_API_ROOT}member", json={ @@ -149,28 +175,37 @@ def test_save_form_create_duplicate_subscription(self, m): self.assertEqual(len(post_matcher.request_history), 1) - subscription = Subscription.objects.filter(user=self.user).get() - - # Subscription should be created locally, based on data returned by API - self.assertEqual(subscription.list_id, "789") - self.assertEqual(subscription.member_id, "member789") - def test_save_form_delete_non_existent_subscription(self, m): """ - Verify that the client properly handles the scenario where the user has a - Subscription to a list locally and tries to delete it, but that relationship - does not exist in the API + Verify that the client properly handles the scenario where the user tries to + delete a subscription that does not exist in the API """ self.setUpMocks(m) - SubscriptionFactory.create(list_id="789", member_id="member789", user=self.user) + m.get( + f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=123", status_code=400 + ) + m.get( + f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=456", status_code=400 + ) + m.get( + f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=789", + json={ + "member": MemberFactory.build( + list_id="789", + member_id="1234567", + email=self.user.email, + custom_fields=None, + ).model_dump() + }, + ) form = NewsletterSubscriptionForm(data={"newsletters": []}, user=self.user) self.assertTrue(form.is_valid()) delete_matcher = m.delete( - f"{LAPOSTA_API_ROOT}member/member789?list_id=789", + f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=789", json={ "error": { "type": "invalid_input", @@ -186,15 +221,29 @@ 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) + m.get( + f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=123", status_code=400 + ) + m.get( + f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=456", + json={ + "member": MemberFactory.build( + list_id="456", + member_id="1234567", + email=self.user.email, + custom_fields=None, + ).model_dump() + }, + ) + m.get( + f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=789", status_code=400 + ) form = NewsletterSubscriptionForm(data={"newsletters": ["789"]}, user=self.user) @@ -211,7 +260,7 @@ def test_save_form_raises_errors(self, m): status_code=500, ) delete_matcher = m.delete( - f"{LAPOSTA_API_ROOT}member/member456?list_id=456", + f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=456", json={ "error": { "type": "internal", @@ -240,8 +289,3 @@ def test_save_form_raises_errors(self, m): ] }, ) - - # Local subscription should not be deleted - subscription = Subscription.objects.get(user=self.user) - - self.assertEqual(subscription.list_id, "456")