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

[16.0][IMP] hr_holidays_natural_period: exclude public holidays in natural days computation #149

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

Conversation

LauraCForgeFlow
Copy link

This improvement allows for the leave days computation in natural days to exclude public holidays.
It also removes the 'Half Day' checkbox when requesting a leave with a leave type that uses natural days.

@OCA-git-bot
Copy link
Contributor

Hi @victoralmau,
some modules you are maintaining are being modified, check this out!

@LauraCForgeFlow LauraCForgeFlow changed the title [IMP] hr_holidays_natural_period: exclude public holidays in natural days computation [16.0][IMP] hr_holidays_natural_period: exclude public holidays in natural days computation Nov 14, 2024
Copy link
Member

@victoralmau victoralmau left a comment

Choose a reason for hiding this comment

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

IMO this is not necessary, because you can define whether to exclude public holidays or not with https://github.com/OCA/hr-holidays/blob/16.0/hr_holidays_public/models/hr_leave_type.py#L11.

@LauraCForgeFlow
Copy link
Author

I noticed that even when excluding public holidays with the exclude_public_holidays field mentioned, the computation of the number of days in natural days did not properly exclude public holidays.

If we remove the changes in https://github.com/OCA/hr-holidays/pull/149/files#diff-ab3064ebe6a3018dc278f12ada4ba1e2e695dc08fef6a4ceb6f0c3c935b3ea24 and execute the tests, those that involve excluding public holidays fail (test_hr_leave_natural_day_public_holiday_02, test_hr_leave_natural_day_public_holiday_weekend_02).

However, with the changes introduced in this PR, all tests pass successfully.

@victoralmau
Copy link
Member

Excuse me, one important thing. The hr_holidays_natural_period module is intended to use natural days, therefore, public holidays should NOT be excluded; if the user selects Monday through Friday, if Wednesday is a holiday it should NOT be excluded (IMO). What do you think @pedrobaeza?

@pedrobaeza pedrobaeza added this to the 16.0 milestone Nov 15, 2024
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

As said by my colleague, the module was conceived to always compute natural days without excluding public holidays, as that's how all our companies that are using it work.

Anyway, the module can be extended to be compatible with such configuration. What is not correct is to change the current module dependency, nor adapting data through migration script. Due to both things, although we may do a "soft dependency" only injecting the context, that means the migration script and to trick the tests, I think the best to do in this case is that you create a glue module hr_holidays_natural_period_public that do the bridge, and install when you need natural periods, but excluding public holidays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants