From 7f76024bf45837afbe8693de69ae6b893a277f03 Mon Sep 17 00:00:00 2001 From: Krishna Shirsath Date: Mon, 8 Jun 2026 22:48:26 +0530 Subject: [PATCH 01/10] fix(vehicle_log): add validation to prevent cancellation of Vehicle Log linked to Expense Claim. --- .../doctype/vehicle_log/test_vehicle_log.py | 35 +++++++++++++++++++ hrms/hr/doctype/vehicle_log/vehicle_log.py | 15 ++++++++ 2 files changed, 50 insertions(+) diff --git a/hrms/hr/doctype/vehicle_log/test_vehicle_log.py b/hrms/hr/doctype/vehicle_log/test_vehicle_log.py index bba8e1a7ef..9c6165fbb7 100644 --- a/hrms/hr/doctype/vehicle_log/test_vehicle_log.py +++ b/hrms/hr/doctype/vehicle_log/test_vehicle_log.py @@ -61,6 +61,41 @@ def test_vehicle_log_with_service_expenses(self): frappe.delete_doc("Expense Claim", expense_claim.name) frappe.delete_doc("Vehicle Log", vehicle_log.name) + def test_cancel_vehicle_log_linked_to_draft_expense_claim(self): + vehicle_log = make_vehicle_log(self.license_plate, self.employee_id) + currency, cost_center = frappe.db.get_value( + "Company", "_Test Company", ["default_currency", "cost_center"] + ) + expense_claim = frappe.get_doc( + { + "doctype": "Expense Claim", + "employee": self.employee_id, + "company": "_Test Company", + "currency": currency, + "exchange_rate": 1, + "payable_account": frappe.db.get_value("Company", "_Test Company", "default_payable_account"), + "vehicle_log": vehicle_log.name, + "expenses": [ + { + "expense_type": "Travel", + "default_account": "Travel Expenses - _TC", + "currency": currency, + "amount": 100, + "sanctioned_amount": 100, + "cost_center": cost_center, + } + ], + } + ).insert() + + self.assertRaises(frappe.ValidationError, vehicle_log.cancel) + vehicle_log.reload() + self.assertEqual(vehicle_log.docstatus, 1) + + expense_claim.delete() + vehicle_log.cancel() + frappe.delete_doc("Vehicle Log", vehicle_log.name) + def get_vehicle(employee_id): license_plate = random_string(10).upper() diff --git a/hrms/hr/doctype/vehicle_log/vehicle_log.py b/hrms/hr/doctype/vehicle_log/vehicle_log.py index 7b3ae5c6f7..479f2528da 100644 --- a/hrms/hr/doctype/vehicle_log/vehicle_log.py +++ b/hrms/hr/doctype/vehicle_log/vehicle_log.py @@ -46,6 +46,21 @@ def validate(self): def on_submit(self): frappe.db.set_value("Vehicle", self.license_plate, "last_odometer", self.odometer) + def before_cancel(self): + expense_claim = frappe.db.exists( + "Expense Claim", + { + "vehicle_log": self.name, + "docstatus": ["!=", 2], + }, + ) + if expense_claim: + frappe.throw( + _("Cannot cancel Vehicle Log {0} because it is linked to Expense Claim {1}").format( + self.name, expense_claim + ) + ) + def on_cancel(self): distance_travelled = self.odometer - self.last_odometer if distance_travelled > 0: From eff239f91306741cb16a2d39297e534119b5e6fe Mon Sep 17 00:00:00 2001 From: Krishna Shirsath Date: Thu, 11 Jun 2026 17:05:11 +0530 Subject: [PATCH 02/10] fix(vehicle_log): update cancellation logic to unlink Vehicle Log from submitted Expense Claims. --- hrms/hr/doctype/vehicle_log/test_vehicle_log.py | 15 ++++++++++----- hrms/hr/doctype/vehicle_log/vehicle_log.py | 17 ++++++++++++----- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/hrms/hr/doctype/vehicle_log/test_vehicle_log.py b/hrms/hr/doctype/vehicle_log/test_vehicle_log.py index 9c6165fbb7..a8a80dcaf7 100644 --- a/hrms/hr/doctype/vehicle_log/test_vehicle_log.py +++ b/hrms/hr/doctype/vehicle_log/test_vehicle_log.py @@ -61,7 +61,7 @@ def test_vehicle_log_with_service_expenses(self): frappe.delete_doc("Expense Claim", expense_claim.name) frappe.delete_doc("Vehicle Log", vehicle_log.name) - def test_cancel_vehicle_log_linked_to_draft_expense_claim(self): + def test_cancel_vehicle_log_unlinks_draft_expense_claim(self): vehicle_log = make_vehicle_log(self.license_plate, self.employee_id) currency, cost_center = frappe.db.get_value( "Company", "_Test Company", ["default_currency", "cost_center"] @@ -73,6 +73,7 @@ def test_cancel_vehicle_log_linked_to_draft_expense_claim(self): "company": "_Test Company", "currency": currency, "exchange_rate": 1, + "approval_status": "Approved", "payable_account": frappe.db.get_value("Company", "_Test Company", "default_payable_account"), "vehicle_log": vehicle_log.name, "expenses": [ @@ -88,12 +89,16 @@ def test_cancel_vehicle_log_linked_to_draft_expense_claim(self): } ).insert() - self.assertRaises(frappe.ValidationError, vehicle_log.cancel) vehicle_log.reload() - self.assertEqual(vehicle_log.docstatus, 1) - - expense_claim.delete() vehicle_log.cancel() + expense_claim.reload() + + self.assertEqual(vehicle_log.docstatus, 2) + self.assertIsNone(expense_claim.vehicle_log) + + expense_claim.submit() + + expense_claim.cancel() frappe.delete_doc("Vehicle Log", vehicle_log.name) diff --git a/hrms/hr/doctype/vehicle_log/vehicle_log.py b/hrms/hr/doctype/vehicle_log/vehicle_log.py index 479f2528da..4c7cb311e1 100644 --- a/hrms/hr/doctype/vehicle_log/vehicle_log.py +++ b/hrms/hr/doctype/vehicle_log/vehicle_log.py @@ -47,20 +47,27 @@ def on_submit(self): frappe.db.set_value("Vehicle", self.license_plate, "last_odometer", self.odometer) def before_cancel(self): - expense_claim = frappe.db.exists( + submitted_expense_claim = frappe.db.exists( "Expense Claim", { "vehicle_log": self.name, - "docstatus": ["!=", 2], + "docstatus": 1, }, ) - if expense_claim: + if submitted_expense_claim: frappe.throw( - _("Cannot cancel Vehicle Log {0} because it is linked to Expense Claim {1}").format( - self.name, expense_claim + _("Cannot cancel Vehicle Log {0} because it is linked to submitted Expense Claim {1}").format( + self.name, submitted_expense_claim ) ) + for expense_claim in frappe.get_all( + "Expense Claim", + filters={"vehicle_log": self.name, "docstatus": 0}, + pluck="name", + ): + frappe.db.set_value("Expense Claim", expense_claim, "vehicle_log", None) + def on_cancel(self): distance_travelled = self.odometer - self.last_odometer if distance_travelled > 0: From 5f229848d811e11d338cb88f38b344c9ea332b8e Mon Sep 17 00:00:00 2001 From: Krishna Shirsath Date: Mon, 15 Jun 2026 11:41:16 +0530 Subject: [PATCH 03/10] fix(vehicle_log): enhance cancellation logic to handle linked submitted and draft Expense Claims --- .../doctype/vehicle_log/test_vehicle_log.py | 48 ++++++++++++++++++- hrms/hr/doctype/vehicle_log/vehicle_log.js | 33 +++++++++++++ hrms/hr/doctype/vehicle_log/vehicle_log.py | 36 +++++++------- 3 files changed, 97 insertions(+), 20 deletions(-) diff --git a/hrms/hr/doctype/vehicle_log/test_vehicle_log.py b/hrms/hr/doctype/vehicle_log/test_vehicle_log.py index a8a80dcaf7..861f642ce4 100644 --- a/hrms/hr/doctype/vehicle_log/test_vehicle_log.py +++ b/hrms/hr/doctype/vehicle_log/test_vehicle_log.py @@ -1,12 +1,15 @@ # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors # See license.txt +import json + import frappe +from frappe.desk.form.linked_with import cancel_all_linked_docs, get_submitted_linked_docs from frappe.utils import cstr, flt, nowdate, random_string from erpnext.setup.doctype.employee.test_employee import make_employee -from hrms.hr.doctype.vehicle_log.vehicle_log import make_expense_claim +from hrms.hr.doctype.vehicle_log.vehicle_log import get_draft_expense_claims, make_expense_claim from hrms.tests.utils import HRMSTestSuite @@ -89,18 +92,61 @@ def test_cancel_vehicle_log_unlinks_draft_expense_claim(self): } ).insert() + self.assertIn(expense_claim.name, get_draft_expense_claims(vehicle_log.name)) + vehicle_log.reload() vehicle_log.cancel() expense_claim.reload() self.assertEqual(vehicle_log.docstatus, 2) self.assertIsNone(expense_claim.vehicle_log) + self.assertNotIn(expense_claim.name, get_draft_expense_claims(vehicle_log.name)) expense_claim.submit() expense_claim.cancel() frappe.delete_doc("Vehicle Log", vehicle_log.name) + def test_cancel_vehicle_log_with_submitted_expense_claim_uses_linked_doc_cancellation(self): + vehicle_log = make_vehicle_log(self.license_plate, self.employee_id) + currency, cost_center = frappe.db.get_value( + "Company", "_Test Company", ["default_currency", "cost_center"] + ) + expense_claim = frappe.get_doc( + { + "doctype": "Expense Claim", + "employee": self.employee_id, + "company": "_Test Company", + "currency": currency, + "exchange_rate": 1, + "approval_status": "Approved", + "payable_account": frappe.db.get_value("Company", "_Test Company", "default_payable_account"), + "vehicle_log": vehicle_log.name, + "expenses": [ + { + "expense_type": "Travel", + "default_account": "Travel Expenses - _TC", + "currency": currency, + "amount": 100, + "sanctioned_amount": 100, + "cost_center": cost_center, + } + ], + } + ).insert() + expense_claim.submit() + + linked_docs = get_submitted_linked_docs("Vehicle Log", vehicle_log.name)["docs"] + self.assertIn({"doctype": "Expense Claim", "name": expense_claim.name, "docstatus": 1}, linked_docs) + + cancel_all_linked_docs(json.dumps(linked_docs)) + expense_claim.reload() + self.assertEqual(expense_claim.docstatus, 2) + + vehicle_log.reload() + vehicle_log.cancel() + self.assertEqual(vehicle_log.docstatus, 2) + def get_vehicle(employee_id): license_plate = random_string(10).upper() diff --git a/hrms/hr/doctype/vehicle_log/vehicle_log.js b/hrms/hr/doctype/vehicle_log/vehicle_log.js index 1a544594fd..c958f8178f 100644 --- a/hrms/hr/doctype/vehicle_log/vehicle_log.js +++ b/hrms/hr/doctype/vehicle_log/vehicle_log.js @@ -24,6 +24,39 @@ frappe.ui.form.on("Vehicle Log", { } }, + before_cancel: function (frm) { + return new Promise((resolve, reject) => { + frappe.call({ + method: "hrms.hr.doctype.vehicle_log.vehicle_log.get_draft_expense_claims", + args: { + vehicle_log: frm.doc.name, + }, + callback: function (r) { + const expense_claims = r.message || []; + if (!expense_claims.length) { + resolve(); + return; + } + + const expense_claim_links = expense_claims + .map((name) => frappe.utils.get_form_link("Expense Claim", name, true)) + .join(", "); + + frappe.confirm( + __( + "Draft Expense Claim {0} will be unlinked once Vehicle Log {1} is cancelled. Do you want to proceed?", + [expense_claim_links, frm.doc.name.bold()], + ), + () => resolve(), + () => reject(), + __("Yes"), + __("No"), + ); + }, + }); + }); + }, + expense_claim: function (frm) { frappe.call({ method: "hrms.hr.doctype.vehicle_log.vehicle_log.make_expense_claim", diff --git a/hrms/hr/doctype/vehicle_log/vehicle_log.py b/hrms/hr/doctype/vehicle_log/vehicle_log.py index 4c7cb311e1..359dc2655b 100644 --- a/hrms/hr/doctype/vehicle_log/vehicle_log.py +++ b/hrms/hr/doctype/vehicle_log/vehicle_log.py @@ -47,25 +47,7 @@ def on_submit(self): frappe.db.set_value("Vehicle", self.license_plate, "last_odometer", self.odometer) def before_cancel(self): - submitted_expense_claim = frappe.db.exists( - "Expense Claim", - { - "vehicle_log": self.name, - "docstatus": 1, - }, - ) - if submitted_expense_claim: - frappe.throw( - _("Cannot cancel Vehicle Log {0} because it is linked to submitted Expense Claim {1}").format( - self.name, submitted_expense_claim - ) - ) - - for expense_claim in frappe.get_all( - "Expense Claim", - filters={"vehicle_log": self.name, "docstatus": 0}, - pluck="name", - ): + for expense_claim in _get_draft_expense_claims(self.name): frappe.db.set_value("Expense Claim", expense_claim, "vehicle_log", None) def on_cancel(self): @@ -99,3 +81,19 @@ def make_expense_claim(docname: str) -> dict: {"expense_date": vehicle_log.date, "description": _("Vehicle Expenses"), "amount": claim_amount}, ) return exp_claim.as_dict() + + +@frappe.whitelist() +def get_draft_expense_claims(vehicle_log: str) -> list[str]: + frappe.has_permission("Vehicle Log", doc=vehicle_log, throw=True) + + return _get_draft_expense_claims(vehicle_log) + + +def _get_draft_expense_claims(vehicle_log: str) -> list[str]: + return frappe.get_all( + "Expense Claim", + filters={"vehicle_log": vehicle_log, "docstatus": 0}, + pluck="name", + order_by="creation asc", + ) From 3cf53cabd5e88efae6923851c51b190ed4a2ce83 Mon Sep 17 00:00:00 2001 From: Krishna Shirsath Date: Mon, 15 Jun 2026 16:43:45 +0530 Subject: [PATCH 04/10] fix(vehicle_log): improve cancellation logic to handle linked draft Expense Claims and update description handling --- .../hr/doctype/expense_claim/expense_claim.js | 10 ++ .../expense_claim_detail.json | 4 +- .../doctype/vehicle_log/test_vehicle_log.py | 118 +++++++++++++++++- hrms/hr/doctype/vehicle_log/vehicle_log.js | 41 ++++-- hrms/hr/doctype/vehicle_log/vehicle_log.py | 37 +++++- 5 files changed, 193 insertions(+), 17 deletions(-) diff --git a/hrms/hr/doctype/expense_claim/expense_claim.js b/hrms/hr/doctype/expense_claim/expense_claim.js index 99e4a4b482..f47fd406bc 100644 --- a/hrms/hr/doctype/expense_claim/expense_claim.js +++ b/hrms/hr/doctype/expense_claim/expense_claim.js @@ -519,6 +519,16 @@ frappe.ui.form.on("Expense Claim Detail", { if (!d.expense_type) { return; } + + // Fetch description only if empty + if (!d.description) { + frappe.db.get_value("Expense Claim Type", d.expense_type, "description").then((r) => { + if (r.message && r.message.description) { + frappe.model.set_value(cdt, cdn, "description", r.message.description); + } + }); + } + return frappe.call({ method: "hrms.hr.doctype.expense_claim.expense_claim.get_expense_claim_account_and_cost_center", args: { diff --git a/hrms/hr/doctype/expense_claim_detail/expense_claim_detail.json b/hrms/hr/doctype/expense_claim_detail/expense_claim_detail.json index 4faf15fe47..9078953f37 100644 --- a/hrms/hr/doctype/expense_claim_detail/expense_claim_detail.json +++ b/hrms/hr/doctype/expense_claim_detail/expense_claim_detail.json @@ -60,8 +60,6 @@ "read_only": 1 }, { - "fetch_from": "expense_type.description", - "fetch_if_empty": 1, "fieldname": "description", "fieldtype": "Text Editor", "in_list_view": 1, @@ -155,7 +153,7 @@ "idx": 1, "istable": 1, "links": [], - "modified": "2025-09-10 19:21:50.625260", + "modified": "2026-06-15 16:46:22.402968", "modified_by": "Administrator", "module": "HR", "name": "Expense Claim Detail", diff --git a/hrms/hr/doctype/vehicle_log/test_vehicle_log.py b/hrms/hr/doctype/vehicle_log/test_vehicle_log.py index 861f642ce4..792efa3108 100644 --- a/hrms/hr/doctype/vehicle_log/test_vehicle_log.py +++ b/hrms/hr/doctype/vehicle_log/test_vehicle_log.py @@ -9,7 +9,11 @@ from erpnext.setup.doctype.employee.test_employee import make_employee -from hrms.hr.doctype.vehicle_log.vehicle_log import get_draft_expense_claims, make_expense_claim +from hrms.hr.doctype.vehicle_log.vehicle_log import ( + get_draft_expense_claim_cancellation_actions, + get_draft_expense_claims, + make_expense_claim, +) from hrms.tests.utils import HRMSTestSuite @@ -50,7 +54,6 @@ def test_vehicle_log_fuel_expense(self): self.assertEqual(fuel_expense, 50 * 500) vehicle_log.cancel() - frappe.delete_doc("Expense Claim", expense_claim.name) frappe.delete_doc("Vehicle Log", vehicle_log.name) def test_vehicle_log_with_service_expenses(self): @@ -61,7 +64,6 @@ def test_vehicle_log_with_service_expenses(self): self.assertEqual(expenses, 27000) vehicle_log.cancel() - frappe.delete_doc("Expense Claim", expense_claim.name) frappe.delete_doc("Vehicle Log", vehicle_log.name) def test_cancel_vehicle_log_unlinks_draft_expense_claim(self): @@ -93,6 +95,10 @@ def test_cancel_vehicle_log_unlinks_draft_expense_claim(self): ).insert() self.assertIn(expense_claim.name, get_draft_expense_claims(vehicle_log.name)) + self.assertEqual( + get_draft_expense_claim_cancellation_actions(vehicle_log.name), + [{"name": expense_claim.name, "action": "unlink"}], + ) vehicle_log.reload() vehicle_log.cancel() @@ -101,12 +107,118 @@ def test_cancel_vehicle_log_unlinks_draft_expense_claim(self): self.assertEqual(vehicle_log.docstatus, 2) self.assertIsNone(expense_claim.vehicle_log) self.assertNotIn(expense_claim.name, get_draft_expense_claims(vehicle_log.name)) + # non-Vehicle-Log expense rows remain intact + self.assertEqual(len(expense_claim.expenses), 1) + self.assertEqual(expense_claim.expenses[0].expense_type, "Travel") expense_claim.submit() expense_claim.cancel() frappe.delete_doc("Vehicle Log", vehicle_log.name) + def test_cancel_vehicle_log_deletes_claim_with_only_vehicle_log_expenses(self): + vehicle_log = make_vehicle_log(self.license_plate, self.employee_id) + currency, cost_center = frappe.db.get_value( + "Company", "_Test Company", ["default_currency", "cost_center"] + ) + + expense_claim = frappe.get_doc( + { + "doctype": "Expense Claim", + "employee": self.employee_id, + "company": "_Test Company", + "currency": currency, + "exchange_rate": 1, + "approval_status": "Approved", + "payable_account": frappe.db.get_value("Company", "_Test Company", "default_payable_account"), + "vehicle_log": vehicle_log.name, + "expenses": [ + { + "expense_date": nowdate(), + "expense_type": "Travel", + "default_account": "Travel Expenses - _TC", + "currency": currency, + "description": "Vehicle Expenses", + "amount": 25000, + "sanctioned_amount": 25000, + "cost_center": cost_center, + } + ], + } + ).insert() + + self.assertIn(expense_claim.name, get_draft_expense_claims(vehicle_log.name)) + self.assertEqual( + get_draft_expense_claim_cancellation_actions(vehicle_log.name), + [{"name": expense_claim.name, "action": "delete"}], + ) + + vehicle_log.reload() + vehicle_log.cancel() + + self.assertFalse(frappe.db.exists("Expense Claim", expense_claim.name)) + frappe.delete_doc("Vehicle Log", vehicle_log.name) + + def test_cancel_vehicle_log_removes_only_vehicle_log_rows_from_mixed_claim(self): + vehicle_log = make_vehicle_log(self.license_plate, self.employee_id) + currency, cost_center = frappe.db.get_value( + "Company", "_Test Company", ["default_currency", "cost_center"] + ) + + expense_claim = frappe.get_doc( + { + "doctype": "Expense Claim", + "employee": self.employee_id, + "company": "_Test Company", + "currency": currency, + "exchange_rate": 1, + "approval_status": "Approved", + "payable_account": frappe.db.get_value("Company", "_Test Company", "default_payable_account"), + "vehicle_log": vehicle_log.name, + "expenses": [ + { + "expense_date": nowdate(), + "expense_type": "Travel", + "default_account": "Travel Expenses - _TC", + "currency": currency, + "description": "Vehicle Expenses", + "amount": 25000, + "sanctioned_amount": 25000, + "cost_center": cost_center, + }, + { + "expense_date": nowdate(), + "expense_type": "Travel", + "default_account": "Travel Expenses - _TC", + "currency": currency, + "description": "Accident Repair", + "amount": 5000, + "sanctioned_amount": 5000, + "cost_center": cost_center, + }, + ], + } + ).insert() + + self.assertIn(expense_claim.name, get_draft_expense_claims(vehicle_log.name)) + self.assertEqual(len(expense_claim.expenses), 2) + self.assertEqual( + get_draft_expense_claim_cancellation_actions(vehicle_log.name), + [{"name": expense_claim.name, "action": "unlink"}], + ) + + vehicle_log.reload() + vehicle_log.cancel() + + expense_claim.reload() + self.assertEqual(len(expense_claim.expenses), 1) + self.assertEqual(expense_claim.expenses[0].description, "Accident Repair") + self.assertIsNone(expense_claim.vehicle_log) + + expense_claim.submit() + expense_claim.cancel() + frappe.delete_doc("Vehicle Log", vehicle_log.name) + def test_cancel_vehicle_log_with_submitted_expense_claim_uses_linked_doc_cancellation(self): vehicle_log = make_vehicle_log(self.license_plate, self.employee_id) currency, cost_center = frappe.db.get_value( diff --git a/hrms/hr/doctype/vehicle_log/vehicle_log.js b/hrms/hr/doctype/vehicle_log/vehicle_log.js index c958f8178f..e2792ff15a 100644 --- a/hrms/hr/doctype/vehicle_log/vehicle_log.js +++ b/hrms/hr/doctype/vehicle_log/vehicle_log.js @@ -27,7 +27,7 @@ frappe.ui.form.on("Vehicle Log", { before_cancel: function (frm) { return new Promise((resolve, reject) => { frappe.call({ - method: "hrms.hr.doctype.vehicle_log.vehicle_log.get_draft_expense_claims", + method: "hrms.hr.doctype.vehicle_log.vehicle_log.get_draft_expense_claim_cancellation_actions", args: { vehicle_log: frm.doc.name, }, @@ -38,15 +38,8 @@ frappe.ui.form.on("Vehicle Log", { return; } - const expense_claim_links = expense_claims - .map((name) => frappe.utils.get_form_link("Expense Claim", name, true)) - .join(", "); - frappe.confirm( - __( - "Draft Expense Claim {0} will be unlinked once Vehicle Log {1} is cancelled. Do you want to proceed?", - [expense_claim_links, frm.doc.name.bold()], - ), + get_expense_claim_cancellation_message(expense_claims, frm.doc.name), () => resolve(), () => reject(), __("Yes"), @@ -70,3 +63,33 @@ frappe.ui.form.on("Vehicle Log", { }); }, }); + +function get_expense_claim_cancellation_message(expense_claims, vehicle_log) { + const message_parts = []; + const claims_to_delete = get_expense_claim_links(expense_claims, "delete"); + const claims_to_unlink = get_expense_claim_links(expense_claims, "unlink"); + + if (claims_to_delete) { + message_parts.push(__("Will be deleted: {0}", [claims_to_delete])); + } + + if (claims_to_unlink) { + message_parts.push( + __("Will be unlinked, and Vehicle Expenses will be removed: {0}", [claims_to_unlink]), + ); + } + + return __( + "Cancelling Vehicle Log {0} will affect linked draft Expense Claims:

{1}

Do you want to proceed?", + [vehicle_log.bold(), message_parts.join("
")], + ); +} + +function get_expense_claim_links(expense_claims, action) { + return expense_claims + .filter((expense_claim) => expense_claim.action === action) + .map((expense_claim) => + frappe.utils.get_form_link("Expense Claim", expense_claim.name, true), + ) + .join(", "); +} diff --git a/hrms/hr/doctype/vehicle_log/vehicle_log.py b/hrms/hr/doctype/vehicle_log/vehicle_log.py index 359dc2655b..05f2c413ec 100644 --- a/hrms/hr/doctype/vehicle_log/vehicle_log.py +++ b/hrms/hr/doctype/vehicle_log/vehicle_log.py @@ -47,8 +47,16 @@ def on_submit(self): frappe.db.set_value("Vehicle", self.license_plate, "last_odometer", self.odometer) def before_cancel(self): - for expense_claim in _get_draft_expense_claims(self.name): - frappe.db.set_value("Expense Claim", expense_claim, "vehicle_log", None) + for expense_claim_name in _get_draft_expense_claims(self.name): + expense_claim = frappe.get_doc("Expense Claim", expense_claim_name) + + remaining_expenses = _get_non_vehicle_log_expenses(expense_claim) + if remaining_expenses: + expense_claim.expenses = remaining_expenses + expense_claim.vehicle_log = None + expense_claim.save() + else: + expense_claim.delete() def on_cancel(self): distance_travelled = self.odometer - self.last_odometer @@ -90,6 +98,19 @@ def get_draft_expense_claims(vehicle_log: str) -> list[str]: return _get_draft_expense_claims(vehicle_log) +@frappe.whitelist() +def get_draft_expense_claim_cancellation_actions(vehicle_log: str) -> list[dict[str, str]]: + frappe.has_permission("Vehicle Log", doc=vehicle_log, throw=True) + + return [ + { + "name": expense_claim.name, + "action": "unlink" if _get_non_vehicle_log_expenses(expense_claim) else "delete", + } + for expense_claim in _get_draft_expense_claim_docs(vehicle_log) + ] + + def _get_draft_expense_claims(vehicle_log: str) -> list[str]: return frappe.get_all( "Expense Claim", @@ -97,3 +118,15 @@ def _get_draft_expense_claims(vehicle_log: str) -> list[str]: pluck="name", order_by="creation asc", ) + + +def _get_draft_expense_claim_docs(vehicle_log: str) -> list[Document]: + return [ + frappe.get_doc("Expense Claim", expense_claim_name) + for expense_claim_name in _get_draft_expense_claims(vehicle_log) + ] + + +def _get_non_vehicle_log_expenses(expense_claim: Document) -> list[Document]: + vehicle_log_description = _("Vehicle Expenses") + return [row for row in expense_claim.expenses if row.description != vehicle_log_description] From f3317d3d54524fa86d34967b3e6f04afe4d1c890 Mon Sep 17 00:00:00 2001 From: Krishna Shirsath Date: Fri, 19 Jun 2026 22:23:05 +0530 Subject: [PATCH 05/10] fix(vehicle_log): handle error in before_cancel promise rejection --- hrms/hr/doctype/vehicle_log/vehicle_log.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hrms/hr/doctype/vehicle_log/vehicle_log.js b/hrms/hr/doctype/vehicle_log/vehicle_log.js index e2792ff15a..bf689d4382 100644 --- a/hrms/hr/doctype/vehicle_log/vehicle_log.js +++ b/hrms/hr/doctype/vehicle_log/vehicle_log.js @@ -46,6 +46,9 @@ frappe.ui.form.on("Vehicle Log", { __("No"), ); }, + error: function (err) { + reject(err); + }, }); }); }, From bed999f997fa78342b941c3a42816c8ee99dccee Mon Sep 17 00:00:00 2001 From: Krishna Shirsath Date: Mon, 22 Jun 2026 17:49:05 +0530 Subject: [PATCH 06/10] refactor(vehicle_log): remove rows with the Vehicle Expenses description and cancellation message changed --- hrms/hr/doctype/vehicle_log/vehicle_log.js | 33 ++++++++-------------- hrms/hr/doctype/vehicle_log/vehicle_log.py | 14 +++++---- 2 files changed, 20 insertions(+), 27 deletions(-) diff --git a/hrms/hr/doctype/vehicle_log/vehicle_log.js b/hrms/hr/doctype/vehicle_log/vehicle_log.js index bf689d4382..3c64ba4f7c 100644 --- a/hrms/hr/doctype/vehicle_log/vehicle_log.js +++ b/hrms/hr/doctype/vehicle_log/vehicle_log.js @@ -68,31 +68,22 @@ frappe.ui.form.on("Vehicle Log", { }); function get_expense_claim_cancellation_message(expense_claims, vehicle_log) { - const message_parts = []; - const claims_to_delete = get_expense_claim_links(expense_claims, "delete"); - const claims_to_unlink = get_expense_claim_links(expense_claims, "unlink"); - - if (claims_to_delete) { - message_parts.push(__("Will be deleted: {0}", [claims_to_delete])); - } + const expense_claim = expense_claims[0]; + const expense_claim_link = frappe.utils.get_form_link( + "Expense Claim", + expense_claim.name, + true, + ); - if (claims_to_unlink) { - message_parts.push( - __("Will be unlinked, and Vehicle Expenses will be removed: {0}", [claims_to_unlink]), + if (expense_claim.action === "delete") { + return __( + "Cancelling Vehicle Log {0} will delete draft Expense Claim {1} because it only contains Vehicle Expenses.

Do you want to continue?", + [vehicle_log.bold(), expense_claim_link], ); } return __( - "Cancelling Vehicle Log {0} will affect linked draft Expense Claims:

{1}

Do you want to proceed?", - [vehicle_log.bold(), message_parts.join("
")], + "Cancelling Vehicle Log {0} will update draft Expense Claim {1}.

Vehicle Expenses rows will be removed. Other expenses will remain.

Do you want to continue?", + [vehicle_log.bold(), expense_claim_link], ); } - -function get_expense_claim_links(expense_claims, action) { - return expense_claims - .filter((expense_claim) => expense_claim.action === action) - .map((expense_claim) => - frappe.utils.get_form_link("Expense Claim", expense_claim.name, true), - ) - .join(", "); -} diff --git a/hrms/hr/doctype/vehicle_log/vehicle_log.py b/hrms/hr/doctype/vehicle_log/vehicle_log.py index 05f2c413ec..fb3c353a8b 100644 --- a/hrms/hr/doctype/vehicle_log/vehicle_log.py +++ b/hrms/hr/doctype/vehicle_log/vehicle_log.py @@ -50,9 +50,11 @@ def before_cancel(self): for expense_claim_name in _get_draft_expense_claims(self.name): expense_claim = frappe.get_doc("Expense Claim", expense_claim_name) - remaining_expenses = _get_non_vehicle_log_expenses(expense_claim) - if remaining_expenses: - expense_claim.expenses = remaining_expenses + if _has_non_vehicle_log_expenses(expense_claim): + for row in list(expense_claim.expenses): + if row.description == _("Vehicle Expenses"): + expense_claim.remove(row) + expense_claim.vehicle_log = None expense_claim.save() else: @@ -105,7 +107,7 @@ def get_draft_expense_claim_cancellation_actions(vehicle_log: str) -> list[dict[ return [ { "name": expense_claim.name, - "action": "unlink" if _get_non_vehicle_log_expenses(expense_claim) else "delete", + "action": "unlink" if _has_non_vehicle_log_expenses(expense_claim) else "delete", } for expense_claim in _get_draft_expense_claim_docs(vehicle_log) ] @@ -127,6 +129,6 @@ def _get_draft_expense_claim_docs(vehicle_log: str) -> list[Document]: ] -def _get_non_vehicle_log_expenses(expense_claim: Document) -> list[Document]: +def _has_non_vehicle_log_expenses(expense_claim: Document) -> bool: vehicle_log_description = _("Vehicle Expenses") - return [row for row in expense_claim.expenses if row.description != vehicle_log_description] + return any(row.description != vehicle_log_description for row in expense_claim.expenses) From 94ed07a0532ff072d84025387e7f25e25826b9be Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Wed, 24 Jun 2026 13:38:50 +0530 Subject: [PATCH 07/10] fix: permission check for whitelisted methods --- hrms/api/__init__.py | 6 ++++++ hrms/api/roster.py | 1 + hrms/controllers/employee_boarding_controller.py | 1 + hrms/hr/doctype/appraisal/appraisal.py | 1 + hrms/hr/doctype/appraisal_cycle/appraisal_cycle.py | 4 ++++ hrms/hr/doctype/attendance/attendance.py | 1 + .../employee_attendance_tool.py | 1 + .../hr/doctype/employee_checkin/employee_checkin.py | 6 ++++++ .../employee_onboarding/employee_onboarding.py | 1 + hrms/hr/doctype/expense_claim/expense_claim.py | 2 ++ .../full_and_final_statement.py | 2 ++ hrms/hr/doctype/interview/interview.py | 3 +++ .../interview_feedback/interview_feedback.py | 1 + hrms/hr/doctype/job_applicant/job_applicant.py | 1 + hrms/hr/doctype/job_requisition/job_requisition.py | 1 + .../hr/doctype/leave_adjustment/leave_adjustment.py | 4 ++-- .../doctype/leave_application/leave_application.py | 13 ++++++++++--- .../leave_ledger_entry/leave_ledger_entry.py | 2 ++ hrms/hr/doctype/staffing_plan/staffing_plan.py | 2 ++ hrms/hr/doctype/training_result/training_result.py | 1 + hrms/hr/doctype/vehicle_log/vehicle_log.py | 2 ++ .../organizational_chart/organizational_chart.py | 2 +- hrms/hr/page/team_updates/team_updates.py | 2 +- hrms/overrides/employee_payment_entry.py | 4 ++++ .../employee_benefit_claim.py | 2 ++ hrms/payroll/doctype/payroll_entry/payroll_entry.py | 4 ++++ .../doctype/salary_component/salary_component.py | 1 + hrms/payroll/doctype/salary_slip/salary_slip.py | 4 ++++ .../doctype/salary_structure/salary_structure.py | 3 +++ .../salary_structure_assignment.py | 1 + .../salary_withholding/salary_withholding.py | 1 + 31 files changed, 73 insertions(+), 7 deletions(-) diff --git a/hrms/api/__init__.py b/hrms/api/__init__.py index 6ea7f1bf70..d2c1773153 100644 --- a/hrms/api/__init__.py +++ b/hrms/api/__init__.py @@ -701,6 +701,7 @@ def get_currency_symbols() -> dict: @frappe.whitelist() def get_company_cost_center_and_expense_account(company: str) -> dict: + frappe.has_permission("Company", "read", company, throw=True) return frappe.db.get_value( "Company", company, ["cost_center", "default_expense_claim_payable_account"], as_dict=True ) @@ -779,6 +780,11 @@ def upload_base64_file( @frappe.whitelist() def delete_attachment(filename: str): + attached_to_doctype, attached_to_name = frappe.db.get_value( + "File", filename, ["attached_to_doctype", "attached_to_name"] + ) + if attached_to_doctype and attached_to_name: + frappe.has_permission(attached_to_doctype, "write", attached_to_name, throw=True) frappe.delete_doc("File", filename) diff --git a/hrms/api/roster.py b/hrms/api/roster.py index 6a2c249d03..3b3f40e978 100644 --- a/hrms/api/roster.py +++ b/hrms/api/roster.py @@ -69,6 +69,7 @@ def get_events( @frappe.whitelist() def get_schedule_from_assignment(shift_schedule_assignment: str): + frappe.has_permission("Shift Schedule Assignment", "read", shift_schedule_assignment, throw=True) shift_schedule = frappe.db.get_value( "Shift Schedule Assignment", shift_schedule_assignment, "shift_schedule" ) diff --git a/hrms/controllers/employee_boarding_controller.py b/hrms/controllers/employee_boarding_controller.py index bb99c762dd..26b777eef5 100644 --- a/hrms/controllers/employee_boarding_controller.py +++ b/hrms/controllers/employee_boarding_controller.py @@ -157,6 +157,7 @@ def on_cancel(self): @frappe.whitelist() def get_onboarding_details(parent: str, parenttype: str): + frappe.has_permission(parenttype, "read", parent, throw=True) return frappe.get_all( "Employee Boarding Activity", fields=[ diff --git a/hrms/hr/doctype/appraisal/appraisal.py b/hrms/hr/doctype/appraisal/appraisal.py index 41f0c5fb69..d252191807 100644 --- a/hrms/hr/doctype/appraisal/appraisal.py +++ b/hrms/hr/doctype/appraisal/appraisal.py @@ -307,6 +307,7 @@ def set_goal_score(self, update=False): @frappe.whitelist() def get_feedback_history(employee: str, appraisal: str) -> dict: + frappe.has_permission("Appraisal", "read", appraisal, throw=True) data = frappe._dict() data.feedback_history = frappe.get_list( "Employee Performance Feedback", diff --git a/hrms/hr/doctype/appraisal_cycle/appraisal_cycle.py b/hrms/hr/doctype/appraisal_cycle/appraisal_cycle.py index 5b1fe17a39..7ea41a2011 100644 --- a/hrms/hr/doctype/appraisal_cycle/appraisal_cycle.py +++ b/hrms/hr/doctype/appraisal_cycle/appraisal_cycle.py @@ -62,6 +62,7 @@ def check_if_appraisals_exist(self): @frappe.whitelist() def set_employees(self): """Pull employees in appraisee list based on selected filters""" + self.check_permission("write") employees = self.get_employees_for_appraisal() appraisal_templates = self.get_appraisal_template_map() @@ -234,6 +235,7 @@ def validate_active_appraisal_cycle(appraisal_cycle: str) -> None: @frappe.whitelist() def get_appraisal_cycle_summary(cycle_name: str) -> dict: + frappe.has_permission("Appraisal Cycle", "read", cycle_name, throw=True) summary = frappe._dict() summary["appraisees"] = frappe.db.count( @@ -283,6 +285,8 @@ def get_employees_without_feedback(cycle_name: str | None = None) -> int: "Appraisal Cycle", {"status": "In Progress"}, order_by="start_date desc" ) + frappe.has_permission("Appraisal Cycle", "read", cycle_name, throw=True) + filtered_records = SubQuery( frappe.qb.from_(Feedback) .select(Feedback.employee) diff --git a/hrms/hr/doctype/attendance/attendance.py b/hrms/hr/doctype/attendance/attendance.py index 8b198e6b81..25d90bc694 100644 --- a/hrms/hr/doctype/attendance/attendance.py +++ b/hrms/hr/doctype/attendance/attendance.py @@ -432,6 +432,7 @@ def process_bulk_attendance_in_batches(data, chunk_size=20): def get_unmarked_days( employee: str, from_date: str | date, to_date: str | date, exclude_holidays: str | int = 0 ) -> list: + frappe.has_permission("Employee", "read", employee, throw=True) joining_date, relieving_date = frappe.get_cached_value( "Employee", employee, ["date_of_joining", "relieving_date"] ) diff --git a/hrms/hr/doctype/employee_attendance_tool/employee_attendance_tool.py b/hrms/hr/doctype/employee_attendance_tool/employee_attendance_tool.py index e930a2f3ea..239670fecb 100644 --- a/hrms/hr/doctype/employee_attendance_tool/employee_attendance_tool.py +++ b/hrms/hr/doctype/employee_attendance_tool/employee_attendance_tool.py @@ -203,6 +203,7 @@ def mark_employee_attendance( attendance.insert() attendance.submit() if mark_half_day: + frappe.has_permission("Attendance", "write", throw=True) if isinstance(half_day_employee_list, str): half_day_employee_list = json.loads(half_day_employee_list) Attendance = frappe.qb.DocType("Attendance") diff --git a/hrms/hr/doctype/employee_checkin/employee_checkin.py b/hrms/hr/doctype/employee_checkin/employee_checkin.py index 3bbb592558..0fd63c30e5 100644 --- a/hrms/hr/doctype/employee_checkin/employee_checkin.py +++ b/hrms/hr/doctype/employee_checkin/employee_checkin.py @@ -182,6 +182,12 @@ def add_log_based_on_employee_field( if not employee_field_value or not timestamp: frappe.throw(_("'employee_field_value' and 'timestamp' are required.")) + allowed_employee_fieldnames = {"name", "employee", "attendance_device_id"} + if employee_fieldname not in allowed_employee_fieldnames: + frappe.throw( + _("'employee_fieldname' must be one of {0}.").format(", ".join(allowed_employee_fieldnames)) + ) + employee = frappe.db.get_values( "Employee", {employee_fieldname: employee_field_value}, diff --git a/hrms/hr/doctype/employee_onboarding/employee_onboarding.py b/hrms/hr/doctype/employee_onboarding/employee_onboarding.py index 289a98ca3e..caf431f80d 100644 --- a/hrms/hr/doctype/employee_onboarding/employee_onboarding.py +++ b/hrms/hr/doctype/employee_onboarding/employee_onboarding.py @@ -92,6 +92,7 @@ def on_cancel(self): @frappe.whitelist() def mark_onboarding_as_completed(self): + self.check_permission("write") for activity in self.activities: frappe.db.set_value("Task", activity.task, "status", "Completed") frappe.db.set_value("Project", self.project, "status", "Completed") diff --git a/hrms/hr/doctype/expense_claim/expense_claim.py b/hrms/hr/doctype/expense_claim/expense_claim.py index a5e749ed1d..16472343a6 100644 --- a/hrms/hr/doctype/expense_claim/expense_claim.py +++ b/hrms/hr/doctype/expense_claim/expense_claim.py @@ -677,6 +677,7 @@ def get_advances(expense_claim: str | dict | Document, advance_id: str | None = if isinstance(expense_claim, str): expense_claim = frappe._dict(json.loads(expense_claim)) expense_claim_doc = frappe.get_doc(expense_claim) + frappe.has_permission("Employee", "read", expense_claim_doc.employee, throw=True) expense_claim_doc.advances = [] advance = frappe.qb.DocType("Employee Advance") @@ -714,6 +715,7 @@ def get_advances(expense_claim: str | dict | Document, advance_id: str | None = @frappe.whitelist() def get_expense_claim(employee_advance: str | dict) -> Document: + frappe.has_permission("Employee Advance", "read", employee_advance, throw=True) if isinstance(employee_advance, str): employee_advance = frappe.get_doc("Employee Advance", employee_advance) diff --git a/hrms/hr/doctype/full_and_final_statement/full_and_final_statement.py b/hrms/hr/doctype/full_and_final_statement/full_and_final_statement.py index e7f3ca82c6..b5a297e2bb 100644 --- a/hrms/hr/doctype/full_and_final_statement/full_and_final_statement.py +++ b/hrms/hr/doctype/full_and_final_statement/full_and_final_statement.py @@ -310,6 +310,8 @@ def get_account_and_amount(ref_doctype: str, ref_document: str, company: str) -> if not ref_doctype or not ref_document: return None + frappe.has_permission(ref_doctype, "read", ref_document, throw=True) + if ref_doctype == "Salary Slip": salary_details = frappe.db.get_value( "Salary Slip", ref_document, ["payroll_entry", "net_pay"], as_dict=1 diff --git a/hrms/hr/doctype/interview/interview.py b/hrms/hr/doctype/interview/interview.py index 828f5d0fae..82565a2cff 100644 --- a/hrms/hr/doctype/interview/interview.py +++ b/hrms/hr/doctype/interview/interview.py @@ -156,6 +156,7 @@ def on_discard(self): @frappe.whitelist() def get_interviewers(interview_type: str) -> list[dict]: + frappe.has_permission("Interview Type", "read", interview_type, throw=True) return frappe.get_all("Interviewer", filters={"parent": interview_type}, fields=["user as interviewer"]) @@ -202,6 +203,7 @@ def get_feedback(interview: str) -> list[dict]: @frappe.whitelist() def get_skill_wise_average_rating(interview: str) -> list[dict]: + frappe.has_permission("Interview", "read", interview, throw=True) skill_assessment = frappe.qb.DocType("Skill Assessment") interview_feedback = frappe.qb.DocType("Interview Feedback") return ( @@ -340,6 +342,7 @@ def send_daily_feedback_reminder(): @frappe.whitelist() def get_expected_skill_set(interview_type: str): + frappe.has_permission("Interview Type", "read", interview_type, throw=True) return frappe.get_all( "Expected Skill Set", filters={"parent": interview_type}, fields=["skill"], order_by="idx" ) diff --git a/hrms/hr/doctype/interview_feedback/interview_feedback.py b/hrms/hr/doctype/interview_feedback/interview_feedback.py index 31220e18ee..7437350e52 100644 --- a/hrms/hr/doctype/interview_feedback/interview_feedback.py +++ b/hrms/hr/doctype/interview_feedback/interview_feedback.py @@ -102,4 +102,5 @@ def update_interview_average_rating(self): @frappe.whitelist() def get_applicable_interviewers(interview: str) -> list[str]: + frappe.has_permission("Interview", "read", interview, throw=True) return frappe.get_all("Interview Detail", filters={"parent": interview}, pluck="interviewer") diff --git a/hrms/hr/doctype/job_applicant/job_applicant.py b/hrms/hr/doctype/job_applicant/job_applicant.py index 01b1139ebd..33dc1299b1 100644 --- a/hrms/hr/doctype/job_applicant/job_applicant.py +++ b/hrms/hr/doctype/job_applicant/job_applicant.py @@ -114,6 +114,7 @@ def create_interview(job_applicant: str, interview_type: str) -> Document: @frappe.whitelist() def get_interview_details(job_applicant: str) -> dict: + frappe.has_permission("Job Applicant", "read", job_applicant, throw=True) interview_details = frappe.db.get_all( "Interview", filters={"job_applicant": job_applicant, "docstatus": ["!=", 2]}, diff --git a/hrms/hr/doctype/job_requisition/job_requisition.py b/hrms/hr/doctype/job_requisition/job_requisition.py index 4f9536da23..436f21f296 100644 --- a/hrms/hr/doctype/job_requisition/job_requisition.py +++ b/hrms/hr/doctype/job_requisition/job_requisition.py @@ -58,6 +58,7 @@ def check_duplicate_job_requisition(self): @frappe.whitelist() def associate_job_opening(self, job_opening: str) -> None: + frappe.has_permission("Job Opening", "write", job_opening, throw=True) frappe.db.set_value( "Job Opening", job_opening, {"job_requisition": self.name, "vacancies": self.no_of_positions} ) diff --git a/hrms/hr/doctype/leave_adjustment/leave_adjustment.py b/hrms/hr/doctype/leave_adjustment/leave_adjustment.py index e2a46dfddd..3585e1b40d 100644 --- a/hrms/hr/doctype/leave_adjustment/leave_adjustment.py +++ b/hrms/hr/doctype/leave_adjustment/leave_adjustment.py @@ -130,7 +130,7 @@ def get_leave_allocation_for_posting_date( """ Returns the leave allocation for the given employee, leave type and posting date. """ - return frappe.get_all( + return frappe.get_list( "Leave Allocation", { "employee": employee, @@ -151,7 +151,7 @@ def get_allocated_leave_types( """ Returns the leave types allocated to the given employee """ - return frappe.get_all( + return frappe.get_list( "Leave Allocation", { "employee": filters.get("employee"), diff --git a/hrms/hr/doctype/leave_application/leave_application.py b/hrms/hr/doctype/leave_application/leave_application.py index f8c3183383..c1ec44369a 100755 --- a/hrms/hr/doctype/leave_application/leave_application.py +++ b/hrms/hr/doctype/leave_application/leave_application.py @@ -968,6 +968,7 @@ def get_number_of_leave_days( ) -> float: """Returns number of leave days between 2 dates after considering half day and holidays (Based on the include_holiday setting in Leave Type)""" + validate_leave_access(employee) number_of_days = date_diff(to_date, from_date) + 1 if cint(half_day) == 1: @@ -1020,7 +1021,7 @@ def get_leave_details(employee: str, date: str | datetime.date, for_salary_slip: return { "leave_allocation": leave_allocation, - "leave_approver": get_leave_approver(employee), + "leave_approver": get_employee_leave_approver(employee), "lwps": lwp, } @@ -1338,6 +1339,7 @@ def get_leave_entries(employee, leave_type, from_date, to_date): @frappe.whitelist() def get_holidays(employee: str, from_date: str | datetime.date, to_date: str | datetime.date) -> int: """get holidays between two dates for the given employee""" + validate_leave_access(employee) holidays = get_holiday_dates_between_range(employee, from_date, to_date) return len(holidays) @@ -1526,6 +1528,11 @@ def get_approved_leaves_for_period(employee, leave_type, from_date, to_date): @frappe.whitelist() def get_leave_approver(employee: str) -> str: + validate_leave_access(employee) + return get_employee_leave_approver(employee) + + +def get_employee_leave_approver(employee: str) -> str: leave_approver, department = frappe.db.get_value("Employee", employee, ["leave_approver", "department"]) if not leave_approver and department: @@ -1549,13 +1556,13 @@ def get_leave_approver_and_mandatory(employee: str) -> dict: return { "is_mandatory": 1 if mandatory else 0, - "leave_approver": get_leave_approver(employee), + "leave_approver": get_employee_leave_approver(employee), } def validate_leave_access(employee): employee_user = frappe.db.get_value("Employee", employee, "user_id") - leave_approver = get_leave_approver(employee) + leave_approver = get_employee_leave_approver(employee) if frappe.session.user not in (employee_user, leave_approver) and ( not frappe.has_permission("Employee", "read", employee) diff --git a/hrms/hr/doctype/leave_ledger_entry/leave_ledger_entry.py b/hrms/hr/doctype/leave_ledger_entry/leave_ledger_entry.py index a269b071d3..08a476fba1 100644 --- a/hrms/hr/doctype/leave_ledger_entry/leave_ledger_entry.py +++ b/hrms/hr/doctype/leave_ledger_entry/leave_ledger_entry.py @@ -228,6 +228,8 @@ def expire_allocation(allocation: str | Document | frappe._dict, expiry_date: da allocation = json.loads(allocation) allocation = frappe.get_doc("Leave Allocation", allocation["name"]) + frappe.has_permission("Leave Allocation", "write", allocation.name, throw=True) + leaves = get_remaining_leaves(allocation) expiry_date = expiry_date if expiry_date else allocation.to_date diff --git a/hrms/hr/doctype/staffing_plan/staffing_plan.py b/hrms/hr/doctype/staffing_plan/staffing_plan.py index ceb337b96b..d7bc25edc9 100644 --- a/hrms/hr/doctype/staffing_plan/staffing_plan.py +++ b/hrms/hr/doctype/staffing_plan/staffing_plan.py @@ -242,6 +242,8 @@ def get_active_staffing_plan_details( from_date: str | datetime.date | None = None, to_date: str | datetime.date | None = None, ) -> list[dict] | None: + frappe.has_permission("Staffing Plan", "read", throw=True) + if from_date is None: from_date = getdate(nowdate()) if to_date is None: diff --git a/hrms/hr/doctype/training_result/training_result.py b/hrms/hr/doctype/training_result/training_result.py index b4685285ed..06a25ceb9a 100644 --- a/hrms/hr/doctype/training_result/training_result.py +++ b/hrms/hr/doctype/training_result/training_result.py @@ -47,4 +47,5 @@ def on_submit(self): @frappe.whitelist() def get_employees(training_event: str): + frappe.has_permission("Training Event", "read", training_event, throw=True) return frappe.get_doc("Training Event", training_event).employees diff --git a/hrms/hr/doctype/vehicle_log/vehicle_log.py b/hrms/hr/doctype/vehicle_log/vehicle_log.py index 7b3ae5c6f7..c13163ac70 100644 --- a/hrms/hr/doctype/vehicle_log/vehicle_log.py +++ b/hrms/hr/doctype/vehicle_log/vehicle_log.py @@ -57,6 +57,8 @@ def on_cancel(self): @frappe.whitelist() def make_expense_claim(docname: str) -> dict: + frappe.has_permission("Vehicle Log", "read", docname, throw=True) + expense_claim = frappe.db.exists("Expense Claim", {"vehicle_log": docname}) if expense_claim: frappe.throw(_("Expense Claim {0} already exists for the Vehicle Log").format(expense_claim)) diff --git a/hrms/hr/page/organizational_chart/organizational_chart.py b/hrms/hr/page/organizational_chart/organizational_chart.py index 28cabcd29a..9eb7364476 100644 --- a/hrms/hr/page/organizational_chart/organizational_chart.py +++ b/hrms/hr/page/organizational_chart/organizational_chart.py @@ -16,7 +16,7 @@ def get_children(parent: str | None = None, company: str | None = None, exclude_ if exclude_node: filters.append(["name", "!=", exclude_node]) - employees = frappe.get_all( + employees = frappe.get_list( "Employee", fields=[ "employee_name as name", diff --git a/hrms/hr/page/team_updates/team_updates.py b/hrms/hr/page/team_updates/team_updates.py index 20d7153101..ae29b0f1dc 100644 --- a/hrms/hr/page/team_updates/team_updates.py +++ b/hrms/hr/page/team_updates/team_updates.py @@ -5,7 +5,7 @@ @frappe.whitelist() def get_data(start: int = 0): - # frappe.only_for('Employee', 'System Manager') + frappe.only_for("Employee", "System Manager") data = frappe.get_all( "Communication", fields=("content", "text_content", "sender", "creation"), diff --git a/hrms/overrides/employee_payment_entry.py b/hrms/overrides/employee_payment_entry.py index 87d21d3ba0..d22cb2069a 100644 --- a/hrms/overrides/employee_payment_entry.py +++ b/hrms/overrides/employee_payment_entry.py @@ -81,6 +81,7 @@ def get_payment_entry_for_employee( bank_amount: float | None = None, ): """Function to make Payment Entry for Employee Advance, Gratuity, Expense Claim, Leave Encashment""" + frappe.has_permission(dt, "read", dn, throw=True) doc = frappe.get_doc(dt, dn) party_account = get_party_account(doc) @@ -235,6 +236,7 @@ def get_payment_reference_details( party_type: str | None = None, party: str | None = None, ): + frappe.has_permission(reference_doctype, "read", reference_name, throw=True) if reference_doctype in ("Expense Claim", "Employee Advance", "Gratuity", "Leave Encashment"): return get_reference_details_for_employee(reference_doctype, reference_name, party_account_currency) else: @@ -251,6 +253,7 @@ def get_reference_details_for_employee( Returns payment reference details for employee related doctypes: Employee Advance, Expense Claim, Gratuity, Leave Encashment """ + frappe.has_permission(reference_doctype, "read", reference_name, throw=True) total_amount = outstanding_amount = exchange_rate = None ref_doc = frappe.get_doc(reference_doctype, reference_name) @@ -323,6 +326,7 @@ def set_exchange_rate_in_advance(doc: Document, method: None = None): if doc.references: for reference_doc in doc.references: if reference_doc.reference_doctype == "Employee Advance" and doc.target_exchange_rate: + frappe.has_permission("Employee Advance", "write", reference_doc.reference_name, throw=True) frappe.db.set_value( "Employee Advance", reference_doc.reference_name, diff --git a/hrms/payroll/doctype/employee_benefit_claim/employee_benefit_claim.py b/hrms/payroll/doctype/employee_benefit_claim/employee_benefit_claim.py index b6f2fe01c3..4e8efe5e3d 100644 --- a/hrms/payroll/doctype/employee_benefit_claim/employee_benefit_claim.py +++ b/hrms/payroll/doctype/employee_benefit_claim/employee_benefit_claim.py @@ -192,6 +192,8 @@ def get_benefit_components( doctype: str, txt: str, searchfield: str, start: int, page_len: int, filters: dict ) -> list: """Fetch benefit components to choose from based on employee and date filters.""" + filters = frappe.parse_json(filters) + frappe.has_permission("Employee", "read", filters.get("employee"), throw=True) employee = filters.get("employee") date = filters.get("date") company = filters.get("company") diff --git a/hrms/payroll/doctype/payroll_entry/payroll_entry.py b/hrms/payroll/doctype/payroll_entry/payroll_entry.py index 10516f2fe2..73d3cb269d 100644 --- a/hrms/payroll/doctype/payroll_entry/payroll_entry.py +++ b/hrms/payroll/doctype/payroll_entry/payroll_entry.py @@ -1265,6 +1265,8 @@ def get_holidays_count(self, holiday_list: str, start_date: str, end_date: str) @frappe.whitelist() def create_overtime_slips(self) -> None: + self.check_permission("write") + from hrms.hr.doctype.overtime_slip.overtime_slip import ( create_overtime_slips_for_employees, filter_employees_for_overtime_slip_creation, @@ -1302,6 +1304,8 @@ def create_overtime_slips(self) -> None: @frappe.whitelist() def submit_overtime_slips(self) -> None: + self.check_permission("write") + from hrms.hr.doctype.overtime_slip.overtime_slip import ( submit_overtime_slips_for_employees, ) diff --git a/hrms/payroll/doctype/salary_component/salary_component.py b/hrms/payroll/doctype/salary_component/salary_component.py index 860d74f063..981fdc3402 100644 --- a/hrms/payroll/doctype/salary_component/salary_component.py +++ b/hrms/payroll/doctype/salary_component/salary_component.py @@ -165,6 +165,7 @@ def update_salary_structures( structures = self.get_structures_to_be_updated() for structure in structures: + frappe.has_permission("Salary Structure", "write", structure, throw=True) salary_structure = frappe.get_doc("Salary Structure", structure) # this is only used for versioning and we do not want # to make separate db calls by using load_doc_before_save diff --git a/hrms/payroll/doctype/salary_slip/salary_slip.py b/hrms/payroll/doctype/salary_slip/salary_slip.py index 230a87b396..d25c1896f9 100644 --- a/hrms/payroll/doctype/salary_slip/salary_slip.py +++ b/hrms/payroll/doctype/salary_slip/salary_slip.py @@ -2597,6 +2597,7 @@ def get_lwp_or_ppl_for_date_range(employee, start_date, end_date): @frappe.whitelist() def make_salary_slip_from_timesheet(source_name: str, target_doc: str | Document | None = None) -> Document: + frappe.has_permission("Timesheet", "read", source_name, throw=True) target = frappe.new_doc("Salary Slip") set_missing_values(source_name, target) target.run_method("get_emp_and_working_day_details") @@ -2659,6 +2660,9 @@ def enqueue_email_salary_slips(names: list | str) -> None: if isinstance(names, str): names = json.loads(names) + for name in names: + frappe.has_permission("Salary Slip", "read", name, throw=True) + frappe.enqueue("hrms.payroll.doctype.salary_slip.salary_slip.email_salary_slips", names=names) frappe.msgprint( _("Salary slip emails have been enqueued for sending. Check {0} for status.").format( diff --git a/hrms/payroll/doctype/salary_structure/salary_structure.py b/hrms/payroll/doctype/salary_structure/salary_structure.py index 39ef26d4c2..d8715f9d92 100644 --- a/hrms/payroll/doctype/salary_structure/salary_structure.py +++ b/hrms/payroll/doctype/salary_structure/salary_structure.py @@ -375,6 +375,9 @@ def make_salary_slip( for_preview: int = 0, lwp_days_corrected: float | None = None, ) -> str | Document: + if employee: + frappe.has_permission("Employee", "read", employee, throw=True) + return _make_salary_slip( source_name, target_doc=target_doc, diff --git a/hrms/payroll/doctype/salary_structure_assignment/salary_structure_assignment.py b/hrms/payroll/doctype/salary_structure_assignment/salary_structure_assignment.py index 68fdaafc11..712d3799ee 100644 --- a/hrms/payroll/doctype/salary_structure_assignment/salary_structure_assignment.py +++ b/hrms/payroll/doctype/salary_structure_assignment/salary_structure_assignment.py @@ -452,6 +452,7 @@ def get_assigned_salary_structure(employee, on_date): @frappe.whitelist() def get_employee_currency(employee: str) -> str: + frappe.has_permission("Employee", "read", employee, throw=True) employee_currency = frappe.db.get_value("Salary Structure Assignment", {"employee": employee}, "currency") if not employee_currency: frappe.throw( diff --git a/hrms/payroll/doctype/salary_withholding/salary_withholding.py b/hrms/payroll/doctype/salary_withholding/salary_withholding.py index 133ac2b57b..6762099635 100644 --- a/hrms/payroll/doctype/salary_withholding/salary_withholding.py +++ b/hrms/payroll/doctype/salary_withholding/salary_withholding.py @@ -131,6 +131,7 @@ def on_discard(self): @frappe.whitelist() def get_payroll_frequency(employee: str, posting_date: str | date) -> str | None: + frappe.has_permission("Employee", "read", employee, throw=True) salary_structure = frappe.db.get_value( "Salary Structure Assignment", { From b2ffc74558a61b863ddc13c0e24f1b7e85484000 Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Wed, 24 Jun 2026 19:49:39 +0530 Subject: [PATCH 08/10] test: Add endpoint tests --- .../employee_benefit_claim.py | 1 - hrms/tests/test_whitelist_permissions.py | 215 ++++++++++++++++++ 2 files changed, 215 insertions(+), 1 deletion(-) create mode 100644 hrms/tests/test_whitelist_permissions.py diff --git a/hrms/payroll/doctype/employee_benefit_claim/employee_benefit_claim.py b/hrms/payroll/doctype/employee_benefit_claim/employee_benefit_claim.py index 4e8efe5e3d..87caa854d2 100644 --- a/hrms/payroll/doctype/employee_benefit_claim/employee_benefit_claim.py +++ b/hrms/payroll/doctype/employee_benefit_claim/employee_benefit_claim.py @@ -192,7 +192,6 @@ def get_benefit_components( doctype: str, txt: str, searchfield: str, start: int, page_len: int, filters: dict ) -> list: """Fetch benefit components to choose from based on employee and date filters.""" - filters = frappe.parse_json(filters) frappe.has_permission("Employee", "read", filters.get("employee"), throw=True) employee = filters.get("employee") date = filters.get("date") diff --git a/hrms/tests/test_whitelist_permissions.py b/hrms/tests/test_whitelist_permissions.py new file mode 100644 index 0000000000..0466d8a36b --- /dev/null +++ b/hrms/tests/test_whitelist_permissions.py @@ -0,0 +1,215 @@ +# Copyright (c) 2026, Frappe Technologies Pvt. Ltd. and Contributors +# See license.txt +""" +Real-user permission scenarios for the whitelisted endpoints +Each test switches `frappe.session.user` to a concrete +user (owner / leave approver / unrelated employee / roleless user) and asserts: + + * an unauthorized user is blocked with `frappe.PermissionError`, and + * a legitimately authorized user (the owner, the approver, or an admin) is NOT + blocked by the added check (self-service is preserved). + +Coverage maps to the audit findings: + - leave_ledger_entry.expire_allocation ............ High (write bypass) + - employee_payment_entry.set_exchange_rate_in_advance High (write bypass) + - leave_application.get_leave_approver / get_holidays / get_number_of_leave_days + self-service read gate + - attendance.get_unmarked_days .................... Employee-data read leak + - api.get_company_cost_center_and_expense_account .. company-config read leak + - team_updates.get_data ........................... restored only_for role gate +""" + +import json + +import frappe +from frappe.permissions import add_user_permission +from frappe.utils import add_days, getdate +from frappe.utils.user import add_role + +from erpnext.setup.doctype.employee.test_employee import make_employee + +from hrms.api import get_company_cost_center_and_expense_account +from hrms.hr.doctype.attendance.attendance import get_unmarked_days +from hrms.hr.doctype.leave_allocation.test_leave_allocation import create_leave_allocation +from hrms.hr.doctype.leave_application.leave_application import ( + get_holidays, + get_leave_approver, + get_number_of_leave_days, +) +from hrms.hr.doctype.leave_ledger_entry.leave_ledger_entry import expire_allocation +from hrms.hr.page.team_updates.team_updates import get_data as get_team_updates +from hrms.overrides.employee_payment_entry import set_exchange_rate_in_advance +from hrms.tests.utils import HRMSTestSuite + + +class TestWhitelistPermissions(HRMSTestSuite): + @classmethod + def setUpClass(cls): + super().setUpClass() + frappe.set_user("Administrator") + cls.company = "_Test Company" + cls.leave_type = "_Test Leave Type" + + def ensure_employee(user): + # make_employee returns None if the user/employee already exists (re-runs) + return make_employee(user, cls.company) or frappe.db.get_value("Employee", {"user_id": user}) + + # owner: an employee acting on their own records + cls.owner_user = "wl_owner@example.com" + cls.owner_emp = ensure_employee(cls.owner_user) + + # the owner's leave approver + cls.approver_user = "wl_approver@example.com" + cls.approver_emp = ensure_employee(cls.approver_user) + add_role(cls.approver_user, "Leave Approver") + frappe.db.set_value("Employee", cls.owner_emp, "leave_approver", cls.approver_user) + + # outsider: an unrelated employee with no access to the owner's records + cls.outsider_user = "wl_outsider@example.com" + cls.outsider_emp = ensure_employee(cls.outsider_user) + + # each employee gets read on their OWN Employee record (standard self-service) + for user, emp in ( + (cls.owner_user, cls.owner_emp), + (cls.approver_user, cls.approver_emp), + (cls.outsider_user, cls.outsider_emp), + ): + if not frappe.db.exists("User Permission", {"user": user, "allow": "Employee", "for_value": emp}): + add_user_permission("Employee", emp, user) + + # a user with no roles at all — used to exercise role/permission gates deterministically + cls.roleless_user = "wl_roleless@example.com" + if not frappe.db.exists("User", cls.roleless_user): + frappe.get_doc( + { + "doctype": "User", + "email": cls.roleless_user, + "first_name": "Roleless", + "send_welcome_email": 0, + } + ).insert(ignore_permissions=True) + + # a submitted leave allocation owned by the owner (target for expire_allocation) + allocation = create_leave_allocation( + employee=cls.owner_emp, + leave_type=cls.leave_type, + from_date=getdate(), + to_date=add_days(getdate(), 364), + new_leaves_allocated=15, + ) + allocation.insert() + allocation.submit() + cls.allocation = allocation + + def setUp(self): + frappe.set_user("Administrator") + + def tearDown(self): + frappe.set_user("Administrator") + + def assert_not_blocked(self, fn, *args, **kwargs): + """The added permission check must NOT raise for an authorized user. + + Unrelated downstream errors are ignored — we only assert the gate lets the + caller through. + """ + try: + fn(*args, **kwargs) + except frappe.PermissionError as e: + self.fail(f"{getattr(fn, '__name__', fn)} blocked an authorized user: {e}") + except Exception: + pass + + # ------------------------------------------------------------------ High: writes + + def test_expire_allocation_blocks_unauthorized_user(self): + payload = self.allocation.as_dict() + + # an unrelated employee must not be able to expire someone else's allocation + frappe.set_user(self.outsider_user) + self.assertRaises(frappe.PermissionError, expire_allocation, payload) + + # an admin (HR-equivalent) may + frappe.set_user("Administrator") + self.assert_not_blocked(expire_allocation, payload) + + def test_set_exchange_rate_in_advance_blocks_unauthorized_user(self): + # crafts the Payment-Entry-shaped doc the endpoint accepts; the reference name + # need not exist — the write-permission check fires before any DB write. + doc = frappe._dict( + references=[ + frappe._dict(reference_doctype="Employee Advance", reference_name="FAKE-ADVANCE-001") + ], + target_exchange_rate=80.0, + ) + + frappe.set_user(self.outsider_user) + self.assertRaises(frappe.PermissionError, set_exchange_rate_in_advance, doc) + + # --------------------------------------------------- self-service leave endpoints + + def test_leave_endpoints_allow_owner_and_approver(self): + from_date = getdate() + to_date = add_days(getdate(), 2) + + # the employee themselves + frappe.set_user(self.owner_user) + self.assert_not_blocked(get_leave_approver, self.owner_emp) + self.assert_not_blocked(get_holidays, self.owner_emp, from_date, to_date) + self.assert_not_blocked(get_number_of_leave_days, self.owner_emp, self.leave_type, from_date, to_date) + + # their leave approver + frappe.set_user(self.approver_user) + self.assert_not_blocked(get_leave_approver, self.owner_emp) + + def test_leave_endpoints_block_unrelated_employee(self): + from_date = getdate() + to_date = add_days(getdate(), 2) + + frappe.set_user(self.outsider_user) + self.assertRaises(frappe.PermissionError, get_leave_approver, self.owner_emp) + self.assertRaises(frappe.PermissionError, get_holidays, self.owner_emp, from_date, to_date) + self.assertRaises( + frappe.PermissionError, + get_number_of_leave_days, + self.owner_emp, + self.leave_type, + from_date, + to_date, + ) + + # -------------------------------------------------------------- read-leak: employee + + def test_get_unmarked_days_respects_employee_read_permission(self): + from_date = getdate() + to_date = add_days(getdate(), 5) + + # owner reading their own attendance gaps is allowed + frappe.set_user(self.owner_user) + self.assert_not_blocked(get_unmarked_days, self.owner_emp, from_date, to_date) + + # an unrelated employee cannot read the owner's attendance data + frappe.set_user(self.outsider_user) + self.assertRaises(frappe.PermissionError, get_unmarked_days, self.owner_emp, from_date, to_date) + + # ----------------------------------------------------------- read-leak: company config + + def test_company_cost_center_requires_company_read(self): + # a user with no Company read permission is blocked + frappe.set_user(self.roleless_user) + self.assertRaises(frappe.PermissionError, get_company_cost_center_and_expense_account, self.company) + + # an admin is not + frappe.set_user("Administrator") + self.assert_not_blocked(get_company_cost_center_and_expense_account, self.company) + + # ------------------------------------------------------------- restored only_for gate + + def test_team_updates_enforces_role_gate(self): + # user without the Employee / System Manager role is blocked + frappe.set_user(self.roleless_user) + self.assertRaises(frappe.PermissionError, get_team_updates) + + # a regular employee passes the gate + frappe.set_user(self.owner_user) + self.assert_not_blocked(get_team_updates) From e37ca9ddf76ce8fa17e24bc0a3875cfadc89ee9b Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Thu, 25 Jun 2026 16:39:28 +0530 Subject: [PATCH 09/10] test: Simulate actual payment entry doc --- hrms/tests/test_whitelist_permissions.py | 35 ++++-------------------- 1 file changed, 5 insertions(+), 30 deletions(-) diff --git a/hrms/tests/test_whitelist_permissions.py b/hrms/tests/test_whitelist_permissions.py index 0466d8a36b..ddbd84a864 100644 --- a/hrms/tests/test_whitelist_permissions.py +++ b/hrms/tests/test_whitelist_permissions.py @@ -8,15 +8,6 @@ * an unauthorized user is blocked with `frappe.PermissionError`, and * a legitimately authorized user (the owner, the approver, or an admin) is NOT blocked by the added check (self-service is preserved). - -Coverage maps to the audit findings: - - leave_ledger_entry.expire_allocation ............ High (write bypass) - - employee_payment_entry.set_exchange_rate_in_advance High (write bypass) - - leave_application.get_leave_approver / get_holidays / get_number_of_leave_days - self-service read gate - - attendance.get_unmarked_days .................... Employee-data read leak - - api.get_company_cost_center_and_expense_account .. company-config read leak - - team_updates.get_data ........................... restored only_for role gate """ import json @@ -101,12 +92,6 @@ def ensure_employee(user): allocation.submit() cls.allocation = allocation - def setUp(self): - frappe.set_user("Administrator") - - def tearDown(self): - frappe.set_user("Administrator") - def assert_not_blocked(self, fn, *args, **kwargs): """The added permission check must NOT raise for an authorized user. @@ -120,8 +105,6 @@ def assert_not_blocked(self, fn, *args, **kwargs): except Exception: pass - # ------------------------------------------------------------------ High: writes - def test_expire_allocation_blocks_unauthorized_user(self): payload = self.allocation.as_dict() @@ -136,18 +119,16 @@ def test_expire_allocation_blocks_unauthorized_user(self): def test_set_exchange_rate_in_advance_blocks_unauthorized_user(self): # crafts the Payment-Entry-shaped doc the endpoint accepts; the reference name # need not exist — the write-permission check fires before any DB write. - doc = frappe._dict( - references=[ - frappe._dict(reference_doctype="Employee Advance", reference_name="FAKE-ADVANCE-001") - ], - target_exchange_rate=80.0, + doc = frappe.new_doc("Payment Entry") + doc.set( + "references", + [frappe._dict(reference_doctype="Employee Advance", reference_name="FAKE-ADVANCE-001")], ) + doc.set("target_exchange_rate", 80.0) frappe.set_user(self.outsider_user) self.assertRaises(frappe.PermissionError, set_exchange_rate_in_advance, doc) - # --------------------------------------------------- self-service leave endpoints - def test_leave_endpoints_allow_owner_and_approver(self): from_date = getdate() to_date = add_days(getdate(), 2) @@ -178,8 +159,6 @@ def test_leave_endpoints_block_unrelated_employee(self): to_date, ) - # -------------------------------------------------------------- read-leak: employee - def test_get_unmarked_days_respects_employee_read_permission(self): from_date = getdate() to_date = add_days(getdate(), 5) @@ -192,8 +171,6 @@ def test_get_unmarked_days_respects_employee_read_permission(self): frappe.set_user(self.outsider_user) self.assertRaises(frappe.PermissionError, get_unmarked_days, self.owner_emp, from_date, to_date) - # ----------------------------------------------------------- read-leak: company config - def test_company_cost_center_requires_company_read(self): # a user with no Company read permission is blocked frappe.set_user(self.roleless_user) @@ -203,8 +180,6 @@ def test_company_cost_center_requires_company_read(self): frappe.set_user("Administrator") self.assert_not_blocked(get_company_cost_center_and_expense_account, self.company) - # ------------------------------------------------------------- restored only_for gate - def test_team_updates_enforces_role_gate(self): # user without the Employee / System Manager role is blocked frappe.set_user(self.roleless_user) From 7000f3a30823e15ccd1d94b57edce0784cba3b07 Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Fri, 26 Jun 2026 16:40:00 +0530 Subject: [PATCH 10/10] chore: delete test file --- hrms/tests/test_whitelist_permissions.py | 190 ----------------------- 1 file changed, 190 deletions(-) delete mode 100644 hrms/tests/test_whitelist_permissions.py diff --git a/hrms/tests/test_whitelist_permissions.py b/hrms/tests/test_whitelist_permissions.py deleted file mode 100644 index ddbd84a864..0000000000 --- a/hrms/tests/test_whitelist_permissions.py +++ /dev/null @@ -1,190 +0,0 @@ -# Copyright (c) 2026, Frappe Technologies Pvt. Ltd. and Contributors -# See license.txt -""" -Real-user permission scenarios for the whitelisted endpoints -Each test switches `frappe.session.user` to a concrete -user (owner / leave approver / unrelated employee / roleless user) and asserts: - - * an unauthorized user is blocked with `frappe.PermissionError`, and - * a legitimately authorized user (the owner, the approver, or an admin) is NOT - blocked by the added check (self-service is preserved). -""" - -import json - -import frappe -from frappe.permissions import add_user_permission -from frappe.utils import add_days, getdate -from frappe.utils.user import add_role - -from erpnext.setup.doctype.employee.test_employee import make_employee - -from hrms.api import get_company_cost_center_and_expense_account -from hrms.hr.doctype.attendance.attendance import get_unmarked_days -from hrms.hr.doctype.leave_allocation.test_leave_allocation import create_leave_allocation -from hrms.hr.doctype.leave_application.leave_application import ( - get_holidays, - get_leave_approver, - get_number_of_leave_days, -) -from hrms.hr.doctype.leave_ledger_entry.leave_ledger_entry import expire_allocation -from hrms.hr.page.team_updates.team_updates import get_data as get_team_updates -from hrms.overrides.employee_payment_entry import set_exchange_rate_in_advance -from hrms.tests.utils import HRMSTestSuite - - -class TestWhitelistPermissions(HRMSTestSuite): - @classmethod - def setUpClass(cls): - super().setUpClass() - frappe.set_user("Administrator") - cls.company = "_Test Company" - cls.leave_type = "_Test Leave Type" - - def ensure_employee(user): - # make_employee returns None if the user/employee already exists (re-runs) - return make_employee(user, cls.company) or frappe.db.get_value("Employee", {"user_id": user}) - - # owner: an employee acting on their own records - cls.owner_user = "wl_owner@example.com" - cls.owner_emp = ensure_employee(cls.owner_user) - - # the owner's leave approver - cls.approver_user = "wl_approver@example.com" - cls.approver_emp = ensure_employee(cls.approver_user) - add_role(cls.approver_user, "Leave Approver") - frappe.db.set_value("Employee", cls.owner_emp, "leave_approver", cls.approver_user) - - # outsider: an unrelated employee with no access to the owner's records - cls.outsider_user = "wl_outsider@example.com" - cls.outsider_emp = ensure_employee(cls.outsider_user) - - # each employee gets read on their OWN Employee record (standard self-service) - for user, emp in ( - (cls.owner_user, cls.owner_emp), - (cls.approver_user, cls.approver_emp), - (cls.outsider_user, cls.outsider_emp), - ): - if not frappe.db.exists("User Permission", {"user": user, "allow": "Employee", "for_value": emp}): - add_user_permission("Employee", emp, user) - - # a user with no roles at all — used to exercise role/permission gates deterministically - cls.roleless_user = "wl_roleless@example.com" - if not frappe.db.exists("User", cls.roleless_user): - frappe.get_doc( - { - "doctype": "User", - "email": cls.roleless_user, - "first_name": "Roleless", - "send_welcome_email": 0, - } - ).insert(ignore_permissions=True) - - # a submitted leave allocation owned by the owner (target for expire_allocation) - allocation = create_leave_allocation( - employee=cls.owner_emp, - leave_type=cls.leave_type, - from_date=getdate(), - to_date=add_days(getdate(), 364), - new_leaves_allocated=15, - ) - allocation.insert() - allocation.submit() - cls.allocation = allocation - - def assert_not_blocked(self, fn, *args, **kwargs): - """The added permission check must NOT raise for an authorized user. - - Unrelated downstream errors are ignored — we only assert the gate lets the - caller through. - """ - try: - fn(*args, **kwargs) - except frappe.PermissionError as e: - self.fail(f"{getattr(fn, '__name__', fn)} blocked an authorized user: {e}") - except Exception: - pass - - def test_expire_allocation_blocks_unauthorized_user(self): - payload = self.allocation.as_dict() - - # an unrelated employee must not be able to expire someone else's allocation - frappe.set_user(self.outsider_user) - self.assertRaises(frappe.PermissionError, expire_allocation, payload) - - # an admin (HR-equivalent) may - frappe.set_user("Administrator") - self.assert_not_blocked(expire_allocation, payload) - - def test_set_exchange_rate_in_advance_blocks_unauthorized_user(self): - # crafts the Payment-Entry-shaped doc the endpoint accepts; the reference name - # need not exist — the write-permission check fires before any DB write. - doc = frappe.new_doc("Payment Entry") - doc.set( - "references", - [frappe._dict(reference_doctype="Employee Advance", reference_name="FAKE-ADVANCE-001")], - ) - doc.set("target_exchange_rate", 80.0) - - frappe.set_user(self.outsider_user) - self.assertRaises(frappe.PermissionError, set_exchange_rate_in_advance, doc) - - def test_leave_endpoints_allow_owner_and_approver(self): - from_date = getdate() - to_date = add_days(getdate(), 2) - - # the employee themselves - frappe.set_user(self.owner_user) - self.assert_not_blocked(get_leave_approver, self.owner_emp) - self.assert_not_blocked(get_holidays, self.owner_emp, from_date, to_date) - self.assert_not_blocked(get_number_of_leave_days, self.owner_emp, self.leave_type, from_date, to_date) - - # their leave approver - frappe.set_user(self.approver_user) - self.assert_not_blocked(get_leave_approver, self.owner_emp) - - def test_leave_endpoints_block_unrelated_employee(self): - from_date = getdate() - to_date = add_days(getdate(), 2) - - frappe.set_user(self.outsider_user) - self.assertRaises(frappe.PermissionError, get_leave_approver, self.owner_emp) - self.assertRaises(frappe.PermissionError, get_holidays, self.owner_emp, from_date, to_date) - self.assertRaises( - frappe.PermissionError, - get_number_of_leave_days, - self.owner_emp, - self.leave_type, - from_date, - to_date, - ) - - def test_get_unmarked_days_respects_employee_read_permission(self): - from_date = getdate() - to_date = add_days(getdate(), 5) - - # owner reading their own attendance gaps is allowed - frappe.set_user(self.owner_user) - self.assert_not_blocked(get_unmarked_days, self.owner_emp, from_date, to_date) - - # an unrelated employee cannot read the owner's attendance data - frappe.set_user(self.outsider_user) - self.assertRaises(frappe.PermissionError, get_unmarked_days, self.owner_emp, from_date, to_date) - - def test_company_cost_center_requires_company_read(self): - # a user with no Company read permission is blocked - frappe.set_user(self.roleless_user) - self.assertRaises(frappe.PermissionError, get_company_cost_center_and_expense_account, self.company) - - # an admin is not - frappe.set_user("Administrator") - self.assert_not_blocked(get_company_cost_center_and_expense_account, self.company) - - def test_team_updates_enforces_role_gate(self): - # user without the Employee / System Manager role is blocked - frappe.set_user(self.roleless_user) - self.assertRaises(frappe.PermissionError, get_team_updates) - - # a regular employee passes the gate - frappe.set_user(self.owner_user) - self.assert_not_blocked(get_team_updates)