-
-
Notifications
You must be signed in to change notification settings - Fork 342
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
[MIG][11.0] hr_commission: Migration to 11.0 #174
Conversation
hr_commission/models/res_partner.py
Outdated
|
||
@api.constrains('agent_type', 'employee') | ||
@api.constrains('agent_type') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedrobaeza All field used by the constrains must be specified into @api.constrains
to trigger the constrains check if value change. employee_id
must be in `@api.constrains
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As that field is a computed non-stored value, it shouldn't be there, and a warning reminds you about that. This is how I saw it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Therefore you should add the field declare into the depends of the compute method...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the compute is precisely for computing that field...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code should look like:
@api.depends('user_ids')
def _compute_employee_id(self):
for partner in self:
if (len(partner.user_ids) == 1 and
len(partner.user_ids.employee_ids) == 1):
partner.employee_id = partner.user_ids.employee_ids
@api.constrains('agent_type', 'user_ids)
def _check_employee(self):
if self.agent_type == 'salesman' and not self.employee_id:
raise exceptions.ValidationError(
_("To be salesman there must be one (and only one) employee linked to this "
"partner. To do this, go to 'Human Resources' and check "
"'Employees'"))
If user_ids
is not into api.constrains
and you remove/modify the link between res.parner
and hr.employee
the constrains will not be triggered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointer. Now I know what you are talking about, but are you sure adding the one2many field to the constraint, it will be triggered on that case? I can develop a test case any way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but are you sure adding the one2many field to the constraint, it will be triggered on that case? I can develop a test case any way.
Not sure but a test will be welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried and it's not triggered, so I'm not sure about adding a lot of code in employee part just for that corner case. Do you see it so important to do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, yes it's important to avoid know bug.
8c419d1
to
256d11b
Compare
@lmignon I have added the code in hr.employee model for checking the condition and test the use case. |
256d11b
to
cb75970
Compare
@lmignon can you finish your review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add some extra description, maybe. But functionally is working fine 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works fine!
@Tecnativa