diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index ddbf454..fd19abb 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -319,7 +319,10 @@ tenant-scoped manager would be exactly the wrong default. thing to get wrong: `duration_months` (period length, from its start), `renewal_lead_days` (how far *before* a period starts its invoice is raised), `grace_days` (how long *after* a period starts it may stay unpaid). Two `CheckConstraint`s keep them coherent. `is_trial` marks a plan offered as a -trial; a trial's length is simply its own `duration_months`. +trial; a trial's length is simply its own `duration_months`. `deleted_at` is a soft-delete marker: +`Due.plan` is `PROTECT`, so a plan that has ever billed anyone can't really be removed — "delete" +hides it (`Plan.objects.visible()` excludes it) and unsubscribes every club currently on it instead; +see `billing/services/plans.py` and `BILLING.md` §11. **`PlanPrice`** — a dated price (`active_from`). A rate change is a new row, never an edit, so every period already opened keeps what it was billed at. diff --git a/BILLING.md b/BILLING.md index 7ee6279..7720329 100644 --- a/BILLING.md +++ b/BILLING.md @@ -404,3 +404,36 @@ default tries localhost:25 and raises `ConnectionRefused` on a box with no MTA deployment that forgets `DJANGO_EMAIL_HOST` will watch `send_billing_reminders --commit` report success while no club hears anything. Set the mail variables in `.env.production` (see `.env.production.example`) before trusting the job. + +## 11. Addendum: deleting a plan + +Added after the initial implementation. `Due.plan` is `PROTECT` — a plan that has ever billed +anyone can never truly be removed, on purpose: `amount`, `period_end` and `grace_until` are frozen +on a `Due` precisely so a later change can't rewrite what was actually charged, and losing the plan +link off an old `Due` would do exactly that to every historical invoice. "Delete" therefore means +one of two things, chosen automatically (`billing/services/plans.py`): + +- **No `Due` ever referenced the plan** (created, never actually used to bill anyone) — the row is + removed outright. +- **At least one `Due` references it** — soft-deleted instead: `Plan.deleted_at` is set and + `is_active` turned off. The row survives (so old invoices still say what they were billed under) + but is hidden from every picker and listing via `Plan.objects.visible()` — an opt-in queryset + method, same shape as `Club.objects.active()`, so the plain default manager stays unfiltered for + Django admin and anything reading historical data. + +Either way, every club **currently on the plan** is unsubscribed outright — its `Subscription` row +is deleted, not just its `plan` field cleared. "No plan" was already a state the rest of the app +fully understood (every billing view already handles `getattr(club, "subscription", None)` being +`None`), so this reuses it rather than inventing a new one. + +One easy-to-miss second group: a club on a **different** plan, mid-trial, configured to convert to +the plan being deleted (`Subscription.post_trial_plan`). Left alone, that club's trial would try to +convert onto a plan that no longer exists (or has been hidden) the moment `open_period()`'s +trial-conversion check next runs. Handled at delete time instead: that club's trial is ended +(`trial_ends_at` and `post_trial_plan` both cleared, per the `CheckConstraint` that requires them +set together or not at all), leaving it on the trial plan with no scheduled conversion until a +platform admin picks a new one. + +The confirmation screen (`controlpanel/templates/controlpanel/plan_delete.html`) is a real page, +not a modal like every other billing action — the whole point is naming exactly which clubs are +affected, in both groups, and that list can be long. diff --git a/billing/migrations/0007_plan_deleted_at.py b/billing/migrations/0007_plan_deleted_at.py new file mode 100644 index 0000000..4463434 --- /dev/null +++ b/billing/migrations/0007_plan_deleted_at.py @@ -0,0 +1,18 @@ +# Generated by Django 6.0.6 on 2026-08-08 17:54 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('billing', '0006_due_last_reminder_level_due_last_reminder_sent_at'), + ] + + operations = [ + migrations.AddField( + model_name='plan', + name='deleted_at', + field=models.DateTimeField(blank=True, editable=False, null=True, verbose_name='deleted at'), + ), + ] diff --git a/billing/models.py b/billing/models.py index 31edd8d..494a360 100644 --- a/billing/models.py +++ b/billing/models.py @@ -37,6 +37,17 @@ def add_months(day: date, months: int) -> date: return day + relativedelta.relativedelta(months=months) +class PlanQuerySet(models.QuerySet): + def visible(self): + """Excludes soft-deleted plans -- see billing.services.plans.delete_plan. + + Opt-in, same shape as club.models.ClubManager.active(): the default manager stays + unfiltered (Django admin, and anything reading historical data, sees everything), + and every picker/listing a platform admin actually chooses from calls this. + """ + return self.filter(deleted_at__isnull=True) + + class Plan(UUIDModel): """What a club is billed on: a duration, a set of clocks, and a dated price. @@ -64,6 +75,14 @@ class Plan(UUIDModel): help_text=_("Offered as a trial rather than as a paid plan. A trial converts to the plan chosen on the subscription once it runs out."), ) + # Not user-editable: set by billing.services.plans.delete_plan. Due.plan is PROTECT, so + # a plan that has ever billed anyone can never actually be removed -- deleting it hides + # it (and clears every club currently on it) instead, so past invoices still say what + # they were billed under. See that module's docstring for the full reasoning. + deleted_at = models.DateTimeField(_("deleted at"), null=True, blank=True, editable=False) + + objects = PlanQuerySet.as_manager() + class Meta: verbose_name = _("plan") verbose_name_plural = _("plans") @@ -86,6 +105,10 @@ class Plan(UUIDModel): def __str__(self): return self.name + @property + def is_deleted(self) -> bool: + return self.deleted_at is not None + def clean(self): """The same two invariants the CheckConstraints enforce, as form errors. diff --git a/billing/services/plans.py b/billing/services/plans.py new file mode 100644 index 0000000..2e803c4 --- /dev/null +++ b/billing/services/plans.py @@ -0,0 +1,85 @@ +"""Deleting a plan. + +Due.plan is PROTECT -- a plan that has ever billed anyone can never truly be removed, and +must not be: `amount`, `period_end` and `grace_until` are frozen on a Due precisely so a +later change can't rewrite what was actually charged, and losing the plan link off an old +Due would do exactly that to every historical invoice. So "delete" means one of two things, +chosen automatically depending on whether the plan has billing history: + +* No Due ever referenced it (created, then never actually used to bill anyone) -- the row + itself is removed. +* At least one Due references it -- soft-deleted instead (Plan.deleted_at set, is_active + turned off): hidden from every picker and listing (Plan.objects.visible()), but the row + survives so every old invoice still says what it was for. + +Either way, every club CURRENTLY on the plan is unsubscribed outright -- its Subscription +row deleted, not just its `plan` field cleared. "No plan" is already a state the rest of the +app fully understands (every billing view already handles `getattr(club, "subscription", +None)` being None), so there is no new state to teach it. + +A second, easy-to-miss group: a club on a DIFFERENT plan, mid-trial, configured to convert +to THIS plan once its trial ends (Subscription.post_trial_plan). Deleting the target plan +out from under that trial can't be allowed to raise a stale IntegrityError days or weeks +later when the trial tries to convert -- so it's handled now, at delete time: that club's +trial is ended (trial_ends_at and post_trial_plan both cleared, per the CheckConstraint that +requires them set together or not at all), leaving it on the trial plan with no scheduled +conversion until a platform admin picks a new one. +""" + +from dataclasses import dataclass + +from django.db import transaction +from django.utils import timezone + +from billing.models import Plan, Subscription + + +@dataclass(frozen=True) +class PlanDeletionImpact: + plan: Plan + #: Clubs currently ON this plan -- lose it entirely (Subscription row deleted). + unsubscribed_clubs: list + #: Clubs on a different plan, mid-trial, configured to convert to this one -- their + #: trial's landing plan is cleared, leaving no scheduled conversion. + broken_trial_clubs: list + #: Whether the Plan row itself will be removed (True) or only hidden (False, because + #: it has billing history). + will_hard_delete: bool + + @property + def has_impact(self) -> bool: + return bool(self.unsubscribed_clubs or self.broken_trial_clubs) + + +def plan_deletion_impact(plan: Plan) -> PlanDeletionImpact: + """What deleting `plan` right now would do -- read-only, for the confirmation screen. + + delete_plan() re-derives the same two lists itself rather than trust one computed here + moments earlier and possibly stale by the time the platform admin actually confirms. + """ + unsubscribed = Subscription.objects.filter(plan=plan).select_related("club").order_by("club__name") + broken_trial = Subscription.objects.filter(post_trial_plan=plan).exclude(plan=plan).select_related("club").order_by("club__name") + + return PlanDeletionImpact( + plan=plan, + unsubscribed_clubs=[subscription.club for subscription in unsubscribed], + broken_trial_clubs=[subscription.club for subscription in broken_trial], + will_hard_delete=not plan.dues.exists(), + ) + + +@transaction.atomic +def delete_plan(plan: Plan) -> PlanDeletionImpact: + impact = plan_deletion_impact(plan) + + Subscription.objects.filter(plan=plan).delete() + Subscription.objects.filter(post_trial_plan=plan).update(trial_ends_at=None, post_trial_plan=None) + + if impact.will_hard_delete: + plan.delete() + else: + plan.deleted_at = timezone.now() + plan.is_active = False + plan.save(update_fields=["deleted_at", "is_active", "modified"]) + + return impact diff --git a/billing/tests.py b/billing/tests.py index f6a7052..0de882f 100644 --- a/billing/tests.py +++ b/billing/tests.py @@ -21,6 +21,7 @@ from .services import BillingError from .services.dues import archivable_clubs, dues_in_grace, dues_overdue, next_period_start, open_period, reactivate, record_payment, remove_payment, renew, start_trial, subscribe, subscriptions_due_for_renewal, waive from .services.invoices import invoice_pdf, issue_invoice, render_pdf from .services.notices import club_billing_notice +from .services.plans import delete_plan, plan_deletion_impact from .services.reminders import admin_emails, reminders_to_send, send_reminder @@ -835,3 +836,124 @@ class BillingReminderTests(BillingTestBase): record_payment(self.due, Decimal("500.00")) self.assertEqual(reminders_to_send([self.club], self.today), []) + + +class PlanVisibilityTests(BillingTestBase): + """Plan.objects.visible() -- see PlanQuerySet.""" + + def test_a_plain_plan_is_visible(self): + self.assertIn(self.plan, Plan.objects.visible()) + + def test_a_soft_deleted_plan_is_excluded(self): + self.plan.deleted_at = timezone.now() + self.plan.save(update_fields=["deleted_at"]) + + self.assertNotIn(self.plan, Plan.objects.visible()) + + def test_the_default_manager_still_returns_a_soft_deleted_plan(self): + # Django admin, and anything reading historical data, must still be able to find it. + self.plan.deleted_at = timezone.now() + self.plan.save(update_fields=["deleted_at"]) + + self.assertIn(self.plan, Plan.objects.all()) + + +class PlanDeletionTests(BillingTestBase): + """billing.services.plans -- see its module docstring for the full reasoning.""" + + def test_a_never_billed_plan_is_hard_deleted(self): + unused = Plan.objects.create(name="Unused") + + impact = delete_plan(unused) + + self.assertTrue(impact.will_hard_delete) + self.assertFalse(Plan.objects.filter(pk=unused.pk).exists()) + + def test_a_plan_with_a_due_cannot_be_hard_deleted(self): + self.bill() + + impact = delete_plan(self.plan) + + self.assertFalse(impact.will_hard_delete) + self.assertTrue(Plan.objects.filter(pk=self.plan.pk).exists()) + + def test_a_cancelled_due_still_protects_the_plan(self): + # PROTECT does not care about the referencing row's own status -- a cancelled due is + # still a row, and financial history includes rows nobody expects to see again. + due = self.bill() + due.status = Due.Status.CANCELLED + due.save(update_fields=["status"]) + + impact = delete_plan(self.plan) + + self.assertFalse(impact.will_hard_delete) + + def test_soft_delete_marks_the_plan_inactive_and_deleted(self): + subscribe(self.club, self.plan) + + delete_plan(self.plan) + self.plan.refresh_from_db() + + self.assertTrue(self.plan.is_deleted) + self.assertFalse(self.plan.is_active) + + def test_deleting_unsubscribes_the_club_entirely_rather_than_nulling_a_field(self): + subscribe(self.club, self.plan) + + delete_plan(self.plan) + + self.assertFalse(Subscription.objects.filter(club=self.club).exists()) + + def test_a_deleted_plans_historical_due_is_untouched(self): + subscribe(self.club, self.plan) + due = self.club.dues.first() + amount, period_end, grace_until = due.amount, due.period_end, due.grace_until + + delete_plan(self.plan) + due.refresh_from_db() + + self.assertEqual(due.plan_id, self.plan.pk) + self.assertEqual(due.amount, amount) + self.assertEqual(due.period_end, period_end) + self.assertEqual(due.grace_until, grace_until) + + def test_a_club_not_on_the_plan_is_unaffected(self): + other_plan = Plan.objects.create(name="Other") + PlanPrice.objects.create(plan=other_plan, active_from=self.today - datetime.timedelta(days=1200), amount=Decimal("100.00")) + untouched = Club.objects.create(name="Untouched FC") + subscribe(untouched, other_plan) + + delete_plan(self.plan) + + self.assertTrue(Subscription.objects.filter(club=untouched, plan=other_plan).exists()) + + def test_a_trial_scheduled_to_convert_to_the_deleted_plan_is_cleared(self): + trial_plan = Plan.objects.create(name="Trial", is_trial=True, duration_months=2, renewal_lead_days=7, grace_days=14) + PlanPrice.objects.create(plan=trial_plan, active_from=self.today - datetime.timedelta(days=1200), amount=Decimal("0.00")) + club = Club.objects.create(name="Mid Trial FC") + start_trial(club, trial_plan, post_trial_plan=self.plan) + + impact = delete_plan(self.plan) + + self.assertEqual([c.pk for c in impact.broken_trial_clubs], [club.pk]) + club.refresh_from_db() + subscription = club.subscription + self.assertEqual(subscription.plan, trial_plan) + self.assertIsNone(subscription.trial_ends_at) + self.assertIsNone(subscription.post_trial_plan) + + def test_a_club_currently_on_the_plan_is_not_also_counted_as_a_broken_trial(self): + subscribe(self.club, self.plan) + + impact = delete_plan(self.plan) + + self.assertEqual(impact.unsubscribed_clubs, [self.club]) + self.assertEqual(impact.broken_trial_clubs, []) + + def test_plan_deletion_impact_is_read_only(self): + subscribe(self.club, self.plan) + + plan_deletion_impact(self.plan) + + self.assertTrue(Subscription.objects.filter(club=self.club).exists()) + self.assertFalse(self.plan.is_deleted) diff --git a/controlpanel/forms.py b/controlpanel/forms.py index 60bbc81..9497366 100644 --- a/controlpanel/forms.py +++ b/controlpanel/forms.py @@ -126,7 +126,7 @@ class SubscriptionForm(forms.ModelForm): # An inactive plan still bills its existing subscriptions, but must not be picked up # by a new one — which is the whole point of retiring a plan. Trial plans are excluded # too: they are reached through the trial form, which converts them properly. - self.fields["plan"].queryset = Plan.objects.filter(is_active=True, is_trial=False) + self.fields["plan"].queryset = Plan.objects.visible().filter(is_active=True, is_trial=False) class TrialForm(forms.Form): @@ -150,8 +150,8 @@ class TrialForm(forms.Form): super().__init__(*args, **kwargs) # Same reasoning as SubscriptionForm: a retired plan keeps billing whoever is # already on it, but must not be offered for a new trial or a new plan either. - self.fields["trial_plan"].queryset = Plan.objects.filter(is_active=True, is_trial=True) - self.fields["post_trial_plan"].queryset = Plan.objects.filter(is_active=True, is_trial=False) + self.fields["trial_plan"].queryset = Plan.objects.visible().filter(is_active=True, is_trial=True) + self.fields["post_trial_plan"].queryset = Plan.objects.visible().filter(is_active=True, is_trial=False) class DuePaymentForm(forms.Form): diff --git a/controlpanel/templates/controlpanel/billing.html b/controlpanel/templates/controlpanel/billing.html index f8af31e..b0c8aa9 100644 --- a/controlpanel/templates/controlpanel/billing.html +++ b/controlpanel/templates/controlpanel/billing.html @@ -65,6 +65,7 @@
{% blocktrans with plan=plan.name %}“{{ plan }}” has never billed anyone, so it will be removed completely.{% endblocktrans %}
+ {% else %} +{% blocktrans with plan=plan.name %}“{{ plan }}” has billing history, so it will be hidden rather than removed — past invoices will still show what they were billed under.{% endblocktrans %}
+ {% endif %} +{% trans "Deleting this plan removes their subscription outright — each shows as not billed for anything afterwards, the same as a club that was never put on a plan." %}
+{% trans "They stay on their current trial plan, but the scheduled switch is cancelled — pick a new plan for them before the trial ends." %}
+