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) <noreply@anthropic.com>
This commit is contained in:
Mihir Kandoi
2026-06-19 13:13:14 +05:30
parent f768778d81
commit 39eb34f333
2 changed files with 30 additions and 6 deletions

View File

@@ -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")

View File

@@ -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")