mirror of
https://github.com/frappe/erpnext.git
synced 2026-08-13 06:31:48 +00:00
fix(manufacturing): deterministic arbitrary-pick in BOM explosion & WO transfer tracking
Same class as the sub_assembly_queries / bom_stock_analysis fixes in this PR: two more merged queries wrapped a non-functionally-dependent column in Max(), so the engines can disagree and the value is wrong for the case the column drives. bom_explosion._subitems_query — Max(is_phantom_item): Rows are grouped by item_code and get_subitems() drops any grouped row whose is_phantom_item is truthy. When one item_code is listed in a BOM both as a phantom sub-assembly and as a plain raw material, Max() returns 1 and the real raw material is silently dropped from the plan. Use Min(): an item is phantom only when EVERY line for it is phantom, so a real material is never lost. required_items._material_transfer_qty_by_item — Max(original_item): original_item is the output dict key. The same item B can be transferred both for itself (original_item NULL) and as a substitute for required item A (original_item=A). Grouping by item_code alone with Max() merged the two and credited B's whole transfer to A, leaving B at 0. Group by (item_code, original_item) and accumulate into the keyed dict so each transfer is credited to the right required item (two rows can resolve to one key, e.g. A's own transfer and B-for-A, hence += not plain assignment). Both were previously undefined SQL (loose GROUP BY); the fix makes MariaDB and Postgres agree on the correct, deterministic value. Other Max()-wrapped columns in these queries are functionally dependent on the grouped item and unchanged. Tests (fail on the old code, pass on both engines): - test_subitems_query_keeps_real_rm_listed_alongside_phantom - test_transferred_qty_not_misattributed_between_item_and_its_substitute Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -116,9 +116,12 @@ def _subitems_query(company, bom_no, include_non_stock_items, parent_qty, planne
|
||||
|
||||
def _subitem_columns(bom_item, bom, item, item_default, item_uom, parent_qty, planned_qty):
|
||||
qty = IfNull(parent_qty * Sum(bom_item.stock_qty / IfNull(bom.quantity, 1)) * planned_qty, 0).as_("qty")
|
||||
# only item_code is grouped; the rest are functionally dependent on the grouped item (item
|
||||
# attributes) or arbitrary per BOM Item on MySQL -> Max() keeps the GROUP BY valid on postgres
|
||||
# while returning the same value MySQL picked.
|
||||
# only item_code is grouped; the remaining item-attribute columns are functionally dependent on it,
|
||||
# so Max() returns their single value on both engines. is_phantom_item is the exception: the same
|
||||
# item_code can sit on a phantom line and a real-RM line in one BOM, and get_subitems() drops any
|
||||
# row whose is_phantom_item is truthy. Max() would let a single phantom line mask the real material
|
||||
# and silently drop it; Min() instead treats the item as phantom only when EVERY line is phantom, so
|
||||
# a real raw material is never lost. Deterministic and identical on MariaDB and Postgres.
|
||||
return [
|
||||
bom_item.item_code,
|
||||
Max(item.default_material_request_type).as_("default_material_request_type"),
|
||||
@@ -136,7 +139,7 @@ def _subitem_columns(bom_item, bom, item, item_default, item_uom, parent_qty, pl
|
||||
Max(item_uom.conversion_factor).as_("conversion_factor"),
|
||||
Max(bom.item).as_("main_bom_item"),
|
||||
Max(bom.name).as_("main_bom"),
|
||||
Max(bom_item.is_phantom_item).as_("is_phantom_item"),
|
||||
Min(bom_item.is_phantom_item).as_("is_phantom_item"),
|
||||
]
|
||||
|
||||
|
||||
|
||||
@@ -2917,6 +2917,49 @@ class TestProductionPlan(ERPNextTestSuite):
|
||||
self.assertEqual(by_bom_no[phantom_bom.name].is_phantom_item, 1)
|
||||
self.assertEqual(by_bom_no[normal_bom.name].is_phantom_item, 0)
|
||||
|
||||
def test_subitems_query_keeps_real_rm_listed_alongside_phantom(self):
|
||||
"""bom_explosion._subitems_query groups BOM lines by item_code, and get_subitems() drops any
|
||||
grouped row whose is_phantom_item is truthy. When one item_code is listed in a BOM both as a
|
||||
phantom sub-assembly and as a plain raw material, Max(is_phantom_item)=1 made get_subitems
|
||||
silently drop the real material. Min(is_phantom_item) keeps it (phantom only when every line
|
||||
is phantom) and is deterministic on MariaDB and Postgres.
|
||||
"""
|
||||
from erpnext.manufacturing.doctype.production_plan.services.bom_explosion import _subitems_query
|
||||
|
||||
component = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name
|
||||
rm_phantom = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name
|
||||
|
||||
phantom_bom = make_bom(item=component, raw_materials=[rm_phantom], do_not_save=True)
|
||||
phantom_bom.is_phantom_bom = 1
|
||||
phantom_bom.save()
|
||||
phantom_bom.submit()
|
||||
# the phantom BOM is auto-set as the component's default; clear it so the second component line
|
||||
# stays a plain (non-phantom) raw material instead of inheriting the phantom BOM as its bom_no.
|
||||
frappe.db.set_value("Item", component, "default_bom", "")
|
||||
frappe.clear_document_cache("Item", component)
|
||||
|
||||
fg_item = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name
|
||||
parent = make_bom(item=fg_item, raw_materials=[component], do_not_save=True)
|
||||
parent.items[0].bom_no = phantom_bom.name # phantom line -> is_phantom_item = 1
|
||||
component_doc = frappe.get_doc("Item", component)
|
||||
parent.append(
|
||||
"items",
|
||||
{
|
||||
"item_code": component,
|
||||
"qty": 1,
|
||||
"uom": component_doc.stock_uom,
|
||||
"stock_uom": component_doc.stock_uom,
|
||||
},
|
||||
) # plain raw-material line (no bom_no) -> is_phantom_item = 0
|
||||
parent.save()
|
||||
parent.submit()
|
||||
|
||||
rows = _subitems_query("_Test Company", parent.name, 1, 1, 1)
|
||||
component_rows = [r for r in rows if r.item_code == component]
|
||||
self.assertEqual(len(component_rows), 1)
|
||||
# Min() keeps the real material; the old Max() returned 1 and get_subitems dropped it.
|
||||
self.assertEqual(component_rows[0].is_phantom_item, 0)
|
||||
|
||||
|
||||
def create_production_plan(**args):
|
||||
"""
|
||||
|
||||
@@ -191,17 +191,25 @@ class RequiredItemsService:
|
||||
frappe.qb.from_(ste)
|
||||
.inner_join(ste_child)
|
||||
.on(ste_child.parent == ste.name)
|
||||
# original_item is arbitrary per grouped item_code on MySQL -> Max() keeps the GROUP BY valid
|
||||
# on postgres while returning the same value (it is only used as a dict key fallback below)
|
||||
# original_item becomes the output dict key below, so it must stay coherent per row: the
|
||||
# same item_code can be transferred both for itself (original_item NULL) and as a substitute
|
||||
# for another required item (original_item set). Max() over a single item_code group could
|
||||
# pick the substitute's original_item and misattribute the item's own transfer to it. Group
|
||||
# by (item_code, original_item) so each pair sums separately, then accumulate into the keyed
|
||||
# dict (two distinct rows can resolve to the same key, e.g. A's own transfer and B-for-A).
|
||||
.select(
|
||||
ste_child.item_code,
|
||||
fn.Max(ste_child.original_item).as_("original_item"),
|
||||
ste_child.original_item,
|
||||
fn.Sum(ste_child.transfer_qty).as_("qty"),
|
||||
)
|
||||
.where(self._material_transfer_filter(ste, is_return))
|
||||
.groupby(ste_child.item_code)
|
||||
.groupby(ste_child.item_code, ste_child.original_item)
|
||||
)
|
||||
return frappe._dict({d.original_item or d.item_code: d.qty for d in (query.run(as_dict=1) or [])})
|
||||
qty_by_item = frappe._dict()
|
||||
for d in query.run(as_dict=1) or []:
|
||||
key = d.original_item or d.item_code
|
||||
qty_by_item[key] = (qty_by_item.get(key) or 0.0) + flt(d.qty)
|
||||
return qty_by_item
|
||||
|
||||
def _material_transfer_filter(self, ste, is_return):
|
||||
return (
|
||||
|
||||
@@ -4815,6 +4815,57 @@ class TestWorkOrder(ERPNextTestSuite):
|
||||
# generated qty (3.0 for 8 units) differs from the BOM-scaled qty (7.5 for 20 units)
|
||||
self.assertEqual(flt(row.qty, 6), 3.0)
|
||||
|
||||
def test_transferred_qty_not_misattributed_between_item_and_its_substitute(self):
|
||||
"""When one item is transferred both for itself and as a substitute for another required item,
|
||||
each transfer must be credited to the right required item.
|
||||
|
||||
_material_transfer_qty_by_item grouped Stock Entry Detail by item_code only and picked
|
||||
Max(original_item); for item B transferred once for itself (original_item NULL) and once as a
|
||||
substitute for A (original_item=A), Max picked A and credited B's whole transfer to A, leaving
|
||||
B at 0. Grouping by (item_code, original_item) and accumulating into the keyed dict attributes
|
||||
each transfer correctly, deterministically on MariaDB and Postgres.
|
||||
"""
|
||||
from erpnext.manufacturing.doctype.work_order.services.required_items import RequiredItemsService
|
||||
|
||||
source_warehouse = "Stores - _TC"
|
||||
fg_item = make_item("Test WO SelfSub FG", {"is_stock_item": 1}).name
|
||||
item_a = make_item("Test WO SelfSub RM A", {"is_stock_item": 1, "allow_alternative_item": 1}).name
|
||||
item_b = make_item("Test WO SelfSub RM B", {"is_stock_item": 1, "allow_alternative_item": 1}).name
|
||||
|
||||
# B is a registered alternative for A
|
||||
if not frappe.db.exists("Item Alternative", {"item_code": item_a, "alternative_item_code": item_b}):
|
||||
frappe.get_doc(
|
||||
{
|
||||
"doctype": "Item Alternative",
|
||||
"item_code": item_a,
|
||||
"alternative_item_code": item_b,
|
||||
"two_way": 1,
|
||||
}
|
||||
).insert()
|
||||
|
||||
# stock B generously (covers B-for-A plus B-for-itself)
|
||||
for item, qty in ((item_a, 50), (item_b, 100)):
|
||||
test_stock_entry.make_stock_entry(
|
||||
item_code=item, target=source_warehouse, qty=qty, basic_rate=100
|
||||
)
|
||||
|
||||
make_bom(item=fg_item, source_warehouse=source_warehouse, raw_materials=[item_a, item_b])
|
||||
wo = make_wo_order_test_record(item=fg_item, qty=10, source_warehouse=source_warehouse)
|
||||
|
||||
transfer = frappe.get_doc(make_stock_entry(wo.name, "Material Transfer for Manufacture", 10))
|
||||
transfer.save()
|
||||
# substitute B for the A line; the existing B line stays as B's own transfer
|
||||
for d in transfer.items:
|
||||
if d.item_code == item_a:
|
||||
d.item_code = item_b
|
||||
d.original_item = item_a
|
||||
transfer.submit()
|
||||
|
||||
qty_by_item = RequiredItemsService(wo)._material_transfer_qty_by_item(is_return=0)
|
||||
# B transferred as a substitute for A -> credited to A; B transferred for itself -> credited to B.
|
||||
self.assertEqual(flt(qty_by_item.get(item_a)), 10.0)
|
||||
self.assertEqual(flt(qty_by_item.get(item_b)), 10.0)
|
||||
|
||||
|
||||
def get_reserved_entries(voucher_no, warehouse=None):
|
||||
doctype = frappe.qb.DocType("Stock Reservation Entry")
|
||||
|
||||
Reference in New Issue
Block a user