feat: Allowing closing individual items in sales order #51485
Conversation
nishkagosalia
force-pushed
the
gh-39030
branch
from
04186e4 to
ddb34d1
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #51485 +/- ##
===========================================
+ Coverage 79.24% 79.25% +0.01%
===========================================
Files 1168 1168
Lines 122566 122688 +122
===========================================
+ Hits 97126 97238 +112
- Misses 25440 25450 +10
🚀 New features to boost your workflow:
|
|
nishkagosalia
force-pushed
the
gh-39030
branch
3 times, most recently
from
d7ff5d8 to
9a8d9e0
Compare
nishkagosalia
requested review from
mihir-kandoi,
rohitwaghchaure and
ruthra-kumar
as code owners
📝 WalkthroughWalkthroughAdds an item-level boolean field Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
🤖 Fix all issues with AI Agents
In @erpnext/selling/doctype/sales_order/sales_order.py:
- Around line 2081-2114: In close_or_reopen_selected_items, the subcontracted
Sales Order check (so.is_subcontracted) must run immediately after loading the
Sales Order (so = frappe.get_doc(...)) and before any modifications, saving
(so.save()) or calls to so.update_reserved_qty(), so move the existing
frappe.throw(_("Cannot close items in a subcontracted Sales Order")) to right
after the so variable is created and return/throw early to prevent changing
rows, saving the doc, or updating reserved quantities for subcontracted orders.
- Line 1967: The code erroneously calls so.get("items", {"is_closed": 0})
treating the second arg as a filter; replace this by retrieving the items list
(e.g. so_items = so.get("items", []) or set so.items = so.get("items") ) and
apply the filter when iterating: loop over so_items and skip entries where
getattr(item, "is_closed", 0) is truthy (use getattr(item, "is_closed", 0) to
safely handle packed_items that may lack the field); ensure subsequent logic
uses the filtered list or skips closed items rather than relying on get() to
filter.
In @erpnext/selling/doctype/sales_order/test_sales_order.py:
- Around line 64-66: The test uses incorrect non-boolean integers for the
boolean Check field is_stock_item; update the three make_item calls
(make_item("_Test SO Item Level Closing 1", {"is_stock_item": 4}),
make_item("_Test SO Item Level Closing 2", {"is_stock_item": 5}),
make_item("_Test SO Item Level Closing 3", {"is_stock_item": 6})) to use the
correct boolean/Check value of 1 (or True) for is_stock_item so the items are
properly treated as stock items.
🧹 Nitpick comments (4)
erpnext/stock/stock_balance.py (1)
140-140: Minor formatting inconsistency in SQL.The logic correctly filters out closed items, but the spacing around the equals operator should be consistent with the rest of the SQL query.
🔎 Suggested formatting fix
- and so_item.is_closed=0 and so.status not in ('On Hold', 'Closed'))) + and so_item.is_closed = 0 and so.status not in ('On Hold', 'Closed')))erpnext/patches/v16_0/update_sales_order_item_status.py (1)
9-15: Minor optimization: remove redundant WHERE condition.Line 12 duplicates the JOIN condition from line 9 (
sales_order.name == sales_order_item.parent). This redundancy doesn't affect correctness but adds unnecessary overhead during patch execution.🔎 Suggested optimization
frappe.qb.update(sales_order_item).join(sales_order).on(sales_order.name == sales_order_item.parent).set( sales_order_item.is_closed, 1 ).where( - (sales_order.name == sales_order_item.parent) - & (sales_order.status == "Closed") + (sales_order.status == "Closed") & (sales_order_item.is_closed == 0) ).run()erpnext/selling/doctype/sales_order/sales_order.js (2)
1794-1866: Translate dialog title.The dialog title at line 1824 should be wrapped in the translation function for internationalization.
🔎 Suggested fix
var d = new frappe.ui.Dialog({ - title: "Re-open Selected Items", + title: __("Re-open Selected Items"), size: "large",
1868-1962: Translate dialog title.The dialog title at line 1898 should be wrapped in the translation function for internationalization.
🔎 Suggested fix
var d = new frappe.ui.Dialog({ - title: "Close Selected Items", + title: __("Close Selected Items"), size: "large",
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
erpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/manufacturing/doctype/work_order/work_order.pyerpnext/patches.txterpnext/patches/v16_0/update_sales_order_item_status.pyerpnext/public/js/utils.jserpnext/selling/doctype/sales_order/sales_order.jserpnext/selling/doctype/sales_order/sales_order.pyerpnext/selling/doctype/sales_order/test_sales_order.pyerpnext/selling/doctype/sales_order_item/sales_order_item.jsonerpnext/selling/doctype/sales_order_item/sales_order_item.pyerpnext/selling/report/item_wise_sales_history/item_wise_sales_history.pyerpnext/selling/report/sales_order_analysis/sales_order_analysis.pyerpnext/stock/stock_balance.py
🧰 Additional context used
🧠 Learnings (4)
📚 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/patches/v16_0/update_sales_order_item_status.pyerpnext/selling/report/item_wise_sales_history/item_wise_sales_history.pyerpnext/selling/report/sales_order_analysis/sales_order_analysis.pyerpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/manufacturing/doctype/work_order/work_order.pyerpnext/selling/doctype/sales_order/sales_order.pyerpnext/selling/doctype/sales_order_item/sales_order_item.pyerpnext/stock/stock_balance.pyerpnext/selling/doctype/sales_order/test_sales_order.py
📚 Learning: 2025-09-30T11:04:46.510Z
Learnt from: rohitwaghchaure
Repo: frappe/erpnext PR: 49766
File: erpnext/manufacturing/doctype/production_plan/production_plan.py:1717-1717
Timestamp: 2025-09-30T11:04:46.510Z
Learning: In the Production Plan's `get_items_for_material_requests` function in `erpnext/manufacturing/doctype/production_plan/production_plan.py`, always use `data.get("sales_order")` instead of `doc.get("sales_order")` when iterating over `po_items`. This ensures raw materials are correctly grouped by each production item's respective Sales Order, not a global document-level Sales Order.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.jserpnext/manufacturing/doctype/production_plan/production_plan.py
📚 Learning: 2025-08-12T22:10:55.921Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:929-953
Timestamp: 2025-08-12T22:10:55.921Z
Learning: In ERPNext stock management, there's a critical distinction between availability and reservation calculations: availability functions (like get_bundle_availability) should filter by current status (disabled=0) to determine what can be sold now, while reservation functions (like get_bundle_pos_reserved_qty) should include all historical transactions regardless of current bundle status to accurately reflect stock that was actually consumed. This prevents stock accounting errors when bundle configurations change after sales.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.pyerpnext/stock/stock_balance.py
📚 Learning: 2025-08-12T21:33:27.483Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:871-871
Timestamp: 2025-08-12T21:33:27.483Z
Learning: In ERPNext Product Bundle configurations, items can have qty = 0, which causes division by zero errors in POS bundle availability calculations. The fix is to use a high fallback value (like 1000000) instead of 0 when item.qty is zero, so these items don't constrain bundle availability.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.py
🧬 Code graph analysis (1)
erpnext/selling/doctype/sales_order/test_sales_order.py (1)
erpnext/selling/doctype/sales_order/sales_order.py (1)
close_or_reopen_selected_items(2082-2114)
⏰ 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 (21)
erpnext/patches.txt (1)
459-459: LGTM!The patch entry is correctly placed and follows the standard naming convention for v16_0 patches.
erpnext/selling/doctype/sales_order_item/sales_order_item.py (1)
50-50: LGTM!The type hint for the new
is_closedfield is correctly added in the TYPE_CHECKING block and properly formatted.erpnext/manufacturing/doctype/production_plan/production_plan.py (1)
1545-1545: LGTM!The filter correctly excludes closed Sales Order Items from production planning queries, aligning with the PR's item-level closure feature.
erpnext/manufacturing/doctype/work_order/work_order.py (1)
392-406: Consider adding item-level closure filter to the Sales Order query.The
query_sales_orderfunction filters out Sales Orders with status "Closed" but does not check theis_closedfield on individual Sales Order Items. Since this autocomplete usesor_filtersto match theproduction_itemagainst Sales Order Items, a Sales Order Item withis_closed = 1would still be returned. Adding a filter to exclude closed items would align with the item-level closure feature introduced in this PR.erpnext/selling/report/sales_order_analysis/sales_order_analysis.py (2)
81-82: LGTM!The addition of
soi.is_closedto the SELECT query is clean and correctly retrieves the new field for reporting.
330-335: LGTM!The "Is Closed" column definition follows the established pattern and correctly exposes the item-level closure status in the report UI.
erpnext/public/js/utils.js (1)
613-637: LGTM!The filtering logic correctly excludes closed Sales Order items from the update dialog, preventing accidental modifications while leaving other document types unaffected. This provides good UX defense-in-depth alongside backend validations.
erpnext/selling/doctype/sales_order_item/sales_order_item.json (1)
1015-1022: LGTM!The
is_closedfield definition is well-structured:
allow_on_submit: 1correctly enables closing/reopening after SO submissionin_list_view: 1provides necessary visibility in the items grid- Check fieldtype is appropriate for this boolean flag
erpnext/selling/doctype/sales_order/test_sales_order.py (5)
102-108: LGTM!Good test coverage for validating that fully delivered items cannot be closed, preventing data inconsistency.
111-128: LGTM!Excellent validation that closing an item correctly releases its reserved quantity in the Bin, ensuring inventory accuracy.
131-146: LGTM!Good test coverage ensuring that closed items cannot be updated via
update_child_qty_rate, maintaining data integrity.
149-151: LGTM!Well-designed test verifying that pick lists correctly exclude closed items, showing only the remaining open item for picking.
153-167: LGTM!Comprehensive test validating that reopening a closed item correctly restores its reserved quantity, ensuring the close/reopen workflow is fully reversible.
erpnext/selling/report/item_wise_sales_history/item_wise_sales_history.py (1)
127-133: LGTM! Column integration looks correct.The "Is Closed" column is properly added to the report with correct field type (Check), and the data flow from query → row dict → column display is consistent.
Also applies to: 169-169, 218-218
erpnext/selling/doctype/sales_order/sales_order.js (2)
1000-1010: LGTM! Button placement is appropriate.The "Close selected items" and "Re-open selected items" buttons are correctly placed under the Status dropdown when the order has pending delivery or billing.
1724-1724: LGTM! Correctly excludes closed items.The additional
!d.is_closedcheck properly prevents closed items from being included in the pending quantity calculation for Purchase Order creation.erpnext/selling/doctype/sales_order/sales_order.py (5)
594-599: LGTM! Status update integration is correct.The integration with
close_or_reopen_selected_itemsproperly handles item-level state changes when the Sales Order status is updated to Draft (reopen) or Closed.
1080-1081: LGTM! Correctly excludes closed items from Material Request.The added condition properly prevents closed Sales Order items from being included in Material Request generation.
1197-1200: LGTM! Correctly excludes closed items from Delivery Note.The condition properly filters out closed items during Delivery Note creation.
1439-1443: LGTM! Correctly excludes closed items from Sales Invoice.The condition properly prevents closed items from being included in Sales Invoice generation.
1890-1890: LGTM! Correctly excludes closed items from Pick List.The condition properly prevents closed items from being included in Pick List creation.
nishkagosalia
force-pushed
the
gh-39030
branch
from
9a8d9e0 to
361e3c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
erpnext/patches/v16_0/update_sales_order_item_status.py (1)
4-17: Preserve and restore the originalauto_commit_on_many_writessetting.The patch hardcodes the reset to
0at line 17, but this will incorrectly change the setting if it was already1before the patch ran.🔎 Proposed fix
def execute(): + original_auto_commit = frappe.db.auto_commit_on_many_writes frappe.db.auto_commit_on_many_writes = 1 sales_order = frappe.qb.DocType("Sales Order") sales_order_item = frappe.qb.DocType("Sales Order Item") frappe.qb.update(sales_order_item).join(sales_order).on(sales_order.name == sales_order_item.parent).set( sales_order_item.is_closed, 1 ).where( (sales_order.name == sales_order_item.parent) & (sales_order.status == "Closed") & (sales_order_item.is_closed == 0) ).run() - frappe.db.auto_commit_on_many_writes = 0 + frappe.db.auto_commit_on_many_writes = original_auto_commiterpnext/selling/doctype/sales_order/sales_order.py (1)
2083-2117: Consider streamlining the status update when all items are closed.At lines 2114-2116, when all items are closed via selective closing, the code calls
so.update_status("Closed"), which in turn (at line 597) invokesclose_or_reopen_selected_items(self.name, "Close", all_items_closed=True)again. Since all items are already marked closed and saved at line 2108, this second call performs redundant iteration over the items without changing their state. While not incorrect, this results in a minor inefficiency.💡 Alternative approach
Consider directly updating the status and calling the necessary hooks without re-invoking
close_or_reopen_selected_items:if not all_items_closed and all(d.is_closed for d in so.items): so.status = "Closed" - so.update_status("Closed") + so.set_status(update=True, status="Closed") + so.update_subcontracting_order_status() + so.notify_update() + clear_doctype_notifications(so) return TrueNote: This mirrors the logic in
update_status()but avoids the redundant call toclose_or_reopen_selected_items.erpnext/selling/doctype/sales_order/sales_order.js (1)
1868-1962: Implementation is correct, but consider UX refinement for the "Select all" checkbox.The method is technically sound and correctly handles all scenarios. However, the "Select all items" checkbox defaulting to checked (line 1905) might lead to confusion:
- Users clicking "Close" without reviewing will close the entire Sales Order, not individual items
- The hidden table when "Select all" is checked reduces visibility into what will happen
Consider one of these UX improvements:
- Default the checkbox to unchecked to encourage explicit selection
- Add a warning message when "Select all" is checked explaining it will close the entire SO
- Show a different dialog for "close all" vs "close selected"
That said, the implementation correctly handles the logic: closing all items properly triggers the full SO closure, which is the right behavior.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
erpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/manufacturing/doctype/work_order/work_order.pyerpnext/patches.txterpnext/patches/v16_0/update_sales_order_item_status.pyerpnext/public/js/utils.jserpnext/selling/doctype/sales_order/sales_order.jserpnext/selling/doctype/sales_order/sales_order.pyerpnext/selling/doctype/sales_order/test_sales_order.pyerpnext/selling/doctype/sales_order_item/sales_order_item.jsonerpnext/selling/doctype/sales_order_item/sales_order_item.pyerpnext/selling/report/item_wise_sales_history/item_wise_sales_history.pyerpnext/selling/report/sales_order_analysis/sales_order_analysis.pyerpnext/stock/stock_balance.py
🚧 Files skipped from review as they are similar to previous changes (9)
- erpnext/patches.txt
- erpnext/selling/report/item_wise_sales_history/item_wise_sales_history.py
- erpnext/selling/doctype/sales_order_item/sales_order_item.json
- erpnext/stock/stock_balance.py
- erpnext/manufacturing/doctype/work_order/work_order.py
- erpnext/manufacturing/doctype/production_plan/production_plan.py
- erpnext/public/js/utils.js
- erpnext/selling/report/sales_order_analysis/sales_order_analysis.py
- erpnext/selling/doctype/sales_order_item/sales_order_item.py
🧰 Additional context used
🧠 Learnings (4)
📚 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/selling/doctype/sales_order/test_sales_order.pyerpnext/selling/doctype/sales_order/sales_order.pyerpnext/patches/v16_0/update_sales_order_item_status.py
📚 Learning: 2025-09-30T11:04:46.510Z
Learnt from: rohitwaghchaure
Repo: frappe/erpnext PR: 49766
File: erpnext/manufacturing/doctype/production_plan/production_plan.py:1717-1717
Timestamp: 2025-09-30T11:04:46.510Z
Learning: In the Production Plan's `get_items_for_material_requests` function in `erpnext/manufacturing/doctype/production_plan/production_plan.py`, always use `data.get("sales_order")` instead of `doc.get("sales_order")` when iterating over `po_items`. This ensures raw materials are correctly grouped by each production item's respective Sales Order, not a global document-level Sales Order.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.pyerpnext/selling/doctype/sales_order/sales_order.js
📚 Learning: 2025-08-12T22:10:55.921Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:929-953
Timestamp: 2025-08-12T22:10:55.921Z
Learning: In ERPNext stock management, there's a critical distinction between availability and reservation calculations: availability functions (like get_bundle_availability) should filter by current status (disabled=0) to determine what can be sold now, while reservation functions (like get_bundle_pos_reserved_qty) should include all historical transactions regardless of current bundle status to accurately reflect stock that was actually consumed. This prevents stock accounting errors when bundle configurations change after sales.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.py
📚 Learning: 2025-08-12T21:33:27.483Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:871-871
Timestamp: 2025-08-12T21:33:27.483Z
Learning: In ERPNext Product Bundle configurations, items can have qty = 0, which causes division by zero errors in POS bundle availability calculations. The fix is to use a high fallback value (like 1000000) instead of 0 when item.qty is zero, so these items don't constrain bundle availability.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.py
🧬 Code graph analysis (2)
erpnext/selling/doctype/sales_order/test_sales_order.py (1)
erpnext/selling/doctype/sales_order/sales_order.py (1)
close_or_reopen_selected_items(2084-2117)
erpnext/selling/doctype/sales_order/sales_order.js (2)
erpnext/stock/doctype/stock_entry/stock_entry.js (5)
d(345-378)d(399-399)d(709-712)d(727-727)values(714-714)erpnext/accounts/doctype/sales_invoice/sales_invoice.js (3)
d(1064-1118)me(75-75)me(409-409)
⏰ 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 (1)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Summary
🔇 Additional comments (4)
erpnext/selling/doctype/sales_order/test_sales_order.py (1)
61-168: Excellent test coverage for item-level closing feature.The test comprehensively validates:
- Prevention of closing fully delivered items
- Reserved quantity updates on closing/reopening
- Prevention of updating closed items
- Pick list creation excluding closed items
- Proper state restoration on reopening
The test structure is clear and follows existing patterns in the file.
erpnext/selling/doctype/sales_order/sales_order.js (3)
999-1010: LGTM! UI buttons properly integrated.The "Close selected items" and "Re-open selected items" buttons are correctly placed in the Status group with appropriate visibility conditions. The implementation follows existing patterns in the file.
1724-1724: Correct logic to exclude closed items from Purchase Order creation.The added condition
&& !d.is_closedproperly filters out closed Sales Order items when determining which items should be included in the Purchase Order dialog. This aligns with the expected behavior where closed items should not be ordered.
1794-1866: Well-implemented dialog for reopening selected items.The method correctly:
- Filters to show only closed items
- Presents a clear dialog for item selection
- Validates user input before making server call
- Provides appropriate feedback on success
The implementation follows established patterns in the codebase.
|
|
@nishkagosalia @rohitwaghchaure Should we implement it for Purchase Order as well? |
For Closed item keeping the Color grey, since orange which is by default is also looking like red. |
nishkagosalia
force-pushed
the
gh-39030
branch
2 times, most recently
from
4120332 to
d0c1041
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
erpnext/stock/stock_balance.py (1)
121-140: Missingis_closedcheck for Packed Items in reserved quantity query.The
is_closed=0filter on direct Sales Order Items (line 140) prevents closed items from being counted in reserved stock, but the Packed Items branch (lines 121-129) lacks this check. Since packed items reference specific Sales Order Items viaparent_detail_docname, excluding closed Sales Order Items from reserved quantity while including their packed items creates an inconsistency in reserved stock calculations.Include a filter to verify the parent Sales Order Item is not closed in the packed items subquery:
Suggested fix
from ( select qty, parent_detail_docname, parent, name from `tabPacked Item` dnpi_in where item_code = %s and warehouse = %s and parenttype='Sales Order' and item_code != parent_item and exists (select * from `tabSales Order` so - where name = dnpi_in.parent and docstatus = 1 and status not in ('On Hold', 'Closed')) + where name = dnpi_in.parent and docstatus = 1 and status not in ('On Hold', 'Closed')) + and exists (select * from `tabSales Order Item` soi + where soi.name = dnpi_in.parent_detail_docname and soi.is_closed = 0) ) dnpi)
🤖 Fix all issues with AI agents
In @erpnext/selling/doctype/sales_order/sales_order.js:
- Around line 1846-1849: The validation that checks selected_items currently
only shows a message but does not stop execution; update the block around
selected_items (the code that filters values.reopen_items and then calls
frappe.msgprint) to return early when selected_items.length is zero—i.e., after
frappe.msgprint(__("Please select one item to re-open")) add a return (or wrap
the subsequent frappe.call in an else) so the frappe.call that sends
selected_items is not invoked with an empty array.
- Around line 1825-1836: The dialog strings are not localized: wrap the dialog
title and the table label in the i18n helper __(); specifically update the
frappe.ui.Dialog instantiation (the title property and the Table field's label
property inside the fields array for fieldname "reopen_items") to use
__("Re-open Selected Items") and __("Items") respectively so both strings are
translatable.
- Around line 1943-1945: The validation in close_selected_items() shows a msg
via frappe.msgprint when selected_items is empty but does not stop execution;
add an early return right after the msg (mirror the fix in
reopen_selected_items()) so that when selected_items.length === 0 the function
exits and does not call frappe.call with an empty array.
- Around line 1899-1926: The dialog strings are not internationalized: wrap the
title "Close Selected Items" and the field labels "Select all items" and "Items"
with the translation function __() so they can be localized; update the
frappe.ui.Dialog creation (variable d) and the field definitions (fieldname
"select_all" label, and fieldname "close_items" label) to call __() around those
literal strings and leave other properties (close_item_fields, get_data, etc.)
unchanged.
🧹 Nitpick comments (4)
erpnext/patches/v16_0/update_sales_order_item_status.py (1)
5-17: Preserve originalauto_commit_on_many_writesvalue before modification.The patch unconditionally sets
auto_commit_on_many_writes = 0at the end, but if it was already set to a different value before the patch ran, this would incorrectly override it. Additionally, the join conditionsales_order.name == sales_order_item.parentappears in both the.on()clause (line 9) and the.where()clause (line 12), which is redundant.Suggested improvement
def execute(): - frappe.db.auto_commit_on_many_writes = 1 + previous_auto_commit = frappe.db.auto_commit_on_many_writes + frappe.db.auto_commit_on_many_writes = 1 sales_order = frappe.qb.DocType("Sales Order") sales_order_item = frappe.qb.DocType("Sales Order Item") frappe.qb.update(sales_order_item).join(sales_order).on(sales_order.name == sales_order_item.parent).set( sales_order_item.is_closed, 1 ).where( - (sales_order.name == sales_order_item.parent) - & (sales_order.status == "Closed") + (sales_order.status == "Closed") & (sales_order_item.is_closed == 0) ).run() - frappe.db.auto_commit_on_many_writes = 0 + frappe.db.auto_commit_on_many_writes = previous_auto_commiterpnext/selling/doctype/sales_order_item/sales_order_item.json (1)
1015-1021: Consider addingprint_hideandreport_hideproperties.The
is_closedfield is correctly defined withallow_on_submit: 1to enable modification after submission. However, consider addingprint_hide: 1andreport_hide: 1since this is an internal status field that likely shouldn't appear on printed documents or standard reports.Suggested enhancement
{ "allow_on_submit": 1, "default": "0", "fieldname": "is_closed", "fieldtype": "Check", - "label": "Is Closed" + "label": "Is Closed", + "print_hide": 1, + "report_hide": 1 }erpnext/selling/doctype/sales_order/sales_order.js (2)
1001-1012: Consider conditionally showing buttons based on item states.The buttons are always visible when the SO is actionable, but "Re-open selected items" is only useful when closed items exist, and "Close selected items" is only useful when closable items exist. Consider checking
frm.doc.items.some(d => d.is_closed)andfrm.doc.items.some(d => !d.is_closed && d.qty > flt(d.delivered_qty))respectively to conditionally show these buttons.This would improve UX by not showing irrelevant actions, though the current implementation is functionally correct.
1800-1808: Consider removing unusedis_closeproperty or fixing naming.The
is_closeproperty (line 1805) is mapped fromd.is_closedbut never used in the dialog or likely on the server side. If needed, the naming should be consistent (is_closed). If not needed, it should be removed to reduce payload size.Proposed cleanup (if property is unused)
this.data = this.frm.doc.items .filter((d) => d.is_closed) .map((d) => { return { docname: d.name, item_code: d.item_code, qty: d.qty, - is_close: d.is_closed, delivered_qty: d.delivered_qty, }; });Apply the same change to
close_selected_items()at line 1879.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
erpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/manufacturing/doctype/work_order/work_order.pyerpnext/patches.txterpnext/patches/v16_0/update_sales_order_item_status.pyerpnext/public/js/utils.jserpnext/selling/doctype/sales_order/sales_order.jserpnext/selling/doctype/sales_order/sales_order.pyerpnext/selling/doctype/sales_order/test_sales_order.pyerpnext/selling/doctype/sales_order_item/sales_order_item.jsonerpnext/selling/doctype/sales_order_item/sales_order_item.pyerpnext/selling/report/item_wise_sales_history/item_wise_sales_history.pyerpnext/selling/report/sales_order_analysis/sales_order_analysis.pyerpnext/stock/stock_balance.py
🚧 Files skipped from review as they are similar to previous changes (7)
- erpnext/public/js/utils.js
- erpnext/selling/doctype/sales_order/sales_order.py
- erpnext/selling/report/item_wise_sales_history/item_wise_sales_history.py
- erpnext/manufacturing/doctype/production_plan/production_plan.py
- erpnext/selling/doctype/sales_order/test_sales_order.py
- erpnext/manufacturing/doctype/work_order/work_order.py
- erpnext/selling/report/sales_order_analysis/sales_order_analysis.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-12T22:10:55.921Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:929-953
Timestamp: 2025-08-12T22:10:55.921Z
Learning: In ERPNext stock management, there's a critical distinction between availability and reservation calculations: availability functions (like get_bundle_availability) should filter by current status (disabled=0) to determine what can be sold now, while reservation functions (like get_bundle_pos_reserved_qty) should include all historical transactions regardless of current bundle status to accurately reflect stock that was actually consumed. This prevents stock accounting errors when bundle configurations change after sales.
Applied to files:
erpnext/stock/stock_balance.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/stock/stock_balance.pyerpnext/selling/doctype/sales_order_item/sales_order_item.pyerpnext/patches/v16_0/update_sales_order_item_status.py
📚 Learning: 2025-09-30T11:04:46.510Z
Learnt from: rohitwaghchaure
Repo: frappe/erpnext PR: 49766
File: erpnext/manufacturing/doctype/production_plan/production_plan.py:1717-1717
Timestamp: 2025-09-30T11:04:46.510Z
Learning: In the Production Plan's `get_items_for_material_requests` function in `erpnext/manufacturing/doctype/production_plan/production_plan.py`, always use `data.get("sales_order")` instead of `doc.get("sales_order")` when iterating over `po_items`. This ensures raw materials are correctly grouped by each production item's respective Sales Order, not a global document-level Sales Order.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.js
⏰ 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). (7)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Patch Test
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: linters
- GitHub Check: Summary
🔇 Additional comments (6)
erpnext/patches.txt (1)
459-460: LGTM!The patch entry is correctly placed in the
[post_model_sync]section, ensuring theis_closedfield exists on Sales Order Item before the migration runs.erpnext/selling/doctype/sales_order_item/sales_order_item.py (1)
50-50: LGTM!The
is_closedtype annotation is correctly added within the auto-generated TYPE_CHECKING block, consistent with the new field definition in the DocType JSON.erpnext/selling/doctype/sales_order_item/sales_order_item.json (1)
16-17: Field placement aligns with PR feedback.The
is_closedfield is positioned afterreserve_stockin the field order and doesn't havein_list_viewset, so it won't appear in the grid view. This addresses the PR feedback about removing the Is Closed column from the grid.erpnext/selling/doctype/sales_order/sales_order.js (3)
29-35: LGTM! Indicator logic correctly prioritizes delivery status.The condition ordering ensures fully delivered items remain green while closed (but undelivered) items display grey, aligning with the UI feedback from reviewers.
1726-1726: LGTM! Correctly excludes closed items from Purchase Order creation.This change ensures closed items are filtered out when populating the Purchase Order dialog, aligning with the feature requirement that closed items should not propagate to downstream documents.
1870-1882: LGTM! Filter logic correctly identifies closable items.The filter
d.qty > flt(d.delivered_qty) && !d.is_closedcorrectly identifies items that have pending quantity and are not already closed, ensuring only actionable items appear in the dialog.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
erpnext/selling/doctype/sales_order/sales_order.py (2)
586-599: Blocker:statusvalue mismatch breaks reopen flows ("Reopen"vs"Re-open").
SalesOrder.update_status()passes"Reopen"(Line 593) whileclose_or_reopen_selected_items()checks for"Re-open"(Line 2134), so items won’t be reopened.Proposed fix (normalize status once)
@frappe.whitelist() def close_or_reopen_selected_items(sales_order, status, selected_items=None, all_items_closed=False): + status = (status or "").replace("-", "").strip().lower() items = [] so = frappe.get_doc("Sales Order", sales_order) @@ - if status == "Close" and not all_items_closed: - if row.delivered_qty and row.qty == row.delivered_qty: + if status == "close" and not all_items_closed: + if row.delivered_qty and abs(flt(row.delivered_qty)) >= abs(flt(row.qty)): frappe.throw(_("Item cannot be closed as it is already delivered")) row.is_closed = 1 - elif status == "Re-open": + elif status in ("reopen",): if so.docstatus == 1: so.check_credit_limit() row.is_closed = 0def update_status(self, status): @@ if status == "Draft" and self.docstatus == 1: self.check_credit_limit() - close_or_reopen_selected_items(self.name, "Reopen", all_items_closed=True) + close_or_reopen_selected_items(self.name, "Reopen", all_items_closed=True) if status == "Closed": - close_or_reopen_selected_items(self.name, "Close", all_items_closed=True) + close_or_reopen_selected_items(self.name, "Close", all_items_closed=True) else: self.update_reserved_qty()(With normalization, callers can keep using either
"Reopen"or"Re-open".)Also applies to: 2112-2148
1092-1112: Closed Product Bundle items can still leak viaPacked Itemmapping (Material Request).
You addedand not item.is_closedfor"Sales Order Item"(Line 1110), but the"Packed Item"mapping (Lines 1092-1097) has no equivalent filter; for a closed bundle SO item, its packed rows can still be pulled into MR.erpnext/selling/doctype/sales_order/sales_order.js (2)
996-1013: PO selection UX: exclude closed items consistently (including the “pending items” pre-check).
You filterpo_itemswith!d.is_closed(Line 1726), but the earlierpending_itemscheck (Line 1589+) can still pass due to closed rows, leading to an empty selection dialog.Also applies to: 1721-1736
25-35: Use"gray"instead of"grey"for the closed-row indicator.The Frappe framework defines indicator colors as
["blue", "gray", "green", "orange", "yellow"](American spelling). Using"grey"(British spelling) will not apply the expected styling to closed items.
🤖 Fix all issues with AI agents
In @erpnext/patches/v16_0/update_sales_order_item_status.py:
- Around line 4-17: The patch mutates the global flag
frappe.db.auto_commit_on_many_writes without preserving its prior state and can
leak that setting on exceptions; modify execute() to save the current value of
frappe.db.auto_commit_on_many_writes to a local variable, set it to 1 before
running the QB update, and restore the saved value in a finally block so the
original state is always reinstated (use the existing frappe.qb.update(...)
block and ensure the restore happens even if .run() raises).
In @erpnext/selling/doctype/sales_order/sales_order.js:
- Around line 1796-1964: The dialogs in reopen_selected_items and
close_selected_items proceed to call the server even when no rows are selected
and use hard-coded strings for titles/labels; update the dialog titles ("Re-open
Selected Items", "Close Selected Items") and the "Select all items" label to use
__('...') for i18n, and add an early return immediately after the msgprint calls
in both primary_action handlers (the checks that show __("Please select one item
to re-open") and __("Please select one item to close")) so the function exits
instead of continuing to frappe.call; reference reopen_selected_items,
close_selected_items, the primary_action functions, and the dialog variable d to
apply these changes.
In @erpnext/selling/doctype/sales_order/test_sales_order.py:
- Around line 61-167: The test fails due to a mismatch in status naming
("Reopen" vs "Re-open"); in test_sales_order_item_level_closing update the call
to close_or_reopen_selected_items that reopens the item to use the
server-expected value ("Re-open") instead of "Reopen" (i.e., replace
close_or_reopen_selected_items(so.name, "Reopen", json.dumps([...])) with
close_or_reopen_selected_items(so.name, "Re-open", json.dumps([...])) so the
status string matches backend expectations; alternatively, if you prefer the
backend change, normalize the status handling inside
close_or_reopen_selected_items to accept both "Reopen" and "Re-open".
🧹 Nitpick comments (1)
erpnext/public/js/utils.js (1)
605-638: Filter forSales Order Item.is_closedis good; consider a safer child-table access.
Ifopts.child_docnameis ever omitted, this will throw. A small defensive fallback keeps the utility robust.Proposed tweak
const frm = opts.frm; const cannot_add_row = typeof opts.cannot_add_row === "undefined" ? true : opts.cannot_add_row; const child_docname = typeof opts.cannot_add_row === "undefined" ? "items" : opts.child_docname; @@ const is_sales_order = frm.doc.doctype === "Sales Order"; - this.data = frm.doc[opts.child_docname] + const rows = frm.doc[opts.child_docname || child_docname] || []; + this.data = rows .filter((d) => { if (is_sales_order) { return !d.is_closed; } return true; })
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
erpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/manufacturing/doctype/work_order/work_order.pyerpnext/patches.txterpnext/patches/v16_0/update_sales_order_item_status.pyerpnext/public/js/utils.jserpnext/selling/doctype/sales_order/sales_order.jserpnext/selling/doctype/sales_order/sales_order.pyerpnext/selling/doctype/sales_order/test_sales_order.pyerpnext/selling/doctype/sales_order_item/sales_order_item.jsonerpnext/selling/doctype/sales_order_item/sales_order_item.pyerpnext/selling/report/item_wise_sales_history/item_wise_sales_history.pyerpnext/selling/report/sales_order_analysis/sales_order_analysis.pyerpnext/stock/stock_balance.py
🚧 Files skipped from review as they are similar to previous changes (6)
- erpnext/selling/report/sales_order_analysis/sales_order_analysis.py
- erpnext/stock/stock_balance.py
- erpnext/manufacturing/doctype/production_plan/production_plan.py
- erpnext/selling/doctype/sales_order_item/sales_order_item.json
- erpnext/selling/report/item_wise_sales_history/item_wise_sales_history.py
- erpnext/manufacturing/doctype/work_order/work_order.py
🧰 Additional context used
🧠 Learnings (4)
📚 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/selling/doctype/sales_order/test_sales_order.pyerpnext/patches/v16_0/update_sales_order_item_status.pyerpnext/selling/doctype/sales_order_item/sales_order_item.pyerpnext/selling/doctype/sales_order/sales_order.py
📚 Learning: 2025-09-30T11:04:46.510Z
Learnt from: rohitwaghchaure
Repo: frappe/erpnext PR: 49766
File: erpnext/manufacturing/doctype/production_plan/production_plan.py:1717-1717
Timestamp: 2025-09-30T11:04:46.510Z
Learning: In the Production Plan's `get_items_for_material_requests` function in `erpnext/manufacturing/doctype/production_plan/production_plan.py`, always use `data.get("sales_order")` instead of `doc.get("sales_order")` when iterating over `po_items`. This ensures raw materials are correctly grouped by each production item's respective Sales Order, not a global document-level Sales Order.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.jserpnext/selling/doctype/sales_order/sales_order.py
📚 Learning: 2025-08-12T22:10:55.921Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:929-953
Timestamp: 2025-08-12T22:10:55.921Z
Learning: In ERPNext stock management, there's a critical distinction between availability and reservation calculations: availability functions (like get_bundle_availability) should filter by current status (disabled=0) to determine what can be sold now, while reservation functions (like get_bundle_pos_reserved_qty) should include all historical transactions regardless of current bundle status to accurately reflect stock that was actually consumed. This prevents stock accounting errors when bundle configurations change after sales.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.py
📚 Learning: 2025-08-12T21:33:27.483Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:871-871
Timestamp: 2025-08-12T21:33:27.483Z
Learning: In ERPNext Product Bundle configurations, items can have qty = 0, which causes division by zero errors in POS bundle availability calculations. The fix is to use a high fallback value (like 1000000) instead of 0 when item.qty is zero, so these items don't constrain bundle availability.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.py
🧬 Code graph analysis (3)
erpnext/selling/doctype/sales_order/test_sales_order.py (2)
erpnext/selling/doctype/sales_order/sales_order.py (2)
close_or_reopen_selected_items(2113-2148)create_pick_list(1880-1958)erpnext/controllers/accounts_controller.py (1)
update_child_qty_rate(3824-4211)
erpnext/selling/doctype/sales_order/sales_order.js (1)
erpnext/stock/doctype/stock_entry/stock_entry.js (5)
d(363-396)d(417-417)d(727-730)d(745-745)values(732-732)
erpnext/selling/doctype/sales_order/sales_order.py (4)
erpnext/selling/doctype/customer/customer.py (1)
check_credit_limit(558-615)erpnext/subcontracting/doctype/subcontracting_order/subcontracting_order.py (1)
update_status(333-365)erpnext/stock/doctype/stock_reservation_entry/stock_reservation_entry.py (1)
update_status(530-553)erpnext/stock/doctype/material_request/material_request.py (2)
update_status(249-253)update_status(482-485)
⏰ 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 (3)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Summary
🔇 Additional comments (3)
erpnext/patches.txt (1)
459-460: Patch entry addition looks fine.
Just ensure this patch runs only after theSales Order Item.is_closedfield exists (post model sync), which it appears to.erpnext/selling/doctype/sales_order/sales_order.py (1)
1212-1229:is_closedgating in downstream creation paths is consistent (good).
The addednot <row>.is_closedchecks for DN/SI/Pick List/WO item selection look aligned with the feature intent.Also applies to: 1458-1475, 1914-1920, 1996-2001
erpnext/selling/doctype/sales_order_item/sales_order_item.py (1)
15-52: TYPE_CHECKING addition foris_closedlooks good.
nishkagosalia
force-pushed
the
gh-39030
branch
from
d0c1041 to
dc710f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @erpnext/manufacturing/doctype/production_plan/production_plan.py:
- Around line 1547-1548: The filter at the end of the sales order item predicate
uses (so_item.is_closed == 0) without accounting for NULLs; update that
condition to ((so_item.is_closed == 0) | (so_item.is_closed.isnull())) so items
with is_closed NULL are treated as open; locate the check in production_plan.py
where so_item is used in the query/filter (around the block containing "&
(so_item.is_closed == 0)") and replace that expression accordingly.
In @erpnext/selling/doctype/sales_order/sales_order.py:
- Around line 593-594: The call to close_or_reopen_selected_items uses "Reopen"
but the function logic expects "Re-open", causing reopen to never trigger;
update the caller at the sales order reopen site (where
close_or_reopen_selected_items(self.name, "Reopen", all_items_closed=True) is
invoked) to pass "Re-open" to match the check in close_or_reopen_selected_items,
or alternatively standardize the function to accept both variants (e.g.,
normalize the status string inside close_or_reopen_selected_items) so that the
comparison at the function (the code that checks for "Re-open") and all callers
are consistent.
- Around line 2145-2148: The snippet in close_or_reopen_selected_items sets
so.status = "Closed" and then calls so.update_status("Closed"), which triggers
update_status to call close_or_reopen_selected_items again and creates a
redundant circular path; fix it by making the call conditional—only invoke
so.update_status("Closed") if so.status != "Closed" (or introduce and pass a
guard flag to prevent re-entry), so update_status/close_or_reopen_selected_items
are not called recursively when the Sales Order is already closed; update the
logic in sales_order.py (function close_or_reopen_selected_items) to perform the
check or pass a skip-recursion parameter to break the cycle.
🧹 Nitpick comments (5)
erpnext/selling/report/sales_order_analysis/sales_order_analysis.py (1)
330-336: Consider makingIs Closedcolumn conditional on non-grouped view.The
Is Closedcolumn is added tocolumns.extend()which applies to all views. However, when grouping by Sales Order, individual item-levelis_closedstatus doesn't aggregate meaningfully (items could have mixed closed states). Similar toItem Code,Description, andWarehousecolumns (lines 239-251, 338-347), this column should probably only appear whennot filters.get("group_by_so").Proposed fix
{"label": _("Delay (in Days)"), "fieldname": "delay", "fieldtype": "Data", "width": 100}, { "label": _("Time Taken to Deliver"), "fieldname": "time_taken_to_deliver", "fieldtype": "Duration", "width": 100, }, - { - "label": _("Is Closed"), - "fieldname": "is_closed", - "fieldtype": "Check", - "width": 80, - }, ] ) if not filters.get("group_by_so"): + columns.append( + { + "label": _("Is Closed"), + "fieldname": "is_closed", + "fieldtype": "Check", + "width": 80, + } + ) columns.append( { "label": _("Warehouse"),erpnext/selling/report/item_wise_sales_history/item_wise_sales_history.py (1)
127-133: Remove unnecessaryoptionsattribute for Check fieldtype.The
optionsfield is not applicable forCheckfieldtype columns in Frappe reports. This attribute is typically used forLinkorSelectfieldtypes. While it won't cause errors, it's misleading and should be removed for clarity.Suggested fix
{ "label": _("Is Closed"), "fieldtype": "Check", "fieldname": "is_closed", - "options": "is_closed", "width": 80, },erpnext/selling/doctype/sales_order/test_sales_order.py (1)
61-167: Comprehensive test for item-level closing feature.The test covers critical scenarios including:
- Preventing closure of fully delivered items
- Reserved quantity updates on close/reopen
- Blocking updates to closed items
- Pick list exclusion of closed items
One minor observation: the
frappe.get_allcalls (lines 112-117, 122-127, 161-166) assume a Bin record exists. Whilemake_stock_entryshould create the Bin, consider usingget_reserved_qtyhelper (already defined at line 2811) for consistency with other tests in this file.Optional: Use existing helper for consistency
- self.assertEqual( - frappe.get_all( - "Bin", - filters={"item_code": so.items[1].item_code, "warehouse": so.items[1].warehouse}, - fields=["reserved_qty"], - )[0].reserved_qty, - 3, - ) + self.assertEqual( + get_reserved_qty(item_code=so.items[1].item_code, warehouse=so.items[1].warehouse), + 3, + )erpnext/selling/doctype/sales_order/sales_order.js (2)
1796-1868: Minor: Unused and inconsistent field name.The
is_closefield (line 1805) is mapped but not used in the dialog fields or server call. This is dead code that should be removed for clarity.Remove unused field
this.data = this.frm.doc.items .filter((d) => d.is_closed) .map((d) => { return { docname: d.name, item_code: d.item_code, qty: d.qty, - is_close: d.is_closed, - delivered_qty: d.delivered_qty, }; });
1871-1966: Well-designed dialog with smart "Select All" behavior.The implementation correctly handles:
- Closing the entire SO when "Select all items" is checked
- Closing the entire SO when all items are manually selected
- Individual item closure otherwise
Minor: Same unused fields (
is_close,delivered_qty) as inreopen_selected_items.Remove unused fields for consistency
this.data = this.frm.doc.items .filter((d) => d.qty > flt(d.delivered_qty) && !d.is_closed) .map((d) => { return { docname: d.name, item_code: d.item_code, qty: d.qty, - is_close: d.is_closed, - delivered_qty: d.delivered_qty, }; });
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
erpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/manufacturing/doctype/work_order/work_order.pyerpnext/patches.txterpnext/patches/v16_0/update_sales_order_item_status.pyerpnext/public/js/utils.jserpnext/selling/doctype/sales_order/sales_order.jserpnext/selling/doctype/sales_order/sales_order.pyerpnext/selling/doctype/sales_order/test_sales_order.pyerpnext/selling/doctype/sales_order_item/sales_order_item.jsonerpnext/selling/doctype/sales_order_item/sales_order_item.pyerpnext/selling/report/item_wise_sales_history/item_wise_sales_history.pyerpnext/selling/report/sales_order_analysis/sales_order_analysis.pyerpnext/stock/stock_balance.py
🚧 Files skipped from review as they are similar to previous changes (5)
- erpnext/patches/v16_0/update_sales_order_item_status.py
- erpnext/manufacturing/doctype/work_order/work_order.py
- erpnext/selling/doctype/sales_order_item/sales_order_item.json
- erpnext/selling/doctype/sales_order_item/sales_order_item.py
- erpnext/patches.txt
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-30T11:04:46.510Z
Learnt from: rohitwaghchaure
Repo: frappe/erpnext PR: 49766
File: erpnext/manufacturing/doctype/production_plan/production_plan.py:1717-1717
Timestamp: 2025-09-30T11:04:46.510Z
Learning: In the Production Plan's `get_items_for_material_requests` function in `erpnext/manufacturing/doctype/production_plan/production_plan.py`, always use `data.get("sales_order")` instead of `doc.get("sales_order")` when iterating over `po_items`. This ensures raw materials are correctly grouped by each production item's respective Sales Order, not a global document-level Sales Order.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.pyerpnext/selling/doctype/sales_order/sales_order.jserpnext/manufacturing/doctype/production_plan/production_plan.py
📚 Learning: 2025-08-12T22:10:55.921Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:929-953
Timestamp: 2025-08-12T22:10:55.921Z
Learning: In ERPNext stock management, there's a critical distinction between availability and reservation calculations: availability functions (like get_bundle_availability) should filter by current status (disabled=0) to determine what can be sold now, while reservation functions (like get_bundle_pos_reserved_qty) should include all historical transactions regardless of current bundle status to accurately reflect stock that was actually consumed. This prevents stock accounting errors when bundle configurations change after sales.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.pyerpnext/stock/stock_balance.py
📚 Learning: 2025-08-12T21:33:27.483Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:871-871
Timestamp: 2025-08-12T21:33:27.483Z
Learning: In ERPNext Product Bundle configurations, items can have qty = 0, which causes division by zero errors in POS bundle availability calculations. The fix is to use a high fallback value (like 1000000) instead of 0 when item.qty is zero, so these items don't constrain bundle availability.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.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/selling/doctype/sales_order/sales_order.pyerpnext/selling/report/sales_order_analysis/sales_order_analysis.pyerpnext/selling/report/item_wise_sales_history/item_wise_sales_history.pyerpnext/stock/stock_balance.pyerpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/selling/doctype/sales_order/test_sales_order.py
🧬 Code graph analysis (1)
erpnext/selling/doctype/sales_order/test_sales_order.py (2)
erpnext/selling/doctype/sales_order/sales_order.py (3)
close_or_reopen_selected_items(2113-2148)make_delivery_note(1149-1320)create_pick_list(1880-1958)erpnext/controllers/accounts_controller.py (1)
update_child_qty_rate(3824-4211)
⏰ 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 (1)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (11)
erpnext/manufacturing/doctype/production_plan/production_plan.py (1)
371-372: Correctly handles NULL values foris_closedfiltering.The filters properly account for existing Sales Order Items that may have
is_closed = NULLbefore the migration patch runs, ensuring backward compatibility.Also applies to: 436-437
erpnext/selling/report/sales_order_analysis/sales_order_analysis.py (1)
81-82: SQL change correctly fetchesis_closedfield.The field is properly added to the SELECT list for report data.
erpnext/selling/doctype/sales_order/sales_order.py (2)
595-599: Logic correctly separates reserved quantity updates.The branching properly delegates reserved quantity updates:
close_or_reopen_selected_itemshandles it for Close/Re-open transitions, whileupdate_reserved_qtyhandles other status changes.
1109-1110: Correctly excludes closed items from downstream document creation.All document mapping conditions properly filter out closed items:
- Material Request creation (line 1110)
- Delivery Note creation (lines 1226-1229)
- Sales Invoice creation (lines 1468-1472)
- Pick List creation (line 1919)
- Work Order items (lines 1998-2000 with safe
getattrfor packed items)This aligns with the PR objective that closed items should not appear in documents created from the Sales Order.
Also applies to: 1226-1229, 1468-1472, 1919-1919, 1998-2000
erpnext/public/js/utils.js (1)
613-637: Correctly filters closed items from Update Items dialog.The implementation properly:
- Checks if the document is a Sales Order before applying the filter
- Excludes items where
is_closedis truthy- Preserves all the original field mappings for non-closed items
This aligns with the PR objective that closed items cannot be updated.
erpnext/stock/stock_balance.py (1)
138-141: NULL handling foris_closedis unnecessary; Packed Items filtering should be verified.Two issues with this change:
NULL handling is not needed: The
is_closedfield in Sales Order Item is a Check (checkbox) field that defaults to 0 and does not allow NULL values by schema definition. New records will always haveis_closed = 0, and existing records before any schema migration should have been set to a default value. The conditionso_item.is_closed=0correctly handles all valid states.Packed Items inconsistency needs clarification: The first UNION branch (Packed Items, lines 101-129) does not filter by
is_closed. Verify whether Packed Items should inherit the parent Sales Order Item's closure status, or if they are tracked independently for reservation purposes. If Packed Items can be reserved separately from their parent SO Items, the current behavior may be correct.erpnext/selling/report/item_wise_sales_history/item_wise_sales_history.py (1)
218-218: LGTM!The
is_closedfield is correctly added to the query selection, enabling the report to display item-level closure status.erpnext/selling/doctype/sales_order/test_sales_order.py (1)
23-32: LGTM!The import of
close_or_reopen_selected_itemsis correctly added alongside other Sales Order functions.erpnext/selling/doctype/sales_order/sales_order.js (3)
25-38: LGTM!The indicator logic correctly prioritizes the display:
- Yellow for zero-qty unit price items
- Green for fully delivered items
- Grey for closed (but not fully delivered) items
- Orange for pending items
This ordering ensures closed items are visually distinguished while maintaining delivery status visibility.
1001-1012: LGTM!The custom buttons are correctly placed under the "Status" group and follow the same visibility conditions as the existing "Close" button. The placement ensures consistency with the existing UI patterns.
1726-1726: LGTM!Correctly excludes closed items from the Purchase Order creation dialog. This aligns with the PR objective that closed items should not be reflected in other doctypes.
nishkagosalia
force-pushed
the
gh-39030
branch
from
dc710f4 to
62a8ae5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
erpnext/selling/report/sales_order_analysis/sales_order_analysis.py (1)
330-335: Consider limitingIs Closedcolumn to non-grouped view only.The
Is Closedcolumn is added withincolumns.extend(), which means it appears in both grouped and non-grouped views. Whengroup_by_sois enabled, multiple items are aggregated into one row, and theis_closedvalue would reflect only the last processed item's status, which could be misleading.Consider moving this column definition to the non-grouped section (similar to
Item CodeandWarehousecolumns on lines 239-251 and 338-347).♻️ Suggested refactor
Move the
Is Closedcolumn to the non-grouped section:if not filters.get("group_by_so"): columns.append( { "label": _("Item Code"), "fieldname": "item_code", "fieldtype": "Link", "options": "Item", "width": 100, } ) columns.append( {"label": _("Description"), "fieldname": "description", "fieldtype": "Small Text", "width": 100} ) + columns.append( + { + "label": _("Is Closed"), + "fieldname": "is_closed", + "fieldtype": "Check", + "width": 80, + } + )And remove it from
columns.extend()block.erpnext/selling/doctype/sales_order_item/sales_order_item.json (1)
1015-1021: Consider addingread_onlyto prevent direct field editing.The
is_closedfield allows modification after submit (allow_on_submit: 1) but lacksread_only: 1. This means users could potentially toggleis_closeddirectly in the form, bypassing theclose_or_reopen_selected_itemsAPI which handles critical side effects like updating reserved quantities and checking credit limits.If the intention is to only allow closing/reopening through the dedicated UI actions, consider adding
read_only: 1to enforce this:♻️ Suggested change
{ "allow_on_submit": 1, "default": "0", "fieldname": "is_closed", "fieldtype": "Check", - "label": "Is Closed" + "label": "Is Closed", + "read_only": 1 }erpnext/selling/doctype/sales_order/test_sales_order.py (1)
61-167: Well-structured test with comprehensive coverage.The test thoroughly covers the item-level closing functionality:
- Prevents closing fully delivered items
- Validates reserved quantity updates on close/reopen
- Ensures closed items cannot be updated
- Verifies pick list excludes closed items
Minor suggestions for robustness:
- Lines 111-117 and 121-128 use
frappe.get_all("Bin", ...)which could return an empty list if the Bin doesn't exist yet, causing an IndexError on[0].reserved_qty.- Line 151 only checks location count; consider also verifying which item is in the pick list to ensure item_3 (the open item) is included.
♻️ Optional: Add defensive check for Bin query
- self.assertEqual( - frappe.get_all( - "Bin", - filters={"item_code": so.items[1].item_code, "warehouse": so.items[1].warehouse}, - fields=["reserved_qty"], - )[0].reserved_qty, - 3, - ) + bin_data = frappe.get_all( + "Bin", + filters={"item_code": so.items[1].item_code, "warehouse": so.items[1].warehouse}, + fields=["reserved_qty"], + ) + self.assertTrue(bin_data, "Bin record should exist") + self.assertEqual(bin_data[0].reserved_qty, 3)erpnext/selling/doctype/sales_order/sales_order.py (1)
2112-2151: Well-implemented item-level closing API with a few considerations.The implementation correctly:
- Validates against subcontracted SOs
- Prevents closing fully delivered items
- Handles credit limit checks on reopen
- Updates reserved quantities appropriately
- Auto-transitions SO to Closed when all items are closed
Considerations:
Line 2130-2131: The validation only prevents closing fully delivered items. Consider whether items with pending billing should also be restricted or at least warned about, as closing an item may affect billing workflows.
Line 2131: The error message "Item cannot be closed as it is already delivered" could be more specific by including the item code or row number for multi-item operations.
Missing return on Re-open all items: When
status == "Re-open"andall_items_closed=True, the function opens all items but doesn't have explicit handling for updating SO status from Closed back to an active state. This is handled by the caller (update_status), but it's worth verifying this interaction.♻️ Optional: Improve error message specificity
if status == "Close" and not all_items_closed: if row.delivered_qty and row.qty == row.delivered_qty: - frappe.throw(_("Item cannot be closed as it is already delivered")) + frappe.throw(_("Row #{0}: Item {1} cannot be closed as it is already delivered").format(row.idx, row.item_code))
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
erpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/manufacturing/doctype/work_order/work_order.pyerpnext/patches.txterpnext/patches/v16_0/update_sales_order_item_status.pyerpnext/public/js/utils.jserpnext/selling/doctype/sales_order/sales_order.jserpnext/selling/doctype/sales_order/sales_order.pyerpnext/selling/doctype/sales_order/test_sales_order.pyerpnext/selling/doctype/sales_order_item/sales_order_item.jsonerpnext/selling/doctype/sales_order_item/sales_order_item.pyerpnext/selling/report/item_wise_sales_history/item_wise_sales_history.pyerpnext/selling/report/sales_order_analysis/sales_order_analysis.pyerpnext/stock/stock_balance.py
✅ Files skipped from review due to trivial changes (1)
- erpnext/patches.txt
🚧 Files skipped from review as they are similar to previous changes (7)
- erpnext/selling/doctype/sales_order_item/sales_order_item.py
- erpnext/selling/doctype/sales_order/sales_order.js
- erpnext/public/js/utils.js
- erpnext/manufacturing/doctype/production_plan/production_plan.py
- erpnext/manufacturing/doctype/work_order/work_order.py
- erpnext/selling/report/item_wise_sales_history/item_wise_sales_history.py
- erpnext/patches/v16_0/update_sales_order_item_status.py
🧰 Additional context used
🧠 Learnings (4)
📚 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/selling/doctype/sales_order/test_sales_order.pyerpnext/stock/stock_balance.pyerpnext/selling/doctype/sales_order/sales_order.pyerpnext/selling/report/sales_order_analysis/sales_order_analysis.py
📚 Learning: 2025-08-12T22:10:55.921Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:929-953
Timestamp: 2025-08-12T22:10:55.921Z
Learning: In ERPNext stock management, there's a critical distinction between availability and reservation calculations: availability functions (like get_bundle_availability) should filter by current status (disabled=0) to determine what can be sold now, while reservation functions (like get_bundle_pos_reserved_qty) should include all historical transactions regardless of current bundle status to accurately reflect stock that was actually consumed. This prevents stock accounting errors when bundle configurations change after sales.
Applied to files:
erpnext/stock/stock_balance.pyerpnext/selling/doctype/sales_order/sales_order.py
📚 Learning: 2025-09-30T11:04:46.510Z
Learnt from: rohitwaghchaure
Repo: frappe/erpnext PR: 49766
File: erpnext/manufacturing/doctype/production_plan/production_plan.py:1717-1717
Timestamp: 2025-09-30T11:04:46.510Z
Learning: In the Production Plan's `get_items_for_material_requests` function in `erpnext/manufacturing/doctype/production_plan/production_plan.py`, always use `data.get("sales_order")` instead of `doc.get("sales_order")` when iterating over `po_items`. This ensures raw materials are correctly grouped by each production item's respective Sales Order, not a global document-level Sales Order.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.py
📚 Learning: 2025-08-12T21:33:27.483Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:871-871
Timestamp: 2025-08-12T21:33:27.483Z
Learning: In ERPNext Product Bundle configurations, items can have qty = 0, which causes division by zero errors in POS bundle availability calculations. The fix is to use a high fallback value (like 1000000) instead of 0 when item.qty is zero, so these items don't constrain bundle availability.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.py
🧬 Code graph analysis (1)
erpnext/selling/doctype/sales_order/test_sales_order.py (2)
erpnext/selling/doctype/sales_order/sales_order.py (1)
close_or_reopen_selected_items(2113-2152)erpnext/controllers/accounts_controller.py (1)
update_child_qty_rate(3824-4211)
⏰ 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 (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Summary
🔇 Additional comments (8)
erpnext/selling/report/sales_order_analysis/sales_order_analysis.py (1)
81-82: LGTM!The
soi.is_closedfield is correctly added to the SELECT statement to surface the closed status of each Sales Order Item in the report data.erpnext/selling/doctype/sales_order/sales_order.py (6)
593-598: Logic flow looks correct for status transitions.The delegation to
close_or_reopen_selected_itemswithall_items_closed=Trueproperly handles:
- Re-opening all items when transitioning from Closed to Draft
- Closing all items when transitioning to Closed status
The
elsebranch callsupdate_reserved_qty()for other status changes (like On Hold), which is appropriate.
1109-1110: LGTM!Correctly excludes closed Sales Order Items from Material Request creation. The condition is appropriately placed within the existing filter logic.
1226-1229: LGTM!Correctly excludes closed items from Delivery Note creation. The multi-line condition is clear and readable.
1468-1472: LGTM!Correctly excludes closed items from Sales Invoice creation while preserving the unit price row handling logic.
1919-1919: LGTM!Pick List correctly excludes closed items from being picked.
1998-1999: LGTM!Using
getattr(i, "is_closed", 0)safely handles bothso.itemsandso.packed_items, since packed items may not have theis_closedattribute.erpnext/stock/stock_balance.py (1)
140-140: Add is_closed check for parent Sales Order Item in packed item path.The
is_closedfilter is only applied to direct Sales Order Items (line 140), but the Packed Item path (lines 121-129) does not check whether the parent Sales Order Item is closed. This causes packed items from closed SO items to still contribute toreserved_qty, creating an inconsistency.Add a filter on the parent Sales Order Item's
is_closedstatus in the packed item subqueries. For example, modify the subqueries that fetchso_item_qty,so_item_delivered_qty, andso_item_returned_qtyto includeand is_closed=0when selecting fromtabSales Order Item.⛔ Skipped due to learnings
Learnt from: LewisMojica Repo: frappe/erpnext PR: 49108 File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:929-953 Timestamp: 2025-08-12T22:10:55.921Z Learning: In ERPNext stock management, there's a critical distinction between availability and reservation calculations: availability functions (like get_bundle_availability) should filter by current status (disabled=0) to determine what can be sold now, while reservation functions (like get_bundle_pos_reserved_qty) should include all historical transactions regardless of current bundle status to accurately reflect stock that was actually consumed. This prevents stock accounting errors when bundle configurations change after sales.
nishkagosalia
force-pushed
the
gh-39030
branch
from
62a8ae5 to
0772cc7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @erpnext/selling/doctype/sales_order_item/sales_order_item.json:
- Around line 17-18: The is_closed field must be made non-editable in the client
so users cannot toggle it directly; update the sales_order_item JSON definitions
where "is_closed" appears (including the duplicated block around lines
1015-1021) to remove it from any editable row/form lists and set its field
properties to be server-controlled (e.g., add "read_only": 1 and/or "hidden": 1
in the field definition and remove it from any "in_grid_view"/row form arrays),
ensuring only the server action that closes/reopens items can change is_closed.
In @erpnext/selling/doctype/sales_order/sales_order.py:
- Around line 2112-2152: In close_or_reopen_selected_items, when status ==
"Close" and all_items_closed=True the loop currently skips setting
row.is_closed, so update the loop logic in close_or_reopen_selected_items to set
row.is_closed = 1 for every row when closing the whole order (i.e., handle the
all_items_closed branch for "Close"); also move the credit check
so.check_credit_limit() out of the per-row loop (call it once before iterating
when status == "Re-open" and so.docstatus == 1) to avoid repeated checks; ensure
update_reserved_qty is still called correctly (no items set => update all) and
preserve existing behavior for the selected_items path.
In @erpnext/selling/doctype/sales_order/test_sales_order.py:
- Around line 61-167: The Bin lookups in test_sales_order_item_level_closing use
frappe.get_all(...)[0].reserved_qty which can raise IndexError if no Bin row
exists; update each assertion to first fetch the list and assert its length > 0
(or use frappe.get_value("Bin", filters, "reserved_qty") which returns None if
missing) and then assert reserved_qty equals the expected value; adjust the
three places referencing frappe.get_all(...)[0].reserved_qty (before closing,
after closing, after re-open) to perform a safe presence check and then compare
the reserved_qty to avoid IndexError and provide a clearer test failure.
In @erpnext/selling/report/item_wise_sales_history/item_wise_sales_history.py:
- Around line 127-133: The column definition for the "is_closed" field
incorrectly includes an "options" key; remove the "options": "is_closed" entry
from the dict that defines the column with label _("Is Closed") and fieldname
"is_closed" so that the Check fieldtype has no options property (update the
column config where the dict with "label": _("Is Closed"), "fieldtype": "Check",
"fieldname": "is_closed" is declared).
🧹 Nitpick comments (6)
erpnext/patches/v16_0/update_sales_order_item_status.py (1)
4-18: Preserve originalauto_commit_on_many_writesvalue.The current implementation unconditionally sets
auto_commit_on_many_writesto0in thefinallyblock, which could override the original value if it was already1.Also, line 13 has a redundant condition that's already specified in the JOIN clause.
♻️ Suggested fix
def execute(): - frappe.db.auto_commit_on_many_writes = 1 + original_auto_commit = frappe.db.auto_commit_on_many_writes + frappe.db.auto_commit_on_many_writes = 1 sales_order = frappe.qb.DocType("Sales Order") sales_order_item = frappe.qb.DocType("Sales Order Item") try: frappe.qb.update(sales_order_item).join(sales_order).on( sales_order.name == sales_order_item.parent ).set(sales_order_item.is_closed, 1).where( - (sales_order.name == sales_order_item.parent) - & (sales_order.status == "Closed") + (sales_order.status == "Closed") & (sales_order_item.is_closed == 0) ).run() finally: - frappe.db.auto_commit_on_many_writes = 0 + frappe.db.auto_commit_on_many_writes = original_auto_commiterpnext/selling/report/sales_order_analysis/sales_order_analysis.py (1)
239-336:Is Closedcolumn should not appear (or should be aggregated) whengroup_by_sois enabled.
When grouping by Sales Order, a singleis_closedvalue on the aggregated row is misleading (it’ll just be whichever row was deep-copied first).Proposed diff
@@ if not filters.get("group_by_so"): columns.append( { "label": _("Item Code"), @@ columns.append( {"label": _("Description"), "fieldname": "description", "fieldtype": "Small Text", "width": 100} ) + columns.append( + { + "label": _("Is Closed"), + "fieldname": "is_closed", + "fieldtype": "Check", + "width": 80, + } + ) columns.extend( [ @@ { "label": _("Time Taken to Deliver"), "fieldname": "time_taken_to_deliver", "fieldtype": "Duration", "width": 100, }, - { - "label": _("Is Closed"), - "fieldname": "is_closed", - "fieldtype": "Check", - "width": 80, - }, ] )erpnext/selling/doctype/sales_order/sales_order.js (3)
1002-1012: Show “Re-open selected items” only when there are closed items (and “Close selected items” only when there are closable items).
Right now the buttons can lead to an empty dialog + confusing “Please select…” message.
1796-1869: Guard against opening the re-open dialog with zero rows.
Ifthis.data.length === 0, considerfrappe.msgprint(__("No closed items to re-open"))and return.
1871-1966: Verifyonchangecallback context; preferd.get_value("select_all")to avoidthisambiguity.
Depending on Frappe’s binding,this.get_value()may not be stable here.Proposed diff
@@ onchange: function () { const table_field = d.get_field("close_items"); - table_field.df.hidden = this.get_value(); + table_field.df.hidden = d.get_value("select_all"); table_field.refresh(); },erpnext/selling/doctype/sales_order/sales_order.py (1)
586-602: Avoid double reserved-qty updates on “Re-open” status transition.
update_status("Draft")callsclose_or_reopen_selected_items(...Re-open...)(which updates reserved qty) and then callsself.update_reserved_qty()again in theelse:branch.Proposed diff
@@ - if status == "Closed": + if status == "Closed": close_or_reopen_selected_items(self.name, "Close", all_items_closed=True) - else: + elif not (status == "Draft" and self.docstatus == 1): self.update_reserved_qty()
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
erpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/manufacturing/doctype/work_order/work_order.pyerpnext/patches.txterpnext/patches/v16_0/update_sales_order_item_status.pyerpnext/public/js/utils.jserpnext/selling/doctype/sales_order/sales_order.jserpnext/selling/doctype/sales_order/sales_order.pyerpnext/selling/doctype/sales_order/test_sales_order.pyerpnext/selling/doctype/sales_order_item/sales_order_item.jsonerpnext/selling/doctype/sales_order_item/sales_order_item.pyerpnext/selling/report/item_wise_sales_history/item_wise_sales_history.pyerpnext/selling/report/sales_order_analysis/sales_order_analysis.pyerpnext/stock/stock_balance.py
🚧 Files skipped from review as they are similar to previous changes (3)
- erpnext/selling/doctype/sales_order_item/sales_order_item.py
- erpnext/stock/stock_balance.py
- erpnext/manufacturing/doctype/work_order/work_order.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-30T11:04:46.510Z
Learnt from: rohitwaghchaure
Repo: frappe/erpnext PR: 49766
File: erpnext/manufacturing/doctype/production_plan/production_plan.py:1717-1717
Timestamp: 2025-09-30T11:04:46.510Z
Learning: In the Production Plan's `get_items_for_material_requests` function in `erpnext/manufacturing/doctype/production_plan/production_plan.py`, always use `data.get("sales_order")` instead of `doc.get("sales_order")` when iterating over `po_items`. This ensures raw materials are correctly grouped by each production item's respective Sales Order, not a global document-level Sales Order.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.pyerpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/selling/doctype/sales_order/sales_order.js
📚 Learning: 2025-08-12T22:10:55.921Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:929-953
Timestamp: 2025-08-12T22:10:55.921Z
Learning: In ERPNext stock management, there's a critical distinction between availability and reservation calculations: availability functions (like get_bundle_availability) should filter by current status (disabled=0) to determine what can be sold now, while reservation functions (like get_bundle_pos_reserved_qty) should include all historical transactions regardless of current bundle status to accurately reflect stock that was actually consumed. This prevents stock accounting errors when bundle configurations change after sales.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.py
📚 Learning: 2025-08-12T21:33:27.483Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:871-871
Timestamp: 2025-08-12T21:33:27.483Z
Learning: In ERPNext Product Bundle configurations, items can have qty = 0, which causes division by zero errors in POS bundle availability calculations. The fix is to use a high fallback value (like 1000000) instead of 0 when item.qty is zero, so these items don't constrain bundle availability.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.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/selling/doctype/sales_order/sales_order.pyerpnext/selling/doctype/sales_order/test_sales_order.pyerpnext/patches/v16_0/update_sales_order_item_status.pyerpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/selling/report/item_wise_sales_history/item_wise_sales_history.pyerpnext/selling/report/sales_order_analysis/sales_order_analysis.py
🧬 Code graph analysis (1)
erpnext/selling/doctype/sales_order/test_sales_order.py (2)
erpnext/selling/doctype/sales_order/sales_order.py (1)
close_or_reopen_selected_items(2113-2152)erpnext/controllers/accounts_controller.py (1)
update_child_qty_rate(3824-4211)
⏰ 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 (2)
- GitHub Check: Patch Test
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Summary
🔇 Additional comments (16)
erpnext/patches.txt (1)
459-460: LGTM!The patch
update_sales_order_item_statusis correctly placed in thepost_model_syncsection, ensuring theis_closedfield exists onSales Order Itembefore the data migration runs.erpnext/manufacturing/doctype/production_plan/production_plan.py (3)
371-371: LGTM!The filter correctly excludes closed items while handling NULL values for backward compatibility with existing data that hasn't been migrated yet.
436-436: LGTM!Consistent application of the
is_closedfilter for packed items, maintaining parity with the main items query.
1547-1547: LGTM!This correctly addresses the reviewer feedback to exclude closed items from production planning when fetching Sales Orders via "Get Finished Goods".
erpnext/public/js/utils.js (1)
613-637: LGTM!The implementation correctly filters out closed items from the "Update Items" dialog for Sales Orders only, preventing users from modifying closed items. The filter handles both
undefinedandfalsevalues appropriately with!d.is_closed.erpnext/selling/report/item_wise_sales_history/item_wise_sales_history.py (2)
169-169: LGTM!The
is_closedfield is correctly mapped from the query result to the report data row.
218-218: LGTM!The
is_closedfield is correctly added to the query SELECT clause to populate the report column.erpnext/selling/report/sales_order_analysis/sales_order_analysis.py (1)
81-82: Good: report rows now includedescription+is_closedfor non-grouped view.
This aligns the dataset with the visible columns and the new per-item close state.erpnext/selling/doctype/sales_order/test_sales_order.py (1)
23-32: Nice: direct import/use ofclose_or_reopen_selected_itemskeeps the test focused.erpnext/selling/doctype/sales_order/sales_order.js (2)
25-35: Good: closed-item indicator uses grey and avoids the “orange looks like red” problem.
1721-1736: Good: Purchase Order item picker excludesis_closedrows.erpnext/selling/doctype/sales_order/sales_order.py (5)
1087-1111: Good: Material Request mapping excludes closed SO items.
1225-1229: Good: Delivery Note mapping excludes closed SO items.
1465-1474: Good: Sales Invoice mapping excludes closed SO items (non-unit-price rows).
1914-1920: Good: Pick List generation skipsis_closeditems.
1996-2000: Good: Work Order item collection skips closed rows (and safely handles Packed Item viagetattr).
erpnext/selling/report/item_wise_sales_history/item_wise_sales_history.py
Show resolved
Hide resolved
nishkagosalia
force-pushed
the
gh-39030
branch
from
0772cc7 to
ed6a103
Compare
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 (3)
erpnext/selling/doctype/sales_order/sales_order.py (2)
1087-1112: Good: SO Item → Material Request mapping now excludes closed rows; but packed-items mapping may still leak closed bundle children.
If “closed item” is a Product Bundle parent, the"Packed Item"mapping here still has nois_closedguard (andget_remaining_packed_item_qty(...)doesn’t check it either).
1914-1920: Verify Packed Item exclusion for closed bundles in Pick List creation.The
should_pick_order_itemfilter correctly excludes closed Sales Order items from Pick Lists (line 1938). However,Packed Itemrows are mapped unconditionally without this filter (lines 1940–1948). This means closed Product Bundles still have their Packed Items included in Pick Lists, creating inconsistent behavior. Apply the sameshould_pick_order_itemcondition to the Packed Item mapping to ensure closed bundle parents are also excluded.erpnext/manufacturing/doctype/production_plan/production_plan.py (1)
1536-1549:validate_sales_orders()should filter closed SO items to matchget_so_items()behavior.The validation uses
sales_order_query()which lacks theis_closedfilter, butget_so_items()includes it at lines 376 and 433. If an SO item has remaining qty but is marked as closed, validation will pass while actual retrieval will skip it, causing inconsistent behavior. Add& ((so_item.is_closed == 0) | (so_item.is_closed.isnull()))to thesales_order_query()query.
🤖 Fix all issues with AI agents
In @erpnext/patches/v16_0/update_sales_order_item_status.py:
- Around line 4-18: The patch currently forces
frappe.db.auto_commit_on_many_writes = 1 and then resets it to 0
unconditionally; change this to save the original value (orig =
frappe.db.auto_commit_on_many_writes), set it to 1 for the update, and restore
it to orig in the finally block to avoid leaking state; also extend the update
WHERE clause on the Sales Order Item DocType (sales_order_item) to include rows
where is_closed is NULL in addition to is_closed == 0 so the condition becomes
(sales_order_item.is_closed == 0 OR sales_order_item.is_closed IS NULL) while
keeping the rest of the join/filters the same (refer to execute, sales_order,
sales_order_item, and frappe.qb.update).
🧹 Nitpick comments (7)
erpnext/selling/report/sales_order_analysis/sales_order_analysis.py (1)
330-335: Consider hidingIs Closedcolumn when grouping by Sales Order.The
Is Closedcolumn is added to the common columns section, meaning it appears even whengroup_by_sois enabled. However, when grouping by SO, individual item fields likeitem_code,description, andwarehouseare conditionally excluded (see lines 239-251 and 338-347).Since
is_closedis an item-level attribute, showing it in a grouped view could be misleading—the displayed value would be from the first item processed, not an aggregate of all items in the order.♻️ Suggested fix: Move is_closed column to non-grouped section
+ if not filters.get("group_by_so"): + columns.append( + { + "label": _("Is Closed"), + "fieldname": "is_closed", + "fieldtype": "Check", + "width": 80, + } + ) + columns.extend( + [ { - "label": _("Is Closed"), - "fieldname": "is_closed", - "fieldtype": "Check", - "width": 80, - }, - ] - ) - if not filters.get("group_by_so"): - columns.append( - { "label": _("Warehouse"),Alternatively, place the Is Closed column alongside other item-specific columns in the existing
if not filters.get("group_by_so")block around line 239.erpnext/manufacturing/doctype/production_plan/production_plan.py (1)
352-373: Consider usingIfNull(is_closed, 0) == 0instead of(is_closed = 0 OR is_closed IS NULL)for readability / query-planner friendliness.
Current logic is correct, but theOR ... IS NULLpattern can be avoided.Suggested diff
@@ .where( (so_item.parent.isin(so_list)) & (so_item.docstatus == 1) & ((so_item.stock_qty - so_item.stock_reserved_qty) > so_item.work_order_qty) - & ((so_item.is_closed == 0) | (so_item.is_closed.isnull())) + & (IfNull(so_item.is_closed, 0) == 0) )erpnext/selling/doctype/sales_order/sales_order.js (3)
24-38: Closed-item indicator works, but consider precedence: a closed + fully-delivered row will currently show green (not grey).
If the intent is “closed overrides all”, move thedoc.is_closedcheck above the delivered check.
1796-1869:reopen_selected_items(): consider short-circuiting when there are no closed items to display.
Right now it opens an empty dialog and then fails selection.
1871-1966:close_selected_items(): “Select all items” currently delegates to full SO close (OK), but consider clearer labeling.
Since it closes the whole Sales Order (not just remaining rows), wording like “Close Sales Order (all remaining items)” may reduce confusion.erpnext/selling/doctype/sales_order/sales_order.py (2)
586-599:update_status("Draft")likely does redundant credit-limit + reserved-qty work due to how it callsclose_or_reopen_selected_items.
close_or_reopen_selected_items(... all_items_closed=True)already updates reserved qty, and it currently checks credit limit inside the per-row loop.
2112-2152:close_or_reopen_selected_items(): movecheck_credit_limit()out of the item loop + validateselected_itemspresence.
As-is, reopening on a large SO can run credit-limit checks N times.Suggested diff
def close_or_reopen_selected_items(sales_order, status, selected_items=None, all_items_closed=False): items = [] so = frappe.get_doc("Sales Order", sales_order) @@ if not all_items_closed: selected_items = parse_json(selected_items) + if not selected_items: + frappe.throw(_("Please select at least one item")) items = {i["docname"] for i in selected_items} + if status == "Re-open" and so.docstatus == 1: + so.check_credit_limit() + for row in so.items: @@ elif status == "Re-open": - if so.docstatus == 1: - so.check_credit_limit() row.is_closed = 0
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
erpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/manufacturing/doctype/work_order/work_order.pyerpnext/patches.txterpnext/patches/v16_0/update_sales_order_item_status.pyerpnext/public/js/utils.jserpnext/selling/doctype/sales_order/sales_order.jserpnext/selling/doctype/sales_order/sales_order.pyerpnext/selling/doctype/sales_order/test_sales_order.pyerpnext/selling/doctype/sales_order_item/sales_order_item.jsonerpnext/selling/doctype/sales_order_item/sales_order_item.pyerpnext/selling/report/item_wise_sales_history/item_wise_sales_history.pyerpnext/selling/report/sales_order_analysis/sales_order_analysis.pyerpnext/stock/stock_balance.py
🚧 Files skipped from review as they are similar to previous changes (3)
- erpnext/patches.txt
- erpnext/selling/report/item_wise_sales_history/item_wise_sales_history.py
- erpnext/public/js/utils.js
🧰 Additional context used
🧠 Learnings (5)
📚 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/selling/doctype/sales_order_item/sales_order_item.pyerpnext/stock/stock_balance.pyerpnext/selling/doctype/sales_order/sales_order.pyerpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/selling/report/sales_order_analysis/sales_order_analysis.pyerpnext/selling/doctype/sales_order/test_sales_order.pyerpnext/manufacturing/doctype/work_order/work_order.pyerpnext/patches/v16_0/update_sales_order_item_status.py
📚 Learning: 2025-08-12T22:10:55.921Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:929-953
Timestamp: 2025-08-12T22:10:55.921Z
Learning: In ERPNext stock management, there's a critical distinction between availability and reservation calculations: availability functions (like get_bundle_availability) should filter by current status (disabled=0) to determine what can be sold now, while reservation functions (like get_bundle_pos_reserved_qty) should include all historical transactions regardless of current bundle status to accurately reflect stock that was actually consumed. This prevents stock accounting errors when bundle configurations change after sales.
Applied to files:
erpnext/stock/stock_balance.pyerpnext/selling/doctype/sales_order/sales_order.py
📚 Learning: 2025-09-30T11:04:46.510Z
Learnt from: rohitwaghchaure
Repo: frappe/erpnext PR: 49766
File: erpnext/manufacturing/doctype/production_plan/production_plan.py:1717-1717
Timestamp: 2025-09-30T11:04:46.510Z
Learning: In the Production Plan's `get_items_for_material_requests` function in `erpnext/manufacturing/doctype/production_plan/production_plan.py`, always use `data.get("sales_order")` instead of `doc.get("sales_order")` when iterating over `po_items`. This ensures raw materials are correctly grouped by each production item's respective Sales Order, not a global document-level Sales Order.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.pyerpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/selling/doctype/sales_order/sales_order.js
📚 Learning: 2025-08-12T21:33:27.483Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:871-871
Timestamp: 2025-08-12T21:33:27.483Z
Learning: In ERPNext Product Bundle configurations, items can have qty = 0, which causes division by zero errors in POS bundle availability calculations. The fix is to use a high fallback value (like 1000000) instead of 0 when item.qty is zero, so these items don't constrain bundle availability.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.py
📚 Learning: 2025-08-01T11:04:59.343Z
Learnt from: karm1000
Repo: frappe/erpnext PR: 48865
File: erpnext/accounts/doctype/purchase_invoice/purchase_invoice.json:1657-1664
Timestamp: 2025-08-01T11:04:59.343Z
Learning: In ERPNext/Frappe framework, virtual fields (marked with "is_virtual": 1) are always read-only by default and do not require an explicit "read_only": 1 property in their JSON definition.
Applied to files:
erpnext/selling/doctype/sales_order_item/sales_order_item.json
🧬 Code graph analysis (2)
erpnext/selling/doctype/sales_order/sales_order.js (3)
erpnext/accounts/doctype/sales_invoice/sales_invoice.js (4)
doc(866-866)d(1064-1118)me(75-75)me(409-409)erpnext/public/js/utils.js (2)
d(1014-1046)values(1028-1028)erpnext/stock/doctype/stock_entry/stock_entry.js (5)
d(363-396)d(417-417)d(727-730)d(745-745)values(732-732)
erpnext/selling/doctype/sales_order/test_sales_order.py (2)
erpnext/selling/doctype/sales_order/sales_order.py (1)
close_or_reopen_selected_items(2113-2152)erpnext/controllers/accounts_controller.py (1)
update_child_qty_rate(3824-4211)
⏰ 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 (12)
erpnext/selling/doctype/sales_order_item/sales_order_item.py (1)
50-50: LGTM!The type annotation correctly reflects the new
is_closedCheck field added to the Sales Order Item DocType.erpnext/manufacturing/doctype/work_order/work_order.py (1)
2393-2407: LGTM!The filter correctly excludes Closed Sales Orders from Work Order creation queries. This complements the item-level
is_closedfiltering implemented in other parts of the codebase (e.g., Production Plan, stock reservation logic).erpnext/selling/report/sales_order_analysis/sales_order_analysis.py (1)
81-82: LGTM!The
is_closedfield is correctly added to the query SELECT list to expose item-level closure status in the report data.erpnext/selling/doctype/sales_order_item/sales_order_item.json (1)
1014-1022: LGTM!The field definition is well-structured:
allow_on_submit: 1enables closing/reopening items on submitted Sales Ordersread_only: 1ensures the field is controlled programmatically via the API, preventing manual edits- No
in_list_viewproperty means the column won't appear in the grid view, addressing the reviewer feedback about removing the Is Closed column from the griderpnext/stock/stock_balance.py (1)
140-140: Addis_closed=0filter to Packed Items subqueries in the reserved quantity calculation.The Packed Items path (lines 104-116) retrieves parent Sales Order Item data via
parent_detail_docnamebut does not check theis_closedstatus of that parent item. When a Sales Order Item is closed, its packed items should not contribute to reserved quantity—consistent with how the Sales Order Items path is filtered at line 140. Update the subqueries fetchingso_item_qty,so_item_delivered_qty, andso_item_returned_qtyto includeand is_closed = 0in their WHERE clauses.⛔ Skipped due to learnings
Learnt from: LewisMojica Repo: frappe/erpnext PR: 49108 File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:929-953 Timestamp: 2025-08-12T22:10:55.921Z Learning: In ERPNext stock management, there's a critical distinction between availability and reservation calculations: availability functions (like get_bundle_availability) should filter by current status (disabled=0) to determine what can be sold now, while reservation functions (like get_bundle_pos_reserved_qty) should include all historical transactions regardless of current bundle status to accurately reflect stock that was actually consumed. This prevents stock accounting errors when bundle configurations change after sales.erpnext/manufacturing/doctype/production_plan/production_plan.py (1)
401-438: Good: packed-items fetch now respects parent SO item closure.
This aligns with the requirement that closed items shouldn’t flow into production planning.erpnext/selling/doctype/sales_order/test_sales_order.py (2)
23-32: Import is fine; keeps test using the public API entrypoint.
61-168: Solid end-to-end coverage for item-level close/reopen + reserved_qty.
This test exercises the main invariants (no closing delivered rows, no updates to closed rows, reservation recalculation, pick-list behavior).erpnext/selling/doctype/sales_order/sales_order.js (2)
1002-1013: Nice: item-level close/reopen actions are exposed under Status.
1721-1736: Good: Purchase Order selection excludes closed SO items.erpnext/selling/doctype/sales_order/sales_order.py (2)
1212-1230: Good: Delivery Note mapping now blocks closed SO items.
1464-1475: Good: Sales Invoice mapping now blocks closed SO items.
nishkagosalia
force-pushed
the
gh-39030
branch
from
a02572e to
dc7cfcb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @erpnext/patches/v16_0/update_sales_order_item_status.py:
- Around line 4-18: The code unconditionally resets
frappe.db.auto_commit_on_many_writes to 0 in execute(), which can clobber a
pre-existing value; capture the original value before changing it (e.g., prev =
frappe.db.auto_commit_on_many_writes), set it to 1 for the update block, and in
the finally block restore the original using
frappe.db.auto_commit_on_many_writes = prev; update references are in the
execute function and touch sales_order and sales_order_item DocTypes used in the
qb.update call.
In @erpnext/selling/doctype/sales_order/sales_order.py:
- Around line 2150-2155: The Sales Order closure branch sets so.status and calls
so.set_status/update_subcontracting_order_status/notify_update but omits calling
so.update_blanket_order(), causing inconsistency with
close_or_unclose_sales_orders; add a call to so.update_blanket_order() after the
status is set (e.g., immediately after so.set_status or right after
so.update_subcontracting_order_status()) and before/alongside
so.notify_update()/clear_doctype_notifications so blanket orders are updated
when all items are closed.
🧹 Nitpick comments (2)
erpnext/patches/v16_0/update_sales_order_item_status.py (1)
10-16: Redundant join condition in WHERE clause.The condition
sales_order.name == sales_order_item.parentis already specified in the.on()clause. It doesn't cause incorrect behavior, but it's redundant.Suggested simplification
try: frappe.qb.update(sales_order_item).join(sales_order).on( sales_order.name == sales_order_item.parent ).set(sales_order_item.is_closed, 1).where( - (sales_order.name == sales_order_item.parent) - & (sales_order.status == "Closed") + (sales_order.status == "Closed") & ((sales_order_item.is_closed == 0) | sales_order_item.is_closed.isnull()) ).run()erpnext/selling/doctype/sales_order/sales_order.py (1)
2134-2136: Condition may incorrectly allow closing fully delivered items whendelivered_qtyis 0.The condition
row.delivered_qty and row.qty == row.delivered_qtywill short-circuit toFalsewhendelivered_qtyis 0 (falsy), meaning items with 0 delivered qty won't trigger the throw. However, if the intent is to prevent closing items that are fully delivered, the condition should just compare equality.Suggested fix
- if row.delivered_qty and row.qty == row.delivered_qty and not all_items_closed: + if row.delivered_qty == row.qty and row.delivered_qty and not all_items_closed:Or if the intent is to skip already-delivered items silently rather than throw:
- if row.delivered_qty and row.qty == row.delivered_qty and not all_items_closed: - frappe.throw(_("Item cannot be closed as it is already delivered")) + if row.qty and row.delivered_qty >= row.qty and not all_items_closed: + continue # Skip already fully delivered items
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/patches/v16_0/update_sales_order_item_status.pyerpnext/selling/doctype/sales_order/sales_order.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-30T11:04:46.510Z
Learnt from: rohitwaghchaure
Repo: frappe/erpnext PR: 49766
File: erpnext/manufacturing/doctype/production_plan/production_plan.py:1717-1717
Timestamp: 2025-09-30T11:04:46.510Z
Learning: In the Production Plan's `get_items_for_material_requests` function in `erpnext/manufacturing/doctype/production_plan/production_plan.py`, always use `data.get("sales_order")` instead of `doc.get("sales_order")` when iterating over `po_items`. This ensures raw materials are correctly grouped by each production item's respective Sales Order, not a global document-level Sales Order.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.py
📚 Learning: 2025-08-12T22:10:55.921Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:929-953
Timestamp: 2025-08-12T22:10:55.921Z
Learning: In ERPNext stock management, there's a critical distinction between availability and reservation calculations: availability functions (like get_bundle_availability) should filter by current status (disabled=0) to determine what can be sold now, while reservation functions (like get_bundle_pos_reserved_qty) should include all historical transactions regardless of current bundle status to accurately reflect stock that was actually consumed. This prevents stock accounting errors when bundle configurations change after sales.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.py
📚 Learning: 2025-08-12T21:33:27.483Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:871-871
Timestamp: 2025-08-12T21:33:27.483Z
Learning: In ERPNext Product Bundle configurations, items can have qty = 0, which causes division by zero errors in POS bundle availability calculations. The fix is to use a high fallback value (like 1000000) instead of 0 when item.qty is zero, so these items don't constrain bundle availability.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.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/selling/doctype/sales_order/sales_order.pyerpnext/patches/v16_0/update_sales_order_item_status.py
🧬 Code graph analysis (1)
erpnext/selling/doctype/sales_order/sales_order.py (2)
erpnext/selling/doctype/customer/customer.py (1)
check_credit_limit(558-615)erpnext/stock/doctype/stock_entry/stock_entry.py (1)
update_subcontracting_order_status(3389-3395)
⏰ 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 (7)
erpnext/selling/doctype/sales_order/sales_order.py (7)
586-601: LGTM!The delegation to
close_or_reopen_selected_itemswithall_items_closed=Truecorrectly handles both Close and Re-open transitions for the entire Sales Order, ensuring item-levelis_closedflags are synchronized with the document-level status.
1106-1111: LGTM!Adding
not item.is_closedto the condition correctly prevents closed Sales Order items from being included in Material Requests.
1225-1229: LGTM!The condition correctly excludes closed items from Delivery Note creation while maintaining the existing checks for delivered quantity and drop-ship items.
1465-1474: LGTM!The condition correctly excludes closed items from Sales Invoice creation while preserving the special handling for unit price rows.
1914-1920: LGTM!Adding
not item.is_closedtoshould_pick_order_itemcorrectly prevents closed items from being included in Pick Lists.
1995-2004: LGTM!The logic correctly handles both packed items (inheriting closure state from their parent SO item via
so_items_is_closed_map) and regular items. This ensures closed items are excluded from Work Order creation.
2117-2156: Production Plan already respectsis_closedstatus correctly.The implementation is sound. The concern about Production Plan fetching closed items has been verified and is not an issue:
get_so_items()(line 371), packed items query (line 436), andget_sales_orders()(line 1547) all filter with((so_item.is_closed == 0) | (so_item.is_closed.isnull())), correctly excluding closed items.The visual indicator for closed items is a separate UI concern outside the scope of this backend implementation.
nishkagosalia
force-pushed
the
gh-39030
branch
from
dc7cfcb to
ec29217
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @erpnext/patches/v16_0/update_sales_order_item_status.py:
- Around line 4-17: In execute(), save the original
frappe.db.auto_commit_on_many_writes to a local variable before changing it,
then restore that saved value in the finally block so you don't unconditionally
overwrite callers' setting; also remove the redundant sales_order.name ==
sales_order_item.parent predicate from the .where(...) since that join condition
is already expressed in .join(...).on(...); update the code locations around
execute(), the frappe.db.auto_commit_on_many_writes assignment, and the
frappe.qb.update(...).join(...).set(...).where(...).run() call accordingly.
In @erpnext/selling/doctype/sales_order/sales_order.js:
- Around line 1796-1869: In reopen_selected_items(), the mapped object sets a
typo key "is_close" instead of the correct "is_closed"; update the mapping
inside reopen_selected_items() (the this.data =
this.frm.doc.items.filter(...).map(...) block) to use is_closed: d.is_closed so
the dialog data keys match the actual field name and avoid future lookup bugs.
- Around line 1871-1966: In close_selected_items(), the mapped data uses the
wrong property name is_close instead of is_closed; update the object returned in
close_selected_items() (the mapping that builds this.data) to use is_closed:
d.is_closed (matching reopen_selected_items and the rest of the code) so
selection/filtering and table behavior work correctly.
🧹 Nitpick comments (2)
erpnext/selling/report/sales_order_analysis/sales_order_analysis.py (1)
330-335: Consider excludingis_closedcolumn when grouping by Sales Order.The
is_closedcolumn is added within thecolumns.extend()block, which means it appears even whengroup_by_sois enabled. When rows are aggregated at the Sales Order level, displaying a per-itemis_closedcheckbox doesn't provide meaningful information since different items may have different closure states.Suggested fix: Move column to non-grouped section
columns.append( {"label": _("Description"), "fieldname": "description", "fieldtype": "Small Text", "width": 100} ) + columns.append( + { + "label": _("Is Closed"), + "fieldname": "is_closed", + "fieldtype": "Check", + "width": 80, + } + ) columns.extend( [ # ... other columns ... { "label": _("Time Taken to Deliver"), "fieldname": "time_taken_to_deliver", "fieldtype": "Duration", "width": 100, }, - { - "label": _("Is Closed"), - "fieldname": "is_closed", - "fieldtype": "Check", - "width": 80, - }, ] )erpnext/selling/doctype/sales_order/sales_order.js (1)
1846-1850: Inconsistent user message: should require "at least one item" not "one item".The message "Please select one item to re-open" is slightly misleading since users can select multiple items. Consider "Please select at least one item to re-open" for consistency with similar patterns elsewhere in the codebase.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
erpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/manufacturing/doctype/work_order/work_order.pyerpnext/patches.txterpnext/patches/v16_0/update_sales_order_item_status.pyerpnext/public/js/utils.jserpnext/selling/doctype/sales_order/sales_order.jserpnext/selling/doctype/sales_order/sales_order.pyerpnext/selling/doctype/sales_order/test_sales_order.pyerpnext/selling/doctype/sales_order_item/sales_order_item.jsonerpnext/selling/doctype/sales_order_item/sales_order_item.pyerpnext/selling/report/item_wise_sales_history/item_wise_sales_history.pyerpnext/selling/report/sales_order_analysis/sales_order_analysis.pyerpnext/stock/stock_balance.py
🚧 Files skipped from review as they are similar to previous changes (6)
- erpnext/selling/doctype/sales_order_item/sales_order_item.json
- erpnext/stock/stock_balance.py
- erpnext/selling/report/item_wise_sales_history/item_wise_sales_history.py
- erpnext/manufacturing/doctype/work_order/work_order.py
- erpnext/patches.txt
- erpnext/selling/doctype/sales_order_item/sales_order_item.py
🧰 Additional context used
🧠 Learnings (4)
📚 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/patches/v16_0/update_sales_order_item_status.pyerpnext/selling/report/sales_order_analysis/sales_order_analysis.pyerpnext/selling/doctype/sales_order/test_sales_order.pyerpnext/selling/doctype/sales_order/sales_order.pyerpnext/manufacturing/doctype/production_plan/production_plan.py
📚 Learning: 2025-09-30T11:04:46.510Z
Learnt from: rohitwaghchaure
Repo: frappe/erpnext PR: 49766
File: erpnext/manufacturing/doctype/production_plan/production_plan.py:1717-1717
Timestamp: 2025-09-30T11:04:46.510Z
Learning: In the Production Plan's `get_items_for_material_requests` function in `erpnext/manufacturing/doctype/production_plan/production_plan.py`, always use `data.get("sales_order")` instead of `doc.get("sales_order")` when iterating over `po_items`. This ensures raw materials are correctly grouped by each production item's respective Sales Order, not a global document-level Sales Order.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.pyerpnext/selling/doctype/sales_order/sales_order.jserpnext/manufacturing/doctype/production_plan/production_plan.py
📚 Learning: 2025-08-12T22:10:55.921Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:929-953
Timestamp: 2025-08-12T22:10:55.921Z
Learning: In ERPNext stock management, there's a critical distinction between availability and reservation calculations: availability functions (like get_bundle_availability) should filter by current status (disabled=0) to determine what can be sold now, while reservation functions (like get_bundle_pos_reserved_qty) should include all historical transactions regardless of current bundle status to accurately reflect stock that was actually consumed. This prevents stock accounting errors when bundle configurations change after sales.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.py
📚 Learning: 2025-08-12T21:33:27.483Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:871-871
Timestamp: 2025-08-12T21:33:27.483Z
Learning: In ERPNext Product Bundle configurations, items can have qty = 0, which causes division by zero errors in POS bundle availability calculations. The fix is to use a high fallback value (like 1000000) instead of 0 when item.qty is zero, so these items don't constrain bundle availability.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.py
🧬 Code graph analysis (2)
erpnext/selling/doctype/sales_order/test_sales_order.py (1)
erpnext/selling/doctype/sales_order/sales_order.py (3)
close_or_reopen_selected_items(2118-2157)make_delivery_note(1149-1320)create_pick_list(1880-1958)
erpnext/selling/doctype/sales_order/sales_order.js (1)
erpnext/stock/doctype/stock_entry/stock_entry.js (5)
d(363-396)d(417-417)d(727-730)d(745-745)values(732-732)
⏰ 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 (2)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (19)
erpnext/manufacturing/doctype/production_plan/production_plan.py (3)
371-371: LGTM - Correctly excludes closed items from production planning.The filter
(so_item.is_closed == 0) | (so_item.is_closed.isnull())properly handles both explicitly open items and legacy items whereis_closedmay be NULL, ensuring backward compatibility.
436-436: LGTM - Consistent filtering for packed items.The same
is_closedfilter pattern is correctly applied to the packed items query, ensuring closed parent items don't contribute packed items to production planning.
1547-1547: LGTM - Open sales orders query correctly excludes closed items.This ensures the "Get Open Sales Orders" feature respects item-level closure, addressing the reviewer feedback about Production Plan's "Get Finished Goods" fetching closed items.
erpnext/public/js/utils.js (1)
613-637: LGTM - Properly filters closed items from the Update Items dialog.The implementation correctly:
- Scopes the filter to Sales Orders only, leaving other doctypes unaffected
- Uses a truthiness check that handles both
is_closed = 0andundefined/null- Aligns with the PR requirement that closed items cannot be updated
erpnext/selling/doctype/sales_order/test_sales_order.py (2)
25-25: LGTM - Import added for the new API.
61-167: Comprehensive test coverage for item-level closing.The test effectively validates:
- Cannot close a fully delivered item (raises ValidationError)
- Closing an item zeroes its
reserved_qtyin Bin- Closed items cannot be updated via
update_child_qty_rate- Pick list creation excludes closed items
- Reopening restores
reserved_qtyConsider adding an assertion to verify
so.items[1].is_closed == 1after closing to explicitly confirm the field state.Optional enhancement
close_or_reopen_selected_items(so.name, "Close", json.dumps([{"docname": so.items[1].name}])) so.reload() + self.assertEqual(so.items[1].is_closed, 1) self.assertEqual( frappe.get_all(erpnext/selling/report/sales_order_analysis/sales_order_analysis.py (1)
81-82: LGTM -is_closedfield correctly added to data query.The field is properly selected alongside other item-level fields, making it available for the report data.
erpnext/selling/doctype/sales_order/sales_order.js (4)
25-38: Indicator precedence issue: fully delivered items will never show grey.The condition check order means
is_closed(grey) will never be reached for items wherestock_qty <= delivered_qty, as that condition comes first. This might be intentional (delivered items show green even if later closed), but per PR comments requesting a visual indicator for closed items, you may want closed items to always appear grey regardless of delivery status.Consider reordering if closed should take precedence:
🔧 Suggested fix
frm.set_indicator_formatter("item_code", function (doc) { let color; if (!doc.qty && frm.doc.has_unit_price_items) { color = "yellow"; + } else if (doc.is_closed) { + color = "grey"; } else if (doc.stock_qty <= doc.delivered_qty) { color = "green"; - } else if (doc.is_closed) { - color = "grey"; } else { color = "orange"; } return color; });
1001-1012: LGTM!The custom buttons for closing and reopening selected items are correctly placed within the status button group and only appear when appropriate (submitted SO, not closed/on-hold, and per_delivered/per_billed < 100).
1726-1726: LGTM!Correctly filters out closed items when populating Purchase Order items dialog, ensuring closed items cannot be ordered.
1933-1943: Dual path to close entire order may cause confusion.When
select_allis checked OR all items are manually selected, the code callsme.close_sales_order()which closes the entire SO (setting status to "Closed"). This differs from closing individual items (settingis_closedper item). Ensure this is the intended behavior—users might expect "Close selected items" with all items selected to close items individually while keeping the SO open.erpnext/selling/doctype/sales_order/sales_order.py (8)
593-598: Logic issue:update_reserved_qtyskipped when status is "Closed".The
elseon line 597 meansupdate_reserved_qtyis only called when status is NOT "Closed". However, when closing the entire SO, you still need to update reserved quantities (to release them). Theclose_or_reopen_selected_itemsfunction does callupdate_reserved_qty, but only after this method returns. If the flow relies onclose_or_reopen_selected_itemsto handle it, this seems okay, but the control flow is confusing.Additionally, when
status == "Closed", the code callsclose_or_reopen_selected_itemswithall_items_closed=True, but if items were already individually closed before calling "Close" on the SO, this would redundantly set them again.
1106-1110: LGTM!Correctly excludes closed items from Material Request mapping by adding
and not item.is_closedto the condition.
1225-1229: LGTM!Correctly excludes closed items from Delivery Note creation by adding
and not doc.is_closedto the condition function.
1465-1474: LGTM!Correctly excludes closed items from Sales Invoice mapping. The condition properly handles unit price rows while also checking for
is_closed.
1914-1920: LGTM!Correctly excludes closed items from Pick List creation by adding
and not item.is_closedto the condition.
1995-2004: LGTM!Correctly handles closed items in Work Order item retrieval. The logic properly checks
is_closedfor both direct SO items and packed items (via theso_items_is_closed_maplookup).
2140-2142: Credit limit check called during re-open but not during close.The credit limit check on line 2141 is good for re-opening, but consider whether closing individual items should also verify that the remaining open items don't violate any business rules.
2150-2155: Auto-closing SO when all items are individually closed is a good UX touch.This correctly transitions the SO to "Closed" status when all items have been individually closed, maintaining consistency with the overall SO state. The
update_subcontracting_order_statusand notification calls ensure related records are updated.
nishkagosalia
force-pushed
the
gh-39030
branch
from
ec29217 to
d8db98d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
erpnext/patches/v16_0/update_sales_order_item_status.py (1)
5-18: Preserve the originalauto_commit_on_many_writesvalue.The
finallyblock unconditionally setsauto_commit_on_many_writes = 0, which may not restore the original state if it was already enabled before this patch ran.Additionally, the condition on line 13 (
sales_order.name == sales_order_item.parent) duplicates the join clause on line 11 and can be removed.Suggested fix
def execute(): + original_auto_commit = frappe.db.auto_commit_on_many_writes frappe.db.auto_commit_on_many_writes = 1 sales_order = frappe.qb.DocType("Sales Order") sales_order_item = frappe.qb.DocType("Sales Order Item") try: frappe.qb.update(sales_order_item).join(sales_order).on( sales_order.name == sales_order_item.parent ).set(sales_order_item.is_closed, 1).where( - (sales_order.name == sales_order_item.parent) - & (sales_order.status == "Closed") + (sales_order.status == "Closed") & ((sales_order_item.is_closed == 0) | sales_order_item.is_closed.isnull()) ).run() finally: - frappe.db.auto_commit_on_many_writes = 0 + frappe.db.auto_commit_on_many_writes = original_auto_commit
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
erpnext/patches/v16_0/update_sales_order_item_status.pyerpnext/selling/doctype/sales_order/sales_order.jserpnext/selling/doctype/sales_order/sales_order.py
🚧 Files skipped from review as they are similar to previous changes (1)
- erpnext/selling/doctype/sales_order/sales_order.js
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-30T11:04:46.510Z
Learnt from: rohitwaghchaure
Repo: frappe/erpnext PR: 49766
File: erpnext/manufacturing/doctype/production_plan/production_plan.py:1717-1717
Timestamp: 2025-09-30T11:04:46.510Z
Learning: In the Production Plan's `get_items_for_material_requests` function in `erpnext/manufacturing/doctype/production_plan/production_plan.py`, always use `data.get("sales_order")` instead of `doc.get("sales_order")` when iterating over `po_items`. This ensures raw materials are correctly grouped by each production item's respective Sales Order, not a global document-level Sales Order.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.py
📚 Learning: 2025-08-12T22:10:55.921Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:929-953
Timestamp: 2025-08-12T22:10:55.921Z
Learning: In ERPNext stock management, there's a critical distinction between availability and reservation calculations: availability functions (like get_bundle_availability) should filter by current status (disabled=0) to determine what can be sold now, while reservation functions (like get_bundle_pos_reserved_qty) should include all historical transactions regardless of current bundle status to accurately reflect stock that was actually consumed. This prevents stock accounting errors when bundle configurations change after sales.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.py
📚 Learning: 2025-08-12T21:33:27.483Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:871-871
Timestamp: 2025-08-12T21:33:27.483Z
Learning: In ERPNext Product Bundle configurations, items can have qty = 0, which causes division by zero errors in POS bundle availability calculations. The fix is to use a high fallback value (like 1000000) instead of 0 when item.qty is zero, so these items don't constrain bundle availability.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.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/selling/doctype/sales_order/sales_order.pyerpnext/patches/v16_0/update_sales_order_item_status.py
🧬 Code graph analysis (1)
erpnext/selling/doctype/sales_order/sales_order.py (3)
erpnext/buying/doctype/purchase_order/purchase_order.py (2)
can_update_items(614-623)update_subcontracting_order_status(645-654)erpnext/stock/doctype/stock_entry/stock_entry.py (1)
update_subcontracting_order_status(3389-3395)erpnext/subcontracting/doctype/subcontracting_order/subcontracting_order.py (1)
update_subcontracting_order_status(521-525)
⏰ 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 (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (7)
erpnext/selling/doctype/sales_order/sales_order.py (7)
586-601: LGTM!The
update_statusmethod correctly delegates close/reopen operations toclose_or_reopen_selected_itemswithall_items_closed=Truefor whole-order transitions, while preserving reserved quantity updates for other status changes.
1106-1111: LGTM!Correctly excludes closed items from Material Request creation by adding the
not item.is_closedcondition.
1225-1229: LGTM!Correctly excludes closed items from Delivery Note creation.
1465-1475: LGTM!Correctly excludes closed items from Sales Invoice creation by adding the
not doc.is_closedcondition.
1914-1920: LGTM!Correctly excludes closed items from Pick List creation.
1995-2005: LGTM!The implementation correctly handles both direct SO items and packed items:
- Direct items are checked via their own
is_closedflag- Packed items inherit closure status from their parent SO item via
parent_detail_docname
1979-2041: Production Plan integration already properly excludes closed items.Verification confirms that Production Plan's
get_so_items()andget_open_sales_orders()functions inerpnext/manufacturing/doctype/production_plan/production_plan.pyalready include theis_closedflag check at multiple points (lines 371, 436, and 1547). All queries correctly filter out closed Sales Order Items using((so_item.is_closed == 0) | (so_item.is_closed.isnull()))conditions. No further action required.
Will implement separately not in this PR |
nishkagosalia
force-pushed
the
gh-39030
branch
from
d8db98d to
2215efa
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
erpnext/selling/doctype/sales_order/sales_order.py (3)
1015-1116: Material Request: Packed Items still map even if the parent SO item is closed.You added
and not item.is_closedfor"Sales Order Item"mapping, but"Packed Item"mapping has no check against the parent Sales Order Item’sis_closed. For product bundles, this can still create MR lines for a “closed” bundle via its packed items, violating the PR objective “closed items do not reflect in other doctypes created from the Sales Order”.Fix idea: precompute a
closed_so_item_namesset once inmake_material_request()(bysource_name) and then add a condition for packed items likeitem.parent_detail_docname not in closed_so_item_names.
1458-1475: Sales Invoice: unit-price rows bypassnot doc.is_closed(closed items can still invoice).Current condition is effectively:
- if unit-price row → always map (even if closed)
- else → map only if
not doc.is_closedIf
is_closedis meant to suppress all downstream creation, applynot doc.is_closedto unit-price rows too.Suggested rewrite
- "condition": lambda doc: ( - True - if is_unit_price_row(doc) - else ( - doc.qty - and (doc.base_amount == 0 or abs(doc.billed_amt) < abs(doc.amount)) - and not doc.is_closed - ) - ) - and select_item(doc), + "condition": lambda doc: ( + (is_unit_price_row(doc) or ( + doc.qty + and (doc.base_amount == 0 or abs(doc.billed_amt) < abs(doc.amount)) + )) + and not doc.is_closed + and select_item(doc) + ),
1880-1959: Pick List: Packed Items mapping likely needs parent-itemis_closedgating too.You added
and not item.is_closedinshould_pick_order_item()(Sales Order Item mapping), but"Packed Item"mapping has no condition. For product bundles (where the parent item itself won’t map due tois_product_bundle()), packed items can still be added to the Pick List even if the parent Sales Order Item is closed.Suggested pattern (similar to
get_work_order_items): build aso_items_is_closed_map(or aclosed_so_item_namesset) and add a"condition"for"Packed Item"mapping like:
not so_items_is_closed_map.get(doc.parent_detail_docname)- and the existing quantity math implies pending > 0
🤖 Fix all issues with AI agents
In @erpnext/patches/v16_0/update_sales_order_item_status.py:
- Around line 1-18: The finally block currently forces
frappe.db.auto_commit_on_many_writes = 0 which can clobber a pre-existing
setting; update the execute() function to save the current value (e.g., old_val
= frappe.db.auto_commit_on_many_writes) before setting it to 1, and in the
finally block restore it (frappe.db.auto_commit_on_many_writes = old_val) so the
original state is preserved; reference the execute function and the
frappe.db.auto_commit_on_many_writes variable when making the change.
In @erpnext/selling/doctype/sales_order/sales_order.js:
- Around line 1796-1869: The reopen_selected_items method opens a dialog even
when this.data is empty and shows a misleading message "Please select one item
to re-open"; add a guard at the start of reopen_selected_items to check if
this.data is empty and call frappe.msgprint(__("No closed items to re-open"))
and return, and update the validation in the dialog primary_action (where it
checks selected_items.length) to use __("Please select at least one item to
re-open") instead of "select one item" so the wording matches multiple
selection.
- Around line 1871-1966: The close_selected_items() handler conflates item-level
closure with full order closure: when "select_all" is checked or when user
selects all table rows the code calls close_sales_order() which closes the
entire Sales Order instead of marking each item's is_closed; fix by: 1) add a
guard at the start of close_selected_items() to handle empty this.data and show
a message if there are no closable items; 2) change the primary_action logic in
close_selected_items() so that when values.select_all is true (or when
selected_items covers all rows) you either loop over this.data and call the
server method to set is_closed for each item (invoking
erpnext.selling.doctype.sales_order.sales_order.close_or_reopen_selected_items
with status "Close" and the full item list) OR prompt the user with a clear
confirmation that closing all items will close the entire order and then call
close_sales_order() only if they confirm; 3) update the validation message text
from "Please select one item to close" to "Please select at least one item to
close" and ensure you reference the dialog instance d, the table field
"close_items", and the server method name close_or_reopen_selected_items when
making the changes.
🧹 Nitpick comments (5)
erpnext/manufacturing/doctype/work_order/work_order.py (1)
2393-2407: Consider filtering out “On Hold” Sales Orders too (UI/UX consistency).
WorkOrder.check_sales_order_on_hold_or_close()blocks both “Closed” and “On Hold”, butquery_sales_order()now only excludes “Closed”, so users can still pick an “On Hold” SO and then hit an avoidable validation error later. Consider excluding both here.Proposed tweak
filters=[ ["Sales Order", "docstatus", "=", 1], - ["Sales Order", "status", "!=", "Closed"], + ["Sales Order", "status", "not in", ("Closed", "On Hold")], ],erpnext/selling/doctype/sales_order/sales_order.py (2)
586-602: Potential double/looped credit-limit checks on “Re-open” (do it once).
update_status()already callsself.check_credit_limit()beforeclose_or_reopen_selected_items(..., "Re-open", all_items_closed=True), butclose_or_reopen_selected_items()also callsso.check_credit_limit()inside the row loop. That’s redundant and can run N times for N items.Consider moving the credit-limit check to a single pre-loop check inside
close_or_reopen_selected_items()whenstatus == "Re-open" and so.docstatus == 1.Also applies to: 2117-2160
2117-2160: Hardenclose_or_reopen_selected_items()input validation and delivered-qty comparisons.A couple of edge cases worth tightening:
- If
all_items_closedis false andselected_itemsisNone/empty,parse_json(selected_items)can throw a non-obvious error.- The “already delivered” check uses
row.qty == row.delivered_qty; elsewhere you commonly useabs(...)for safety with returns/negative flows.Small defensive tweaks (illustrative):
Possible hardening
def close_or_reopen_selected_items(sales_order, status, selected_items=None, all_items_closed=False): + if status not in ("Close", "Re-open"): + frappe.throw(_("Invalid status")) if not frappe.has_permission("Sales Order", "write"): frappe.throw(_("Not permitted"), frappe.PermissionError) items = [] so = frappe.get_doc("Sales Order", sales_order) # check if subcontracted so exists against the SO if not so.can_update_items(): frappe.throw(_("Cannot close items in a subcontracted Sales Order")) if not all_items_closed: + if not selected_items: + frappe.throw(_("Please select at least one item")) selected_items = parse_json(selected_items) items = {i["docname"] for i in selected_items} + if status == "Re-open" and so.docstatus == 1: + so.check_credit_limit() + for row in so.items: if not all_items_closed and row.name not in items: continue if status == "Close": - if row.delivered_qty and row.qty == row.delivered_qty and not all_items_closed: + if row.delivered_qty and abs(row.qty) == abs(row.delivered_qty) and not all_items_closed: frappe.throw(_("Item cannot be closed as it is already delivered"))erpnext/selling/doctype/sales_order/test_sales_order.py (1)
61-167: Solid integration coverage for item-level close/reopen + reserved qty behavior.One gap to consider adding (if product bundles are in scope for this feature): closing a bundle parent should prevent downstream creation for its packed items (e.g., Pick List / Material Request).
erpnext/selling/doctype/sales_order/sales_order.js (1)
1001-1012: Consider conditionally showing buttons based on item availability.The buttons are always shown regardless of whether closable/reopenable items exist. While the dialogs handle empty states, it may improve UX to conditionally display:
- "Close selected items" only when there are items with
qty > delivered_qty && !is_closed- "Re-open selected items" only when there are items with
is_closed♻️ Optional: Conditional button visibility
+ const hasClosableItems = this.frm.doc.items.some( + (item) => item.qty > flt(item.delivered_qty) && !item.is_closed + ); + const hasClosedItems = this.frm.doc.items.some((item) => item.is_closed); - this.frm.add_custom_button( - __("Close selected items"), - () => this.close_selected_items(), - __("Status") - ); - - this.frm.add_custom_button( - __("Re-open selected items"), - () => this.reopen_selected_items(), - __("Status") - ); + if (hasClosableItems) { + this.frm.add_custom_button( + __("Close selected items"), + () => this.close_selected_items(), + __("Status") + ); + } + + if (hasClosedItems) { + this.frm.add_custom_button( + __("Re-open selected items"), + () => this.reopen_selected_items(), + __("Status") + ); + }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
erpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/manufacturing/doctype/work_order/work_order.pyerpnext/patches.txterpnext/patches/v16_0/update_sales_order_item_status.pyerpnext/public/js/utils.jserpnext/selling/doctype/sales_order/sales_order.jserpnext/selling/doctype/sales_order/sales_order.pyerpnext/selling/doctype/sales_order/test_sales_order.pyerpnext/selling/doctype/sales_order_item/sales_order_item.jsonerpnext/selling/doctype/sales_order_item/sales_order_item.pyerpnext/selling/report/item_wise_sales_history/item_wise_sales_history.pyerpnext/selling/report/sales_order_analysis/sales_order_analysis.pyerpnext/stock/stock_balance.py
🚧 Files skipped from review as they are similar to previous changes (7)
- erpnext/public/js/utils.js
- erpnext/manufacturing/doctype/production_plan/production_plan.py
- erpnext/selling/doctype/sales_order_item/sales_order_item.json
- erpnext/selling/report/sales_order_analysis/sales_order_analysis.py
- erpnext/stock/stock_balance.py
- erpnext/patches.txt
- erpnext/selling/report/item_wise_sales_history/item_wise_sales_history.py
🧰 Additional context used
🧠 Learnings (4)
📚 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/selling/doctype/sales_order/test_sales_order.pyerpnext/selling/doctype/sales_order/sales_order.pyerpnext/manufacturing/doctype/work_order/work_order.pyerpnext/selling/doctype/sales_order_item/sales_order_item.pyerpnext/patches/v16_0/update_sales_order_item_status.py
📚 Learning: 2025-09-30T11:04:46.510Z
Learnt from: rohitwaghchaure
Repo: frappe/erpnext PR: 49766
File: erpnext/manufacturing/doctype/production_plan/production_plan.py:1717-1717
Timestamp: 2025-09-30T11:04:46.510Z
Learning: In the Production Plan's `get_items_for_material_requests` function in `erpnext/manufacturing/doctype/production_plan/production_plan.py`, always use `data.get("sales_order")` instead of `doc.get("sales_order")` when iterating over `po_items`. This ensures raw materials are correctly grouped by each production item's respective Sales Order, not a global document-level Sales Order.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.jserpnext/selling/doctype/sales_order/sales_order.py
📚 Learning: 2025-08-12T22:10:55.921Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:929-953
Timestamp: 2025-08-12T22:10:55.921Z
Learning: In ERPNext stock management, there's a critical distinction between availability and reservation calculations: availability functions (like get_bundle_availability) should filter by current status (disabled=0) to determine what can be sold now, while reservation functions (like get_bundle_pos_reserved_qty) should include all historical transactions regardless of current bundle status to accurately reflect stock that was actually consumed. This prevents stock accounting errors when bundle configurations change after sales.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.py
📚 Learning: 2025-08-12T21:33:27.483Z
Learnt from: LewisMojica
Repo: frappe/erpnext PR: 49108
File: erpnext/accounts/doctype/pos_invoice/pos_invoice.py:871-871
Timestamp: 2025-08-12T21:33:27.483Z
Learning: In ERPNext Product Bundle configurations, items can have qty = 0, which causes division by zero errors in POS bundle availability calculations. The fix is to use a high fallback value (like 1000000) instead of 0 when item.qty is zero, so these items don't constrain bundle availability.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.py
🧬 Code graph analysis (1)
erpnext/selling/doctype/sales_order/test_sales_order.py (1)
erpnext/selling/doctype/sales_order/sales_order.py (1)
close_or_reopen_selected_items(2118-2160)
⏰ 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 (4)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Summary
🔇 Additional comments (6)
erpnext/selling/doctype/sales_order_item/sales_order_item.py (1)
9-103: Type hint foris_closedlooks good.This is a clean, low-risk typing addition that matches the new field and helps static analysis.
erpnext/selling/doctype/sales_order/sales_order.py (2)
1212-1229: DN item gating bynot doc.is_closedlooks correct.This keeps closed lines out of Delivery Note creation without changing the existing delivery-date filters.
1979-2042: Closed-item skipping inget_work_order_items()(incl. packed items) is a good approach.The
so_items_is_closed_mapcheck for packed items is the right shape to prevent closed parent lines from leaking into Work Order planning.erpnext/selling/doctype/sales_order/test_sales_order.py (1)
23-32: Import update is fine.erpnext/selling/doctype/sales_order/sales_order.js (2)
29-35: LGTM! Indicator priority is correct.The condition order correctly prioritizes showing green for fully delivered items before checking
is_closed. Closed items that aren't fully delivered will correctly show grey. This aligns with the PR comment requesting grey as the closed-item indicator.
1726-1735: LGTM!Correctly excludes closed items from the Purchase Order items dialog, aligning with the PR requirement that closed items should not reflect in other doctypes.
nishkagosalia
force-pushed
the
gh-39030
branch
from
2215efa to
afa823d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @erpnext/selling/doctype/sales_order/sales_order.js:
- Around line 1850-1854: Fix the typo in the user message shown when no items
are selected: in the block that builds selected_items (variable selected_items)
and calls frappe.msgprint, update the string from "Please select atleast one
item to re-open" to "Please select at least one item to re-open" so the message
uses the correct "at least" phrasing.
🧹 Nitpick comments (3)
erpnext/manufacturing/doctype/production_plan/production_plan.py (1)
2144-2151: Consider addingis_closedfilter for consistency.The
sales_order_queryfunction is used byvalidate_sales_ordersbut doesn't filter out closed items. This could allow a Sales Order with only closed items to pass validation, but thenget_so_itemsreturns nothing since it now filters byis_closed == 0.This may be related to the reported issue where "Get Finished Goods" fetched closed items when a Sales Order was added manually.
Suggested fix
query = ( frappe.qb.from_(so_table) .join(table) .on(table.parent == so_table.name) .select(table.parent) .distinct() - .where((table.qty > table.production_plan_qty) & (table.docstatus == 1)) + .where((table.qty > table.production_plan_qty) & (table.docstatus == 1) & (table.is_closed == 0)) )erpnext/selling/doctype/sales_order/sales_order.js (2)
1001-1012: Consider conditionally showing "Re-open selected items" button only when closed items exist.The button is always visible when the SO is open, but if no items are closed, clicking it just shows a message. For better UX, you could conditionally render it:
♻️ Optional: Conditionally show re-open button
this.frm.add_custom_button( __("Close selected items"), () => this.close_selected_items(), __("Status") ); +const has_closed_items = this.frm.doc.items.some((item) => item.is_closed); +if (has_closed_items) { this.frm.add_custom_button( __("Re-open selected items"), () => this.reopen_selected_items(), __("Status") ); +}
1913-1922: The "Select all items" checkbox behavior may confuse users.When
select_allis checked (the default), the table is hidden and clicking "Close" will close the entire Sales Order (not just the items). The dialog title "Close Selected Items" may mislead users into thinking they're only closing specific items.Consider either:
- Renaming the checkbox to something clearer like "Close entire Sales Order"
- Or changing the default to unchecked so users explicitly see the items they're closing
♻️ Suggested: Clarify the select_all checkbox intent
{ fieldname: "select_all", fieldtype: "Check", - label: __("Select all items"), - default: 1, + label: __("Close entire Sales Order"), + default: 0, onchange: function () { const table_field = d.get_field("close_items"); - table_field.df.hidden = this.get_value(); + table_field.df.hidden = !this.get_value(); table_field.refresh(); }, },
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/selling/doctype/sales_order/sales_order.js
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-30T11:04:46.510Z
Learnt from: rohitwaghchaure
Repo: frappe/erpnext PR: 49766
File: erpnext/manufacturing/doctype/production_plan/production_plan.py:1717-1717
Timestamp: 2025-09-30T11:04:46.510Z
Learning: In the Production Plan's `get_items_for_material_requests` function in `erpnext/manufacturing/doctype/production_plan/production_plan.py`, always use `data.get("sales_order")` instead of `doc.get("sales_order")` when iterating over `po_items`. This ensures raw materials are correctly grouped by each production item's respective Sales Order, not a global document-level Sales Order.
Applied to files:
erpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/selling/doctype/sales_order/sales_order.js
📚 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/manufacturing/doctype/production_plan/production_plan.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: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (7)
erpnext/manufacturing/doctype/production_plan/production_plan.py (3)
367-372: LGTM — Closed items correctly excluded from main SO items query.The
is_closed == 0filter is properly added to exclude closed Sales Order Items from production planning.
429-437: LGTM — Closed items correctly excluded from packed items query.Consistent with the main items query filter, ensuring packed items from closed Sales Order Items are also excluded from production planning.
1540-1548: LGTM — Closed items correctly excluded from open sales orders retrieval.The filter ensures that Sales Orders are only pulled when they have non-closed items available for production.
erpnext/selling/doctype/sales_order/sales_order.js (4)
29-35: LGTM! The indicator logic correctly prioritizes delivery status over closed status.The ordering ensures fully-delivered items show green regardless of
is_closed, while partially-delivered closed items correctly display grey. This aligns with the reviewer feedback requesting grey as the closed-item indicator.
1726-1735: LGTM! Correctly excludes closed items from Purchase Order creation.This aligns with the PR objective that closed items should not reflect in other doctypes created from the Sales Order.
1941-1951: Closing all closable items triggers closing the entire Sales Order — verify this is intended.When
select_allis checked or when all displayed items are selected, the code callsclose_sales_order()which closes the entire SO. However,me.dataonly contains items whereqty > delivered_qty && !is_closed, not items that are already fully delivered.If a Sales Order has 3 items where 1 is fully delivered and 2 are pending, selecting both pending items in this dialog will close the entire order (including the fully-delivered item's status context). Is this the intended behavior, or should it only close the selected items individually?
1796-1873: LGTM! Thereopen_selected_itemsmethod is well-implemented.The method correctly:
- Filters for only closed items
- Provides a clear dialog for selection
- Calls the server method with appropriate parameters
- Reloads the document and shows feedback on success
nishkagosalia
force-pushed
the
gh-39030
branch
from
afa823d to
af95290
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @erpnext/selling/doctype/sales_order/sales_order.js:
- Around line 1875-1974: The dialog's "Select all items" checkbox (in
close_selected_items) defaults to checked and, when checked, calls
close_sales_order (closing the whole Sales Order) which is unexpected; change
the select_all field's default from 1 to 0 (or add clarifying copy) so
item-level closing is the safer default, and update the validation message for
no-selection from "Please select one item to close" to match the reopen flow
(e.g., "Please select at least one item to close") so messaging is consistent
across close_or_reopen_selected_items and reopen_selected_items.
🧹 Nitpick comments (2)
erpnext/selling/doctype/sales_order/sales_order.js (2)
1796-1873: LGTM with minor suggestion.The method correctly filters closed items, validates selection, and communicates with the server. The pattern matches existing ERPNext dialog conventions.
Consider adding
freeze: trueandfreeze_messageto thefrappe.callfor better UX during the server operation, consistent with other similar operations in this file (e.g.,create_stock_reservation_entries).♻️ Optional: Add freeze indicator
frappe.call({ method: "erpnext.selling.doctype.sales_order.sales_order.close_or_reopen_selected_items", args: { sales_order: me.frm.doc.name, selected_items: selected_items, status: "Re-open" }, + freeze: true, + freeze_message: __("Re-opening Items..."), callback: (r) => {
1956-1969: Consider adding freeze indicator for consistency.Similar to
reopen_selected_items, addingfreeze: trueandfreeze_messagewould improve UX during the server call.♻️ Optional: Add freeze indicator
frappe.call({ method: "erpnext.selling.doctype.sales_order.sales_order.close_or_reopen_selected_items", args: { sales_order: me.frm.doc.name, selected_items: selected_items, status: "Close" }, + freeze: true, + freeze_message: __("Closing Items..."), callback: (r) => {
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/manufacturing/doctype/production_plan/production_plan.pyerpnext/selling/doctype/sales_order/sales_order.js
🚧 Files skipped from review as they are similar to previous changes (1)
- erpnext/manufacturing/doctype/production_plan/production_plan.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-30T11:04:46.510Z
Learnt from: rohitwaghchaure
Repo: frappe/erpnext PR: 49766
File: erpnext/manufacturing/doctype/production_plan/production_plan.py:1717-1717
Timestamp: 2025-09-30T11:04:46.510Z
Learning: In the Production Plan's `get_items_for_material_requests` function in `erpnext/manufacturing/doctype/production_plan/production_plan.py`, always use `data.get("sales_order")` instead of `doc.get("sales_order")` when iterating over `po_items`. This ensures raw materials are correctly grouped by each production item's respective Sales Order, not a global document-level Sales Order.
Applied to files:
erpnext/selling/doctype/sales_order/sales_order.js
🧬 Code graph analysis (1)
erpnext/selling/doctype/sales_order/sales_order.js (1)
erpnext/stock/doctype/stock_entry/stock_entry.js (5)
d(363-396)d(417-417)d(727-730)d(745-745)values(732-732)
⏰ 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 (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (3)
erpnext/selling/doctype/sales_order/sales_order.js (3)
25-38: LGTM!The indicator logic correctly prioritizes fully-delivered items (green) over closed status (grey), with partially-delivered open items showing orange. This provides clear visual feedback for item states.
1001-1012: LGTM!The buttons are correctly placed within the existing permission and status checks, ensuring they only appear when the Sales Order is submitted, not on hold, and has pending delivery/billing.
1726-1726: LGTM!Correctly excludes closed items from the Purchase Order creation dialog, aligning with the PR objective that closed items should not reflect in downstream transactions.
nishkagosalia
force-pushed
the
gh-39030
branch
from
af95290 to
75a93df
Compare
nishkagosalia
force-pushed
the
gh-39030
branch
from
75a93df to
1ba7fea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
erpnext/selling/doctype/sales_order/sales_order.py (1)
1091-1111: Closed bundle items can still create Material Requests via packed items.
Sales Order Itemmapping blocksis_closed, butPacked Itemmapping doesn’t check the parent SO item’s closed state. This allows closed bundles to flow into Material Requests, violating the “closed items don’t reflect in other doctypes” requirement.🛠️ Suggested fix
def make_material_request(source_name, target_doc=None): requested_item_qty = get_requested_item_qty(source_name) + closed_so_items = set( + frappe.get_all( + "Sales Order Item", + filters={"parent": source_name, "is_closed": 1}, + pluck="name", + ) + ) @@ "Packed Item": { "doctype": "Material Request Item", "field_map": {"parent": "sales_order", "uom": "stock_uom", "name": "packed_item"}, - "condition": lambda item: get_remaining_packed_item_qty(item) > 0, + "condition": lambda item: ( + get_remaining_packed_item_qty(item) > 0 + and item.parent_detail_docname not in closed_so_items + ), "postprocess": update_item, },
🤖 Fix all issues with AI agents
In `@erpnext/selling/doctype/sales_order/sales_order.py`:
- Around line 1466-1472: The current conditional returns True unconditionally
for is_unit_price_row(doc), allowing closed rows to be invoiced; update the
expression so unit‑price rows still respect the closed flag by changing the
first branch to "is_unit_price_row(doc) and not doc.is_closed" (leaving the
existing else branch intact), so the overall check requires not doc.is_closed
for unit‑price rows as well.
♻️ Duplicate comments (3)
erpnext/patches/v16_0/update_sales_order_item_status.py (1)
4-18: Restore originalauto_commit_on_many_writesvalue and remove redundant condition.Two issues remain from previous reviews:
Original value not preserved: The
finallyblock unconditionally setsauto_commit_on_many_writes = 0, but the original value might have been different. This could affect other patches running in sequence.Redundant WHERE condition: Line 13 (
sales_order.name == sales_order_item.parent) duplicates the JOIN's ON clause at line 11.Proposed fix
def execute(): + original_auto_commit = frappe.db.auto_commit_on_many_writes frappe.db.auto_commit_on_many_writes = 1 sales_order = frappe.qb.DocType("Sales Order") sales_order_item = frappe.qb.DocType("Sales Order Item") try: frappe.qb.update(sales_order_item).join(sales_order).on( sales_order.name == sales_order_item.parent ).set(sales_order_item.is_closed, 1).where( - (sales_order.name == sales_order_item.parent) - & (sales_order.status == "Closed") + (sales_order.status == "Closed") & ((sales_order_item.is_closed == 0) | sales_order_item.is_closed.isnull()) ).run() finally: - frappe.db.auto_commit_on_many_writes = 0 + frappe.db.auto_commit_on_many_writes = original_auto_commiterpnext/selling/doctype/sales_order/sales_order.py (2)
2144-2152: Avoid per‑row credit limit checks on reopen.
This runscheck_credit_limit()once per reopened row. It’s redundant and can be expensive.♻️ Suggested refactor
- for row in so.items: + if status == "Re-open" and so.docstatus == 1: + so.check_credit_limit() + + for row in so.items: if not all_items_closed and row.name not in items: continue @@ - elif status == "Re-open": - if so.docstatus == 1: - so.check_credit_limit() - row.is_closed = 0 + elif status == "Re-open": + row.is_closed = 0
2160-2165: Blanket Order not updated when all items close.
When item‑level closure auto‑closes the SO,update_blanket_order()is skipped.🛠️ Suggested fix
if not all_items_closed and all(d.is_closed for d in so.items): so.status = "Closed" so.set_status(update=True, status="Closed") so.update_subcontracting_order_status() + so.update_blanket_order() so.notify_update() clear_doctype_notifications(so)
🧹 Nitpick comments (2)
erpnext/selling/doctype/sales_order/sales_order.js (1)
1900-1938: Consider defaulting "Select all items" to unchecked for safer UX.The "Select all items" checkbox defaults to checked (line 1904), which means clicking "Close" will close the entire Sales Order (via
close_sales_order()at line 1930), not individual items. Since the button label is "Close items", users expecting item-level control might accidentally close the whole SO.Consider defaulting
select_allto0so users must explicitly opt into closing the entire order:Suggested change
{ fieldname: "select_all", fieldtype: "Check", label: __("Select all items"), - default: 1, + default: 0, onchange: function () {erpnext/selling/report/sales_order_analysis/sales_order_analysis.py (1)
330-335: Consider showing "Is Closed" column only in non-grouped view.The "Is Closed" column is added to the
columns.extend()block, which means it appears for both grouped and non-grouped views. However,is_closedis a per-item attribute that doesn't aggregate meaningfully when grouped by Sales Order.Similar to the "Warehouse" column (lines 338-347), this column should likely only appear when
not filters.get("group_by_so"):Suggested change
"fieldname": "time_taken_to_deliver", "fieldtype": "Duration", "width": 100, }, - { - "label": _("Is Closed"), - "fieldname": "is_closed", - "fieldtype": "Check", - "width": 80, - }, ] ) if not filters.get("group_by_so"): columns.append( { "label": _("Warehouse"), ... } ) + columns.append( + { + "label": _("Is Closed"), + "fieldname": "is_closed", + "fieldtype": "Check", + "width": 80, + } + )
nishkagosalia
force-pushed
the
gh-39030
branch
from
1ba7fea to
a947200
Compare
nishkagosalia
force-pushed
the
gh-39030
branch
from
a947200 to
bdc3c17
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@erpnext/accounts/doctype/payment_entry/payment_entry.py`:
- Around line 2827-2828: get_sales_order_amounts currently mutates
self.grand_total / self.base_grand_total which causes repeated calls to produce
wrong totals; update the get_sales_order_amounts method (in sales_order.py) to
compute closed_amount and closed_base_amount by iterating self.get("items"),
derive adjusted_grand_total and adjusted_base_total as local variables (do not
assign back to self.grand_total/self.base_grand_total), compute outstanding
using those adjusted locals and return them; ensure callers like
get_reference_details / payment_entry.py (which calls get_sales_order_amounts)
keep using the returned values without relying on mutated document fields.
- Around line 2879-2880: The call to doc.get_sales_order_amounts() mutates the
full Document; to avoid persisting changes in this request, call the method on a
copied document instead of doc itself (e.g. create a deepcopy of doc and call
deepcopy_doc.get_sales_order_amounts()), or refactor get_sales_order_amounts to
be non-mutating and return amounts without changing self; update the dt ==
"Sales Order" branch so it uses the non-mutating approach (reference:
get_sales_order_amounts and the dt == "Sales Order" block).
In `@erpnext/selling/doctype/sales_order/sales_order.py`:
- Around line 827-834: The get_sales_order_amounts function currently mutates
self.grand_total and self.base_grand_total by decrementing them for closed
items; make it idempotent by computing local totals (e.g., local_grand_total =
self.grand_total and local_base_grand_total = self.base_grand_total), loop over
self.get("items") and subtract item.amount/item.base_amount from the local
variables when item.is_closed is truthy, then compute outstanding_amount =
local_grand_total - self.advance_paid, assign only self.outstanding_amount (do
not overwrite self.grand_total/self.base_grand_total), and return the
local_grand_total and outstanding_amount so repeated calls do not change the
object's stored totals.
bdc3c17 to
df16bdc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@erpnext/accounts/doctype/payment_entry/payment_entry.py`:
- Around line 2879-2880: The Sales Order totals are being unconditionally
overridden using doc.get_sales_order_amounts() which returns amounts in the SO
currency; this misstates paid/received amounts when the party account uses the
company currency. In the dt == "Sales Order" branch (payment_entry.py) change
the logic to check if party_account_currency == doc.company_currency and, if so,
compute grand_total and outstanding_amount from the SO base totals (subtracting
closed item base amounts) instead of get_sales_order_amounts(); otherwise
continue to use get_sales_order_amounts() for currency-matching cases. Ensure
you reference and update the variables grand_total and outstanding_amount only
after selecting the appropriate source (base totals vs
get_sales_order_amounts()) so paid/received amounts are in the correct currency.
🧹 Nitpick comments (1)
erpnext/selling/doctype/sales_order/sales_order.py (1)
593-598: Avoid duplicate reserved‑qty recomputation on reopen.
close_or_reopen_selected_items()already updates reserved quantities, so the trailingelse: self.update_reserved_qty()still runs on the Draft reopen path and does the work twice. Consider making the second branchelifso reopen doesn’t hit the extra update.♻️ Suggested tweak
- if status == "Draft" and self.docstatus == 1: - self.check_credit_limit() - close_or_reopen_selected_items(self.name, "Re-open", all_items_closed=True) - - if status == "Closed": - close_or_reopen_selected_items(self.name, "Close", all_items_closed=True) - else: - self.update_reserved_qty() + if status == "Draft" and self.docstatus == 1: + self.check_credit_limit() + close_or_reopen_selected_items(self.name, "Re-open", all_items_closed=True) + elif status == "Closed": + close_or_reopen_selected_items(self.name, "Close", all_items_closed=True) + else: + self.update_reserved_qty()
nishkagosalia
force-pushed
the
gh-39030
branch
from
df16bdc to
9b56793
Compare
4 participants
This adds the feature: #39030
By this feature we now allow the items to be closed at individual level in sales order. It ensures the following things:
no-docs