Fix advance payment#22
Conversation
WalkthroughThis update introduces and refines the handling of advance payment ledger entries across accounting modules. It adds new fields for tracking advance voucher references, updates ledger entry creation and reconciliation logic, adjusts related tests, and modifies UI mappings. Several methods are refactored or removed, and new database patch scripts are included to update and migrate ledger data accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PaymentEntry
participant GL_Entry
participant AdvancePaymentLedgerEntry
participant JournalEntry
User->>PaymentEntry: Submit Payment Entry
PaymentEntry->>GL_Entry: Create GL Entries
PaymentEntry->>AdvancePaymentLedgerEntry: Create Advance Payment Ledger Entries (if advance)
PaymentEntry->>GL_Entry: Update GL with advance_voucher_type/no
PaymentEntry->>AdvancePaymentLedgerEntry: Update or delink on reconciliation/cancellation
User->>JournalEntry: Submit Journal Entry
JournalEntry->>GL_Entry: Create GL Entries
JournalEntry->>AdvancePaymentLedgerEntry: Create Advance Payment Ledger Entries (if advance)
JournalEntry->>GL_Entry: Update GL with advance_voucher_type/no
JournalEntry->>AdvancePaymentLedgerEntry: Update or delink on reconciliation/cancellation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: in erpnext payment ledger entry, advance payment amounts are stored as negative values, but when cal...Applied to files:
🧬 Code Graph Analysis (2)erpnext/accounts/doctype/unreconcile_payment/unreconcile_payment.py (1)
erpnext/accounts/doctype/unreconcile_payment/test_unreconcile_payment.py (1)
⏰ 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). (1)
🔇 Additional comments (8)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
erpnext/patches/v15_0/create_advance_payment_ledger_records.py (2)
39-66: LGTM! Well-structured query with minor optimization opportunity.The query builder implementation is clean and handles currency selection correctly. The inner join and filtering logic ensure data integrity.
Consider filtering negative amounts at the query level for better performance:
entries = ( frappe.qb.from_(per) .inner_join(pe) .on(pe.name == per.parent) .select( pe.company, per.parenttype.as_("voucher_type"), per.parent.as_("voucher_no"), per.reference_doctype.as_("against_voucher_type"), per.reference_name.as_("against_voucher_no"), per.allocated_amount.as_("amount"), Case() .when(pe.payment_type == "Receive", pe.paid_from_account_currency) .else_(pe.paid_to_account_currency) .as_("currency"), ) - .where(per.reference_doctype.isin(advance_doctpyes) & per.docstatus.eq(1)) + .where(per.reference_doctype.isin(advance_doctpyes) & per.docstatus.eq(1) & per.allocated_amount > 0) .run(as_dict=True) )
69-96: LGTM! Consistent implementation with payment entries.The journal entry processing follows the same solid pattern as payment entries, with appropriate amount selection based on account type.
Consider applying the same optimization for negative amounts:
- .where(jea.reference_type.isin(advance_doctpyes) & jea.docstatus.eq(1)) + .where(jea.reference_type.isin(advance_doctpyes) & jea.docstatus.eq(1) & + ((jea.account_type == "Receivable" & jea.credit_in_account_currency > 0) | + (jea.account_type != "Receivable" & jea.debit_in_account_currency > 0)))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
erpnext/accounts/doctype/advance_payment_ledger_entry/advance_payment_ledger_entry.json(3 hunks)erpnext/accounts/doctype/advance_payment_ledger_entry/advance_payment_ledger_entry.py(2 hunks)erpnext/accounts/doctype/journal_entry/journal_entry.py(1 hunks)erpnext/accounts/doctype/journal_entry_account/journal_entry_account.json(2 hunks)erpnext/accounts/doctype/journal_entry_account/journal_entry_account.py(1 hunks)erpnext/accounts/doctype/payment_entry/payment_entry.py(5 hunks)erpnext/accounts/doctype/payment_entry/test_payment_entry.py(2 hunks)erpnext/accounts/doctype/payment_entry_reference/payment_entry_reference.json(2 hunks)erpnext/accounts/doctype/payment_entry_reference/payment_entry_reference.py(1 hunks)erpnext/accounts/doctype/payment_ledger_entry/payment_ledger_entry.json(1 hunks)erpnext/accounts/doctype/payment_request/test_payment_request.py(1 hunks)erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py(1 hunks)erpnext/accounts/doctype/repost_payment_ledger/repost_payment_ledger.py(1 hunks)erpnext/accounts/doctype/unreconcile_payment/unreconcile_payment.py(4 hunks)erpnext/accounts/general_ledger.py(1 hunks)erpnext/accounts/utils.py(14 hunks)erpnext/buying/doctype/purchase_order/purchase_order.py(1 hunks)erpnext/controllers/accounts_controller.py(2 hunks)erpnext/patches.txt(2 hunks)erpnext/patches/v15_0/create_advance_payment_ledger_records.py(1 hunks)erpnext/patches/v15_0/update_payment_ledger_entries_against_advance_doctypes.py(1 hunks)erpnext/public/js/utils/unreconcile.js(3 hunks)erpnext/selling/doctype/sales_order/sales_order.py(1 hunks)
🧰 Additional context used
🧠 Learnings (19)
📓 Common learnings
Learnt from: ljain112
PR: ljain112/erpnext#20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/accounts/doctype/payment_ledger_entry/payment_ledger_entry.json (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/patches.txt (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/accounts/doctype/repost_payment_ledger/repost_payment_ledger.py (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/accounts/doctype/payment_request/test_payment_request.py (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/accounts/general_ledger.py (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/accounts/doctype/payment_entry/test_payment_entry.py (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/patches/v15_0/update_payment_ledger_entries_against_advance_doctypes.py (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/accounts/doctype/payment_entry_reference/payment_entry_reference.py (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/selling/doctype/sales_order/sales_order.py (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/accounts/doctype/advance_payment_ledger_entry/advance_payment_ledger_entry.json (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/accounts/doctype/advance_payment_ledger_entry/advance_payment_ledger_entry.py (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/accounts/doctype/journal_entry/journal_entry.py (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/patches/v15_0/create_advance_payment_ledger_records.py (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/controllers/accounts_controller.py (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/accounts/doctype/unreconcile_payment/unreconcile_payment.py (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/accounts/doctype/payment_entry/payment_entry.py (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
erpnext/accounts/utils.py (1)
Learnt from: ljain112
PR: #20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
🧬 Code Graph Analysis (2)
erpnext/accounts/doctype/repost_payment_ledger/repost_payment_ledger.py (1)
erpnext/accounts/utils.py (3)
_delete_adv_pl_entries(1526-1528)_delete_pl_entries(1521-1523)create_payment_ledger_entry(1917-1933)
erpnext/accounts/doctype/advance_payment_ledger_entry/advance_payment_ledger_entry.py (1)
erpnext/accounts/utils.py (1)
update_voucher_outstanding(1936-1980)
⏰ 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). (1)
- GitHub Check: Patch Test
🔇 Additional comments (53)
erpnext/accounts/doctype/payment_ledger_entry/payment_ledger_entry.json (1)
200-201: LGTM: Minor formatting improvement.Adding the trailing newline character improves file formatting consistency and follows standard practices for text files.
erpnext/patches.txt (2)
365-365: LGTM: Added version date comment.The date comment
#2025-07-04helps track when this patch was added and provides better version control.
428-428: LGTM: New patch entry for advance payment ledger updates.The new patch
update_payment_ledger_entries_against_advance_doctypesis appropriately placed in the v15.0 section and aligns with the broader advance payment functionality enhancements.erpnext/selling/doctype/sales_order/sales_order.py (1)
460-460: LGTM: Consistent with existing ledger entry handling.Adding "Advance Payment Ledger Entry" to the ignored doctypes during cancellation follows the same pattern as other ledger entries and prevents advance payment records from blocking Sales Order cancellation.
erpnext/accounts/doctype/repost_payment_ledger/repost_payment_ledger.py (2)
11-11: LGTM: Import necessary for advance payment ledger cleanup.Adding
_delete_adv_pl_entriesimport enables the deletion of advance payment ledger entries during reposting, which is essential for data consistency.
19-19: LGTM: Consistent cleanup of advance payment ledger entries.Adding the call to
_delete_adv_pl_entriesensures advance payment ledger entries are properly cleaned up during reposting, following the same pattern as regular payment ledger entries on line 18.erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py (1)
172-175: LGTM: Consistent cleanup of advance payment ledger entries.Adding deletion of "Advance Payment Ledger Entry" records follows the same pattern as the existing "GL Entry" and "Payment Ledger Entry" deletions, ensuring comprehensive cleanup when
delete_cancelled_entriesis enabled.erpnext/accounts/doctype/payment_request/test_payment_request.py (1)
474-474: LGTM! Good clarification of expected behavior.The comment addition clarifies that outstanding amount will be zero for orders after full payment allocation, aligning Sales Order behavior with other document types. This improves code readability and helps future maintainers understand the expected behavior.
erpnext/accounts/doctype/advance_payment_ledger_entry/advance_payment_ledger_entry.json (4)
15-16: LGTM! Field order updated correctly.The field order has been properly updated to include the new "event" and "delinked" fields, maintaining proper structure.
73-79: LGTM! New "delinked" field properly configured.The new "delinked" checkbox field is well-designed:
- Read-only to prevent manual manipulation
- Default value of "0" is appropriate
- Will support tracking entries that have been delinked during cancellation operations
This aligns with the enhanced advance payment ledger entry handling described in the PR summary.
81-81: LGTM! Grid pagination setting added.The grid page length of 50 provides reasonable pagination for better UI performance when displaying ledger entries.
119-119: LGTM! Dynamic row format supports flexible UI.The dynamic row format setting enables flexible display behavior, which is consistent with similar updates mentioned in related DocTypes.
erpnext/accounts/doctype/journal_entry_account/journal_entry_account.py (1)
20-21: LGTM! Type annotations for advance voucher fields added correctly.The new optional fields
advance_voucher_noandadvance_voucher_typeare properly typed:
advance_voucher_noasDF.DynamicLink | Noneis appropriate for dynamic linkingadvance_voucher_typeasDF.Link | Nonecorrectly represents the voucher type reference- Both fields are optional (| None) which is suitable since they may not always be populated
These additions support the consistent tracking of advance payment references across the accounting system.
erpnext/accounts/general_ledger.py (1)
319-320: LGTM! Advance voucher fields correctly added to merge properties.Adding
"advance_voucher_type"and"advance_voucher_no"to the merge properties list is essential for maintaining data integrity:
- Ensures GL entries with different advance voucher references won't be incorrectly merged
- Maintains proper segregation of advance payment ledger entries
- Aligns with the broader advance payment tracking system introduced in this PR
The placement alongside other voucher-related fields (
"against_voucher","against_voucher_type","voucher_no") is logical and consistent.erpnext/accounts/doctype/payment_entry/test_payment_entry.py (2)
54-56: LGTM! Test updated to reflect new GL entry behavior.The test correctly expects the GL entry's
against_voucherfield to reference the payment entry (pe.name) instead of the sales order (so.name). This aligns with the new advance payment handling where GL entries reference the actual payment transaction rather than the underlying order document.
86-88: LGTM! Consistent test update for multi-currency scenario.Similar to the previous change, this test correctly expects the GL entry to reference the payment entry (
pe.name) rather than the sales order. The update maintains consistency across different test scenarios including multi-currency payments.erpnext/buying/doctype/purchase_order/purchase_order.py (1)
512-518: LGTM! Appropriate addition to ignored linked doctypes.Adding "Advance Payment Ledger Entry" to the ignored linked doctypes during purchase order cancellation is correct and necessary. This follows the established pattern of ignoring internal accounting records (GL Entry, Payment Ledger Entry) that are managed automatically by the system and shouldn't prevent cancellation of source documents.
erpnext/patches/v15_0/update_payment_ledger_entries_against_advance_doctypes.py (1)
8-25: LGTM! Well-structured database patch for advance payment data migration.The patch correctly updates Payment Ledger Entry records to ensure proper voucher references for advance payments. The logic is sound:
- Properly filters records using advance payment doctypes
- Updates
against_voucher_typeandagainst_voucher_noto reference original vouchers- Includes safety check with early return if no advance payment doctypes exist
- Uses Query Builder for safe database operations
The patch appears to be idempotent, which is good practice for database migrations.
erpnext/accounts/doctype/payment_entry_reference/payment_entry_reference.py (1)
19-20: LGTM! Appropriate type annotations for new advance voucher fields.The new type annotations correctly define:
advance_voucher_typeas a Link field to DocType (nullable)advance_voucher_noas a Dynamic Link field dependent on the voucher type (nullable)These align with the JSON schema changes and support the enhanced advance payment tracking functionality.
erpnext/accounts/doctype/payment_entry_reference/payment_entry_reference.json (1)
26-27: LGTM! Well-structured schema additions for advance voucher tracking.The JSON schema changes are properly implemented:
- Field definitions (lines 157-172): Both fields are correctly configured as read-only with appropriate field types
- Dynamic linking:
advance_voucher_noproperly depends onadvance_voucher_typefor dynamic linking- Field order: New fields are logically placed at the end of the field order
- Row format: Setting
row_formatto "Dynamic" is appropriate for tables with dynamic links- Column layout: Both fields use 2 columns for consistent display
These changes align perfectly with the Python type annotations and support the enhanced advance payment functionality.
Also applies to: 157-172, 184-184
erpnext/public/js/utils/unreconcile.js (3)
45-46: LGTM! Field reference update aligns with backend changes.The update from
voucher_type/voucher_notoreference_doctype/reference_nameis consistent with the broader refactoring to standardize voucher references across the accounting system.
57-58: LGTM! Consistent field reference update.The field name changes are applied consistently across different document type cases, maintaining the same mapping logic while using the updated field references.
72-82: LGTM! Dialog field definitions updated correctly.The field definitions properly update the internal field names while preserving user-facing labels. The Dynamic Link options field correctly references
reference_doctypeto maintain proper field linking functionality.erpnext/accounts/doctype/advance_payment_ledger_entry/advance_payment_ledger_entry.py (3)
4-4: LGTM! Necessary imports added for new functionality.The uncommented
frappeimport and newupdate_voucher_outstandingimport support the newon_updatemethod functionality.Also applies to: 7-8
24-24: LGTM! New delinked field added to type annotations.The addition of the
delinkedcheckbox field supports the enhanced advance payment ledger functionality for tracking delinked entries.
30-36: LGTM! Well-implementedon_updatemethod with proper guard conditions.The method correctly handles outstanding amount updates for Purchase/Sales Orders when advance payment ledger entries are modified. The guard conditions properly check for the voucher type, update flag, and exclude reverse depreciation entries. Passing
Nonefor account, party_type, and party parameters is appropriate since theupdate_voucher_outstandingfunction handles Purchase/Sales Orders by callingset_total_advance_paid()directly on the document.erpnext/accounts/doctype/journal_entry_account/journal_entry_account.json (3)
35-36: LGTM! Logical field ordering for advance voucher references.The new advance voucher fields are appropriately positioned in the field order after the existing reference fields, maintaining logical grouping.
268-283: LGTM! Well-structured field definitions for advance voucher tracking.The field definitions follow proper Frappe patterns:
advance_voucher_typeas a Link to DocType for referencing document typesadvance_voucher_noas a Dynamic Link with proper options reference- Appropriate read-only and no_copy properties for system-managed fields
288-288: LGTM! Appropriate metadata updates.The modified timestamp reflects the schema changes, and setting
row_formatto "Dynamic" is correct when adding Dynamic Link fields to the DocType.Also applies to: 295-295
erpnext/accounts/doctype/journal_entry/journal_entry.py (1)
1182-1243: LGTM! Well-structured refactoring with improved advance payment handling.The refactoring improves code organization and readability:
- Efficiency improvement: Getting
advance_doctypesonce at the method start rather than potentially multiple times- Better structure: Building the
rowdictionary separately makes the code more readable and maintainable- Correct advance payment logic: When
reference_typeis in advance doctypes, the logic properly:
- Sets
against_voucher_type/against_voucherto the current Journal Entry- Sets
advance_voucher_type/advance_voucher_noto the original reference document- This correctly tracks the relationship between the advance payment and the original order
The row dictionary comprehensively includes all required fields including the new advance voucher tracking fields.
erpnext/controllers/accounts_controller.py (4)
425-426: Good refactoring of deletion order.Moving
_remove_advance_payment_ledger_entries()after the deletion of GL/PLE/SLE entries ensures proper cleanup order. This prevents potential referential integrity issues during document deletion.
2215-2223: Well-structured query refactoring with proper filtering.The addition of the
delinked == 0filter correctly excludes delinked advance entries from the total calculation. The refactored query structure with chained.where()calls improves readability. The use ofAbs(Sum(amount))properly handles negative advance payment values as per the established pattern.
2226-2240: Verify the removal of advance payment validation logic.The method has been simplified by removing validation that compared advance paid amounts against order totals. While the simplification improves code clarity, please ensure that:
- This validation has been moved to an appropriate location (e.g., during payment entry creation)
- The business logic intentionally allows advances to exceed order amounts
- No data integrity issues will arise from this change
Could you confirm whether the advance payment validation is handled elsewhere in the codebase? If needed, I can help search for alternative validation locations.
2214-2241: Architectural improvement: Centralized advance payment handling.The removal of
make_advance_payment_ledger_for_journal,make_advance_payment_ledger_for_payment, andmake_advance_payment_ledger_entriesmethods aligns with the architectural shift to centralize advance payment ledger entry management. This promotes better code organization and maintainability by moving these operations to dedicated utility functions.erpnext/patches/v15_0/create_advance_payment_ledger_records.py (2)
1-25: LGTM! Well-structured imports and constants.The imports are appropriate for the refactored functionality, and the FIELDS constant provides a clean structure for bulk insert operations.
111-127: LGTM! Correctly implements advance payment amount storage pattern.The function correctly converts amounts to negative values (line 123) before storage, which aligns with the ERPNext pattern where advance payment amounts are stored as negative values in the ledger.
erpnext/accounts/doctype/payment_entry/payment_entry.py (5)
202-203: LGTM - Method reordering improves logical flow.Moving
update_payment_requests()beforemake_gl_entries()ensures payment request updates are processed before creating GL entries, which is a logical improvement in the execution order.
305-305: LGTM - Consistent method reordering.The reordering of
update_payment_requests(cancel=True)beforemake_gl_entries(cancel=1)maintains consistency with the on_submit method and ensures proper processing order during cancellation.
1438-1458: LGTM - Advance payment tracking enhancement properly implemented.The addition of
advance_voucher_typeandadvance_voucher_nofields along with the new conditional logic for advance payment doctypes correctly implements the enhanced advance payment tracking. The logic properly:
- Adds advance voucher tracking fields to GL entries
- Sets appropriate against_voucher references for advance payments
- Maintains backward compatibility for existing scenarios
1588-1589: LGTM - Consistent advance voucher field addition.Adding
advance_voucher_typeandadvance_voucher_nofields in the advance GL entries maintains consistency with the main GL entry creation logic.
1614-1615: LGTM - Consistent advance voucher field handling.The consistent addition of
advance_voucher_typeandadvance_voucher_nofields across all GL entry creation points ensures comprehensive advance payment tracking.erpnext/accounts/doctype/unreconcile_payment/unreconcile_payment.py (3)
46-51: Good refactoring to promote code reuse.The change to use
get_linked_payments_for_dochelper function improves maintainability and reduces code duplication.
59-73: LGTM! Clean separation of concerns.The removal of advance payment ledger creation logic aligns with the architectural changes to handle advances separately. The remaining logic focuses solely on unlinking references and updating outstanding amounts.
79-102: Correct implementation for counting both payment types.The function properly accumulates counts from both Payment Ledger Entry and Advance Payment Ledger Entry tables. The filters for advance payments correctly check for negative amounts (as per ERPNext convention for advance payments) and submitted events.
erpnext/accounts/utils.py (9)
485-521: Well-structured handling of advance GL entries.The function correctly differentiates between payment entries with separate party accounts (requiring advance GL entries) and regular entries (requiring ledger reposting). The collection of referenced rows ensures proper processing.
1526-1529: Clean implementation following established patterns.The function follows the same pattern as
_delete_pl_entriesand provides a clear, focused utility for deleting advance payment ledger entries.
677-680: Appropriate handling of advance voucher references.The logic correctly identifies advance payment doctypes and sets the corresponding voucher fields, enabling proper tracking of advance references.
708-731: Consistent advance voucher handling across payment references.The implementation correctly handles advance voucher fields for both new and existing payment entry references, maintaining consistency with the journal entry approach.
947-964: Comprehensive delinking of advance payment entries.The implementation properly handles delinking of advance payment ledger entries from both voucher and against_voucher perspectives, ensuring complete reference removal.
1850-1892: Proper dual-entry creation for advance payments.The implementation correctly creates both payment ledger and advance payment ledger entries when advance voucher information is present, maintaining proper accounting records.
1896-1915: Well-structured advance ledger entry creation.The helper function cleanly creates advance payment ledger entries with appropriate event type determination based on voucher relationships.
1936-1944: Appropriate special handling for order doctypes.The function correctly identifies Purchase and Sales Orders for special advance payment handling, delegating to their specific
set_total_advance_paidmethod.
1983-2028: Complete delinking implementation for both ledger types.The function appropriately handles delinking for both Payment Ledger Entry and Advance Payment Ledger Entry doctypes, with proper event tracking for advances.
| def execute(): | ||
| """ | ||
| Description: | ||
| Create Advance Payment Ledger Entry for all Payments made against Sales / Purchase Orders | ||
| """ | ||
| frappe.db.truncate(DOCTYPE) | ||
| advance_doctpyes = get_advance_payment_doctypes() | ||
| make_advance_ledger_entries_for_payment_entries(advance_doctpyes) | ||
| make_advance_ledger_entries_for_journal_entries(advance_doctpyes) |
There was a problem hiding this comment.
Critical: Add safeguards for destructive truncate operation.
The frappe.db.truncate(DOCTYPE) operation will delete ALL advance payment ledger entries without any backup or validation. If the subsequent operations fail, this could leave the system in an inconsistent state.
Consider adding safeguards:
def execute():
"""
Description:
Create Advance Payment Ledger Entry for all Payments made against Sales / Purchase Orders
"""
+ # Validate that we can recreate the data before truncating
+ advance_doctpyes = get_advance_payment_doctypes()
+ if not advance_doctpyes:
+ frappe.throw("No advance payment doctypes found. Aborting patch execution.")
+
frappe.db.truncate(DOCTYPE)
- advance_doctpyes = get_advance_payment_doctypes()
+
+ try:
make_advance_ledger_entries_for_payment_entries(advance_doctpyes)
make_advance_ledger_entries_for_journal_entries(advance_doctpyes)
+ frappe.db.commit()
+ except Exception:
+ frappe.db.rollback()
+ raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def execute(): | |
| """ | |
| Description: | |
| Create Advance Payment Ledger Entry for all Payments made against Sales / Purchase Orders | |
| """ | |
| frappe.db.truncate(DOCTYPE) | |
| advance_doctpyes = get_advance_payment_doctypes() | |
| make_advance_ledger_entries_for_payment_entries(advance_doctpyes) | |
| make_advance_ledger_entries_for_journal_entries(advance_doctpyes) | |
| def execute(): | |
| """ | |
| Description: | |
| Create Advance Payment Ledger Entry for all Payments made against Sales / Purchase Orders | |
| """ | |
| # Validate that we can recreate the data before truncating | |
| advance_doctpyes = get_advance_payment_doctypes() | |
| if not advance_doctpyes: | |
| frappe.throw("No advance payment doctypes found. Aborting patch execution.") | |
| frappe.db.truncate(DOCTYPE) | |
| try: | |
| make_advance_ledger_entries_for_payment_entries(advance_doctpyes) | |
| make_advance_ledger_entries_for_journal_entries(advance_doctpyes) | |
| frappe.db.commit() | |
| except Exception: | |
| frappe.db.rollback() | |
| raise |
🤖 Prompt for AI Agents
In erpnext/patches/v15_0/create_advance_payment_ledger_records.py around lines
28 to 36, the direct call to frappe.db.truncate(DOCTYPE) is risky as it deletes
all records without backup or validation. To fix this, implement a safeguard by
first backing up the existing data or wrapping the truncate and subsequent
operations in a transaction to allow rollback on failure. Alternatively, add a
confirmation step or a conditional check before truncating to prevent accidental
data loss.
| def bulk_insert_advance_entries(entries): | ||
| details = [] | ||
| user = frappe.session.user | ||
| now = now_datetime() | ||
| for entry in entries: | ||
| if entry.amount < 0: | ||
| continue | ||
| details.append(get_values(user, now, entry)) | ||
|
|
||
| def execute(): | ||
| """ | ||
| Description: | ||
| Create Advance Payment Ledger Entry for all Payments made against Sales / Purchase Orders | ||
| """ | ||
| frappe.db.truncate("Advance Payment Ledger Entry") | ||
| payment_entries = get_payments_with_so_po_reference() | ||
| make_advance_ledger_entries(payment_entries) | ||
| frappe.db.bulk_insert(DOCTYPE, fields=FIELDS, values=details) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and logging for bulk operations.
The bulk insert logic is sound, but could benefit from better error handling and visibility into the operation.
def bulk_insert_advance_entries(entries):
details = []
user = frappe.session.user
now = now_datetime()
for entry in entries:
if entry.amount < 0:
continue
details.append(get_values(user, now, entry))
+ if not details:
+ frappe.logger().info(f"No valid entries to insert for {DOCTYPE}")
+ return
+
+ try:
frappe.db.bulk_insert(DOCTYPE, fields=FIELDS, values=details)
+ frappe.logger().info(f"Successfully inserted {len(details)} {DOCTYPE} records")
+ except Exception as e:
+ frappe.logger().error(f"Failed to bulk insert {DOCTYPE} records: {str(e)}")
+ raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def bulk_insert_advance_entries(entries): | |
| details = [] | |
| user = frappe.session.user | |
| now = now_datetime() | |
| for entry in entries: | |
| if entry.amount < 0: | |
| continue | |
| details.append(get_values(user, now, entry)) | |
| def execute(): | |
| """ | |
| Description: | |
| Create Advance Payment Ledger Entry for all Payments made against Sales / Purchase Orders | |
| """ | |
| frappe.db.truncate("Advance Payment Ledger Entry") | |
| payment_entries = get_payments_with_so_po_reference() | |
| make_advance_ledger_entries(payment_entries) | |
| frappe.db.bulk_insert(DOCTYPE, fields=FIELDS, values=details) | |
| def bulk_insert_advance_entries(entries): | |
| details = [] | |
| user = frappe.session.user | |
| now = now_datetime() | |
| for entry in entries: | |
| if entry.amount < 0: | |
| continue | |
| details.append(get_values(user, now, entry)) | |
| if not details: | |
| frappe.logger().info(f"No valid entries to insert for {DOCTYPE}") | |
| return | |
| try: | |
| frappe.db.bulk_insert(DOCTYPE, fields=FIELDS, values=details) | |
| frappe.logger().info(f"Successfully inserted {len(details)} {DOCTYPE} records") | |
| except Exception as e: | |
| frappe.logger().error(f"Failed to bulk insert {DOCTYPE} records: {str(e)}") | |
| raise |
🤖 Prompt for AI Agents
In erpnext/patches/v15_0/create_advance_payment_ledger_records.py around lines
99 to 108, the bulk_insert_advance_entries function lacks error handling and
logging for the bulk insert operation. Wrap the bulk insert call in a try-except
block to catch any exceptions, log an error message with relevant details if an
exception occurs, and optionally re-raise or handle the error gracefully to
improve visibility and robustness of the operation.
ljain112
force-pushed
the
fix-advance-payment
branch
from
8501d46 to
8dda5e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
erpnext/accounts/doctype/payment_reconciliation/test_payment_reconciliation.py (1)
2211-2342: Comprehensive test for cross-fiscal-year advance payment reconciliation.The test method is well-structured and covers an important edge case of advance payment reconciliation across closed fiscal periods. The test follows a logical progression through multiple phases with appropriate setup and cleanup.
A few observations:
- Good test isolation: The method properly saves and restores company settings
- Comprehensive scenario coverage: Tests both "Oldest Of Invoice Or Advance" and "Reconciliation Date" settings
- Proper assertions: Validates payment references, amounts, and reconciliation effect dates
However, consider these improvements:
Potential improvements:
# Consider extracting helper methods for common operations + def _create_advance_payment(self, amount, posting_date, supplier): + """Helper method to create advance payment""" + pe = self.create_payment_entry(amount=amount, posting_date=posting_date) + pe.party_type = "Supplier" + pe.party = supplier + pe.payment_type = "Pay" + pe.paid_from = self.cash + pe.paid_to = self.advance_payable_account + return pe.save().submit() + + def _create_supplier_invoice(self, qty, rate, posting_date, supplier): + """Helper method to create supplier invoice""" + pi = self.create_purchase_invoice(qty=qty, rate=rate, do_not_submit=True) + pi.posting_date = posting_date + pi.set_posting_time = 1 + pi.supplier = supplier + pi.credit_to = self.creditors + return pi.save().submit() # Then use these helpers to reduce code duplication: - pe = self.create_payment_entry(amount=1000, posting_date=prev_fy_start_date) - pe.party_type = "Supplier" - pe.party = self.supplier - pe.payment_type = "Pay" - pe.paid_from = self.cash - pe.paid_to = self.advance_payable_account - pe.save().submit() + pe = self._create_advance_payment(1000, prev_fy_start_date, self.supplier)Documentation enhancement:
Consider adding inline comments to explain the business logic being tested at each phase, especially around the reconciliation effect date assertions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/accounts/doctype/payment_reconciliation/test_payment_reconciliation.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ljain112
PR: ljain112/erpnext#20
File: erpnext/controllers/accounts_controller.py:2203-2217
Timestamp: 2025-07-26T07:46:45.956Z
Learning: In ERPNext Payment Ledger Entry, advance payment amounts are stored as negative values, but when calculating total advance paid for display purposes, they should be converted to positive values using Abs(Sum(amount)) to show meaningful totals to users.
⏰ 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). (1)
- GitHub Check: Patch Test
🔇 Additional comments (1)
erpnext/accounts/doctype/payment_reconciliation/test_payment_reconciliation.py (1)
2338-2339: Validate reconciliation effect dates correctly.The test correctly verifies that reconciliation effect dates match the expected posting dates based on company settings. This is crucial for ensuring proper accounting period allocation.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Tests