-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[12.0][MIG] onchange helper #1461
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
840c2b1 to
d8fd6ed
Compare
1ed501c to
68b0591
Compare
5c318a1 to
a132f3e
Compare
a132f3e to
a22f10e
Compare
lmignon
left a comment
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.
Thank you @sbejaoui LGTM (Code review only)
other improvement: set onchange_specs to all fields as _onchange_spec() retrun onchange fields for default view return field value if it's set in onchange_fields (usuful to get default value for a 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.
@lmignon , I pushed some changes. can you check them please.
|
@astirpe @sebastienbeau @florian-dacosta Could you take a look to the two last commits PLZ? |
056b070 to
424ae0a
Compare
424ae0a to
cf21042
Compare
onchange_helper/models/base.py
Outdated
| return { | ||
| f: v | ||
| for f, v in all_values.items() | ||
| if not self._fields[f].compute |
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.
@sbejaoui You should take a look at https://github.com/OCA/server-tools/pull/1484/files
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.
done, thanks :)
| # We get default values, they may be used in onchange | ||
| record_values = self.default_get(self._fields.keys()) | ||
| for field in self._fields: | ||
| if field not in all_values: |
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.
@sbejaoui Do you remember the motivations behind the lines from 55 to 56? IMO we must exclude non stored fields. I observe that if I put all the fields of my model, my non stored fields are recomputed even if not accessed by an onchange...
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.
We try to get values from existing record or defaults one to fill missing data in all_values passed to play_onchange.
server-tools/onchange_helper/models/base.py
Lines 43 to 50 in 176916f
| if self: | |
| self.ensure_one() | |
| record_values = self._convert_to_write( | |
| { | |
| field_name: self[field_name] | |
| for field_name, field in self._fields.items() | |
| } | |
| ) |
IMO,
field_name: self[field_name] trigger the compute process.
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 do we really need all values.... We should let the ORM compute the one required once accessed. In my case, these lines trigger the compute of useless field and cause performance issue.
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 we can exclude computed fields.
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've the same feeling.
|
Hi, thanks for your work. I have a use case that doesn't work with this code. Float fields with a value of 0.0 is evaluated to False and then is not returned to vals It's is not easy to reproduce with unit tests on res.partner. My case : I import csv of purchase line with only specified product_id and qty I want this onchange played called by onchange_product_id() vals = self.env['purchase.order.line'].play_onchanges(vals, ['product_id'])
I'll investigate deeper. |
|
@bealdav I'm refactoring this module in V10 to improve the performances... In the same time I'm adding a lot of tests. I've a corner case that doesn't pass the tests (recursive onchange on on2many relation). I'll also add a test for this UC to see if the new code is more robust. https://github.com/OCA/server-tools/pull/1510/files |
|
@lmignon thanks a lot |
njeudy
left a comment
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.
Just install and test on a copy of a prod database.
Ok for me it works as needed !
|
This PR has the |
|
@OCA/core-maintainers |
Syncing from upstream OCA/server-tools (15.0)
No description provided.