From 7359c6de0885ded140879d1389c233ed4c618cc8 Mon Sep 17 00:00:00 2001 From: Rob Coekaerts Date: Thu, 22 Dec 2022 17:30:28 +0100 Subject: [PATCH] [IMP] hr_holidays: Remove leave splitting Currently when a global leave is removed or modified in such a way that the duration of an existing leave increases, the leave is instead split into multiple leaves with a gap in between such that it no longer includes the period that was previously covered by the global leave. However, this splitting is very error-prone and from a user perspective is not even always the behaviour that is expected, especially because editing global leaves is a very low-threshold operation during which it is in no way indicated that existing leaves might be impacted. Also note that this is very specific edge case, that in normal operation should almost never occur. It is therefore best to simply use an approach that has a minimal impact on the codebase complexity and produces no unexpected results for the end user. Therefore in this commit, we handle this case in the following way: - If a global leave is created or extended in duration, we simply recompute the impacted leave durations (as was the case before). - If a global leave is shortened in duration or deleted, the durations of the affected leaves are also recomputed, but: - In the case that existing allocations can cover this increase in leave duration, the leave is left 'as is', but the user is notified that additional allocation time has been taken. - In the case that existing allocations cannot cover this increase, the user is again notified, but the leave is additionally reset to the 'draft' state (to prevent the allocation from going negative, which is impossible). With this approach, it now up to the user to handle this specific edge case, while no information is changed or lost, which was possible to happen with the previous approach. Part-of: odoo/odoo#119317 --- addons/hr_holidays/models/hr_leave.py | 55 ------------------- addons/hr_holidays/models/resource.py | 24 ++++---- .../hr_holidays/tests/test_leave_requests.py | 11 ++-- 3 files changed, 18 insertions(+), 72 deletions(-) diff --git a/addons/hr_holidays/models/hr_leave.py b/addons/hr_holidays/models/hr_leave.py index e1f0ff5e1d6..d2074b6c942 100644 --- a/addons/hr_holidays/models/hr_leave.py +++ b/addons/hr_holidays/models/hr_leave.py @@ -12,7 +12,6 @@ from datetime import datetime, timedelta, time from pytz import timezone, UTC from odoo.addons.base.models.ir_model import MODULE_UNINSTALL_FLAG -from odoo.tools import date_utils from odoo import api, Command, fields, models, tools from odoo.addons.base.models.res_partner import _tz_get @@ -1475,60 +1474,6 @@ class HolidaysRequest(models.Model): if not is_officer and (state == 'validate' and val_type == 'hr') and holiday.holiday_type == 'employee': raise UserError(_('You must either be a Time off Officer or Time off Manager to approve this leave')) - ################################################### - # Time Off modification methods - ################################################### - - def _split_leave_on_gto(self, gto): #gto = global time off - self.ensure_one() - - leave_start = date_utils.start_of(self.date_from, 'day') - leave_end = date_utils.end_of(self.date_to - timedelta(seconds=1), 'day') - gto_start = date_utils.start_of(gto['date_from'], 'day') - gto_end = date_utils.end_of(gto['date_to'], 'day') - leave_tz = timezone(self.employee_id.resource_id.tz) - - if gto_start <= leave_start\ - and gto_end > leave_start\ - and gto_end < leave_end: - self.write({ - 'date_from': leave_tz.localize(gto_end + timedelta(seconds=1))\ - .astimezone(UTC).replace(tzinfo=None) - }) - return self.env['hr.leave'] - if gto_start > leave_start\ - and gto_end < leave_end: - copys = { - 'date_from': self.date_from, - 'date_to': leave_tz.localize(gto_start - timedelta(seconds=1))\ - .astimezone(UTC).replace(tzinfo=None) - } - self.write({ - 'date_from': leave_tz.localize(gto_end + timedelta(seconds=1))\ - .astimezone(UTC).replace(tzinfo=None) - }) - return self.copy(copys) - if gto_start > leave_start\ - and gto_start < leave_end\ - and gto_end >= leave_end: - self.write({ - 'date_to': leave_tz.localize(gto_start - timedelta(seconds=1))\ - .astimezone(UTC).replace(tzinfo=None) - }) - return self.env['hr.leave'] - - def split_leave(self, time_domain_dict): - self.ensure_one() - - new_leaves = self.env['hr.leave'] - for global_time_off in sorted( - filter(lambda r: r['date_to'] > self.date_from and r['date_from'] < self.date_to, time_domain_dict), - key=lambda r: r['date_from']): - new_leave = self._split_leave_on_gto(global_time_off) - if new_leave: - new_leaves |= new_leave - return new_leaves - # ------------------------------------------------------------ # Activity methods # ------------------------------------------------------------ diff --git a/addons/hr_holidays/models/resource.py b/addons/hr_holidays/models/resource.py index 4ce33ccb214..a903055b739 100644 --- a/addons/hr_holidays/models/resource.py +++ b/addons/hr_holidays/models/resource.py @@ -67,22 +67,20 @@ class CalendarLeaves(models.Model): sick_time_status = self.env.ref('hr_holidays.holiday_status_sl') for previous_duration, leave, state in zip(previous_durations, leaves, previous_states): duration_difference = previous_duration - leave.number_of_days - if duration_difference > 0 and leave['holiday_allocation_id'] and leave.number_of_days == 0.0: + message = False + if duration_difference > 0 and leave['holiday_allocation_id']: message = _("Due to a change in global time offs, you have been granted %s day(s) back.", duration_difference) - leave._notify_change(message) if leave.number_of_days > previous_duration\ and leave.holiday_status_id not in sick_time_status: - new_leaves = leave.split_leave(time_domain_dict) - leaves |= new_leaves - previous_states += [state] * len(new_leaves) - - leaves_to_cancel = self.env['hr.leave'] - for state, leave in zip(previous_states, leaves): - leave.write({'state': state}) - if leave.number_of_days == 0.0: - leaves_to_cancel |= leave - - leaves_to_cancel._force_cancel(_("a new public holiday completely overrides this leave."), 'mail.mt_comment') + message = _("Due to a change in global time offs, %s extra day(s) have been taken from your allocation. Please review this leave if you need it to be changed.", -1 * duration_difference) + try: + leave.write({'state': state}) + leave._check_holidays() + except ValidationError: + leave.action_refuse() + message = _("Due to a change in global time offs, this leave no longer has the required amount of available allocation and has been set to refused. Please review this leave.") + if message: + leave._notify_change(message) def _convert_timezone(self, utc_naive_datetime, tz_from, tz_to): """ diff --git a/addons/hr_holidays/tests/test_leave_requests.py b/addons/hr_holidays/tests/test_leave_requests.py index d61c4581785..c4e6bb57b40 100644 --- a/addons/hr_holidays/tests/test_leave_requests.py +++ b/addons/hr_holidays/tests/test_leave_requests.py @@ -758,7 +758,7 @@ class TestLeaveRequests(TestHrHolidaysCommon): 'date_from': '2021-12-15 00:00:00', 'date_to': '2021-12-15 23:59:59', }) - self.assertEqual(time_off_1.number_of_days, 2) + self.assertEqual(time_off_1.number_of_days, 5) self.assertEqual(time_off_2.number_of_days, 4) def test_time_off_recovery_on_unlink(self): @@ -776,9 +776,9 @@ class TestLeaveRequests(TestHrHolidaysCommon): }) self.assertEqual(time_off.number_of_days, 4) global_time_off.unlink() - self.assertEqual(time_off.number_of_days, 3) + self.assertEqual(time_off.number_of_days, 5) - def test_time_off_auto_cancel(self): + def test_time_off_duration_zero(self): time_off = self.env['hr.leave'].create({ 'name': 'Holiday Request', 'employee_id': self.employee_emp_id, @@ -786,12 +786,15 @@ class TestLeaveRequests(TestHrHolidaysCommon): 'date_from': '2021-11-15 00:00:00', 'date_to': '2021-11-19 23:59:59', }) + self.assertEqual(time_off.number_of_days, 5) self.env['resource.calendar.leaves'].create({ 'name': 'Global Time Off', 'date_from': '2021-11-15 00:00:00', 'date_to': '2021-11-19 23:59:59', }) - self.assertEqual(time_off.active, False) + self.assertEqual(time_off.active, True) + self.assertEqual(time_off.number_of_days, 0) + def test_holiday_type_requires_no_allocation(self): # holiday_type_2 initially requires an allocation # Once an allocation is granted and a leave is taken,