Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] hr_holidays: reintroduce no_validation leave_type #30806

Closed
wants to merge 1 commit into
base: saas-12.1
from

Conversation

Projects
None yet
5 participants
@RomainLibert
Copy link
Contributor

RomainLibert commented Feb 4, 2019

Since 5995680#diff-ab6382882079a64a861bbba2741a1075 it was impossible to use the no_validation
on hr_leave_type.

This commit reintroduces this possibility and ensures the leave user gets
an actvity for the leave

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@RomainLibert RomainLibert added RD HR labels Feb 4, 2019

@RomainLibert RomainLibert requested a review from tivisse Feb 4, 2019

@robodoo robodoo added the seen 🙂 label Feb 4, 2019

@RomainLibert RomainLibert force-pushed the odoo-dev:saas-12.1-fix-hr-holidays-notification-rli branch 3 times, most recently Feb 4, 2019

@robodoo robodoo added the CI 🤖 label Feb 4, 2019

@RomainLibert RomainLibert force-pushed the odoo-dev:saas-12.1-fix-hr-holidays-notification-rli branch Feb 8, 2019

@robodoo robodoo removed the CI 🤖 label Feb 8, 2019

@RomainLibert RomainLibert force-pushed the odoo-dev:saas-12.1-fix-hr-holidays-notification-rli branch Feb 8, 2019

@robodoo robodoo added the CI 🤖 label Feb 8, 2019

@RomainLibert RomainLibert force-pushed the odoo-dev:saas-12.1-fix-hr-holidays-notification-rli branch Feb 14, 2019

@robodoo robodoo removed the CI 🤖 label Feb 14, 2019

@RomainLibert RomainLibert force-pushed the odoo-dev:saas-12.1-fix-hr-holidays-notification-rli branch Feb 14, 2019

@robodoo robodoo added the CI 🤖 label Feb 14, 2019

@RomainLibert RomainLibert force-pushed the odoo-dev:saas-12.1-fix-hr-holidays-notification-rli branch Feb 22, 2019

@robodoo robodoo removed the CI 🤖 label Feb 22, 2019

@odony odony requested a review from tbe-odoo Feb 22, 2019

@RomainLibert RomainLibert force-pushed the odoo-dev:saas-12.1-fix-hr-holidays-notification-rli branch 2 times, most recently Feb 22, 2019

@tbe-odoo
Copy link
Contributor

tbe-odoo left a comment

Hello, I think this PR need some changes regarding the method to check if a user can approve or not a leave.

addons/hr_holidays/models/hr_leave.py Outdated
elif self.state == 'confirm' or (self.state == 'validate' and self.validation_type == 'no_validation'):
if self.employee_id.leave_manager_id:
responsible = self.employee_id.leave_manager_id
if self.employee_id.parent_id.user_id:

This comment has been minimized.

@tbe-odoo

tbe-odoo Feb 22, 2019

Contributor
Suggested change
if self.employee_id.parent_id.user_id:
elif self.employee_id.parent_id.user_id:
addons/hr_holidays/models/hr_leave.py Outdated
responsible = self.employee_id.leave_manager_id
if self.employee_id.parent_id.user_id:
responsible = self.employee_id.parent_id.user_id
if self.department_id.manager_id.user_id:

This comment has been minimized.

@tbe-odoo

tbe-odoo Feb 22, 2019

Contributor
Suggested change
if self.department_id.manager_id.user_id:
elif self.department_id.manager_id.user_id:
@@ -49,8 +49,7 @@
<field name="time_type" groups="base.group_no_one"/>
</group>
<group name="validation" string="Validation">
<field name="validation_type" widget="radio" invisible="1"/>
<field name="double_validation"/>
<field name="validation_type" widget="radio"/>

This comment has been minimized.

@tbe-odoo

tbe-odoo Feb 22, 2019

Contributor

Removing double_validation from a the view isn't a stable change but since we are the only one using this version it's ok.

We just need to remember it at the deployment.

This comment has been minimized.

@RomainLibert

RomainLibert Feb 22, 2019

Author Contributor

I wasn't sure if removing a field from a view was stable or not, I'll make it invisible instead of removing it

This comment has been minimized.

@tbe-odoo

tbe-odoo Feb 22, 2019

Contributor

Well even if you make it invisible in the view, it will cause issue since the field doesn't exist anymore, people will have to do a module upgrade to avoid having issues.

But as I said, don't bother yourself we are the only one using this version, so we will pay attention when deploying it.

addons/hr_holidays/models/hr_leave.py Outdated
@@ -754,25 +756,35 @@ def _check_approval_update(self, state):

if (state == 'validate1' and val_type == 'both') or (state == 'validate' and val_type == 'manager'):
manager = holiday.employee_id.parent_id or holiday.employee_id.department_id.manager_id
if (manager and manager != current_employee) and not self.env.user.has_group('hr_holidays.group_hr_holidays_manager'):
if not (holiday.employee_id.leave_manager_id and holiday.employee_id.leave_manager_id == self.env.user and is_team_leader) and (manager and manager != current_employee) and not is_manager:

This comment has been minimized.

@tbe-odoo

tbe-odoo Feb 22, 2019

Contributor

I haven't tested but it looks like this condition will not work as intended.
By that I mean that you have to be both "leave manager" and be the employee's leave manager.

So if you are manager but not the leave manager of this employee you will not be able to approve it.

I suggest to rewrite the whole method to take in account all the cases (for example if the current user has multiple employees, weird case but we will cover it).

Here is an example, that I haven't run once, but it should help to understand what I mean

def _check_approval_update(self, state):
    """ Check if target state is achievable. """
    # retrieve user's access rights
    is_team_leader = self.env.user.has_group('hr_holidays.group_hr_holidays_team_leader')
    is_officer = self.env.user.has_group('hr_holidays.group_hr_holidays_user')
    is_manager = self.env.user.has_group('hr_holidays.group_hr_holidays_manager')

    # you need to be at least team leader to approve leaves
    if not is_team_leader:
        raise UserError(_('Only a Team Leader, Leave Officer or Manager can approve or refuse leave requests.'))

    # retrieve current user linked employees
    user_employee_ids = self.env['hr.employee'].search([('user_id', '=', self.env.uid)])
    # loop through all the holidays to confirm
    for holiday in self:
        val_type = holiday.holiday_status_id.validation_type
        if state == 'confirm':
            continue

        if state == 'draft':
            if not is_manager and holiday.employee_id not in user_employee_ids:
                raise UserError(_('Only a Leave Manager can reset other people leaves.'))
            continue

        # check the record rules
        holiday.check_access_rule('write')
        # only managers can approve their own leaves
        if holiday.employee_id in user_employee_ids and not is_manager:
            raise UserError(_('Only a Leave Manager can approve its own requests.'))        

        if (state, val_type) in [('validate1', 'both'), ('validate', 'manager')]:
            is_employee_manager_id = (holiday.employee_id.parent_id or holiday.employee_id.department_id.manager_id) in user_employee_ids
            is_employee_leave_manager_id = holiday.employee_id.leave_manager_id == self.env.user
            # if the user has only team leader rights but this is also checked by a record rule
            if not (is_employee_manager_id or is_employee_leave_manager_id) and not is_officer:
                raise UserError(_('You must be either %s\'s manager or Leave officer to approve this leave') % (holiday.employee_id.name))

        if state == 'validate' and val_type == 'both':
            if not is_manager:
                raise UserError(_('Only an Leave Manager can apply the second approval on leave requests.'))

And by the way, some cases here do not need to be checked with python code, record rules can do the job since we call holiday.check_access_rule('write')

This comment has been minimized.

@RomainLibert

RomainLibert Feb 22, 2019

Author Contributor

I'll have a look at these in more details

responsible = self.env.user

if self.validation_type == 'hr' or (self.validation_type == 'both' and self.state == 'validate1'):
responsible = self.env['res.users'].search([

This comment has been minimized.

@tbe-odoo

tbe-odoo Feb 22, 2019

Contributor

I find this really really weird to select someone randomly

This comment has been minimized.

@RomainLibert

RomainLibert Feb 22, 2019

Author Contributor

Yes I agree with you, this more than just weird this is just crappy

This feature is something we do in master using a field to specify who is the responsible directly on the leave type, but it seems that for some reason we only saw that this feature was apparently absolutely essential and that we could not live without it any longer so the basic plan is to implement the master's feature in saas-12.1.

Indeed trying to implement this feature in a stable version makes it that much harder since we cannot use a field (rightfully) hence the idea to just take someone at random and maybe at some point hardcode an id in internal.
Which still seems like a bad idea to me, especially if the wait is only of about 5 months ...

This comment has been minimized.

@tbe-odoo

tbe-odoo Feb 22, 2019

Contributor

Yup, I don't see any other way to introduce this feature correctly in a stable version

This comment has been minimized.

@alexey-pelykh

alexey-pelykh Feb 22, 2019

Contributor

Completely offtopic to the PR yet about leaves: at the same time there's an accrual leave allocation that uses create_date of entry as when employee started working - if to talk about sanity ( ͡° ͜ʖ ͡°)

@RomainLibert RomainLibert force-pushed the odoo-dev:saas-12.1-fix-hr-holidays-notification-rli branch 3 times, most recently Feb 22, 2019

[FIX] hr_holidays: reintroduce no_validation leave_type
Since 5995680#diff-ab6382882079a64a861bbba2741a1075 it was impossible to use the no_validation
on hr_leave_type.

This commit reintroduces this possibility and ensures the leave user gets
an actvity for the leave

@tivisse tivisse force-pushed the odoo-dev:saas-12.1-fix-hr-holidays-notification-rli branch to cbc17fd Feb 25, 2019

@tivisse

This comment has been minimized.

Copy link
Contributor

tivisse commented Feb 25, 2019

@robodoo robodoo added the r+ 👌 label Feb 25, 2019

if state == 'confirm':
continue
elif state == 'draft':
if holiday.employee_id != current_employee:

This comment has been minimized.

@tbe-odoo

tbe-odoo Feb 25, 2019

Contributor

Officers should be able too

This comment has been minimized.

@RomainLibert

RomainLibert Feb 25, 2019

Author Contributor

According to the docstring he shouldn't
And that's what was previously implemented

if state == 'draft':
    if holiday.employee_id != current_employee and not is_manager:
        raise UserError(_('Only a Leave Manager can reset other people leaves.'))
    continue
@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 25, 2019

Staging failed: ci/runbot on ac79694cf236e35ea9c30ce09e0a0bf9c2c0b9ab (view more at http://runbot.odoo.com/runbot/build/469001)

@RomainLibert

This comment has been minimized.

Copy link
Contributor Author

RomainLibert commented Feb 26, 2019

@robodoo retry

@robodoo robodoo added CI 🤖 r+ 👌 and removed error 🙅 labels Feb 26, 2019

robodoo pushed a commit that referenced this pull request Feb 26, 2019

[FIX] hr_holidays: reintroduce no_validation leave_type
Since 5995680#diff-ab6382882079a64a861bbba2741a1075 it was impossible to use the no_validation
on hr_leave_type.

This commit reintroduces this possibility and ensures the leave user gets
an actvity for the leave

closes #30806
@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 26, 2019

Merged, thanks!

@robodoo robodoo closed this Feb 26, 2019

@RomainLibert RomainLibert deleted the odoo-dev:saas-12.1-fix-hr-holidays-notification-rli branch Feb 28, 2019

@RomainLibert RomainLibert restored the odoo-dev:saas-12.1-fix-hr-holidays-notification-rli branch Feb 28, 2019

@RomainLibert RomainLibert deleted the odoo-dev:saas-12.1-fix-hr-holidays-notification-rli branch Feb 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.