From fc5572c11f99ab690e4c5967c2695a4c480ffbd8 Mon Sep 17 00:00:00 2001 From: Adrien Widart Date: Mon, 4 Jul 2022 14:56:32 +0000 Subject: [PATCH] [FIX] {sale_,}stock: reverse orig/dest moves if is return In a multistep configuration, decreasing the quantity on a line of a confirmed sale order may create some inconsistencies. To reproduce the issue: 1. In Settings, enable "Multi-Step Routes" 2. Edit the existing warehouse: - Outgoing Shipments: 3 steps 3. Create a storable product P 4. Set the on-hand qty to 5 5. Create and confirm a sale order SO with 5 x P 6. Process the PICK/PACK pickings 7. Edit the SO: - Decrease the qty: 3 x P 8. In the SO's deliveries, open the return of the pack step (Output -> Packing) - Error: Nothing changes, there isn't any reserved quantity. However, there are currently 5 x P at the Output location, so the reservation should work 9. Inventory > Report > Stock Moves, list all SM related to P - Error: The origin/destination moves of the pack's return (Output -> Packing) are incorrect too: the origins contain the pick's return (Packing -> Stock) while it should not contain anything and the destination moves contain the ship move (Output -> Customer) while it should contain the pick's return (Packing -> Stock) `SM_ship`: SM used to deliver from Output to Customers `SM_ret_ship`: SM used to return P from Customers to Output `SM_ret_pack`: SM used to return P from Output to Packing `SM_ret_pick`: SM used to return P from Packing to Stock When confirming a move, we transform the move if its demand is negative: https://github.com/odoo/odoo/blob/53b424d9e43b07fe7aa84ff00035fb7772ef6291/addons/stock/models/stock_move.py#L1239-L1247 We reverse the source/destination locations but we don't do the same with the origin/destination moves. That is an issue: before reversing, we have: | SM | Qty | From | To | Origin | Destination | |-------------|-----|---------|-----------|-------------|-------------| | SM_ret_pick | -2 | Stock | Packing | / | SM_ret_pack | | SM_ret_pack | -2 | Packing | Output | SM_ret_pick | SM_ret_ship | | SM_ret_ship | -2 | Output | Customers | SM_ret_pack | / | Therefore, reversing the source/destination locations without updating the origin/destination moves does not make sense: we should reverse them too depending on their direction (positive/negative) and their source/destination location. Also, before trying to transform the moves, we try to merge them: https://github.com/odoo/odoo/blob/53b424d9e43b07fe7aa84ff00035fb7772ef6291/addons/stock/models/stock_move.py#L1236-L1237 This is relevant for `SM_ret_ship`: we find an existing SM (`SM_ship`, qty=5) that can absorb the negative demand of `SM_ret_ship` (qty=-2). In such case, we add the origin/destination moves of the negative SM to the positive one. However, in the above case, it will create an inconsistency: after the fix, at that point, `SM_ret_pick` and `SM_ret_pack` have been correctly reversed, so we have: | SM | Qty | From | To | Origin | Destination | |-------------|-----|---------|-----------|-------------|-------------| | SM_ret_pick | 2 | Packing | Stock | SM_ret_pack | / | | SM_ret_pack | 2 | Output | Packing | SM_ret_pack | SM_ret_pick | | SM_ret_ship | -2 | Output | Customers | / | SM_ret_pack | | SM_ship | 5 | Output | Customers | (SM_pack) | / | Therefore, adding `SM_ret_pack` to the destination moves of `SM_ship` is incorrect. (Note that, without the fix, `SM_ret_pack` would be added to the origins of `SM_ship`, which would be incorrect too) Finally, the reservation (step 8) does't work because of two reasons: - If there are some `move_orig_ids`, we use the done value of these SM to assign the current SM. However, as explained above, there is an issue with this field - If there isn't any `move_orig_ids`, there is an issue with the procure method. Its value is `make_to_order`: we skip the reservation for such a value: https://github.com/odoo/odoo/blob/53b424d9e43b07fe7aa84ff00035fb7772ef6291/addons/stock/models/stock_move.py#L1499-L1501 OPW-2889679 closes odoo/odoo#95263 closes odoo/odoo#95926 X-original-commit: fb94c140a91aca26a56c5162d61b00b4a4b0d85b Signed-off-by: William Henrotin (whe) Signed-off-by: Adrien Widart --- addons/sale_stock/tests/test_sale_stock.py | 52 ++++++++++++++++++++++ addons/stock/models/stock_move.py | 16 ++++++- 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/addons/sale_stock/tests/test_sale_stock.py b/addons/sale_stock/tests/test_sale_stock.py index 482fcd1074c..e056c6d7d03 100644 --- a/addons/sale_stock/tests/test_sale_stock.py +++ b/addons/sale_stock/tests/test_sale_stock.py @@ -1205,3 +1205,55 @@ class TestSaleStock(TestSaleCommon, ValuationReconciliationTestCommon): self.assertEqual(so.order_line[1].product_id, self.product_b) self.assertEqual(so.order_line[1].qty_delivered, 1) self.assertEqual(so.order_line[1].product_uom.id, uom_km_id) + + def test_return_with_mto_and_multisteps(self): + """ + Suppose a product P and a 3-steps delivery. + Sell 5 x P, process pick & pack pickings and then decrease the qty on + the SO line: + - the ship picking should be updated + - there should be a return R1 for the pack picking + - there should be a return R2 for the pick picking + - it should be possible to reserve R1 + """ + warehouse = self.env['stock.warehouse'].search([('company_id', '=', self.env.company.id)], limit=1) + warehouse.delivery_steps = 'pick_pack_ship' + stock_location = warehouse.lot_stock_id + pack_location, out_location, custo_location = warehouse.delivery_route_id.rule_ids.location_dest_id + + product = self.env['product.product'].create({ + 'name': 'SuperProduct', + 'type': 'product', + }) + + self.env['stock.quant']._update_available_quantity(product, stock_location, 5) + + so_form = Form(self.env['sale.order']) + so_form.partner_id = self.partner_a + with so_form.order_line.new() as line: + line.product_id = product + line.product_uom_qty = 5 + so = so_form.save() + so.action_confirm() + + pick_picking, pack_picking, _ = so.picking_ids + (pick_picking + pack_picking).move_ids.quantity_done = 5 + (pick_picking + pack_picking).button_validate() + + with Form(so) as so_form: + with so_form.order_line.edit(0) as line: + line.product_uom_qty = 3 + + move_lines = so.picking_ids.move_ids.sorted('id') + ship_sm, pack_sm, pick_sm, ret_pack_sm, ret_pick_sm = move_lines + self.assertRecordValues(move_lines, [ + {'location_id': out_location.id, 'location_dest_id': custo_location.id, 'move_orig_ids': pack_sm.ids, 'move_dest_ids': []}, + {'location_id': pack_location.id, 'location_dest_id': out_location.id, 'move_orig_ids': pick_sm.ids, 'move_dest_ids': ship_sm.ids}, + {'location_id': stock_location.id, 'location_dest_id': pack_location.id, 'move_orig_ids': [], 'move_dest_ids': pack_sm.ids}, + {'location_id': out_location.id, 'location_dest_id': pack_location.id, 'move_orig_ids': [], 'move_dest_ids': ret_pick_sm.ids}, + {'location_id': pack_location.id, 'location_dest_id': stock_location.id, 'move_orig_ids': ret_pack_sm.ids, 'move_dest_ids': []}, + ]) + + ret_pack_sm.picking_id.action_assign() + self.assertEqual(ret_pack_sm.state, 'assigned') + self.assertEqual(ret_pack_sm.move_line_ids.reserved_uom_qty, 2) diff --git a/addons/stock/models/stock_move.py b/addons/stock/models/stock_move.py index 841d629d933..01923d1d824 100644 --- a/addons/stock/models/stock_move.py +++ b/addons/stock/models/stock_move.py @@ -894,8 +894,8 @@ class StockMove(models.Model): if float_compare(pos_move.product_uom_qty, abs(neg_move.product_uom_qty), precision_rounding=pos_move.product_uom.rounding) >= 0: pos_move.product_uom_qty += neg_move.product_uom_qty pos_move.write({ - 'move_dest_ids': [Command.link(m.id) for m in neg_move.mapped('move_dest_ids')], - 'move_orig_ids': [Command.link(m.id) for m in neg_move.mapped('move_orig_ids')], + 'move_dest_ids': [Command.link(m.id) for m in neg_move.mapped('move_dest_ids') if m.location_id == pos_move.location_dest_id], + 'move_orig_ids': [Command.link(m.id) for m in neg_move.mapped('move_orig_ids') if m.location_dest_id == pos_move.location_id], }) merged_moves |= pos_move moves_to_unlink |= neg_move @@ -1256,9 +1256,21 @@ class StockMove(models.Model): move.product_uom_qty, 0, precision_rounding=move.product_uom.rounding) < 0) for move in neg_r_moves: move.location_id, move.location_dest_id = move.location_dest_id, move.location_id + orig_move_ids, dest_move_ids = [], [] + for m in move.move_orig_ids | move.move_dest_ids: + from_loc, to_loc = m.location_id, m.location_dest_id + if float_compare(m.product_uom_qty, 0, precision_rounding=m.product_uom.rounding) < 0: + from_loc, to_loc = to_loc, from_loc + if to_loc == move.location_id: + orig_move_ids += m.ids + elif move.location_dest_id == from_loc: + dest_move_ids += m.ids + move.move_orig_ids, move.move_dest_ids = [(6, 0, orig_move_ids)], [(6, 0, dest_move_ids)] move.product_uom_qty *= -1 if move.picking_type_id.return_picking_type_id: move.picking_type_id = move.picking_type_id.return_picking_type_id + # We are returning some products, we must take them in the source location + move.procure_method = 'make_to_stock' neg_r_moves._assign_picking() # call `_action_assign` on every confirmed move which location_id bypasses the reservation + those expected to be auto-assigned