From e162ff8701013939e9dca2fef70ac5794c33d0c3 Mon Sep 17 00:00:00 2001 From: RomainLibert Date: Wed, 18 Apr 2018 17:02:04 +0200 Subject: [PATCH] [FIX] hr_holidays: make rights coherent As we have two parallel tasks changing the rights for the approval of leaves, we have to make sure that it stays coherent. Basically by default an hr_holiday manager can approve anything and the hr_holiday officer can only approve the leaves of his department and cannot approve his own leaves. But you can override this behaviour by setting the Validation By to "Manager" which will force the leave to be approved by an hr_holiday manager. --- addons/hr_holidays/models/hr_leave.py | 8 +++---- .../hr_holidays/models/hr_leave_allocation.py | 8 +++---- .../hr_holidays/tests/test_holidays_flow.py | 24 +++++++++---------- 3 files changed, 17 insertions(+), 23 deletions(-) diff --git a/addons/hr_holidays/models/hr_leave.py b/addons/hr_holidays/models/hr_leave.py index 0a3b61045b5..377e6d7d640 100644 --- a/addons/hr_holidays/models/hr_leave.py +++ b/addons/hr_holidays/models/hr_leave.py @@ -143,9 +143,7 @@ class HolidaysRequest(models.Model): """ for holiday in self: # User is holiday manager and has no manager - manager = self.user_has_groups('hr_holidays.group_hr_holidays_manager') \ - and not holiday.employee_id.parent_id \ - and not holiday.department_id.manager_id + manager = self.user_has_groups('hr_holidays.group_hr_holidays_manager') holiday.can_approve = (holiday.employee_id.user_id.id != self.env.uid) or manager @api.onchange('holiday_type') @@ -412,10 +410,10 @@ class HolidaysRequest(models.Model): for holiday in self: validation_type = holiday.holiday_status_id.validation_type manager = holiday.employee_id.parent_id or holiday.employee_id.department_id.manager_id - if (validation_type in ['manager', 'both']) and (manager and manager != current_employee)\ + if (validation_type in ['hr', 'both']) and (manager and manager != current_employee)\ and not self.env.user.has_group('hr_holidays.group_hr_holidays_manager'): raise UserError(_('You must be %s manager to approve this leave') % (holiday.employee_id.name)) - elif validation_type == 'hr' and not self.env.user.has_group('hr_holidays.group_hr_holidays_manager'): + elif validation_type == 'manager' and not self.env.user.has_group('hr_holidays.group_hr_holidays_manager'): raise UserError(_('You must be a Human Resource Manager to approve this Leave')) self.filtered(lambda hol: hol.validation_type == 'both').write({'state': 'validate1', 'first_approver_id': current_employee.id}) diff --git a/addons/hr_holidays/models/hr_leave_allocation.py b/addons/hr_holidays/models/hr_leave_allocation.py index 75d2e0995dc..018866da480 100644 --- a/addons/hr_holidays/models/hr_leave_allocation.py +++ b/addons/hr_holidays/models/hr_leave_allocation.py @@ -115,9 +115,7 @@ class HolidaysAllocation(models.Model): """ for holiday in self: # User is holiday manager and has no manager - manager = self.user_has_groups('hr_holidays.group_hr_holidays_manager') \ - and not holiday.employee_id.parent_id \ - and not holiday.department_id.manager_id + manager = self.user_has_groups('hr_holidays.group_hr_holidays_manager') holiday.can_approve = (holiday.employee_id.user_id.id != self.env.uid) or manager @api.onchange('holiday_type') @@ -255,10 +253,10 @@ class HolidaysAllocation(models.Model): for holiday in self: validation_type = holiday.holiday_status_id.validation_type manager = holiday.employee_id.parent_id or holiday.employee_id.department_id.manager_id - if (validation_type in ['manager', 'both']) and (manager and manager != current_employee)\ + if (validation_type in ['hr', 'both']) and (manager and manager != current_employee)\ and not self.env.user.has_group('hr_holidays.group_hr_holidays_manager'): raise UserError(_('You must be %s manager to approve this leave') % (holiday.employee_id.name)) - elif validation_type == 'hr' and not self.env.user.has_group('hr_holidays.group_hr_holidays_manager'): + elif validation_type == 'manager' and not self.env.user.has_group('hr_holidays.group_hr_holidays_manager'): raise UserError(_('You must be a Human Resource Manager to approve this Leave')) self.filtered(lambda hol: hol.validation_type == 'both').write({'state': 'validate1', 'first_approver_id': current_employee.id}) diff --git a/addons/hr_holidays/tests/test_holidays_flow.py b/addons/hr_holidays/tests/test_holidays_flow.py index ff5f3a80307..6bbb81d57c0 100644 --- a/addons/hr_holidays/tests/test_holidays_flow.py +++ b/addons/hr_holidays/tests/test_holidays_flow.py @@ -101,14 +101,9 @@ class TestHolidaysFlow(TestHrHolidaysBase): hol1_employee_group.action_approve() self.assertEqual(hol1_manager_group.state, 'confirm', 'hr_holidays: employee should not be able to validate its own leave request') - # HrUser validates the employee leave request -> should not work - with self.assertRaises(UserError): - hol1_user_group.action_approve() - self.assertEqual(hol1_manager_group.state, 'confirm', 'hr_holidays: hr user should not be able to validate manager only leaves') - - # HrManager validates the employee leave request - hol1_manager_group.action_approve() - self.assertEqual(hol1_manager_group.state, 'validate', 'hr_holidays: validates leave request should be in validate state') + # HrUser validates the employee leave request -> should work + hol1_user_group.action_approve() + self.assertEqual(hol1_manager_group.state, 'validate', 'hr_holidays: validated leave request should be in validate state') # Employee creates a leave request in a no-limit category department manager only hol12_employee_group = HolidaysEmployeeGroup.create({ @@ -128,8 +123,12 @@ class TestHolidaysFlow(TestHrHolidaysBase): hol12_employee_group.action_approve() self.assertEqual(hol12_user_group.state, 'confirm', 'hr_holidays: employee should not be able to validate its own leave request') - # HrUser validates the employee leave request - hol12_user_group.action_approve() + # HrUser validates the employee leave request -> should not work + with self.assertRaises(UserError): + hol12_user_group.action_approve() + + # HrManager validate the employee leave request + hol12_manager_group.action_approve() self.assertEqual(hol1_user_group.state, 'validate', 'hr_holidays: validates leave request should be in validate state') # -------------------------------------------------- @@ -328,7 +327,7 @@ class TestHolidaysFlow(TestHrHolidaysBase): with self.assertRaises(UserError): hol50.action_approve() - # Manager should not be able to approve it's own leave + # Manager should be able to approve it's own leave hol51 = HolidaysEmployeeGroup.sudo(self.user_hrmanager_2_id).create({ 'name': 'Hol51', 'employee_id': self.employee_hrmanager_2_id, @@ -338,8 +337,7 @@ class TestHolidaysFlow(TestHrHolidaysBase): 'number_of_days_temp': 1, }) - with self.assertRaises(UserError): - hol51.action_approve() + hol51.action_approve() # Unless there is not manager above hol52 = HolidaysEmployeeGroup.sudo(self.user_hrmanager_id).create({