fix(payment-ledger-entry): add advance doctype in against voucher no #49451
Conversation
github-actions
bot
added
accounts
needs-tests
📝 WalkthroughWalkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
erpnext/accounts/report/accounts_receivable/accounts_receivable.js (1)
43-54: Autocomplete options are populated asynchronously but returned synchronously → empty options at runtimeget_party_type_options() returns [] immediately and fills later via Promise; the filter won’t reliably update. Populate options onload and refresh the filter, and use correct or_filters syntax.
Apply the following diff:
@@ { fieldname: "party_type", label: __("Party Type"), - fieldtype: "Autocomplete", - options: get_party_type_options(), + fieldtype: "Autocomplete", + options: [], on_change: function () { frappe.query_report.set_filter_value("party", ""); frappe.query_report.toggle_filter_display( "customer_group", frappe.query_report.get_filter_value("party_type") !== "Customer" ); }, }, @@ - onload: function (report) { + onload: function (report) { report.page.add_inner_button(__("Accounts Receivable Summary"), function () { var filters = report.get_values(); frappe.set_route("query-report", "Accounts Receivable Summary", { company: filters.company }); }); + // Populate Party Types: include "Employee" OR account_type "Receivable" + populate_party_type_options(report); }, @@ -function get_party_type_options() { - let options = []; - frappe.db - .get_list("Party Type", { - or_filters: { name: "Employee", account_type: "Receivable" }, - fields: ["name"], - }) - .then((res) => { - res.forEach((party_type) => { - options.push(party_type.name); - }); - }); - return options; -} +function populate_party_type_options(report) { + frappe.db + .get_list("Party Type", { + filters: [["account_type", "=", "Receivable"]], + or_filters: [["name", "=", "Employee"]], + fields: ["name"], + limit: 0, + }) + .then((res) => { + const options = res.map((r) => r.name); + const f = report.get_filter("party_type"); + f.df.options = options; + f.refresh(); + // Sensible default + if (!f.get_value() && options.includes("Customer")) { + frappe.query_report.set_filter_value("party_type", "Customer"); + } + }); +}This guarantees options load correctly and keeps the intent of including Employee even if its Party Type isn’t Receivable.
Also applies to: 198-204, 208-221
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
erpnext/accounts/report/accounts_receivable/accounts_receivable.js(2 hunks)erpnext/accounts/utils.py(1 hunks)
⏰ 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: Summary
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Patch Test
🔇 Additional comments (2)
erpnext/accounts/utils.py (1)
1850-1859: Confirm Advance PLE event semantics after remap (Submit vs Adjustment)After remapping against_voucher_type/no away from the self-voucher, get_advance_ledger_entry() will classify the Advance PLE event as "Adjustment" (not "Submit"). Please confirm this is intended for Employee Advance flows; otherwise, compute event based on the original GLE condition (gle.against_voucher == gle.voucher_no) or pass an explicit event flag from caller.
I can draft a safe change that derives event from GLE-only fields if you want to keep "Submit" for initial advance booking and "Adjustment" only for later applications.
Also applies to: 1902-1919
erpnext/accounts/report/accounts_receivable/accounts_receivable.js (1)
183-187: New “Handle Employee Advances” filter looks fine—ensure it’s actually used server-sideUI addition is clear. Please verify the report script consumes handle_employee_advances and toggles logic accordingly; otherwise it will be a no-op for users.
If it isn’t wired, I can propose the minimal server change to gate the new behavior behind this flag.
| .get_list("Party Type", { | ||
| or_filters: { name: "Employee", account_type: "Receivable" }, | ||
| fields: ["name"], | ||
| }) |
There was a problem hiding this comment.
or_filters shape is invalid here; use array-of-conditions, not an object
frappe.db.get_list expects or_filters as an array (e.g., [["name","=","Employee"], ["account_type","=","Receivable"]]). Passing an object will not OR the clauses and may be ignored.
Apply this fix as part of the broader patch below.
🤖 Prompt for AI Agents
In erpnext/accounts/report/accounts_receivable/accounts_receivable.js around
lines 211 to 214, the call to frappe.db.get_list uses or_filters as an object
which is invalid; change or_filters to an array-of-conditions like
[["name","=","Employee"], ["account_type","=","Receivable"]]. Update the
get_list invocation to pass or_filters: [ ["name","=","Employee"],
["account_type","=","Receivable"] ] so the two clauses are properly ORed and the
query returns expected rows.
| against_voucher_type = gle.against_voucher_type | ||
| against_voucher_no = gle.against_voucher | ||
|
|
||
| if gle.against_voucher: | ||
| if gle.against_voucher == gle.voucher_no and gle.advance_voucher_no: | ||
| against_voucher_type = gle.advance_voucher_type | ||
| against_voucher_no = gle.advance_voucher_no | ||
| else: | ||
| against_voucher_type = gle.voucher_type | ||
| against_voucher_no = gle.voucher_no |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against partial advance fields to avoid None against_voucher_type in PLE
Good call mapping self-referencing GLEs to their advance voucher. Add a tight guard to only remap when both advance_voucher_type and advance_voucher_no exist; otherwise we risk writing a PLE row with a null against_voucher_type which can break downstream filters and reconciliation.
Apply this minimal patch:
- if gle.against_voucher:
- if gle.against_voucher == gle.voucher_no and gle.advance_voucher_no:
- against_voucher_type = gle.advance_voucher_type
- against_voucher_no = gle.advance_voucher_no
+ if gle.against_voucher:
+ if (
+ gle.against_voucher == gle.voucher_no
+ and gle.advance_voucher_type
+ and gle.advance_voucher_no
+ ):
+ against_voucher_type = gle.advance_voucher_type
+ against_voucher_no = gle.advance_voucher_no📝 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.
| against_voucher_type = gle.against_voucher_type | |
| against_voucher_no = gle.against_voucher | |
| if gle.against_voucher: | |
| if gle.against_voucher == gle.voucher_no and gle.advance_voucher_no: | |
| against_voucher_type = gle.advance_voucher_type | |
| against_voucher_no = gle.advance_voucher_no | |
| else: | |
| against_voucher_type = gle.voucher_type | |
| against_voucher_no = gle.voucher_no | |
| against_voucher_type = gle.against_voucher_type | |
| against_voucher_no = gle.against_voucher | |
| if gle.against_voucher: | |
| if ( | |
| gle.against_voucher == gle.voucher_no | |
| and gle.advance_voucher_type | |
| and gle.advance_voucher_no | |
| ): | |
| against_voucher_type = gle.advance_voucher_type | |
| against_voucher_no = gle.advance_voucher_no | |
| else: | |
| against_voucher_type = gle.voucher_type | |
| against_voucher_no = gle.voucher_no |
🤖 Prompt for AI Agents
In erpnext/accounts/utils.py around lines 1850 to 1859, the current logic remaps
against_voucher_type/against_voucher_no when gle.against_voucher equals
gle.voucher_no and gle.advance_voucher_no exists, which can leave
against_voucher_type as None; update the guard so the remap only occurs when
both gle.advance_voucher_type and gle.advance_voucher_no are truthy (i.e., check
both fields before assigning advance_voucher_type and advance_voucher_no),
leaving the else branch unchanged.
Issue:
The Accounts Receivable report shows the Expense Claim and Payment Entries made against the Employee Advance.
ref: 47589
Before:
employee-advance-accounts-receivable-issue.mp4
After:
employee-advance-accounts-receivable-fix.mp4
Backport needed for v15