[MIG][17.0] product_configurator: Migration to 17.0 #141
Conversation
|
@bizzappdev are you taking over #135 ? |
Yes, We have started working on this MR for the migration. |
|
/ocabot migration product_configurator |
bizzappdev
force-pushed
the
17.0-mig-product_configurator-BAD
branch
5 times, most recently
from
232f351 to
e5a6322
Compare
|
|
||
| # Process each `field_vals` operation for the current attribute | ||
| for field_vals in vals.get(field_name, []): | ||
| if field_vals and field_vals[0] == Command.SET: |
There was a problem hiding this comment.
Don't we need to take care of other Commands, too (e.g: CLEAR)
There was a problem hiding this comment.
Those are not triggered from the onchange web client, so even if we add code for those cases, we will not have a proper way to reproduce and test them.
There was a problem hiding this comment.
I see. It's a bit fragile, though.
If, for some reason, Odoo refactors the onchange and starts outputting other commands, this would fail.
I wonder if we can use the field's convert_to_cache method instead. (many examples in odoo code)
That method offers the advantage of not having to worry about the internals of onchange nor commands
There was a problem hiding this comment.
Indeed, the optimal way to use convert_to_cache is to use this logic for the dynamic fields that are not present in _fields, which will restrict us from using direct convert_to_cache.
One possibility is to have a field instance via add_field, but I am not sure it is wiser to create a field instance just to use convert_to_cache.
There was a problem hiding this comment.
I see. Thanks for taking the time to explain all this.
| elif field_vals and field_vals[0] == Command.UNLINK: | ||
| if field_vals[1] in final_val: | ||
| final_val.remove(field_vals[1]) | ||
|
|
There was a problem hiding this comment.
Seems some of these cases aren't covered by unit tests.
Would it be possible to test them?
There was a problem hiding this comment.
We will take care of the test cases in later stages.
| local_domain_prefix = field_names and field_names[0].startswith( | ||
| domain_field_prefix | ||
| ) | ||
| if field_type == list and ( |
There was a problem hiding this comment.
The previous lines of code kind of assume the field_names is a list
(with things like field_names and field_names[0].startswith)
So, can it be something different than list or not?
Either this condition is redundant, or the previous code might fail
Looking at the core methods, it seems it's safe to assume it's always going to be a list, and so this condition can be removed
For simplicity and readability, I'd do something like this:
regular_field_names = (
fname for fname in field_names
if not any(fname.startswith(prefix) for prefix in self._prefixes.values())
)
if regular_field_names:There was a problem hiding this comment.
In odoo v17.0, it will return the list of fields changed from the web client. In the case of form view, even if it is always one field, it is still a list of a field.
There was a problem hiding this comment.
Yes, that's my point.
Checking if field_type == list is redundant then :)
bizzappdev
force-pushed
the
17.0-mig-product_configurator-BAD
branch
from
e5a6322 to
3d27245
Compare
|
|
||
| # Process each `field_vals` operation for the current attribute | ||
| for field_vals in vals.get(field_name, []): | ||
| if field_vals and field_vals[0] == Command.SET: |
There was a problem hiding this comment.
I see. It's a bit fragile, though.
If, for some reason, Odoo refactors the onchange and starts outputting other commands, this would fail.
I wonder if we can use the field's convert_to_cache method instead. (many examples in odoo code)
That method offers the advantage of not having to worry about the internals of onchange nor commands
| local_domain_prefix = field_names and field_names[0].startswith( | ||
| domain_field_prefix | ||
| ) | ||
| if field_type == list and ( |
There was a problem hiding this comment.
Yes, that's my point.
Checking if field_type == list is redundant then :)
bizzappdev
force-pushed
the
17.0-mig-product_configurator-BAD
branch
from
3d27245 to
0d3ade8
Compare
bizzappdev
force-pushed
the
17.0-mig-product_configurator-BAD
branch
from
0d3ade8 to
e78af0c
Compare
|
@ivantodorovich After your approval, we have made some changes to the code to make it compatible and generic with the website_product_configurator module. |
|
/ocabot merge nobump |
|
Hey, thanks for contributing! Proceeding to merge this for you. |
|
Congratulations, your PR was merged at 356cbb9. Thanks a lot for contributing to OCA. ❤️ |
No description provided.