Merge lp:~akretion-team/openobject-addons/trunk-payment-term-on-purchase into lp:openobject-addons

[Alexis de Lattre]

On the sale orders, we have a 'Payment term' field (copied from the partner by the onchange on partner_id) that we can personnalise for a particular sale order, and that is used when generating the invoice... but there is no such thing on purchase orders !

This merge proposal adds payment_term on purchase.order object.
The behavior of payment_term is the same as for the sale order :
  - the payment_term is copied from the partner to the purchase order (by the onchange on partner_id)
  - then it is copied from the purchase order to the supplier invoice

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

Hello Alexis,

this feature is long awaited and Renato and me confirm such a merge would save us from always doing this customization.

Now, I think you did little mistakes in manipulating Python boolean expressions, look lines 30 and 31 of the diff, you have code like:
'fiscal_position': supplier.property_account_position.id or False
it should instead be supplier.property_account_position and supplier.property_account_position.id or False.id or False
I believe there is the same error line 51 also.

Please fix that then it would be OK for me. Thanks for all that work, this is very good for the OpenERP product.

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

review: Needs Fixing

[lp:~akretion-team/openobject-addons/trunk-payment-term-on-purchase]

6457. By Alexis de Lattre on 2012-01-31

Remove my code change on fiscal_position (it had no impact... but I guess it is better not to change anything else)

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

review: Approve

[qdp (OpenERP) (qdp)]

Hi Alexis,

it's a pleasure to see the openerp community contributing for a better product and new features, although here i somewhat have some reserves:

*first of all: i think you should better handle the cases where no payment_term are given on the PO. For example, the line 90 of your diff:
    'payment_term': orders[0].payment_term.id,
could be changed into:
    'payment_term': orders[0].payment_term and orders[0].payment_term.id or pay_term,

The same remark applies for line 41.

*Another point, more problematic but not under your responsibility: the field used as payment term for suppliers is the same as for customers. Obviously setting a payment term for the sales you'll make for a given partner doesn't assume any reciprocity when you will purchase to him. Those 2 different concepts should be depicted in different fields...

It was already there in the onchange of partner in supplier invoices for example, but i'd rather see merge proposals fixing this lack in the design rather that proposals using and going further in that bad idea.

*Last but not least: i'm not sure this enhancement should be integrated in the core. I'd see it more like an optional module available for those who need it. So i think you should bundle it as a module overwriting the existing behavior and publish it on apps.openerp.com (of course if you need to refactor some code during that process, we will gladly review/accept your merge proposals).

The reason why i have such a feeling is that i believe the added-value won't be that huge compared to the effort and complexity we will add.

So for these reasons, i'm rejecting this merge proposal. I hope i made it clear and that it won't slow you down for your next contributions!

Best Regards,
Quentin

review: Disapprove

[Alexis de Lattre (alexis-via)]

OK, I understand your point about the need for 2 payment terms : one for partner-as-supplier and one for partner-as-customer. I am ready to propose a new merge proposal that adds a field "property_payment_term_supplier" on res.partner (if you prefer another name, please tell me) and modifies the on_change on the invoice and all related code.

I am surprised when you say "i'm not sure this enhancement should be integrated in the core". I would just like to tell you the feedback of our OpenERP customers ; they're saying : "I can change the 'payment term' on a sale order... why isn't it possible to do the same on a purchase order ?" If the feature is native for the sale order, I think it should also be native on purchase orders.

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

Hi,

I must say I completely agree with Alexis on that one. If you have some options available in the SO, then you should be able to select the same on the PO... At least all customers wish that.

+1 to accept this merge

Regards,

Joël

Unmerged revisions

6457. By Alexis de Lattre on 2012-01-31

Remove my code change on fiscal_position (it had no impact... but I guess it is better not to change anything else)

6456. By Alexis de Lattre on 2012-01-31

Add payment_term on purchase.order object.
The behavior of payment_term is the same as for the sale order :
- the payment_term is copied from the partner to the purchase order
- then it is copied from the purchase order to the supplier invoice
Add payment_term to the purchase.order form view, next to fiscal position (same as for sale.order form view)

=== modified file 'purchase/purchase.py'
--- purchase/purchase.py	2012-01-27 04:43:26 +0000
+++ purchase/purchase.py	2012-01-31 22:52:26 +0000
@@ -204,6 +204,7 @@
                 'purchase.order.line': (_get_order, None, 10),
             }, multi="sums",help="The total amount"),
         'fiscal_position': fields.many2one('account.fiscal.position', 'Fiscal Position'),
+        'payment_term': fields.many2one('account.payment.term', 'Payment Term'),
         'product_id': fields.related('order_line','product_id', type='many2one', relation='product.product', string='Product'),
         'create_uid':  fields.many2one('res.users', 'Responsible'),
         'company_id': fields.many2one('res.company','Company',required=True,select=1),
@@ -266,12 +267,19 @@
     def onchange_partner_id(self, cr, uid, ids, partner_id):
         partner = self.pool.get('res.partner')
         if not partner_id:
-            return {'value':{'partner_address_id': False, 'fiscal_position': False}}
+            return {'value':{
+                'partner_address_id': False,
+                'fiscal_position': False,
+                'payment_term': False
+                }}
         supplier_address = partner.address_get(cr, uid, [partner_id], ['default'])
         supplier = partner.browse(cr, uid, partner_id)
-        pricelist = supplier.property_product_pricelist_purchase.id
-        fiscal_position = supplier.property_account_position and supplier.property_account_position.id or False
-        return {'value':{'partner_address_id': supplier_address['default'], 'pricelist_id': pricelist, 'fiscal_position': fiscal_position}}
+        return {'value': {
+            'partner_address_id': supplier_address['default'],
+            'pricelist_id': supplier.property_product_pricelist_purchase.id,
+            'fiscal_position': supplier.property_account_position and supplier.property_account_position.id or False,
+            'payment_term': supplier.property_payment_term.id or False
+            }}
 
     def wkf_approve_order(self, cr, uid, ids, context=None):
         self.write(cr, uid, ids, {'state': 'approved', 'date_approve': time.strftime('%Y-%m-%d')})
@@ -382,7 +390,7 @@
                 '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,
+                'payment_term': order.payment_term.id,
                 'company_id': order.company_id.id,
             }
             inv_id = inv_obj.create(cr, uid, inv_data, context=context)
@@ -915,7 +923,8 @@
                 'pricelist_id': pricelist_id,
                 'date_order': purchase_date.strftime(DEFAULT_SERVER_DATETIME_FORMAT),
                 'company_id': procurement.company_id.id,
-                'fiscal_position': partner.property_account_position and partner.property_account_position.id or False
+                'fiscal_position': partner.property_account_position and partner.property_account_position.id or False,
+                'payment_term': partner.property_payment_term.id or False
             }
             res[procurement.id] = self.create_procurement_purchase_order(cr, uid, procurement, po_vals, line_vals, context=context)
             self.write(cr, uid, [procurement.id], {'state': 'running', 'purchase_id': res[procurement.id]})

=== modified file 'purchase/purchase_view.xml'
--- purchase/purchase_view.xml	2012-01-27 04:43:26 +0000
+++ purchase/purchase_view.xml	2012-01-31 22:52:26 +0000
@@ -217,6 +217,7 @@
                             <group colspan="2" col="2">
                                 <separator string="Invoice Control" colspan="2"/>
                                 <field name="invoice_method"/>
+                                <field name="payment_term" widget="selection"/>
                                 <field name="fiscal_position" widget="selection"/>
                             </group>
                             <newline/>

=== modified file 'purchase/stock.py'
--- purchase/stock.py	2012-01-31 10:53:22 +0000
+++ purchase/stock.py	2012-01-31 22:52:26 +0000
@@ -74,6 +74,7 @@
         if picking.purchase_id:
             invoice_vals['address_contact_id'], invoice_vals['address_invoice_id'] = \
                 self.pool.get('res.partner').address_get(cr, uid, [partner.id], ['contact', 'invoice']).values()
+            invoice_vals['payment_term'] = picking.purchase_id.payment_term.id
             invoice_vals['fiscal_position'] = picking.purchase_id.fiscal_position.id
         return invoice_vals
 

=== modified file 'purchase/wizard/purchase_line_invoice.py'
--- purchase/wizard/purchase_line_invoice.py	2011-09-08 09:27:55 +0000
+++ purchase/wizard/purchase_line_invoice.py	2012-01-31 22:52:26 +0000
@@ -74,10 +74,6 @@
                 journal_id = account_jrnl_obj.search(cr, uid, [('type', '=', 'purchase')], context=None)
                 journal_id = journal_id and journal_id[0] or False
                 a = partner.property_account_payable.id
-                if partner and partner.property_payment_term.id:
-                    pay_term = partner.property_payment_term.id
-                else:
-                    pay_term = False
                 inv = {
                     'name': name,
                     'origin': name,
@@ -91,7 +87,7 @@
                     'invoice_line': [(6,0,lines_ids)],
                     'currency_id' : orders[0].pricelist_id.currency_id.id,
                     'comment': multiple_order_invoice_notes(orders),
-                    'payment_term': pay_term,
+                    'payment_term': orders[0].payment_term.id,
                     'fiscal_position': partner.property_account_position.id
                 }
                 inv_id = invoice_obj.create(cr, uid, inv)