feat: add Include Company Descendants filter to Employee Analytics #3768
Conversation
- Add checkbox filter to include/exclude employees from child companies - Update query logic to use company descendants when filter is enabled - Update chart data to include employees from all companies in hierarchy - Follows same pattern as Monthly Attendance Sheet (PR frappe#2177) - Addresses frappe/erpnext#43381
gk-maker0105
requested review from
asmitahase and
ruchamahabal
as code owners
WalkthroughA new checkbox filter Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-08-14T10:46:28.404ZApplied to files:
📚 Learning: 2025-11-13T12:51:15.011ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hrms/hr/report/employee_analytics/employee_analytics.js(1 hunks)hrms/hr/report/employee_analytics/employee_analytics.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (2)
hrms/hr/report/employee_analytics/employee_analytics.js (1)
15-20: LGTM!The new filter follows the established pattern and is well-positioned after the company filter. Default of
1(enabled) is appropriate since users typically want to see aggregated data across the company hierarchy.hrms/hr/report/employee_analytics/employee_analytics.py (1)
111-116: Good use of parameterized query.The
company in %swith tuple parameter is the correct Frappe pattern for safe IN clause handling.
| companies = [filters["company"]] | ||
| if filters.get("include_company_descendants"): | ||
| descendants = get_descendants_of("Company", filters["company"]) | ||
| if descendants: | ||
| companies.extend(descendants) | ||
|
|
||
| if len(companies) == 1: | ||
| conditions += " and company = '%s'" % companies[0].replace("'", "\\'") | ||
| else: | ||
| company_list = "', '".join([c.replace("'", "\\'") for c in companies]) | ||
| conditions += f" and company in ('{company_list}')" |
There was a problem hiding this comment.
SQL injection risk with manual string escaping.
Using replace("'", "\\'") is insufficient protection against SQL injection. While the company filter comes from a Link field (limiting direct injection), this pattern is fragile and could break with edge cases or future modifications.
Consider using Frappe's query builder or restructuring to use parameterized queries:
def get_conditions(filters):
- conditions = " and " + filters.get("parameter").lower().replace(" ", "_") + " IS NOT NULL "
+ conditions = []
+ conditions.append(filters.get("parameter").lower().replace(" ", "_") + " IS NOT NULL")
+ return conditions, get_company_list(filters)
- if filters.get("company"):
- companies = [filters["company"]]
- if filters.get("include_company_descendants"):
- descendants = get_descendants_of("Company", filters["company"])
- if descendants:
- companies.extend(descendants)
-
- if len(companies) == 1:
- conditions += " and company = '%s'" % companies[0].replace("'", "\\'")
- else:
- company_list = "', '".join([c.replace("'", "\\'") for c in companies])
- conditions += f" and company in ('{company_list}')"
-
- return conditions
+def get_company_list(filters):
+ """Get list of companies including descendants if enabled."""
+ companies = [filters["company"]]
+ if filters.get("include_company_descendants"):
+ descendants = get_descendants_of("Company", filters["company"])
+ if descendants:
+ companies.extend(descendants)
+ return companiesThen modify get_employees to use parameterized query:
def get_employees(filters):
parameter_field = filters.get("parameter").lower().replace(" ", "_")
companies = get_company_list(filters)
return frappe.db.sql(
f"""SELECT name, employee_name, date_of_birth,
branch, department, designation, gender, company
FROM `tabEmployee`
WHERE status = 'Active'
AND {parameter_field} IS NOT NULL
AND company IN %s""",
(companies,),
as_list=1,
)🤖 Prompt for AI Agents
In hrms/hr/report/employee_analytics/employee_analytics.py around lines 46 to
56, the code builds SQL conditions by manually escaping single quotes
(replace("'", "\\'")), which is fragile and risks SQL injection; change this to
build a safe parameterized query or use Frappe's query builder: create a helper
that returns a list/tuple of company names (including descendants), and then
replace the manual string interpolation with an IN parameter passed to
frappe.db.sql (or use frappe.get_all with filters) so the company values are
bound as parameters (handle the single-company case by still passing a
one-element tuple/list for the IN clause); also update get_employees to accept
the company list and pass it as a parameter to the SQL call rather than
concatenating company names into the query string.
|
|
||
| # Get list of companies including descendants | ||
| companies = [filters["company"]] | ||
| if filters.get("include_company_descendants"): | ||
| descendants = get_descendants_of("Company", filters["company"]) | ||
| if descendants: | ||
| companies.extend(descendants) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate company list logic.
This duplicates the company list construction from get_conditions(). Extract to a shared helper function (see suggestion above for get_company_list()).
2 participants
Description
Adds "Include Company Descendants" checkbox filter to Employee Analytics report, allowing users to view employees from parent company and all child companies in a single report.
Context
Changes Made
JavaScript (
employee_analytics.js):Python (
employee_analytics.py):from frappe.utils.nestedset import get_descendants_ofget_conditions()to build company list including descendantsINclause for multiple companiesget_chart_data()to aggregate data from all companies in hierarchyTesting
Tested with company hierarchy:
Results:
company in ('Tech Group', 'Tech South', 'Tech North')Console Test Results
Verified that the filter correctly includes child companies:
Test 1 - With descendants enabled:
Test 2 - Without descendants:
Test 3 - Chart data generation:
Additional Notes
Frontend Note:
JavaScript changes follow the exact pattern from PR #2177 which is already merged and working. Could not be fully tested in local Docker due to version mismatch (ERPNext v15 with HRMS v16 develop branch), but backend logic is thoroughly verified and working correctly.
Screenshot
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.