fix(subscription): correct billing/deferred bugs and tighten guards (#55554)

erpnext/accounts/doctype/subscription/subscription.py

@@ -269,7 +269,7 @@ class Subscription(Document):
 		Returns `True` if the grace period for the `Subscription` has passed
 		"""
 		if not self.current_invoice_is_past_due():
-			return
+			return False
 
 		grace_period = cint(frappe.get_value("Subscription Settings", None, "grace_period"))
 		return getdate(posting_date) >= getdate(add_days(self.current_invoice.due_date, grace_period))
@@ -281,6 +281,9 @@ class Subscription(Document):
 		if not self.current_invoice or self.is_paid(self.current_invoice):
 			return False
 
+		if not self.current_invoice.due_date:
+			return False
+
 		return getdate(posting_date) >= getdate(self.current_invoice.due_date)
 
 	@property
@@ -345,7 +348,13 @@ class Subscription(Document):
 			frappe.throw(_("Trial Period Start date cannot be after Subscription Start Date"))
 
 	def validate_end_date(self) -> None:
+		if not self.plans:
+			return
+
 		billing_cycle_info = self.get_billing_cycle_data()
+		if not billing_cycle_info:
+			return
+
 		end_date = add_to_date(self.start_date, **billing_cycle_info)
 
 		if self.end_date and getdate(self.end_date) <= getdate(end_date):
@@ -514,7 +523,7 @@ class Subscription(Document):
 
 			item_code = plan_doc.item
 
-			if self.party == "Customer":
+			if self.party_type == "Customer":
 				deferred_field = "enable_deferred_revenue"
 			else:
 				deferred_field = "enable_deferred_expense"
@@ -598,19 +607,28 @@ class Subscription(Document):
 		if self.has_outstanding_invoice() and not self.generate_new_invoices_past_due_date:
 			return False
 
-		if self.generate_invoice_at == "Beginning of the current subscription period" and (
-			getdate(posting_date) == getdate(self.current_invoice_start)
-		):
-			return True
-		elif self.generate_invoice_at == "Days before the current subscription period" and (
-			getdate(posting_date) == getdate(add_days(self.current_invoice_start, -1 * self.number_of_days))
-		):
-			return True
-		elif getdate(posting_date) == getdate(self.current_invoice_end):
-			return True
+		posting = getdate(posting_date)
+
+		if self.generate_invoice_at == "Beginning of the current subscription period":
+			trigger = getdate(self.current_invoice_start)
+		elif self.generate_invoice_at == "Days before the current subscription period":
+			trigger = getdate(add_days(self.current_invoice_start, -1 * self.number_of_days))
 		else:
+			trigger = getdate(self.current_invoice_end)
+
+		if posting < trigger:
 			return False
 
+		# Cap the late-fire window at one billing cycle past the period end so a
+		# multi-year gap doesn't retroactively bill cycle after cycle in one call.
+		billing_cycle_info = self.get_billing_cycle_data()
+		if billing_cycle_info:
+			upper = getdate(add_to_date(self.current_invoice_end, **billing_cycle_info))
+		else:
+			upper = getdate(self.current_invoice_end)
+
+		return posting <= upper
+
 	def is_current_invoice_generated(
 		self,
 		_current_start_date: DateTimeLikeObject | None = None,
@@ -650,13 +668,6 @@ class Subscription(Document):
 		if invoice:
 			return frappe.get_doc(self.invoice_document_type, invoice[0])
 
-	def cancel_subscription_at_period_end(self) -> None:
-		"""
-		Called when `Subscription.cancel_at_period_end` is truthy
-		"""
-		self.status = "Cancelled"
-		self.cancelation_date = nowdate()
-
 	@property
 	def invoices(self) -> list[dict]:
 		return frappe.get_all(
@@ -703,7 +714,7 @@ class Subscription(Document):
 		self.status = "Cancelled"
 		self.cancelation_date = nowdate()
 
-		if to_generate_invoice and self.cancelation_date >= self.current_invoice_start:
+		if to_generate_invoice and getdate(self.cancelation_date) >= getdate(self.current_invoice_start):
 			self.generate_invoice(self.current_invoice_start, self.cancelation_date)
 
 		self.save()
@@ -731,7 +742,7 @@ class Subscription(Document):
 		"""
 
 		# Don't process future subscriptions
-		if nowdate() < self.current_invoice_start:
+		if getdate(nowdate()) < getdate(self.current_invoice_start):
 			frappe.msgprint(_("Subscription for Future dates cannot be processed."))
 			return
 
@@ -770,10 +781,10 @@ def process_all(subscription: list, posting_date: DateTimeLikeObject | None = No
 
 	for subscription_name in subscription:
 		try:
-			subscription = frappe.get_doc("Subscription", subscription_name)
-			subscription.process(posting_date)
+			sub = frappe.get_doc("Subscription", subscription_name)
+			sub.process(posting_date)
 			if not frappe.in_test:
 				frappe.db.commit()
 		except frappe.ValidationError:
 			frappe.db.rollback()
-			subscription.log_error("Subscription failed")
+			sub.log_error("Subscription failed")

erpnext/accounts/doctype/subscription/test_subscription.py

@@ -16,7 +16,7 @@ from frappe.utils.data import (
 )
 
 from erpnext.accounts.doctype.payment_entry.payment_entry import get_payment_entry
-from erpnext.accounts.doctype.subscription.subscription import get_prorata_factor
+from erpnext.accounts.doctype.subscription.subscription import Subscription, get_prorata_factor, process_all
 from erpnext.tests.utils import ERPNextTestSuite
 
 
@@ -637,6 +637,62 @@ class TestSubscription(ERPNextTestSuite):
 
 		self.assertRaises(frappe.ValidationError, subscription.process, posting_date=add_days(start_date, 7))
 
+	def test_invoice_generated_when_scheduler_runs_one_day_late(self):
+		subscription = create_subscription(start_date="2018-01-01")
+		self.assertEqual(subscription.current_invoice_end, "2018-01-31")
+
+		subscription.process(posting_date="2018-02-01")
+		self.assertEqual(len(subscription.invoices), 1)
+
+	def test_deferred_revenue_applied_for_customer_subscription(self):
+		item_code = "_Test Non Stock Item"
+		frappe.db.set_value("Item", item_code, "enable_deferred_revenue", 1)
+		try:
+			subscription = create_subscription(start_date="2018-01-01")
+			items = subscription.get_items_from_plans(subscription.plans)
+			self.assertEqual(items[0].get("enable_deferred_revenue"), 1)
+			self.assertEqual(getdate(items[0]["service_start_date"]), getdate("2018-01-01"))
+			self.assertEqual(getdate(items[0]["service_end_date"]), getdate("2018-01-31"))
+		finally:
+			frappe.db.set_value("Item", item_code, "enable_deferred_revenue", 0)
+
+	def test_validate_end_date_with_no_plans_does_not_crash(self):
+		sub = frappe.new_doc("Subscription")
+		sub.party_type = "Customer"
+		sub.party = "_Test Customer"
+		sub.company = "_Test Company"
+		sub.start_date = "2018-01-01"
+		sub.end_date = "2018-03-01"
+		try:
+			sub.validate_end_date()
+		except TypeError as e:
+			self.fail(f"validate_end_date crashed with no plans: {e}")
+
+	def test_process_all_logs_error_when_first_subscription_fails(self):
+		sub1 = create_subscription(start_date="2018-01-01")
+		sub2 = create_subscription(start_date="2018-01-02")
+
+		processed = []
+		original_process = Subscription.process
+		original_rollback = frappe.db.rollback
+
+		def patched(self, posting_date=None):
+			processed.append(self.name)
+			if self.name == sub1.name:
+				raise frappe.ValidationError("forced failure")
+
+		Subscription.process = patched
+		# process_all calls frappe.db.rollback() on error which would otherwise wipe
+		# the test transaction; stub it so we can observe the iteration in isolation.
+		frappe.db.rollback = lambda *a, **kw: None
+		try:
+			process_all([sub1.name, sub2.name])
+		finally:
+			Subscription.process = original_process
+			frappe.db.rollback = original_rollback
+
+		self.assertEqual(processed, [sub1.name, sub2.name])
+
 	def test_subscription_auto_completion(self):
 		create_plan(
 			plan_name="_Test Plan 3 Day",