Skip to content
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

[16.0] [IMP] Make packaging required on orders for configured products. #3528

Draft
wants to merge 2 commits into
base: 16.0
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl-3.0)
from freezegun import freeze_time

from odoo.exceptions import UserError
from odoo.fields import Command
from odoo.tests import tagged
from odoo.tests.common import Form
Expand Down Expand Up @@ -234,3 +235,16 @@
},
],
)

def test_product_require_packaging(self):
"""Test that products with the required packaging cannot be sold
separated without packaging."""
so_f = Form(self.env["sale.order"])
so_f.partner_id = self.partner
self.prod_1.require_packaging = True
with so_f.order_line.new() as line_f:
line_f.product_id = self.prod_1
self.assertTrue(line_f.require_packaging)
with self.assertRaises(UserError):
line_f.product_uom_qty = 1
so_f.save()

Check warning on line 250 in sale_order_product_recommendation_packaging_default/tests/test_recommendation.py

View check run for this annotation

Codecov / codecov/patch

sale_order_product_recommendation_packaging_default/tests/test_recommendation.py#L250

Added line #L250 was not covered by tests
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl-3.0)


from odoo import api, fields, models
from odoo import _, api, fields, models
from odoo.exceptions import UserError


class SaleOrderRecommendationLine(models.TransientModel):
Expand All @@ -26,6 +27,7 @@
digits="Product Unit of Measure",
help="Quantity of packagings to sell.",
)
require_packaging = fields.Boolean(compute="_compute_require_packaging")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: you can use a readonly related field instead, which is essentially the same:

Suggested change
require_packaging = fields.Boolean(compute="_compute_require_packaging")
require_packaging = fields.Boolean(related="product_id.require_packaging", readonly=True)


@api.depends("product_id", "units_included", "sale_uom_id")
def _compute_product_packaging(self):
Expand Down Expand Up @@ -109,3 +111,22 @@
# Nothing to create
return result
return self._prepare_packaging_line_vals(result)

@api.depends("product_id")
def _compute_require_packaging(self):
for line in self:
line.require_packaging = line.product_id.require_packaging

Check warning on line 118 in sale_order_product_recommendation_packaging_default/wizards/sale_order_recommendation.py

View check run for this annotation

Codecov / codecov/patch

sale_order_product_recommendation_packaging_default/wizards/sale_order_recommendation.py#L118

Added line #L118 was not covered by tests
Comment on lines +114 to +118
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: if using a related field, you don't really need to add a compute method.

Suggested change
@api.depends("product_id")
def _compute_require_packaging(self):
for line in self:
line.require_packaging = line.product_id.require_packaging


@api.depends("product_id", "product_uom_qty", "product_uom")
def _warning_product_packaging_required(self):
Comment on lines +120 to +121
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a constraint instead:

Suggested change
@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):

"""Notify the user when a packaging is required."""
result = {}

Check warning on line 123 in sale_order_product_recommendation_packaging_default/wizards/sale_order_recommendation.py

View check run for this annotation

Codecov / codecov/patch

sale_order_product_recommendation_packaging_default/wizards/sale_order_recommendation.py#L123

Added line #L123 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: no need for a result while constraining.

Suggested change
result = {}

for line in self:
if line.require_packaging and not line.product_packaging_id:
raise UserError(

Check warning on line 126 in sale_order_product_recommendation_packaging_default/wizards/sale_order_recommendation.py

View check run for this annotation

Codecov / codecov/patch

sale_order_product_recommendation_packaging_default/wizards/sale_order_recommendation.py#L126

Added line #L126 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: within constrains, you should raise ValidationErrors:

Suggested change
raise UserError(
raise ValidationError(

_(
"The product %s requires a packaging to be sold"
% line.product_id.name
Comment on lines +128 to +129
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: you must render the message before composing it for translations to work fine:

Suggested change
"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

)
)
return result

Check warning on line 132 in sale_order_product_recommendation_packaging_default/wizards/sale_order_recommendation.py

View check run for this annotation

Codecov / codecov/patch

sale_order_product_recommendation_packaging_default/wizards/sale_order_recommendation.py#L132

Added line #L132 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

continuing from above...

Suggested change
return result

Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,14 @@
ref="sale_order_product_recommendation.sale_order_recommendation_view_form"
/>
<field name="arch" type="xml">

<xpath
expr="//field[@name='line_ids']/form//field[@name='units_included']"
position="before"
>
<field name="require_packaging" invisible="1" />
</xpath>

<xpath
expr="//field[@name='line_ids']/tree/field[@name='units_included']"
position="before"
Expand All @@ -36,12 +44,13 @@
name="product_packaging_id"
groups="product.group_stock_packaging"
context="{'default_product_id': product_id}"
attrs="{'required': [('require_packaging', '=', True)]}"
/>
<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)]}"
/>
Comment on lines 49 to 54
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</xpath>
</field>
Expand Down
5 changes: 5 additions & 0 deletions sale_packaging_default/README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ To configure this module, you need to:
1. Go to *Sales > Configuration > Settings*.
2. Under *Product Catalog*, enable *Product Packagings*.

To configure products for mandatory packaging:

1. Go to \*Inventory > Product and select a product.
2. Under the ‘Inventory’ tab check the box for ‘Require Packaging'.

Usage
=====

Expand Down
1 change: 1 addition & 0 deletions sale_packaging_default/__manifest__.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"installable": True,
"depends": ["sale"],
"data": [
"views/product_template_view.xml",
"views/sale_order_view.xml",
],
}
1 change: 1 addition & 0 deletions sale_packaging_default/models/__init__.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
from . import product_packaging
from . import product_template
from . import sale_order_line
9 changes: 9 additions & 0 deletions sale_packaging_default/models/product_template.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Copyright 2024 Moduon Team S.L.
# License LGPL-3.0 or later (https://www.gnu.org/licenses/lgpl-3.0)
from odoo import fields, models


class ProductTemplate(models.Model):
_inherit = "product.template"

require_packaging = fields.Boolean(default=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: add some help and make sure you cannot require packaging without packaging:

Suggested change
require_packaging = fields.Boolean(default=False)
require_packaging = fields.Boolean(help="Activate it to require that the product is always sold in a complete packaging.")
@api.constrains("require_packaging", "packaging_ids")
def _check_packaging_required(self):
wrong = (self - self.filtered("packaging_ids")).filtered("require_packaging")
if wrong:
raise ValidationError(_("Products %s require packaging but have none", ", ".join(wrong.mapped("display_name"))))

27 changes: 26 additions & 1 deletion sale_packaging_default/models/sale_order_line.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@
# License LGPL-3.0 or later (https://www.gnu.org/licenses/lgpl-3.0)
from contextlib import suppress

from odoo import api, fields, models
from odoo import _, api, fields, models
from odoo.exceptions import UserError


class SaleOrderLine(models.Model):
_inherit = "sale.order.line"

require_packaging = fields.Boolean(compute="_compute_require_packaging")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use related instead

Suggested change
require_packaging = fields.Boolean(compute="_compute_require_packaging")
require_packaging = fields.Boolean(related="product_id.require_packaging", readonly=True)


def onchange(self, values, field_name, field_onchange):
"""Record which field was being changed."""
if isinstance(field_name, list):
Expand Down Expand Up @@ -43,6 +46,23 @@
and line.product_uom_qty % line.product_packaging_id.qty
):
line.product_packaging_id = False
res = line._warning_product_packaging_required()
if res:
return res

Check warning on line 51 in sale_packaging_default/models/sale_order_line.py

View check run for this annotation

Codecov / codecov/patch

sale_packaging_default/models/sale_order_line.py#L51

Added line #L51 was not covered by tests
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
Comment on lines +49 to 66
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: use a constrain instead, and I summarize all the comments from above here too:

Suggested change
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
)
)


@api.model
Expand Down Expand Up @@ -84,3 +104,8 @@
_self = self.with_context(keep_product_packaging=True)
result = super(SaleOrderLine, _self)._compute_product_uom_qty()
return result

@api.depends("product_id")
def _compute_require_packaging(self):
for line in self:
line.require_packaging = line.product_id.require_packaging
Comment on lines +107 to +111
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use related instead

Suggested change
@api.depends("product_id")
def _compute_require_packaging(self):
for line in self:
line.require_packaging = line.product_id.require_packaging

6 changes: 6 additions & 0 deletions sale_packaging_default/readme/CONFIGURE.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,9 @@ To configure this module, you need to:

1. Go to *Sales \> Configuration \> Settings*.
2. Under *Product Catalog*, enable *Product Packagings*.


To configure products for mandatory packaging:

1. Go to *Inventory \> Product and select a product.
2. Under the ‘Inventory’ tab check the box for ‘Require Packaging'.
5 changes: 5 additions & 0 deletions sale_packaging_default/static/description/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,11 @@ <h1><a class="toc-backref" href="#toc-entry-2">Configuration</a></h1>
<li>Go to <em>Sales &gt; Configuration &gt; Settings</em>.</li>
<li>Under <em>Product Catalog</em>, enable <em>Product Packagings</em>.</li>
</ol>
<p>To configure products for mandatory packaging:</p>
<ol class="arabic simple">
<li>Go to *Inventory &gt; Product and select a product.</li>
<li>Under the ‘Inventory’ tab check the box for ‘Require Packaging’.</li>
</ol>
</div>
<div class="section" id="usage">
<h1><a class="toc-backref" href="#toc-entry-3">Usage</a></h1>
Expand Down
14 changes: 14 additions & 0 deletions sale_packaging_default/tests/test_sale_packaging_default.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Copyright 2023 Moduon Team S.L.
# License LGPL-3.0 or later (https://www.gnu.org/licenses/lgpl-3.0)
from odoo import fields
from odoo.exceptions import UserError
from odoo.tests.common import Form

from odoo.addons.product.tests.common import ProductCommon
Expand Down Expand Up @@ -126,3 +127,16 @@
self.assertEqual(line_f.product_packaging_id, self.p2_three_pack)
self.assertEqual(line_f.product_packaging_qty, 10)
self.assertEqual(line_f.product_uom_qty, 30)

def test_product_require_packaging(self):
"""Test that products with the required packaging cannot be sold
separated without packaging."""
so_f = Form(self.env["sale.order"])
so_f.partner_id = self.partner
self.product2.require_packaging = True
with so_f.order_line.new() as line_f:
line_f.product_id = self.product2
self.assertTrue(line_f.require_packaging)
with self.assertRaises(UserError):
line_f.product_uom_qty = 1
so_f.save()

Check warning on line 142 in sale_packaging_default/tests/test_sale_packaging_default.py

View check run for this annotation

Codecov / codecov/patch

sale_packaging_default/tests/test_sale_packaging_default.py#L142

Added line #L142 was not covered by tests
22 changes: 22 additions & 0 deletions sale_packaging_default/views/product_template_view.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?xml version="1.0" encoding="utf-8" ?>
<!-- Copyright 2024 Moduon Team S.L. <[email protected]>
License LGPL-3.0 or later (https://www.gnu.org/licenses/lgpl). -->
<data>

<record id="product_template_form_view_inherit" model="ir.ui.view">
<field name="name">Simplify packaging fields</field>
<field name="model">product.template</field>
<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">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

<field
name="require_packaging"
placeholder="Indicates if the product can be sold individually or if it requires keeping the original packaging."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
placeholder="Indicates if the product can be sold individually or if it requires keeping the original packaging."

/>
</group>
</xpath>
</field>
</record>

</data>
27 changes: 27 additions & 0 deletions sale_packaging_default/views/sale_order_view.xml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,33 @@
position="move"
/>
</xpath>

<xpath
expr="//field[@name='order_line']/tree/field[@name='product_packaging_id']"
position="before"
>
<field name="require_packaging" invisible="1" />
</xpath>

<!-- Make packaging mandatory when the product is not allow to divide the packaging -->
<xpath
expr="//field[@name='order_line']/tree/field[@name='product_packaging_id']"
position="attributes"
>
<attribute
name="attrs"
>{'required': [('require_packaging', '=', True)]}</attribute>
</xpath>

<!-- 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>
Comment on lines +57 to +65
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: so, we require packaging, but we don't allow to set how many? It doesn't make sense:
image

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>
</record>
</data>
Loading