fix: rewrite item rate calculation (backport #56315)

Co-authored-by: Harsh Patadia <harsh@Harshs-MacBook-Air.local>
Co-authored-by: Sagar Vora <16315650+sagarvora@users.noreply.github.com>
Co-authored-by: Smit Vora <mailsmitvora@gmail.com>

erpnext/accounts/doctype/sales_invoice/test_sales_invoice.py

@@ -2150,11 +2150,14 @@ class TestSalesInvoice(ERPNextTestSuite):
 	def test_create_so_with_margin(self):
 		si = create_sales_invoice(item_code="_Test Item", qty=1, do_not_submit=True)
 		price_list_rate = flt(100) * flt(si.plc_conversion_rate)
+
 		si.items[0].price_list_rate = price_list_rate
 		si.items[0].margin_type = "Percentage"
 		si.items[0].margin_rate_or_amount = 25
 		si.items[0].discount_amount = 0.0
 		si.items[0].discount_percentage = 0.0
+		# set rate to zero, so that it is recalculated on save
+		si.items[0].rate = 0
 		si.save()
 		self.assertEqual(si.get("items")[0].rate, flt((price_list_rate * 25) / 100 + price_list_rate))
 

erpnext/controllers/accounts_controller.py

@@ -4153,6 +4153,7 @@ def update_child_qty_rate(
 					#  if rate is greater than price_list_rate, set margin
 					#  or set discount
 					child_item.discount_percentage = 0
+					child_item.discount_amount = 0
 					child_item.margin_type = "Amount"
 					child_item.margin_rate_or_amount = flt(
 						child_item.rate - child_item.price_list_rate,
@@ -4160,14 +4161,11 @@ def update_child_qty_rate(
 					)
 					child_item.rate_with_margin = child_item.rate
 				else:
-					child_item.discount_percentage = flt(
-						(1 - flt(child_item.rate) / flt(child_item.price_list_rate)) * 100.0,
-						child_item.precision("discount_percentage"),
-					)
-					child_item.discount_amount = flt(child_item.price_list_rate) - flt(child_item.rate)
 					child_item.margin_type = ""
 					child_item.margin_rate_or_amount = 0
-					child_item.rate_with_margin = 0
+					child_item.rate_with_margin = child_item.price_list_rate
+					child_item.discount_percentage = 0
+					child_item.discount_amount = flt(child_item.rate_with_margin) - flt(child_item.rate)
 
 		child_item.flags.ignore_validate_update_after_submit = True
 		if new_child_flag:

erpnext/controllers/taxes_and_totals.py

@@ -165,83 +165,85 @@ class calculate_taxes_and_totals:
 
 		self.doc.conversion_rate = flt(self.doc.conversion_rate)
 
-	def calculate_item_values(self):
+	def calculate_item_rate(self, item):
-		if self.doc.get("is_consolidated"):
+		if not item.price_list_rate:
+			remove_margin(item)
+			remove_discount(item)
+			item.rate_with_margin = 0
 			return
 
-		if not self.discount_amount_applied:
+		has_pricing_rules = item.pricing_rules and not self.doc.ignore_pricing_rule
-			do_not_round_fields = ["valuation_rate", "incoming_rate", "sales_incoming_rate"]
+		if has_pricing_rules:
+			remove_margin(item)
 
-			for item in self.doc.items:
+			for d in get_applied_pricing_rules(item.pricing_rules):
-				self.doc.round_floats_in(item, do_not_round_fields=do_not_round_fields)
+				pricing_rule = frappe.get_cached_doc("Pricing Rule", d)
 
-				if item.discount_percentage == 100:
+				if not (
-					item.rate = 0.0
+					pricing_rule.margin_type
-				elif item.price_list_rate:
+					and pricing_rule.margin_rate_or_amount
-					if not item.rate or (item.pricing_rules and item.discount_percentage > 0):
+					and (
-						item.rate = flt(
+						pricing_rule.margin_type == "Percentage" or pricing_rule.currency == self.doc.currency
-							item.price_list_rate * (1.0 - (item.discount_percentage / 100.0)),
-							item.precision("rate"),
-						)
-
-						item.discount_amount = item.price_list_rate * (item.discount_percentage / 100.0)
-
-					elif item.discount_amount and item.pricing_rules:
-						item.rate = item.price_list_rate - item.discount_amount
-
-				if item.doctype in [
-					"Quotation Item",
-					"Sales Order Item",
-					"Delivery Note Item",
-					"Sales Invoice Item",
-					"POS Invoice Item",
-					"Purchase Invoice Item",
-					"Purchase Order Item",
-					"Purchase Receipt Item",
-				]:
-					item.rate_with_margin, item.base_rate_with_margin = self.calculate_margin(item)
-					if flt(item.rate_with_margin) > 0:
-						item.rate = flt(
-							item.rate_with_margin * (1.0 - (item.discount_percentage / 100.0)),
-							item.precision("rate"),
-						)
-
-						if item.discount_amount and not item.discount_percentage:
-							item.rate = item.rate_with_margin - item.discount_amount
-						else:
-							item.discount_amount = flt(
-								item.rate_with_margin - item.rate, item.precision("discount_amount")
-							)
-
-					elif flt(item.price_list_rate) > 0:
-						item.discount_amount = flt(
-							item.price_list_rate - item.rate, item.precision("discount_amount")
-						)
-				elif flt(item.price_list_rate) > 0 and not item.discount_amount:
-					item.discount_amount = flt(
-						item.price_list_rate - item.rate, item.precision("discount_amount")
 					)
-
-				item.net_rate = item.rate
-
-				if (
-					not item.qty
-					and self.doc.get("is_return")
-					and self.doc.get("doctype") != "Purchase Receipt"
 				):
-					item.amount = flt(-1 * item.rate, item.precision("amount"))
+					continue
-				elif not item.qty and self.doc.get("is_debit_note"):
-					item.amount = flt(item.rate, item.precision("amount"))
-				else:
-					item.amount = flt(item.rate * item.qty, item.precision("amount"))
 
-				item.net_amount = item.amount
+				item.margin_type = pricing_rule.margin_type
+				item.margin_rate_or_amount = pricing_rule.margin_rate_or_amount
 
-				self._set_in_company_currency(
+		item.rate_with_margin = get_rate_with_margin(item)
-					item, ["price_list_rate", "rate", "net_rate", "amount", "net_amount"]
+		if item.discount_percentage > 0:
-				)
+			item.discount_amount = flt(
+				item.rate_with_margin * item.discount_percentage / 100.0, item.precision("discount_amount")
+			)
 
-				item.item_tax_amount = 0.0
+		calculated_rate = flt(item.rate_with_margin - item.discount_amount, item.precision("rate"))
+
+		# if rate is 0 or pricing rules are applicable, calculated rate is preferred
+		if has_pricing_rules or not item.rate:
+			item.rate = calculated_rate
+			return
+
+		# discount and margin are correct, exit early
+		if item.rate == calculated_rate:
+			return
+
+		# item rate does not match calculated rate. prefer item rate, reset margin / discount
+		if item.rate > item.price_list_rate:
+			item.margin_type = "Amount"
+			item.margin_rate_or_amount = flt(
+				item.rate - item.price_list_rate, item.precision("margin_rate_or_amount")
+			)
+			item.rate_with_margin = item.rate
+			remove_discount(item)
+			return
+
+		item.rate_with_margin = item.price_list_rate
+		item.discount_amount = flt(item.rate_with_margin - item.rate, item.precision("discount_amount"))
+		item.discount_percentage = 0
+		remove_margin(item)
+
+	def calculate_item_values(self):
+		if self.doc.get("is_consolidated") or self.discount_amount_applied:
+			return
+
+		do_not_round_fields = ["valuation_rate", "incoming_rate", "sales_incoming_rate"]
+		for item in self.doc.items:
+			self.doc.round_floats_in(item, do_not_round_fields=do_not_round_fields)
+			self.calculate_item_rate(item)
+
+			item.net_rate = item.rate
+			if not item.qty and self.doc.get("is_return") and self.doc.get("doctype") != "Purchase Receipt":
+				item.amount = flt(-1 * item.rate, item.precision("amount"))
+			elif not item.qty and self.doc.get("is_debit_note"):
+				item.amount = flt(item.rate, item.precision("amount"))
+			else:
+				item.amount = flt(item.rate * item.qty, item.precision("amount"))
+			item.net_amount = item.amount
+			self._set_in_company_currency(
+				item, ["price_list_rate", "rate_with_margin", "rate", "net_rate", "amount", "net_amount"]
+			)
+			item.item_tax_amount = 0.0
 
 	def _set_in_company_currency(self, doc, fields):
 		"""set values in base currency"""
@@ -1135,48 +1137,6 @@ class calculate_taxes_and_totals:
 
 			self.calculate_outstanding_amount()
 
-	def calculate_margin(self, item):
-		rate_with_margin = 0.0
-		base_rate_with_margin = 0.0
-		if item.price_list_rate:
-			if item.pricing_rules and not self.doc.ignore_pricing_rule:
-				has_margin = False
-				for d in get_applied_pricing_rules(item.pricing_rules):
-					pricing_rule = frappe.get_cached_doc("Pricing Rule", d)
-
-					if pricing_rule.margin_rate_or_amount and (
-						(
-							pricing_rule.currency == self.doc.currency
-							and pricing_rule.margin_type in ["Amount", "Percentage"]
-						)
-						or pricing_rule.margin_type == "Percentage"
-					):
-						item.margin_type = pricing_rule.margin_type
-						item.margin_rate_or_amount = pricing_rule.margin_rate_or_amount
-						has_margin = True
-
-				if not has_margin:
-					item.margin_type = None
-					item.margin_rate_or_amount = 0.0
-
-			if not item.pricing_rules and flt(item.rate) > flt(item.price_list_rate):
-				item.margin_type = "Amount"
-				item.margin_rate_or_amount = flt(
-					item.rate - item.price_list_rate, item.precision("margin_rate_or_amount")
-				)
-				item.rate_with_margin = item.rate
-
-			elif item.margin_type and item.margin_rate_or_amount:
-				margin_value = (
-					item.margin_rate_or_amount
-					if item.margin_type == "Amount"
-					else flt(item.price_list_rate) * flt(item.margin_rate_or_amount) / 100
-				)
-				rate_with_margin = flt(item.price_list_rate) + flt(margin_value)
-				base_rate_with_margin = flt(rate_with_margin) * flt(self.doc.conversion_rate)
-
-		return rate_with_margin, base_rate_with_margin
-
 	def set_item_wise_tax_breakup(self):
 		self.doc.other_charges_calculation = get_itemised_tax_breakup_html(self.doc)
 
@@ -1211,6 +1171,29 @@ class calculate_taxes_and_totals:
 				)
 
 
+def remove_discount(item):
+	item.discount_percentage = 0.0
+	item.discount_amount = 0.0
+
+
+def remove_margin(item):
+	item.margin_type = None
+	item.margin_rate_or_amount = 0.0
+
+
+def get_rate_with_margin(item):
+	if not item.margin_type:
+		return item.price_list_rate
+
+	if item.margin_type == "Percentage":
+		return flt(
+			item.price_list_rate * (1 + (item.margin_rate_or_amount / 100.0)),
+			item.precision("rate_with_margin"),
+		)
+
+	return flt(item.price_list_rate + item.margin_rate_or_amount, item.precision("rate_with_margin"))
+
+
 def get_itemised_tax_breakup_html(doc):
 	if not doc.taxes:
 		return

erpnext/public/js/controllers/taxes_and_totals.js

@@ -10,29 +10,30 @@ erpnext.taxes_and_totals = class TaxesAndTotals extends erpnext.payments {
 
 	apply_pricing_rule_on_item(item) {
 		let effective_item_rate = item.price_list_rate;
-		let item_rate = item.rate;
 		if (["Sales Order", "Quotation"].includes(item.parenttype) && item.blanket_order_rate) {
 			effective_item_rate = item.blanket_order_rate;
 		}
+
+		let rate_with_margin;
 		if (item.margin_type == "Percentage") {
-			item.rate_with_margin =
+			rate_with_margin = effective_item_rate * (1 + item.margin_rate_or_amount / 100);
-				flt(effective_item_rate) + flt(effective_item_rate) * (flt(item.margin_rate_or_amount) / 100);
 		} else {
-			item.rate_with_margin = flt(effective_item_rate) + flt(item.margin_rate_or_amount);
+			rate_with_margin = effective_item_rate + item.margin_rate_or_amount;
 		}
-		item.base_rate_with_margin = flt(item.rate_with_margin) * flt(this.frm.doc.conversion_rate);
+		item.rate_with_margin = flt(rate_with_margin, precision("rate_with_margin", item));
 
-		item_rate = flt(item.rate_with_margin, precision("rate", item));
+		if (item.discount_percentage) {
-
+			item.discount_amount = flt(
-		if (item.discount_percentage && !item.discount_amount) {
+				(item.rate_with_margin * item.discount_percentage) / 100,
-			item.discount_amount = (flt(item.rate_with_margin) * flt(item.discount_percentage)) / 100;
+				precision("discount_amount", item)
+			);
 		}
 
-		if (item.discount_amount > 0) {
+		let item_rate = item.rate_with_margin;
-			item_rate = flt(item.rate_with_margin - item.discount_amount, precision("rate", item));
+		if (item.discount_amount) {
-			item.discount_percentage = (100 * flt(item.discount_amount)) / flt(item.rate_with_margin);
+			item_rate = item.rate_with_margin - item.discount_amount;
 		}
-
+		item_rate = flt(item_rate, precision("rate", item));
 		frappe.model.set_value(item.doctype, item.name, "rate", item_rate);
 	}
 

erpnext/public/js/controllers/transaction.js

@@ -13,39 +13,58 @@ erpnext.TransactionController = class TransactionController extends erpnext.taxe
 		frappe.flags.hide_serial_batch_dialog = true;
 		frappe.ui.form.on(this.frm.doctype + " Item", "rate", function (frm, cdt, cdn) {
 			var item = frappe.get_doc(cdt, cdn);
-			var has_margin_field = frappe.meta.has_field(cdt, "margin_type");
 
-			frappe.model.round_floats_in(item, ["rate", "price_list_rate"]);
+			frappe.model.round_floats_in(item, [
+				"rate",
+				"price_list_rate",
+				"margin_rate_or_amount",
+				"discount_amount",
+				"discount_percentage",
+			]);
 
 			if (item.price_list_rate && !item.blanket_order_rate) {
-				if (item.rate > item.price_list_rate && has_margin_field) {
+				const rate_with_margin = get_rate_with_margin(item);
+
+				if (item.discount_percentage) {
+					item.discount_amount = flt(
+						(rate_with_margin * item.discount_percentage) / 100.0,
+						precision("discount_amount", item)
+					);
+				}
+
+				const calculated_rate = flt(rate_with_margin - item.discount_amount, precision("rate", item));
+
+				if (calculated_rate !== item.rate) {
 					// if rate is greater than price_list_rate, set margin
-					// or set discount
+					// otherwise, set discount
-					item.discount_percentage = 0;
+					if (item.rate > item.price_list_rate) {
-					item.margin_type = "Amount";
+						item.margin_type = "Amount";
-					item.margin_rate_or_amount = flt(
+						item.margin_rate_or_amount = flt(
-						item.rate - item.price_list_rate,
+							item.rate - item.price_list_rate,
-						precision("margin_rate_or_amount", item)
+							precision("margin_rate_or_amount", item)
-					);
+						);
-					item.rate_with_margin = item.rate;
+						item.rate_with_margin = item.rate;
-				} else {
+						item.discount_amount = 0;
-					item.discount_percentage = flt(
+						item.discount_percentage = 0;
-						(1 - item.rate / item.price_list_rate) * 100.0,
+					} else {
-						precision("discount_percentage", item)
+						item.margin_type = "";
-					);
+						item.margin_rate_or_amount = 0;
-					item.discount_amount = flt(item.price_list_rate) - flt(item.rate);
+						item.rate_with_margin = item.price_list_rate;
-					item.margin_type = "";
+						item.discount_percentage = 0;
-					item.margin_rate_or_amount = 0;
+						item.discount_amount = flt(
-					item.rate_with_margin = 0;
+							item.rate_with_margin - item.rate,
+							precision("discount_amount", item)
+						);
+					}
 				}
 			} else {
-				item.discount_percentage = 0.0;
 				item.margin_type = "";
 				item.margin_rate_or_amount = 0;
 				item.rate_with_margin = 0;
+				item.discount_amount = 0;
+				item.discount_percentage = 0.0;
 			}
-			item.base_rate_with_margin = item.rate_with_margin * flt(frm.doc.conversion_rate);
+			me.set_in_company_currency(item, ["rate_with_margin"]);
-
 			cur_frm.cscript.set_gross_profit(item);
 			cur_frm.cscript.calculate_taxes_and_totals();
 			cur_frm.cscript.calculate_stock_uom_rate(frm, cdt, cdn);
@@ -3363,3 +3382,13 @@ erpnext.set_unit_price_items_note = (frm) => {
 		);
 	}
 };
+
+function get_rate_with_margin(item) {
+	if (!item.margin_type) return item.price_list_rate;
+
+	if (item.margin_type === "Percentage") {
+		return flt(item.price_list_rate * (1 + item.margin_rate_or_amount / 100), precision("rate", item));
+	}
+
+	return flt(item.price_list_rate + item.margin_rate_or_amount, precision("rate", item));
+}

erpnext/selling/doctype/quotation/test_quotation.py

@@ -403,9 +403,9 @@ class TestQuotation(ERPNextTestSuite):
 		quotation.save()
 		quotation.submit()
 
-		self.assertEqual(quotation.payment_schedule[0].payment_amount, 8906.00)
+		self.assertEqual(quotation.payment_schedule[0].payment_amount, 500.00)
 		self.assertEqual(quotation.payment_schedule[0].due_date, quotation.transaction_date)
-		self.assertEqual(quotation.payment_schedule[1].payment_amount, 8906.00)
+		self.assertEqual(quotation.payment_schedule[1].payment_amount, 500.00)
 		self.assertEqual(quotation.payment_schedule[1].due_date, add_days(quotation.transaction_date, 30))
 
 		sales_order = make_sales_order(quotation.name)
@@ -425,11 +425,11 @@ class TestQuotation(ERPNextTestSuite):
 		sales_order.set("taxes", [])
 		sales_order.save()
 
-		self.assertEqual(sales_order.payment_schedule[0].payment_amount, 8906.00)
+		self.assertEqual(sales_order.payment_schedule[0].payment_amount, 500.00)
 		self.assertEqual(
 			getdate(sales_order.payment_schedule[0].due_date), getdate(quotation.transaction_date)
 		)
-		self.assertEqual(sales_order.payment_schedule[1].payment_amount, 8906.00)
+		self.assertEqual(sales_order.payment_schedule[1].payment_amount, 500.00)
 		self.assertEqual(
 			getdate(sales_order.payment_schedule[1].due_date),
 			getdate(add_days(quotation.transaction_date, 30)),
@@ -465,11 +465,13 @@ class TestQuotation(ERPNextTestSuite):
 
 		rate_with_margin = flt((1500 * 18.75) / 100 + 1500)
 
-		test_record = dict(self.globalTestRecords["Quotation"][0])
+		test_record = frappe.copy_doc(self.globalTestRecords["Quotation"][0])
 
-		test_record["items"][0]["price_list_rate"] = 1500
+		test_record.items[0].price_list_rate = 1500
-		test_record["items"][0]["margin_type"] = "Percentage"
+		test_record.items[0].margin_type = "Percentage"
-		test_record["items"][0]["margin_rate_or_amount"] = 18.75
+		test_record.items[0].margin_rate_or_amount = 18.75
+		# set rate to zero, so that it is recalculated on save
+		test_record.items[0].rate = 0
 
 		quotation = frappe.copy_doc(test_record)
 		quotation.transaction_date = nowdate()

erpnext/selling/doctype/sales_order/test_sales_order.py

@@ -1473,6 +1473,8 @@ class TestSalesOrder(ERPNextTestSuite):
 		so.items[0].price_list_rate = price_list_rate = 100
 		so.items[0].margin_type = "Percentage"
 		so.items[0].margin_rate_or_amount = 25
+		# set rate to zero, so that it is recalculated on save
+		so.items[0].rate = 0
 		so.save()
 
 		new_so = frappe.copy_doc(so)

erpnext/utilities/transaction_base.py

@@ -514,18 +514,15 @@ class TransactionBase(StatusUpdater):
 		item_obj.base_rate_with_margin = flt(item_obj.rate_with_margin) * flt(self.conversion_rate)
 		item_rate = flt(item_obj.rate_with_margin, item_obj.precision("rate"))
 
-		if item_obj.discount_percentage and not item_obj.discount_amount:
+		if item_obj.discount_percentage:
 			item_obj.discount_amount = (
 				flt(item_obj.rate_with_margin) * flt(item_obj.discount_percentage) / 100
 			)
 
-		if item_obj.discount_amount and item_obj.discount_amount > 0:
+		if item_obj.discount_amount:
 			item_rate = flt(
 				(item_obj.rate_with_margin) - (item_obj.discount_amount), item_obj.precision("rate")
 			)
-			item_obj.discount_percentage = (
-				100 * flt(item_obj.discount_amount) / flt(item_obj.rate_with_margin)
-			)
 
 		item_obj.rate = item_rate