[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

@@ -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})

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})

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({