[9.0][ADD] Module 'hr_holidays_leave_auto_approve':

[espo-tony]

This module allows the user to define a leave type in order to make the system
automatically approving all the leave requests (and leave allocation requests)
belonging to that leave type.

[elicoidal]

Thanks for the contribution!

[elicoidal]

@espo-tony Thanks for your contribution to the OCA.
Let me introduce to you some basic information about contribution in OCA and I will review other points in more comments inside the code..
In general, please follow the principles:

• Contribution guidelines: https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md
• Please follow the OCA README template:
  https://github.com/OCA/maintainer-tools/blob/master/template/module/README.rst
• We recommend to include a screenshot as well to help new comers (nice to have)
• Please use simple headers in all py files following OCA templates
  https://github.com/OCA/maintainer-tools/blob/master/template/module/__openerp__.py#L3

In particular here in the README you have to fix:

• the badge
• the title (line before, same length as title and line after)
• update URLs to https

[espo-tony]

Thanks for the informations. The readme file should now be consistent with the convention.

[elicoidal]

It is better to use enumerated list for procedures using "#. "
eg:
#. Go on the leave type configuration menu
#. Select the leave type you wish to setup
#. Mark the flag 'Auto Approve'.

[elicoidal]

suggestion: "If True, leaves belonging to this leave type will be automatically approved"

[espo-tony]

Thanks!

[gurneyalex]

nice and useful module. One change requested regarding the decorator on def create

[gurneyalex]

when redefining create() in the new API you also need to use the api.returns as is done in https://github.com/odoo/odoo/blob/9.0/openerp/models.py#L4118 otherwise you will break other addons extending the same model using the old api.

[espo-tony]

@gurneyalex , thanks for the review.
I've just committed the requested change.

[astirpe]

Could be

[astirpe]

id must appear before model

[espo-tony]

@gurneyalex , @elicoidal

Please, can you give a look to my last commits?
If they are satisfying maybe it's also possible to approve the changes and remove the label 'needs fixing'?
Thank you in advance.

[damdam-s]

thanks

[pedrobaeza]

As @elicoidal's concerns are covered, I merge.

[pedrobaeza]

Uhm, please squash your commits as there are a lot of merge operations before I merge.

[feketemihai]

@pedrobaeza you can squash directly when merging...

[pedrobaeza]

I know, but being 2 contributors I can't do it without losing one of them. That's why I ask them to do it preserving each author's part

[Garamotte]

Please avoid using python's reserved names (type here) as variable name.

Also, you should use directly use result.holiday_status_id instead of values.get('holiday_status_id'), which is the real data written on the record, because some methods called by the super could have changed the values.
Moreover, this will directly give you the recordset, without the need to call browse, and you can safely read fields on this without testing if it exists.

Finally, you can rewrite this part this way :

result = super(HrHolidays, self).create(values) 
if result.holiday_status_id.auto_approve:
    result.sudo().holiday_validate()
return result

[espo-tony]

@sylvain-garancher ,

You're perfectly right. Thanks for the hint.

[Garamotte]

holidays_validate is a method called by the workflow.
Please trigger workflow processes, instead of calling the method directly.

Also, are you sure this always works ? The admin user always have rights to call holidays_validate ?
How about checking the auto_approve status in the holidays_validate method (or workflow condition) instead ?
This way, you don't need sudo, so the write_uid will be consistent.

[espo-tony]

@sylvain-garancher ,
About using the workflow process you are right. I fixed this in the latest commit.

About using sudo:
Unfortunately, relying on the workflow mechanism makes impossible to validate a leave request if the user is not at least an HR officer (group: base.group_hr_user). That's because the workflow transition from the state confirm when a signal validate is received is defined as:

<record model="workflow.transition" id="holiday_confirm2validate"> <!-- 2. submitted->accepted (validate signal) if not double_validation-->
        <field name="act_from" ref="act_confirm" />
        <field name="act_to" ref="act_validate" />
        <field name="signal">validate</field>
        <field name="condition">not double_validation</field>
        <field name="group_id" ref="base.group_hr_user"/>

A user not belonging to the group base.group_hr_user is not allowed to "see" this transition, therefore its signal doesn't trigger anything. A normal user is not allowed to approve a leave request and that can't (and shouldn't) be bypassed.

Apart from that, using sudo (which conceptually means designating the system rather than the user as validator) seems more correct to me. Indeed it's the system and not the requesting user to approve the leave request, following a specific configuration setted on the leave type by an HR officer. In my opinion, the requesting user shouldn't absolutely be involved into the approval process (e.g.: the message tracking the state change shouldn't be authored by the user).

[astirpe]

@pedrobaeza You can squash this PR without mentioning my name as committing contributor. My name is already present in the contributors list of the readme. Thank you!

[pedrobaeza]

Please remember to squash commits

[Garamotte]

When the admin user doesn't have the base.group_hr_user group, automatic approve doesn't work : the workflow raises the "Contact a human resource manager" error.

I think it would be better to :

• Add a new workflow transition from confirm to validate, with the base.group_user group (employee), holiday_status_id.auto_approve as condition, and no signal.
• Override the _check_state_access_right, to allow changing the state to validate if the defined holiday_status_id allows auto approve. You don't have the record in this method, but when called from create, you will always have the holiday_status_id field.

By doing this :

• You won't need sudo (group employee) : No access rights issue, consistent write_uid, etc.
• You won't need to call signal_workflow (no signal) : No need to override the create method.

[espo-tony]

Just overriding the _check_state_access_right to build some logic dependent on holiday_status_id is not going to work, as this method is called also in the write and usually we don't have access to holiday_status_id into vals, when called by a write.
Now, we are sure that during the process of auto approval the write is called several times and in particular when the state is changed by the workflow transition. When this happens, as we have state into values and we don't have any way to access holiday_status_id, in case we're using a user without base.group_hr_user group, the system will always raise the error.

In addition, in my opinion it's a conceptual mistake to use the requesting user to validate: it's not really the user that is validating the request, the system does it. For me it's much more coherent that fields such as write_uid, but especially the state-change track message, report that was the system to approve the request and not the user himself.

In my opinion, we should rather look at this functionality like at a kind of automatic action with immediate trigger upon the creation. Let's imagine that instead than implementing the auto-approval functionality into the create we decided to configure a cron job in order to change the state of those leave requests, not approved yet, that belongs to an auto-approval leave type. In this case the write would be done by the admin user and it would be considered correct.

Following these thoughts I consider more correct to solve the issue incurring when the admin user doesn't have the base.group_hr_user group as I did in my latest commit. Please, give a look at it.

[Garamotte]

For the _check_state_access_right from write, as the transition should be automatic from create, I think this is not an issue. You can check if the holiday_status_id is in the values, and simply call super instead.

For the user who performs the approve, and the write_uid related concerns, we simply have a different point of view.
Personnaly, I prefer creating a new workflow transition (which, with the group and the condition, is clearly declared for this), than using sudo. But your override of the _check_state_access_right to allow admin change the state in any case is another option, both works as expected.

Any other opinion ? @pedrobaeza @gurneyalex ?

[espo-tony]

Even if the transition was automatically triggered from create, this would always call a write passing as only value the state. That's because the function holidays_validate (called by the workflow transition) is defined as following:

    def holidays_validate(self, cr, uid, ids, context=None):
        obj_emp = self.pool.get('hr.employee')
        ids2 = obj_emp.search(cr, uid, [('user_id', '=', uid)])
        manager = ids2 and ids2[0] or False
        self.write(cr, uid, ids, {'state': 'validate'}, context=context)
        ...

So, unless we don't define a brand new method to be invoked from the new workflow transition in order to perform the state change, this will always be an issue. In my opinion it's clear that defining and using a method different from holidays_validate is not a good idea, as it would fork the standard workflow and break the inheritance chain.

[Garamotte]

Oh, ok, I see, I forgot that :)
So, let's continue with the current behaviour, thanks for the explanation.

[pedrobaeza]

OK, now I can finally merge if you squash the commits 😉

[espo-tony]

@pedrobaeza
I've performed the squash. Thank you.

[pedrobaeza]

hehe, you've finally remove all commits except one. That's something I can do directly with GitHub, but there were 2 authors. That's why I asked you to squash. Anyway, I'll finally merge for not delaying this more.