WARNING: THIS SITE IS A MIRROR OF GITHUB.COM / IT CANNOT LOGIN OR REGISTER ACCOUNTS / THE CONTENTS ARE PROVIDED AS-IS / THIS SITE ASSUMES NO RESPONSIBILITY FOR ANY DISPLAYED CONTENT OR LINKS / IF YOU FOUND SOMETHING MAY NOT GOOD FOR EVERYONE, CONTACT ADMIN AT ilovescratch@foxmail.com
Skip to content

feat: add Include Company Descendants filter to Employee Analytics #3768

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gk-maker0105 wants to merge 2 commits into
base: develop
Choose a base branch
from
+38 −7
Open

feat: add Include Company Descendants filter to Employee Analytics #3768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
293485d
feat: add Include Company Descendants filter to Employee Analytics
gk-maker0105 Nov 25, 2025
e2ebc7a
fix: add status = 'Active' filter to employee count query
gk-maker0105 Nov 25, 2025
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
6 changes: 6 additions & 0 deletions hrms/hr/report/employee_analytics/employee_analytics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@ frappe.query_reports["Employee Analytics"] = {
default: frappe.defaults.get_user_default("Company"),
reqd: 1,
},
{
fieldname: "include_company_descendants",
label: __("Include Company Descendants"),
fieldtype: "Check",
default: 1,
},
{
fieldname: "parameter",
label: __("Parameter"),
Expand Down
39 changes: 32 additions & 7 deletions hrms/hr/report/employee_analytics/employee_analytics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import frappe
from frappe import _
from frappe.utils.nestedset import get_descendants_of


def execute(filters=None):
Expand Down Expand Up @@ -42,7 +43,18 @@ def get_conditions(filters):
conditions = " and " + filters.get("parameter").lower().replace(" ", "_") + " IS NOT NULL "

if filters.get("company"):
conditions += " and company = '%s'" % filters["company"].replace("'", "\\'")
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}')"
Comment on lines +46 to +56
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 25, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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 companies

Then 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.


return conditions


Expand All @@ -66,21 +78,28 @@ def get_parameters(filters):

return frappe.db.sql("""select name from `tab""" + parameter + """` """, as_list=1)


def get_chart_data(parameters, employees, filters):
if not parameters:
parameters = []
datasets = []
parameter_field_name = filters.get("parameter").lower().replace(" ", "_")
label = []

# 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)
Comment on lines +87 to +93
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 25, 2025

Choose a reason for hiding this comment

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

🛠️ 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()).


for parameter in parameters:
if parameter:
total_employee = frappe.db.sql(
"""select count(*) from
`tabEmployee` where """
`tabEmployee` where status = 'Active' and """
+ parameter_field_name
+ """ = %s and company = %s""",
(parameter[0], filters.get("company")),
+ """ = %s and company in %s""",
(parameter[0], companies),
as_list=1,
)
if total_employee[0][0]:
Expand All @@ -89,12 +108,18 @@ def get_chart_data(parameters, employees, filters):

values = [value for value in datasets if value != 0]

total_employee = frappe.db.count("Employee", {"status": "Active"})
total_employee = frappe.db.sql(
"""select count(*) from `tabEmployee`
where status = 'Active' and company in %s""",
(companies,),
as_list=1,
)[0][0]

others = total_employee - sum(values)

label.append(["Not Set"])
values.append(others)

chart = {"data": {"labels": label, "datasets": [{"name": "Employees", "values": values}]}}
chart["type"] = "donut"
return chart
return chart