From 59fbad66d1417221b44e0d17fe64b354057b5a36 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Thu, 21 Sep 2023 22:52:29 +0530 Subject: [PATCH 1/5] fix: alternate formula eval implementation Current frappe.safe_eval transforms code so if you have nested iterations with too much depth then it can hit recursion limit of python. There's no workaround for this and people need large formulas in some countries, so this is alternate implementation for that. (cherry picked from commit 7fb3508e2c38f6e42ca9f18b543df86519108bb5) # Conflicts: # hrms/payroll/doctype/salary_slip/salary_slip.py # hrms/payroll/doctype/salary_slip/test_salary_slip.py --- .../doctype/salary_slip/salary_slip.py | 60 ++++++++++++++++++- .../doctype/salary_slip/test_salary_slip.py | 47 +++++++++++++++ 2 files changed, 105 insertions(+), 2 deletions(-) diff --git a/hrms/payroll/doctype/salary_slip/salary_slip.py b/hrms/payroll/doctype/salary_slip/salary_slip.py index dbaf4f796e..2c2aea5daa 100644 --- a/hrms/payroll/doctype/salary_slip/salary_slip.py +++ b/hrms/payroll/doctype/salary_slip/salary_slip.py @@ -2,6 +2,7 @@ # License: GNU General Public License v3. See license.txt +import unicodedata from datetime import date import frappe @@ -1047,14 +1048,14 @@ def eval_condition_and_formula(self, struct_row, data): try: condition = sanitize_expression(struct_row.condition) if condition: - if not frappe.safe_eval(condition, self.whitelisted_globals, data): + if not __safe_eval(condition, self.whitelisted_globals, data): return None amount = struct_row.amount if struct_row.amount_based_on_formula: formula = sanitize_expression(struct_row.formula) if formula: amount = flt( - frappe.safe_eval(formula, self.whitelisted_globals, data), struct_row.precision("amount") + __safe_eval(formula, self.whitelisted_globals, data), struct_row.precision("amount") ) if amount: data[struct_row.abbr] = amount @@ -2235,3 +2236,58 @@ def throw_error_message(row, error, title, description=None): ).format(**data) frappe.throw(message, title=title) +<<<<<<< HEAD +======= + + +def on_doctype_update(): + frappe.db.add_index("Salary Slip", ["employee", "start_date", "end_date"]) + + +def __safe_eval(code: str, eval_globals: dict | None = None, eval_locals: dict | None = None): + """Old version of safe_eval from framework. + + Note: current frappe.safe_eval transforms code so if you have nested + iterations with too much depth then it can hit recursion limit of python. + There's no workaround for this and people need large formulas in some + countries so this is alternate implementation for that. + + WARNING: DO NOT use this function anywhere else outside of this file. + """ + code = unicodedata.normalize("NFKC", code) + + __check_attributes(code) + + whitelisted_globals = {"int": int, "float": float, "long": int, "round": round} + if not eval_globals: + eval_globals = {} + + eval_globals["__builtins__"] = {} + eval_globals.update(whitelisted_globals) + return eval(code, eval_globals, eval_locals) # nosemgrep + + +def __check_attributes(code: str) -> None: + import ast + + from frappe.utils.safe_exec import UNSAFE_ATTRIBUTES + + unsafe_attrs = set(UNSAFE_ATTRIBUTES).union(["__"]) - {"format"} + + for attribute in unsafe_attrs: + if attribute in code: + raise SyntaxError(f'Illegal rule {frappe.bold(code)}. Cannot use "{attribute}"') + + BLOCKED_NODES = (ast.NamedExpr,) + + tree = ast.parse(code, mode="eval") + for node in ast.walk(tree): + if isinstance(node, BLOCKED_NODES): + raise SyntaxError(f"Operation not allowed: line {node.lineno} column {node.col_offset}") + if ( + isinstance(node, ast.Attribute) + and isinstance(node.attr, str) + and node.attr in UNSAFE_ATTRIBUTES + ): + raise SyntaxError(f'Illegal rule {frappe.bold(code)}. Cannot use "{node.attr}"') +>>>>>>> 7fb3508e (fix: alternate formula eval implementation) diff --git a/hrms/payroll/doctype/salary_slip/test_salary_slip.py b/hrms/payroll/doctype/salary_slip/test_salary_slip.py index 7abe17b25d..d017230bf6 100644 --- a/hrms/payroll/doctype/salary_slip/test_salary_slip.py +++ b/hrms/payroll/doctype/salary_slip/test_salary_slip.py @@ -35,7 +35,19 @@ create_payroll_period, ) from hrms.payroll.doctype.payroll_entry.payroll_entry import get_month_details +<<<<<<< HEAD from hrms.payroll.doctype.salary_slip.salary_slip import make_salary_slip_from_timesheet +======= +from hrms.payroll.doctype.salary_slip.salary_slip import ( + HOLIDAYS_BETWEEN_DATES, + LEAVE_TYPE_MAP, + SALARY_COMPONENT_VALUES, + TAX_COMPONENTS_BY_COMPANY, + __safe_eval, + make_salary_slip_from_timesheet, +) +from hrms.payroll.doctype.salary_slip.salary_slip_loan_utils import if_lending_app_installed +>>>>>>> 7fb3508e (fix: alternate formula eval implementation) from hrms.payroll.doctype.salary_structure.salary_structure import make_salary_slip from hrms.tests.test_utils import get_first_sunday @@ -1387,6 +1399,41 @@ def test_variable_tax_component(self): self.assertListEqual(tax_component, ["_Test TDS"]) +class TestSalarySlipSafeEval(FrappeTestCase): + def test_safe_eval_for_salary_slip(self): + TEST_CASES = { + "1+1": 2, + '"abc" in "abl"': False, + '"a" in "abl"': True, + '"a" in ("a", "b")': True, + '"a" in {"a", "b"}': True, + '"a" in {"a": 1, "b": 2}': True, + '"a" in ["a" ,"b"]': True, + } + + for code, result in TEST_CASES.items(): + self.assertEqual(__safe_eval(code), result) + + self.assertRaises(AttributeError, __safe_eval, "frappe.utils.os.path", {}) + + # Doc/dict objects + user = frappe.new_doc("User") + user.user_type = "System User" + user.enabled = 1 + self.assertTrue(__safe_eval("user_type == 'System User'", eval_locals=user.as_dict())) + self.assertEqual( + "System User Test", __safe_eval("user_type + ' Test'", eval_locals=user.as_dict()) + ) + self.assertEqual(1, __safe_eval("int(enabled)", eval_locals=user.as_dict())) + + # Walrus not allowed + self.assertRaises(SyntaxError, __safe_eval, "(x := (40+2))") + + # Format check but saner + self.assertTrue(__safe_eval("'x' != 'Information Techonology'")) + self.assertRaises(SyntaxError, __safe_eval("'blah'.format(1)")) + + def make_income_tax_components(): tax_components = [ { From 2baea34d5a31a6e524f3cfc5f4b0c250d70deecd Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Fri, 22 Sep 2023 00:44:43 +0530 Subject: [PATCH 2/5] fix: use single underscore fn & fix assertions in tests (cherry picked from commit 68e03993e6439d41ce6d1c47fb8a256de76b7daf) --- .../payroll/doctype/salary_slip/salary_slip.py | 10 +++++----- .../doctype/salary_slip/test_salary_slip.py | 18 +++++++++--------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/hrms/payroll/doctype/salary_slip/salary_slip.py b/hrms/payroll/doctype/salary_slip/salary_slip.py index 2c2aea5daa..d2bb986d1e 100644 --- a/hrms/payroll/doctype/salary_slip/salary_slip.py +++ b/hrms/payroll/doctype/salary_slip/salary_slip.py @@ -1048,14 +1048,14 @@ def eval_condition_and_formula(self, struct_row, data): try: condition = sanitize_expression(struct_row.condition) if condition: - if not __safe_eval(condition, self.whitelisted_globals, data): + if not _safe_eval(condition, self.whitelisted_globals, data): return None amount = struct_row.amount if struct_row.amount_based_on_formula: formula = sanitize_expression(struct_row.formula) if formula: amount = flt( - __safe_eval(formula, self.whitelisted_globals, data), struct_row.precision("amount") + _safe_eval(formula, self.whitelisted_globals, data), struct_row.precision("amount") ) if amount: data[struct_row.abbr] = amount @@ -2244,7 +2244,7 @@ def on_doctype_update(): frappe.db.add_index("Salary Slip", ["employee", "start_date", "end_date"]) -def __safe_eval(code: str, eval_globals: dict | None = None, eval_locals: dict | None = None): +def _safe_eval(code: str, eval_globals: dict | None = None, eval_locals: dict | None = None): """Old version of safe_eval from framework. Note: current frappe.safe_eval transforms code so if you have nested @@ -2256,7 +2256,7 @@ def __safe_eval(code: str, eval_globals: dict | None = None, eval_locals: dict | """ code = unicodedata.normalize("NFKC", code) - __check_attributes(code) + _check_attributes(code) whitelisted_globals = {"int": int, "float": float, "long": int, "round": round} if not eval_globals: @@ -2267,7 +2267,7 @@ def __safe_eval(code: str, eval_globals: dict | None = None, eval_locals: dict | return eval(code, eval_globals, eval_locals) # nosemgrep -def __check_attributes(code: str) -> None: +def _check_attributes(code: str) -> None: import ast from frappe.utils.safe_exec import UNSAFE_ATTRIBUTES diff --git a/hrms/payroll/doctype/salary_slip/test_salary_slip.py b/hrms/payroll/doctype/salary_slip/test_salary_slip.py index d017230bf6..cc8b058f4b 100644 --- a/hrms/payroll/doctype/salary_slip/test_salary_slip.py +++ b/hrms/payroll/doctype/salary_slip/test_salary_slip.py @@ -43,7 +43,7 @@ LEAVE_TYPE_MAP, SALARY_COMPONENT_VALUES, TAX_COMPONENTS_BY_COMPANY, - __safe_eval, + _safe_eval, make_salary_slip_from_timesheet, ) from hrms.payroll.doctype.salary_slip.salary_slip_loan_utils import if_lending_app_installed @@ -1412,26 +1412,26 @@ def test_safe_eval_for_salary_slip(self): } for code, result in TEST_CASES.items(): - self.assertEqual(__safe_eval(code), result) + self.assertEqual(_safe_eval(code), result) - self.assertRaises(AttributeError, __safe_eval, "frappe.utils.os.path", {}) + self.assertRaises(NameError, _safe_eval, "frappe.utils.os.path", {}) # Doc/dict objects user = frappe.new_doc("User") user.user_type = "System User" user.enabled = 1 - self.assertTrue(__safe_eval("user_type == 'System User'", eval_locals=user.as_dict())) + self.assertTrue(_safe_eval("user_type == 'System User'", eval_locals=user.as_dict())) self.assertEqual( - "System User Test", __safe_eval("user_type + ' Test'", eval_locals=user.as_dict()) + "System User Test", _safe_eval("user_type + ' Test'", eval_locals=user.as_dict()) ) - self.assertEqual(1, __safe_eval("int(enabled)", eval_locals=user.as_dict())) + self.assertEqual(1, _safe_eval("int(enabled)", eval_locals=user.as_dict())) # Walrus not allowed - self.assertRaises(SyntaxError, __safe_eval, "(x := (40+2))") + self.assertRaises(SyntaxError, _safe_eval, "(x := (40+2))") # Format check but saner - self.assertTrue(__safe_eval("'x' != 'Information Techonology'")) - self.assertRaises(SyntaxError, __safe_eval("'blah'.format(1)")) + self.assertTrue(_safe_eval("'x' != 'Information Techonology'")) + self.assertRaises(SyntaxError, _safe_eval, "'blah'.format(1)") def make_income_tax_components(): From 06f319e8d2ff2cbc8dbf62f0743ad66756672778 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Fri, 22 Sep 2023 14:42:33 +0530 Subject: [PATCH 3/5] chore: conflicts --- hrms/payroll/doctype/salary_slip/salary_slip.py | 7 ------- hrms/payroll/doctype/salary_slip/test_salary_slip.py | 9 --------- 2 files changed, 16 deletions(-) diff --git a/hrms/payroll/doctype/salary_slip/salary_slip.py b/hrms/payroll/doctype/salary_slip/salary_slip.py index d2bb986d1e..0dd7162ed5 100644 --- a/hrms/payroll/doctype/salary_slip/salary_slip.py +++ b/hrms/payroll/doctype/salary_slip/salary_slip.py @@ -2236,12 +2236,6 @@ def throw_error_message(row, error, title, description=None): ).format(**data) frappe.throw(message, title=title) -<<<<<<< HEAD -======= - - -def on_doctype_update(): - frappe.db.add_index("Salary Slip", ["employee", "start_date", "end_date"]) def _safe_eval(code: str, eval_globals: dict | None = None, eval_locals: dict | None = None): @@ -2290,4 +2284,3 @@ def _check_attributes(code: str) -> None: and node.attr in UNSAFE_ATTRIBUTES ): raise SyntaxError(f'Illegal rule {frappe.bold(code)}. Cannot use "{node.attr}"') ->>>>>>> 7fb3508e (fix: alternate formula eval implementation) diff --git a/hrms/payroll/doctype/salary_slip/test_salary_slip.py b/hrms/payroll/doctype/salary_slip/test_salary_slip.py index cc8b058f4b..3097347332 100644 --- a/hrms/payroll/doctype/salary_slip/test_salary_slip.py +++ b/hrms/payroll/doctype/salary_slip/test_salary_slip.py @@ -35,19 +35,10 @@ create_payroll_period, ) from hrms.payroll.doctype.payroll_entry.payroll_entry import get_month_details -<<<<<<< HEAD -from hrms.payroll.doctype.salary_slip.salary_slip import make_salary_slip_from_timesheet -======= from hrms.payroll.doctype.salary_slip.salary_slip import ( - HOLIDAYS_BETWEEN_DATES, - LEAVE_TYPE_MAP, - SALARY_COMPONENT_VALUES, - TAX_COMPONENTS_BY_COMPANY, _safe_eval, make_salary_slip_from_timesheet, ) -from hrms.payroll.doctype.salary_slip.salary_slip_loan_utils import if_lending_app_installed ->>>>>>> 7fb3508e (fix: alternate formula eval implementation) from hrms.payroll.doctype.salary_structure.salary_structure import make_salary_slip from hrms.tests.test_utils import get_first_sunday From 037d12e0c6abb3e4fc39c1e1ef17891f5492abde Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Fri, 22 Sep 2023 17:56:34 +0530 Subject: [PATCH 4/5] fix: compensatory leave request for Half Day & WFH (backport #906) (#907) Co-authored-by: Rucha Mahabal --- .../compensatory_leave_request.py | 15 +++++-- .../test_compensatory_leave_request.py | 45 +++++++++++++++---- 2 files changed, 49 insertions(+), 11 deletions(-) diff --git a/hrms/hr/doctype/compensatory_leave_request/compensatory_leave_request.py b/hrms/hr/doctype/compensatory_leave_request/compensatory_leave_request.py index 689ceccb3d..efa4fdcd6e 100644 --- a/hrms/hr/doctype/compensatory_leave_request/compensatory_leave_request.py +++ b/hrms/hr/doctype/compensatory_leave_request/compensatory_leave_request.py @@ -35,18 +35,27 @@ def validate(self): frappe.throw(_("Leave Type is madatory")) def validate_attendance(self): - attendance = frappe.get_all( + attendance_records = frappe.get_all( "Attendance", filters={ "attendance_date": ["between", (self.work_from_date, self.work_end_date)], - "status": "Present", + "status": ("in", ["Present", "Work From Home", "Half Day"]), "docstatus": 1, "employee": self.employee, }, fields=["attendance_date", "status"], ) - if len(attendance) < date_diff(self.work_end_date, self.work_from_date) + 1: + half_days = [entry.attendance_date for entry in attendance_records if entry.status == "Half Day"] + + if half_days and (not self.half_day or getdate(self.half_day_date) not in half_days): + frappe.throw( + _( + "You were only present for Half Day on {}. Cannot apply for a full day compensatory leave" + ).format(", ".join([frappe.bold(format_date(half_day)) for half_day in half_days])) + ) + + if len(attendance_records) < date_diff(self.work_end_date, self.work_from_date) + 1: frappe.throw(_("You are not present all day(s) between compensatory leave request days")) def validate_holidays(self): diff --git a/hrms/hr/doctype/compensatory_leave_request/test_compensatory_leave_request.py b/hrms/hr/doctype/compensatory_leave_request/test_compensatory_leave_request.py index f1c4da5f22..66ec3b0260 100644 --- a/hrms/hr/doctype/compensatory_leave_request/test_compensatory_leave_request.py +++ b/hrms/hr/doctype/compensatory_leave_request/test_compensatory_leave_request.py @@ -14,14 +14,12 @@ class TestCompensatoryLeaveRequest(FrappeTestCase): def setUp(self): - frappe.db.sql(""" delete from `tabCompensatory Leave Request`""") - frappe.db.sql(""" delete from `tabLeave Ledger Entry`""") - frappe.db.sql(""" delete from `tabLeave Allocation`""") - frappe.db.sql( - """ delete from `tabAttendance` where attendance_date in {0} """.format( - (today(), add_days(today(), -1)) - ) - ) # nosec + frappe.db.delete("Compensatory Leave Request") + frappe.db.delete("Leave Ledger Entry") + frappe.db.delete("Leave Allocation") + frappe.db.delete("Attendance") + frappe.db.delete("Leave Period") + create_leave_period(add_months(today(), -3), add_months(today(), 3), "_Test Company") create_holiday_list() @@ -99,6 +97,37 @@ def test_creation_of_leave_ledger_entry_on_submit(self): self.assertEqual(leave_ledger_entry[0].leave_type, compensatory_leave_request.leave_type) self.assertEqual(leave_ledger_entry[0].leaves, -1) + def test_half_day_compensatory_leave(self): + employee = get_employee() + mark_attendance(employee, status="Half Day") + date = today() + compensatory_leave_request = frappe.new_doc("Compensatory Leave Request") + compensatory_leave_request.update( + dict( + employee=employee.name, + leave_type="Compensatory Off", + work_from_date=date, + work_end_date=date, + reason="test", + ) + ) + + # cannot apply for full day compensatory leave for a half day attendance + self.assertRaises(frappe.ValidationError, compensatory_leave_request.submit) + + compensatory_leave_request.half_day = 1 + compensatory_leave_request.half_day_date = date + compensatory_leave_request.submit() + + # check creation of leave ledger entry on submission of leave request + leave_ledger_entry = frappe.get_all( + "Leave Ledger Entry", + fields="*", + filters={"transaction_name": compensatory_leave_request.leave_allocation}, + ) + + self.assertEqual(leave_ledger_entry[0].leaves, 0.5) + def get_compensatory_leave_request(employee, leave_date=today()): prev_comp_leave_req = frappe.db.get_value( From 4f4bbe0c971d0984f9653228906af47be06edcc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Rold=C3=A1n?= Date: Mon, 25 Sep 2023 02:08:01 -0300 Subject: [PATCH 5/5] fix: required_apps (#911) --- hrms/hooks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hrms/hooks.py b/hrms/hooks.py index f084e1084f..61f7afc821 100644 --- a/hrms/hooks.py +++ b/hrms/hooks.py @@ -4,7 +4,7 @@ app_description = "Modern HR and Payroll Software" app_email = "contact@frappe.io" app_license = "GNU General Public License (v3)" -required_apps = ["erpnext"] +required_apps = ["frappe/erpnext"] # Includes in