From edc8daae21c69a365957edb173f28f874f7ff10e Mon Sep 17 00:00:00 2001 From: Jatin3128 <140256508+Jatin3128@users.noreply.github.com> Date: Wed, 5 Aug 2026 12:26:01 +0530 Subject: [PATCH] fix(subscription): don't reactivate a cancelled subscription (backport #57774) (#57780) * fix(subscription): don't reactivate a cancelled subscription Backport of #57774 to version-15-hotfix. set_subscription_status() unconditionally set status to Active once there was no outstanding invoice, with no check for whether the subscription had been intentionally cancelled. process()'s cancel_at_period_end check compared posting_date against getdate(self.end_date), and getdate(None) returns today, so an empty end_date was silently treated as "cancel now" on every reprocess. version-15-hotfix does not yet have the Payment Entry -> invoice -> refresh_subscription_status() hook chain that #57761 reports (that was added later), and the scheduler already excludes Cancelled subscriptions, so the exact automatic repro in the issue does not apply here. The underlying status logic is defective the same way though, and reachable directly (e.g. any caller invoking the whitelisted process()/set_subscription_status() on a cancelled subscription), so fixing it here too rather than leaving the same class of bug in place. Related to #57761 * fix(test): avoid tripping the unrelated period-end cancel clause The test backdated start_date to 2018-01-01, so by the time process() ran, current_invoice_end was long past. That made the *other* half of the cancel_at_period_end OR-condition (period end reached) true on its own, so process() tried to cancel an already-cancelled subscription and hit cancel_subscription()'s "already cancelled" guard - unrelated to the empty end_date bug being tested. Use nowdate() as start_date so current_invoice_end sits safely in the future, isolating the assertion to the empty end_date guard. Also drop the fragile intermediate "Unpaid" status assertion, which wasn't part of what this test verifies. * fix(subscription): don't let period rollover defeat cancel_at_period_end process() can advance current_invoice_end to the next period (via update_subscription_period(), when generating the current period's invoice) before the cancel_at_period_end check further down runs. For a subscription with no end_date, that check now compared posting_date against the already-rolled-forward current_invoice_end, which is always in the future, so cancel_at_period_end was silently never honored. Snapshot current_invoice_end before any rollover and use that in the check instead, so it still targets the period that just ended. Fixes the P1 flagged by Greptile review on this PR. --------- Co-authored-by: test --- .../doctype/subscription/subscription.py | 12 ++++- .../doctype/subscription/test_subscription.py | 53 +++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/erpnext/accounts/doctype/subscription/subscription.py b/erpnext/accounts/doctype/subscription/subscription.py index 3665bf34bf2..e7dc9b078fc 100644 --- a/erpnext/accounts/doctype/subscription/subscription.py +++ b/erpnext/accounts/doctype/subscription/subscription.py @@ -222,6 +222,9 @@ class Subscription(Document): """ Sets the status of the `Subscription` """ + if self.status == "Cancelled": + return + if self.is_trialling(): self.status = "Trialling" elif self.status == "Active" and self.end_date and getdate(posting_date) > getdate(self.end_date): @@ -558,6 +561,11 @@ class Subscription(Document): 1. `process_for_active` 2. `process_for_past_due` """ + # Snapshot before update_subscription_period() below can roll this forward, + # so the cancel_at_period_end check further down still targets the period + # that just ended, not the next one. + current_period_end = self.current_invoice_end + if not self.is_current_invoice_generated( self.current_invoice_start, self.current_invoice_end ) and self.can_generate_new_invoice(posting_date): @@ -567,8 +575,8 @@ class Subscription(Document): self.update_subscription_period() if self.cancel_at_period_end and ( - getdate(posting_date) >= getdate(self.current_invoice_end) - or getdate(posting_date) >= getdate(self.end_date) + getdate(posting_date) >= getdate(current_period_end) + or (self.end_date and getdate(posting_date) >= getdate(self.end_date)) ): self.cancel_subscription() diff --git a/erpnext/accounts/doctype/subscription/test_subscription.py b/erpnext/accounts/doctype/subscription/test_subscription.py index 41ada4c804f..5cf52fa1eb4 100644 --- a/erpnext/accounts/doctype/subscription/test_subscription.py +++ b/erpnext/accounts/doctype/subscription/test_subscription.py @@ -280,6 +280,59 @@ class TestSubscription(FrappeTestCase): settings.cancel_after_grace = default_grace_period_action settings.save() + def test_cancelled_subscription_stays_cancelled_after_payment_and_reprocess(self): + # https://github.com/frappe/erpnext/issues/57761 + subscription = create_subscription( + start_date=nowdate(), generate_invoice_at="Beginning of the current subscription period" + ) + subscription.process(posting_date=nowdate()) # generate first invoice + invoice = subscription.get_current_invoice() + self.assertIsNotNone(invoice) + + invoice.db_set("outstanding_amount", 0) + invoice.db_set("status", "Paid") + + subscription.cancel_subscription() + self.assertEqual(subscription.status, "Cancelled") + cancelation_date = getdate(subscription.cancelation_date) + + subscription.set_subscription_status() + self.assertEqual(subscription.status, "Cancelled") + self.assertEqual(getdate(subscription.cancelation_date), cancelation_date) + + subscription.cancel_at_period_end = 1 + subscription.end_date = None + invoice_count = len(subscription.invoices) + subscription.process() + self.assertEqual(subscription.status, "Cancelled") + self.assertEqual(len(subscription.invoices), invoice_count) + + def test_subscription_cancels_at_period_end_without_end_date(self): + # https://github.com/frappe/erpnext/issues/57761 -- generate_invoice() rolls + # current_invoice_end forward to the next period before this check runs, so + # with no end_date to fall back on, cancel_at_period_end must compare + # against the period that just ended, not the (already advanced) next one. + create_plan( + plan_name="_Test plan name 11", + cost=80, + currency="INR", + billing_interval="Day", + billing_interval_count=3, + ) + subscription = create_subscription( + start_date=nowdate(), + generate_invoice_at="End of the current subscription period", + plans=[{"plan": "_Test plan name 11", "qty": 1}], + ) + subscription.cancel_at_period_end = 1 + self.assertEqual(len(subscription.invoices), 0) + period_end = subscription.current_invoice_end + + subscription.process(posting_date=period_end) + + self.assertEqual(subscription.status, "Cancelled") + self.assertEqual(len(subscription.invoices), 1) + def test_subscription_restart_and_process(self): settings = frappe.get_single("Subscription Settings") default_grace_period_action = settings.cancel_after_grace