Merge lp:~akretion-team/openobject-addons/addons-purchase-extensible-action-picking-create into lp:openobject-addons

[Raphaël Valyi - http://www.akretion.com]

Hello,

This refactoring is the equivalent of the one you just merged in the sale module:
https://code.launchpad.net/~akretion-team/openobject-addons/addons-sale-extensible-action-ship-create/+merge/76609

But this time it's in the purchase module.
It breaks down the monolothic generation of the purchase picking and receptions moves and make sure those objects can be customized or localized before they are created into the database.
This is very useful in localizations such as for Brazil where some fiscal code must be propagated from the purchase to the supplier invoice, passing through the picking.

This refactoring also permit to have several pickings for one purchase order, something useful for blanket orders or lines with drop shipping for instance.

[Cloves Almeida (cjalmeida)]

It seems to me that, in line 63, the code would always create a new picking regardless a "picking_id" is provided.

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

Hello Colves,

Thank you for reviewing, you were absolutely right, we where creating the picking no matter what so our promise to be able to split the receptions was broken.

I fixed it in commit #5468

BTW, it buzzed me how this code would no create a picking when you buy service. But it's works because the purchase module checks that in the workflow. This is a different system that what is implemented in the sale module, I'm not sure if it's on purpose. Anyway it all works, I was just mentioning it's strange the design is different from the sale module.

Cloves, can you approve he review if you agree with the code now?
Dear OpenERP SA, thanks for merging this...

[Olivier Dony (Odoo) (odo-openerp)]

@Cloves: thanks for reviewing!

@Raphael:
This looks good, I will merge it after fixing some remaining glitches, such as:

 - remnants of copy/pasted `sale`-related stuff in the docstring
 - wrong indentation for move/picking confirmation code, causing useless repeated calls
 - passing down of *args without unpacking them, creating undesired args nesting (will correct this in `sale` too)
 - [order_line for order_line in order.order_line] -> order.order_line

And while doing this, we can also take the opportunity to:
 - change the return values to lists, as you suggested, which it should already have done
   (few cases where this return value was used can be fixed when porting modules to 6.1)
 - do some cleanup in existing code, e.g.:
   + order_line.product_id.product_tmpl_id.type -> order_line.product_id.type
   + self.pool.get('stock.move').write(cr, uid, [order_line.move_dest_id.id], {'location_id': order.location_id.id})
     should just be
     order_line.move_dest_id.write({'location_id': order.location_id.id})
   + etc.

BTW, great work on the docstring! :-)

Thank you this nice MP, and for keeping on improving OpenERP's business API and code extensibility!

review: Approve

[Olivier Dony (Odoo) (odo-openerp)]

Note: I forgot to mention that in order to bind the PO's workflow to a single picking subflow, the action_picking_create() method must only return one picking ID. By default we consider the first (and usually only) picking as the critical one for the PO workflow, but this can now be easily overridden if needed. I changed that and added a comment about it in the code.

Thanks!

=== modified file 'purchase/purchase.py'
--- purchase/purchase.py	2011-10-18 08:42:34 +0000
+++ purchase/purchase.py	2011-10-27 21:32:28 +0000
@@ -425,57 +425,83 @@
             self.log(cr, uid, id, message)
         return True
 
+    def _prepare_order_picking(self, cr, uid, order, *args):
+        pick_name = self.pool.get('ir.sequence').get(cr, uid, 'stock.picking.in')
+        istate = 'none'
+        if order.invoice_method=='picking':
+            istate = '2binvoiced'
+        return {
+            'name': pick_name,
+            'origin': order.name+((order.origin and (':'+order.origin)) or ''),
+            'type': 'in',
+            'address_id': order.dest_address_id.id or order.partner_address_id.id,
+            'invoice_state': istate,
+            'purchase_id': order.id,
+            'company_id': order.company_id.id,
+            'move_lines' : [],
+        }
+         
+    def _prepare_order_line_move(self, cr, uid, order, order_line, picking_id, *args):
+        return {
+            'name': order.name + ': ' +(order_line.name or ''),
+            'product_id': order_line.product_id.id,
+            'product_qty': order_line.product_qty,
+            'product_uos_qty': order_line.product_qty,
+            'product_uom': order_line.product_uom.id,
+            'product_uos': order_line.product_uom.id,
+            'date': order_line.date_planned,
+            'date_expected': order_line.date_planned,
+            'location_id': order.partner_id.property_stock_supplier.id,
+            'location_dest_id': order.location_id.id,
+            'picking_id': picking_id,
+            'address_id': order.dest_address_id.id or order.partner_address_id.id,
+            'move_dest_id': order_line.move_dest_id.id,
+            'state': 'draft',
+            'purchase_line_id': order_line.id,
+            'company_id': order.company_id.id,
+            'price_unit': order_line.price_unit
+        }
+
+    def _create_pickings(self, cr, uid, order, order_lines, picking_id=False, *args):
+        """
+        Creates pickings and appropriate stock moves for given order lines.
+
+        If ``picking_id`` is provided, the stock moves will be added to it, otherwise
+        a standard outgoing picking will be created to wrap the stock moves, as returned
+        by :meth:`~._prepare_order_picking`.
+
+        Modules that wish to customize the procurements or partition the stock moves over
+        multiple stock pickings may override this method and call ``super()`` with
+        different subsets of ``order_lines`` and/or preset ``picking_id`` values.
+
+        :param browse_record order: purchase order to which the order lines belong
+        :param list(browse_record) order_lines: sale order line records to procure
+        :param int picking_id: optional ID of a stock picking to which the created stock moves
+                               will be added. A new picking will be created if ommitted.
+        :return: True
+        """
+        if not picking_id: 
+            picking_id = self.pool.get('stock.picking').create(cr, uid, self._prepare_order_picking(cr, uid, order, args))
+        todo_moves = []
+        for order_line in order_lines:
+            if not order_line.product_id:
+                continue
+            if order_line.product_id.product_tmpl_id.type in ('product', 'consu'):
+                move = self.pool.get('stock.move').create(cr, uid, self._prepare_order_line_move(cr, uid, order, order_line, picking_id, args))
+                if order_line.move_dest_id:
+                    self.pool.get('stock.move').write(cr, uid, [order_line.move_dest_id.id], {'location_id': order.location_id.id})
+                todo_moves.append(move)
+                self.pool.get('stock.move').action_confirm(cr, uid, todo_moves)
+                self.pool.get('stock.move').force_assign(cr, uid, todo_moves)
+                wf_service = netsvc.LocalService("workflow")
+                wf_service.trg_validate(uid, 'stock.picking', picking_id, 'button_confirm', cr)
+            
+        return picking_id
+    
     def action_picking_create(self,cr, uid, ids, *args):
-        picking_id = False
         for order in self.browse(cr, uid, ids):
-            loc_id = order.partner_id.property_stock_supplier.id
-            istate = 'none'
-            if order.invoice_method=='picking':
-                istate = '2binvoiced'
-            pick_name = self.pool.get('ir.sequence').get(cr, uid, 'stock.picking.in')
-            picking_id = self.pool.get('stock.picking').create(cr, uid, {
-                'name': pick_name,
-                'origin': order.name+((order.origin and (':'+order.origin)) or ''),
-                'type': 'in',
-                'address_id': order.dest_address_id.id or order.partner_address_id.id,
-                'invoice_state': istate,
-                'purchase_id': order.id,
-                'company_id': order.company_id.id,
-                'move_lines' : [],
-            })
-            todo_moves = []
-            for order_line in order.order_line:
-                if not order_line.product_id:
-                    continue
-                if order_line.product_id.product_tmpl_id.type in ('product', 'consu'):
-                    dest = order.location_id.id
-                    move = self.pool.get('stock.move').create(cr, uid, {
-                        'name': order.name + ': ' +(order_line.name or ''),
-                        'product_id': order_line.product_id.id,
-                        'product_qty': order_line.product_qty,
-                        'product_uos_qty': order_line.product_qty,
-                        'product_uom': order_line.product_uom.id,
-                        'product_uos': order_line.product_uom.id,
-                        'date': order_line.date_planned,
-                        'date_expected': order_line.date_planned,
-                        'location_id': loc_id,
-                        'location_dest_id': dest,
-                        'picking_id': picking_id,
-                        'address_id': order.dest_address_id.id or order.partner_address_id.id,
-                        'move_dest_id': order_line.move_dest_id.id,
-                        'state': 'draft',
-                        'purchase_line_id': order_line.id,
-                        'company_id': order.company_id.id,
-                        'price_unit': order_line.price_unit
-                    })
-                    if order_line.move_dest_id:
-                        self.pool.get('stock.move').write(cr, uid, [order_line.move_dest_id.id], {'location_id':order.location_id.id})
-                    todo_moves.append(move)
-            self.pool.get('stock.move').action_confirm(cr, uid, todo_moves)
-            self.pool.get('stock.move').force_assign(cr, uid, todo_moves)
-            wf_service = netsvc.LocalService("workflow")
-            wf_service.trg_validate(uid, 'stock.picking', picking_id, 'button_confirm', cr)
-        return picking_id
+            picking_id = self._create_pickings(cr, uid, order, [order_line for order_line in order.order_line], None, args)
+        return picking_id #FIXME this is brittle to assume there is only 1 picking_id, but has been kept for API compatibility
 
     def copy(self, cr, uid, id, default=None, context=None):
         if not default:
@@ -490,7 +516,6 @@
         })
         return super(purchase_order, self).copy(cr, uid, id, default, context)
 
-
     def do_merge(self, cr, uid, ids, context=None):
         """
         To merge similar type of purchase orders.