fix: distribution of additional costs in mfg stock entry (frappe#27629)

* refactor: remove unnecessary list comprehensions

* fix: correct cost distribution logic

While apportioning costs same condition should be present on both sides
so total value is representative of all items to be apportioned.

Here while calculating incoming_items_cost only FG items are considered,
but while apportioning all items with to_warehouse are considered.

Solution: only apportion additional cost on FG items

* test: test cost distribution

* fix: patch for additional cost

fix(patch): consider PCV while patching

- consider Period closing voucher while patching
- recomute rates for SLE of affected stock entries

consider only FG/scrap item SLEs for recomputation of rates

* fix: remove client side logic for addn cost

All of this is done in python code hence removed client side code.

erpnext/patches.txt

@@ -311,3 +311,4 @@ erpnext.patches.v13_0.gst_fields_for_pos_invoice
 erpnext.patches.v13_0.create_accounting_dimensions_in_pos_doctypes
 erpnext.patches.v13_0.create_custom_field_for_finance_book
 erpnext.patches.v13_0.modify_invalid_gain_loss_gl_entries
+erpnext.patches.v13_0.fix_additional_cost_in_mfg_stock_entry

erpnext/patches/v13_0/fix_additional_cost_in_mfg_stock_entry.py

@@ -0,0 +1,76 @@
+from typing import List, NewType
+
+import frappe
+
+StockEntryCode = NewType("StockEntryCode", str)
+
+
+def execute():
+	stock_entry_codes = find_broken_stock_entries()
+
+	for stock_entry_code in stock_entry_codes:
+		patched_stock_entry = patch_additional_cost(stock_entry_code)
+		create_repost_item_valuation(patched_stock_entry)
+
+
+def find_broken_stock_entries() -> List[StockEntryCode]:
+	period_closing_date = frappe.db.get_value(
+		"Period Closing Voucher", {"docstatus": 1}, "posting_date", order_by="posting_date desc"
+	)
+
+	stock_entries_to_patch = frappe.db.sql(
+		"""
+		select se.name, sum(sed.additional_cost) as item_additional_cost, se.total_additional_costs
+		from `tabStock Entry` se
+		join `tabStock Entry Detail` sed
+			on sed.parent = se.name
+		where
+			se.docstatus = 1 and
+			se.posting_date > %s
+		group by
+			sed.parent
+		having
+			item_additional_cost != se.total_additional_costs
+	""",
+		period_closing_date,
+		as_dict=True,
+	)
+
+	return [d.name for d in stock_entries_to_patch]
+
+
+def patch_additional_cost(code: StockEntryCode):
+	stock_entry = frappe.get_doc("Stock Entry", code)
+	stock_entry.distribute_additional_costs()
+	stock_entry.update_valuation_rate()
+	stock_entry.set_total_incoming_outgoing_value()
+	stock_entry.set_total_amount()
+	stock_entry.db_update()
+	for item in stock_entry.items:
+		item.db_update()
+	return stock_entry
+
+
+def create_repost_item_valuation(stock_entry):
+	from erpnext.controllers.stock_controller import create_repost_item_valuation_entry
+
+	# turn on recalculate flag so reposting corrects the incoming/outgoing rates.
+	frappe.db.set_value(
+		"Stock Ledger Entry",
+		{"voucher_no": stock_entry.name, "actual_qty": (">", 0)},
+		"recalculate_rate",
+		1,
+		update_modified=False,
+	)
+
+	create_repost_item_valuation_entry(
+		args=frappe._dict(
+			{
+				"posting_date": stock_entry.posting_date,
+				"posting_time": stock_entry.posting_time,
+				"voucher_type": stock_entry.doctype,
+				"voucher_no": stock_entry.name,
+				"company": stock_entry.company,
+			}
+		)
+	)

erpnext/stock/doctype/stock_entry/stock_entry.js

@@ -548,44 +548,7 @@ frappe.ui.form.on('Stock Entry', {
 	calculate_basic_amount: function(frm, item) {
 		item.basic_amount = flt(flt(item.transfer_qty) * flt(item.basic_rate),
 			precision("basic_amount", item));
-
-		frm.events.calculate_amount(frm);
-	},
-
-	calculate_amount: function(frm) {
 		frm.events.calculate_total_additional_costs(frm);
-		let total_basic_amount = 0;
-		if (in_list(["Repack", "Manufacture"], frm.doc.purpose)) {
-			total_basic_amount = frappe.utils.sum(
-				(frm.doc.items || []).map(function(i) {
-					return i.is_finished_item ? flt(i.basic_amount) : 0;
-				})
-			);
-		} else {
-			total_basic_amount = frappe.utils.sum(
-				(frm.doc.items || []).map(function(i) {
-					return i.t_warehouse ? flt(i.basic_amount) : 0;
-				})
-			);
-		}
-		for (let i in frm.doc.items) {
-			let item = frm.doc.items[i];
-
-			if (((in_list(["Repack", "Manufacture"], frm.doc.purpose) && item.is_finished_item) || item.t_warehouse) && total_basic_amount) {
-				item.additional_cost = (flt(item.basic_amount) / total_basic_amount) * frm.doc.total_additional_costs;
-			} else {
-				item.additional_cost = 0;
-			}
-
-			item.amount = flt(item.basic_amount + flt(item.additional_cost), precision("amount", item));
-
-			if (flt(item.transfer_qty)) {
-				item.valuation_rate = flt(flt(item.basic_rate) + (flt(item.additional_cost) / flt(item.transfer_qty)),
-					precision("valuation_rate", item));
-			}
-		}
-
-		refresh_field('items');
 	},
 
 	calculate_total_additional_costs: function(frm) {
@@ -781,11 +744,6 @@ frappe.ui.form.on('Landed Cost Taxes and Charges', {
 	amount: function(frm, cdt, cdn) {
 		frm.events.set_base_amount(frm, cdt, cdn);
 
-		// Adding this check because same table in used in LCV
-		// This causes an error if you try to post an LCV immediately after a Stock Entry
-		if (frm.doc.doctype == 'Stock Entry') {
-			frm.events.calculate_amount(frm);
-		}
 	},
 
 	expense_account: function(frm, cdt, cdn) {

erpnext/stock/doctype/stock_entry/stock_entry.py

@@ -555,22 +555,27 @@ def get_basic_rate_for_manufactured_item(self, finished_item_qty, outgoing_items
 
 	def distribute_additional_costs(self):
 		# If no incoming items, set additional costs blank
-		if not any([d.item_code for d in self.items if d.t_warehouse]):
+		if not any(d.item_code for d in self.items if d.t_warehouse):
 			self.additional_costs = []
 
-		self.total_additional_costs = sum([flt(t.base_amount) for t in self.get("additional_costs")])
+		self.total_additional_costs = sum(flt(t.base_amount) for t in self.get("additional_costs"))
 
 		if self.purpose in ("Repack", "Manufacture"):
-			incoming_items_cost = sum([flt(t.basic_amount) for t in self.get("items") if t.is_finished_item])
+			incoming_items_cost = sum(flt(t.basic_amount) for t in self.get("items") if t.is_finished_item)
 		else:
-			incoming_items_cost = sum([flt(t.basic_amount) for t in self.get("items") if t.t_warehouse])
+			incoming_items_cost = sum(flt(t.basic_amount) for t in self.get("items") if t.t_warehouse)
 
-		if incoming_items_cost:
-			for d in self.get("items"):
-				if (self.purpose in ("Repack", "Manufacture") and d.is_finished_item) or d.t_warehouse:
-					d.additional_cost = (flt(d.basic_amount) / incoming_items_cost) * self.total_additional_costs
-				else:
-					d.additional_cost = 0
+		if not incoming_items_cost:
+			return
+
+		for d in self.get("items"):
+			if self.purpose in ("Repack", "Manufacture") and not d.is_finished_item:
+				d.additional_cost = 0
+				continue
+			elif not d.t_warehouse:
+				d.additional_cost = 0
+				continue
+			d.additional_cost = (flt(d.basic_amount) / incoming_items_cost) * self.total_additional_costs
 
 	def update_valuation_rate(self):
 		for d in self.get("items"):
@@ -805,7 +810,11 @@ def get_sle_for_target_warehouse(self, sl_entries, finished_item_row):
 	def get_gl_entries(self, warehouse_account):
 		gl_entries = super(StockEntry, self).get_gl_entries(warehouse_account)
 
-		total_basic_amount = sum([flt(t.basic_amount) for t in self.get("items") if t.t_warehouse])
+		if self.purpose in ("Repack", "Manufacture"):
+			total_basic_amount = sum(flt(t.basic_amount) for t in self.get("items") if t.is_finished_item)
+		else:
+			total_basic_amount = sum(flt(t.basic_amount) for t in self.get("items") if t.t_warehouse)
+
 		divide_based_on = total_basic_amount
 
 		if self.get("additional_costs") and not total_basic_amount:
@@ -816,20 +825,24 @@ def get_gl_entries(self, warehouse_account):
 
 		for t in self.get("additional_costs"):
 			for d in self.get("items"):
-				if d.t_warehouse:
-					item_account_wise_additional_cost.setdefault((d.item_code, d.name), {})
-					item_account_wise_additional_cost[(d.item_code, d.name)].setdefault(t.expense_account, {
-						"amount": 0.0,
-						"base_amount": 0.0
-					})
+				if self.purpose in ("Repack", "Manufacture") and not d.is_finished_item:
+					continue
+				elif not d.t_warehouse:
+					continue
+
+				item_account_wise_additional_cost.setdefault((d.item_code, d.name), {})
+				item_account_wise_additional_cost[(d.item_code, d.name)].setdefault(t.expense_account, {
+					"amount": 0.0,
+					"base_amount": 0.0
+				})
 
-					multiply_based_on = d.basic_amount if total_basic_amount else d.qty
+				multiply_based_on = d.basic_amount if total_basic_amount else d.qty
 
-					item_account_wise_additional_cost[(d.item_code, d.name)][t.expense_account]["amount"] += \
-						flt(t.amount * multiply_based_on) / divide_based_on
+				item_account_wise_additional_cost[(d.item_code, d.name)][t.expense_account]["amount"] += \
+					flt(t.amount * multiply_based_on) / divide_based_on
 
-					item_account_wise_additional_cost[(d.item_code, d.name)][t.expense_account]["base_amount"] += \
-						flt(t.base_amount * multiply_based_on) / divide_based_on
+				item_account_wise_additional_cost[(d.item_code, d.name)][t.expense_account]["base_amount"] += \
+					flt(t.base_amount * multiply_based_on) / divide_based_on
 
 		if item_account_wise_additional_cost:
 			for d in self.get("items"):

erpnext/stock/doctype/stock_entry/test_stock_entry.py

@@ -837,6 +837,39 @@ def test_conversion_factor_change(self):
 
 		frappe.db.set_default("allow_negative_stock", 0)
 
+	def test_additional_cost_distribution_manufacture(self):
+		se = frappe.get_doc(
+				doctype="Stock Entry",
+				purpose="Manufacture",
+				additional_costs=[frappe._dict(base_amount=100)],
+				items=[
+					frappe._dict(item_code="RM", basic_amount=10),
+					frappe._dict(item_code="FG", basic_amount=20, t_warehouse="X", is_finished_item=1),
+					frappe._dict(item_code="scrap", basic_amount=30, t_warehouse="X")
+				],
+			)
+
+		se.distribute_additional_costs()
+
+		distributed_costs = [d.additional_cost for d in se.items]
+		self.assertEqual([0.0, 100.0, 0.0], distributed_costs)
+
+	def test_additional_cost_distribution_non_manufacture(self):
+		se = frappe.get_doc(
+				doctype="Stock Entry",
+				purpose="Material Receipt",
+				additional_costs=[frappe._dict(base_amount=100)],
+				items=[
+					frappe._dict(item_code="RECEIVED_1", basic_amount=20, t_warehouse="X"),
+					frappe._dict(item_code="RECEIVED_2", basic_amount=30, t_warehouse="X")
+				],
+			)
+
+		se.distribute_additional_costs()
+
+		distributed_costs = [d.additional_cost for d in se.items]
+		self.assertEqual([40.0, 60.0], distributed_costs)
+
 def make_serialized_item(**args):
 	args = frappe._dict(args)
 	se = frappe.copy_doc(test_records[0])