Merge pull request #1160 from ae-utbm/fix-old-subscribers-group

fix old subscribers group attribution

subscription/models.py

@@ -18,7 +18,6 @@ from datetime import date, timedelta
 
 from dateutil.relativedelta import relativedelta
 from django.conf import settings
-from django.contrib.auth.forms import PasswordResetForm
 from django.core.exceptions import ValidationError
 from django.db import models
 from django.urls import reverse
@@ -72,24 +71,22 @@ class Subscription(models.Model):
         else:
             return f"No user - {self.pk}"
 
-    def save(self, *args, **kwargs):
-        super().save()
+    def save(self, *args, **kwargs) -> None:
+        if self.member.was_subscribed:
+            super().save()
+            return
+
         from counter.models import Customer
 
-        _, account_created = Customer.get_or_create(self.member)
-        if account_created:
-            # Someone who subscribed once will be considered forever
-            # as an old subscriber.
-            self.member.groups.add(settings.SITH_GROUP_OLD_SUBSCRIBERS_ID)
-            form = PasswordResetForm({"email": self.member.email})
-            if form.is_valid():
-                form.save(
-                    use_https=True,
-                    email_template_name="core/new_user_email.jinja",
-                    subject_template_name="core/new_user_email_subject.jinja",
-                    from_email="ae@utbm.fr",
-                )
+        customer, _ = Customer.get_or_create(self.member)
+        # Someone who subscribed once will be considered forever
+        # as an old subscriber.
+        self.member.groups.add(settings.SITH_GROUP_OLD_SUBSCRIBERS_ID)
         self.member.make_home()
+        # now that the user is an old subscriber, change the cached
+        # property accordingly
+        self.member.__dict__["was_subscribed"] = True
+        super().save()
 
     def get_absolute_url(self):
         return reverse("core:user_edit", kwargs={"user_id": self.member_id})

subscription/tests/test_new_subscription.py

@@ -5,6 +5,7 @@ from typing import Callable
 
 import pytest
 from dateutil.relativedelta import relativedelta
+from django.conf import settings
 from django.contrib.auth.models import Permission
 from django.test import Client
 from django.urls import reverse
@@ -14,8 +15,10 @@ from pytest_django.asserts import assertRedirects
 from pytest_django.fixtures import SettingsWrapper
 
 from core.baker_recipes import board_user, old_subscriber_user, subscriber_user
-from core.models import User
+from core.models import Group, User
+from counter.models import Customer
 from subscription.forms import SubscriptionExistingUserForm, SubscriptionNewUserForm
+from subscription.models import Subscription
 
 
 @pytest.mark.django_db
@@ -189,3 +192,17 @@ def test_submit_form_new_user(client: Client, settings: SettingsWrapper):
             kwargs={"subscription_id": current_subscription.id},
         ),
     )
+
+
+@pytest.mark.django_db
+def test_subscription_for_user_that_had_a_sith_account():
+    """Test that a newly subscribed user is added to the old subscribers group,
+    even if there already was a sith account (e.g. created during an eboutic purchase).
+    """
+    user = baker.make(User)
+    Customer.get_or_create(user)
+    group = Group.objects.get(id=settings.SITH_GROUP_OLD_SUBSCRIBERS_ID)
+    assert not user.groups.contains(group)
+    subscription = baker.prepare(Subscription, member=user)
+    subscription.save()
+    assert user.groups.contains(group)

subscription/views.py

@@ -14,6 +14,7 @@
 #
 
 from django.conf import settings
+from django.contrib.auth.forms import PasswordResetForm
 from django.contrib.auth.mixins import PermissionRequiredMixin
 from django.core.exceptions import PermissionDenied
 from django.urls import reverse, reverse_lazy
@@ -65,11 +66,23 @@ class CreateSubscriptionExistingUserFragment(CreateSubscriptionFragment):
 
 
 class CreateSubscriptionNewUserFragment(CreateSubscriptionFragment):
-    """Create a subscription for a user who already exists."""
+    """Create a subscription for a user who doesn't exist yet."""
 
     form_class = SubscriptionNewUserForm
     extra_context = {"post_url": reverse_lazy("subscription:fragment-new-user")}
 
+    def form_valid(self, form):
+        res = super().form_valid(form)
+        reset_form = PasswordResetForm({"email": form.cleaned_data["email"]})
+        if reset_form.is_valid():
+            reset_form.save(
+                use_https=True,
+                email_template_name="core/new_user_email.jinja",
+                subject_template_name="core/new_user_email_subject.jinja",
+                from_email="ae@utbm.fr",
+            )
+        return res
+
 
 class SubscriptionCreatedFragment(PermissionRequiredMixin, DetailView):
     template_name = "subscription/fragments/creation_success.jinja"