From 992530c7064220a99c42bcd284465db05e2f0c44 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Wed, 29 Jul 2026 15:36:22 +0530 Subject: [PATCH] fix: keep progress percentages honest when a row is closed A closed row was counted as fully settled, so closing one of two unbilled Delivery Note rows pushed per_billed to 50 and the document read "Partially Billed" with nothing invoiced. The same inflation applied to per_received, per_delivered and per_picked. Closed rows now leave the denominator instead of counting as done, so a percentage stays a true measure of what was received, delivered or billed against what is still expected. Writing off one of two unbilled rows leaves per_billed at 0; billing the other takes it to 100. When every row is closed nothing is outstanding, so the percentage is 100 by definition. This mirrors how Delivery Note already excludes returned value from the billing denominator. Amending a cancelled document no longer inherits its write-offs. Frappe copies no_copy fields when amending so a document can be corrected and resubmitted, and unlike billed_amt or received_qty nothing recomputes `closed`, so the flag would silently keep a row out of billing on the new document. --- .../doctype/purchase_order/services/status.py | 13 ++++++--- erpnext/controllers/accounts_controller.py | 3 +++ erpnext/controllers/item_close.py | 14 ++++++++++ erpnext/controllers/status_updater.py | 19 +++++++------ erpnext/controllers/tests/test_item_close.py | 20 +++++++++++--- .../tests/test_item_close_billing.py | 27 +++++++++++++++---- .../tests/test_item_close_sales_order.py | 4 +-- .../doctype/sales_order/services/status.py | 6 ++--- .../services/billing_status.py | 10 +++++-- 9 files changed, 88 insertions(+), 28 deletions(-) diff --git a/erpnext/buying/doctype/purchase_order/services/status.py b/erpnext/buying/doctype/purchase_order/services/status.py index b9c43a5652e..cf799a71076 100644 --- a/erpnext/buying/doctype/purchase_order/services/status.py +++ b/erpnext/buying/doctype/purchase_order/services/status.py @@ -56,9 +56,16 @@ class StatusService: doc = self.doc total_qty, received_qty = 0.0, 0.0 for item in doc.items: - received_qty += item.qty if item.closed else min(item.received_qty, item.qty) + if item.closed: + continue + received_qty += min(item.received_qty, item.qty) total_qty += item.qty + if total_qty and received_qty: - doc.db_set("per_received", flt(received_qty / total_qty) * 100, update_modified=False) + per_received = flt(received_qty / total_qty) * 100 + elif doc.items and not total_qty: + per_received = 100 else: - doc.db_set("per_received", 0, update_modified=False) + per_received = 0 + + doc.db_set("per_received", per_received, update_modified=False) diff --git a/erpnext/controllers/accounts_controller.py b/erpnext/controllers/accounts_controller.py index 56e6e381bb5..0a02ab188e0 100644 --- a/erpnext/controllers/accounts_controller.py +++ b/erpnext/controllers/accounts_controller.py @@ -39,6 +39,7 @@ from erpnext.accounts.utils import ( get_advance_payment_doctypes as _get_advance_payment_doctypes, ) from erpnext.accounts.utils import get_fiscal_year, validate_fiscal_year +from erpnext.controllers.item_close import clear_closed_rows_on_amend from erpnext.controllers.print_settings import ( set_print_templates_for_item_table, set_print_templates_for_taxes, @@ -211,6 +212,8 @@ class AccountsController(TransactionBase): frappe.msgprint(msg) def validate(self): + clear_closed_rows_on_amend(self) + if not self.get("is_return") and not self.get("is_debit_note"): self.validate_qty_is_not_zero() diff --git a/erpnext/controllers/item_close.py b/erpnext/controllers/item_close.py index 911f9e907d5..f89b3c72a52 100644 --- a/erpnext/controllers/item_close.py +++ b/erpnext/controllers/item_close.py @@ -106,6 +106,20 @@ def reopen_parent_if_closed(doc) -> None: doc.update_status(REOPEN_STATUS[doc.doctype]) +def clear_closed_rows_on_amend(doc) -> None: + """An amended document starts with nothing written off. + + Frappe copies `no_copy` fields when amending so a cancelled document can be + corrected and resubmitted, which would otherwise carry a write-off decision + that was made against the cancelled document onto the new one. + """ + if not doc.is_new() or not doc.get("amended_from") or not has_closable_items(doc.doctype): + return + + for row in doc.get("items") or []: + row.closed = 0 + + def validate_parent_reopen(doc) -> None: """Block reopening a parent whose rows are all closed. diff --git a/erpnext/controllers/status_updater.py b/erpnext/controllers/status_updater.py index 8cea29bed61..a108d507390 100644 --- a/erpnext/controllers/status_updater.py +++ b/erpnext/controllers/status_updater.py @@ -674,20 +674,23 @@ class StatusUpdater(Document): # For operator dicts, the alias is in the "as" key; for strings, use the field name directly ref_key = target_ref_field.get("as") if isinstance(target_ref_field, dict) else target_ref_field - def settled(record): - """A closed row is settled in full, so it stops holding the parent open.""" - if tracks_closed_rows and record["closed"]: - return abs(record[ref_key]) + # A closed row is written off, so it leaves the denominator rather than + # counting as done. The percentage stays a true measure of what was + # actually received, delivered or billed against what is still expected. + open_records = [r for r in child_records if not (tracks_closed_rows and r["closed"])] - return min(abs(record[target_field]), abs(record[ref_key])) - - sum_ref = sum(abs(record[ref_key]) for record in child_records) + sum_ref = sum(abs(record[ref_key]) for record in open_records) if sum_ref > 0: percentage = round( - sum(settled(record) for record in child_records) / sum_ref * 100, + sum(min(abs(record[target_field]), abs(record[ref_key])) for record in open_records) + / sum_ref + * 100, 6, ) + elif child_records and not open_records: + # every row written off, so nothing is outstanding + percentage = 100 else: percentage = 0 diff --git a/erpnext/controllers/tests/test_item_close.py b/erpnext/controllers/tests/test_item_close.py index 4daa6f8b7ea..678ed6dfccf 100644 --- a/erpnext/controllers/tests/test_item_close.py +++ b/erpnext/controllers/tests/test_item_close.py @@ -17,9 +17,7 @@ WAREHOUSE = "_Test Warehouse - _TC" def get_ordered_qty(item_code): - return flt( - frappe.db.get_value("Bin", {"item_code": item_code, "warehouse": WAREHOUSE}, "ordered_qty") - ) + return flt(frappe.db.get_value("Bin", {"item_code": item_code, "warehouse": WAREHOUSE}, "ordered_qty")) class TestPurchaseOrderItemClose(ERPNextTestSuite): @@ -114,7 +112,8 @@ class TestPurchaseOrderItemClose(ERPNextTestSuite): self.assertEqual(po.status, "To Receive and Bill") self.assertTrue(po.items[0].closed) self.assertFalse(po.items[1].closed) - self.assertEqual(po.per_received, 50) + # nothing received, and the closed row is written off rather than counted + self.assertEqual(po.per_received, 0) self.assertEqual(get_ordered_qty(self.second_item), 10) self.assertEqual(get_ordered_qty(self.first_item), 0) @@ -215,3 +214,16 @@ class TestPurchaseOrderItemClose(ERPNextTestSuite): ["any-row"], 1, ) + + def test_amending_clears_closed_rows(self): + """Frappe keeps no_copy fields when amending, so the flag must be cleared.""" + po = self.make_purchase_order() + self.close_items(po, [po.items[1]]) + po.cancel() + + amended = frappe.copy_doc(po, ignore_no_copy=True) + amended.docstatus = 0 + amended.amended_from = po.name + amended.insert() + + self.assertFalse(any(row.closed for row in amended.items)) diff --git a/erpnext/controllers/tests/test_item_close_billing.py b/erpnext/controllers/tests/test_item_close_billing.py index d298fe89b27..e9faa25dd72 100644 --- a/erpnext/controllers/tests/test_item_close_billing.py +++ b/erpnext/controllers/tests/test_item_close_billing.py @@ -41,13 +41,15 @@ class TestPurchaseReceiptItemClose(ERPNextTestSuite): update_closed_status(doc.doctype, doc.name, [row.name for row in rows], closed) doc.reload() - def test_closing_row_settles_billing_percentage(self): + def test_closing_a_row_does_not_inflate_billing_percentage(self): receipt = self.make_purchase_receipt() self.assertEqual(receipt.per_billed, 0) self.close_items(receipt, [receipt.items[1]]) - self.assertEqual(receipt.per_billed, 50) + # nothing was billed, so the receipt must not read as partly billed + self.assertEqual(receipt.per_billed, 0) + self.assertEqual(receipt.status, "To Bill") def test_closing_every_row_closes_the_receipt(self): receipt = self.make_purchase_receipt() @@ -87,7 +89,7 @@ class TestPurchaseReceiptItemClose(ERPNextTestSuite): self.close_items(receipt, [receipt.items[1]], closed=0) self.assertNotEqual(receipt.status, "Closed") - self.assertEqual(receipt.per_billed, 50) + self.assertEqual(receipt.per_billed, 0) class TestDeliveryNoteItemClose(ERPNextTestSuite): @@ -118,13 +120,15 @@ class TestDeliveryNoteItemClose(ERPNextTestSuite): update_closed_status(doc.doctype, doc.name, [row.name for row in rows], closed) doc.reload() - def test_closing_row_settles_billing_percentage(self): + def test_closing_a_row_does_not_inflate_billing_percentage(self): note = self.make_delivery_note() self.assertEqual(note.per_billed, 0) self.close_items(note, [note.items[1]]) - self.assertEqual(note.per_billed, 50) + # nothing was billed, so the note must not read as partially billed + self.assertEqual(note.per_billed, 0) + self.assertEqual(note.status, "To Bill") def test_closing_every_row_closes_the_note(self): note = self.make_delivery_note() @@ -158,3 +162,16 @@ class TestDeliveryNoteItemClose(ERPNextTestSuite): self.assertEqual(note.per_returned, 0) self.assertEqual(note.status, "Closed") + + def test_amending_clears_closed_rows(self): + """Frappe keeps no_copy fields when amending, so the flag must be cleared.""" + note = self.make_delivery_note() + self.close_items(note, [note.items[1]]) + note.cancel() + + amended = frappe.copy_doc(note, ignore_no_copy=True) + amended.docstatus = 0 + amended.amended_from = note.name + amended.insert() + + self.assertFalse(any(row.closed for row in amended.items)) diff --git a/erpnext/controllers/tests/test_item_close_sales_order.py b/erpnext/controllers/tests/test_item_close_sales_order.py index fe233daff84..ad45d6ead26 100644 --- a/erpnext/controllers/tests/test_item_close_sales_order.py +++ b/erpnext/controllers/tests/test_item_close_sales_order.py @@ -15,9 +15,7 @@ WAREHOUSE = "_Test Warehouse - _TC" def get_reserved_qty(item_code): - return flt( - frappe.db.get_value("Bin", {"item_code": item_code, "warehouse": WAREHOUSE}, "reserved_qty") - ) + return flt(frappe.db.get_value("Bin", {"item_code": item_code, "warehouse": WAREHOUSE}, "reserved_qty")) class TestSalesOrderItemClose(ERPNextTestSuite): diff --git a/erpnext/selling/doctype/sales_order/services/status.py b/erpnext/selling/doctype/sales_order/services/status.py index 142cfe01adb..bb6921cc232 100644 --- a/erpnext/selling/doctype/sales_order/services/status.py +++ b/erpnext/selling/doctype/sales_order/services/status.py @@ -119,12 +119,12 @@ class StatusService: per_picked = 0.0 for so_item in doc.items: + if so_item.closed: + continue if cint( frappe.get_cached_value("Item", so_item.item_code, "is_stock_item") ) or doc.has_product_bundle(so_item.item_code): - total_picked_qty += ( - flt(so_item.stock_qty) if so_item.closed else flt(so_item.picked_qty) - ) + total_picked_qty += flt(so_item.picked_qty) total_qty += flt(so_item.stock_qty) if total_picked_qty and total_qty: diff --git a/erpnext/stock/doctype/purchase_receipt/services/billing_status.py b/erpnext/stock/doctype/purchase_receipt/services/billing_status.py index 160d6085945..fe3f0825424 100644 --- a/erpnext/stock/doctype/purchase_receipt/services/billing_status.py +++ b/erpnext/stock/doctype/purchase_receipt/services/billing_status.py @@ -187,6 +187,9 @@ def update_billing_percentage( billed_qty_amt_based_on_po = get_billed_qty_amount_against_purchase_order(pr_doc) for item in pr_doc.items: + if item.closed: + continue + returned_qty = flt(item_wise_returned_qty.get(item.name)) returned_amount = flt(returned_qty) * flt(item.rate) pending_amount = flt(item.amount) - returned_amount @@ -198,7 +201,7 @@ def update_billing_percentage( total_billable_amount = pending_amount if item.billed_amt <= pending_amount else item.billed_amt total_amount += total_billable_amount - total_billed_amount += total_billable_amount if item.closed else abs(flt(item.billed_amt)) + total_billed_amount += abs(flt(item.billed_amt)) if pr_doc.get("is_return") and not total_amount and total_billed_amount: total_amount = total_billed_amount @@ -271,7 +274,10 @@ def update_billing_percentage( if pi_landed_cost_amount < 0: total_billed_amount += abs(pi_landed_cost_amount) - percent_billed = round(100 * (total_billed_amount / (total_amount or 1)), 6) + if not total_amount and pr_doc.items and all(item.closed for item in pr_doc.items): + percent_billed = 100 + else: + percent_billed = round(100 * (total_billed_amount / (total_amount or 1)), 6) pr_doc.db_set("per_billed", percent_billed) if update_modified: