refactor: Inactive Customers Report#54574
Open
diptanilsaha wants to merge 1 commit intofrappe:developfrom
Open
refactor: Inactive Customers Report#54574diptanilsaha wants to merge 1 commit intofrappe:developfrom
Inactive Customers Report#54574diptanilsaha wants to merge 1 commit intofrappe:developfrom
Conversation
Member
diptanilsaha
commented
Apr 28, 2026
diptanilsaha
commented
- Add Test Cases
github-actions
Bot
added
needs-tests
diptanilsaha
added
dont-merge
and removed
needs-tests
Contributor
📝 WalkthroughWalkthroughThe Inactive Customers report is enhanced with a company filter field in the report configuration and refactored from a procedural SQL-based approach to a class-based implementation. The refactoring introduces validation requiring both Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@erpnext/selling/report/inactive_customers/inactive_customers.py`:
- Around line 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.
- Around line 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.
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: bb72e08b-cc1d-4d1f-9c25-8fcd4240cf85
📒 Files selected for processing (2)
erpnext/selling/report/inactive_customers/inactive_customers.jserpnext/selling/report/inactive_customers/inactive_customers.py
Comment on lines
+41
to
+52
| 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)) |
Contributor
There was a problem hiding this comment.
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.
Comment on lines
+129
to
+148
| 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) | ||
| ) |
Contributor
There was a problem hiding this comment.
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_ordercounts returnstotal_order_valueincludes negative return amountslast_order_datecould 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.
Comment on lines
+160
to
+174
| 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 {} |
Contributor
There was a problem hiding this comment.
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #54574 +/- ##
===========================================
- Coverage 79.44% 79.41% -0.03%
===========================================
Files 1160 1160
Lines 126147 126193 +46
===========================================
+ Hits 100212 100217 +5
- Misses 25935 25976 +41
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.