Skip to content

Commit

Permalink
🐛 [#2257] Fix Laposta subscription delete for emails with +
Browse files Browse the repository at this point in the history
  • Loading branch information
@stevenbal
stevenbal committed Apr 5, 2024
1 parent eff1484 commit 889b6fa
Show file tree
Hide file tree
Showing 5 changed files with 127 additions and 94 deletions.
40 changes: 24 additions & 16 deletions src/open_inwoner/accounts/tests/test_profile_views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1081,18 +1081,22 @@ def test_render_form_if_newsletters_are_found(self, m):
self.config.save()

m.get(
f"{self.config.api_root}member/{self.user.email}?list_id=list1",
f"{self.config.api_root}member?list_id=list1",
json={
"member": MemberFactory.build(
list_id="list1",
member_id="1234567",
email=self.user.email,
custom_fields=None,
).model_dump()
"data": [
{
"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",
f"{self.config.api_root}member?list_id=list2",
status_code=400,
)

Expand All @@ -1116,18 +1120,22 @@ def test_render_form_limit_newsletters_to_admin_selection(self, m):
self.config.save()

m.get(
f"{self.config.api_root}member/{self.user.email}?list_id=list1",
f"{self.config.api_root}member?list_id=list1",
json={
"member": MemberFactory.build(
list_id="list1",
member_id="1234567",
email=self.user.email,
custom_fields=None,
).model_dump()
"data": [
{
"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",
f"{self.config.api_root}member?list_id=list2",
status_code=400,
)

Expand Down
2 changes: 1 addition & 1 deletion src/open_inwoner/accounts/views/profile.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ def crumbs(self):

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):
Expand Down
31 changes: 25 additions & 6 deletions src/open_inwoner/laposta/client.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ def get_lists(self) -> list[LapostaList]:
return lists

def create_subscription(self, list_id: str, user_data: UserData) -> Member | None:
# Ensure the current subscriptions for this email address are fetched again after
# this API call
cache.delete(f"laposta_list_subscriptions:{user_data.email}")

response = self.post(
Expand All @@ -59,10 +61,18 @@ def create_subscription(self, list_id: str, user_data: UserData) -> Member | Non

return Member(**data["member"])

def remove_subscription(self, list_id: str, email: str) -> Member | None:
def remove_subscription(
self, list_id: str, member_id: str, email: str
) -> Member | None:
# Ensure the current subscriptions for this email address are fetched again after
# this API call
cache.delete(f"laposta_list_subscriptions:{email}")

response = self.delete(f"member/{quote(email)}", params={"list_id": list_id})
# The API cannot deal with urlencoded email addresses that contain a `+`,
# so we use the member_id instead
response = self.delete(
f"member/{quote(member_id)}", params={"list_id": list_id}
)
if response.status_code == 400:
data = response.json()
error = data.get("error", {})
Expand All @@ -81,12 +91,21 @@ def remove_subscription(self, list_id: str, email: str) -> Member | None:
@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 = []
def get_subscriptions_for_email(
self, list_ids: list[str], email: str
) -> dict[str, str]:
# The API cannot deal with urlencoded email addresses that contain a `+`,
# so instead we fetch all members for a given list and check if there is a
# member with a matching email
subscribed_to = {}
for list_id in list_ids:
response = self.get(f"member/{quote(email)}", params={"list_id": list_id})
response = self.get("member", params={"list_id": list_id})
if response.status_code == 200:
subscribed_to.append(list_id)
data = get_json_response(response)
for member_data in data["data"]:
member = Member(**member_data["member"])
if member.email == email:
subscribed_to[list_id] = member.member_id
return subscribed_to


Expand Down
15 changes: 7 additions & 8 deletions src/open_inwoner/laposta/forms.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ 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 laposta_client := create_laposta_client():
Expand All @@ -37,13 +37,11 @@ def __init__(self, user=None, *args, **kwargs):
if choice[0] in limited_to
]

# 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
subscription_mapping = laposta_client.get_subscriptions_for_email(
limited_to, request.user.email
)
request.session["subscription_mapping"] = subscription_mapping
self.fields["newsletters"].initial = list(subscription_mapping.keys())

def save(self, request, *args, **kwargs):
newsletters = self.cleaned_data["newsletters"]
Expand Down Expand Up @@ -87,7 +85,8 @@ 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)
member_id = request.session.get("subscription_mapping", {}).get(list_id)
client.remove_subscription(list_id, member_id, request.user.email)
except (RequestException, ClientError):
logger.exception(
"Something went wrong while trying to delete subscription"
Expand Down
133 changes: 70 additions & 63 deletions src/open_inwoner/laposta/tests/test_forms.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import requests_mock

from open_inwoner.accounts.tests.factories import DigidUserFactory
from open_inwoner.utils.test import ClearCachesMixin
from open_inwoner.utils.test import ClearCachesMixin, SessionMiddleware

from ..forms import NewsletterSubscriptionForm
from ..models import LapostaConfig
Expand All @@ -24,6 +24,9 @@ def setUp(self):

self.request = RequestFactory().get("/")
self.request.user = self.user
middleware = SessionMiddleware()
middleware.process_request(self.request)
self.request.session.save()

self.config = LapostaConfig.get_solo()
self.config.api_root = LAPOSTA_API_ROOT
Expand Down Expand Up @@ -61,38 +64,44 @@ def test_save_form(self, m):
self.setUpMocks(m)

m.get(
f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=123",
f"{LAPOSTA_API_ROOT}member?list_id=123",
json={
"member": MemberFactory.build(
list_id="123",
member_id="1234567",
email=self.user.email,
custom_fields=None,
).model_dump()
"data": [
{
"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",
f"{LAPOSTA_API_ROOT}member?list_id=456",
json={
"member": MemberFactory.build(
list_id="456",
member_id="8765433",
email=self.user.email,
custom_fields=None,
).model_dump()
"data": [
{
"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
)
m.get(f"{LAPOSTA_API_ROOT}member?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())
Expand All @@ -108,9 +117,7 @@ def test_save_form(self, m):
).model_dump()
},
)
delete_matcher = m.delete(
f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=123"
)
delete_matcher = m.delete(f"{LAPOSTA_API_ROOT}member/1234567?list_id=123")

form.save(self.request)

Expand Down Expand Up @@ -141,17 +148,13 @@ def test_save_form_create_duplicate_subscription(self, m):
"""
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
)
m.get(f"{LAPOSTA_API_ROOT}member?list_id=123", status_code=400)
m.get(f"{LAPOSTA_API_ROOT}member?list_id=456", status_code=400)
m.get(f"{LAPOSTA_API_ROOT}member?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())

Expand Down Expand Up @@ -182,30 +185,32 @@ def test_save_form_delete_non_existent_subscription(self, m):
"""
self.setUpMocks(m)

m.get(f"{LAPOSTA_API_ROOT}member?list_id=123", status_code=400)
m.get(f"{LAPOSTA_API_ROOT}member?list_id=456", status_code=400)
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",
f"{LAPOSTA_API_ROOT}member?list_id=789",
json={
"member": MemberFactory.build(
list_id="789",
member_id="1234567",
email=self.user.email,
custom_fields=None,
).model_dump()
"data": [
{
"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)
form = NewsletterSubscriptionForm(
data={"newsletters": []}, request=self.request
)

self.assertTrue(form.is_valid())

delete_matcher = m.delete(
f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=789",
f"{LAPOSTA_API_ROOT}member/1234567?list_id=789",
json={
"error": {
"type": "invalid_input",
Expand All @@ -227,25 +232,27 @@ def test_save_form_raises_errors(self, m):
"""
self.setUpMocks(m)

m.get(f"{LAPOSTA_API_ROOT}member?list_id=123", status_code=400)
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",
f"{LAPOSTA_API_ROOT}member?list_id=456",
json={
"member": MemberFactory.build(
list_id="456",
member_id="1234567",
email=self.user.email,
custom_fields=None,
).model_dump()
"data": [
{
"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
)
m.get(f"{LAPOSTA_API_ROOT}member?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())

Expand All @@ -260,7 +267,7 @@ def test_save_form_raises_errors(self, m):
status_code=500,
)
delete_matcher = m.delete(
f"{LAPOSTA_API_ROOT}member/{self.user.email}?list_id=456",
f"{LAPOSTA_API_ROOT}member/1234567?list_id=456",
json={
"error": {
"type": "internal",
Expand Down

0 comments on commit 889b6fa

Please sign in to comment.