-
Notifications
You must be signed in to change notification settings - Fork 10k
fix(subscription): add cancellation and date validation #51199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThis change modifies the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/accounts/doctype/subscription/subscription.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.
Applied to files:
erpnext/accounts/doctype/subscription/subscription.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: triage
- GitHub Check: Summary
🔇 Additional comments (1)
erpnext/accounts/doctype/subscription/subscription.py (1)
564-574: No action needed. The cancellation logic is correct and complete.The early return at line 574 does not create gaps in the cancellation logic. The new code (lines 564-574) and existing code (lines 579-583) handle complementary scenarios:
New code (lines 564-574): Runs immediately after invoice generation. It proactively checks if the next billing period start would exceed
end_date. When this condition is met, it cancels the subscription and returns, preventing further processing.Existing code (lines 579-583): Only reaches this point if the early return didn't trigger (meaning
next_start <= end_date). It then checks ifposting_date >= current_invoice_endorposting_date >= end_dateto handle other cancellation scenarios.These conditions are distinct and serve different purposes. There are no edge cases where cancellation should occur but neither path triggers. The logic flow ensures that once
cancelation_dateis set,can_generate_new_invoice()will returnFalse, preventing further invoice generation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #51199 +/- ##
========================================
Coverage 79.01% 79.01%
========================================
Files 1175 1175
Lines 120213 120246 +33
========================================
+ Hits 94981 95008 +27
- Misses 25232 25238 +6
🚀 New features to boost your workflow:
|
Prevents subscription periods from extending past the end date and handles cancellation correctly at period end.