Merge lp:~initos.com/openerp-hr/7.0-change-public-holiday into lp:openerp-hr

[Nikolina Nikolova Todorova]

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.

[Markus Schneider (markus-schneider)]

thak you, know we can handle holidays in germany correct

review: Approve

[Leonardo Pistone (lepistone)]

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?

Thanks!

review: Needs Information

[Markus Schneider (markus-schneider)]

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)

[Leonardo Pistone (lepistone)]

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.

Thanks!

review: Abstain

[Markus Schneider (markus-schneider)]

the following tree modules are depends on this:

- hr_holidays_extension
- hr_payroll_extension
- hr_payroll_period

[Markus Schneider (markus-schneider)]

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

[Michael Telahun Makonnen (mmakonnen)]

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

[Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter)]

Please move this MP to https://github.com/OCA/hr

More info at https://github.com/OCA/maintainers-tools/wiki/How-to-move-a-Merge-Proposal-to-GitHub

review: Needs Resubmitting

[Nikolina Nikolova Todorova (nikolina-todorova)]

Hi Markus,

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

Greetings,
Niki

[Markus Schneider (markus-schneider)]

We include the fix and resubmit in Github:

https://github.com/OCA/hr/pull/12

Unmerged revisions

83. By Nikolina Nikolova Todorova on 2014-07-18

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

82. By Nikolina Nikolova Todorova on 2014-06-26

change sql_constraints to accept duplicated year if the country is different

81. By Nikolina Nikolova Todorova on 2014-06-26

add country_id and state_ids to the module

=== modified file 'hr_public_holidays/hr_public_holidays.py'
--- hr_public_holidays/hr_public_holidays.py	2013-09-23 13:55:07 +0000
+++ hr_public_holidays/hr_public_holidays.py	2014-06-26 12:50:56 +0000
@@ -3,6 +3,8 @@
 #
 #    Copyright (C) 2011,2013 Michael Telahun Makonnen <[e-mail]>.
 #    All Rights Reserved.
+#    Copyright (C) 2014 initOS GmbH & Co. KG (<http://www.initos.com>).
+#    Author Nikolina Todorova <[e-mail]>
 #
 #    This program is free software: you can redistribute it and/or modify
 #    it under the terms of the GNU Affero General Public License as published by
@@ -32,13 +34,14 @@
     _columns = {
         'year': fields.char("calendar Year", required=True),
         'line_ids': fields.one2many('hr.holidays.public.line', 'holidays_id', 'Holiday Dates'),
+        'country_id': fields.many2one('res.country', 'Country'),
     }
 
     _rec_name = 'year'
     _order = "year"
 
     _sql_constraints = [
-        ('year_unique', 'UNIQUE(year)', _('Duplicate year!')),
+        ('year_unique', 'UNIQUE(year,country_id)', _('Duplicate year and country!')),
     ]
 
     def is_public_holiday(self, cr, uid, dt, context=None):
@@ -78,6 +81,7 @@
         'date': fields.date('Date', required=True),
         'holidays_id': fields.many2one('hr.holidays.public', 'Holiday Calendar Year'),
         'variable': fields.boolean('Date may change'),
+        'state_ids': fields.many2many('res.country.state', 'hr_holiday_public_state_rel', 'line_id', 'state_id', 'Related states')
     }
 
     _order = "date, name desc"

=== modified file 'hr_public_holidays/hr_public_holidays_view.xml'
--- hr_public_holidays/hr_public_holidays_view.xml	2013-08-12 09:07:07 +0000
+++ hr_public_holidays/hr_public_holidays_view.xml	2014-06-26 12:50:56 +0000
@@ -8,6 +8,7 @@
             <field name="arch" type="xml">
                 <tree string="Public Holidays">
                     <field name="year"/>
+                    <field name="country_id"/>
                 </tree>
             </field>
         </record>
@@ -19,6 +20,7 @@
                 <form string="Public Holidays">
                     <group>
                         <field name="year"/>
+                        <field name="country_id"/>
                     </group>
                     <newline/>
                     <group string="Public Holidays" colspan="4" col="1">
@@ -32,14 +34,28 @@
             <field name="name">hr.holidays.public.line.tree</field>
             <field name="model">hr.holidays.public.line</field>
             <field name="arch" type="xml">
-                <tree string="Public Holidays" editable="top">
+                <tree string="Public Holidays">
                     <field name="date"/>
                     <field name="name"/>
+                    <field name="state_ids"/>
                     <field name="variable"/>
                 </tree>
             </field>
         </record>
         
+        <record id="view_public_holiday_line_form" model="ir.ui.view">
+            <field name="name">hr.holidays.public.line.form</field>
+            <field name="model">hr.holidays.public.line</field>
+            <field name="arch" type="xml">
+					<form string="Public Holidays">
+                        <field name="date"/>
+	                    <field name="name"/> 
+	                    <field name="holidays_id" invisible="1"/>
+	                    <field name="state_ids" domain = "[('country_id','=',parent.country_id)]"/>  
+                	</form>
+            </field>
+        </record>
+        
         <record id="open_public_holidays_view" model="ir.actions.act_window">
             <field name="name">Public Holidays</field>
             <field name="res_model">hr.holidays.public</field>