From 26d0821c939025d322364636cc624859fde13756 Mon Sep 17 00:00:00 2001 From: Nabin Hait Date: Sun, 21 Jun 2026 12:51:07 +0530 Subject: [PATCH] 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. --- .../supplier_scorecard/supplier_scorecard.py | 68 ++++++++++--------- .../test_supplier_scorecard.py | 15 ++++ .../supplier_scorecard_period.py | 1 - .../supplier_scorecard_variable.py | 20 +++--- .../test_supplier_scorecard_variable.py | 14 ++++ 5 files changed, 74 insertions(+), 44 deletions(-) diff --git a/erpnext/buying/doctype/supplier_scorecard/supplier_scorecard.py b/erpnext/buying/doctype/supplier_scorecard/supplier_scorecard.py index fb3a4b10396..7a1db02082e 100644 --- a/erpnext/buying/doctype/supplier_scorecard/supplier_scorecard.py +++ b/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() diff --git a/erpnext/buying/doctype/supplier_scorecard/test_supplier_scorecard.py b/erpnext/buying/doctype/supplier_scorecard/test_supplier_scorecard.py index 16485c0c57a..fb55ff55505 100644 --- a/erpnext/buying/doctype/supplier_scorecard/test_supplier_scorecard.py +++ b/erpnext/buying/doctype/supplier_scorecard/test_supplier_scorecard.py @@ -35,6 +35,21 @@ class TestSupplierScorecard(ERPNextTestSuite): 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" diff --git a/erpnext/buying/doctype/supplier_scorecard_period/supplier_scorecard_period.py b/erpnext/buying/doctype/supplier_scorecard_period/supplier_scorecard_period.py index 5311ef002e6..c7200dd345f 100644 --- a/erpnext/buying/doctype/supplier_scorecard_period/supplier_scorecard_period.py +++ b/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 diff --git a/erpnext/buying/doctype/supplier_scorecard_variable/supplier_scorecard_variable.py b/erpnext/buying/doctype/supplier_scorecard_variable/supplier_scorecard_variable.py index 99b780375fa..0700118c064 100644 --- a/erpnext/buying/doctype/supplier_scorecard_variable/supplier_scorecard_variable.py +++ b/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): diff --git a/erpnext/buying/doctype/supplier_scorecard_variable/test_supplier_scorecard_variable.py b/erpnext/buying/doctype/supplier_scorecard_variable/test_supplier_scorecard_variable.py index 27cc5deb115..175ddbc97a4 100644 --- a/erpnext/buying/doctype/supplier_scorecard_variable/test_supplier_scorecard_variable.py +++ b/erpnext/buying/doctype/supplier_scorecard_variable/test_supplier_scorecard_variable.py @@ -60,6 +60,20 @@ class TestSupplierScorecardVariable(ERPNextTestSuite): 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):