mirror of
https://github.com/frappe/erpnext.git
synced 2026-08-12 22:21:50 +00:00
fix(manufacturing): prefer the phantom line as bom_stock_analysis representative
Address review on #56090: get_bom_data groups components by item_code, so it picks one representative BOM Item line for the (bom_no, is_phantom_item) pair. Taking the first line by idx dropped the phantom flag when a non-phantom line was listed before the phantom one, so explode_phantom_boms skipped the sub-BOM. Keep one row per item_code (preserving the qty_per_unit total per component rather than widening the GROUP BY), but make the representative phantom- preferring: the first line, upgraded to the first phantom line if any exists. A phantom sub-BOM is therefore never dropped due to line order, on either engine. Adds test_phantom_explosion_when_phantom_line_is_not_first (phantom line at idx 2) alongside the existing idx-1 case; both pass on MariaDB and Postgres and the new one fails on the naive first-line representative. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -239,17 +239,21 @@ def get_bom_data(filters):
|
||||
# bom_no + is_phantom_item drive whether/which sub-BOM explode_phantom_boms recurses into, so
|
||||
# they must come from the SAME BOM Item line. Aggregating each independently (Max) could pair a
|
||||
# bom_no from one line with is_phantom_item from another when an item_code repeats in the BOM.
|
||||
# Take the first (lowest idx) line per item_code as the coherent representative.
|
||||
first_line = {}
|
||||
# Rows are grouped by item_code (one qty_per_unit total per component), so pick one coherent
|
||||
# representative line: the first line, but upgrade to the first phantom line if any exists, so a
|
||||
# phantom sub-BOM is never dropped just because a non-phantom line happens to be listed first.
|
||||
representative = {}
|
||||
for line in frappe.get_all(
|
||||
"BOM Item",
|
||||
filters={"parent": filters.get("bom"), "parenttype": "BOM"},
|
||||
fields=["item_code", "bom_no", "is_phantom_item"],
|
||||
order_by="idx",
|
||||
):
|
||||
first_line.setdefault(line.item_code, line)
|
||||
existing = representative.get(line.item_code)
|
||||
if existing is None or (line.is_phantom_item and not existing.is_phantom_item):
|
||||
representative[line.item_code] = line
|
||||
for row in data:
|
||||
line = first_line.get(row.item_code)
|
||||
line = representative.get(row.item_code)
|
||||
if line:
|
||||
row.bom_no = line.bom_no
|
||||
row.is_phantom_item = line.is_phantom_item
|
||||
|
||||
@@ -79,16 +79,10 @@ class TestBOMStockAnalysis(ERPNextTestSuite):
|
||||
)
|
||||
self.assertEqual(footer.get("description"), expected_min)
|
||||
|
||||
def test_phantom_explosion_picks_coherent_sub_bom(self):
|
||||
"""bom_no and is_phantom_item must come from the SAME BOM Item line.
|
||||
|
||||
When a component is listed more than once in a BOM pointing at different sub-BOMs
|
||||
(one phantom, one not), the report groups both lines into a single row by item_code.
|
||||
Aggregating bom_no and is_phantom_item with independent Max() could pair the phantom
|
||||
flag of one line with the bom_no of the other, so explode_phantom_boms recurses into
|
||||
the wrong sub-BOM. We now take one coherent representative line (lowest idx), so the
|
||||
phantom sub-BOM is the one exploded.
|
||||
"""
|
||||
def _build_duplicate_component_bom(self, phantom_first):
|
||||
"""Parent BOM that lists one `component` twice, once via a phantom sub-BOM and once via a
|
||||
non-phantom sub-BOM. `phantom_first` controls which line is at idx 1. Returns the names of
|
||||
(parent_bom, rm_phantom, rm_normal, component)."""
|
||||
rm_phantom = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name
|
||||
rm_normal = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name
|
||||
component = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name
|
||||
@@ -101,10 +95,12 @@ class TestBOMStockAnalysis(ERPNextTestSuite):
|
||||
phantom_bom.submit()
|
||||
normal_bom = make_bom(item=component, raw_materials=[rm_normal])
|
||||
|
||||
# Parent lists `component` twice: phantom line first (idx 1), non-phantom second (idx 2).
|
||||
fg_item = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name
|
||||
first_bom, second_bom = (
|
||||
(phantom_bom.name, normal_bom.name) if phantom_first else (normal_bom.name, phantom_bom.name)
|
||||
)
|
||||
parent = make_bom(item=fg_item, raw_materials=[component], do_not_save=True)
|
||||
parent.items[0].bom_no = phantom_bom.name
|
||||
parent.items[0].bom_no = first_bom
|
||||
component_doc = frappe.get_doc("Item", component)
|
||||
parent.append(
|
||||
"items",
|
||||
@@ -113,21 +109,43 @@ class TestBOMStockAnalysis(ERPNextTestSuite):
|
||||
"qty": 1,
|
||||
"uom": component_doc.stock_uom,
|
||||
"stock_uom": component_doc.stock_uom,
|
||||
"bom_no": normal_bom.name,
|
||||
"bom_no": second_bom,
|
||||
},
|
||||
)
|
||||
parent.save()
|
||||
parent.submit()
|
||||
return parent.name, rm_phantom, rm_normal, component
|
||||
|
||||
raw_data = bom_stock_analysis_report(filters={"qty_to_make": 1, "bom": parent.name})[1]
|
||||
def _assert_phantom_exploded(self, parent_bom, rm_phantom, rm_normal, component):
|
||||
raw_data = bom_stock_analysis_report(filters={"qty_to_make": 1, "bom": parent_bom})[1]
|
||||
items = {row.get("item") for row in raw_data if row}
|
||||
|
||||
# Phantom sub-BOM exploded -> its raw material appears; the component row is replaced.
|
||||
self.assertIn(rm_phantom, items)
|
||||
self.assertNotIn(component, items)
|
||||
# The non-phantom line's sub-BOM must NOT be mis-exploded.
|
||||
self.assertNotIn(rm_normal, items)
|
||||
|
||||
def test_phantom_explosion_picks_coherent_sub_bom(self):
|
||||
"""bom_no and is_phantom_item must come from the SAME BOM Item line.
|
||||
|
||||
When a component is listed more than once in a BOM pointing at different sub-BOMs
|
||||
(one phantom, one not), the report groups both lines into a single row by item_code.
|
||||
Aggregating bom_no and is_phantom_item with independent Max() could pair the phantom
|
||||
flag of one line with the bom_no of the other, so explode_phantom_boms recurses into
|
||||
the wrong sub-BOM. We now take one coherent representative line, so the phantom sub-BOM
|
||||
is the one exploded.
|
||||
"""
|
||||
self._assert_phantom_exploded(*self._build_duplicate_component_bom(phantom_first=True))
|
||||
|
||||
def test_phantom_explosion_when_phantom_line_is_not_first(self):
|
||||
"""The phantom flag must win regardless of line order.
|
||||
|
||||
If the non-phantom line is listed first (idx 1) and the phantom line second, a naive
|
||||
first-line representative would drop the phantom flag and skip the sub-BOM explosion.
|
||||
The representative is phantom-preferring, so the phantom sub-BOM is still exploded.
|
||||
"""
|
||||
self._assert_phantom_exploded(*self._build_duplicate_component_bom(phantom_first=False))
|
||||
|
||||
|
||||
def split_data_and_footer(raw_data):
|
||||
"""Separate component rows from the footer row. Skips blank spacer rows."""
|
||||
|
||||
Reference in New Issue
Block a user