fix(buying): decouple accepted and rejected quantities in purchase returns#51505
fix(buying): decouple accepted and rejected quantities in purchase returns#51505Aakash1o1 wants to merge 2 commits intofrappe:developfrom
Conversation
Aakash1o1
requested review from
mihir-kandoi,
rohitwaghchaure and
ruthra-kumar
as code owners
📝 WalkthroughWalkthroughAdds handling to distinguish rejected vs accepted returned quantities when creating purchase returns. Updates sales_and_purchase_return.py to select returned quantity keys dynamically (rejected_qty_returned / rejected_stock_qty_returned vs standard fields), adjusts reference/current quantity calculations with conversion factors, and enhances get_already_returned_items to aggregate rejected and accepted returns only when the doctype supports the return_qty_from_rejected_warehouse flag. Adds integration tests: one for Purchase Invoice fallback when the rejected column is missing, and one for Purchase Receipt ensuring rejected returns do not block accepted returns. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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)
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: 1
Fix all issues with AI Agents 🤖
In @erpnext/controllers/sales_and_purchase_return.py:
- Around line 218-224: The `stock_qty` branch inside the else is unreachable
because `column == "stock_qty" and not
args.get("return_qty_from_rejected_warehouse")` is already handled earlier;
remove the dead code (the block that multiplies reference_qty/current_stock_qty
by ref.get("conversion_factor", 1.0)/args.get("conversion_factor", 1.0)) or, if
conversion is required for the non-rejected `stock_qty` path, move that
conversion into the earlier `if column == "stock_qty" and not
args.get("return_qty_from_rejected_warehouse")` branch so that reference_qty and
current_stock_qty are multiplied by ref.get("conversion_factor", 1.0) and
args.get("conversion_factor", 1.0) respectively; update only the logic using the
variables column, reference_qty, current_stock_qty, ref.get("conversion_factor")
and args.get("conversion_factor") and remove the unreachable block referencing
return_qty_from_rejected_warehouse.
🧹 Nitpick comments (1)
erpnext/accounts/doctype/purchase_invoice/test_purchase_invoice.py (1)
2968-2985: Test name/docstring claim a “missing column” scenario that isn’t currently exercisedThis test does validate that a normal Purchase Invoice return goes through without crashing, but it never simulates the case where
Purchase Invoice Itemlacks thereturn_qty_from_rejected_warehousecolumn, which is what the docstring and PR description mention. As a result, the new fallback branch inget_already_returned_items(whenfrappe.db.has_column(...)is false) is still untested.To actually cover that code path, consider temporarily monkey‑patching
frappe.db.has_columninside this test so that, for"Purchase Invoice Item"and"return_qty_from_rejected_warehouse", it returnsFalse, and then restoring it in afinallyblock. That will exercise the compatibility logic without touching the real schema.You might also optionally assert against
pi_return(the return document) or just drop the status assertion if the main goal is strictly “does not crash”, but that’s secondary to the coverage gap above.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
erpnext/accounts/doctype/purchase_invoice/test_purchase_invoice.pyerpnext/controllers/sales_and_purchase_return.pyerpnext/stock/doctype/purchase_receipt/test_purchase_receipt.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/test_purchase_invoice.pyerpnext/controllers/sales_and_purchase_return.pyerpnext/stock/doctype/purchase_receipt/test_purchase_receipt.py
🧬 Code graph analysis (2)
erpnext/accounts/doctype/purchase_invoice/test_purchase_invoice.py (1)
erpnext/controllers/sales_and_purchase_return.py (1)
make_return_doc(394-707)
erpnext/stock/doctype/purchase_receipt/test_purchase_receipt.py (1)
erpnext/controllers/sales_and_purchase_return.py (1)
make_return_doc(394-707)
⏰ 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 (4)
erpnext/stock/doctype/purchase_receipt/test_purchase_receipt.py (1)
4723-4763: Good regression coverage for decoupling accepted vs rejected return quantitiesThis test cleanly reproduces the mixed accepted/rejected scenario and verifies that a prior rejected‑qty return no longer reduces the allowed accepted return quantity. The use of
return_against_rejected_qty=Trueand a subsequent standard return hits exactly the paths changed in the controller. Looks solid.erpnext/controllers/sales_and_purchase_return.py (3)
193-216: Logic for separating accepted and rejected return buckets looks correct.The bucket selection properly distinguishes between accepted and rejected quantities based on the
return_qty_from_rejected_warehouseflag. The conversion factor is correctly applied when convertingrejected_qtyto stock units.Minor: Lines 217 and 221 have trailing whitespace.
287-305: Good defensive check for backward compatibility.The
frappe.db.has_columncheck ensures this code works with doctypes that don't have thereturn_qty_from_rejected_warehousefield (like older Purchase Invoice implementations). The conditional SQL aggregation correctly splits quantities into accepted and rejected buckets, treating NULL values as accepted returns for legacy data compatibility.
340-341: LGTM!The new fields are correctly added to the returned items dictionary, enabling the validation logic to check accepted and rejected returns separately.
|
closing in favor of #51514 |
4 participants
Problem
Currently, the system validation for Purchase Returns aggregates all previously returned items into a single bucket, regardless of whether they were returned from the Accepted warehouse or the Rejected warehouse.
This leads to a validation error in mixed scenarios:
The Fix
This PR decouples the return logic so that Accepted and Rejected returns are tracked and validated separately.
SQL Aggregation (
get_already_returned_items):qty(Accepted) andrejected_qty_returned(Rejected).frappe.db.has_columncheck to ensure backward compatibility with Doctypes that lack the rejected warehouse field (e.g., Purchase Invoice).IS NULLhandling to ensure legacy data is treated as "Accepted" returns.Validation Logic (
validate_quantity):Original Accepted Qty - Previous Accepted Returns.Original Rejected Qty - Previous Rejected Returns.stock_qtyis validated against Stock UOM values, whileqtyis validated against Item UOM values, preventing unit mismatch errors.Tests Added
test_return_rejected_does_not_block_accepted_return(Purchase Receipt): Verifies that returning rejected items does not reduce the returnable balance of accepted items.test_purchase_return_fallback_for_missing_column(Purchase Invoice): Verifies that standard returns continue to work on Doctypes where the rejected warehouse column does not exist (crash prevention).