From 5104007d1282dc79236ec127168d451d21e9fe4b Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Fri, 19 Jun 2026 10:15:20 +0530 Subject: [PATCH 1/2] refactor(postgres): port CRM module queries to the query builder Convert the remaining raw frappe.db.sql in the CRM module to frappe.qb / ORM so the queries run on PostgreSQL as well as MariaDB. Faithful conversions -- no MariaDB behaviour change: - opportunity.py, doctype/utils.py (get_last_interaction) - reports: campaign_efficiency, first_response_time_for_opportunity (GROUP BY on the grouped Date(creation) + Avg -- Postgres-valid), lead_conversion_time, prospects_engaged_but_not_converted lead_conversion_time also keeps the IS NOT NULL communication-date guard (forward-port of the fix already on develop). Also drops invalid backtick notation from two get_all order_by clauses in doctype/utils.py (order_by="`creation` DESC"), which frappe's query engine rejects -- a latent failure on both engines, surfaced by the new test. Tests: existing opportunity suite plus new both-engine tests for the four previously untested reports/utils. All green on MariaDB and PostgreSQL. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../crm/doctype/opportunity/opportunity.py | 66 +++++++-------- erpnext/crm/doctype/test_utils.py | 47 +++++++++++ erpnext/crm/doctype/utils.py | 39 +++++---- .../campaign_efficiency.py | 81 ++++++++----------- .../test_campaign_efficiency.py | 46 +++++++++++ .../first_response_time_for_opportunity.py | 28 ++++--- ...est_first_response_time_for_opportunity.py | 60 ++++++++++++++ .../lead_conversion_time.py | 67 +++++++-------- .../test_lead_conversion_time.py | 59 ++++++++++++++ .../prospects_engaged_but_not_converted.py | 57 ++++++++----- ...est_prospects_engaged_but_not_converted.py | 73 +++++++++++++++++ 11 files changed, 449 insertions(+), 174 deletions(-) create mode 100644 erpnext/crm/doctype/test_utils.py create mode 100644 erpnext/crm/report/campaign_efficiency/test_campaign_efficiency.py create mode 100644 erpnext/crm/report/first_response_time_for_opportunity/test_first_response_time_for_opportunity.py create mode 100644 erpnext/crm/report/lead_conversion_time/test_lead_conversion_time.py create mode 100644 erpnext/crm/report/prospects_engaged_but_not_converted/test_prospects_engaged_but_not_converted.py diff --git a/erpnext/crm/doctype/opportunity/opportunity.py b/erpnext/crm/doctype/opportunity/opportunity.py index 6dc5f6a47b4..ede7271132f 100644 --- a/erpnext/crm/doctype/opportunity/opportunity.py +++ b/erpnext/crm/doctype/opportunity/opportunity.py @@ -290,13 +290,19 @@ class Opportunity(TransactionBase, CRMNote): "name", ) else: - return frappe.db.sql( - """ - select q.name - from `tabQuotation` q, `tabQuotation Item` qi - where q.name = qi.parent and q.docstatus=1 and qi.prevdoc_docname =%s - and q.status not in ('Lost', 'Closed')""", - self.name, + q = frappe.qb.DocType("Quotation") + qi = frappe.qb.DocType("Quotation Item") + return ( + frappe.qb.from_(q) + .inner_join(qi) + .on(q.name == qi.parent) + .select(q.name) + .where( + (q.docstatus == 1) + & (qi.prevdoc_docname == self.name) + & q.status.notin(["Lost", "Closed"]) + ) + .run() ) def has_ordered_quotation(self): @@ -305,24 +311,20 @@ class Opportunity(TransactionBase, CRMNote): "Quotation", {"opportunity": self.name, "status": "Ordered", "docstatus": 1}, "name" ) else: - return frappe.db.sql( - """ - select q.name - from `tabQuotation` q, `tabQuotation Item` qi - where q.name = qi.parent and q.docstatus=1 and qi.prevdoc_docname =%s - and q.status = 'Ordered'""", - self.name, + q = frappe.qb.DocType("Quotation") + qi = frappe.qb.DocType("Quotation Item") + return ( + frappe.qb.from_(q) + .inner_join(qi) + .on(q.name == qi.parent) + .select(q.name) + .where((q.docstatus == 1) & (qi.prevdoc_docname == self.name) & (q.status == "Ordered")) + .run() ) def has_lost_quotation(self): - lost_quotation = frappe.db.sql( - """ - select name - from `tabQuotation` - where docstatus=1 - and opportunity =%s and status = 'Lost' - """, - self.name, + lost_quotation = frappe.get_all( + "Quotation", filters={"docstatus": 1, "opportunity": self.name, "status": "Lost"} ) if lost_quotation: if self.has_active_quotation(): @@ -371,19 +373,19 @@ class Opportunity(TransactionBase, CRMNote): @frappe.whitelist() def get_item_details(item_code: str): - item = frappe.db.sql( - """select item_name, stock_uom, image, description, item_group, brand - from `tabItem` where name = %s""", + item = frappe.db.get_value( + "Item", item_code, - as_dict=1, + ["item_name", "stock_uom", "image", "description", "item_group", "brand"], + as_dict=True, ) return { - "item_name": item and item[0]["item_name"] or "", - "uom": item and item[0]["stock_uom"] or "", - "description": item and item[0]["description"] or "", - "image": item and item[0]["image"] or "", - "item_group": item and item[0]["item_group"] or "", - "brand": item and item[0]["brand"] or "", + "item_name": item and item.item_name or "", + "uom": item and item.stock_uom or "", + "description": item and item.description or "", + "image": item and item.image or "", + "item_group": item and item.item_group or "", + "brand": item and item.brand or "", } diff --git a/erpnext/crm/doctype/test_utils.py b/erpnext/crm/doctype/test_utils.py new file mode 100644 index 00000000000..742cad61c0c --- /dev/null +++ b/erpnext/crm/doctype/test_utils.py @@ -0,0 +1,47 @@ +# Copyright (c) 2024, Frappe Technologies Pvt. Ltd. and Contributors +# License: GNU General Public License v3. See license.txt + +import frappe + +from erpnext.crm.doctype.utils import get_last_interaction +from erpnext.tests.utils import ERPNextTestSuite + + +class TestCrmDoctypeUtils(ERPNextTestSuite): + def test_get_last_interaction_for_contact(self): + """Covers the converted Communication query (contact path): returns the earliest Received + communication across the doctypes the contact is linked to. `creation` is unique, so the + LIMIT-1 pick is deterministic and identical on MariaDB and Postgres.""" + customer = "_Test CRM Util Customer" + if not frappe.db.exists("Customer", customer): + frappe.get_doc( + { + "doctype": "Customer", + "customer_name": customer, + "customer_group": "_Test Customer Group", + "territory": "_Test Territory", + } + ).insert(ignore_permissions=True) + + contact = frappe.get_doc( + { + "doctype": "Contact", + "first_name": "CRM Util Test", + "links": [{"link_doctype": "Customer", "link_name": customer}], + } + ).insert(ignore_permissions=True) + + comm = frappe.get_doc( + { + "doctype": "Communication", + "subject": "hi", + "content": "first interaction", + "sent_or_received": "Received", + "reference_doctype": "Customer", + "reference_name": customer, + } + ).insert(ignore_permissions=True) + + result = get_last_interaction(contact=contact.name) + self.assertIsNotNone(result["last_communication"]) + self.assertEqual(result["last_communication"]["name"], comm.name) diff --git a/erpnext/crm/doctype/utils.py b/erpnext/crm/doctype/utils.py index 25239c2309e..2c151377816 100644 --- a/erpnext/crm/doctype/utils.py +++ b/erpnext/crm/doctype/utils.py @@ -1,4 +1,5 @@ import frappe +from frappe.query_builder import Criterion @frappe.whitelist() @@ -9,37 +10,33 @@ def get_last_interaction(contact: str | None = None, lead: str | None = None): last_communication = None last_issue = None if contact: - query_condition = "" - values = [] + communication = frappe.qb.DocType("Communication") + link_conditions = [] contact = frappe.get_doc("Contact", contact) for link in contact.links: if link.link_doctype == "Customer": last_issue = get_last_issue_from_customer(link.link_name) - query_condition += "(`reference_doctype`=%s AND `reference_name`=%s) OR" - values += [link.link_doctype, link.link_name] + link_conditions.append( + (communication.reference_doctype == link.link_doctype) + & (communication.reference_name == link.link_name) + ) - if query_condition: - # remove extra appended 'OR' - query_condition = query_condition[:-2] - last_communication = frappe.db.sql( - f""" - SELECT `name`, `content` - FROM `tabCommunication` - WHERE `sent_or_received`='Received' - AND ({query_condition}) - ORDER BY `creation` - LIMIT 1 - """, - values, - as_dict=1, - ) # nosec + if link_conditions: + last_communication = ( + frappe.qb.from_(communication) + .select(communication.name, communication.content) + .where((communication.sent_or_received == "Received") & Criterion.any(link_conditions)) + .orderby(communication.creation) + .limit(1) + .run(as_dict=1) + ) if lead: last_communication = frappe.get_all( "Communication", filters={"reference_doctype": "Lead", "reference_name": lead, "sent_or_received": "Received"}, fields=["name", "content"], - order_by="`creation` DESC", + order_by="creation desc", limit=1, ) @@ -53,7 +50,7 @@ def get_last_issue_from_customer(customer_name): "Issue", {"customer": customer_name}, ["name", "subject", "customer"], - order_by="`creation` DESC", + order_by="creation desc", limit=1, ) diff --git a/erpnext/crm/report/campaign_efficiency/campaign_efficiency.py b/erpnext/crm/report/campaign_efficiency/campaign_efficiency.py index bf17b2b9ca7..b842f4395c1 100644 --- a/erpnext/crm/report/campaign_efficiency/campaign_efficiency.py +++ b/erpnext/crm/report/campaign_efficiency/campaign_efficiency.py @@ -4,7 +4,8 @@ import frappe from frappe import _ -from frappe.utils import flt +from frappe.query_builder.functions import Sum +from frappe.utils import add_days, flt def execute(filters=None): @@ -30,17 +31,15 @@ def get_columns(based_on): def get_lead_data(filters, based_on): based_on_field = frappe.scrub(based_on) - conditions = get_filter_conditions(filters) - lead_details = frappe.db.sql( - f""" - select {based_on_field}, name - from `tabLead` - where {based_on_field} is not null and {based_on_field} != '' {conditions} - """, - filters, - as_dict=1, - ) + lead_filters = [[based_on_field, "is", "set"]] + if filters.from_date: + lead_filters.append(["creation", ">=", filters.from_date]) + if filters.to_date: + # date(creation) <= to_date, i.e. anything created before the next day + lead_filters.append(["creation", "<", add_days(filters.to_date, 1)]) + + lead_details = frappe.get_all("Lead", filters=lead_filters, fields=[based_on_field, "name"]) lead_map = frappe._dict() for d in lead_details: @@ -64,52 +63,36 @@ def get_lead_data(filters, based_on): return data -def get_filter_conditions(filters): - conditions = "" - if filters.from_date: - conditions += " and date(creation) >= %(from_date)s" - if filters.to_date: - conditions += " and date(creation) <= %(to_date)s" - - return conditions - - def get_lead_quotation_count(leads): - return frappe.db.sql( - """select count(name) from `tabQuotation` - where quotation_to = 'Lead' and party_name in (%s)""" - % ", ".join(["%s"] * len(leads)), - tuple(leads), - )[0][0] # nosec + return frappe.db.count("Quotation", {"quotation_to": "Lead", "party_name": ["in", leads]}) def get_lead_opp_count(leads): - return frappe.db.sql( - """select count(name) from `tabOpportunity` - where opportunity_from = 'Lead' and party_name in (%s)""" - % ", ".join(["%s"] * len(leads)), - tuple(leads), - )[0][0] + return frappe.db.count("Opportunity", {"opportunity_from": "Lead", "party_name": ["in", leads]}) def get_quotation_ordered_count(leads): - return frappe.db.sql( - """select count(name) - from `tabQuotation` where status = 'Ordered' and quotation_to = 'Lead' - and party_name in (%s)""" - % ", ".join(["%s"] * len(leads)), - tuple(leads), - )[0][0] + return frappe.db.count( + "Quotation", {"status": "Ordered", "quotation_to": "Lead", "party_name": ["in", leads]} + ) def get_order_amount(leads): - return frappe.db.sql( - """select sum(base_net_amount) - from `tabSales Order Item` - where prevdoc_docname in ( - select name from `tabQuotation` where status = 'Ordered' - and quotation_to = 'Lead' and party_name in (%s) - )""" - % ", ".join(["%s"] * len(leads)), - tuple(leads), + so_item = frappe.qb.DocType("Sales Order Item") + quotation = frappe.qb.DocType("Quotation") + return ( + frappe.qb.from_(so_item) + .select(Sum(so_item.base_net_amount)) + .where( + so_item.prevdoc_docname.isin( + frappe.qb.from_(quotation) + .select(quotation.name) + .where( + (quotation.status == "Ordered") + & (quotation.quotation_to == "Lead") + & quotation.party_name.isin(leads) + ) + ) + ) + .run() )[0][0] diff --git a/erpnext/crm/report/campaign_efficiency/test_campaign_efficiency.py b/erpnext/crm/report/campaign_efficiency/test_campaign_efficiency.py new file mode 100644 index 00000000000..eb0495c03dd --- /dev/null +++ b/erpnext/crm/report/campaign_efficiency/test_campaign_efficiency.py @@ -0,0 +1,46 @@ +# Copyright (c) 2024, Frappe Technologies Pvt. Ltd. and Contributors +# License: GNU General Public License v3. See license.txt + +import frappe +from frappe.utils import add_days, nowdate + +from erpnext.crm.report.campaign_efficiency.campaign_efficiency import execute +from erpnext.tests.utils import ERPNextTestSuite + + +class TestCampaignEfficiency(ERPNextTestSuite): + def test_lead_count_per_campaign(self): + """execute() groups Leads by utm_campaign over a creation-date window and counts leads per + group. Seed two Leads sharing one distinct UTM Campaign, run the report over a window that + includes their (now-dated) creation, and assert that campaign's row reports lead_count == 2. + The group is unique to this test, so the count is exact rather than a tautology, and both + MariaDB and Postgres must return the same row/value.""" + campaign = "_Test Campaign Eff Campaign" + if not frappe.db.exists("UTM Campaign", campaign): + frappe.get_doc({"doctype": "UTM Campaign", "__newname": campaign}).insert(ignore_permissions=True) + + for i in range(2): + frappe.get_doc( + { + "doctype": "Lead", + "lead_name": f"_Test Campaign Eff Lead {i}", + "utm_campaign": campaign, + } + ).insert(ignore_permissions=True) + + # from_date <= creation(now) < to_date + 1 -> window covers the freshly inserted leads + filters = frappe._dict( + { + "from_date": add_days(nowdate(), -7), + "to_date": add_days(nowdate(), 1), + "based_on": "utm_campaign", + } + ) + columns, data = execute(filters) + + row = next((r for r in data if r.get("utm_campaign") == campaign), None) + self.assertIsNotNone(row, "campaign row missing from report output") + self.assertEqual(row["lead_count"], 2) + # no quotations/orders seeded for these leads -> derived counts are zero + self.assertEqual(row["quot_count"], 0) + self.assertEqual(row["order_count"], 0) diff --git a/erpnext/crm/report/first_response_time_for_opportunity/first_response_time_for_opportunity.py b/erpnext/crm/report/first_response_time_for_opportunity/first_response_time_for_opportunity.py index db36581cecd..9aaa26a1bb0 100644 --- a/erpnext/crm/report/first_response_time_for_opportunity/first_response_time_for_opportunity.py +++ b/erpnext/crm/report/first_response_time_for_opportunity/first_response_time_for_opportunity.py @@ -4,6 +4,8 @@ import frappe from frappe import _ +from frappe.query_builder.functions import Avg, Date +from pypika import Order def execute(filters=None): @@ -17,19 +19,19 @@ def execute(filters=None): }, ] - data = frappe.db.sql( - """ - SELECT - date(creation) as creation_date, - avg(first_response_time) as avg_response_time - FROM tabOpportunity - 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), + opportunity = frappe.qb.DocType("Opportunity") + creation_date = Date(opportunity.creation) + data = ( + frappe.qb.from_(opportunity) + .select( + creation_date.as_("creation_date"), Avg(opportunity.first_response_time).as_("avg_response_time") + ) + .where( + creation_date.between(filters.from_date, filters.to_date) & (opportunity.first_response_time > 0) + ) + .groupby(creation_date) + .orderby(creation_date, order=Order.desc) + .run() ) return columns, data diff --git a/erpnext/crm/report/first_response_time_for_opportunity/test_first_response_time_for_opportunity.py b/erpnext/crm/report/first_response_time_for_opportunity/test_first_response_time_for_opportunity.py new file mode 100644 index 00000000000..584561bd1ad --- /dev/null +++ b/erpnext/crm/report/first_response_time_for_opportunity/test_first_response_time_for_opportunity.py @@ -0,0 +1,60 @@ +# Copyright (c) 2024, Frappe Technologies Pvt. Ltd. and Contributors +# License: GNU General Public License v3. See license.txt + +import frappe +from frappe.utils import add_days, getdate, nowdate + +from erpnext.crm.report.first_response_time_for_opportunity.first_response_time_for_opportunity import ( + execute, +) +from erpnext.tests.utils import ERPNextTestSuite + + +class TestFirstResponseTimeForOpportunity(ERPNextTestSuite): + def test_avg_first_response_time_row(self): + """The report groups Opportunity by Date(creation) and averages first_response_time where it + is > 0, between from_date and to_date. With a single seeded Opportunity created today and a + known first_response_time, the report must return a row for today whose averaged value equals + the seeded duration on both engines (Date(creation) and Avg via the query builder).""" + response_time = 3600 # seconds (Duration) + lead_email = "_test_frt_opp@example.com" + lead_name = "_Test FRT Opportunity Lead" + + lead = frappe.db.exists("Lead", {"email_id": lead_email}) + if not lead: + lead = ( + frappe.get_doc({"doctype": "Lead", "lead_name": lead_name, "email_id": lead_email}) + .insert(ignore_permissions=True) + .name + ) + + opportunity = frappe.get_doc( + { + "doctype": "Opportunity", + "opportunity_from": "Lead", + "party_name": lead, + "company": "_Test Company", + "currency": "INR", + "conversion_rate": 1, + } + ).insert(ignore_permissions=True) + + # first_response_time is a read-only computed field; set it directly. + frappe.db.set_value( + "Opportunity", + opportunity.name, + "first_response_time", + response_time, + update_modified=False, + ) + + columns, data = execute( + frappe._dict(from_date=add_days(nowdate(), -1), to_date=add_days(nowdate(), 1)) + ) + + # rows are positional lists: [creation_date, avg_response_time] + today = getdate(nowdate()) + row = next((r for r in data if getdate(r[0]) == today), None) + self.assertIsNotNone(row, "no report row for today's grouped creation date") + self.assertEqual(getdate(row[0]), today) + self.assertEqual(row[1], response_time) diff --git a/erpnext/crm/report/lead_conversion_time/lead_conversion_time.py b/erpnext/crm/report/lead_conversion_time/lead_conversion_time.py index f11b54a0be6..b2e76a02696 100644 --- a/erpnext/crm/report/lead_conversion_time/lead_conversion_time.py +++ b/erpnext/crm/report/lead_conversion_time/lead_conversion_time.py @@ -4,6 +4,7 @@ import frappe from frappe import _, msgprint +from frappe.query_builder.functions import Count, Date from frappe.utils import date_diff, flt @@ -83,59 +84,51 @@ def get_communication_details(filters): as_dict=1, ) + si = frappe.qb.DocType("Sales Invoice") + comm = frappe.qb.DocType("Communication") + for d in opportunities: - invoice = frappe.db.sql( - """ - SELECT - date(creation) - FROM - `tabSales Invoice` - WHERE - contact_email = %s AND date(creation) between %s and %s AND docstatus != 2 - ORDER BY - creation - LIMIT 1 - """, - (d.contact_email, filters.from_date, filters.to_date), + invoice = ( + frappe.qb.from_(si) + .select(Date(si.creation)) + .where( + (si.contact_email == d.contact_email) + & Date(si.creation).between(filters.from_date, filters.to_date) + & (si.docstatus != 2) + ) + .orderby(si.creation) + .limit(1) + .run() ) if not invoice: continue - communication_count = frappe.db.sql( - """ - SELECT - count(*) - FROM - `tabCommunication` - WHERE - sender = %s AND date(communication_date) <= %s - """, - (d.contact_email, invoice), + invoice_date = invoice[0][0] + + communication_count = ( + frappe.qb.from_(comm) + .select(Count("*")) + .where((comm.sender == d.contact_email) & (Date(comm.communication_date) <= invoice_date)) + .run() )[0][0] if not communication_count: continue - first_contact = frappe.db.sql( - """ - SELECT - date(communication_date) - FROM - `tabCommunication` - WHERE - recipients = %s AND communication_date IS NOT NULL - ORDER BY - communication_date - LIMIT 1 - """, - (d.contact_email), + first_contact = ( + frappe.qb.from_(comm) + .select(Date(comm.communication_date)) + .where((comm.recipients == d.contact_email) & comm.communication_date.isnotnull()) + .orderby(comm.communication_date) + .limit(1) + .run() ) first_contact = first_contact[0][0] if first_contact else None if not first_contact: continue - duration = flt(date_diff(invoice[0][0], first_contact)) + duration = flt(date_diff(invoice_date, first_contact)) support_tickets = len(frappe.db.get_all("Issue", {"raised_by": d.contact_email})) communication_list.append( diff --git a/erpnext/crm/report/lead_conversion_time/test_lead_conversion_time.py b/erpnext/crm/report/lead_conversion_time/test_lead_conversion_time.py new file mode 100644 index 00000000000..c307bc6db8c --- /dev/null +++ b/erpnext/crm/report/lead_conversion_time/test_lead_conversion_time.py @@ -0,0 +1,59 @@ +# Copyright (c) 2024, Frappe Technologies Pvt. Ltd. and Contributors +# License: GNU General Public License v3. See license.txt + +import frappe +from frappe.utils import add_days, nowdate + +from erpnext.accounts.doctype.sales_invoice.test_sales_invoice import create_sales_invoice +from erpnext.crm.report.lead_conversion_time.lead_conversion_time import execute +from erpnext.tests.utils import ERPNextTestSuite + + +class TestLeadConversionTime(ERPNextTestSuite): + def test_first_contact_ignores_null_communication_date(self): + """first_contact ordered by the nullable communication_date and read row[0][0]. With no + IS NOT NULL guard, MariaDB (NULLs-first) returned a NULL-dated Communication -> first_contact + None -> a wrong duration, while Postgres (NULLs-last) returned the earliest real date. Filtering + communication_date IS NOT NULL (and guarding the slice) makes both engines use the earliest + real contact date.""" + email = "_test_lead_conv@example.com" + customer_name = "_Test Lead Conv 22d" + + lead = frappe.get_doc({"doctype": "Lead", "lead_name": customer_name, "email_id": email}).insert( + ignore_permissions=True + ) + frappe.get_doc( + { + "doctype": "Opportunity", + "opportunity_from": "Lead", + "party_name": lead.name, + "company": "_Test Company", + "currency": "INR", + "conversion_rate": 1, + "contact_email": email, + "customer_name": customer_name, + } + ).insert(ignore_permissions=True) + + si = create_sales_invoice(do_not_save=1) + si.contact_email = email + si.save() # draft (docstatus 0 != 2); Date(creation) is today, within range + + # count query filters on `sender`; first_contact filters on `recipients` -> set both + real = frappe.get_doc( + {"doctype": "Communication", "subject": "real", "sender": email, "recipients": email} + ).insert(ignore_permissions=True) + frappe.db.set_value( + "Communication", real.name, "communication_date", add_days(nowdate(), -22), update_modified=False + ) + nulldate = frappe.get_doc( + {"doctype": "Communication", "subject": "nulldate", "sender": email, "recipients": email} + ).insert(ignore_permissions=True) + frappe.db.set_value("Communication", nulldate.name, "communication_date", None, update_modified=False) + + data = execute(frappe._dict({"from_date": add_days(nowdate(), -30), "to_date": nowdate()}))[1] + # rows are lists: [customer, interactions, duration, support_tickets] + row = next((r for r in data if r[0] == customer_name), None) + self.assertIsNotNone(row, "lead's converted-customer row missing") + # duration must be measured from the earliest REAL contact (22 days), not the NULL-dated one + self.assertEqual(row[2], 22.0) diff --git a/erpnext/crm/report/prospects_engaged_but_not_converted/prospects_engaged_but_not_converted.py b/erpnext/crm/report/prospects_engaged_but_not_converted/prospects_engaged_but_not_converted.py index 39b49b20f51..8c6eabad7b0 100644 --- a/erpnext/crm/report/prospects_engaged_but_not_converted/prospects_engaged_but_not_converted.py +++ b/erpnext/crm/report/prospects_engaged_but_not_converted/prospects_engaged_but_not_converted.py @@ -63,28 +63,41 @@ def get_data(filters): lead_filters = get_lead_filters(filters) for lead in frappe.get_all("Lead", fields=["name", "lead_name", "company_name"], filters=lead_filters): - data = frappe.db.sql( - """ - select - `tabCommunication`.reference_doctype, `tabCommunication`.reference_name, - `tabCommunication`.content, `tabCommunication`.communication_date - from - ( - (select name, party_name as lead from `tabOpportunity` where opportunity_from='Lead' and party_name = %(lead)s) - union - (select name, party_name as lead from `tabQuotation` where quotation_to = 'Lead' and party_name = %(lead)s) - union - (select name, lead from `tabIssue` where lead = %(lead)s and status!='Closed') - union - (select %(lead)s, %(lead)s) - ) - as ref_document, `tabCommunication` - where - `tabCommunication`.reference_name = ref_document.name and - `tabCommunication`.sent_or_received = 'Received' - order by - ref_document.lead, `tabCommunication`.creation desc limit %(limit)s""", - {"lead": lead.name, "limit": filters.get("no_of_interaction")}, + # Documents (and the lead itself) that communications may be referenced against + reference_names = set() + reference_names.update( + frappe.get_all( + "Opportunity", + filters={"opportunity_from": "Lead", "party_name": lead.name}, + pluck="name", + ) + ) + reference_names.update( + frappe.get_all( + "Quotation", + filters={"quotation_to": "Lead", "party_name": lead.name}, + pluck="name", + ) + ) + reference_names.update( + frappe.get_all( + "Issue", + filters={"lead": lead.name, "status": ["!=", "Closed"]}, + pluck="name", + ) + ) + reference_names.add(lead.name) + + data = frappe.get_all( + "Communication", + filters={ + "reference_name": ["in", list(reference_names)], + "sent_or_received": "Received", + }, + fields=["reference_doctype", "reference_name", "content", "communication_date"], + order_by="creation desc", + limit=filters.get("no_of_interaction"), + as_list=True, ) for lead_info in data: diff --git a/erpnext/crm/report/prospects_engaged_but_not_converted/test_prospects_engaged_but_not_converted.py b/erpnext/crm/report/prospects_engaged_but_not_converted/test_prospects_engaged_but_not_converted.py new file mode 100644 index 00000000000..1c1e0782d4b --- /dev/null +++ b/erpnext/crm/report/prospects_engaged_but_not_converted/test_prospects_engaged_but_not_converted.py @@ -0,0 +1,73 @@ +# Copyright (c) 2024, Frappe Technologies Pvt. Ltd. and Contributors +# License: GNU General Public License v3. See license.txt + +import frappe + +from erpnext.crm.report.prospects_engaged_but_not_converted.prospects_engaged_but_not_converted import ( + execute, +) +from erpnext.tests.utils import ERPNextTestSuite + + +class TestProspectsEngagedButNotConverted(ERPNextTestSuite): + def test_lead_with_received_communications_appears(self): + """The report lists non-converted Leads that have Communications referencing them + (reference_doctype="Lead", reference_name=lead.name) with sent_or_received="Received". + Seed one Lead and two such Received Communications, then assert the Lead surfaces in the + report data and that the emitted row carries the Lead -> reference_doctype/reference_name + linkage the get_data() join relies on. Asserting a concrete row (not a count) keeps this a + real-state smoke test that exercises the same path on both MariaDB and Postgres.""" + lead_name = "_Test Prospect Engaged" + email = "_test_prospect_engaged@example.com" + + lead = frappe.db.exists("Lead", {"lead_name": lead_name}) + if lead: + lead = frappe.get_doc("Lead", lead) + else: + lead = frappe.get_doc( + { + "doctype": "Lead", + "lead_name": lead_name, + "email_id": email, + "company_name": "_Test Prospect Org", + } + ).insert(ignore_permissions=True) + + # A fresh, non-converted Lead is required for it to pass the report's lead filters. + self.assertNotEqual(lead.status, "Converted") + + for subject in ("_test prospect engaged 1", "_test prospect engaged 2"): + if not frappe.db.exists( + "Communication", + { + "reference_doctype": "Lead", + "reference_name": lead.name, + "subject": subject, + }, + ): + frappe.get_doc( + { + "doctype": "Communication", + "communication_type": "Communication", + "subject": subject, + "content": subject, + "sent_or_received": "Received", + "reference_doctype": "Lead", + "reference_name": lead.name, + } + ).insert(ignore_permissions=True) + + # filters are accessed via .get(...) in the report, so a plain _dict suffices + columns, data = execute(frappe._dict(no_of_interaction=1)) + + # rows are lists: [lead, lead_name, company_name, reference_doctype, reference_name, content, date] + row = next((r for r in data if r[0] == lead.name), None) + self.assertIsNotNone(row, "seeded Lead with Received communications missing from report data") + self.assertEqual(row[3], "Lead") + self.assertEqual(row[4], lead.name) + # content comes from one of the two seeded Received communications + self.assertIn(row[5], ("_test prospect engaged 1", "_test prospect engaged 2")) + + # no_of_interaction=1 caps the per-lead communications to 1 -> exactly one row for this Lead + lead_rows = [r for r in data if r[0] == lead.name] + self.assertEqual(len(lead_rows), 1) From 413ec60a3e05f5ce840cd0cecc8c833c9585d620 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Fri, 19 Jun 2026 10:26:45 +0530 Subject: [PATCH 2/2] refactor(postgres): address Greptile review on prospects report - Fix N+1: the per-lead conversion issued 4 queries per lead (Opportunity, Quotation, Issue, Communication). Collect the reference documents for all leads in 3 bulk queries, then one Communication query per lead -> ~N+3 round-trips instead of 4N, matching the original single-query-per-lead cost. - Constrain reference_doctype in the Communication lookup (names are unique only within a doctype), closing a latent cross-doctype name-collision gap the original also had. Both-engine test still green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../prospects_engaged_but_not_converted.py | 56 ++++++++++--------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/erpnext/crm/report/prospects_engaged_but_not_converted/prospects_engaged_but_not_converted.py b/erpnext/crm/report/prospects_engaged_but_not_converted/prospects_engaged_but_not_converted.py index 8c6eabad7b0..ec138db8563 100644 --- a/erpnext/crm/report/prospects_engaged_but_not_converted/prospects_engaged_but_not_converted.py +++ b/erpnext/crm/report/prospects_engaged_but_not_converted/prospects_engaged_but_not_converted.py @@ -61,37 +61,41 @@ def get_columns(): def get_data(filters): lead_details = [] lead_filters = get_lead_filters(filters) + leads = frappe.get_all("Lead", fields=["name", "lead_name", "company_name"], filters=lead_filters) + if not leads: + return lead_details - for lead in frappe.get_all("Lead", fields=["name", "lead_name", "company_name"], filters=lead_filters): - # Documents (and the lead itself) that communications may be referenced against - reference_names = set() - reference_names.update( - frappe.get_all( - "Opportunity", - filters={"opportunity_from": "Lead", "party_name": lead.name}, - pluck="name", - ) - ) - reference_names.update( - frappe.get_all( - "Quotation", - filters={"quotation_to": "Lead", "party_name": lead.name}, - pluck="name", - ) - ) - reference_names.update( - frappe.get_all( - "Issue", - filters={"lead": lead.name, "status": ["!=", "Closed"]}, - pluck="name", - ) - ) - reference_names.add(lead.name) + lead_names = [lead.name for lead in leads] + # Collect the documents (and the lead itself) that communications may reference, for all leads in + # three bulk queries instead of three per lead. + reference_names = {name: {name} for name in lead_names} + for opp in frappe.get_all( + "Opportunity", + filters={"opportunity_from": "Lead", "party_name": ["in", lead_names]}, + fields=["name", "party_name"], + ): + reference_names[opp.party_name].add(opp.name) + for quotation in frappe.get_all( + "Quotation", + filters={"quotation_to": "Lead", "party_name": ["in", lead_names]}, + fields=["name", "party_name"], + ): + reference_names[quotation.party_name].add(quotation.name) + for issue in frappe.get_all( + "Issue", + filters={"lead": ["in", lead_names], "status": ["!=", "Closed"]}, + fields=["name", "lead"], + ): + reference_names[issue.lead].add(issue.name) + + for lead in leads: data = frappe.get_all( "Communication", filters={ - "reference_name": ["in", list(reference_names)], + # constrain the doctype too: names are unique only within a doctype + "reference_doctype": ["in", ["Lead", "Opportunity", "Quotation", "Issue"]], + "reference_name": ["in", list(reference_names[lead.name])], "sent_or_received": "Received", }, fields=["reference_doctype", "reference_name", "content", "communication_date"],