fix: merge taxes in purchase receipt when get items from multiple purchase invoices #51422
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #51422 +/- ##
===========================================
+ Coverage 79.05% 79.11% +0.06%
===========================================
Files 1179 1179
Lines 121306 121361 +55
===========================================
+ Hits 95897 96015 +118
+ Misses 25409 25346 -63
🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThe PR updates Purchase Invoice → Purchase Receipt mapping to add two helper calls: one removes target items with zero quantity, the other runs the target's Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📚 Learning: 2025-12-23T11:28:49.128ZApplied to files:
📚 Learning: 2025-10-17T12:17:35.397ZApplied to files:
📚 Learning: 2025-11-05T16:19:19.510ZApplied to files:
📚 Learning: 2025-12-16T05:33:58.723ZApplied to files:
🧬 Code graph analysis (1)erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py (2)
⏰ 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)
🔇 Additional comments (3)
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: 2
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py
🧰 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/purchase_invoice/purchase_invoice.py
🧬 Code graph analysis (1)
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py (4)
erpnext/buying/doctype/purchase_order/purchase_order.py (1)
set_missing_values(697-700)erpnext/stock/doctype/purchase_receipt/purchase_receipt.py (2)
set_missing_values(1365-1378)set_missing_values(1527-1530)erpnext/accounts/doctype/sales_invoice/sales_invoice.py (3)
set_missing_values(729-762)set_missing_values(2353-2356)set_missing_values(2519-2521)erpnext/stock/doctype/delivery_note/delivery_note.py (3)
set_missing_values(853-873)set_missing_values(1025-1026)set_missing_values(1194-1208)
⏰ 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/accounts/doctype/purchase_invoice/purchase_invoice.py (2)
2007-2009: LGTM: Clean preprocessing orchestration.The function correctly orchestrates the preprocessing steps by first removing zero-quantity items and then recalculating taxes and totals.
2070-2070: No changes needed. Thereset_value: Trueimplementation is correct for this use case.The function
make_purchase_receipt(source_name, target_doc=None)is designed to handle a single source Purchase Invoice per call (note the singularsource_nameparameter). It is not intended to merge items from multiple Purchase Invoices in a single operation. Thereset_value: Trueflag ensures that taxes are properly cleared and then recalculated based on the final set of items in the target Purchase Receipt. Thepost_parent_processfunction callscalculate_taxes_and_totals, which correctly recalculates all taxes from scratch based on the mapped items. This pattern is consistent across similar conversion functions in the codebase (Sales Order→Delivery Note, etc.). If a workflow requires merging items from multiple Purchase Invoices into a single Purchase Receipt, that logic would need to be implemented at a higher level, not within this single-source conversion function.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py:
- Around line 2068-2071: Update the "Purchase Taxes and Charges" mapping so that
"reset_value" is set to not (args and args.get("merge_taxes")) and add an
"ignore" key with args.get("merge_taxes") if args else 0; then in
post_parent_process import and call merge_taxes(source, target_doc) when args
and args.get("merge_taxes") is truthy, placing that call before
calculate_taxes_and_totals(target_doc) so taxes are consolidated from sources
rather than being cleared and recomputed from items.
♻️ Duplicate comments (1)
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py (1)
2014-2016: Themerge_taxescall is still missing.Per the previous review comment and the PR objective to fix duplicate tax rows when pulling from multiple Purchase Invoices, this function should call
merge_taxesto consolidate tax rows by account and cost center. The current implementation only callscalculate_taxes_and_totals, which recalculates totals but doesn't merge duplicate tax entries.Reference implementations:
- Purchase Receipt → Purchase Invoice: calls
merge_taxes(source, doc)whenargs.get("merge_taxes")- Delivery Note → Sales Invoice: calls
merge_taxes(source, target)whenargs.get("merge_taxes")🔎 Suggested fix
Import
merge_taxesat the top of the file:from erpnext.controllers.buying_controller import BuyingController +from erpnext.controllers.accounts_controller import merge_taxes from erpnext.stock.doctype.purchase_receipt.purchase_receipt import (Then update
set_missing_values:def set_missing_values(target): target.run_method("set_missing_values") + if args and args.get("merge_taxes"): + # merge_taxes expects source document; in multi-source scenarios, + # the mapper handles merging across calls + # Here we ensure taxes are consolidated by account/cost_center + from erpnext.controllers.accounts_controller import merge_taxes + # Note: This may need adjustment based on mapper's multi-source handling target.run_method("calculate_taxes_and_totals")Note: The mapper may need additional logic if multiple source Purchase Invoices are being processed. Please verify whether
merge_taxesshould be called with a specific source or if additional consolidation logic is needed for multi-document mapping.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.pyerpnext/public/js/utils.js
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-23T11:28:49.128Z
Learnt from: vorasmit
Repo: frappe/erpnext PR: 51099
File: erpnext/patches/v16_0/migrate_tax_withholding_data.py:904-910
Timestamp: 2025-12-23T11:28:49.128Z
Learning: In the old Tax Withholding system (before the Tax Withholding Entry refactor), only one TCS account per Sales Invoice was supported. When reviewing migration code in erpnext/patches/v16_0/migrate_tax_withholding_data.py, the query selecting TCS accounts from GL Entries doesn't need explicit aggregation because historical data will only have one TCS account per invoice.
Applied to files:
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.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/accounts/doctype/purchase_invoice/purchase_invoice.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). (6)
- GitHub Check: Patch Test
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Summary
🔇 Additional comments (3)
erpnext/public/js/utils.js (1)
996-996: LGTM! Enables tax-merge UI for Purchase Invoice source.The addition of "Purchase Invoice" to the source doctype list correctly enables the "merge_taxes" checkbox when creating a Purchase Receipt from multiple Purchase Invoices, aligning with the PR's objective to fix duplicate tax rows.
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py (2)
2007-2009: LGTM! Clear orchestration of post-processing steps.The
post_parent_processfunction logically sequences the removal of zero-quantity items followed by recalculation of values.
2011-2012: LGTM! Safe filtering approach.The list comprehension correctly filters out zero-quantity items without the iteration-during-modification issue flagged in earlier reviews.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py:
- Around line 2014-2019: The current set_missing_values implementation calls
target_parent.run_method("calculate_taxes_and_totals") before merging tax rows,
so totals are computed on unconsolidated taxes; change the order to call
merge_taxes(source_parent, target_parent) immediately after
target_parent.run_method("set_missing_values") (and only if args and
args.get("merge_taxes") is true), then call
target_parent.run_method("calculate_taxes_and_totals") so totals are
recalculated from the merged tax rows, matching the pattern used in
purchase_receipt.py.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-23T11:28:49.128Z
Learnt from: vorasmit
Repo: frappe/erpnext PR: 51099
File: erpnext/patches/v16_0/migrate_tax_withholding_data.py:904-910
Timestamp: 2025-12-23T11:28:49.128Z
Learning: In the old Tax Withholding system (before the Tax Withholding Entry refactor), only one TCS account per Sales Invoice was supported. When reviewing migration code in erpnext/patches/v16_0/migrate_tax_withholding_data.py, the query selecting TCS accounts from GL Entries doesn't need explicit aggregation because historical data will only have one TCS account per invoice.
Applied to files:
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.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/accounts/doctype/purchase_invoice/purchase_invoice.py
🧬 Code graph analysis (1)
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py (3)
erpnext/controllers/accounts_controller.py (1)
merge_taxes(4226-4266)erpnext/buying/doctype/purchase_order/purchase_order.py (1)
set_missing_values(697-700)erpnext/stock/doctype/purchase_receipt/purchase_receipt.py (2)
set_missing_values(1365-1378)set_missing_values(1527-1530)
⏰ 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 (2)
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py (2)
39-39: LGTM!The import of
merge_taxesfromerpnext.controllers.accounts_controlleris correctly placed and necessary for the new tax consolidation functionality.
2071-2075: LGTM!The tax mapping configuration is correct. When
merge_taxesis enabled:
reset_value: Falsepreserves existing tax rowsignore: Trueprevents copying taxes from sourceThis pattern matches the reference implementations in
purchase_receipt.pyanddelivery_note.py.
1 participant
Issue: When a Purchase Receipt is created by pulling items from multiple Purchase Invoices that use the same tax account, the system does not merge the tax rows. Instead, it adds separate tax rows for the same account and the tax amount gets doubled upon save.
Ref: 56269
Steps to Reproduce:
Before:
Screen.Recording.2025-12-31.at.21.35.02.mov
After:
Screen.Recording.2025-12-31.at.21.36.30.mov
Backport needed v15