-
-
Notifications
You must be signed in to change notification settings - Fork 110
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
[15.0][IMP] hr_holidays_public: include public holidays in employee's _get_unusual_days #150
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code review 👍🏼
Not sure unusual days is for that. cc @victoralmau |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not agree with this change. Although the _get_unusual_days()
https://github.com/odoo/odoo/blob/3a28e5b0adbb36bdb1155a6854cdfbe4e7f9b187/addons/hr/models/hr_employee.py#L442 method perhaps does not clarify much, it is important to explain what is currently happening in the dashboard with the different options:
- Unusual days based on the employee's work schedule: they are shown in gray color.
- Time Off days: The overlay event is shown on the days that correspond.
- Public holidays: The same event is shown as for time off days.
IMO I think it is clear that public holidays should be treated as holidays and not as non-working days.
Ok, my use case is that all public holidays are non-working days across the company, and in that case it made sense to show it as non working day for all, but I understand that is not the most "generic" case. I am closing this then and move the feature to another place. Thank you for the review. |
Sorry, I didn't agree with adding this as a general thing, maybe being something per company (disabled by default) with |
Even at company level, public holidays may vary depending on the employee locations, so I don't think this should be added this way, even with a configuration switch. |
This is useful in the time off overview calendar, from:
to:
cc @ForgeFlow