From b33d5af4a74e3f4ca515826d4683065e32005e85 Mon Sep 17 00:00:00 2001 From: Arnold Moyaux Date: Tue, 20 Mar 2018 16:42:19 +0100 Subject: [PATCH] [FIX] stock: allow to validate a picking with 2 extra moves Use case to reproduce: - Create a picking containing 2 moves that have a quantity_done greater than reserved quantity - Validate the picking with or without backorder Traceback due to a record that do not exist anymore. It happens due to 2 functionality that have a wrong behavior when used together: - merge_move function that will try to merge moves with same characteristics inside a same picking. - action_done functionality that will create an extra move if the quantity done for a move is greater than reserved quantity. (the extra move is used in order to propagate changes) With our usecase: - MOVE A, reserved_qty: 10, qty_done: 20 - MOVE B, reserved_qty: 10, qty_done: 15 action_done on move A will create a new move thus we will have - MOVE A, reserved_qty: 10, qty_done: 20 - A bis, reserved_qty: 10, qty_done: 0 - MOVE B, reserved_qty: 10, qty_done: 15 then merge move will not only merge move A and A bis but will also merge B which result with inconsenstencies and traceback since system will try to process move B after A - MOVE A,A',B reserved_qty:30, qty_done:35 (the 5 extra qty is not propagated) This commit adds a kwarg in action_confirm that is propagated to merge_move this kwargs is a record set taht will limit the moves that can be used for the merge in order to merge extra move in original move opw-1825264 --- addons/mrp/models/stock_move.py | 4 +-- addons/sale_stock/models/stock.py | 2 +- addons/stock/models/stock_move.py | 31 +++++++++++++++------- addons/stock/tests/test_move2.py | 44 +++++++++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 12 deletions(-) diff --git a/addons/mrp/models/stock_move.py b/addons/mrp/models/stock_move.py index ccba058f291..33434edfd98 100644 --- a/addons/mrp/models/stock_move.py +++ b/addons/mrp/models/stock_move.py @@ -137,12 +137,12 @@ class StockMove(models.Model): If you want to cancel this MO, please change the consumed quantities to 0.')) return super(StockMove, self)._action_cancel() - def _action_confirm(self, merge=True): + def _action_confirm(self, merge=True, merge_into=False): moves = self.env['stock.move'] for move in self: moves |= move.action_explode() # we go further with the list of ids potentially changed by action_explode - return super(StockMove, moves)._action_confirm(merge=merge) + return super(StockMove, moves)._action_confirm(merge=merge, merge_into=merge_into) def action_explode(self): """ Explodes pickings """ diff --git a/addons/sale_stock/models/stock.py b/addons/sale_stock/models/stock.py index d251855ef99..26deb65145d 100644 --- a/addons/sale_stock/models/stock.py +++ b/addons/sale_stock/models/stock.py @@ -28,7 +28,7 @@ class StockMove(models.Model): def _action_done(self): result = super(StockMove, self)._action_done() - for line in self.mapped('sale_line_id'): + for line in result.mapped('sale_line_id'): line.qty_delivered = line._get_delivered_qty() return result diff --git a/addons/stock/models/stock_move.py b/addons/stock/models/stock_move.py index 7d4fe859476..1ec6a2a3542 100644 --- a/addons/stock/models/stock_move.py +++ b/addons/stock/models/stock_move.py @@ -535,7 +535,7 @@ class StockMove(models.Model): move.product_uom.id, move.restrict_partner_id.id, move.scrapped, move.origin_returned_move_id.id ] - def _merge_moves(self): + def _merge_moves(self, merge_into=False): """ This method will, for each move in `self`, go up in their linked picking and try to find in their existing moves a candidate into which we can merge the move. :return: Recordset of moves passed to this method. If some of the passed moves were merged @@ -543,12 +543,19 @@ class StockMove(models.Model): """ distinct_fields = self._prepare_merge_moves_distinct_fields() + candidate_moves_list = [] + if not merge_into: + for picking in self.mapped('picking_id'): + candidate_moves_list.append(picking.move_lines) + else: + candidate_moves_list.append(merge_into | self) + # Move removed after merge moves_to_unlink = self.env['stock.move'] moves_to_merge = [] - for picking in self.mapped('picking_id'): + for candidate_moves in candidate_moves_list: # First step find move to merge. - for k, g in groupby(sorted(picking.move_lines, key=self._prepare_merge_move_sort_method), key=itemgetter(*distinct_fields)): + for k, g in groupby(sorted(candidate_moves, key=self._prepare_merge_move_sort_method), key=itemgetter(*distinct_fields)): moves = self.env['stock.move'].concat(*g).filtered(lambda m: m.state not in ('done', 'cancel', 'draft')) # If we have multiple records we will merge then in a single one. if len(moves) > 1: @@ -697,7 +704,7 @@ class StockMove(models.Model): 'location_dest_id': self.location_dest_id.id, } - def _action_confirm(self, merge=True): + def _action_confirm(self, merge=True, merge_into=False): """ Confirms stock move or put it in waiting if it's linked to another move. :param: merge: According to this boolean, a newly confirmed move will be merged in another move of the same picking sharing its characteristics. @@ -737,7 +744,7 @@ class StockMove(models.Model): moves._assign_picking() self._push_apply() if merge: - return self._merge_moves() + return self._merge_moves(merge_into=merge_into) return self def _prepare_procurement_values(self): @@ -973,7 +980,7 @@ class StockMove(models.Model): The rationale for the creation of an extra move is the application of a potential push rule that will handle the extra quantities. """ - extra_move = self.env['stock.move'] + extra_move = self rounding = self.product_uom.rounding # moves created after the picking is assigned do not have `product_uom_qty`, but we shouldn't create extra moves for them if float_compare(self.quantity_done, self.product_uom_qty, precision_rounding=rounding) > 0: @@ -983,7 +990,11 @@ class StockMove(models.Model): precision_rounding=self.product_uom.rounding, rounding_method ='UP') extra_move_vals = self._prepare_extra_move_vals(extra_move_quantity) - extra_move = self.copy(default=extra_move_vals)._action_confirm() + extra_move = self.copy(default=extra_move_vals) + if extra_move.picking_id: + extra_move = extra_move._action_confirm(merge_into=self) + else: + extra_move = extra_move._action_confirm() # link it to some move lines. We don't need to do it for move since they should be merged. if self.exists() and not self.picking_id: @@ -1022,7 +1033,9 @@ class StockMove(models.Model): for move in moves: if move.state == 'cancel' or move.quantity_done <= 0: continue - moves_todo |= move + # extra move will not be merged in mrp + if not move.picking_id: + moves_todo |= move moves_todo |= move._create_extra_move() # Split moves where necessary and move quants @@ -1052,7 +1065,7 @@ class StockMove(models.Model): .filtered(lambda p: p.quant_ids and len(p.quant_ids) > 1): if len(result_package.quant_ids.mapped('location_id')) > 1: raise UserError(_('You should not put the contents of a package in different locations.')) - picking = self and self[0].picking_id or False + picking = moves_todo and moves_todo[0].picking_id or False moves_todo.write({'state': 'done', 'date': fields.Datetime.now()}) moves_todo.mapped('move_dest_ids')._action_assign() diff --git a/addons/stock/tests/test_move2.py b/addons/stock/tests/test_move2.py index 40ee52e1b7b..dbc10482d4f 100644 --- a/addons/stock/tests/test_move2.py +++ b/addons/stock/tests/test_move2.py @@ -963,6 +963,50 @@ class TestSinglePicking(TestStockCommon): self.assertEqual(sum(move1.move_line_ids.mapped('qty_done')), 2.0) self.assertEqual(move1.state, 'done') + def test_extra_move_4(self): + """ Create a picking with similar moves (created after + confirmation). Action done should propagate all the extra + quantity and only merge extra moves in their original moves. + """ + delivery = self.env['stock.picking'].create({ + 'location_id': self.stock_location, + 'location_dest_id': self.customer_location, + 'partner_id': self.partner_delta_id, + 'picking_type_id': self.picking_type_out, + }) + self.MoveObj.create({ + 'name': self.productA.name, + 'product_id': self.productA.id, + 'product_uom_qty': 5, + 'quantity_done': 10, + 'product_uom': self.productA.uom_id.id, + 'picking_id': delivery.id, + 'location_id': self.stock_location, + 'location_dest_id': self.customer_location, + }) + stock_location = self.env['stock.location'].browse(self.stock_location) + self.env['stock.quant']._update_available_quantity(self.productA, stock_location, 5) + delivery.action_confirm() + delivery.action_assign() + + delivery.write({ + 'move_lines': [(0, 0, { + 'name': self.productA.name, + 'product_id': self.productA.id, + 'product_uom_qty': 0, + 'quantity_done': 10, + 'state': 'assigned', + 'product_uom': self.productA.uom_id.id, + 'picking_id': delivery.id, + 'location_id': self.stock_location, + 'location_dest_id': self.customer_location, + })] + }) + delivery.action_done() + self.assertEqual(len(delivery.move_lines), 2, 'Move should not be merged together') + for move in delivery.move_lines: + self.assertEqual(move.quantity_done, move.product_uom_qty, 'Initial demand should be equals to quantity done') + def test_recheck_availability_1(self): """ Check the good behavior of check availability. I create a DO for 2 unit with only one in stock. After the first check availability, I should have 1 reserved