Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat: toggle self leave approval for employees from HR settings #2486

Merged
merged 12 commits into from
Jan 30, 2025

Conversation

asmitahase
Copy link
Collaborator

@asmitahase asmitahase commented Dec 6, 2024

Issue

Immediately after employees create and save leave application they see the document with "submit" as primary action that can lead to confusion as to clicking submit is part of the leave application process. So the employees ended up submitting their own leaves even if a different leave approver was already set in the application.
Role permission do not help in case one employee is leave approver for another employee.


Before

self_leave_approval_before.mov

After

self_leave_approval_pwa_after.mov
Screen.Recording.2025-01-30.at.2.46.24.PM.mov

Fix

Added a new setting in HR Settings called Prevent self-approval for leaves even if user has permissions which is disabled by default to retain current behaviour.
The submit action is removed on leave application if the settings is checked. Setting is ignored if the leave application doctype has a workflow.
Added tests for when self approval is enabled/disabled
I'll update the documentation once this is merged, no-docs for now

"default": "1",
"fieldname": "allow_self_leave_approval",
"fieldtype": "Check",
"label": "Enable Self-Approval For Leaves"
Copy link
Member

@ruchamahabal ruchamahabal Dec 10, 2024

Choose a reason for hiding this comment

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

Suggested change
"label": "Enable Self-Approval For Leaves"
"label": "Allow self approval for Leave Application"

Fieldname says allow so keep it consistent?

Copy link
Member

Choose a reason for hiding this comment

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

Based on this setting, Approval field should also become read-only - client-side change

Comment on lines 799 to 805

if (not self_leave_approval_allowed) and (
self.employee == get_current_employee_info().get("name")
if get_current_employee_info()
else None and not get_workflow_name("Leave Application")
):
frappe.throw(_("Self-approval for leaves is not allowed"), frappe.ValidationError)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (not self_leave_approval_allowed) and (
self.employee == get_current_employee_info().get("name")
if get_current_employee_info()
else None and not get_workflow_name("Leave Application")
):
frappe.throw(_("Self-approval for leaves is not allowed"), frappe.ValidationError)
employee_user = frappe.db.get_value("Employee", self.employee, "user_id")
if (
employee_user == frappe.session.user
and not self_leave_approval_allowed
and not get_workflow_name("Leave Application")
):
frappe.throw(_("Self approval for leaves is not allowed"))
  • Simplify checks
  • get_current_employee_info fetches additional columns that aren't required here. We only need to compare the session user.
  • Drop frappe.ValidationError, that's the default exception
    image

@ruchamahabal ruchamahabal marked this pull request as draft January 13, 2025 09:17
@ruchamahabal
Copy link
Member

From the last internal discussion, leaving context here:

Approval field has perm level set by default so employees don’t have access to edit the field HR managers and leave approvers do
But when the user themselves are hr managers, you might want to prevent self-approving leaves for such users
So the need for the setting comes from prevention rather than allowance

"Prevent self approval even if the user has permissions" is a better way to communicate instead of allow

And for users who have all perms and self-approval is allowed (the above setting is disabled), we can directly show submit button and change status to "Approved" on submission. No need to click any extra buttons or play around with the fields

@asmitahase asmitahase marked this pull request as ready for review January 30, 2025 07:31
@asmitahase asmitahase merged commit aa59217 into frappe:develop Jan 30, 2025
7 checks passed
asmitahase added a commit that referenced this pull request Jan 30, 2025
…2486

feat: toggle self leave approval for employees from HR settings (backport #2486)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants