fix: payment recalculation logic in case of date update through list view #51118
Conversation
nishkagosalia
requested review from
mihir-kandoi,
rohitwaghchaure and
ruthra-kumar
as code owners
📝 WalkthroughWalkthroughReordered payment-date logic in invoices: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
erpnext/accounts/doctype/sales_invoice/test_sales_invoice.py (1)
115-150: Strengthen test to exercise the actualset_payment_scheduleintegrationThe test correctly verifies
recalculate_payment_due_datein isolation, but it does not cover the real bug path: changing an existing invoice’sposting_dateand lettingAccountsController.set_payment_schedule(viavalidate_invoice_documents_schedule) recalc the payment schedule.Consider extending or adding a test along these lines:
- Create and insert a
Sales Invoicewith a payment terms template so thatpayment_scheduleis auto-built.- Capture the original
payment_schedule[0].due_date.- Change
si.posting_dateto a new date and callsi.save()(orsubmitif that’s the realistic flow).- Reload and assert that
payment_scheduledue dates (andsi.due_date) have shifted according to the newposting_date.This will give you regression coverage on the wiring you added in
set_payment_schedule, not just the helper itself.erpnext/controllers/accounts_controller.py (1)
2570-2571: Make payment-due-date recalculation more defensive and targetedThe new branch and helper solve the core issue (recomputing schedule due dates when
posting_datechanges), but a couple of refinements would reduce surprise behaviour and align with existing expectations aroundget_due_date:
Guard against
posting_datebeingNoneand for non-template rowsIf a
Payment Schedulerow doesn’t originate from a payment term (i.e. has nodue_date_based_on/ is purely manual), recomputing viaget_due_date(terms, posting_date)can either:
- overwrite a user-entered
due_date, or- produce
Noneifdue_date_based_onis unset or an unexpected value.Also, per earlier work in this module, the
posting_datepassed intoget_due_dateis expected to be non-None.You can make
recalculate_payment_due_datesafer with a small guard:-def recalculate_payment_due_date(posting_date, payment_schedule): - for terms in payment_schedule: - terms.due_date = get_due_date(terms, posting_date) +def recalculate_payment_due_date(posting_date, payment_schedule): + # Defensive: nothing to recalculate if we don't have a posting_date + if not posting_date: + return + + for terms in payment_schedule: + # Only recalculate when term metadata is present + if getattr(terms, "due_date_based_on", None): + terms.due_date = get_due_date(terms, posting_date)This keeps the fix for template-driven schedules, while leaving fully manual schedules alone and avoiding accidental
Nonedue dates. Based on learnings, this also maintains the assumption that theposting_datepassed intoget_due_dateis always valid.Align the “old vs new” date comparison with the computed
posting_dateRight now, the condition uses
self.get_doc_before_save().posting_date != posting_date. For doctypes whose driver field istransaction_date(e.g.,Sales Order/Purchase Order), this attribute is alwaysNone, so the condition will always be true even if the effective date didn’t change.If you want this comparison to work uniformly across doctypes, consider computing the “old” logical posting date the same way:
- elif not self.is_new() and self.get_doc_before_save().posting_date != posting_date: - recalculate_payment_due_date(posting_date, self.payment_schedule) + elif not self.is_new(): + prev_doc = self.get_doc_before_save() + prev_posting_date = ( + prev_doc.get("bill_date") + or prev_doc.get("posting_date") + or prev_doc.get("transaction_date") + ) + + if prev_posting_date != posting_date: + recalculate_payment_due_date(posting_date, self.payment_schedule)This keeps the logic focused on “effective posting date changed” regardless of whether the doctype uses
posting_dateortransaction_date, and avoids unnecessary recalcs.Overall behaviour for invoice/payment-term driven schedules stays the same, but these guards make the change safer for edge cases and manual schedules.
Also applies to: 4366-4368
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/doctype/sales_invoice/test_sales_invoice.py(2 hunks)erpnext/controllers/accounts_controller.py(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ljain112
Repo: frappe/erpnext PR: 48864
File: erpnext/controllers/accounts_controller.py:2586-2596
Timestamp: 2025-08-20T11:58:32.385Z
Learning: In erpnext/controllers/accounts_controller.py, the posting_date parameter passed to get_due_date() and get_discount_date() is mandatory and calculated from bill_date/posting_date/transaction_date, so it should not be None.
📚 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/sales_invoice/test_sales_invoice.pyerpnext/controllers/accounts_controller.py
📚 Learning: 2025-08-20T11:58:32.385Z
Learnt from: ljain112
Repo: frappe/erpnext PR: 48864
File: erpnext/controllers/accounts_controller.py:2586-2596
Timestamp: 2025-08-20T11:58:32.385Z
Learning: In erpnext/controllers/accounts_controller.py, the posting_date parameter passed to get_due_date() and get_discount_date() is mandatory and calculated from bill_date/posting_date/transaction_date, so it should not be None.
Applied to files:
erpnext/controllers/accounts_controller.py
🧬 Code graph analysis (2)
erpnext/accounts/doctype/sales_invoice/test_sales_invoice.py (1)
erpnext/controllers/accounts_controller.py (1)
recalculate_payment_due_date(4366-4368)
erpnext/controllers/accounts_controller.py (1)
erpnext/accounts/party.py (1)
get_due_date(625-646)
⏰ 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). (6)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (2)
erpnext/accounts/doctype/sales_invoice/test_sales_invoice.py (1)
30-34: Import ofrecalculate_payment_due_dateis appropriate and scopedImport is used only in the new test and keeps related controller helpers together; no issues from a dependency or style standpoint.
erpnext/controllers/accounts_controller.py (1)
647-666: Callingset_due_date()afterset_payment_schedule()makes the invoice-level due date consistentMoving
self.set_due_date()intovalidate_invoice_documents_scheduleright afterself.set_payment_schedule()ensuresself.due_dateis always derived from the currentpayment_schedulebefore running amount/due-date validation, which aligns the header field with the schedule rows.
|
In the test case we are supposed to edit the |
nishkagosalia
force-pushed
the
gh-50949
branch
from
6ae036b to
258013c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
erpnext/accounts/doctype/sales_invoice/test_sales_invoice.py (2)
119-126: Usefrappe.new_docfor creating new documents.As noted in previous reviews, prefer
frappe.new_doc("Payment Term")overfrappe.get_doc({...})for better readability when creating new documents.Apply this diff:
- payment_term = frappe.get_doc( - { - "doctype": "Payment Term", + payment_term = frappe.new_doc("Payment Term") + payment_term.update( + { "payment_term_name": "Test Term 2 Days", "invoice_portion": 100, "credit_days": 2, - } - ).save() + } + ) + payment_term.save()
128-134: Avoid abbreviations in variable names.As previously noted,
pttis not immediately clear. Usepayment_terms_templatefor better readability.Apply this diff:
- ptt = frappe.get_doc( + payment_terms_template = frappe.new_doc("Payment Terms Template") + payment_terms_template.update( { - "doctype": "Payment Terms Template", "template_name": "Test Template Recalc", "terms": [{"payment_term": payment_term.name, "invoice_portion": 100, "credit_days": 2}], - } - ).save() + } + ) + payment_terms_template.save() si = create_sales_invoice(do_not_save=1) si.set_posting_time = 1 si.posting_date = posting_date - si.payment_terms_template = ptt.name + si.payment_terms_template = payment_terms_template.name
🧹 Nitpick comments (1)
erpnext/accounts/doctype/sales_invoice/test_sales_invoice.py (1)
143-146: Consider direct attribute assignment for clarity.While using
.update()works, direct assignment is more explicit and idiomatic for updating a single field.Apply this diff:
- si.update({"posting_date": new_posting_date}) + si.posting_date = new_posting_date si.save()
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/accounts/doctype/sales_invoice/test_sales_invoice.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ljain112
Repo: frappe/erpnext PR: 48864
File: erpnext/controllers/accounts_controller.py:2586-2596
Timestamp: 2025-08-20T11:58:32.385Z
Learning: In erpnext/controllers/accounts_controller.py, the posting_date parameter passed to get_due_date() and get_discount_date() is mandatory and calculated from bill_date/posting_date/transaction_date, so it should not be None.
📚 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/sales_invoice/test_sales_invoice.py
🧬 Code graph analysis (1)
erpnext/accounts/doctype/sales_invoice/test_sales_invoice.py (3)
erpnext/controllers/accounts_controller.py (2)
InvalidQtyError(86-87)recalculate_payment_due_date(4373-4375)erpnext/accounts/doctype/tax_withholding_category/test_tax_withholding_category.py (1)
create_sales_invoice(1025-1055)erpnext/accounts/doctype/payment_reconciliation/test_payment_reconciliation.py (1)
create_sales_invoice(146-176)
⏰ 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). (6)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (2)
erpnext/accounts/doctype/sales_invoice/test_sales_invoice.py (2)
30-34: LGTM! Import correctly includes the new function.The import of
recalculate_payment_due_dateis properly added and will be used in the test method below.
115-147: Test correctly validates payment due date recalculation.The test successfully verifies that updating
posting_datetriggers recalculation ofpayment_schedule[0].due_date, which is exactly what the PR aims to fix. The test flow is clear and assertions are appropriate.
nishkagosalia
force-pushed
the
gh-50949
branch
from
a7648c5 to
3753552
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/doctype/sales_invoice/test_sales_invoice.py(2 hunks)erpnext/controllers/accounts_controller.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- erpnext/accounts/doctype/sales_invoice/test_sales_invoice.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ljain112
Repo: frappe/erpnext PR: 48864
File: erpnext/controllers/accounts_controller.py:2586-2596
Timestamp: 2025-08-20T11:58:32.385Z
Learning: In erpnext/controllers/accounts_controller.py, the posting_date parameter passed to get_due_date() and get_discount_date() is mandatory and calculated from bill_date/posting_date/transaction_date, so it should not be None.
📚 Learning: 2025-08-20T11:58:32.385Z
Learnt from: ljain112
Repo: frappe/erpnext PR: 48864
File: erpnext/controllers/accounts_controller.py:2586-2596
Timestamp: 2025-08-20T11:58:32.385Z
Learning: In erpnext/controllers/accounts_controller.py, the posting_date parameter passed to get_due_date() and get_discount_date() is mandatory and calculated from bill_date/posting_date/transaction_date, so it should not be None.
Applied to files:
erpnext/controllers/accounts_controller.py
📚 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/controllers/accounts_controller.py
🧬 Code graph analysis (1)
erpnext/controllers/accounts_controller.py (1)
erpnext/accounts/party.py (1)
get_due_date(625-646)
⏰ 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). (6)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (1)
erpnext/controllers/accounts_controller.py (1)
662-662: LGTM! Correct ordering of due date calculation.The repositioning of
set_due_date()afterset_payment_schedule()is correct, as the payment schedule must be populated before extracting the due date from it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #51118 +/- ##
===========================================
+ Coverage 78.97% 79.03% +0.05%
===========================================
Files 1175 1175
Lines 120026 120200 +174
===========================================
+ Hits 94791 94998 +207
+ Misses 25235 25202 -33
🚀 New features to boost your workflow:
|
nishkagosalia
force-pushed
the
gh-50949
branch
from
3753552 to
8f99850
Compare
nishkagosalia
force-pushed
the
gh-50949
branch
from
8f99850 to
3d37ce0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
erpnext/controllers/accounts_controller.py (1)
4378-4380: Add validation and filter to only update entries with payment terms.The function has several concerns that should be addressed:
Missing docstring: Add documentation explaining the function's purpose and parameters.
No posting_date validation: Based on learnings, the posting_date parameter should be validated as non-None before calling
get_due_date.Critical: Updates all entries without checking payment_term: The function currently recalculates due dates for ALL payment schedule entries. If an entry doesn't have a
payment_term,get_due_date()will return None (sincedue_date_based_onwon't be set), which would overwrite manually set due dates. This affects mixed scenarios where some entries use payment terms and others have manual due dates.Apply this diff to add validation and filtering:
def recalculate_payment_due_date(posting_date, payment_schedule): + """ + Recalculate due dates for payment schedule entries when the posting date changes. + Only updates entries that have an associated payment term. + + Args: + posting_date: The new posting date to calculate due dates from (must not be None) + payment_schedule: List of payment schedule entries + """ + if not posting_date: + return + for terms in payment_schedule: - terms.due_date = get_due_date(terms, posting_date) + # Only recalculate if a payment term is defined + if terms.payment_term: + terms.due_date = get_due_date(terms, posting_date)Based on learnings, the posting_date parameter should be mandatory and validated.
🧹 Nitpick comments (2)
erpnext/accounts/doctype/sales_invoice/test_sales_invoice.py (2)
30-34: Remove unused import.The
recalculate_payment_due_datefunction is imported but never used in this test file. The test correctly relies on the automatic recalculation triggered by the controller'ssave()method rather than calling this utility function directly.Apply this diff:
from erpnext.controllers.accounts_controller import ( InvalidQtyError, - recalculate_payment_due_date, update_invoice_status, )
115-141: Good test implementation with a minor suggestion.The test correctly verifies that updating
posting_datetriggers recalculation ofdue_datein the payment schedule, which addresses the issue described in the PR objectives. The test logic is sound and follows the codebase conventions (usingfrappe.new_doc, callingsave()instead ofinsert()).However, consider checking if the payment term and template already exist before creating them, or use more unique names to prevent potential duplicate key errors if tests are run multiple times in scenarios where rollback doesn't occur:
payment_term_name = f"Test Term 2 Days {nowdate()}" template_name = f"Test Template Recalc {nowdate()}"Alternatively, you could check and reuse existing records:
if not frappe.db.exists("Payment Term", "Test Term 2 Days"): payment_term = frappe.new_doc("Payment Term") # ... rest of the creation logic else: payment_term = frappe.get_doc("Payment Term", "Test Term 2 Days")
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/doctype/sales_invoice/test_sales_invoice.py(2 hunks)erpnext/controllers/accounts_controller.py(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ljain112
Repo: frappe/erpnext PR: 48864
File: erpnext/controllers/accounts_controller.py:2586-2596
Timestamp: 2025-08-20T11:58:32.385Z
Learning: In erpnext/controllers/accounts_controller.py, the posting_date parameter passed to get_due_date() and get_discount_date() is mandatory and calculated from bill_date/posting_date/transaction_date, so it should not be None.
📚 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/sales_invoice/test_sales_invoice.pyerpnext/controllers/accounts_controller.py
📚 Learning: 2025-08-20T11:58:32.385Z
Learnt from: ljain112
Repo: frappe/erpnext PR: 48864
File: erpnext/controllers/accounts_controller.py:2586-2596
Timestamp: 2025-08-20T11:58:32.385Z
Learning: In erpnext/controllers/accounts_controller.py, the posting_date parameter passed to get_due_date() and get_discount_date() is mandatory and calculated from bill_date/posting_date/transaction_date, so it should not be None.
Applied to files:
erpnext/controllers/accounts_controller.py
🧬 Code graph analysis (2)
erpnext/accounts/doctype/sales_invoice/test_sales_invoice.py (1)
erpnext/controllers/accounts_controller.py (3)
InvalidQtyError(86-87)recalculate_payment_due_date(4378-4380)update_invoice_status(3502-3555)
erpnext/controllers/accounts_controller.py (1)
erpnext/accounts/party.py (1)
get_due_date(625-646)
⏰ 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). (6)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (2)
erpnext/controllers/accounts_controller.py (2)
662-662: LGTM: Correct ordering of due date calculation.Calling
set_due_date()afterset_payment_schedule()ensures the due date is calculated from the finalized payment schedule, which is the correct logical sequence.
2570-2583: LGTM: Safe posting date change detection.The logic properly detects posting date changes on existing documents and safely checks for payment terms using
any(), which avoids the IndexError that existed in earlier versions. The guards ensure the recalculation only runs when appropriate.
3 participants
The following fixes : #50949
In the scenario when we try updating the posting date in sales invoice through list view using the edit function, it used to not update the payment due date. This is fixed in this solution.
no-docs