[11.0][MIG] hr_public_holidays > hr_holidays_public #465

MiquelRForgeFlow · 2018-07-10T13:51:29Z

Superseeds #401.

MiquelRForgeFlow · 2018-07-12T11:30:24Z

Hey everybody! something on Odoo changed that makes a test fail. Previously was all green some months ago in the commit of @feketemihai . My changes didn't affect the tests but the PR is red. I can't find what's the cause or what's wrong, so please, can someone help to fix this?

[BSSFL-556] Propagate lot from sale line to 3 delivery steps

astirpe

Thank you for this PR! Please check my comments for the code review.

I personally would prefer a pure porting of this module from https://github.com/OCA/hr/tree/10.0/hr_public_holidays to V11, without adding so much other stuff. The module version =< 10 was meant to be a technical module, useful for being extended for further customization. But with the latest PRs this module is becoming something else.

The calculation of the hours should be delegated to a specific module hr_holidays_hour, which migration to V11 is proposed here: #450.

Odoo V12 will have the code for the leave management almost completely rewritten, supporting also the hours calculation, so do we really want to spend so much effort to rewrite the hours management in this module, with the risk to restart all over in V12 (will be released next October 😮 )?

Anyway I will not block this PR if you have different needs, just think about it.

astirpe · 2018-07-30T12:49:58Z

hr_holidays_public/models/hr_holidays_public.py

+from datetime import date
+
+from odoo import api, fields, models, _
+from odoo.exceptions import Warning as UserError


Just use:
from odoo.exceptions import UserError

astirpe · 2018-07-30T12:53:02Z

hr_holidays_public/wizards/holidays_public_next_year_wizard.py

+import logging
+
+from odoo import api, models, fields, _
+from odoo.exceptions import Warning as UserError


from odoo.exceptions import UserError

astirpe · 2018-07-30T12:56:01Z

hr_holidays_public/tests/test_holidays_public.py

+# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html).
+
+from odoo.exceptions import ValidationError
+from odoo.exceptions import Warning as UserError


from odoo.exceptions import UserError, ValidationError

astirpe · 2018-07-30T13:00:31Z

hr_holidays_public/views/hr_holidays_public_view.xml

+            <field name="model">hr.holidays.public</field>
+            <field name="arch" type="xml">
+                <tree string="Public Holidays">
+                    <field name="year"/>


Could you replace <field name="year"/> with <field name="display_name"/> here?

hr_holidays_public/wizards/holidays_public_next_year_wizard.xml

+
+                <notebook>
+                    <page name="defaults" string="Defaults">
+                        <div>     


astirpe · 2018-07-30T13:23:21Z

hr_holidays_public/models/hr_holidays.py

+    show_full_days = fields.Boolean(
+        string="Show Full Days",
+        related="holiday_status_id.compute_full_days")
+    number_of_hours_temp = fields.Float(


Shouldn't this module inherit from hr_holidays_hour? The module hr_holidays_hour already defines those fields for managing the hours. There would be an overall simplification of code by inheriting from hr_holidays_hour, for example you could take advantage of methods for the conversion to hours, the onchanges and the views.

Migration of hr_holidays_hour to V11 is proposed here: #450.

Ok, I will wait then until that module is merged.

@astirpe I have been reviewing the module hr_holidays_hour and the dependency should be just the contrary: to add public holidays, we don't need to make the computation by hours first, but to adjust current Odoo core methods for obtaining the days in float. The hour one should go on top of this. I'm reviewing right now this one and proceed to merge for unlocking this, but I'm available to review any adjustment you need to make over this one for correctly pasting both together. I hope you understand.

…Management modules

[MIG] hr_public_holidays

[MIG] hr_public_holidays/hr_holidays_compute_days: port to 10.0

* [ADD] public holidays: add a wizard to create public holidays for the following year * [REF] odoolint improvements * [REF] adjustments for flake8 and pylint test * Move public holidays menu under Leaves menu. (OCA#290) * [ADJ] hr_public_holidays: take into account the need of several sub-menus * [ADD] Allow the creation of public holidays from a specific template for a specific year, and display a UserError for templates including February 29th. * [REF] removing useless comments * [ADJ] hr_public_holidays: Improving wizard display * [REF] Removing tab characters

* hr_public_holidays: use group 'hr_holidays.group_hr_holidays_manager' * hr_public_holidays: Update README.rst

Add correct calculation of holidays in hr_public_holidays, instead of hr_holidays_compute_days. Remove dependancy of contracts. Add unlink at onchnage of public holiday lines. Update code. Fix flake. Update code, add calculation in hours, update views. Update flake. Update calendar creation with no attendances, otherwise default values were set. Remove config of show days/hours. Update flake. Update readme. Rename module. Updated holiday reports. Update klake and pylint. Update flaket. Update hr_holidays_views.xml Add readonly to show_full_days. Update code according with comments. Increase coverage. update flake8.

pedrobaeza · 2018-09-03T00:43:49Z

After been thinking about this a lot, this is the best option I have found:

Leave hr_holidays_public with the public holidays definition, the exclude_public_holidays field in leave types, and the computation of the days intervals according this definition. This will be transparent for other modules not depending on this one, as the working intervals won't have public holidays and they are able to handle this interval days the way they want.
Rescue hr_holidays_compute_days module for adding day selection helpers (full day selectors for example) and exclude_rest_days and compute_full_days options on leave types, touching the working days computation the way it's done right now in this PR (although avoiding full method overriding).
hr_holidays_hour can plug together with this in 2 ways:
- Putting a hook in this second module and depending of it for not repeating computations.
- Being totally isolated, but repeating some of the computations.

I'm implementing this tomorrow. What do you think?

astirpe · 2018-09-03T08:11:11Z

@pedrobaeza I would prefer to keep hr_holidays_hour independent from other modules, since the idea is to keep it as a technical module that replaces the days with hours. I understand that we prefer to avoid repetition of code, but in this case I don't see any real benefit in doing that.

pedrobaeza · 2018-09-03T08:13:11Z

OK, no problem, as stated, you can do it repeating some of the code. Just take a look for not screwing up inheritance for allowing the installation of the 3 of them.

pedrobaeza · 2018-09-03T10:09:31Z

@mreficent I'm doing the other changes, but thanks for the lint fixup.

Hours part will be in module hr_holidays_hour.

* Squash commits into a single one. * Update tests with old onchange methods. * Fix variable name in tests.. * Update tests. * Update flake8. * Update security rules so that employee can read his schedule and contract, and hr manager to have the same rigths as defined in hr_contract. * Improve onchange employee, add tests for onchange. * Fix flake, whitespaces.

[MIG] hr_public_holidays/hr_holidays_compute_days: port to 10.0

the name collision between _compute_number_of_days defined in this addon and the method for the field number_of_days computation in the base addon hr_holidays caused the latter to not be called => the field number_of_days would be 0 for holidays allocation, and have the wrong sign for holidays request (because it was forced in the the reimplemented version. This patch: * adds tests for the expected value of number_of_days * adds a test for the behavior of the method for holidays allocation * renames the method _compute_number_of_days to _recompute_number_of_days

* To not depend on hr_contract. Working times are set also in employee, and v11 rely on this data, not in contract working time. This simplifies also the code. * To not include all the checks while selecting start or end leave date. At the end, it's the same for you: what counts is the number of leave days. As the compute algorithm already handles overlapping leaves, rest days and public holidays, there's no problem with this. * It handles fraction of days!! New algorithm put the part of the working day that you are leaving. There's a new option at leave type level for keeping the behavior of including full days. This option is checked by default also for keeping backwards compatibility. This code has been made similar to v11. * It includes UI helps for being better to select dates in the form view, allowing to select only dates instead of date + time. * The number of days is not manually modifiable by leave users, but to leave managers. If this behavior is not desirable, you can manually add other groups to the view, as this won't be overwritten with module update.

pedrobaeza · 2018-09-03T23:35:20Z

I have finished the adaptation, reducing the code to the minimal and decoupling all modules from each other. I think it has been a good job 😃

For handling module renaming in OpenUpgrade, there's OCA/OpenUpgrade#1509

astirpe

Review of module hr_holidays_public only

astirpe · 2018-09-04T06:59:10Z

hr_holidays_public/views/hr_holidays_public_view.xml

+                <form string="Public Holidays">
+                    <group>
+                        <field name="year"/>
+                        <field name="country_id"/>


We already have a module that extends this one. Would be nice if you write this block this way (to ease the extensions):

<group> <group name="name"> <field name="year"/> <field name="country_id"/> </group> <group name="extra"> </group> </group>

astirpe · 2018-09-04T07:03:20Z

hr_holidays_public/wizards/holidays_public_next_year_wizard.xml

+
+<odoo>
+
+<record id="holidays_public_next_year_wizard_view" model="ir.ui.view">


Could you indent the records more to the right, making this xml consistent with the other views?

astirpe · 2018-09-04T07:08:43Z

hr_holidays_public/models/hr_holidays_public.py

+        'state_id',
+        'Related States'
+    )
+    calendar_leave_id = fields.Many2one(


Why this field was added?

astirpe · 2018-09-04T07:29:41Z

hr_holidays_public/models/hr_holidays.py

+            )
+        else:
+            obj = self
+        return super(HrHolidays, obj,)._get_number_of_days(


Could you remove the trailing comma after obj?

pedrobaeza · 2018-09-04T08:24:42Z

Hehe, you have just reviewed the untouched part. Checking the comments...

Reduce to the minimum the footprint of this module, adding only public holidays as leaves in a transparent way by other modules.

This has been totally reworked for decoupling parts, not depending now on hr_holidays_public, and making transparent its use for compatibility with other modules.

pedrobaeza · 2018-09-04T08:32:31Z

Changes done

astirpe · 2018-09-04T08:38:01Z

Thanks!

We are not using hr_holidays_compute_days, so I think is better to let someone else (more interested) doing the functional review. I could do a code review later when I will have more time.

Module hr_holidays_public 👍

pedrobaeza · 2018-09-04T09:05:27Z

I think we are going to move forward, and if any problem, we can fill new PRs. This is done for not interfering with your module, so I don't think you will find troubles.

sambarros pushed a commit to sambarros/hr that referenced this pull request Jul 26, 2018

Merge pull request OCA#465 from TelmoSenseFly/BSSFL-556

3cb02cf

[BSSFL-556] Propagate lot from sale line to 3 delivery steps

astirpe suggested changes Jul 30, 2018

View reviewed changes

pedrobaeza mentioned this pull request Aug 7, 2018

[MIG] hr_public_holiday 10.0 to 11.0 #471

Closed

pedrobaeza added this to the 11.0 milestone Aug 7, 2018

pedrobaeza mentioned this pull request Aug 7, 2018

Migration to version 11.0 #374

Closed

71 tasks

pedrobaeza mentioned this pull request Sep 2, 2018

[MIG] [11.0] Migrate hr_public-holidays to 11.0 #401

Closed

pedrobaeza changed the title ~~[11.0][MIG] hr_public_holidays~~ [11.0][MIG] hr_public_holidays > hr_holidays_public Sep 2, 2018

pedrobaeza force-pushed the 11.0-mig-hr_public_holidays branch 2 times, most recently from 9d3b5bf to 4b61577 Compare September 2, 2018 18:16

miketelahun and others added 14 commits September 2, 2018 20:19

Employee and Contract Management, Attendance and Scheduling, Payroll …

0d4561c

…Management modules

[FIX] PEP8

ac1417f

Initialize 8.0 branch

d6f75f8

[MOV] move addons out of __unported__ (they remain not installable)

0fd94b0

Update Hr public holidays.

3647d02

[MIG] hr_public_holidays

OCA Transbot updated translations from Transifex

a069e92

[MIG] Make modules uninstallable

7ff8dd1

[FIX] remove en.po that was erroneously created by transbot

7099c0b

[MIG] Rename manifest files

a2bdbff

[10.0][MIG] hr_holidays_compute_days (OCA#261)

07099bd

[MIG] hr_public_holidays/hr_holidays_compute_days: port to 10.0

Move public holidays menu under Leaves menu. (OCA#290)

0016aac

[10.0] Small improvement to hr_public_holidays (OCA#346)

7e7bb44

* hr_public_holidays: use group 'hr_holidays.group_hr_holidays_manager' * hr_public_holidays: Update README.rst

pedrobaeza force-pushed the 11.0-mig-hr_public_holidays branch from 4b61577 to ff3a6c4 Compare September 2, 2018 18:19

pedrobaeza mentioned this pull request Sep 3, 2018

[11.0][MIG] hr_holidays_hour #450

Merged

MiquelRForgeFlow and others added 10 commits September 3, 2018 21:12

[FIX] hr_holidays_public: Review comments

4e418d4

[FIX+IMP] hr_holidays_public: Strip hours part + complete migration

c3b13ad

Hours part will be in module hr_holidays_hour.

OCA Transbot updated translations from Transifex

8ec7f0e

[10.0][MIG] hr_holidays_compute_days (OCA#261)

53760a0

[MIG] hr_public_holidays/hr_holidays_compute_days: port to 10.0

OCA Transbot updated translations from Transifex

29282a7

Fix _onchange_employee() arguments

b1845b4

[UPD] Update hr_holidays_compute_days.pot

9c8c48d

pedrobaeza force-pushed the 11.0-mig-hr_public_holidays branch from 97e9593 to bec5b57 Compare September 3, 2018 23:26

astirpe suggested changes Sep 4, 2018

View reviewed changes

pedrobaeza force-pushed the 11.0-mig-hr_public_holidays branch from bec5b57 to e40d5a2 Compare September 4, 2018 08:30

pedrobaeza added 2 commits September 4, 2018 10:31

[IMP] hr_holidays_public: Leave only public holidays computation

f9b814c

Reduce to the minimum the footprint of this module, adding only public holidays as leaves in a transparent way by other modules.

[MIG] hr_holidays_compute_days: Adapt to 11.0 + minimize

f9f51f9

This has been totally reworked for decoupling parts, not depending now on hr_holidays_public, and making transparent its use for compatibility with other modules.

pedrobaeza force-pushed the 11.0-mig-hr_public_holidays branch from e40d5a2 to f9f51f9 Compare September 4, 2018 08:31

pedrobaeza merged commit adac20a into OCA:11.0 Sep 4, 2018

MiquelRForgeFlow deleted the 11.0-mig-hr_public_holidays branch September 4, 2018 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[11.0][MIG] hr_public_holidays > hr_holidays_public #465

[11.0][MIG] hr_public_holidays > hr_holidays_public #465

MiquelRForgeFlow commented Jul 10, 2018 •

edited by pedrobaeza

Loading

MiquelRForgeFlow commented Jul 12, 2018

astirpe left a comment

astirpe Jul 30, 2018

astirpe Jul 30, 2018

astirpe Jul 30, 2018

astirpe Jul 30, 2018

This comment was marked as resolved.

astirpe Jul 30, 2018

MiquelRForgeFlow Aug 3, 2018 •

edited

Loading

pedrobaeza Sep 2, 2018

pedrobaeza commented Sep 3, 2018

astirpe commented Sep 3, 2018

pedrobaeza commented Sep 3, 2018

pedrobaeza commented Sep 3, 2018

pedrobaeza commented Sep 3, 2018

astirpe left a comment

astirpe Sep 4, 2018

astirpe Sep 4, 2018

astirpe Sep 4, 2018

pedrobaeza Sep 4, 2018

astirpe Sep 4, 2018

pedrobaeza commented Sep 4, 2018

pedrobaeza commented Sep 4, 2018

astirpe commented Sep 4, 2018

pedrobaeza commented Sep 4, 2018


		<odoo>

		<record id="holidays_public_next_year_wizard_view" model="ir.ui.view">

[11.0][MIG] hr_public_holidays > hr_holidays_public #465

[11.0][MIG] hr_public_holidays > hr_holidays_public #465

Conversation

MiquelRForgeFlow commented Jul 10, 2018 • edited by pedrobaeza Loading

MiquelRForgeFlow commented Jul 12, 2018

astirpe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

MiquelRForgeFlow Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobaeza commented Sep 3, 2018

astirpe commented Sep 3, 2018

pedrobaeza commented Sep 3, 2018

pedrobaeza commented Sep 3, 2018

pedrobaeza commented Sep 3, 2018

astirpe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobaeza commented Sep 4, 2018

pedrobaeza commented Sep 4, 2018

astirpe commented Sep 4, 2018

pedrobaeza commented Sep 4, 2018

MiquelRForgeFlow commented Jul 10, 2018 •

edited by pedrobaeza

Loading

MiquelRForgeFlow Aug 3, 2018 •

edited

Loading