[FIX] delivery,stock{_picking_batch}: correctly handle batch put in pack

Previous fix: odoo/odoo#139013 added in the ability to handle a use case
specific avoided due to its complexity and it being an edge case. I.e.
the ability to do a Put in Pack in a batch picking where there is a
shipping connector involved (i.e. when the `choose_delivery_package`
wizard is opened).

Because the ability to handle this situation is now added to stable, we
have to sort of support it now and handle it not breaking other flows.
Here are the flows that need to be handled (and were broken by the
previous PR): [In all cases, "Packages" setting needs to be activated
and each picking needs at least 1 move of a consumable/storable product]

Flow 1: batch picking + put in pack for single picking
- Create 2 pickings of any operation type
- Create a new batch picking with these 2 pickings
- Open 1 of those pickings directly (i.e. not in the batch)
- Click on "Put in Pack"

Expected result:
Only the move from the open picking is put into a package

Result before this commit:
Both pickings have their moves put into the same package

Additional notes: Because this is not an obvious bug, users may already
had this bug occur in their DBs without realizing it

===

Flow 2: batch picking (or multi-record calling of `action_put_in_pack`)
[different in v17 onwards due to removal of immediate_transfer boolean]
- Create 2 pickings (of different picking types)
- Select both pickings (through direct call in shell or rpc) and call
  `action_put_in_pack`

Expected result:
Moves are blocked from being put into same package since this situation
doesn't make sense (i.e. the products are moved to different locations
but the package can only be in 1 location)

Result before this commit:
The moves will all be put into the same package

Additional notes:
In theory batch picking creation has checks to avoid batches where
there are pickings with more than 1 picking type or have different
`show_reserved` values, but because `_package_move_lines` is a method
that can be called in different use cases (including multi-record
pickings) via customizations/future code changes, we add in checks to
prevent put in pack from finishing in those cases to avoid unexpected
behavior/stack traces. I.e. remember to respect existing
`self.ensure_one` checks since they're probably there for a reason.

===

Flow 3: batch picking w/pickings w/more than 1 delivery carriers (where
none = a different carrier than having 1)
- Create 2 delivery pickings with different `carrier_id` values (i.e.
  different shipping methods assigned to them)
- Add both pickings to a batch
- Click "Put in Pack" in the batch picking

Expected result:
None, we should not handle this case because if the products are in the
same package then the same package info will be sent to both carriers
and the user will be double charged for every move (or
charged(/potentially create the wrong shipping documents) when it
shouldn't be in case of no carrier for one of the pickings)

Result before this commit:
All moves are put in the same package and the double
charging/potentially incorrect shipping documents will occur

Additional notes:
This is the use case that was intended to be avoided when flow was
originally decided to not be handled

closes odoo/odoo#157224

X-original-commit: b3498facab77e1eed8969c018e3f966a80e62654
Signed-off-by: Tiffany Chang (tic) <tic@odoo.com>

addons/delivery/i18n/delivery.pot

@@ -354,8 +354,8 @@ msgstr ""
 #. module: delivery
 #: model:ir.model.fields,help:delivery.field_delivery_carrier__invoice_policy
 msgid ""
-"Estimated Cost: the customer will be invoiced the estimated cost of the shipping.\n"
-"Real Cost: the customer will be invoiced the real cost of the shipping, the cost of theshipping will be updated on the SO after the delivery."
+"Estimated Cost: the customer will be invoiced the estimated cost of the "
+"shipping."
 msgstr ""
 
 #. module: delivery

addons/stock/i18n/stock.pot

@@ -4585,6 +4585,11 @@ msgstr ""
 msgid "Lot Label Format to auto-print"
 msgstr ""
 
+#. module: stock
+#: model:ir.model,name:stock.model_report_stock_label_lot_template_view
+msgid "Lot Label Report"
+msgstr ""
+
 #. module: stock
 #: model:ir.model.fields,field_description:stock.field_product_product__lot_properties_definition
 msgid "Lot Properties"
@@ -10200,6 +10205,15 @@ msgid ""
 " or split the same package into two location."
 msgstr ""
 
+#. module: stock
+#. odoo-python
+#: code:addons/stock/models/stock_picking.py:0
+#, python-format
+msgid ""
+"You cannot pack products into the same package when they are from different "
+"transfers with different operation types."
+msgstr ""
+
 #. module: stock
 #. odoo-python
 #: code:addons/stock/models/stock_move.py:0

addons/stock/models/stock_picking.py

@@ -1499,7 +1499,11 @@ class Picking(models.Model):
                 return action
         return package_id
 
-    def _package_move_lines(self):
+    def _package_move_lines(self, batch_pack=False):
+        # in theory, the picking_type should always be the same (i.e. for batch transfers),
+        # but customizations may bypass it and cause unexpected behavior so we avoid allowing those situations
+        if len(self.picking_type_id) > 1:
+            raise UserError(_("You cannot pack products into the same package when they are from different transfers with different operation types."))
         quantity_move_line_ids = self.move_line_ids.filtered(
             lambda ml:
                 float_compare(ml.quantity, 0.0, precision_rounding=ml.product_uom_id.rounding) > 0 and

addons/stock/tests/test_packing.py

@@ -1511,6 +1511,69 @@ class TestPacking(TestPackingCommon):
         self.assertEqual(quantB.quantity, 4, "All 4 units of product B should be in location B")
         self.assertEqual(quantB.package_id.id, pack.id, "Product B should still be in the initial package.")
 
+    def test_expected_package_move_lines(self):
+        """ Test direct calling of `_package_move_lines` since it doesn't handle all multi-record cases
+        It's unlikely this situations will occur, but in case it is for customizations/future features,
+        ensure that we don't have unexpected behavior """
+
+        self.env['stock.quant']._update_available_quantity(self.productA, self.stock_location, 20.0)
+
+        internal_picking_1 = self.env['stock.picking'].create({
+            'picking_type_id': self.warehouse.int_type_id.id,
+            'location_id': self.stock_location.id,
+            'location_dest_id': self.stock_location.id,
+        })
+        self.env['stock.move'].create({
+            'name': self.productA.name,
+            'product_id': self.productA.id,
+            'product_uom_qty': 5,
+            'quantity': 5,
+            'product_uom': self.productA.uom_id.id,
+            'picking_id': internal_picking_1.id,
+            'location_id': self.stock_location.id,
+            'location_dest_id': self.stock_location.id,
+        })
+
+        internal_picking_2 = self.env['stock.picking'].create({
+            'picking_type_id': self.warehouse.int_type_id.id,
+            'location_id': self.stock_location.id,
+            'location_dest_id': self.stock_location.id,
+        })
+        self.env['stock.move'].create({
+            'name': self.productA.name,
+            'product_id': self.productA.id,
+            'product_uom_qty': 5,
+            'quantity': 5,
+            'product_uom': self.productA.uom_id.id,
+            'picking_id': internal_picking_2.id,
+            'location_id': self.stock_location.id,
+            'location_dest_id': self.stock_location.id,
+        })
+
+        in_picking_1 = self.env['stock.picking'].create({
+            'picking_type_id': self.warehouse.in_type_id.id,
+            'location_id': self.stock_location.id,
+            'location_dest_id': self.stock_location.id,
+            'state': 'draft',
+        })
+        self.env['stock.move'].create({
+            'name': self.productA.name,
+            'product_id': self.productA.id,
+            'product_uom_qty': 5,
+            'quantity': 5,
+            'product_uom': self.productA.uom_id.id,
+            'picking_id': in_picking_1.id,
+            'location_id': self.stock_location.id,
+            'location_dest_id': self.stock_location.id,
+        })
+
+        # can't mix operation types
+        with self.assertRaises(UserError):
+            move_lines_to_pack = (internal_picking_1 | in_picking_1)._package_move_lines()
+
+        move_lines_to_pack = (internal_picking_1 | internal_picking_2)._package_move_lines()
+        self.assertEqual(len(move_lines_to_pack), 2, "all move lines in pickings should have been selected to pack")
+
     def test_package_selection(self):
         """
         Test that the package selection is correct when using the least_package_strategy:

addons/stock_delivery/i18n/stock_delivery.pot

@@ -559,6 +559,16 @@ msgstr ""
 msgid "Weight:"
 msgstr ""
 
+#. module: stock_delivery
+#. odoo-python
+#: code:addons/stock_delivery/models/stock_picking.py:0
+#, python-format
+msgid ""
+"You cannot pack products into the same package when they have different "
+"carriers (i.e. check that all of their transfers have a carrier assigned and"
+" are using the same carrier)."
+msgstr ""
+
 #. module: stock_delivery
 #: model_terms:ir.ui.view,arch_db:stock_delivery.delivery_tracking_url_warning_form
 msgid "You have multiple tracker links, they are available in the chatter."

addons/stock_delivery/models/stock_picking.py

@@ -138,12 +138,15 @@ class StockPicking(models.Model):
     def _pre_put_in_pack_hook(self, move_line_ids):
         res = super(StockPicking, self)._pre_put_in_pack_hook(move_line_ids)
         if not res:
-            if self.carrier_id:
-                return self._set_delivery_package_type()
+            if move_line_ids.carrier_id:
+                if len(move_line_ids.carrier_id) > 1 or any(not ml.carrier_id for ml in move_line_ids):
+                    # avoid (duplicate) costs for products
+                    raise UserError(_("You cannot pack products into the same package when they have different carriers (i.e. check that all of their transfers have a carrier assigned and are using the same carrier)."))
+                return self._set_delivery_package_type(batch_pack=len(move_line_ids.picking_id) > 1)
         else:
             return res
 
-    def _set_delivery_package_type(self):
+    def _set_delivery_package_type(self, batch_pack=False):
         """ This method returns an action allowing to set the package type and the shipping weight
         on the stock.quant.package.
         """
@@ -152,7 +155,8 @@ class StockPicking(models.Model):
         context = dict(
             self.env.context,
             current_package_carrier_type=self.carrier_id.delivery_type,
-            default_picking_id=self.id
+            default_picking_id=self.id,
+            batch_pack=batch_pack,
         )
         # As we pass the `delivery_type` ('fixed' or 'base_on_rule' by default) in a key who
         # correspond to the `package_carrier_type` ('none' to default), we make a conversion.

addons/stock_delivery/tests/test_packing_delivery.py

@@ -1,9 +1,11 @@
 # Part of Odoo. See LICENSE file for full copyright and licensing details.
 
 from odoo.addons.stock.tests.test_packing import TestPackingCommon
+from odoo.exceptions import UserError
 from odoo.tests import Form
 from unittest.mock import patch
 
+
 class TestPacking(TestPackingCommon):
 
     @classmethod
@@ -141,3 +143,56 @@ class TestPacking(TestPackingCommon):
 
         for p in so.picking_ids:
             self.assertEqual(p.carrier_tracking_ref, "666")
+
+    def test_batch_picking_delivery(self):
+        """
+            Check that when Put in Pack is called for batch pickings (i.e. faked with multi-record action
+            calling to avoid extra batch+delivery module for just a test) then:
+             - Same delivery carrier = works
+             - Different delivery carriers = UserError
+        """
+        self.env['stock.quant']._update_available_quantity(self.productA, self.stock_location, 4.0)
+        another_test_carrier_product = self.env['product.product'].create({
+            'name': 'Another test carrier product',
+            'type': 'service',
+        })
+        test_carrier = self.env['delivery.carrier'].create({
+            'name': 'Another Test carrier',
+            'delivery_type': 'fixed',
+            'product_id': another_test_carrier_product.id,
+        })
+
+        delivery_1 = self.env['stock.picking'].create({
+            'partner_id': self.env['res.partner'].create({'name': 'A partner'}).id,
+            'picking_type_id': self.warehouse.out_type_id.id,
+            'location_id': self.stock_location.id,
+            'location_dest_id': self.customer_location.id,
+            'carrier_id': self.test_carrier.id
+        })
+        ml_1 = self.env['stock.move.line'].create({
+            'product_id': self.productA.id,
+            'product_uom_id': self.productA.uom_id.id,
+            'picking_id': delivery_1.id,
+            'quantity': 1,
+            'location_id': self.stock_location.id,
+            'location_dest_id': self.customer_location.id
+        })
+
+        # Test that differing carrier + put in pack = UserError
+        delivery_2 = delivery_1.copy({'carrier_id': test_carrier.id})
+        ml_1.copy({'picking_id': delivery_2.id, 'quantity': 1})
+        # recreate the `action_put_in_pack`` steps so we don't have to add test to new module for batch pickings
+        # to use batch version of method (which bypass the ensure_one() check in the stock_picking action)
+        move_lines_to_pack = (delivery_1 | delivery_2)._package_move_lines()
+        self.assertEqual(len(move_lines_to_pack), 2, 'There should be move lines that can be "put in pack"')
+        with self.assertRaises(UserError):
+            delivery_1._pre_put_in_pack_hook(move_lines_to_pack)
+
+        # Test that same carrier + put in pack = OK!
+        delivery_2.carrier_id = delivery_1.carrier_id
+        move_lines_to_pack = (delivery_1 | delivery_2)._package_move_lines()
+        self.assertEqual(len(move_lines_to_pack), 2, 'There should be move lines that can be "put in pack"')
+        delivery_1._pre_put_in_pack_hook(move_lines_to_pack)
+        package = delivery_1._put_in_pack(move_lines_to_pack)
+        self.assertEqual(delivery_1.move_line_ids.result_package_id, package, 'Delivery 1 moves should have been put in package.')
+        self.assertEqual(delivery_2.move_line_ids.result_package_id, package, 'Delivery 2 moves should have been put in package.')

addons/stock_delivery/wizard/choose_delivery_package.py

@@ -44,8 +44,7 @@ class ChooseDeliveryPackage(models.TransientModel):
             return {'warning': warning_mess}
 
     def action_put_in_pack(self):
-        move_line_ids = self.picking_id._package_move_lines()
-
+        move_line_ids = self.picking_id._package_move_lines(batch_pack=self.env.context.get("batch_pack"))
         delivery_package = self.picking_id._put_in_pack(move_line_ids)
         # write shipping weight and package type on 'stock_quant_package' if needed
         if self.delivery_package_type_id:

addons/stock_picking_batch/models/stock_picking.py

@@ -232,6 +232,11 @@ class StockPicking(models.Model):
 
         return domain
 
+    def _package_move_lines(self, batch_pack=False):
+        if batch_pack:
+            return super(StockPicking, self.batch_id.picking_ids if self.batch_id else self)._package_move_lines(batch_pack)
+        return super()._package_move_lines(batch_pack)
+
     def assign_batch_user(self, user_id):
         if not user_id:
             return
@@ -241,9 +246,6 @@ class StockPicking(models.Model):
             log_message = _('Assigned to %s Responsible', pick.batch_id._get_html_link())
             pick.message_post(body=log_message)
 
-    def _package_move_lines(self):
-        return super(StockPicking, self.batch_id.picking_ids if self.batch_id else self)._package_move_lines()
-
     def action_view_batch(self):
         self.ensure_one()
         return {

addons/stock_picking_batch/models/stock_picking_batch.py

@@ -236,7 +236,7 @@ class StockPickingBatch(models.Model):
         """
         self.ensure_one()
         if self.state not in ('done', 'cancel'):
-            move_line_ids = self.picking_ids[0]._package_move_lines()
+            move_line_ids = self.picking_ids[0]._package_move_lines(batch_pack=True)
             if move_line_ids:
                 res = move_line_ids.picking_id[0]._pre_put_in_pack_hook(move_line_ids)
                 if res:

addons/stock_picking_batch/tests/test_batch_picking.py

@@ -319,6 +319,7 @@ class TestBatchPicking(TransactionCase):
         self.assertEqual(self.picking_client_1.state, 'done', 'Picking 1 should be done')
         self.assertEqual(self.picking_client_1.move_ids.product_uom_qty, 5, 'initial demand should be 5 after picking split')
         self.assertFalse(self.picking_client_2.batch_id)
+
     def test_put_in_pack(self):
         self.env['stock.quant']._update_available_quantity(self.productA, self.stock_location, 10.0)
         self.env['stock.quant']._update_available_quantity(self.productB, self.stock_location, 10.0)
@@ -357,6 +358,20 @@ class TestBatchPicking(TransactionCase):
         # final package location should be correctly set based on wizard
         self.assertEqual(package.location_id.id, self.customer_location.id)
 
+    def test_put_in_pack_within_single_picking(self):
+        """ Test that when `action_put_in_pack` is called on a picking that is also in a batch,
+        only that picking's moves are put in the pack """
+
+        self.env['stock.quant']._update_available_quantity(self.productA, self.stock_location, 10.0)
+        self.env['stock.quant']._update_available_quantity(self.productB, self.stock_location, 10.0)
+
+        self.batch.action_confirm()
+        self.batch.action_assign()
+        self.batch.move_line_ids.quantity = 5
+        package = self.picking_client_1.action_put_in_pack()
+        self.assertEqual(self.picking_client_1.move_line_ids.result_package_id, package)
+        self.assertFalse(self.picking_client_2.move_line_ids.result_package_id, "Other picking in batch shouldn't have been put in a package")
+
     def test_auto_batch(self):
         """ Test a simple auto-batch scenario with new picking type to avoid conflicts with existing picking types.
         The pickings look like this: