From cff93694a77397edd65e0b0080c8b95ba703ffe8 Mon Sep 17 00:00:00 2001 From: "Tiffany Chang (tic)" Date: Thu, 29 Feb 2024 10:31:06 +0000 Subject: [PATCH] [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) --- addons/delivery/i18n/delivery.pot | 4 +- addons/stock/i18n/stock.pot | 14 +++++ addons/stock/models/stock_picking.py | 6 +- addons/stock/tests/test_packing.py | 63 +++++++++++++++++++ addons/stock_delivery/i18n/stock_delivery.pot | 10 +++ addons/stock_delivery/models/stock_picking.py | 12 ++-- .../tests/test_packing_delivery.py | 55 ++++++++++++++++ .../wizard/choose_delivery_package.py | 3 +- .../models/stock_picking.py | 8 ++- .../models/stock_picking_batch.py | 2 +- .../tests/test_batch_picking.py | 15 +++++ 11 files changed, 179 insertions(+), 13 deletions(-) diff --git a/addons/delivery/i18n/delivery.pot b/addons/delivery/i18n/delivery.pot index a66aedc9388..777f0b4668e 100644 --- a/addons/delivery/i18n/delivery.pot +++ b/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 diff --git a/addons/stock/i18n/stock.pot b/addons/stock/i18n/stock.pot index e837b78a557..afa549260e1 100644 --- a/addons/stock/i18n/stock.pot +++ b/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 diff --git a/addons/stock/models/stock_picking.py b/addons/stock/models/stock_picking.py index 54384f0a302..3f3601dfffa 100644 --- a/addons/stock/models/stock_picking.py +++ b/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 diff --git a/addons/stock/tests/test_packing.py b/addons/stock/tests/test_packing.py index 254a90b90c6..a323cd701db 100644 --- a/addons/stock/tests/test_packing.py +++ b/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: diff --git a/addons/stock_delivery/i18n/stock_delivery.pot b/addons/stock_delivery/i18n/stock_delivery.pot index 6b42816cfa1..597ad82431b 100644 --- a/addons/stock_delivery/i18n/stock_delivery.pot +++ b/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." diff --git a/addons/stock_delivery/models/stock_picking.py b/addons/stock_delivery/models/stock_picking.py index 8574f7c379d..10a47fe31e9 100644 --- a/addons/stock_delivery/models/stock_picking.py +++ b/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. diff --git a/addons/stock_delivery/tests/test_packing_delivery.py b/addons/stock_delivery/tests/test_packing_delivery.py index 42fcfb67c26..f0f19aaf649 100644 --- a/addons/stock_delivery/tests/test_packing_delivery.py +++ b/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.') diff --git a/addons/stock_delivery/wizard/choose_delivery_package.py b/addons/stock_delivery/wizard/choose_delivery_package.py index 346eb691f18..f5b735fedab 100644 --- a/addons/stock_delivery/wizard/choose_delivery_package.py +++ b/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: diff --git a/addons/stock_picking_batch/models/stock_picking.py b/addons/stock_picking_batch/models/stock_picking.py index a2b2af07b60..83f478f6acc 100644 --- a/addons/stock_picking_batch/models/stock_picking.py +++ b/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 { diff --git a/addons/stock_picking_batch/models/stock_picking_batch.py b/addons/stock_picking_batch/models/stock_picking_batch.py index 4090581ec58..afbb7c55b74 100644 --- a/addons/stock_picking_batch/models/stock_picking_batch.py +++ b/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: diff --git a/addons/stock_picking_batch/tests/test_batch_picking.py b/addons/stock_picking_batch/tests/test_batch_picking.py index 272b0de3644..2e870c3c691 100644 --- a/addons/stock_picking_batch/tests/test_batch_picking.py +++ b/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: