[16.0] [IMP] Make packaging required on orders for configured products. #3528
Conversation
| @api.depends("product_id", "product_uom_qty", "product_uom") | ||
| def _warning_product_packaging_required(self): |
There was a problem hiding this comment.
This is a constraint instead:
| @api.depends("product_id", "product_uom_qty", "product_uom") | |
| def _warning_product_packaging_required(self): | |
| @api.constrains("product_id", "product_packaging_id") | |
| def _check_product_packaging_required(self): |
| result = {} | ||
| for line in self: | ||
| if line.require_packaging and not line.product_packaging_id: | ||
| raise UserError( |
There was a problem hiding this comment.
issue: within constrains, you should raise ValidationErrors:
| raise UserError( | |
| raise ValidationError( |
| "The product %s requires a packaging to be sold" | ||
| % line.product_id.name |
There was a problem hiding this comment.
issue: you must render the message before composing it for translations to work fine:
| "The product %s requires a packaging to be sold" | |
| % line.product_id.name | |
| "The product %s requires a packaging to be sold", | |
| line.product_id.name |
| @api.depends("product_id", "product_uom_qty", "product_uom") | ||
| def _warning_product_packaging_required(self): | ||
| """Notify the user when a packaging is required.""" | ||
| result = {} |
There was a problem hiding this comment.
issue: no need for a result while constraining.
| result = {} |
| % line.product_id.name | ||
| ) | ||
| ) | ||
| return result |
There was a problem hiding this comment.
continuing from above...
| return result |
| res = line._warning_product_packaging_required() | ||
| if res: | ||
| return res | ||
| return result | ||
|
|
||
| @api.depends("product_id", "product_uom_qty", "product_uom") | ||
| def _warning_product_packaging_required(self): | ||
| """Notify the user when a packaging is required.""" | ||
| result = {} | ||
| for line in self: | ||
| if line.require_packaging and not line.product_packaging_id: | ||
| raise UserError( | ||
| _( | ||
| "The product %s requires a packaging to be sold" | ||
| % line.product_id.name | ||
| ) | ||
| ) | ||
| return result |
There was a problem hiding this comment.
issue: use a constrain instead, and I summarize all the comments from above here too:
| res = line._warning_product_packaging_required() | |
| if res: | |
| return res | |
| return result | |
| @api.depends("product_id", "product_uom_qty", "product_uom") | |
| def _warning_product_packaging_required(self): | |
| """Notify the user when a packaging is required.""" | |
| result = {} | |
| for line in self: | |
| if line.require_packaging and not line.product_packaging_id: | |
| raise UserError( | |
| _( | |
| "The product %s requires a packaging to be sold" | |
| % line.product_id.name | |
| ) | |
| ) | |
| return result | |
| @api.constrains("product_id", "product_packaging_id") | |
| def _check_product_packaging_required(self): | |
| """Notify the user when a packaging is required.""" | |
| for line in self: | |
| if line.require_packaging and not line.product_packaging_id: | |
| raise ValidationError( | |
| _( | |
| "The product %s requires a packaging to be sold", | |
| line.product_id.name | |
| ) | |
| ) |
| <group string="Packaging Division"> | ||
| <field | ||
| name="require_packaging" | ||
| placeholder="Indicates if the product can be sold individually or if it requires keeping the original packaging." |
There was a problem hiding this comment.
issue: a placeholder doesn't make sense if the input is a checkbox. I already commented above about adding help to the field, so this is unnecessary.
| placeholder="Indicates if the product can be sold individually or if it requires keeping the original packaging." |
| <field name="inherit_id" ref="product.product_template_form_view" /> | ||
| <field name="arch" type="xml"> | ||
| <xpath expr="//page[@name='inventory']" position="inside"> | ||
| <group string="Packaging Division"> |
There was a problem hiding this comment.
issue: since you're creating a new group, it can land far away from the packaging configuration. Better, put the new checkbox within the same visual group as the rest of packaging configuration:
| <!-- Make packaging qty readonly when the product is not allow to divide the packaging --> | ||
| <xpath | ||
| expr="//field[@name='order_line']/tree/field[@name='product_packaging_qty']" | ||
| position="attributes" | ||
| > | ||
| <attribute | ||
| name="attrs" | ||
| >{'readonly': [('require_packaging', '=', True)]}</attribute> | ||
| </xpath> |
There was a problem hiding this comment.
issue: so, we require packaging, but we don't allow to set how many? It doesn't make sense:
Instead, please do the opposite thing. The field that you should make readonly is product_uom_qty. That way, the user is not allowed to break the packaging. They're only allowed to increase or decrease whole packaging sets.
| <field | ||
| name="product_packaging_qty" | ||
| groups="product.group_stock_packaging" | ||
| widget="numeric_step" | ||
| attrs="{'readonly': [('product_packaging_id', '=', False)]}" | ||
| attrs="{'readonly': ['|', ('product_packaging_id', '=', False), ('require_packaging', '=', True)]}" | ||
| /> |
There was a problem hiding this comment.
issue: just like below, this field should be writable always when there's a packaging. But instead, do the opposite and make the product_uom_qty field readonly when require_packaging == True.
|
I think the best approach will be using the same workflow as Odoo does: On product.categoryAdd a selection field as
The workflow will be the same as the |
|
Since those fields exist already, would it make more sense to reuse them,
instead of creating new ones?
… Message ID: ***@***.***>
|
|
FYI, this change in 19.0 will totally impact you: odoo/odoo#184131 |
Could be interesting to force commercial users to sell entire packages, but maybe in certain cases allow to send partial packages... |
There was a problem hiding this comment.
We already have something similar in v14.0 https://github.com/OCA/sale-workflow/tree/14.0/sale_by_packaging
I would migrate it to 16.0 instead.
|
Hello @mmequignon , Thank you so much for your feedback and for pointing us to the The main difference lies in flexibility. Your module revolves around strictly selling by packaging, while our use case is much more dynamic and flexible. 🚀 Here’s what we mean:
For our clients, this flexibility is critical, and unfortunately, the Additionally, we’ve noticed that your module introduces extra complexity and dependencies that are unnecessary for our use case, such as:
If we’re mistaken here, we’d love to see more clarity! 🙏 When migrating the module, it would be super helpful if you could improve the README and even record a short video 🖥️🎥 explaining how to handle these use cases:
If your module already covers all these scenarios, we’d be delighted to reconsider it! 😊💡 Thanks again for your input, and let us know your thoughts! Best regards, |
This PR allows a configured product not to be allowed to be sold without packaging.
This video explains the improvement
https://www.loom.com/share/b4b6a6fc9cdb406086115195a1eeb1c4
@moduon: MT-8489