From ef075d7fa6993f2050cc740685c0c0538fe9eadd Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Sat, 31 Aug 2024 12:30:21 +0530 Subject: [PATCH 1/3] fix(Payroll): multiline condition & formula eval failing - sanitize condition & formula fields in structure doc reference to avoid accidental reference to unsanitized fields across functions regression: https://github.com/frappe/hrms/pull/2088 (cherry picked from commit 30572982ed49f61aab95535943370450266060af) --- .../doctype/salary_slip/salary_slip.py | 30 +++++++++++-------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/hrms/payroll/doctype/salary_slip/salary_slip.py b/hrms/payroll/doctype/salary_slip/salary_slip.py index a116c66f5a..5f2787fded 100644 --- a/hrms/payroll/doctype/salary_slip/salary_slip.py +++ b/hrms/payroll/doctype/salary_slip/salary_slip.py @@ -1097,7 +1097,7 @@ def get_income_tax_deducted_till_date(self): def calculate_component_amounts(self, component_type): if not getattr(self, "_salary_structure_doc", None): - self._salary_structure_doc = frappe.get_cached_doc("Salary Structure", self.salary_structure) + self.set_salary_structure_doc() self.add_structure_components(component_type) self.add_additional_salary_components(component_type) @@ -1106,6 +1106,14 @@ def calculate_component_amounts(self, component_type): else: self.add_tax_components() + def set_salary_structure_doc(self) -> None: + self._salary_structure_doc = frappe.get_cached_doc("Salary Structure", self.salary_structure) + # sanitize condition and formula fields + for table in ("earnings", "deductions"): + for row in self._salary_structure_doc.get(table): + row.condition = sanitize_expression(row.condition) + row.formula = sanitize_expression(row.formula) + def add_structure_components(self, component_type): self.data, self.default_data = self.get_data_for_eval() @@ -1192,17 +1200,13 @@ def _fetch_component_values(): 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): - 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") - ) + condition, formula, amount = struct_row.condition, struct_row.formula, struct_row.amount + if condition and not _safe_eval(condition, self.whitelisted_globals, data): + return None + if struct_row.amount_based_on_formula and formula: + amount = flt( + _safe_eval(formula, self.whitelisted_globals, data), struct_row.precision("amount") + ) if amount: data[struct_row.abbr] = amount @@ -2346,4 +2350,4 @@ def email_salary_slips(names) -> None: def get_variables_from_formula(formula: str) -> list[str]: - return [node.id for node in ast.walk(ast.parse(formula)) if isinstance(node, ast.Name)] + return [node.id for node in ast.walk(ast.parse(formula, mode="eval")) if isinstance(node, ast.Name)] From 860ede77ea851449b30e40f3e74f6b30b0d20b96 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Sat, 31 Aug 2024 12:35:25 +0530 Subject: [PATCH 2/3] fix(Salary Structure): sanitize expression fields on `before_update_after_submit` too - multiline formula updates error out on submitted doc because sanitization was only done on `before_validate` (cherry picked from commit f95da271364976d697a6a7edfecc3e4342b071cc) --- hrms/payroll/doctype/salary_structure/salary_structure.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/hrms/payroll/doctype/salary_structure/salary_structure.py b/hrms/payroll/doctype/salary_structure/salary_structure.py index be273466de..e7b3eaaf19 100644 --- a/hrms/payroll/doctype/salary_structure/salary_structure.py +++ b/hrms/payroll/doctype/salary_structure/salary_structure.py @@ -18,6 +18,9 @@ class SalaryStructure(Document): def before_validate(self): self.sanitize_condition_and_formula_fields() + def before_update_after_submit(self): + self.sanitize_condition_and_formula_fields() + def validate(self): self.set_missing_values() self.validate_amount() @@ -30,6 +33,9 @@ def validate(self): def on_update(self): self.reset_condition_and_formula_fields() + def on_update_after_submit(self): + self.reset_condition_and_formula_fields() + def validate_formula_setup(self): for table in ["earnings", "deductions"]: for row in self.get(table): From 8194a25bed19fad9ad4491bfb6692544b3b1c773 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Sat, 31 Aug 2024 13:04:06 +0530 Subject: [PATCH 3/3] test: multiline formula with circular dependency (cherry picked from commit 70cd388e3b1e68d4b01a438d05566abf0659056b) --- hrms/payroll/doctype/salary_slip/test_salary_slip.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hrms/payroll/doctype/salary_slip/test_salary_slip.py b/hrms/payroll/doctype/salary_slip/test_salary_slip.py index babd714440..0305859d18 100644 --- a/hrms/payroll/doctype/salary_slip/test_salary_slip.py +++ b/hrms/payroll/doctype/salary_slip/test_salary_slip.py @@ -1663,7 +1663,7 @@ def test_circular_dependency_in_formula(self): "abbr": "DD", "type": "Deduction", "amount_based_on_formula": 1, - "formula": "DE / 5", + "formula": "DE / 5\nif DE > 0\n else 0", }, ] make_salary_component(deductions, False, company_list=[])