From 48beb2ee23a1a95ae46b8782dfd3749b45e04d2f Mon Sep 17 00:00:00 2001 From: R-Jayaraman Date: Thu, 30 Jul 2026 12:11:11 +0530 Subject: [PATCH 1/3] fix(sales): reject sales returns where every item has zero quantity validate_returned_items() set items_returned=True whenever a row matched a valid item from the original document, even if its qty was 0. This let a Sales Invoice, Delivery Note, or POS Invoice return be submitted with every line at qty=0 - a no-op document with no stock or financial effect that still consumed a document number and linked back to the original transaction. Scoped to the Sales side only: items_returned now flips to True for Sales Invoice/Delivery Note/POS Invoice only when qty (or received_qty) is actually negative, so an all-zero sales return correctly hits the existing "At least one item should be entered with negative quantity" check. Purchase Invoice, Purchase Receipt, and Subcontracting Receipt are unchanged. (cherry picked from commit a3e9d13da30089467441cf48586e5a6f3e211feb) # Conflicts: # erpnext/controllers/sales_and_purchase_return.py --- erpnext/controllers/sales_and_purchase_return.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/erpnext/controllers/sales_and_purchase_return.py b/erpnext/controllers/sales_and_purchase_return.py index c58580739e3..efca82b425b 100644 --- a/erpnext/controllers/sales_and_purchase_return.py +++ b/erpnext/controllers/sales_and_purchase_return.py @@ -158,7 +158,22 @@ def validate_returned_items(doc): ): frappe.throw(_("Warehouse is mandatory")) +<<<<<<< HEAD items_returned = True +======= + if doc.doctype in ( + "Purchase Invoice", + "Purchase Receipt", + "Subcontracting Receipt", + "Sales Invoice", + "Delivery Note", + "POS Invoice", + ): + if flt(d.qty) < 0 or flt(d.get("received_qty")) < 0: + items_returned = True + else: + items_returned = True +>>>>>>> a3e9d13da3 (fix(sales): reject sales returns where every item has zero quantity) elif d.item_name: items_returned = True From 51522816180e1bc590f95119af5fc4a29d3bcdfc Mon Sep 17 00:00:00 2001 From: R-Jayaraman Date: Fri, 31 Jul 2026 12:01:39 +0530 Subject: [PATCH 2/3] test(sales): add coverage for zero-qty return rejection Greptile flagged that the sales-side zero-qty-return fix had no dedicated test proving the behavior - the existing suite happened to pass, but nothing specifically asserted that an all-zero return is rejected while a normal negative-qty return still succeeds. Adds two tests covering the doctypes that rely entirely on this check (no other guard covers them for a non-stock-effect return): - Delivery Note return with qty 0 -> rejected - Sales Invoice return with qty 0 (no update_stock) -> rejected POS Invoice is not covered separately here since it always runs with update_stock=1, which is already guarded by the pre-existing validate_zero_qty_for_return_invoices_with_stock check regardless of this fix. (cherry picked from commit 732c884633acc8ed5862cded0f18f725457a6439) # Conflicts: # erpnext/controllers/tests/test_sales_and_purchase_return.py --- .../tests/test_sales_and_purchase_return.py | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 erpnext/controllers/tests/test_sales_and_purchase_return.py diff --git a/erpnext/controllers/tests/test_sales_and_purchase_return.py b/erpnext/controllers/tests/test_sales_and_purchase_return.py new file mode 100644 index 00000000000..1063b0d6f8d --- /dev/null +++ b/erpnext/controllers/tests/test_sales_and_purchase_return.py @@ -0,0 +1,112 @@ +# Copyright (c) 2025, Frappe Technologies Pvt. Ltd. and Contributors +# See license.txt + +import frappe + +from erpnext.tests.utils import ERPNextTestSuite + + +class TestSalesAndPurchaseReturn(ERPNextTestSuite): + @staticmethod + def _cancel_and_delete(doctype, name): + if not frappe.db.exists(doctype, name): + return + doc = frappe.get_doc(doctype, name) + if doc.docstatus == 1: + doc.cancel() + frappe.delete_doc(doctype, name, force=1) + + def test_sales_return_validates_against_original(self): + # Submitting a return Delivery Note runs validate_returned_items (Item / Packed Item lookups + # via frappe.get_all) and get_already_returned_items (qb GROUP BY of the returned qty) -- both + # converted from raw SQL here. Exercises them on both engines. + from erpnext.stock.doctype.delivery_note.mapper import make_sales_return + from erpnext.stock.doctype.delivery_note.test_delivery_note import create_delivery_note + from erpnext.stock.doctype.stock_entry.stock_entry_utils import make_stock_entry + + se = make_stock_entry(item_code="_Test Item", target="_Test Warehouse - _TC", qty=20, basic_rate=100) + self.addCleanup(self._cancel_and_delete, "Stock Entry", se.name) + + dn = create_delivery_note(qty=5) + self.addCleanup(self._cancel_and_delete, "Delivery Note", dn.name) + + return_dn = make_sales_return(dn.name) + return_dn.insert() + return_dn.submit() + self.addCleanup(self._cancel_and_delete, "Delivery Note", return_dn.name) + + self.assertEqual(return_dn.is_return, 1) + self.assertEqual(return_dn.items[0].qty, -5) + + def test_purchase_invoice_zero_qty_return_is_rejected(self): + # A return with every item at qty 0 moves no stock and no value, so it must be + # rejected the same way a return with no items at all would be. + from erpnext.accounts.doctype.purchase_invoice.test_purchase_invoice import make_purchase_invoice + + pi = make_purchase_invoice(qty=10) + self.addCleanup(self._cancel_and_delete, "Purchase Invoice", pi.name) + + return_pi = make_purchase_invoice( + is_return=1, + return_against=pi.name, + qty=0, + do_not_save=True, + ) + + self.assertRaises(frappe.ValidationError, return_pi.save) + + def test_purchase_invoice_item_name_only_zero_qty_return_is_rejected(self): + # Item Code is not mandatory on Purchase Invoice Item - a row can have only an + # item_name (e.g. a free-text/non-stock line). Such rows fall through to the + # item_name-only branch, which must also reject an all-zero-qty return instead + # of unconditionally treating the row as returned. + from erpnext.accounts.doctype.purchase_invoice.test_purchase_invoice import make_purchase_invoice + + pi = make_purchase_invoice(item_name="_Test Item", qty=10, do_not_submit=True) + pi.items[0].item_code = "" + pi.save() + pi.submit() + self.addCleanup(self._cancel_and_delete, "Purchase Invoice", pi.name) + + return_pi = make_purchase_invoice( + item_name="_Test Item", + is_return=1, + return_against=pi.name, + qty=0, + do_not_save=True, + ) + return_pi.items[0].item_code = "" + + self.assertRaises(frappe.ValidationError, return_pi.save) + + def test_delivery_note_zero_qty_return_is_rejected(self): + # A return with every item at qty 0 moves no stock and no value, so it must be + # rejected the same way a return with no items at all would be. + from erpnext.stock.doctype.delivery_note.mapper import make_sales_return + from erpnext.stock.doctype.delivery_note.test_delivery_note import create_delivery_note + from erpnext.stock.doctype.stock_entry.stock_entry_utils import make_stock_entry + + se = make_stock_entry(item_code="_Test Item", target="_Test Warehouse - _TC", qty=20, basic_rate=100) + self.addCleanup(self._cancel_and_delete, "Stock Entry", se.name) + + dn = create_delivery_note(qty=5) + self.addCleanup(self._cancel_and_delete, "Delivery Note", dn.name) + + return_dn = make_sales_return(dn.name) + return_dn.items[0].qty = 0 + + self.assertRaises(frappe.ValidationError, return_dn.insert) + + def test_sales_invoice_zero_qty_return_is_rejected(self): + # Same rule for a standalone (non stock-affecting) Sales Invoice return: qty 0 on + # every row must be rejected, not silently accepted as a no-op credit note. + from erpnext.accounts.doctype.sales_invoice.test_sales_invoice import create_sales_invoice + from erpnext.controllers.sales_and_purchase_return import make_return_doc + + si = create_sales_invoice(qty=10) + self.addCleanup(self._cancel_and_delete, "Sales Invoice", si.name) + + return_si = make_return_doc(si.doctype, si.name) + return_si.items[0].qty = 0 + + self.assertRaises(frappe.ValidationError, return_si.save) From 5ec87ae06c679a9065edcd72821c61b5499a0192 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Mon, 3 Aug 2026 13:29:37 +0530 Subject: [PATCH 3/3] chore: resolve conflict --- .../controllers/sales_and_purchase_return.py | 4 --- .../tests/test_sales_and_purchase_return.py | 29 ++----------------- 2 files changed, 3 insertions(+), 30 deletions(-) diff --git a/erpnext/controllers/sales_and_purchase_return.py b/erpnext/controllers/sales_and_purchase_return.py index efca82b425b..5f469b6a7fd 100644 --- a/erpnext/controllers/sales_and_purchase_return.py +++ b/erpnext/controllers/sales_and_purchase_return.py @@ -158,9 +158,6 @@ def validate_returned_items(doc): ): frappe.throw(_("Warehouse is mandatory")) -<<<<<<< HEAD - items_returned = True -======= if doc.doctype in ( "Purchase Invoice", "Purchase Receipt", @@ -173,7 +170,6 @@ def validate_returned_items(doc): items_returned = True else: items_returned = True ->>>>>>> a3e9d13da3 (fix(sales): reject sales returns where every item has zero quantity) elif d.item_name: items_returned = True diff --git a/erpnext/controllers/tests/test_sales_and_purchase_return.py b/erpnext/controllers/tests/test_sales_and_purchase_return.py index 1063b0d6f8d..0de679352f7 100644 --- a/erpnext/controllers/tests/test_sales_and_purchase_return.py +++ b/erpnext/controllers/tests/test_sales_and_purchase_return.py @@ -2,11 +2,10 @@ # See license.txt import frappe - -from erpnext.tests.utils import ERPNextTestSuite +from frappe.tests.utils import FrappeTestCase -class TestSalesAndPurchaseReturn(ERPNextTestSuite): +class TestSalesAndPurchaseReturn(FrappeTestCase): @staticmethod def _cancel_and_delete(doctype, name): if not frappe.db.exists(doctype, name): @@ -16,28 +15,6 @@ class TestSalesAndPurchaseReturn(ERPNextTestSuite): doc.cancel() frappe.delete_doc(doctype, name, force=1) - def test_sales_return_validates_against_original(self): - # Submitting a return Delivery Note runs validate_returned_items (Item / Packed Item lookups - # via frappe.get_all) and get_already_returned_items (qb GROUP BY of the returned qty) -- both - # converted from raw SQL here. Exercises them on both engines. - from erpnext.stock.doctype.delivery_note.mapper import make_sales_return - from erpnext.stock.doctype.delivery_note.test_delivery_note import create_delivery_note - from erpnext.stock.doctype.stock_entry.stock_entry_utils import make_stock_entry - - se = make_stock_entry(item_code="_Test Item", target="_Test Warehouse - _TC", qty=20, basic_rate=100) - self.addCleanup(self._cancel_and_delete, "Stock Entry", se.name) - - dn = create_delivery_note(qty=5) - self.addCleanup(self._cancel_and_delete, "Delivery Note", dn.name) - - return_dn = make_sales_return(dn.name) - return_dn.insert() - return_dn.submit() - self.addCleanup(self._cancel_and_delete, "Delivery Note", return_dn.name) - - self.assertEqual(return_dn.is_return, 1) - self.assertEqual(return_dn.items[0].qty, -5) - def test_purchase_invoice_zero_qty_return_is_rejected(self): # A return with every item at qty 0 moves no stock and no value, so it must be # rejected the same way a return with no items at all would be. @@ -82,7 +59,7 @@ class TestSalesAndPurchaseReturn(ERPNextTestSuite): def test_delivery_note_zero_qty_return_is_rejected(self): # A return with every item at qty 0 moves no stock and no value, so it must be # rejected the same way a return with no items at all would be. - from erpnext.stock.doctype.delivery_note.mapper import make_sales_return + from erpnext.stock.doctype.delivery_note.delivery_note import make_sales_return from erpnext.stock.doctype.delivery_note.test_delivery_note import create_delivery_note from erpnext.stock.doctype.stock_entry.stock_entry_utils import make_stock_entry