fix: Incorrect filters in Voucher Child Table of Land Cost Voucher DocType #49500
Conversation
anwarpatelnoori
requested review from
rohitwaghchaure and
mihir-kandoi
as code owners
📝 WalkthroughWalkthroughThe change refactors receipt_document query logic in erpnext/stock/doctype/landed_cost_voucher/landed_cost_voucher.js. It removes the LandedCostVoucher.setup() method and introduces dynamic filter construction in setup_queries for the receipt_document field. Filters now always include docstatus=1 and company for the selected receipt_document_type. Additional filters are applied per type: Purchase Invoice requires update_stock=1; Stock Entry restricts purpose to Manufacture or Repack. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
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. 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 (3)
erpnext/stock/doctype/landed_cost_voucher/landed_cost_voucher.js (3)
137-143: Use numeric literals for Int/Check fields (docstatus,update_stock)Avoid string comparisons against integer fields. Use 1 instead of "1".
- [d.receipt_document_type, "docstatus", "=", "1"], + [d.receipt_document_type, "docstatus", "=", 1], ... - filters.push(["Purchase Invoice", "update_stock", "=", "1"]); + filters.push(["Purchase Invoice", "update_stock", "=", 1]);
141-145: Consistency: use strict equality for both branchesMinor consistency/readability tweak.
- if (d.receipt_document_type == "Purchase Invoice") { + if (d.receipt_document_type === "Purchase Invoice") {
135-140: Guard when type/company is not set to avoid ambiguous queriesWhen the row is new or company/type isn’t chosen yet, the current code builds filters with undefined doctype. Return an empty filter set early.
- var d = locals[cdt][cdn]; - var filters = [ + const d = locals[cdt][cdn]; + if (!d.receipt_document_type || !frm.doc.company) { + return { filters: [] }; + } + const filters = [
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/stock/doctype/landed_cost_voucher/landed_cost_voucher.js(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). (1)
- GitHub Check: Summary
🔇 Additional comments (2)
erpnext/stock/doctype/landed_cost_voucher/landed_cost_voucher.js (2)
136-149: LGTM: Filters now correctly scope by company and exclude cancelled docsThis aligns with the bugfix objective: only submitted (docstatus=1) docs from the selected company are listed; PIs additionally need update_stock=1.
143-145: I’ve started extracting the validpurposeoptions for Stock Entry to verify if “Material Receipt” should be included.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
erpnext/stock/doctype/landed_cost_voucher/landed_cost_voucher.js (4)
135-139: Guard when type/company aren’t selected to avoid malformed queries.If the row opens before selecting receipt_document_type or company, the first two filters will use an undefined doctype/value. Add a short-circuit.
frm.set_query("receipt_document", "purchase_receipts", (doc, cdt, cdn) => { var d = locals[cdt][cdn]; + if (!d.receipt_document_type || !frm.doc.company) { + // No results until prerequisites are set + return { filters: [["name", "=", ""]] }; + } var filters = [ [d.receipt_document_type, "docstatus", "=", 1], [d.receipt_document_type, "company", "=", frm.doc.company], ];
141-145: Consider excluding returns for PR/PI.Typically landed costs shouldn’t be applied to return documents. If aligned with product behavior, add is_return=0 for PR/PI.
if (d.receipt_document_type === "Purchase Invoice") { filters.push(["Purchase Invoice", "update_stock", "=", 1]); + filters.push(["Purchase Invoice", "is_return", "=", 0]); } else if (d.receipt_document_type === "Stock Entry") { filters.push(["Stock Entry", "purpose", "in", ["Manufacture", "Repack"]]); + } else if (d.receipt_document_type === "Purchase Receipt") { + filters.push(["Purchase Receipt", "is_return", "=", 0]); }
136-139: Nit: prefer const and shorthand return.Minor readability/consistency tidy-up if ESLint/style permits.
- var filters = [ + const filters = [ [d.receipt_document_type, "docstatus", "=", 1], [d.receipt_document_type, "company", "=", frm.doc.company], ]; ... - return { - filters: filters, - }; + return { filters };Also applies to: 146-148
146-148: Defense-in-depth: validate on the server.Even with client-side filters, ensure the server methods (e.g., get_receipt_document_details, get_items_from_purchase_receipts) re-validate that docstatus=1 and company matches the LCV.
Happy to draft the backend checks if you want.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/stock/doctype/landed_cost_voucher/landed_cost_voucher.js(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). (1)
- GitHub Check: Summary
🔇 Additional comments (2)
erpnext/stock/doctype/landed_cost_voucher/landed_cost_voucher.js (2)
136-148: Good fix: enforce submitted-only and company-scoped results; PI/SE-specific constraints look right.This should stop cancelled docs (docstatus=2) and cross-company records from appearing; PI update_stock=1 is correctly enforced.
141-145: Double-check Stock Entry purposes.Confirm that Landed Cost Voucher is intended to include Stock Entries only for Manufacture/Repack in v15; some setups use Material Receipt for freight capitalization.
Closes #49499