fix(security): enforce read permission check in Payroll Entry overtime endpoints

[pratheep-bit]

Summary of Changes

• Add self.check_permission("read") to @frappe.whitelist() methods get_unsubmitted_overtime_slips and get_overtime_slip_details in PayrollEntry.
• Prevents low-privileged users from reading unsubmitted overtime slip details without read permissions on Payroll Entry.
• Includes unit test test_get_unsubmitted_overtime_slips_permission_check in test_payroll_entry.py.

[greptile-apps]

Confidence Score: 4/5

The production permission guards are correct; the test does not fully verify both methods.

The two production-code lines are sound. The test only ever exercises get_unsubmitted_overtime_slips — get_overtime_slip_details is silently skipped because both calls share a single assertRaises block and execution stops at the first exception.

Files Needing Attention: test_payroll_entry.py — the permission test needs separate assertRaises blocks for each method.

_{Reviews (14): Last reviewed commit: "fix(test): clear Guest permission cache ..." | Re-trigger Greptile}

[iamkhanraheel]

@pratheep-bit, this test only checks that get_payroll_entries_for_jv returns a list, not that the docstatus == 1 filter actually works. It'd pass even without your the fix you added.
Could you create a Draft Payroll Entry and assert it's excluded from the results (and a submitted one is included)?

[iamkhanraheel]

@pratheep-bit, you've already raised a PR (#4997) for submitted payroll filter. Why is the same commit duplicated here too? Could you drop it from one of the two?

[pratheep-bit]

Hi @iamkhanraheel,

Thanks for the feedback!

1. Dropped Duplicated Commit: Removed the filter on and its corresponding test from this PR, leaving that strictly to PR fix(payroll): filter submitted records in get_payroll_entries_for_jv #4997.
2. Focused Scope: This PR is now cleanly dedicated to adding guards to and in .
3. Unit Test Cleanup: Updated with for clean context restoration.

Force-pushed a single clean commit.

[pratheep-bit]

Fixed CI test failure in test_get_unsubmitted_overtime_slips_permission_check.

Root Cause: The test previously called make_payroll_entry(), which executes fill_employee_details() and searches for active employee salary structures for the given date range. Since no employee salary structure was set up for those dates in the CI runner, it raised ValidationError: No employees found for the mentioned criteria.

Fix: Replaced make_payroll_entry() with frappe.get_doc(doctype="Payroll Entry", ...).insert(), creating the document instance directly without relying on pre-existing employee test setup.

Force-pushed.

[pratheep-bit]

Fixed the CI failure properly this time. The previous amend picked up stale staged code.

What was wrong: The test used make_payroll_entry() which calls fill_employee_details() internally and fails when no employees have active salary structures for the given dates.

Fix: Replaced with frappe.get_doc(doctype="Payroll Entry", ...).insert() — creates a saved Payroll Entry document directly without needing employee setup. The test only verifies permission checks, so it does not need employees populated.

Also removed the duplicated get_payroll_entries_for_jv changes (belongs in PR #4997).

Force-pushed clean single commit 0ea8ed0b5.

[iamkhanraheel]

Hi @iamkhanraheel,

Thanks for the feedback!

1. Dropped Duplicated Commit: Removed the filter on and its corresponding test from this PR, leaving that strictly to PR fix(payroll): filter submitted records in get_payroll_entries_for_jv #4997.
2. Focused Scope: This PR is now cleanly dedicated to adding guards to and in .
3. Unit Test Cleanup: Updated with for clean context restoration.

Force-pushed a single clean commit.

@pratheep-bit, Hope you will push the fix for test #4998 (comment) in #4997

[pratheep-bit]

Hi @iamkhanraheel,

Updated in PR #4998 with and . It now creates a saved Payroll Entry cleanly without depending on pre-existing employee/salary structure setups in the test runner.

Force-pushed clean commit.

[pratheep-bit]

Hi @iamkhanraheel, thank you for the thorough review!

I've addressed both points raised:

1. get_payroll_entries_for_jv now filters by docstatus == 1 — so only submitted Payroll Entries appear in the JV link list. Draft entries are excluded.

2. New test test_get_payroll_entries_for_jv_filters_docstatus — creates a Draft Payroll Entry (docstatus=0) and asserts it does not appear in the results, directly validating the filter. The draft is cleaned up at the end.

The updated commit is: fix(payroll): add docstatus==1 filter to get_payroll_entries_for_jv and assert draft entry is excluded in test

Please let me know if anything else needs to be changed!

[pratheep-bit]

Fixed! Moved the test user setup cleanly before the assertion block and removed the post-set_user get_doc fetch. The permission exception now fires directly on calling get_unsubmitted_overtime_slips() and get_overtime_slip_details().

Also kept PR #4998 strictly scoped to the overtime slip permission check as requested.