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

[FIX] hr_holidays: correctly calculate leave duration #161856

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

taqu-odoo
Copy link
Contributor

@taqu-odoo taqu-odoo commented Apr 15, 2024

Steps

  • Time off installed
  • Put an employee on a different schedule than the company's
    default one and make him for example start earlier.
  • Create a new Time Off type, set "Requires allocation" to
    No Limit.
  • Approvals > Time off > New and create a time off (mode "by
    company") on the day where the employee is starting earlier.
  • Approve it.
  • Approvals > Time off : the leave's duration for the employee
    on a different schedule is incorrect (slightly less than 1 day).

Issue

When creating a leave on a company level, a leave will first be
created with start and end hours based on the company's default
schedule.
When approving the leave, a leave is then created for each
affected employee. The date_from and date_to received by
create are based on the company's default schedule. This is not
generally problematic as it is taken into account. For example,
the date_from and date_to of the employee's leave are correct
even if he is on a different schedule.

However, when we compute the number of days, it is not accounted for.

employee_leave_date_duration[(date_from, date_to)] = self._get_number_of_days_batch(date_from, date_to, employee_ids)

result = employee._get_work_days_data_batch(date_from, date_to, domain=domain)

As a result, the hours worked by an employee on a different schedule
which are "outside" of the company's default schedule don't count
towards the calculation.
https://github.com/odoo/odoo/blob/3eec4d6a69ff76cbf750b960dc8db2eb5e0a45f9/addons/resource/models/resource_mixin.py#L94-101

Note: the issue is the same for leaves given to a department or an
employee tag since they are created the same way.

Fix

When creating a leave for employees, if the leave is not for half
days or hours, give the whole day as a leave to account for different
schedules. Note that the actual date_from and date_to of each employee's
leave are still based on their actual attendance, but if an employee
has hours outside of the company's schedule, they will count towards
the leave's duration.

opw-3703793

@robodoo
Copy link
Contributor

robodoo commented Apr 15, 2024

@C3POdoo C3POdoo added the OE the report is linked to a support ticket (opw-...) label Apr 15, 2024
@taqu-odoo taqu-odoo force-pushed the 16.0-opw-3703793-company_time_off_duration-taqu branch 2 times, most recently from 0ecdae4 to b462f94 Compare April 19, 2024 10:05
@taqu-odoo
Copy link
Contributor Author

@fw-bot up to saas-16.4 6074d86 refactored the leave duration in 17.0 and fixed this issue.

@fw-bot
Copy link
Contributor

fw-bot commented Apr 19, 2024

Forward-porting to 'saas-16.4'.

Copy link
Contributor

@nle-odoo nle-odoo left a comment

Choose a reason for hiding this comment

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

seems to make sense, but I don't know if there is better ways to do it

seeing the code below does something similar:

date_from = values.get('date_from')
date_to = values.get('date_to')
employee = employees.filtered(lambda emp: emp.id == employee_id)
attendance_from, attendance_to = self._get_attendances(employee, date_from.date(), date_to.date())
hour_from = float_to_time(attendance_from.hour_from)
hour_to = float_to_time(attendance_to.hour_to)
hour_from = hour_from.hour + hour_from.minute / 60
hour_to = hour_to.hour + hour_to.minute / 60
values['date_from'] = self._get_start_or_end_from_attendance(hour_from, date_from.date(), employee)
values['date_to'] = self._get_start_or_end_from_attendance(hour_to, date_to.date(), employee)
values['request_date_from'], values['request_date_to'] = values['date_from'].date(), values['date_to'].date()
values['number_of_days'] = employee_leave_date_duration[(date_from, date_to)][values['employee_id']]['days']

I guess it's necessary and there is no method that would make it shorted.

addons/hr_holidays/models/hr_leave.py Outdated Show resolved Hide resolved
addons/hr_holidays/models/hr_leave.py Outdated Show resolved Hide resolved
@taqu-odoo taqu-odoo force-pushed the 16.0-opw-3703793-company_time_off_duration-taqu branch from b462f94 to a75f9fd Compare April 23, 2024 13:03
@taqu-odoo taqu-odoo marked this pull request as ready for review April 23, 2024 13:03
@C3POdoo C3POdoo requested a review from a team April 23, 2024 13:08
@taqu-odoo
Copy link
Contributor Author

It seems to me we could also calculate the number of days of a leave by passing to _get_number_of_days_batch the start of the first day and the end of the last day, but not too sure about that.

Copy link
Contributor

@Bertrand2 Bertrand2 left a comment

Choose a reason for hiding this comment

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

What happens if the leave is for half a day/specific hours? Won't that create a whole day leave when creating the individual leaves?

@taqu-odoo taqu-odoo force-pushed the 16.0-opw-3703793-company_time_off_duration-taqu branch from a75f9fd to 56896d6 Compare April 25, 2024 11:32
@taqu-odoo
Copy link
Contributor Author

I've modified the PR. I think conceptually it makes sense to consider the whole day as a leave if we have different schedules, let me know what you think.

@Bertrand2
Copy link
Contributor

This leaves the initial issue for half-days 🤔 Isn't there any way to just compute date_from and date_to based from the UX fields?

@taqu-odoo taqu-odoo force-pushed the 16.0-opw-3703793-company_time_off_duration-taqu branch from 56896d6 to 029f696 Compare April 29, 2024 08:01
@taqu-odoo
Copy link
Contributor Author

It should now work for half days, the problem is that _get_number_of_days_batch doesn't always return exactly 0.5 for half days.

@Bertrand2
Copy link
Contributor

This doesn't work for half_days. The leave will indeed have a 0.5 days duration, but the date_from and date_to will cover the whole day.

@taqu-odoo taqu-odoo force-pushed the 16.0-opw-3703793-company_time_off_duration-taqu branch from 029f696 to f0fd6ab Compare May 6, 2024 11:37
@taqu-odoo
Copy link
Contributor Author

Hello @Bertrand2, date_from and date_to are now correct for leaves in half_days or in hours.

Copy link
Contributor

@Bertrand2 Bertrand2 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! 😄
I still have some more comments but we're getting there 💪

addons/hr_holidays/models/hr_leave.py Outdated Show resolved Hide resolved
addons/hr_holidays/models/hr_leave.py Outdated Show resolved Hide resolved
addons/hr_holidays/models/hr_leave.py Show resolved Hide resolved
addons/hr_holidays/tests/test_company_leave.py Outdated Show resolved Hide resolved
@taqu-odoo taqu-odoo force-pushed the 16.0-opw-3703793-company_time_off_duration-taqu branch from f0fd6ab to d6ef25b Compare May 10, 2024 08:15
@taqu-odoo
Copy link
Contributor Author

Thanks for all the suggestions 👍 Changes made.

@taqu-odoo taqu-odoo force-pushed the 16.0-opw-3703793-company_time_off_duration-taqu branch from d6ef25b to a6dcb66 Compare May 14, 2024 08:27
@taqu-odoo
Copy link
Contributor Author

fixed 👍

Copy link
Contributor

@Bertrand2 Bertrand2 left a comment

Choose a reason for hiding this comment

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

Just one last precision over what I meant for the tests, otherwise LGTM

@taqu-odoo taqu-odoo force-pushed the 16.0-opw-3703793-company_time_off_duration-taqu branch from a6dcb66 to e5fd8ee Compare May 15, 2024 14:37
Steps
-----
- Time off installed
- Put an employee on a different schedule than the company's
default one and make him for example start earlier.
- Create a new Time Off type, set "Requires allocation" to
No Limit.
- Approvals > Time off > New and create a time off (mode "by
company") on the day where the employee is starting earlier.
- Approve it.
- Approvals > Time off : the leave's duration for the employee
on a different schedule is incorrect (slightly less than 1 day).

Issue
-----
When creating a leave on a company level, a leave will first be
created with start and end hours based on the company's default
schedule.
When approving the leave, a leave is then created for each
affected employee. The date_from and date_to received by
create are based on the company's default schedule. This is not
generally problematic as it is taken into account. For example,
the date_from and date_to of the employee's leave are correct
even if he is on a different schedule.

However, when we compute the number of days, it is not accounted for.
https://github.com/odoo/odoo/blob/64cbe389e698398eee93ebde9c61b2ee79756380/addons/hr_holidays/models/hr_leave.py#L918
https://github.com/odoo/odoo/blob/64cbe389e698398eee93ebde9c61b2ee79756380/addons/hr_holidays/models/hr_leave.py#L744
As a result, the hours worked by an employee on a different schedule
which are "outside" of the company's default schedule don't count
towards the calculation.
https://github.com/odoo/odoo/blob/3eec4d6a69ff76cbf750b960dc8db2eb5e0a45f9/addons/resource/models/resource_mixin.py#L94-101

Note: the issue is the same for leaves given to a department or an
employee tag since they are created the same way.

Fix
-----
Use the whole day as a basis for leave duration calculation if there
is more than one schedule for the employees. Also add more logic to
handle half days and hours leaves which had incorrect start and end
hours.

opw-3703793
@taqu-odoo taqu-odoo force-pushed the 16.0-opw-3703793-company_time_off_duration-taqu branch from e5fd8ee to 8bfd00e Compare May 15, 2024 15:11
@taqu-odoo
Copy link
Contributor Author

Changed 🙂 Thank you for the help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OE the report is linked to a support ticket (opw-...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants