-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
8.0 base field validator #246
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
Conversation
|
IMHO 👍 |
base_field_validator/README.rst
Outdated
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.
make the title a real title by adding a lot of '===' above the title OCA/maintainer-tools@d51c38f
|
@lmignon thanks. I have made some changes 😁 |
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.
Can you remove size param?
In v8 size don't is required
84e0520 to
e7df826
Compare
|
@moylop260 thanks, I made the suggested changes |
|
👍 |
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.
Maybe iterate directly over
self.env['ir.model.validator.line'].search(
[('name.model', '=', model_name),
('field_id.name', 'in', vals.keys())])
?
|
@StefanRijnhart thanks, improved with 50f50c3 |
|
Hi @StefanRijnhart can this be merged? |
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.
This check is now redundant, as we already filter on the field name in the search call.
|
Nearly! |
50f50c3 to
d196c92
Compare
|
@StefanRijnhart thanks, done |
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 @hbrunn mentioned recently in another PR, changing the signature of _register_hook can be dangerous as it is not compatible with other overrides of this method on the same model. https://github.com/OCA/server-tools/pull/300/files#r45976286. BTW @eLBati as the author of the module from that pull request you might want to read up on the general discussion there.
d196c92 to
a1e82ca
Compare
a1e82ca to
37380ba
Compare
|
@StefanRijnhart thanks for #340 |
|
|
||
| class IrModelValidatorLine(models.Model): | ||
| _name = "ir.model.validator.line" | ||
| name = fields.Many2one('ir.model', string="Model", required=True) |
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.
You should call this field model_id.
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, changed
|
In runbot, after I added the validator to the email field of res.partner, I got this: |
a603af8 to
cd60474
Compare
| return True | ||
|
|
||
|
|
||
| class IrModel(orm.Model): |
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.
You do not need to declare this twice. Old and new api methods can be freely mixed in the same models.Model subclass.
|
Finished code review, cannot test functionally due to lack of runbot. I see that in many PR recently, any clues? |
|
Thanks @yajo , I addressed your remarks. I don't know about runbot, you can try asking in #oca https://botbot.me/freenode/oca/ |
| _name = "ir.model.validator.line" | ||
| _rec_name = 'model_id' | ||
| model_id = fields.Many2one('ir.model', string="Model", required=True) | ||
| field_id = fields.Many2one('ir.model.fields', 'Field', required=True) |
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 still feel these 2 fields are redundant. model_id could be a related field to field_id.model_id.
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 can't see how to make the interface work using a related field.
Now runbot should work : https://runbot.odoo-community.org/runbot/build/3142519
Please try it there and tell me how you would change it.
Thanks
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 think yout do not need to replace the UI. Just add related='field_id.model_id', store=True to the model_id field. It should keep on working as now, but you would not need to fill 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.
In both cases user does not have to fill model_id field, as it is the inverse m2o of validator_line_ids and is not visible
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.
Yes, I suppose I would have strucutred it a bit differently, but current structure works fine and is not too far away of that, so 👍
|
👍 |
Syncing from upstream OCA/server-tools (10.0)
Following #238