From f768778d812ebb09e8718824215a96d55983c732 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Fri, 19 Jun 2026 12:58:52 +0530 Subject: [PATCH 1/2] refactor(postgres): port Support module queries to the query builder Convert the remaining raw frappe.db.sql in the Support module to frappe.qb / ORM so the queries run on PostgreSQL as well as MariaDB. Faithful conversions -- no MariaDB behaviour change: - issue.py, warranty_claim.py (Maintenance Visit lookup / make_maintenance_visit) - reports: first_response_time_for_issues and issue_summary (GROUP BY on the grouped Date(creation)/based-on field + Avg/Count -- Postgres-valid), support_hour_distribution Tests: existing issue suite (35) passes on both engines; adds both-engine tests for the previously-untested warranty_claim mapper (3) and the three reports. All green on MariaDB and PostgreSQL. Co-Authored-By: Claude Opus 4.8 (1M context) --- erpnext/support/doctype/issue/issue.py | 2 +- .../warranty_claim/test_warranty_claim.py | 80 ++++++++++++++++++- .../doctype/warranty_claim/warranty_claim.py | 30 ++++--- .../first_response_time_for_issues.py | 27 ++++--- .../test_first_response_time_for_issues.py | 50 ++++++++++++ .../report/issue_summary/issue_summary.py | 30 +++---- .../issue_summary/test_issue_summary.py | 49 ++++++++++++ .../support_hour_distribution.py | 12 +-- .../test_support_hour_distribution.py | 53 ++++++++++++ 9 files changed, 280 insertions(+), 53 deletions(-) create mode 100644 erpnext/support/report/first_response_time_for_issues/test_first_response_time_for_issues.py create mode 100644 erpnext/support/report/issue_summary/test_issue_summary.py create mode 100644 erpnext/support/report/support_hour_distribution/test_support_hour_distribution.py diff --git a/erpnext/support/doctype/issue/issue.py b/erpnext/support/doctype/issue/issue.py index e43d5977781..4886fc38711 100644 --- a/erpnext/support/doctype/issue/issue.py +++ b/erpnext/support/doctype/issue/issue.py @@ -266,7 +266,7 @@ def has_website_permission(doc, ptype, user, verbose=False): def update_issue(contact, method): """Called when Contact is deleted""" - frappe.db.sql("""UPDATE `tabIssue` set contact='' where contact=%s""", contact.name) + frappe.db.set_value("Issue", {"contact": contact.name}, "contact", "") @frappe.whitelist() diff --git a/erpnext/support/doctype/warranty_claim/test_warranty_claim.py b/erpnext/support/doctype/warranty_claim/test_warranty_claim.py index fa87b678f3a..94f28fad72a 100644 --- a/erpnext/support/doctype/warranty_claim/test_warranty_claim.py +++ b/erpnext/support/doctype/warranty_claim/test_warranty_claim.py @@ -1,8 +1,86 @@ # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors and Contributors # See license.txt +import frappe +from frappe.utils.data import today + +from erpnext.support.doctype.warranty_claim.warranty_claim import make_maintenance_visit from erpnext.tests.utils import ERPNextTestSuite class TestWarrantyClaim(ERPNextTestSuite): - pass + def make_warranty_claim(self): + # Warranty Claim is not a submittable doctype; it stays at docstatus 0. + claim = frappe.new_doc("Warranty Claim") + claim.status = "Open" + claim.complaint_date = today() + claim.customer = "_Test Customer" + claim.item_code = "_Test Item" + claim.complaint = "Device stopped working under warranty" + claim.company = "_Test Company" + claim.insert(ignore_permissions=True) + return claim + + def make_maintenance_visit_for_claim(self, claim, completion_status): + visit = frappe.new_doc("Maintenance Visit") + visit.company = "_Test Company" + visit.customer = "_Test Customer" + visit.mntc_date = today() + visit.maintenance_type = "Unscheduled" + visit.completion_status = completion_status + visit.append( + "purposes", + { + "item_code": "_Test Item", + "service_person": "_Test Sales Person", + "work_done": "Replaced the faulty component", + "description": "Warranty repair", + "prevdoc_doctype": "Warranty Claim", + "prevdoc_docname": claim.name, + }, + ) + visit.insert(ignore_permissions=True) + visit.submit() + return visit + + def test_make_maintenance_visit_maps_new_visit_when_none_completed(self): + # No "Fully Completed" visit yet -> converted query returns nothing, + # so a fresh Maintenance Visit draft is mapped from the claim. + claim = self.make_warranty_claim() + + target = make_maintenance_visit(claim.name) + + self.assertIsNotNone(target) + self.assertEqual(target.doctype, "Maintenance Visit") + self.assertTrue(target.is_new()) + # item_code present -> a purpose row is mapped and back-linked to the claim + self.assertEqual(len(target.purposes), 1) + row = target.purposes[0] + self.assertEqual(row.item_code, "_Test Item") + self.assertEqual(row.prevdoc_doctype, "Warranty Claim") + self.assertEqual(row.prevdoc_docname, claim.name) + + def test_make_maintenance_visit_returns_none_when_fully_completed_exists(self): + # A submitted, "Fully Completed" visit pointing at the claim must be + # found by the converted join query -> no new visit is mapped. + claim = self.make_warranty_claim() + visit = self.make_maintenance_visit_for_claim(claim, "Fully Completed") + + # Sanity: the seeded visit really is the one the query should match. + self.assertEqual(visit.docstatus, 1) + self.assertEqual(visit.completion_status, "Fully Completed") + self.assertEqual(visit.purposes[0].prevdoc_docname, claim.name) + + self.assertIsNone(make_maintenance_visit(claim.name)) + + def test_make_maintenance_visit_ignores_partially_completed(self): + # A "Partially Completed" visit must NOT satisfy the query, so a new + # visit is still mapped (the completion_status filter is exercised). + claim = self.make_warranty_claim() + self.make_maintenance_visit_for_claim(claim, "Partially Completed") + + target = make_maintenance_visit(claim.name) + + self.assertIsNotNone(target) + self.assertTrue(target.is_new()) + self.assertEqual(target.doctype, "Maintenance Visit") diff --git a/erpnext/support/doctype/warranty_claim/warranty_claim.py b/erpnext/support/doctype/warranty_claim/warranty_claim.py index dfb8bc8ea71..12fcf6d9fa5 100644 --- a/erpnext/support/doctype/warranty_claim/warranty_claim.py +++ b/erpnext/support/doctype/warranty_claim/warranty_claim.py @@ -62,14 +62,13 @@ class WarrantyClaim(TransactionBase): self.resolution_date = now_datetime() def on_cancel(self): - lst = frappe.db.sql( - """select t1.name - from `tabMaintenance Visit` t1, `tabMaintenance Visit Purpose` t2 - where t2.parent = t1.name and t2.prevdoc_docname = %s and t1.docstatus!=2""", - (self.name), + lst = frappe.get_all( + "Maintenance Visit Purpose", + filters={"prevdoc_docname": self.name, "docstatus": ["!=", 2]}, + pluck="parent", ) if lst: - lst1 = ",".join(x[0] for x in lst) + lst1 = ",".join(lst) frappe.throw(_("Cancel Material Visit {0} before cancelling this Warranty Claim").format(lst1)) else: self.db_set("status", "Cancelled") @@ -86,12 +85,19 @@ def make_maintenance_visit(source_name: str, target_doc: str | Document | None = target_doc.prevdoc_doctype = source_parent.doctype target_doc.prevdoc_docname = source_parent.name - visit = frappe.db.sql( - """select t1.name - from `tabMaintenance Visit` t1, `tabMaintenance Visit Purpose` t2 - where t2.parent=t1.name and t2.prevdoc_docname=%s - and t1.docstatus=1 and t1.completion_status='Fully Completed'""", - source_name, + mv = frappe.qb.DocType("Maintenance Visit") + mvp = frappe.qb.DocType("Maintenance Visit Purpose") + visit = ( + frappe.qb.from_(mv) + .inner_join(mvp) + .on(mvp.parent == mv.name) + .select(mv.name) + .where( + (mvp.prevdoc_docname == source_name) + & (mv.docstatus == 1) + & (mv.completion_status == "Fully Completed") + ) + .run() ) if not visit: diff --git a/erpnext/support/report/first_response_time_for_issues/first_response_time_for_issues.py b/erpnext/support/report/first_response_time_for_issues/first_response_time_for_issues.py index 57fa7bf2b7b..b232dec07ca 100644 --- a/erpnext/support/report/first_response_time_for_issues/first_response_time_for_issues.py +++ b/erpnext/support/report/first_response_time_for_issues/first_response_time_for_issues.py @@ -4,6 +4,7 @@ import frappe from frappe import _ +from frappe.query_builder.functions import Avg, Date def execute(filters=None): @@ -17,19 +18,19 @@ def execute(filters=None): }, ] - data = frappe.db.sql( - """ - SELECT - date(creation) as creation_date, - avg(first_response_time) as avg_response_time - FROM tabIssue - WHERE - date(creation) between %s and %s - and first_response_time > 0 - GROUP BY creation_date - ORDER BY creation_date desc - """, - (filters.from_date, filters.to_date), + issue = frappe.qb.DocType("Issue") + data = ( + frappe.qb.from_(issue) + .select( + Date(issue.creation).as_("creation_date"), + Avg(issue.first_response_time).as_("avg_response_time"), + ) + .where( + Date(issue.creation).between(filters.from_date, filters.to_date) & (issue.first_response_time > 0) + ) + .groupby(Date(issue.creation)) + .orderby(Date(issue.creation), order=frappe.qb.desc) + .run() ) return columns, data diff --git a/erpnext/support/report/first_response_time_for_issues/test_first_response_time_for_issues.py b/erpnext/support/report/first_response_time_for_issues/test_first_response_time_for_issues.py new file mode 100644 index 00000000000..fcb331dddbc --- /dev/null +++ b/erpnext/support/report/first_response_time_for_issues/test_first_response_time_for_issues.py @@ -0,0 +1,50 @@ +# Copyright (c) 2013, Frappe Technologies Pvt. Ltd. and contributors +# For license information, please see license.txt + +import frappe +from frappe.utils import add_days, getdate, now_datetime + +from erpnext.support.report.first_response_time_for_issues.first_response_time_for_issues import ( + execute, +) +from erpnext.tests.utils import ERPNextTestSuite + + +class TestFirstResponseTimeForIssues(ERPNextTestSuite): + def test_avg_first_response_time_grouped_by_creation_date(self): + today = getdate() + + # Isolate today's group: any pre-existing Issue created today (from other + # fixtures running in the same transaction) would pollute the average. + # Rolled back automatically in tearDown. + frappe.db.delete("Issue", {"creation": (">=", today)}) + + # Seed exactly one Issue created today. + issue = frappe.get_doc( + { + "doctype": "Issue", + "subject": "First Response Time Report Issue", + "raised_by": "test_frt_report@example.com", + "description": "First Response Time Report Issue", + "company": "_Test Company", + "creation": now_datetime(), + } + ).insert(ignore_permissions=True) + + # first_response_time is a read-only computed Duration (seconds); set directly. + response_time = 3600 + frappe.db.set_value("Issue", issue.name, "first_response_time", response_time) + + columns, data = execute(frappe._dict(from_date=add_days(today, -1), to_date=add_days(today, 1))) + + # Rows are tuples: (creation_date, avg_response_time) -- report uses .run() w/o as_dict. + rows_for_today = [row for row in data if getdate(row[0]) == today] + self.assertEqual( + len(rows_for_today), + 1, + f"expected exactly one grouped row for {today}, got {rows_for_today}", + ) + + creation_date, avg_response_time = rows_for_today[0] + self.assertEqual(getdate(creation_date), today) + self.assertEqual(float(avg_response_time), float(response_time)) diff --git a/erpnext/support/report/issue_summary/issue_summary.py b/erpnext/support/report/issue_summary/issue_summary.py index d976c2871b2..d676cd8b60f 100644 --- a/erpnext/support/report/issue_summary/issue_summary.py +++ b/erpnext/support/report/issue_summary/issue_summary.py @@ -6,6 +6,7 @@ import json import frappe from frappe import _, scrub +from frappe.query_builder.functions import Avg from frappe.utils import flt @@ -264,21 +265,20 @@ class IssueSummary: self.issue_summary_data[entry][metric] /= flt(assignment_map.get(entry)) else: - data = frappe.db.sql( - f""" - SELECT - {field}, AVG(first_response_time) as avg_frt, - AVG(avg_response_time) as avg_resp_time, - AVG(total_hold_time) as avg_hold_time, - AVG(resolution_time) as avg_resolution_time, - AVG(user_resolution_time) as avg_user_resolution_time - FROM `tabIssue` - WHERE - name IN %(issues)s - GROUP BY {field} - """, - {"issues": issues}, - as_dict=1, + issue = frappe.qb.DocType("Issue") + data = ( + frappe.qb.from_(issue) + .select( + issue[field], + Avg(issue.first_response_time).as_("avg_frt"), + Avg(issue.avg_response_time).as_("avg_resp_time"), + Avg(issue.total_hold_time).as_("avg_hold_time"), + Avg(issue.resolution_time).as_("avg_resolution_time"), + Avg(issue.user_resolution_time).as_("avg_user_resolution_time"), + ) + .where(issue.name.isin(issues)) + .groupby(issue[field]) + .run(as_dict=1) ) for entry in data: diff --git a/erpnext/support/report/issue_summary/test_issue_summary.py b/erpnext/support/report/issue_summary/test_issue_summary.py new file mode 100644 index 00000000000..cf98c44fa4c --- /dev/null +++ b/erpnext/support/report/issue_summary/test_issue_summary.py @@ -0,0 +1,49 @@ +# Copyright (c) 2024, Frappe Technologies Pvt. Ltd. and contributors +# For license information, please see license.txt + +import frappe +from frappe.utils import add_days, today + +from erpnext.support.report.issue_summary.issue_summary import execute +from erpnext.tests.utils import ERPNextTestSuite + + +class TestIssueSummary(ERPNextTestSuite): + def test_count_grouped_by_issue_priority(self): + # Unique Issue Priority so this group is isolated from any pre-existing data. + priority = "__Test Issue Summary Priority" + if not frappe.db.exists("Issue Priority", priority): + frappe.get_doc({"doctype": "Issue Priority", "name": priority}).insert() + + opening_date = today() + for subject in ("__Test Issue Summary A", "__Test Issue Summary B"): + frappe.get_doc( + { + "doctype": "Issue", + "subject": subject, + "priority": priority, + "status": "Open", + "opening_date": opening_date, + } + ).insert() + + filters = frappe._dict( + { + "based_on": "Issue Priority", + "from_date": add_days(opening_date, -1), + "to_date": add_days(opening_date, 1), + } + ) + + columns, data, _msg, _chart, _summary = execute(filters) + + # get_rows() emits one row per group keyed on "priority" for based_on == "Issue Priority". + seeded_row = next((row for row in data if row.get("priority") == priority), None) + self.assertIsNotNone( + seeded_row, + f"expected a summary row for priority {priority!r}, got {[r.get('priority') for r in data]}", + ) + + # Two seeded Open issues -> total_issues == 2 and the "open" status bucket == 2. + self.assertEqual(seeded_row["total_issues"], 2) + self.assertEqual(seeded_row["open"], 2) diff --git a/erpnext/support/report/support_hour_distribution/support_hour_distribution.py b/erpnext/support/report/support_hour_distribution/support_hour_distribution.py index 2a978310ddb..1890b1db40e 100644 --- a/erpnext/support/report/support_hour_distribution/support_hour_distribution.py +++ b/erpnext/support/report/support_hour_distribution/support_hour_distribution.py @@ -51,17 +51,7 @@ def get_data(filters): def get_hours_count(start_time, end_time): - data = ( - frappe.db.sql( - """ select count(*) from `tabIssue` where creation - between %(start_time)s and %(end_time)s""", - {"start_time": start_time, "end_time": end_time}, - as_list=1, - ) - or [] - ) - - return data[0][0] if len(data) > 0 else 0 + return frappe.db.count("Issue", {"creation": ["between", [start_time, end_time]]}) def get_columns(): diff --git a/erpnext/support/report/support_hour_distribution/test_support_hour_distribution.py b/erpnext/support/report/support_hour_distribution/test_support_hour_distribution.py new file mode 100644 index 00000000000..2703cb6572d --- /dev/null +++ b/erpnext/support/report/support_hour_distribution/test_support_hour_distribution.py @@ -0,0 +1,53 @@ +import frappe +from frappe.utils import get_datetime, getdate + +from erpnext.support.doctype.issue.test_issue import make_issue +from erpnext.support.report.support_hour_distribution.support_hour_distribution import execute +from erpnext.tests.utils import ERPNextTestSuite + + +class TestSupportHourDistribution(ERPNextTestSuite): + def test_issue_buckets_into_expected_time_slot(self): + # The report buckets Issues by `creation` into 3-hour slots over the + # from_date..to_date range. `creation` is auto-stamped on insert, so we + # force it afterwards to a known time. 14:00 sits squarely inside the + # "12PM - 3PM" slot (12:00:00 - 15:00:00), away from any slot boundary, + # so the bucket assignment is unambiguous. + report_date = getdate() + issue = make_issue(customer="_Test Customer", index=1) + creation = get_datetime(f"{report_date.strftime('%Y-%m-%d')} 14:00:00") + frappe.db.set_value("Issue", issue.name, "creation", creation, update_modified=False) + + filters = frappe._dict( + { + "from_date": report_date, + "to_date": report_date, + "periodicity": "Daily", + } + ) + + columns, data, _, chart = execute(filters) + + # Single day in range -> exactly one row. + self.assertEqual(len(data), 1) + row = data[0] + self.assertEqual(row["date"], report_date) + + # Real-state check: the report must count exactly the Issues whose + # `creation` falls in the 12PM-3PM window (inclusive `between`), which + # now includes our seeded record. Compare against an independent count. + slot_start = get_datetime(f"{report_date.strftime('%Y-%m-%d')} 12:00:00") + slot_end = get_datetime(f"{report_date.strftime('%Y-%m-%d')} 15:00:00") + expected = frappe.db.count("Issue", {"creation": ["between", [slot_start, slot_end]]}) + self.assertGreaterEqual(expected, 1) + self.assertEqual(row["12PM - 3PM"], expected) + + # Columns: Date + 8 time slots. + self.assertEqual(len(columns), 9) + + # Chart aggregates per-slot totals across the range; the 12PM-3PM + # (5th) label must include our seeded record. + labels = chart["data"]["labels"] + values = chart["data"]["datasets"][0]["values"] + self.assertGreaterEqual(values[labels.index("12PM - 3PM")], 1) + self.assertEqual(row["12PM - 3PM"], values[labels.index("12PM - 3PM")]) From 39eb34f333c715812c876aea9280c2c12a17d1ce Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Fri, 19 Jun 2026 13:13:14 +0530 Subject: [PATCH 2/2] 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")