Optimized handling of variant creation and line item updating #30
Conversation
…product for exact the same configuration. Assigned the return value to variable „variant_exists“ for performance reasons. (To not run the comparision multiple times - for checking and returning)
If one or more attribute_values are changed, a new product.product should be created (or existing one is searched of course). Value product_id on sale_order_line_item is updated of course. Otherwise we are changing the product.product everywhere! Even on other sale orders, which is totaly messing it up!
|
|
||
| if self.product_id.attribute_value_ids.ids != self.value_ids.ids: | ||
| new_product = self.product_tmpl_id.create_variant(self.value_ids.ids, custom_vals) | ||
| self.order_line_id.write({'product_id': new_product.id}) |
There was a problem hiding this comment.
This indeed does make sense, if the attribute values are changed, there is a chance another variant exists with the same attr values and should be treated as a "new configuration"
However, the hook introduced by Richard "_extra_line_values" should be kept.
Another consideration would be changing the create_variant method name to get_variant or search_create_variant as this implies returning either an existing or a new variant. (To avoid confusion).
Lastly if a custom_value is searchable that means it should also generate a new variant but it's a bit more advanced and design might be changed to keep custom values only on order lines.
If you can handle the first two that would be great, if not i will do them myself.
There was a problem hiding this comment.
I didn´t realy get the point. Why can´t the hook be kept?
I can´t see any conflicts with my changes, sorry.
There was a problem hiding this comment.
Here https://github.com/pledra/odoo-product-configurator/pull/30/files#diff-dc0d573fe7a7ffdcfaac0b70bd2d0ff7L812 you remove e method introduced by Richard (self._extra_line_values).
This hook method is used to update the sale.order.line object with the line's name and also gives the user the ability to add extra values to the write.
So instead of writing directly to the order line and passing only product_id as you do here: https://github.com/pledra/odoo-product-configurator/pull/30/files#diff-dc0d573fe7a7ffdcfaac0b70bd2d0ff7R811
Best is to call the method just before self.unlink() with the same if condition that was removed and you can also add the product_id by default in the method
There was a problem hiding this comment.
My biggest concern here is the assumption that two are the same if all attributes are the same, not counting custom values. Our implementation sees custom values driving the manufacturing order, so different custom attributes do constitute a different variant.
This code alters the custom attributes on the variant, and so effectively alters every order which points to the variant.
I think this approach has merit but may already be handled better by the option introduced in #42
|
@svenrissmann let me know if you can make the updates or if I should take over the PR |
|
|
||
| if self.product_id.attribute_value_ids.ids != self.value_ids.ids: | ||
| new_product = self.product_tmpl_id.create_variant(self.value_ids.ids, custom_vals) | ||
| self.order_line_id.write({'product_id': new_product.id}) |
There was a problem hiding this comment.
Also a good idea if you're at it would be to rename create_variant to something more closer to it's new functionality that you just added.
Something like create_get_variant or get_variant?
There was a problem hiding this comment.
@richard-willdooit your input on this is appreciated
3 participants
I think my changes are really self-explaining.
I´ve tested them on current odoo 10.0 docker image (official)