Merge into lp:openerp-hr

Proposed by Nikolina Nikolova Todorova
Status: Rejected
Rejected by: Sandy Carter (
Proposed branch:
Merge into: lp:openerp-hr
Diff against target: 86 lines (+22/-2)
2 files modified
hr_public_holidays/ (+5/-1)
hr_public_holidays/hr_public_holidays_view.xml (+17/-1)
To merge this branch: bzr merge
Reviewer Review Type Date Requested Status
Sandy Carter ( Needs Resubmitting
Michael Telahun Makonnen Needs Fixing
Leonardo Pistone Abstain
Markus Schneider (community) Approve
Review via email:

Description of the change

In the module hr_public_holidays were added field 'country_id' in the 'hr.holidays.public', and field 'state_ids' in the 'hr.holidays.public.line'.
The reason is that we can have multi country companies and each country has different holidays as well as in some countries there are different holidays for each state.
By adding these fields we can configure different list of holidays for every country, and add list of states to every holiday.

To post a comment you must log in.
Revision history for this message
Markus Schneider (markus-schneider) wrote :

thak you, know we can handle holidays in germany correct

review: Approve
Revision history for this message
Leonardo Pistone (lepistone) wrote :

Hi Nikolina, Markus.

I did not make any tests, but that raised a few questions:

- Now we have those new fields, how will that behave will modules that depend on this one, like hr_holidays_extension from this same branch? For example, if those fields exist, one could expect them to be taken into account for the employee's schedule.

- How should national holidays be entered? No states or all of them?

- Without migration scripts, updating the module leaves country and states empty. Is that the intended behaviour?


review: Needs Information
Revision history for this message
Markus Schneider (markus-schneider) wrote :

Hi Leonardo,

our idea / interpretation is that "empty country" means "all countries" and "empty state" means "all states of selected country"

so no need to change existing data.

so we don't check all other modules in this repositorie, but only adding a new field and with this interpretation is no need for a change other modules at the moment. Only if you want to use the new version with country / state behavior, you can change the existing module but then you need to handle the data as well separately.

If you have only with one country and only national holiday it can stay if it is or we miss something?

For handling states holidays and national holidays we will have a module share soon (need some cleanup)

Revision history for this message
Leonardo Pistone (lepistone) wrote :

Dear Markus, Nikolina,

for "empty countries" meaning "all countries" it's ok for me, if you can say that on the help string of the field, for example. That way I agree with you migrations are not an issue.

As for the interactions other modules, I am not sure, I am abstaining so that others can speak.


review: Abstain
Revision history for this message
Markus Schneider (markus-schneider) wrote :

the following tree modules are depends on this:

- hr_holidays_extension
- hr_payroll_extension
- hr_payroll_period

Revision history for this message
Markus Schneider (markus-schneider) wrote :

as Michael Telahun Makonnen is the author of this addons i ask him to review our change.

We check the addons by our self and think that there should be no problem

Revision history for this message
Michael Telahun Makonnen (mmakonnen) wrote :

The basic idea seems good to me. However, as far as I can tell this patch only adds the necessary fields in the object. It doesn't make any use of them. For example, the current code (and all the modules that depend on it) assume that you will have the holidays list for only one country in the db. After this change, if you have holidays for multiple countries in the db the is_public_holiday() method will search the set of all holidays for all countries to give you an answer. Depending on your use case this may or may not be what you want.

I think this patch would be much more useful if you could update it and either modify is_public_holiday() and get_holidays_list() in a backwards-compatible manner to specify a combination of countries and states or provide new functions that do the same.

Once this patch is merged I will update the dependent modules as necessary.

review: Needs Fixing
Revision history for this message
Sandy Carter ( (sandy-carter) wrote :
review: Needs Resubmitting
Revision history for this message
Nikolina Nikolova Todorova (nikolina-todorova) wrote :

Hi Markus,

I made a github account with uname: ntodorova and my initos email.


Revision history for this message
Markus Schneider (markus-schneider) wrote :

We include the fix and resubmit in Github:

Unmerged revisions

83. By Nikolina Nikolova Todorova

change is_public_holiday and get_holidays_list function to handle the new country/state functionality

82. By Nikolina Nikolova Todorova

change sql_constraints to accept duplicated year if the country is different

81. By Nikolina Nikolova Todorova

add country_id and state_ids to the module

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'hr_public_holidays/'
2--- hr_public_holidays/ 2013-09-23 13:55:07 +0000
3+++ hr_public_holidays/ 2014-06-26 12:50:56 +0000
4@@ -3,6 +3,8 @@
5 #
6 # Copyright (C) 2011,2013 Michael Telahun Makonnen <>.
7 # All Rights Reserved.
8+# Copyright (C) 2014 initOS GmbH & Co. KG (<>).
9+# Author Nikolina Todorova <>
10 #
11 # This program is free software: you can redistribute it and/or modify
12 # it under the terms of the GNU Affero General Public License as published by
13@@ -32,13 +34,14 @@
14 _columns = {
15 'year': fields.char("calendar Year", required=True),
16 'line_ids': fields.one2many('hr.holidays.public.line', 'holidays_id', 'Holiday Dates'),
17+ 'country_id': fields.many2one('', 'Country'),
18 }
20 _rec_name = 'year'
21 _order = "year"
23 _sql_constraints = [
24- ('year_unique', 'UNIQUE(year)', _('Duplicate year!')),
25+ ('year_unique', 'UNIQUE(year,country_id)', _('Duplicate year and country!')),
26 ]
28 def is_public_holiday(self, cr, uid, dt, context=None):
29@@ -78,6 +81,7 @@
30 'date':'Date', required=True),
31 'holidays_id': fields.many2one('hr.holidays.public', 'Holiday Calendar Year'),
32 'variable': fields.boolean('Date may change'),
33+ 'state_ids': fields.many2many('', 'hr_holiday_public_state_rel', 'line_id', 'state_id', 'Related states')
34 }
36 _order = "date, name desc"
38=== modified file 'hr_public_holidays/hr_public_holidays_view.xml'
39--- hr_public_holidays/hr_public_holidays_view.xml 2013-08-12 09:07:07 +0000
40+++ hr_public_holidays/hr_public_holidays_view.xml 2014-06-26 12:50:56 +0000
41@@ -8,6 +8,7 @@
42 <field name="arch" type="xml">
43 <tree string="Public Holidays">
44 <field name="year"/>
45+ <field name="country_id"/>
46 </tree>
47 </field>
48 </record>
49@@ -19,6 +20,7 @@
50 <form string="Public Holidays">
51 <group>
52 <field name="year"/>
53+ <field name="country_id"/>
54 </group>
55 <newline/>
56 <group string="Public Holidays" colspan="4" col="1">
57@@ -32,14 +34,28 @@
58 <field name="name">hr.holidays.public.line.tree</field>
59 <field name="model">hr.holidays.public.line</field>
60 <field name="arch" type="xml">
61- <tree string="Public Holidays" editable="top">
62+ <tree string="Public Holidays">
63 <field name="date"/>
64 <field name="name"/>
65+ <field name="state_ids"/>
66 <field name="variable"/>
67 </tree>
68 </field>
69 </record>
71+ <record id="view_public_holiday_line_form" model="ir.ui.view">
72+ <field name="name">hr.holidays.public.line.form</field>
73+ <field name="model">hr.holidays.public.line</field>
74+ <field name="arch" type="xml">
75+ <form string="Public Holidays">
76+ <field name="date"/>
77+ <field name="name"/>
78+ <field name="holidays_id" invisible="1"/>
79+ <field name="state_ids" domain = "[('country_id','=',parent.country_id)]"/>
80+ </form>
81+ </field>
82+ </record>
84 <record id="open_public_holidays_view" model="ir.actions.act_window">
85 <field name="name">Public Holidays</field>
86 <field name="res_model">hr.holidays.public</field>