From 617271f0d0aaa3ab73bb643c4ae0c2f020f54605 Mon Sep 17 00:00:00 2001 From: Bernard Siebens Date: Sat, 8 Aug 2026 19:14:36 +0200 Subject: [PATCH] Honor auto_archive in the reminder email subject and the trial form Two gaps found while checking whether auto_archive is honored end-to-end (archive_overdue_clubs and the on-screen banner already got it right): - reminder_subject.txt branched only on notice.level, so a club with auto_archive off -- one that will NEVER be archived -- still got "Action required: X is about to be archived" as its subject line, contradicting the correctly-worded body underneath. Now gated on notice.level == 'error' AND notice.will_archive. - TrialForm had no auto_renew/auto_archive fields at all, so a trial could only ever be started on the service defaults (both True). The only way to change either afterwards was "Change plan", which ends the trial as a side effect. Added both, matching SubscriptionForm's existing pair. --- .../billing/email/reminder_subject.txt | 2 +- billing/tests.py | 36 +++++++++++++++++++ controlpanel/forms.py | 5 +++ controlpanel/tests.py | 17 ++++++++- controlpanel/views.py | 2 ++ 5 files changed, 60 insertions(+), 2 deletions(-) diff --git a/billing/templates/billing/email/reminder_subject.txt b/billing/templates/billing/email/reminder_subject.txt index d21c442..c59af35 100644 --- a/billing/templates/billing/email/reminder_subject.txt +++ b/billing/templates/billing/email/reminder_subject.txt @@ -1 +1 @@ -{% load i18n %}{% if notice.level == 'error' %}{% blocktrans with club=club.name %}Action required: {{ club }} is about to be archived{% endblocktrans %}{% else %}{% blocktrans with club=club.name %}Platform fees are due for {{ club }}{% endblocktrans %}{% endif %} +{% load i18n %}{% if notice.level == 'error' and notice.will_archive %}{% blocktrans with club=club.name %}Action required: {{ club }} is about to be archived{% endblocktrans %}{% else %}{% blocktrans with club=club.name %}Platform fees are due for {{ club }}{% endblocktrans %}{% endif %} diff --git a/billing/tests.py b/billing/tests.py index da73c63..f6a7052 100644 --- a/billing/tests.py +++ b/billing/tests.py @@ -752,6 +752,42 @@ class BillingReminderTests(BillingTestBase): def test_a_reminder_goes_to_the_club_admins(self): self.assertEqual(admin_emails(self.club), ["admin@ajax.example"]) + def make_overdue_club(self, name, *, auto_archive): + # A separate club rather than resubscribing self.club: open_period() caches + # club.subscription on the instance it's given, so calling subscribe() twice + # against the SAME Python object -- only possible by reusing one across two calls, + # never a real request -- would read the first call's now-stale cached subscription. + club = Club.objects.create(name=name) + user = User.objects.create_user(email=f"{name.lower()}@ajax.example", password="pw-secret-123") + member = Member.objects.create(user=user, first_name="A", last_name="Admin") + ClubRole.objects.create(club=club, member=member, role=ClubRole.Roles.ADMIN) + subscribe(club, self.plan, start=self.today - datetime.timedelta(days=DEFAULT_GRACE_DAYS + 5), auto_archive=auto_archive) + return club + + def test_an_overdue_reminder_threatens_archiving_when_it_will_happen(self): + club = self.make_overdue_club("Archive On", auto_archive=True) + + notice = club_billing_notice(club, self.today) + self.assertTrue(notice.will_archive) + send_reminder(club, notice, recipients=["archive-on@ajax.example"]) + + self.assertIn("about to be archived", mail.outbox[0].subject) + self.assertIn("archived", mail.outbox[0].body) + + def test_an_overdue_reminder_does_not_threaten_archiving_when_auto_archive_is_off(self): + # This is the bug the two-column-modal review turned up: the subject branched only + # on notice.level, so a club that will NEVER be archived still got told it was + # "about to be archived" -- while the body correctly said otherwise. + club = self.make_overdue_club("Archive Off", auto_archive=False) + + notice = club_billing_notice(club, self.today) + self.assertFalse(notice.will_archive) + send_reminder(club, notice, recipients=["archive-off@ajax.example"]) + + self.assertNotIn("about to be archived", mail.outbox[0].subject) + self.assertNotIn("archived", mail.outbox[0].body) + self.assertIn("good standing", mail.outbox[0].body) + def test_sending_records_the_level_and_fills_the_outbox(self): notice = club_billing_notice(self.club, self.today) send_reminder(self.club, notice, recipients=["admin@ajax.example"]) diff --git a/controlpanel/forms.py b/controlpanel/forms.py index 60d822a..60bbc81 100644 --- a/controlpanel/forms.py +++ b/controlpanel/forms.py @@ -140,6 +140,11 @@ class TrialForm(forms.Form): trial_plan = forms.ModelChoiceField(queryset=Plan.objects.none(), label=_("Trial plan"), help_text=_("What this club is billed on during the trial. Its length is the plan's own duration.")) post_trial_plan = forms.ModelChoiceField(queryset=Plan.objects.none(), label=_("Then switch to"), help_text=_("The plan it lands on automatically once the trial ends.")) start = forms.DateField(required=False, widget=forms.DateInput(attrs={"type": "date"}), label=_("Trial starts"), help_text=_("Left blank, the trial starts today.")) + # Same two switches SubscriptionForm offers. Without them here a trial could only be + # started on the defaults, and the only way to change them afterwards is the "Change + # plan" modal -- which ends the trial as a side effect. + auto_renew = forms.BooleanField(required=False, initial=True, label=_("Auto renew"), help_text=_("Issue the next period automatically before this one ends.")) + auto_archive = forms.BooleanField(required=False, initial=True, label=_("Auto archive"), help_text=_("Archive this club when a period goes unpaid past its grace period.")) def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) diff --git a/controlpanel/tests.py b/controlpanel/tests.py index 5995cdd..d87c903 100644 --- a/controlpanel/tests.py +++ b/controlpanel/tests.py @@ -1502,7 +1502,10 @@ class TrialPanelTests(ControlPanelTestBase): PlanPrice.objects.create(plan=self.plan, active_from=self.today - datetime.timedelta(days=1200), amount=Decimal("500.00")) def start_trial(self, **data): - data = {"trial_plan": self.trial_plan.pk, "post_trial_plan": self.plan.pk, "start": ""} | data + # "on" for both, matching how a freshly opened modal actually renders: unbound, + # TrialForm's auto_renew/auto_archive are initial=True, so a real submission that + # never touches either checkbox posts them checked. + data = {"trial_plan": self.trial_plan.pk, "post_trial_plan": self.plan.pk, "start": "", "auto_renew": "on", "auto_archive": "on"} | data return self.client.post(reverse("controlpanel:club_trial_start", args=[self.club.pk]), data) def test_starting_a_trial_opens_a_short_first_period(self): @@ -1541,6 +1544,18 @@ class TrialPanelTests(ControlPanelTestBase): self.assertContains(response, "On trial") self.assertContains(response, "Standard") + def test_a_trial_can_be_started_with_auto_archive_off(self): + # Same switch SubscriptionForm offers -- a trial must be able to opt out of + # auto-archiving too, not just a paid subscription. + self.start_trial(auto_archive="") + + self.assertFalse(self.club.subscription.auto_archive) + + def test_a_trial_defaults_to_auto_archive_on(self): + self.start_trial() + + self.assertTrue(self.club.subscription.auto_archive) + def test_manually_changing_plan_mid_trial_clears_the_trial(self): self.start_trial() other = Plan.objects.create(name="Other") diff --git a/controlpanel/views.py b/controlpanel/views.py index 1342c2e..1dec476 100644 --- a/controlpanel/views.py +++ b/controlpanel/views.py @@ -550,6 +550,8 @@ class ClubStartTrialView(PlatformStaffRequiredMixin, RedirectOnInvalidMixin, For trial_plan, post_trial_plan=form.cleaned_data["post_trial_plan"], start=form.cleaned_data.get("start"), + auto_renew=form.cleaned_data["auto_renew"], + auto_archive=form.cleaned_data["auto_archive"], ) notify(self.request, f"s|Trial started|{club} is on a {trial_plan.duration_months}-month trial of {trial_plan}, then switches to {form.cleaned_data['post_trial_plan']}.")