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

[10.0] [ADD] hr_payroll_account_operating_unit #82

Conversation

serpentcs-dev1
Copy link
Member

HR Payroll Account with Operating Units

This module introduces the following features:

  • Adds Operating Unit (OU) to the account moves and its lines created by the payslip, based on the Operating Unit (OU) defined in the Employee's Contract.

@nikul-serpentcs
Copy link
Member

@aheficent Could you please review code.

Copy link
Contributor

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only code review 😐

for slip in self:
# Check that all slips are related to contracts
# that belong to the same OU.
if OU:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't compare directly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is comparing the OU in contracts between the Payslip. I don't see the use case for this though.

Btw, what changes do you want me to make?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if there is a way to map the contracts by different OU and if there was more than one raise the error. But I'm not really sure and this works. It's ok to me.

@api.multi
def write(self, vals):
res = super(HrPayslip, self).write(vals)
if 'move_id' in vals and vals['move_id']:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use vals.get('move_id', False) but it's the same

# that belong to the same OU.
if OU:
if slip.contract_id.operating_unit_id.id != OU:
raise UserError(_('Configuration error!\nThe Contracts must\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The \n is not needed just close the quotes and continue in the next line

slip.contract_id.operating_unit_id.id})
if slip.move_id.line_ids:
slip.move_id.line_ids.\
write({'operating_unit_id':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't be better to include this in the account_move class?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get you?! Do you want me to move this whole thing to while account_move are created?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In v9 it was done by passing the operating unit in the context and then overriding the create method in the account_move. But maybe this is good so leave it like this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, Thanks

@AaronHForgeFlow
Copy link
Contributor

Code looks now. We have to wait to the dependencies are merged.

@AaronHForgeFlow
Copy link
Contributor

Superseded by #127

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants