[FIX] mrp: mo creation with move_finished_ids without byproduct_id

When creating a new manufacturing order with `move_finished_ids`
passed in the values, but `byproduct_id` is not amongst the move values,
the creation of the manufacturing crashed.

```
  File "/home/odoo/src/odoo/master/addons/mrp/models/mrp_production.py", line 811, in create
    vals['move_finished_ids'] = list(filter(lambda move: move[2]['byproduct_id'] is False, vals['move_finished_ids']))
  File "/home/odoo/src/odoo/master/addons/mrp/models/mrp_production.py", line 811, in <lambda>
    vals['move_finished_ids'] = list(filter(lambda move: move[2]['byproduct_id'] is False, vals['move_finished_ids']))
KeyError: 'byproduct_id'
```

By reading the implementation of the byproduct moves field:
```py
    @api.depends('move_finished_ids')
    def _compute_move_byproduct_ids(self):
        for order in self:
            order.move_byproduct_ids = order.move_finished_ids.filtered(lambda m: m.product_id != order.product_id)
```
it seems a better implementation to filter the move on the fact their
product is different than the manufacturing order to determine whether
they are byproduct or finished product moves.

Take the opportunity to write a unit test testing the tricky behavior
of the create override when both `move_finished_ids` and `move_byproduct_ids`
are passed in the create values.

This is related to revision
odoo/odoo@6c087f2043

closes odoo/odoo#96044

Signed-off-by: Denis Ledoux (dle) <dle@odoo.com>

addons/mrp/models/mrp_production.py

@@ -807,9 +807,8 @@ class MrpProduction(models.Model):
         for vals in vals_list:
             # Remove from `move_finished_ids` the by-product moves and then move `move_byproduct_ids`
             # into `move_finished_ids` to avoid duplicate and inconsistency.
-            if vals.get('move_finished_ids', False):
-                vals['move_finished_ids'] = list(filter(lambda move: move[2]['byproduct_id'] is False, vals['move_finished_ids']))
-            if vals.get('move_byproduct_ids', False):
+            if vals.get('move_finished_ids', False) and vals.get('move_byproduct_ids', False):
+                vals['move_finished_ids'] = list(filter(lambda move: move[2]['product_id'] == vals['product_id'], vals['move_finished_ids']))
                 vals['move_finished_ids'] = vals.get('move_finished_ids', []) + vals['move_byproduct_ids']
                 del vals['move_byproduct_ids']
             if not vals.get('name', False) or vals['name'] == _('New'):

addons/mrp/tests/test_byproduct.py

@@ -132,3 +132,113 @@ class TestMrpByProduct(common.TransactionCase):
             'byproduct_ids': [(0, 0, {'product_id': self.product_b.id, 'product_qty': 1})]
         })
         self.assertEqual(bom.byproduct_ids.product_uom_id, self.env.ref('uom.product_uom_dozen'))
+
+    def test_finished_and_byproduct_moves(self):
+        """
+            Tests the behavior of the `create` override in the model `mrp.production`
+            regarding the values for the fields `move_finished_ids` and `move_byproduct_ids`.
+            The behavior is a bit tricky, because the moves included in `move_byproduct_ids`
+            are included in the `move_finished_ids`. `move_byproduct_ids` is a subset of `move_finished_ids`.
+
+            So, when creating a manufacturing order, whether:
+            - Only `move_finished_ids` is passed, containing both the finished product and the by-products of the BOM,
+            - Only `move_byproduct_ids` is passed, only containing the by-products of the BOM,
+            - Both `move_finished_ids` and `move_byproduct_ids` are passed,
+              holding the product finished and the byproducts respectively
+            At the end, in the created manufacturing order
+            `move_finished_ids` must contain both the finished product, and the by-products,
+            `move_byproduct_ids` must contain only the by-products.
+
+            Besides, the code shouldn't raise an error
+            because only one of the two `move_finished_ids`, `move_byproduct_ids` is provided.
+
+            In addition, the test voluntary sets a different produced quantity
+            for the finished product and the by-products moves than defined in the BOM
+            as it's the point to manually pass the `move_finished_ids` and `move_byproduct_ids`
+            when creating a manufacturing order, set different values than the defaults, in this case
+            a different produced quantity than the defaults from the BOM.
+        """
+        bom_product_a = self.MrpBom.create({
+            'product_tmpl_id': self.product_a.product_tmpl_id.id,
+            'product_qty': 1.0,
+            'type': 'normal',
+            'bom_line_ids': [(0, 0, {
+                'product_id': self.product_c_id, 'product_uom_id': self.uom_unit_id, 'product_qty': 2.0
+            })],
+            'byproduct_ids': [(0, 0, {
+                'product_id': self.product_b.id, 'product_uom_id': self.uom_unit_id, 'product_qty': 1.0
+            })]
+        })
+        for expected_finished_qty, expected_byproduct_qty, values in [
+            # Only `move_finished_ids` passed, containing both the finished product and the by-product
+            (3.0, 4.0, {
+                'move_finished_ids': [
+                    (0, 0, {
+                        'product_id': self.product_a.id,
+                        'product_uom_qty': 3.0,
+                        'location_id': self.product_a.property_stock_production,
+                        'location_dest_id': self.warehouse.lot_stock_id.id,
+                    }),
+                    (0, 0, {
+                        'product_id': self.product_b.id,
+                        'product_uom_qty': 4.0,
+                        'location_id': self.product_a.property_stock_production,
+                        'location_dest_id': self.warehouse.lot_stock_id.id,
+                    }),
+                ],
+            }),
+            # Only `move_byproduct_ids` passed, containing the by-product move only
+            (2.0, 4.0, {
+                'move_byproduct_ids': [
+                    (0, 0, {
+                        'product_id': self.product_b.id,
+                        'product_uom_qty': 4.0,
+                        'location_id': self.product_a.property_stock_production,
+                        'location_dest_id': self.warehouse.lot_stock_id.id,
+                    }),
+                ],
+            }),
+            # Both `move_finished_ids` and `move_byproduct_ids` passed,
+            # containing respectively the finished product and the by-product
+            (3.0, 4.0, {
+                'move_finished_ids': [
+                    (0, 0, {
+                        'product_id': self.product_a.id,
+                        'product_uom_qty': 3.0,
+                        'location_id': self.product_a.property_stock_production,
+                        'location_dest_id': self.warehouse.lot_stock_id.id,
+                    }),
+                ],
+                'move_byproduct_ids': [
+                    (0, 0, {
+                        'product_id': self.product_b.id,
+                        'product_uom_qty': 4.0,
+                        'location_id': self.product_a.property_stock_production,
+                        'location_dest_id': self.warehouse.lot_stock_id.id,
+                    }),
+                ],
+            }),
+        ]:
+            mo = self.env['mrp.production'].create({
+                'product_id': self.product_a.id,
+                'bom_id': bom_product_a.id,
+                'product_qty': 2.0,
+                **values,
+            })
+            self.assertEqual(mo.move_finished_ids.product_id, self.product_a + self.product_b)
+            self.assertEqual(mo.move_byproduct_ids.product_id, self.product_b)
+
+            finished_move = mo.move_finished_ids.filtered(lambda x: x.product_id == self.product_a)
+            self.assertEqual(
+                finished_move.product_uom_qty, expected_finished_qty, "Wrong produced quantity of finished product."
+            )
+
+            by_product_move = mo.move_finished_ids.filtered(lambda x: x.product_id == self.product_b)
+            self.assertEqual(
+                by_product_move.product_uom_qty, expected_byproduct_qty, "Wrong produced quantity of by-product."
+            )
+
+            # Also check the produced quantity of the by-product through `move_byproduct_ids`
+            self.assertEqual(
+                mo.move_byproduct_ids.product_uom_qty, expected_byproduct_qty, "Wrong produced quantity of by-product."
+            )

addons/mrp/views/mrp_production_views.xml

@@ -341,7 +341,6 @@
                         <page string="By-Products" name="finished_products" groups="mrp.group_mrp_byproducts">
                             <field name="move_byproduct_ids" context="{'default_date': date_planned_finished, 'default_date_deadline': date_deadline, 'default_location_id': production_location_id, 'default_location_dest_id': location_dest_id, 'default_state': 'draft', 'default_production_id': id, 'default_picking_type_id': picking_type_id, 'default_company_id': company_id}" attrs="{'readonly': ['|', ('state', '=', 'cancel'), '&amp;', ('state', '=', 'done'), ('is_locked', '=', True)]}" options="{'delete': [('state', '=', 'draft')]}">
                                 <tree default_order="is_done,sequence" decoration-muted="is_done" editable="bottom">
-                                    <field name="byproduct_id" invisible="1"/>
                                     <field name="product_id" context="{'default_detailed_type': 'product'}" domain="[('id', '!=', parent.product_id)]" required="1"/>
                                     <field name="location_dest_id" string="To" readonly="1" groups="stock.group_stock_multi_locations"/>
                                     <field name="company_id" invisible="1"/>