Merge lp:~openerp-dev/openobject-addons/trunk-bug-724131-rpa into lp:openobject-addons

[Rucha (Open ERP)]

lp:724131: Fixed missing relation between PO line and Invoice line when purchase order invoice control is Based on generated invoice,

[qdp (OpenERP) (qdp)]

Hello Rucha,

just few comments:
1) can you make a new function to retrieve the dict to use to create the invoice line, _prepare_invoice_line_data(), in order to increase the inheritability and flexibility of our code

2) really strange: i didn't know that the field invoice_id on account.invoice.line wasn't mandatory! anyway, for performance reasons it would be better to create the invoice with no lines, then to create the lines with the invoice_id set.

3) i don't like this syntax. Is there a reason to do it? a good one? if no, i propose that you let it like that to keep the code as much consistent as possible.
75 - self.write(cr, uid, [o.id], {'invoice_ids': [(4, inv_id)]})
78 + o.write({'invoice_ids': [(4, inv_id)]})

4) we need more comments, more docstrings in the new functions

5) some context may be added in function calls, from here to there, in the lines you already changed

Thanks

review: Needs Fixing

[qdp (OpenERP) (qdp)]

just a thought for point 2: not sure that doing like this will call only once the fonctional fields of the invoice. Test it and if it's not the case then you can forget this point.

thanks

[Rucha (Open ERP) (rpa-openerp)]

Thanks Quentin for your time,
my reply on your comments

> just few comments:
> 1) can you make a new function to retrieve the dict to use to create the invoice line,
> _prepare_invoice_line_data(), in order to increase the inheritability and flexibility of our code

I can not make a new function as inv_line_create function is used in other modules too, and might be used somewhere else in other community modules. I just changed signature of inv_line_create to return dict of data and this will be less risky as it requires less changes in overridden method.

> 2) really strange: i didn't know that the field invoice_id on account.invoice.line wasn't mandatory! anyway, for performance reasons it would be better to create the invoice with no lines, then to create the lines with the invoice_id set.

I found its good "to create the invoice with no lines, then to create the lines with the invoice_id set."
  so I did that.

3) i don't like this syntax. Is there a reason to do it? a good one? if no, i propose that you let it like that to keep the code as much consistent as possible.
75 - self.write(cr, uid, [o.id], {'invoice_ids': [(4, inv_id)]})
78 + o.write({'invoice_ids': [(4, inv_id)]})

> 4) we need more comments, more docstrings in the new functions

Added!

> 5) some context may be added in function calls, from here to there, in the lines you already changed

true, I just tried to keep it more clean in order to focus on only fix, but anyways I agree on that point and improved.

And as a bonus I changed stupid variable names a, o, il ..
this will cost you more to review but will be better, what do you think?

Thanks

review: Needs Resubmitting

[Rucha (Open ERP) (rpa-openerp)]

> 3) i don't like this syntax. Is there a reason to do it? a good one? if no, i propose that you let it like that to keep the code as much consistent as possible.
> 75 - self.write(cr, uid, [o.id], {'invoice_ids': [(4, inv_id)]})
> 78 + o.write({'invoice_ids': [(4, inv_id)]})

as now we use attributes of self.pool.get() for browse_record too, thats quite good, easy and consistent too (if we make practice of using that.)

[qdp (OpenERP) (qdp)]

> just few comments:
> 1) can you make a new function to retrieve the dict to use to create the invoice line,
> _prepare_invoice_line_data(), in order to increase the inheritability and flexibility of our code

I can not make a new function as inv_line_create function is used in other modules too, and might be used somewhere else in other community modules. I just changed signature of inv_line_create to return dict of data and this will be less risky as it requires less changes in overridden method.

=> o_O geez.. no you're wrong changing the signature is the same as it break the API too, maybe even worst as it may silent errors in some cases. Thus changing the name makes it more clear and that the good opportunity do to it.

>And as a bonus I changed stupid variable names a, o, il ..
>this will cost you more to review but will be better, what do you think?
=> that's an excelent idea

great work, i merged after changing the name of function

=== modified file 'account_anglo_saxon/purchase.py'
--- account_anglo_saxon/purchase.py	2011-10-27 21:11:24 +0000
+++ account_anglo_saxon/purchase.py	2011-11-15 11:51:27 +0000
@@ -26,8 +26,8 @@
     _inherit = "purchase.order"
     _description = "Purchase Order"
 
-    def inv_line_create(self, cr, uid, a, ol):
-        line = super(purchase_order, self).inv_line_create(cr, uid, a, ol)
+    def prepare_inv_line(self, cr, uid, account_id, order_line, context=None):
+        line = super(purchase_order, self).prepare_inv_line(cr, uid, account_id, order_line, context=context)
         if ol.product_id and not ol.product_id.type == 'service':
             oa = ol.product_id.property_stock_account_input and ol.product_id.property_stock_account_input.id
             if not oa:
@@ -35,6 +35,6 @@
             if oa:
                 fpos = ol.order_id.fiscal_position or False
                 a = self.pool.get('account.fiscal.position').map_account(cr, uid, fpos, oa)
-                line[2].update({'account_id': a})
+                line.update({'account_id': a})
         return line
 purchase_order()

=== modified file 'purchase/purchase.py'
--- purchase/purchase.py	2011-11-14 11:18:10 +0000
+++ purchase/purchase.py	2011-11-15 11:51:27 +0000
@@ -319,17 +319,25 @@
                        'ref_partner_id': po.partner_id.id,
                        'ref_doc1': 'purchase.order,%d' % (po.id,),
                 })
-    def inv_line_create(self, cr, uid, a, ol):
-        return (0, False, {
-            'name': ol.name,
-            'account_id': a,
-            'price_unit': ol.price_unit or 0.0,
-            'quantity': ol.product_qty,
-            'product_id': ol.product_id.id or False,
-            'uos_id': ol.product_uom.id or False,
-            'invoice_line_tax_id': [(6, 0, [x.id for x in ol.taxes_id])],
-            'account_analytic_id': ol.account_analytic_id.id or False,
-        })
+
+    def prepare_inv_line(self, cr, uid, account_id, order_line, context=None):
+        """Collects require data from purchase order line that is used to create invoice line 
+        for that purchase order line
+        :param account_id: Expense account of the product of PO line if any.
+        :param browse_record order_line: Purchase order line browse record
+        :return: Value for fields of invoice lines.
+        :rtype: dict
+        """
+        return {
+            'name': order_line.name,
+            'account_id': account_id,
+            'price_unit': order_line.price_unit or 0.0,
+            'quantity': order_line.product_qty,
+            'product_id': order_line.product_id.id or False,
+            'uos_id': order_line.product_uom.id or False,
+            'invoice_line_tax_id': [(6, 0, [x.id for x in order_line.taxes_id])],
+            'account_analytic_id': order_line.account_analytic_id.id or False,
+        }
 
     def action_cancel_draft(self, cr, uid, ids, *args):
         if not len(ids):
@@ -345,55 +353,71 @@
             self.log(cr, uid, id, message)
         return True
 
-    #TOFIX
-    # - implement hook method on create invoice and invoice line
-    # - doc string
-    def action_invoice_create(self, cr, uid, ids, *args):
+    def action_invoice_create(self, cr, uid, ids, context=None):
+        """Generates invoice for given ids of purchase orders and links that invoice ID to purchase order.
+        :param ids: list of ids of purchase orders.
+        :return: ID of created invoice.
+        :rtype: int
+        """
         res = False
 
         journal_obj = self.pool.get('account.journal')
-        for o in self.browse(cr, uid, ids):
-            il = []
-            todo = []
-            for ol in o.order_line:
-                todo.append(ol.id)
-                if ol.product_id:
-                    a = ol.product_id.product_tmpl_id.property_account_expense.id
-                    if not a:
-                        a = ol.product_id.categ_id.property_account_expense_categ.id
-                    if not a:
-                        raise osv.except_osv(_('Error !'), _('There is no expense account defined for this product: "%s" (id:%d)') % (ol.product_id.name, ol.product_id.id,))
-                else:
-                    a = self.pool.get('ir.property').get(cr, uid, 'property_account_expense_categ', 'product.category').id
-                fpos = o.fiscal_position or False
-                a = self.pool.get('account.fiscal.position').map_account(cr, uid, fpos, a)
-                il.append(self.inv_line_create(cr, uid, a, ol))
+        inv_obj = self.pool.get('account.invoice')
+        inv_line_obj = self.pool.get('account.invoice.line')
+        fiscal_obj = self.pool.get('account.fiscal.position')
+        property_obj = self.pool.get('ir.property')
 
-            a = o.partner_id.property_account_payable.id
-            journal_ids = journal_obj.search(cr, uid, [('type', '=','purchase'),('company_id', '=', o.company_id.id)], limit=1)
+        for order in self.browse(cr, uid, ids, context=context):
+            pay_acc_id = order.partner_id.property_account_payable.id
+            journal_ids = journal_obj.search(cr, uid, [('type', '=','purchase'),('company_id', '=', order.company_id.id)], limit=1)
             if not journal_ids:
                 raise osv.except_osv(_('Error !'),
-                    _('There is no purchase journal defined for this company: "%s" (id:%d)') % (o.company_id.name, o.company_id.id))
-            inv = {
-                'name': o.partner_ref or o.name,
-                'reference': o.partner_ref or o.name,
-                'account_id': a,
+                    _('There is no purchase journal defined for this company: "%s" (id:%d)') % (order.company_id.name, order.company_id.id))
+
+            # generate invoice line correspond to PO line and link that to created invoice (inv_id) and PO line
+            inv_lines = []
+            for po_line in order.order_line:
+                if po_line.product_id:
+                    acc_id = po_line.product_id.product_tmpl_id.property_account_expense.id
+                    if not acc_id:
+                        acc_id = po_line.product_id.categ_id.property_account_expense_categ.id
+                    if not acc_id:
+                        raise osv.except_osv(_('Error !'), _('There is no expense account defined for this product: "%s" (id:%d)') % (po_line.product_id.name, po_line.product_id.id,))
+                else:
+                    acc_id = property_obj.get(cr, uid, 'property_account_expense_categ', 'product.category').id
+                fpos = order.fiscal_position or False
+                acc_id = fiscal_obj.map_account(cr, uid, fpos, acc_id)
+
+                inv_line_data = self.prepare_inv_line(cr, uid, acc_id, po_line, context=context)
+                inv_line_id = inv_line_obj.create(cr, uid, inv_line_data, context=context)
+                inv_lines.append(inv_line_id)
+
+                po_line.write({'invoiced':True, 'invoice_lines': [(4, inv_line_id)]}, context=context)
+
+            # get invoice data and create invoice
+            inv_data = {
+                'name': order.partner_ref or order.name,
+                'reference': order.partner_ref or order.name,
+                'account_id': pay_acc_id,
                 'type': 'in_invoice',
-                'partner_id': o.partner_id.id,
-                'currency_id': o.pricelist_id.currency_id.id,
-                'address_invoice_id': o.partner_address_id.id,
-                'address_contact_id': o.partner_address_id.id,
+                'partner_id': order.partner_id.id,
+                'currency_id': order.pricelist_id.currency_id.id,
+                'address_invoice_id': order.partner_address_id.id,
+                'address_contact_id': order.partner_address_id.id,
                 'journal_id': len(journal_ids) and journal_ids[0] or False,
-                'origin': o.name,
-                'invoice_line': il,
-                'fiscal_position': o.fiscal_position.id or o.partner_id.property_account_position.id,
-                'payment_term': o.partner_id.property_payment_term and o.partner_id.property_payment_term.id or False,
-                'company_id': o.company_id.id,
+                'invoice_line': [(6, 0, inv_lines)], 
+                'origin': order.name,
+                'fiscal_position': order.fiscal_position.id or order.partner_id.property_account_position.id,
+                'payment_term': order.partner_id.property_payment_term and order.partner_id.property_payment_term.id or False,
+                'company_id': order.company_id.id,
             }
-            inv_id = self.pool.get('account.invoice').create(cr, uid, inv, {'type':'in_invoice'})
-            self.pool.get('account.invoice').button_compute(cr, uid, [inv_id], {'type':'in_invoice'}, set_total=True)
-            self.pool.get('purchase.order.line').write(cr, uid, todo, {'invoiced':True})
-            self.write(cr, uid, [o.id], {'invoice_ids': [(4, inv_id)]})
+            inv_id = inv_obj.create(cr, uid, inv_data, context=context)
+
+            # compute the invoice
+            inv_obj.button_compute(cr, uid, [inv_id], context=context, set_total=True)
+
+            # Link this new invoice to related purchase order
+            order.write({'invoice_ids': [(4, inv_id)]}, context=context)
             res = inv_id
         return res
 

=== modified file 'purchase_analytic_plans/purchase_analytic_plans.py'
--- purchase_analytic_plans/purchase_analytic_plans.py	2011-01-14 00:11:01 +0000
+++ purchase_analytic_plans/purchase_analytic_plans.py	2011-11-15 11:51:27 +0000
@@ -35,9 +35,9 @@
     _name='purchase.order'
     _inherit='purchase.order'
 
-    def inv_line_create(self, cr, uid, a, ol):
-        res=super(purchase_order,self).inv_line_create(cr, uid, a, ol)
-        res[2]['analytics_id']=ol.analytics_id.id
+    def prepare_inv_line(self, cr, uid, account_id, order_line, context=None):
+        res = super(purchase_order, self).prepare_inv_line(cr, uid, account_id, order_line, context=context)
+        res['analytics_id'] = order_line.analytics_id.id
         return res
 
 purchase_order()