fix: correct Supplier Scorecard standing and on-time shipment logic

Bugs surfaced while writing coverage for the scorecard engine:

- update_standing treated every band as [min, max), leaving the global
  ceiling open, so a perfect score (100) - including the no-period
  fallback - mapped to no standing. Make the top band inclusive of its
  upper bound.
- get_on_time_shipments counted PR lines where qty exactly matched the PO
  line, so on-time deliveries split across partial receipts were never
  counted while still inflating late shipments (and could push
  get_late_shipments negative). Count fully-on-time PO lines instead,
  keeping units consistent with get_total_shipments.
- validate_standings now rejects inverted bands (min >= max) and checks
  band continuity directly instead of relying on fragile float-equality
  accumulation.
- Remove dead 'crit.score = 0' after frappe.throw in calculate_criteria.

erpnext/buying/doctype/supplier_scorecard/supplier_scorecard.py

@@ -60,25 +60,20 @@ class SupplierScorecard(Document):
 			self.save()
 
 	def validate_standings(self):
-		# Check that there are no overlapping scores and check that there are no missing scores
-		score = 0
-		for c1 in self.standings:
-			for c2 in self.standings:
-				if c1 != c2:
-					if c1.max_grade > c2.min_grade and c1.min_grade < c2.max_grade:
-						throw(
-							_("Overlap in scoring between {0} and {1}").format(
-								c1.standing_name, c2.standing_name
-							)
-						)
-				if c2.min_grade == score:
-					score = c2.max_grade
-		if score < 100:
-			throw(
-				_(
-					"Unable to find score starting at {0}. You need to have standing scores covering 0 to 100"
-				).format(score)
-			)
+		# Standings must form a continuous chain of bands covering 0 to 100 with no gaps or overlaps
+		expected_min = 0
+		for standing in sorted(self.standings, key=lambda s: s.min_grade or 0):
+			if standing.min_grade >= standing.max_grade:
+				throw(
+					_("Standing {0} must have a minimum grade lower than its maximum grade").format(
+						standing.standing_name
+					)
+				)
+			if standing.min_grade != expected_min:
+				throw(_("Standing scores must be continuous and cover 0 to 100 without gaps or overlaps"))
+			expected_min = standing.max_grade
+		if expected_min < 100:
+			throw(_("Standing scores must cover the full range from 0 to 100"))
 
 	def validate_criteria_weights(self):
 		weight = 0
@@ -119,22 +114,29 @@ class SupplierScorecard(Document):
 			self.supplier_score = 100
 
 	def update_standing(self):
-		# Get the setup document
-
+		highest_grade = max((s.max_grade for s in self.standings if s.max_grade), default=0)
 		for standing in self.standings:
-			if (not standing.min_grade or (standing.min_grade <= self.supplier_score)) and (
-				not standing.max_grade or (standing.max_grade > self.supplier_score)
-			):
-				self.status = standing.standing_name
-				self.indicator_color = standing.standing_color
-				self.notify_supplier = standing.notify_supplier
-				self.notify_employee = standing.notify_employee
-				self.employee_link = standing.employee_link
+			if self.score_within_standing(standing, highest_grade):
+				self.apply_standing(standing)
 
-				# Update supplier standing info
-				for fieldname in ("prevent_pos", "prevent_rfqs", "warn_rfqs", "warn_pos"):
-					self.set(fieldname, standing.get(fieldname))
-					frappe.db.set_value("Supplier", self.supplier, fieldname, self.get(fieldname))
+	def score_within_standing(self, standing, highest_grade):
+		score = self.supplier_score
+		above_min = not standing.min_grade or standing.min_grade <= score
+		if standing.max_grade and standing.max_grade == highest_grade:
+			# Top band is inclusive of its upper bound so a perfect score still maps to a standing
+			return above_min and score <= standing.max_grade
+		return above_min and (not standing.max_grade or standing.max_grade > score)
+
+	def apply_standing(self, standing):
+		self.status = standing.standing_name
+		self.indicator_color = standing.standing_color
+		self.notify_supplier = standing.notify_supplier
+		self.notify_employee = standing.notify_employee
+		self.employee_link = standing.employee_link
+
+		for fieldname in ("prevent_pos", "prevent_rfqs", "warn_rfqs", "warn_pos"):
+			self.set(fieldname, standing.get(fieldname))
+			frappe.db.set_value("Supplier", self.supplier, fieldname, self.get(fieldname))
 
 
 @frappe.whitelist()

erpnext/buying/doctype/supplier_scorecard/test_supplier_scorecard.py

@@ -3,7 +3,12 @@
 
 
 import frappe
+from frappe.utils import add_days, getdate, nowdate
 
+from erpnext.buying.doctype.supplier_scorecard.supplier_scorecard import (
+	get_scorecard_date,
+	make_all_scorecards,
+)
 from erpnext.tests.utils import ERPNextTestSuite
 
 
@@ -18,6 +23,71 @@ class TestSupplierScorecard(ERPNextTestSuite):
 			d.weight = 0
 		self.assertRaises(frappe.ValidationError, my_doc.insert)
 
+	def test_overlapping_standings_are_rejected(self):
+		doc = make_supplier_scorecard()
+		# "Poor" (30-50) stretched to 60 now overlaps "Average" (50-80)
+		doc.standings[1].max_grade = 60
+		self.assertRaises(frappe.ValidationError, doc.validate_standings)
+
+	def test_standings_must_cover_full_range(self):
+		doc = make_supplier_scorecard()
+		# "Excellent" capped at 90 leaves the 90-100 band uncovered
+		doc.standings[3].max_grade = 90
+		self.assertRaises(frappe.ValidationError, doc.validate_standings)
+
+	def test_inverted_standing_band_rejected(self):
+		doc = make_supplier_scorecard()
+		doc.standings = []
+		doc.append("standings", {"standing_name": "Inverted", "min_grade": 60, "max_grade": 40})
+		self.assertRaises(frappe.ValidationError, doc.validate_standings)
+
+	def test_perfect_score_maps_to_top_standing(self):
+		# A perfect score (the upper bound of the top band) must still resolve to a standing
+		supplier = create_test_supplier("_Test Supplier SC Perfect")
+		doc = make_supplier_scorecard()
+		doc.supplier = supplier
+		doc.supplier_score = 100
+		doc.update_standing()
+		self.assertEqual(doc.status, "Excellent")
+
+	def test_total_score_defaults_to_100_without_periods(self):
+		doc = make_supplier_scorecard()
+		doc.name = "_Test Scorecard Without Periods"
+		doc.calculate_total_score()
+		self.assertEqual(doc.supplier_score, 100)
+
+	def test_update_standing_propagates_blocking_flags_to_supplier(self):
+		supplier = create_test_supplier("_Test Supplier SC Standing")
+		doc = make_supplier_scorecard()
+		doc.supplier = supplier
+		doc.supplier_score = 20  # falls in the "Very Poor" (0-30) band
+		doc.update_standing()
+
+		self.assertEqual(doc.status, "Very Poor")
+		self.assertEqual(doc.prevent_pos, 1)
+		self.assertEqual(doc.prevent_rfqs, 1)
+		self.assertEqual(frappe.db.get_value("Supplier", supplier, "prevent_pos"), 1)
+		self.assertEqual(frappe.db.get_value("Supplier", supplier, "prevent_rfqs"), 1)
+
+	def test_scorecard_period_end_dates(self):
+		start = getdate("2024-01-01")
+		self.assertEqual(get_scorecard_date("Per Week", start), getdate("2024-01-08"))
+		self.assertEqual(get_scorecard_date("Per Month", start), getdate("2024-01-31"))
+		self.assertEqual(get_scorecard_date("Per Year", start), getdate("2024-12-31"))
+
+	def test_make_all_scorecards_is_idempotent(self):
+		supplier = create_test_supplier("_Test Supplier SC Idempotent")
+		frappe.db.set_value("Supplier", supplier, "creation", add_days(nowdate(), -75))
+
+		doc = make_supplier_scorecard()
+		doc.supplier = supplier
+		doc.name = supplier
+		doc.insert()  # on_update generates the period scorecards
+
+		created = frappe.db.count("Supplier Scorecard Period", {"scorecard": doc.name, "docstatus": 1})
+		self.assertGreater(created, 0)
+		self.assertEqual(make_all_scorecards(doc.name), 0)
+
 
 def make_supplier_scorecard():
 	my_doc = frappe.get_doc(valid_scorecard[0])
@@ -32,6 +102,18 @@ def make_supplier_scorecard():
 	return my_doc
 
 
+def create_test_supplier(supplier_name):
+	if not frappe.db.exists("Supplier", supplier_name):
+		frappe.get_doc(
+			{
+				"doctype": "Supplier",
+				"supplier_name": supplier_name,
+				"supplier_group": "_Test Supplier Group",
+			}
+		).insert()
+	return supplier_name
+
+
 valid_scorecard = [
 	{
 		"standings": [

erpnext/buying/doctype/supplier_scorecard_period/supplier_scorecard_period.py

@@ -82,7 +82,6 @@ class SupplierScorecardPeriod(Document):
 					).format(crit.criteria_name),
 					frappe.ValidationError,
 				)
-				crit.score = 0
 
 	def calculate_score(self):
 		myscore = 0

erpnext/buying/doctype/supplier_scorecard_period/test_supplier_scorecard_period.py

@@ -1,8 +1,64 @@
 # Copyright (c) 2017, Frappe Technologies Pvt. Ltd. and Contributors
 # See license.txt
 
+import frappe
+
 from erpnext.tests.utils import ERPNextTestSuite
 
 
 class TestSupplierScorecardPeriod(ERPNextTestSuite):
-	pass
+	def test_criteria_score_is_clamped_to_bounds(self):
+		period = make_period(
+			criteria=[
+				{"criteria_name": "Over", "formula": "200", "max_score": 100, "weight": 50},
+				{"criteria_name": "Negative", "formula": "-50", "max_score": 100, "weight": 50},
+			]
+		)
+		period.calculate_criteria()
+
+		self.assertEqual(period.criteria[0].score, 100)  # capped at max_score
+		self.assertEqual(period.criteria[1].score, 0)  # floored at zero
+
+	def test_invalid_criteria_formula_raises(self):
+		period = make_period(
+			criteria=[{"criteria_name": "Bad", "formula": "{missing} +", "max_score": 100, "weight": 100}]
+		)
+		self.assertRaises(frappe.ValidationError, period.calculate_criteria)
+
+	def test_eval_statement_substitutes_variable_values(self):
+		period = make_period(
+			variables=[
+				{"variable_label": "A", "param_name": "a", "path": "get_total_workdays", "value": 5},
+				{"variable_label": "B", "param_name": "b", "path": "get_total_workdays", "value": 0},
+			]
+		)
+		# present value -> formatted; missing/zero value -> "0.0"
+		self.assertEqual(period.get_eval_statement("{a} + {b}"), "5.00 + 0.0")
+
+	def test_period_score_is_weighted_sum_of_criteria(self):
+		period = make_period(
+			criteria=[
+				{"criteria_name": "C1", "formula": "80", "max_score": 100, "weight": 25},
+				{"criteria_name": "C2", "formula": "40", "max_score": 100, "weight": 75},
+			]
+		)
+		period.calculate_criteria()
+		period.calculate_score()
+
+		# 80 * 0.25 + 40 * 0.75 = 50
+		self.assertEqual(period.total_score, 50)
+
+	def test_criteria_weights_must_total_100(self):
+		period = make_period(
+			criteria=[{"criteria_name": "C1", "formula": "100", "max_score": 100, "weight": 60}]
+		)
+		self.assertRaises(frappe.ValidationError, period.validate_criteria_weights)
+
+
+def make_period(variables=None, criteria=None):
+	period = frappe.new_doc("Supplier Scorecard Period")
+	for variable in variables or []:
+		period.append("variables", variable)
+	for criterion in criteria or []:
+		period.append("criteria", criterion)
+	return period

erpnext/buying/doctype/supplier_scorecard_variable/supplier_scorecard_variable.py

@@ -8,7 +8,7 @@ import frappe
 from frappe import _
 from frappe.model.document import Document
 from frappe.query_builder.functions import DateDiff, Sum
-from frappe.utils import getdate
+from frappe.utils import flt, getdate
 
 
 class VariablePathNotFound(frappe.ValidationError):
@@ -184,16 +184,18 @@ def get_total_days_late(scorecard):
 
 
 def get_on_time_shipments(scorecard):
-	"""Gets the number of on time shipments (counting each item) in the period (based on Purchase Receipts vs POs)"""
+	"""Counts PO lines (scheduled in the period) fully received on or before their schedule date.
 
-	from frappe.query_builder.functions import Count
+	Counting in PO-line units keeps this consistent with get_total_shipments so that
+	get_late_shipments (total - on time) stays non-negative even for split deliveries.
+	"""
 
 	PO = frappe.qb.DocType("Purchase Order")
 	PO_Item = frappe.qb.DocType("Purchase Order Item")
 	PR = frappe.qb.DocType("Purchase Receipt")
 	PR_Item = frappe.qb.DocType("Purchase Receipt Item")
 
-	query = (
+	rows = (
 		frappe.qb.from_(PR_Item)
 		.join(PR)
 		.on(PR_Item.parent == PR.name)
@@ -201,17 +203,15 @@ def get_on_time_shipments(scorecard):
 		.on(PR_Item.purchase_order_item == PO_Item.name)
 		.join(PO)
 		.on(PO_Item.parent == PO.name)
-		.select(Count(PR_Item.qty))
+		.select(PO_Item.name, PO_Item.qty, Sum(PR_Item.qty).as_("received_on_time"))
 		.where(PO.supplier == scorecard.supplier)
 		.where(PO_Item.schedule_date[scorecard.start_date : scorecard.end_date])
 		.where(PO_Item.schedule_date >= PR.posting_date)
-		.where(PO_Item.qty == PR_Item.qty)
 		.where(PR_Item.docstatus == 1)
-	)
+		.groupby(PO_Item.name, PO_Item.qty)
+	).run(as_dict=True)
 
-	result = query.run(as_list=True)
-	total_items_delivered_on_time = result[0][0] if result and result[0][0] is not None else 0
-	return total_items_delivered_on_time
+	return sum(1 for row in rows if flt(row.received_on_time) >= flt(row.qty))
 
 
 def get_late_shipments(scorecard):

erpnext/buying/doctype/supplier_scorecard_variable/test_supplier_scorecard_variable.py

@@ -3,9 +3,15 @@
 
 
 import frappe
+from frappe.utils import add_days, nowdate
 
+from erpnext.buying.doctype.purchase_order.mapper import make_purchase_receipt as make_pr_from_po
+from erpnext.buying.doctype.purchase_order.test_purchase_order import create_purchase_order
 from erpnext.buying.doctype.supplier_scorecard_variable.supplier_scorecard_variable import (
 	VariablePathNotFound,
+	get_on_time_shipments,
+	get_total_cost_of_shipments,
+	get_total_days_late,
 )
 from erpnext.tests.utils import ERPNextTestSuite
 
@@ -27,6 +33,79 @@ class TestSupplierScorecardVariable(ERPNextTestSuite):
 		for d in test_bad_variables:
 			self.assertRaises(VariablePathNotFound, frappe.get_doc(d).insert)
 
+	def test_total_cost_of_shipments_counts_only_in_period(self):
+		supplier = create_scorecard_supplier()
+		create_scorecard_po(supplier, nowdate(), qty=10, rate=100)  # in period -> 1000
+		create_scorecard_po(supplier, add_days(nowdate(), 60), qty=5, rate=100)  # outside period
+
+		scorecard = scorecard_for(supplier)
+		self.assertEqual(get_total_cost_of_shipments(scorecard), 1000)
+
+	def test_on_time_and_delayed_shipments(self):
+		supplier = create_scorecard_supplier()
+		on_time_po = create_scorecard_po(supplier, add_days(nowdate(), 5), qty=10, rate=100)
+		late_po = create_scorecard_po(
+			supplier,
+			add_days(nowdate(), -5),
+			transaction_date=add_days(nowdate(), -10),
+			qty=10,
+			rate=100,
+		)
+		for po in (on_time_po, late_po):
+			receipt = make_pr_from_po(po.name)
+			receipt.insert()
+			receipt.submit()
+
+		scorecard = scorecard_for(supplier)
+		self.assertEqual(get_on_time_shipments(scorecard), 1)
+		self.assertEqual(get_total_days_late(scorecard), 50)  # 5 days late * 10 qty
+
+	def test_split_on_time_receipts_count_as_one_shipment(self):
+		# A PO line fully received on time across two partial receipts is one on-time shipment
+		supplier = create_scorecard_supplier()
+		po = create_scorecard_po(supplier, add_days(nowdate(), 5), qty=10, rate=100)
+		for received in (6, 4):
+			receipt = make_pr_from_po(po.name)
+			receipt.items[0].qty = received
+			receipt.items[0].received_qty = received
+			receipt.items[0].stock_qty = received
+			receipt.insert()
+			receipt.submit()
+
+		self.assertEqual(get_on_time_shipments(scorecard_for(supplier)), 1)
+
+
+def create_scorecard_supplier(supplier_name="_Test Supplier Scorecard"):
+	if not frappe.db.exists("Supplier", supplier_name):
+		frappe.get_doc(
+			{
+				"doctype": "Supplier",
+				"supplier_name": supplier_name,
+				"supplier_group": "_Test Supplier Group",
+			}
+		).insert()
+	return supplier_name
+
+
+def create_scorecard_po(supplier, schedule_date, transaction_date=None, qty=10, rate=100):
+	po = create_purchase_order(
+		supplier=supplier, transaction_date=transaction_date, qty=qty, rate=rate, do_not_save=True
+	)
+	po.schedule_date = schedule_date
+	po.items[0].schedule_date = schedule_date
+	po.set_missing_values()
+	po.insert()
+	po.submit()
+	return po
+
+
+def scorecard_for(supplier):
+	return frappe._dict(
+		supplier=supplier,
+		start_date=add_days(nowdate(), -30),
+		end_date=add_days(nowdate(), 30),
+	)
+
 
 test_existing_variables = [
 	{