[16.0][MIG] sale_order_line_packaging_qty

[carmenbianca]

Includes #2239

Internal task: https://gestion.coopiteasy.be/web#id=10668&action=475&active_id=486&model=project.task&view_type=form&menu_id=

[carmenbianca]

@bealdav ping :)

[carmenbianca]

product_packaging was renamed to product_packaging_id in Odoo 15 (and moved from sale_stock to sale).

product_packaging_id became a computed field in Odoo 16.

I was able to work with the former, but the latter implies a not-insignificant refactoring. I'll get back to this later.

[carmenbianca]

This may not make any sense, because product_packaging_id became a computed field in Odoo 16.

[carmenbianca]

You can't write to a computed field. For some reason this doesn't throw an error, but the behaviour here (and in other tests) is incorrect.

[bealdav]

Thanks for your deep check

[carmenbianca]

Writing down my findings before proceeding with a refactoring.

Here's the definition of the product_packaging_id field in sale :

    product_packaging_id = fields.Many2one(
        comodel_name='product.packaging',
        string="Packaging",
        compute='_compute_product_packaging_id',
        store=True, readonly=False, precompute=True,
        domain="[('sales', '=', True), ('product_id','=',product_id)]",
        check_company=True)

In spite of being a computed field, it is set readonly=False. This is unexpected. The Odoo section documentation reads as thus: "Computed fields are readonly by default. To allow setting values on a computed field, use the inverse parameter. It is the name of a function reversing the computation and setting the relevant fields".

The compute method is as follows:

    @api.depends('product_id', 'product_uom_qty', 'product_uom')
    def _compute_product_packaging_id(self):
        for line in self:
            # remove packaging if not match the product
            if line.product_packaging_id.product_id != line.product_id:
                line.product_packaging_id = False
            # Find biggest suitable packaging
            if line.product_id and line.product_uom_qty and line.product_uom:
                line.product_packaging_id = line.product_id.packaging_ids.filtered(
                    'sales')._find_suitable_product_packaging(line.product_uom_qty, line.product_uom) or line.product_packaging_id

Special attention to two lines. First, if line.product_packaging_id.product_id != line.product_id. It seems rather strange to me that a compute method can attempt to read the value of the field that it is trying to compute. Second, line.product_packaging_id = [...] or line.product_packaging_id. This one is especially amusing, because it (conditionally) sets the value it is trying to compute to itself.

Furthermore, this method exists in sale:

    @api.onchange('product_packaging_id')
    def _onchange_product_packaging_id(self):
        if self.product_packaging_id and self.product_uom_qty:
            newqty = self.product_packaging_id._check_qty(self.product_uom_qty, self.product_uom, "UP")
            if float_compare(newqty, self.product_uom_qty, precision_rounding=self.product_uom.rounding) != 0:
                return {
                    'warning': {
                        'title': _('Warning'),
                        'message': _(
                            "This product is packaged by %(pack_size).2f %(pack_name)s. You should sell %(quantity).2f %(unit)s.",
                            pack_size=self.product_packaging_id.qty,
                            pack_name=self.product_id.uom_id.name,
                            quantity=newqty,
                            unit=self.product_uom.name
                        ),
                    },
                }

I have to assume that the user can write to this computed field. The user-defined value is written to the database. But when the compute function is triggered, the user-defined value is (potentially) overwritten.

This matches with my functional testing. I created a product Foo which has packaging for 1 item, 5 items, and 100 items. When I type '10' into the sale order line quantity, it automagically sets the packaging quantity to 2 and the packaging to 'Five items'

Now I can click on 'Five items', and select 'One item'. This results in the packaging quantity being set to 10 (I assume because product_packaging_qty's computed field is triggered).

The above findings imply that this module may not need quite as much refactoring as I thought, because we can still write to the field, and still do onchange methods. I'll just need to validate that everything in this module still makes sense given the above context.

[carmenbianca]

Well this is awkward. I hadn't thought to check whether the functionality of this module already existed in Odoo.

Since Odoo 15, the identically named field product_packaging_qty exists on sale.order.line. Here is the compute function from Odoo 16:

    @api.depends('product_packaging_id', 'product_uom', 'product_uom_qty')
    def _compute_product_packaging_qty(self):
        for line in self:
            if not line.product_packaging_id:
                line.product_packaging_qty = False
            else:
                packaging_uom = line.product_packaging_id.product_uom_id
                packaging_uom_qty = line.product_uom._compute_quantity(line.product_uom_qty, packaging_uom)
                line.product_packaging_qty = float_round(
                    packaging_uom_qty / line.product_packaging_id.qty,
                    precision_rounding=packaging_uom.rounding)

Here is the compute function from this module (given the current state of the PR):

    @api.depends(
        "product_uom_qty",
        "product_uom",
        "product_packaging_id",
        "product_packaging_id.qty",
    )
    def _compute_product_packaging_qty(self):
        for sol in self:
            if (
                not sol.product_packaging_id
                or sol.product_uom_qty == 0
                or sol.product_packaging_id.qty == 0
            ):
                sol.product_packaging_qty = 0
                continue
            # Consider uom
            if sol.product_id.uom_id != sol.product_uom:
                product_qty = sol.product_uom._compute_quantity(
                    sol.product_uom_qty, sol.product_id.uom_id
                )
            else:
                product_qty = sol.product_uom_qty
            # Provide default values in case of missing product.
            # Odoo does not prevent you to modify product packaging
            # that are already linked to a sale order.
            # If by chance the product is removed (yes, you can do it)
            # this computation will be broken w/out this default.
            precision = sol.product_packaging_id.product_uom_id.rounding or 0.01
            qty_mod = float_round(
                product_qty % sol.product_packaging_id.qty,
                precision_rounding=precision,
            )
            # After the rounding, the value could be equals to
            # sol.product_packaging_id.qty.
            # So just re-apply the '%'
            qty_mod = qty_mod % sol.product_packaging_id.qty
            if not float_is_zero(
                qty_mod,
                precision_digits=precision,
            ):
                # If qty does not fit in package reset package qty
                sol.product_packaging_qty = 0
            else:
                # Maybe that product_packaging_qty should be an Integer, no ?
                qty = product_qty / sol.product_packaging_id.qty
                sol.product_packaging_qty = qty

These two methods are almost identical (although the latter is more verbose), barring a few implementation details:

• sale sets the value to False if there is no packaging. sale_order_line_packaging_qty sets to 0.
• sale_order_line_packaging_qty sets to 0 if the quantity of the product is not identical to or a multiple of the packaging's quantity. sale is more tolerant and sets to a rounded value.
• Maybe more implementation details? These are the big lines.

The question, then, is do we port this module to overwrite the default behaviour in sale, or do we instruct all modules that depend on this module to drop the dependency in Odoo 15/16 and make adjustments to be compatible with upstream Odoo's implementation details?

@bealdav

[bealdav]

Good catch, I haven't seen the odoo side.

[carmenbianca]

@grindtildeath Hi, you're the author of this module and the module I'm trying to port in #2570, which depends on this module.

Would you like to review the above comments? Specifically, my question is as follows, quoted from my last comment:

The question, then, is do we port this module to overwrite the default behaviour in sale, or do we instruct all modules that depend on this module to drop the dependency in Odoo 15/16 and make adjustments to be compatible with upstream Odoo's implementation details?

I am inclined to the latter. Let's not port this module, and let's make changes (if necessary) in #2570 to work with upstream Odoo sale. But I'm not comfortable with making that decision without passing it by the maintainers.

[grindtildeath]

Hello @carmenbianca

To me as well, this module is not needed anymore if there's a standard field in Odoo with the same purpose. AFAICS the fields exists in standard since v15.0 so #1919 should not be needed neither.

BTW, the documentation of the computed fields is lacking the last changes in ORM, where it's allowed to have a computed field that is writeable by setting readonly=False and store=True, without the need of an inverse function.
IMO it's actually a better alternative than using onchange to have such a field modified.

[jbaudoux]

FYI #2570 (comment)

[rafaelbn]

/ocabot migration sale_order_line_packaging_qty

[rafaelbn]

To me as well, this module is not needed anymore if there's a standard field in Odoo with the same purpose. AFAICS the fields exists in standard since v15.0 so #1919 should not be needed neither.

I agree, IMO this PR should closed.