Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@

frappe.query_reports["Inactive Customers"] = {
filters: [
{
fieldname: "company",
label: __("Company"),
fieldtype: "Link",
options: "Company",
},
{
fieldname: "days_since_last_order",
label: __("Days Since Last Order"),
Expand Down
232 changes: 161 additions & 71 deletions erpnext/selling/report/inactive_customers/inactive_customers.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,81 +4,171 @@

import frappe
from frappe import _
from frappe.query_builder import Case, DocType, Order
from frappe.query_builder import functions as fn
from frappe.query_builder.utils import QueryBuilder
from frappe.utils import cint
from pypika.terms import Term

from erpnext import get_company_currency


def execute(filters=None):
if not filters:
filters = {}

days_since_last_order = filters.get("days_since_last_order")
doctype = filters.get("doctype")

if cint(days_since_last_order) <= 0:
frappe.throw(_("'Days Since Last Order' must be greater than or equal to zero"))

columns = get_columns()
customers = get_sales_details(doctype)

data = []
for cust in customers:
if cint(cust[8]) >= cint(days_since_last_order):
cust.insert(7, get_last_sales_amt(cust[0], doctype))
data.append(cust)
return columns, data


def get_sales_details(doctype):
cond = """sum(so.base_net_total) as 'total_order_considered',
max(so.posting_date) as 'last_order_date',
DATEDIFF(CURRENT_DATE, max(so.posting_date)) as 'days_since_last_order' """
if doctype == "Sales Order":
cond = """sum(if(so.status = "Stopped",
so.base_net_total * so.per_delivered/100,
so.base_net_total)) as 'total_order_considered',
max(so.transaction_date) as 'last_order_date',
DATEDIFF(CURRENT_DATE, max(so.transaction_date)) as 'days_since_last_order'"""

return frappe.db.sql(
f"""select
cust.name,
cust.customer_name,
cust.territory,
cust.customer_group,
count(distinct(so.name)) as 'num_of_order',
sum(base_net_total) as 'total_order_value', {cond}
from `tabCustomer` cust, `tab{doctype}` so
where cust.name = so.customer and so.docstatus = 1
group by cust.name
order by 'days_since_last_order' desc """,
as_list=1,
)


def get_last_sales_amt(customer, doctype):
cond = "posting_date"
if doctype == "Sales Order":
cond = "transaction_date"
res = frappe.db.sql(
f"""select base_net_total from `tab{doctype}`
where customer = %s and docstatus = 1 order by {cond} desc
limit 1""",
customer,
)

return res and res[0][0] or 0


def get_columns():
return [
_("Customer") + ":Link/Customer:120",
_("Customer Name") + ":Data:120",
_("Territory") + "::120",
_("Customer Group") + "::120",
_("Number of Order") + "::120",
_("Total Order Value") + ":Currency:120",
_("Total Order Considered") + ":Currency:160",
_("Last Order Amount") + ":Currency:160",
_("Last Order Date") + ":Date:160",
_("Days Since Last Order") + "::160",
]
return InactiveCustomersReport(filters).run()


class InactiveCustomersReport:
filters: dict
query: QueryBuilder
data: list
columns: list
date_field: str

def __init__(self, filters):
self.filters = filters
self.columns = []

def run(self):
self.validate_filters()
self.prepare_columns()
self.get_data()

return self.columns, self.data

def validate_filters(self):
# Mandatory filters.
filters = {"days_since_last_order": _("Days Since Last Order"), "doctype": _("DocType")}
for fieldname, label in filters.items():
if not self.filters.get(fieldname):
frappe.throw(_("{0} is a required filter.").format(frappe.bold(label)))

if fieldname == "days_since_last_order" and cint(self.filters.get(fieldname)) < 0:
frappe.throw(_("{0} must be greater than zero.").format(frappe.bold(label)))

if fieldname == "doctype" and self.filters.get(fieldname) not in ["Sales Invoice", "Sales Order"]:
frappe.throw(_("{0} can be either Sales Invoice or Sales Order.").format(label))
Comment on lines +41 to +52
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Validation incorrectly rejects days_since_last_order = 0 as missing.

The check not self.filters.get(fieldname) treats 0 as falsy, causing it to be rejected as a missing required filter. However, 0 is a valid input meaning "customers whose last order was today or earlier." Additionally, the error message on line 49 says "must be greater than zero" but the condition allows zero (< 0).

🐛 Proposed fix
 	def validate_filters(self):
 		# Mandatory filters.
-		filters = {"days_since_last_order": _("Days Since Last Order"), "doctype": _("DocType")}
-		for fieldname, label in filters.items():
-			if not self.filters.get(fieldname):
-				frappe.throw(_("{0} is a required filter.").format(frappe.bold(label)))
-
-			if fieldname == "days_since_last_order" and cint(self.filters.get(fieldname)) < 0:
-				frappe.throw(_("{0} must be greater than zero.").format(frappe.bold(label)))
-
-			if fieldname == "doctype" and self.filters.get(fieldname) not in ["Sales Invoice", "Sales Order"]:
-				frappe.throw(_("{0} can be either Sales Invoice or Sales Order.").format(label))
+		if self.filters.get("days_since_last_order") is None:
+			frappe.throw(_("{0} is a required filter.").format(frappe.bold(_("Days Since Last Order"))))
+
+		if cint(self.filters.get("days_since_last_order")) < 0:
+			frappe.throw(_("{0} must be zero or greater.").format(frappe.bold(_("Days Since Last Order"))))
+
+		if not self.filters.get("doctype"):
+			frappe.throw(_("{0} is a required filter.").format(frappe.bold(_("DocType"))))
+
+		if self.filters.get("doctype") not in ["Sales Invoice", "Sales Order"]:
+			frappe.throw(_("{0} can be either Sales Invoice or Sales Order.").format(_("DocType")))
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@erpnext/selling/report/inactive_customers/inactive_customers.py` around lines
41 - 52, The current validate_filters() treats 0 as missing because it uses "if
not self.filters.get(fieldname)"; change that to an explicit missing check
(e.g., if self.filters.get(fieldname) is None or self.filters.get(fieldname) ==
"") so numeric 0 is accepted, keep the numeric bounds check as "if fieldname ==
'days_since_last_order' and cint(self.filters.get(fieldname)) < 0" but update
the error text to say "must be zero or greater" (use frappe.bold(label) for
consistency); leave the doctype validation logic as-is.


def prepare_columns(self):
self.make_column(_("Customer"), "customer", "Link", options="Customer", width=200)

if frappe.get_single_value("Selling Settings", "cust_master_name") != "Customer Name":
self.make_column(_("Customer Name"), "customer_name", width=200)

self.make_column(_("Company"), "company", "Link", options="Company", width=200)

self.make_column(_("Territory"), "territory", "Link", options="Territory")

self.make_column(_("Customer Group"), "customer_group", "Link", options="Customer Group")

self.make_column(_("Number of Order"), "num_of_order", "Int")

self.make_column(_("Currency"), "currency", "Link", options="Currency", hidden=1)

self.make_column(_("Total Order Value"), "total_order_value", "Currency", 120, "currency")

self.make_column(_("Total Order Considered"), "total_order_considered", "Currency", 120, "currency")

self.make_column(
_("Last Order"), "last_order", "Link", options=self.filters.get("doctype"), width=200
)

self.make_column(_("Last Order Amount"), "last_order_amount", "Currency", 160, "currency")

self.make_column(_("Last Order Date"), "last_order_date", "Date")

self.make_column(_("Days Since Last Order"), "days_since_last_order", "Int")

def make_column(
self,
label: str,
fieldname: str,
fieldtype: str = "Data",
width: int = 140,
options: str = "",
hidden: int = 0,
):
self.columns.append(
dict(
label=label,
fieldname=fieldname,
fieldtype=fieldtype,
options=options,
width=width,
hidden=hidden,
)
)

def get_data(self):
self._build_query_and_get_data()
self._insert_last_sales_amt_and_company_currency()

def _build_query_and_get_data(self):
Customer = DocType("Customer")
SalesDocType = DocType(self.filters.get("doctype"))

self.date_field = (
"posting_date" if self.filters.get("doctype") == "Sales Invoice" else "transaction_date"
)

days_since_last_order = fn.CurDate() - fn.Max(fn.Field(self.date_field, table=SalesDocType))

sum_terms = SalesDocType.base_net_total
if self.filters.get("doctype") == "Sales Order":
sum_terms = (
Case()
.when(
SalesDocType.status == "Stopped",
SalesDocType.base_net_total * SalesDocType.per_delivered / 100,
)
.else_(sum_terms)
)

self.query = (
frappe.qb.from_(Customer)
.join(SalesDocType)
.on((Customer.name == SalesDocType.customer) & (SalesDocType.docstatus == 1))
.select(
Customer.name.as_("customer"),
Customer.customer_name,
Customer.territory,
Customer.customer_group,
SalesDocType.company,
fn.Count(SalesDocType.name, "num_of_order"),
fn.Sum(SalesDocType.base_net_total, "total_order_value"),
fn.Sum(sum_terms, "total_order_considered"),
fn.Max(fn.Field(self.date_field, table=SalesDocType), "last_order_date"),
days_since_last_order.as_("days_since_last_order"),
)
.groupby(Customer.name, SalesDocType.company)
.having(days_since_last_order >= self.filters.get("days_since_last_order"))
.orderby(Term("days_since_last_order"), order=Order.desc)
)
Comment on lines +129 to +148
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Returns are counted in aggregates but excluded from last order lookup for Sales Invoice.

The main query includes all submitted documents (docstatus == 1) without filtering is_return, so for Sales Invoice:

  • num_of_order counts returns
  • total_order_value includes negative return amounts
  • last_order_date could be a return's date

But _get_last_sales_details() excludes returns (is_return: 0). This inconsistency could lead to confusing results where the displayed "Last Order Date" doesn't match the date of the displayed "Last Order."

🐛 Proposed fix - exclude returns from the main query for Sales Invoice
 		self.query = (
 			frappe.qb.from_(Customer)
 			.join(SalesDocType)
 			.on((Customer.name == SalesDocType.customer) & (SalesDocType.docstatus == 1))
 			.select(
 				...
 			)
 			.groupby(Customer.name, SalesDocType.company)
 			.having(days_since_last_order >= self.filters.get("days_since_last_order"))
 			.orderby(Term("days_since_last_order"), order=Order.desc)
 		)
 
+		if self.filters.get("doctype") == "Sales Invoice":
+			self.query = self.query.where(SalesDocType.is_return == 0)
+
 		if self.filters.get("company"):
 			self.query = self.query.where(SalesDocType.company == self.filters.get("company"))
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@erpnext/selling/report/inactive_customers/inactive_customers.py` around lines
129 - 148, The main QB query (self.query) joins Customer with SalesDocType and
aggregates submitted docs (docstatus == 1) but doesn't exclude returns, causing
num_of_order, total_order_value and last_order_date to count/reflect Sales
Invoices with is_return = 1 while _get_last_sales_details() intentionally
excludes returns; modify the query building for Sales Invoice (identify via
SalesDocType or the value used for invoices) to add an additional filter
excluding returns (is_return == 0) when selecting/aggregating, so Customer,
SalesDocType, fn.Count, fn.Sum, fn.Max and days_since_last_order calculations
align with _get_last_sales_details() behavior.


if self.filters.get("company"):
self.query = self.query.where(SalesDocType.company == self.filters.get("company"))

self.data = self.query.run(as_dict=1)

def _insert_last_sales_amt_and_company_currency(self):
for d in self.data:
d.update({"currency": get_company_currency(d.get("company"))})
d.update(self._get_last_sales_details(d.get("customer")))

def _get_last_sales_details(self, customer):
filters = {"customer": customer, "docstatus": 1}

if self.filters.get("doctype") == "Sales Invoice":
filters.update({"is_return": 0})

last_sales_amount = frappe.get_all(
self.filters.get("doctype"),
fields=["name as last_order", "base_net_total as last_order_amount"],
filters=filters,
order_by=f"{self.date_field} desc",
limit=1,
)

return last_sales_amount[0] if last_sales_amount else {}
Comment on lines +160 to +174
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Last order details may show orders from a different company than the row's company.

The query groups by (customer, company), so a customer with orders from multiple companies will have separate rows. However, _get_last_sales_details() only filters by customer and docstatus, not by company. This means the "Last Order" and "Last Order Amount" could be from a different company than the row's company.

🐛 Proposed fix
-	def _get_last_sales_details(self, customer):
-		filters = {"customer": customer, "docstatus": 1}
+	def _get_last_sales_details(self, customer, company):
+		filters = {"customer": customer, "docstatus": 1, "company": company}
 
 		if self.filters.get("doctype") == "Sales Invoice":
 			filters.update({"is_return": 0})

Also update the caller on line 158:

-			d.update(self._get_last_sales_details(d.get("customer")))
+			d.update(self._get_last_sales_details(d.get("customer"), d.get("company")))
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@erpnext/selling/report/inactive_customers/inactive_customers.py` around lines
160 - 174, The last-order lookup in _get_last_sales_details only filters by
customer/docstatus and can return orders from a different company; change
_get_last_sales_details(self, customer) to accept a company argument (e.g.,
_get_last_sales_details(self, customer, company)) and add "company": company to
the filters dict (and keep the is_return condition), then update the caller that
invokes _get_last_sales_details to pass the row's company value so the query is
scoped to the correct company.

Loading