[16.0] hr_holidays_public: Deprecate use of employee_id in favour of partner_id #110
Conversation
| # TODO: Drop function in next migration | ||
| employee = False | ||
| partner = False | ||
| if employee_id is not None: |
There was a problem hiding this comment.
what if you call this method with (False, False) ?
There was a problem hiding this comment.
From where it's called, it's supposed to have a value or be None, so no need to handle such a case
grindtildeath
force-pushed
the
16.0-imp-hr_holidays_partner_deprecate_employee
branch
from
586110e to
8e940f5
Compare
We need this for planning the shipments to the customers. Some (country) state have specific bank holidays and we need to plan transport accordingly |
|
Hello @pedrobaeza little ping to know if it can be merge :) |
There was a problem hiding this comment.
After analyzing this, it's a bit weird to have something prefixed with hr, but using a partner, but it's OK if you don't deprecate ever the employee_id argument. It may be needed because in any override, you may have some information on the employee that vary the result. Losing the argument, you won't be able to access to this info.
Helper functions to find public holidays did allow to use an hr.employee to filter country and states based on the employee address. Since only the address of the employee was used, modifying the functions to use a res.partner instead of an hr.employee allows more possibilities such as checking public holidays for customers and suppliers.
grindtildeath
force-pushed
the
16.0-imp-hr_holidays_partner_deprecate_employee
branch
from
8e940f5 to
42ceea6
Compare
|
@pedrobaeza I kept the possiblity to use IMO for future versions public holidays should be taken out of the hr module as it's not necessarily linked only to hr. We could have a calendar_holidays_public module and then we could bridge the gap to the hr keeping hr_holidays_public. |
|
@pedrobaeza Can you please update your review? |
|
What a great day to merge this nice PR. Let's do it! |
|
Congratulations, your PR was merged at cef403f. Thanks a lot for contributing to OCA. ❤️ |
Helper functions to find public holidays did allow to use an hr.employee to filter country and states based on the employee address.
Since only the address of the employee was used, modifying the functions to use a res.partner instead of an hr.employee allows more possibilities such as checking public holidays for customers and suppliers.