mirror of
https://github.com/frappe/erpnext.git
synced 2026-09-15 09:53:09 +00:00
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.
This commit is contained in:
@@ -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()
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -82,7 +82,6 @@ class SupplierScorecardPeriod(Document):
|
||||
).format(crit.criteria_name),
|
||||
frappe.ValidationError,
|
||||
)
|
||||
crit.score = 0
|
||||
|
||||
def calculate_score(self):
|
||||
myscore = 0
|
||||
|
||||
@@ -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):
|
||||
|
||||
@@ -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):
|
||||
|
||||
Reference in New Issue
Block a user