Merge lp:~camptocamp/ocb-addons/7.0-fix-1238508-nbi into lp:ocb-addons

[Nicolas Bessi - Camptocamp]

Fix bug 1238508

[Stefan Rijnhart (Opener) (stefan-opener)]

Hi Nicolas,

thanks! Can you please explain why this works? To my untrained eye in this area, it seems as if you only put action_confirm and the write action into the loop. That would make sense if action_confirm only takes one id into account like some workflow methods, but that does not seem to be the case either.

review: Needs Information

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

The removed code is outside the for loop:

                  if todo:
19 - self.action_confirm(cr, uid, todo, context=context)
20 -
21 - self.write(cr, uid, move_ids, {'state': 'done', 'date': time.strftime(DEFAULT_SERVER_DATETIME_FORMAT)}, context=context)

So the moves pass to state done at the end of the loop.

But the in the loop they do this search:
other_upstream_move_ids = self.search(cr, uid, [('id','!=',move.id),('state','not in',['done','cancel']),
                                            ('move_dest_id','=',move.move_dest_id.id)], context=context)

that has for finality to check that the current move is the last one in state confirmed/assigned.
But as the confirmation and the write is done after the loop this will never happen.

I just put the removed code inside the current loop.

Regards

Nicolas

[Stefan Rijnhart (Opener) (stefan-opener)]

OK, thanks for clarifying! Code looks good.

review: Approve

[Holger Brunn (Therp) (hbrunn)]

review: Approve (code review)

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

To be more precise, the problem occurs when several moves have the same move_dest_id (which happens when a move is split into different production lots, e.g. with the product_serial community addon on which we are working), and then all moves are processed.

The problem does not happen if the moves are processed one by one.

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

I added a test, which I hope clarifies the case in which the issue occurs.

review: Approve (code review, automated test)

=== modified file 'stock/__openerp__.py'
--- stock/__openerp__.py	2013-08-23 13:20:49 +0000
+++ stock/__openerp__.py	2013-10-14 13:29:49 +0000
@@ -94,6 +94,7 @@
 #        'test/opening_stock.yml',
 #        'test/shipment.yml',
 #        'test/stock_report.yml',
+        'test/stock_move_chain_validation.yml',
     ],
     'installable': True,
     'application': True,

=== modified file 'stock/stock.py'
--- stock/stock.py	2013-10-04 12:22:57 +0000
+++ stock/stock.py	2013-10-14 13:29:49 +0000
@@ -2383,7 +2383,6 @@
                 todo.append(move.id)
         if todo:
             self.action_confirm(cr, uid, todo, context=context)
-            todo = []
 
         for move in self.browse(cr, uid, ids, context=context):
             if move.state in ['done','cancel']:
@@ -2407,12 +2406,11 @@
 
             self._create_product_valuation_moves(cr, uid, move, context=context)
             if move.state not in ('confirmed','done','assigned'):
-                todo.append(move.id)
-
-        if todo:
-            self.action_confirm(cr, uid, todo, context=context)
-
-        self.write(cr, uid, move_ids, {'state': 'done', 'date': time.strftime(DEFAULT_SERVER_DATETIME_FORMAT)}, context=context)
+                self.action_confirm(cr, uid, [move.id], context=context)
+            self.write(cr, uid, [move.id], 
+                       {'state': 'done', 
+                       'date': time.strftime(DEFAULT_SERVER_DATETIME_FORMAT)}, 
+                       context=context)
         for id in move_ids:
              wf_service.trg_trigger(uid, 'stock.move', id, cr)
 

=== added file 'stock/test/stock_move_chain_validation.yml'
--- stock/test/stock_move_chain_validation.yml	1970-01-01 00:00:00 +0000
+++ stock/test/stock_move_chain_validation.yml	2013-10-14 13:29:49 +0000
@@ -0,0 +1,98 @@
+-
+ I create an outgoing move for 2 LCD17
+-
+  !record {model: stock.move, id: dest_move_lcd17}:
+     product_qty: 2
+     product_id: product.product_product_7
+     product_uom: product.product_uom_unit
+     location_id: stock_location_components
+     location_dest_id: stock_location_customers
+-
+ I create 2 moves for 1 LCD17 chained with the outgoing one
+-
+  !record {model: stock.move, id: move_lcd17_1}:
+     product_qty: 1
+     product_id: product.product_product_7
+     product_uom: product.product_uom_unit
+     location_id: stock_location_stock
+     location_dest_id: stock_location_components
+     move_dest_id: dest_move_lcd17
+-
+  !record {model: stock.move, id: move_lcd17_2}:
+     product_qty: 1
+     product_id: product.product_product_7
+     product_uom: product.product_uom_unit
+     location_id: stock_location_stock
+     location_dest_id: stock_location_components
+     move_dest_id: dest_move_lcd17
+-
+ I confirm all moves
+-
+ !python {model: stock.move}: |
+   self.action_confirm(cr, uid, [ref('stock.dest_move_lcd17'),ref('stock.move_lcd17_1'),ref('stock.move_lcd17_2')], context=context)
+-
+  I process the 2 moves chained with the outgoing move
+-
+ !python {model: stock.move}: |
+   self.action_done(cr, uid, [ref('stock.move_lcd17_1'),ref('stock.move_lcd17_2')], context=context)
+-
+  the outgoing move must be assigned
+-
+ !python {model: stock.move}: |
+   move = self.browse(cr, uid, ref('stock.dest_move_lcd17'), context=context)
+   assert move.state == 'assigned', "out move was not assigned when internal moves where processed"
+-
+ I create an outgoing move for 2 LCD17
+-
+  !record {model: stock.move, id: dest_move2_lcd17}:
+     product_qty: 2
+     product_id: product.product_product_7
+     product_uom: product.product_uom_unit
+     location_id: stock_location_components
+     location_dest_id: stock_location_customers
+-
+ I create 2 moves for 1 LCD17 chained with the outgoing one
+-
+  !record {model: stock.move, id: move2_lcd17_1}:
+     product_qty: 1
+     product_id: product.product_product_7
+     product_uom: product.product_uom_unit
+     location_id: stock_location_stock
+     location_dest_id: stock_location_components
+     move_dest_id: dest_move2_lcd17
+-
+  !record {model: stock.move, id: move2_lcd17_2}:
+     product_qty: 1
+     product_id: product.product_product_7
+     product_uom: product.product_uom_unit
+     location_id: stock_location_stock
+     location_dest_id: stock_location_components
+     move_dest_id: dest_move2_lcd17
+-
+ I confirm all moves
+-
+ !python {model: stock.move}: |
+   self.action_confirm(cr, uid, [ref('stock.dest_move2_lcd17'),ref('stock.move2_lcd17_1'),ref('stock.move2_lcd17_2')], context=context)
+-
+  I process the 1st move chained with the outgoing move
+-
+ !python {model: stock.move}: |
+   self.action_done(cr, uid, [ref('stock.move2_lcd17_1')], context=context)
+-
+  the outgoing move must not be assigned
+-
+ !python {model: stock.move}: |
+   move = self.browse(cr, uid, ref('stock.dest_move2_lcd17'), context=context)
+   assert move.state != 'assigned', "out move was assigned when only 1st internal moves where processed"
+-
+  I process the 2nd move chained with the outgoing move
+-
+ !python {model: stock.move}: |
+   self.action_done(cr, uid, [ref('stock.move2_lcd17_2')], context=context)
+-
+  the outgoing move must not be assigned
+-
+ !python {model: stock.move}: |
+   move = self.browse(cr, uid, ref('stock.dest_move2_lcd17'), context=context)
+   assert move.state == 'assigned', "out move was not assigned when internal moves where processed"
+