From 39eb34f333c715812c876aea9280c2c12a17d1ce Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Fri, 19 Jun 2026 13:13:14 +0530 Subject: [PATCH] refactor(postgres): address Greptile review on warranty_claim on_cancel - Filter the parent Maintenance Visit's docstatus (mv.docstatus != 2) via a qb join, as the original SQL did, instead of the child Maintenance Visit Purpose row's docstatus. Synced in normal flows, but exactly faithful to the original intent. - Add a limit(500) to bound the read on a cancellation path. Adds two both-engine tests calling on_cancel directly: an active (non-cancelled) visit blocks the claim cancel; with no referencing visit the claim is marked Cancelled. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../warranty_claim/test_warranty_claim.py | 17 +++++++++++++++++ .../doctype/warranty_claim/warranty_claim.py | 19 +++++++++++++------ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/erpnext/support/doctype/warranty_claim/test_warranty_claim.py b/erpnext/support/doctype/warranty_claim/test_warranty_claim.py index 94f28fad72a..18b634d59a1 100644 --- a/erpnext/support/doctype/warranty_claim/test_warranty_claim.py +++ b/erpnext/support/doctype/warranty_claim/test_warranty_claim.py @@ -84,3 +84,20 @@ class TestWarrantyClaim(ERPNextTestSuite): self.assertIsNotNone(target) self.assertTrue(target.is_new()) self.assertEqual(target.doctype, "Maintenance Visit") + + def test_on_cancel_blocked_by_active_maintenance_visit(self): + # on_cancel's converted query joins Maintenance Visit Purpose -> Maintenance Visit and + # filters the PARENT visit's docstatus != 2; a submitted (non-cancelled) visit referencing + # the claim must block cancellation. + claim = self.make_warranty_claim() + self.make_maintenance_visit_for_claim(claim, "Partially Completed") + + self.assertRaises(frappe.ValidationError, claim.on_cancel) + + def test_on_cancel_allowed_when_no_active_visit(self): + # No referencing visit -> the query returns nothing -> the claim is marked Cancelled. + claim = self.make_warranty_claim() + + claim.on_cancel() + + self.assertEqual(frappe.db.get_value("Warranty Claim", claim.name, "status"), "Cancelled") diff --git a/erpnext/support/doctype/warranty_claim/warranty_claim.py b/erpnext/support/doctype/warranty_claim/warranty_claim.py index 12fcf6d9fa5..8336ee317ad 100644 --- a/erpnext/support/doctype/warranty_claim/warranty_claim.py +++ b/erpnext/support/doctype/warranty_claim/warranty_claim.py @@ -62,13 +62,20 @@ class WarrantyClaim(TransactionBase): self.resolution_date = now_datetime() def on_cancel(self): - lst = frappe.get_all( - "Maintenance Visit Purpose", - filters={"prevdoc_docname": self.name, "docstatus": ["!=", 2]}, - pluck="parent", + mv = frappe.qb.DocType("Maintenance Visit") + mvp = frappe.qb.DocType("Maintenance Visit Purpose") + # filter the parent Maintenance Visit's docstatus (as the original SQL did), not the child row's + visits = ( + frappe.qb.from_(mvp) + .inner_join(mv) + .on(mvp.parent == mv.name) + .select(mv.name) + .where((mvp.prevdoc_docname == self.name) & (mv.docstatus != 2)) + .limit(500) + .run() ) - if lst: - lst1 = ",".join(lst) + if visits: + lst1 = ",".join(x[0] for x in visits) frappe.throw(_("Cancel Material Visit {0} before cancelling this Warranty Claim").format(lst1)) else: self.db_set("status", "Cancelled")