From 9ab123034a378c1ce987b603c882cd98dfbdb71d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Voet=20=28ryv=29?= Date: Thu, 8 Oct 2020 11:00:35 +0000 Subject: [PATCH] [IMP] stock: fix and batch push_apply - Fix the issue of the added test. - Also batch the `push_apply` + `_action_confirm` by doing confirm in BFS instead of DFS. Improve a lot the performance when the _action_confirm is on batch. closes odoo/odoo#61567 Signed-off-by: Arnold Moyaux --- addons/stock/models/stock_move.py | 48 +++++++++++++---------- addons/stock/models/stock_rule.py | 5 ++- addons/stock/tests/test_move2.py | 64 +++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 22 deletions(-) diff --git a/addons/stock/models/stock_move.py b/addons/stock/models/stock_move.py index 3bbcc0a2ebf..a29ce1b33df 100644 --- a/addons/stock/models/stock_move.py +++ b/addons/stock/models/stock_move.py @@ -14,6 +14,7 @@ from dateutil import relativedelta from odoo import SUPERUSER_ID, _, api, fields, models from odoo.exceptions import UserError from odoo.osv import expression +from odoo.tools import OrderedSet from odoo.tools.float_utils import float_compare, float_is_zero, float_repr, float_round from odoo.tools.misc import format_date @@ -719,6 +720,7 @@ class StockMove(models.Model): return True def _push_apply(self): + new_moves = [] for move in self: # if the move is already chained, there is no need to check push rules if move.move_dest_ids: @@ -729,12 +731,15 @@ class StockMove(models.Model): # first priority goes to the preferred routes defined on the move itself (e.g. coming from a SO line) warehouse_id = move.warehouse_id or move.picking_id.picking_type_id.warehouse_id if move.location_dest_id.company_id == self.env.company: - rules = self.env['procurement.group']._search_rule(move.route_ids, move.product_id, warehouse_id, domain) + rule = self.env['procurement.group']._search_rule(move.route_ids, move.product_id, warehouse_id, domain) else: - rules = self.sudo().env['procurement.group']._search_rule(move.route_ids, move.product_id, warehouse_id, domain) + rule = self.sudo().env['procurement.group']._search_rule(move.route_ids, move.product_id, warehouse_id, domain) # Make sure it is not returning the return - if rules and (not move.origin_returned_move_id or move.origin_returned_move_id.location_dest_id.id != rules.location_id.id): - rules._run_push(move) + if rule and (not move.origin_returned_move_id or move.origin_returned_move_id.location_dest_id.id != rule.location_id.id): + new_move = rule._run_push(move) + if new_move: + new_moves.append(new_move) + return self.env['stock.move'].concat(*new_moves) def _merge_moves_fields(self): """ This method will return a dict of stock move’s values that represent the values of all moves in `self` merged. """ @@ -763,7 +768,7 @@ class StockMove(models.Model): return [ move.product_id.id, move.price_unit, move.procure_method, move.location_id, move.location_dest_id, move.product_uom.id, move.restrict_partner_id.id, move.scrapped, move.origin_returned_move_id.id, - move.package_level_id.id, move.propagate_cancel, move.description_picking + move.package_level_id.id, move.propagate_cancel, move.description_picking or "" ] def _clean_merged(self): @@ -1070,27 +1075,25 @@ class StockMove(models.Model): :param: merge: According to this boolean, a newly confirmed move will be merged in another move of the same picking sharing its characteristics. """ - move_create_proc = self.env['stock.move'] - move_to_confirm = self.env['stock.move'] - move_waiting = self.env['stock.move'] - - to_assign = {} + # Use OrderedSet of id (instead of recordset + |= ) for performance + move_create_proc, move_to_confirm, move_waiting = OrderedSet(), OrderedSet(), OrderedSet() + to_assign = defaultdict(OrderedSet) for move in self: if move.state != 'draft': continue - # if the move is preceeded, then it's waiting (if preceeding move is done, then action_assign has been called already and its state is already available) + # if the move is preceded, then it's waiting (if preceding move is done, then action_assign has been called already and its state is already available) if move.move_orig_ids: - move_waiting |= move + move_waiting.add(move.id) else: if move.procure_method == 'make_to_order': - move_create_proc |= move + move_create_proc.add(move.id) else: - move_to_confirm |= move + move_to_confirm.add(move.id) if move._should_be_assigned(): key = (move.group_id.id, move.location_id.id, move.location_dest_id.id) - if key not in to_assign: - to_assign[key] = self.env['stock.move'] - to_assign[key] |= move + to_assign[key].add(move.id) + + move_create_proc, move_to_confirm, move_waiting = self.browse(move_create_proc), self.browse(move_to_confirm), self.browse(move_waiting) # create procurements for make to order moves procurement_requests = [] @@ -1107,15 +1110,20 @@ class StockMove(models.Model): (move_waiting | move_create_proc).write({'state': 'waiting'}) # assign picking in batch for all confirmed move that share the same details - for moves in to_assign.values(): - moves._assign_picking() - self._push_apply() + for moves_ids in to_assign.values(): + self.browse(moves_ids)._assign_picking() + new_push_moves = self._push_apply() self._check_company() moves = self if merge: moves = self._merge_moves(merge_into=merge_into) + # call `_action_assign` on every confirmed move which location_id bypasses the reservation moves.filtered(lambda move: not move.picking_id.immediate_transfer and move._should_bypass_reservation() and move.state == 'confirmed')._action_assign() + + if new_push_moves: + new_push_moves._action_confirm() + return moves def _prepare_procurement_values(self): diff --git a/addons/stock/models/stock_rule.py b/addons/stock/models/stock_rule.py index b76d722255a..c16a02e1419 100644 --- a/addons/stock/models/stock_rule.py +++ b/addons/stock/models/stock_rule.py @@ -164,6 +164,7 @@ class StockRule(models.Model): Care this function is not call by method run. It is called explicitely in stock_move.py inside the method _push_apply """ + self.ensure_one() new_date = fields.Datetime.to_string(move.date + relativedelta(days=self.delay)) if self.auto == 'transparent': old_dest_location = move.location_dest_id @@ -171,7 +172,7 @@ class StockRule(models.Model): # avoid looping if a push rule is not well configured; otherwise call again push_apply to see if a next step is defined if self.location_id != old_dest_location: # TDE FIXME: should probably be done in the move model IMO - move._push_apply() + return move._push_apply()[:1] else: new_move_vals = self._push_prepare_move_copy_values(move, new_date) new_move = move.sudo().copy(new_move_vals) @@ -179,7 +180,7 @@ class StockRule(models.Model): new_move.write({'procure_method': 'make_to_stock'}) if not new_move.location_id.should_bypass_reservation(): move.write({'move_dest_ids': [(4, new_move.id)]}) - new_move._action_confirm() + return new_move def _push_prepare_move_copy_values(self, move_to_copy, new_date): company_id = self.company_id.id diff --git a/addons/stock/tests/test_move2.py b/addons/stock/tests/test_move2.py index 6ba16035914..e41dabb488a 100644 --- a/addons/stock/tests/test_move2.py +++ b/addons/stock/tests/test_move2.py @@ -1703,6 +1703,70 @@ class TestSinglePicking(TestStockCommon): self.assertEqual(len(qc_move), 1) self.assertTrue(qc_move.move_orig_ids == input_move, 'Move between QC and stock should only have the input move as origin') + def test_merge_chained_moves_multi_confirm(self): + """ Imagine multiple step delivery. A receipt picking for the same product should by add to + a existing picking from input to QC and another from QC to stock. + This existing picking is confirm in the same time (not possible in stock, but can be with batch picking) + and have some move to merge. + """ + warehouse = self.env['stock.warehouse'].create({ + 'name': 'TEST WAREHOUSE', + 'code': 'TEST1', + 'reception_steps': 'three_steps', + }) + receipt1 = self.env['stock.picking'].create({ + 'location_id': self.supplier_location, + 'location_dest_id': warehouse.wh_input_stock_loc_id.id, + 'picking_type_id': warehouse.in_type_id.id, + }) + move_receipt_1 = self.MoveObj.create({ + 'name': self.productA.name, + 'product_id': self.productA.id, + 'product_uom_qty': 5, + 'product_uom': self.productA.uom_id.id, + 'picking_id': receipt1.id, + 'location_id': self.supplier_location, + 'location_dest_id': warehouse.wh_input_stock_loc_id.id, + }) + receipt2 = self.env['stock.picking'].create({ + 'location_id': warehouse.wh_input_stock_loc_id.id, + 'location_dest_id': warehouse.wh_qc_stock_loc_id.id, + 'picking_type_id': warehouse.int_type_id.id, + }) + move1_receipt_2 = self.MoveObj.create({ + 'name': self.productB.name, + 'product_id': self.productB.id, + 'product_uom_qty': 1, + 'product_uom': self.productB.uom_id.id, + 'picking_id': receipt2.id, + 'location_id': warehouse.wh_input_stock_loc_id.id, + 'location_dest_id': warehouse.wh_qc_stock_loc_id.id, + }) + move2_receipt_2 = self.MoveObj.create({ + 'name': self.productB.name, + 'product_id': self.productB.id, + 'product_uom_qty': 2, + 'product_uom': self.productB.uom_id.id, + 'picking_id': receipt2.id, + 'location_id': warehouse.wh_input_stock_loc_id.id, + 'location_dest_id': warehouse.wh_qc_stock_loc_id.id, + }) + (receipt1 | receipt2).action_confirm() + + # Check following move has been created + self.assertTrue(move_receipt_1.move_dest_ids, 'No move created from push rules') + self.assertTrue((move1_receipt_2 | move2_receipt_2).exists().move_dest_ids, 'No move created from push rules') + self.assertEqual(len((move1_receipt_2 | move2_receipt_2).exists()), 1, 'Move has been merged with the other one') + self.assertEqual(move_receipt_1.move_dest_ids.picking_id, receipt2, 'Dest Move of receipt1 should be in the receipt2') + + # Check no move is still in draft + self.assertTrue("draft" not in (receipt1 | receipt2).move_lines.mapped("state")) + + # Check the content of the pickings + self.assertEqual(receipt1.move_lines.mapped("product_uom_qty"), [5]) + self.assertEqual(receipt2.move_lines.filtered(lambda m: m.product_id == self.productB).mapped("product_uom_qty"), [3]) + self.assertEqual(receipt2.move_lines.filtered(lambda m: m.product_id == self.productA).mapped("product_uom_qty"), [5]) + def test_empty_moves_validation_1(self): """ Use button validate on a picking that contains only moves without initial demand and without quantity done should be