mirror of
https://github.com/frappe/erpnext.git
synced 2026-08-11 05:31:48 +00:00
fix(manufacturing): stop BOM Stock Analysis inflating both its sums (#57709)
* fix(manufacturing): stop BOM Stock Analysis inflating both its sums get_bom_data left-joined Bin on item_code alone and then summed over the result. Bin holds one row per warehouse and BOM Item one row per line, so the join is a cross product and each SUM counts the other side's rows: Sum(qty_consumed_per_unit) x (number of warehouses holding the item) Sum(bin.actual_qty) x (number of BOM lines carrying the item) A component on two BOM lines, stocked in two warehouses, reported a per-unit requirement of 10 instead of 5 and available stock of 20 instead of 10 -- wrong on both engines, and wrong in the single-line case too as soon as the item sits in more than one warehouse. Aggregate Bin to one row per item_code before joining, so neither sum can see the other's duplicates. The warehouse filter moves into that subquery; it previously sat in the outer WHERE against a left-joined column, which silently made the join inner, so the join is now made inner explicitly when a warehouse is given to keep items with no bin there excluded as before. * test(manufacturing): cover the BOM Stock Analysis bin-join cross product Component on two BOM lines, stocked in two warehouses: the join yields four rows, so both sums are doubled. Asserts qty_per_unit is the sum of the lines' own per-unit quantities and actual_qty the real total across warehouses. Fails on the previous single-query form with 10.0 != 5.0.
This commit is contained in:
@@ -190,27 +190,14 @@ def batch_fetch_purchase_rates(bom_data):
|
||||
}
|
||||
|
||||
|
||||
def get_bom_data(filters):
|
||||
bom_item_table = "BOM Explosion Item" if filters.get("show_exploded_view") else "BOM Item"
|
||||
|
||||
bom_item = frappe.qb.DocType(bom_item_table)
|
||||
def get_stock_qty_by_item(filters):
|
||||
"""One row per item_code, so joining it to BOM Item cannot multiply either side's sum."""
|
||||
bin = frappe.qb.DocType("Bin")
|
||||
|
||||
query = (
|
||||
frappe.qb.from_(bom_item)
|
||||
.left_join(bin)
|
||||
.on(bom_item.item_code == bin.item_code)
|
||||
.select(
|
||||
bom_item.item_code,
|
||||
# non-grouped columns are constant per grouped item_code -> Max() keeps the GROUP BY valid
|
||||
Max(bom_item.description).as_("description"),
|
||||
Max(bom_item.parent).as_("from_bom_no"),
|
||||
Sum(bom_item.qty_consumed_per_unit).as_("qty_per_unit"),
|
||||
IfNull(Sum(bin.actual_qty), 0).as_("actual_qty"),
|
||||
)
|
||||
.where((bom_item.parent == filters.get("bom")) & (bom_item.parenttype == "BOM"))
|
||||
.groupby(bom_item.item_code)
|
||||
.orderby(Min(bom_item.idx))
|
||||
frappe.qb.from_(bin)
|
||||
.select(bin.item_code, Sum(bin.actual_qty).as_("actual_qty"))
|
||||
.groupby(bin.item_code)
|
||||
)
|
||||
|
||||
if filters.get("warehouse"):
|
||||
@@ -233,6 +220,33 @@ def get_bom_data(filters):
|
||||
else:
|
||||
query = query.where(bin.warehouse == filters.get("warehouse"))
|
||||
|
||||
return query
|
||||
|
||||
|
||||
def get_bom_data(filters):
|
||||
bom_item_table = "BOM Explosion Item" if filters.get("show_exploded_view") else "BOM Item"
|
||||
|
||||
bom_item = frappe.qb.DocType(bom_item_table)
|
||||
stock_qty = get_stock_qty_by_item(filters).as_("stock_qty")
|
||||
|
||||
base = frappe.qb.from_(bom_item)
|
||||
base = base.join(stock_qty) if filters.get("warehouse") else base.left_join(stock_qty)
|
||||
|
||||
query = (
|
||||
base.on(bom_item.item_code == stock_qty.item_code)
|
||||
.select(
|
||||
bom_item.item_code,
|
||||
# non-grouped columns are constant per grouped item_code -> Max() keeps the GROUP BY valid
|
||||
Max(bom_item.description).as_("description"),
|
||||
Max(bom_item.parent).as_("from_bom_no"),
|
||||
Sum(bom_item.qty_consumed_per_unit).as_("qty_per_unit"),
|
||||
IfNull(Max(stock_qty.actual_qty), 0).as_("actual_qty"),
|
||||
)
|
||||
.where((bom_item.parent == filters.get("bom")) & (bom_item.parenttype == "BOM"))
|
||||
.groupby(bom_item.item_code)
|
||||
.orderby(Min(bom_item.idx))
|
||||
)
|
||||
|
||||
data = query.run(as_dict=True)
|
||||
|
||||
if bom_item_table == "BOM Item":
|
||||
|
||||
@@ -1,13 +1,18 @@
|
||||
# Copyright (c) 2026, Frappe Technologies Pvt. Ltd. and contributors
|
||||
# For license information, please see license.txt
|
||||
import frappe
|
||||
from frappe.utils import fmt_money
|
||||
from frappe.utils import flt, fmt_money
|
||||
|
||||
from erpnext.manufacturing.doctype.production_plan.test_production_plan import make_bom
|
||||
from erpnext.manufacturing.report.bom_stock_analysis.bom_stock_analysis import (
|
||||
execute as bom_stock_analysis_report,
|
||||
)
|
||||
from erpnext.manufacturing.report.bom_stock_analysis.bom_stock_analysis import get_bom_data
|
||||
from erpnext.stock.doctype.item.test_item import make_item
|
||||
from erpnext.stock.doctype.stock_reconciliation.test_stock_reconciliation import (
|
||||
create_stock_reconciliation,
|
||||
)
|
||||
from erpnext.stock.doctype.warehouse.test_warehouse import create_warehouse
|
||||
from erpnext.tests.utils import ERPNextTestSuite
|
||||
|
||||
|
||||
@@ -146,6 +151,41 @@ class TestBOMStockAnalysis(ERPNextTestSuite):
|
||||
"""
|
||||
self._assert_phantom_exploded(*self._build_duplicate_component_bom(phantom_first=False))
|
||||
|
||||
def test_bom_data_is_not_multiplied_by_the_bin_join(self):
|
||||
"""Bin joins one row per warehouse, BOM Item one per line -- neither sum may count the other.
|
||||
|
||||
With the component listed on two BOM lines and stocked in two warehouses, the join yields
|
||||
four rows. Summing qty_consumed_per_unit over it counts each line once per warehouse, and
|
||||
summing actual_qty counts each warehouse once per line.
|
||||
"""
|
||||
rm = make_item(properties={"is_stock_item": 1, "valuation_rate": 10})
|
||||
fg = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name
|
||||
|
||||
bom = make_bom(item=fg, raw_materials=[rm.name], rm_qty=2, do_not_save=True)
|
||||
bom.append(
|
||||
"items",
|
||||
{"item_code": rm.name, "qty": 3, "uom": rm.stock_uom, "stock_uom": rm.stock_uom},
|
||||
)
|
||||
bom.save()
|
||||
bom.submit()
|
||||
|
||||
for suffix, qty in (("A", 6), ("B", 4)):
|
||||
warehouse = create_warehouse(f"_Test BOM Stock Analysis {suffix}")
|
||||
create_stock_reconciliation(item_code=rm.name, warehouse=warehouse, qty=qty, rate=10)
|
||||
|
||||
rows = [row for row in get_bom_data({"bom": bom.name}) if row.item_code == rm.name]
|
||||
self.assertEqual(len(rows), 1)
|
||||
|
||||
lines = [line for line in bom.items if line.item_code == rm.name]
|
||||
self.assertEqual(len(lines), 2)
|
||||
|
||||
self.assertAlmostEqual(
|
||||
flt(rows[0].qty_per_unit),
|
||||
sum(flt(line.qty_consumed_per_unit) for line in lines),
|
||||
places=6,
|
||||
)
|
||||
self.assertAlmostEqual(flt(rows[0].actual_qty), 10.0, places=6)
|
||||
|
||||
|
||||
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