Merge lp:~therp-nl/account-financial-tools/6.1-account-purchase-default into lp:~account-core-editors/account-financial-tools/6.1

[Ronald Portier (Therp)]

Resubmit based on 6.1 branch.

This branch makes it very easy to set the counterpart account to use on a supplier invoice.

For invoices created from purchase orders, or for buying products that are already in the database, this module is not usefull.

But for entering regular invoices for expenses like telecommunication bills, office supplies etc., not having to continually enter the account on which these expenses are to be booked is incredibly usefull and can save huge amounts of time.

This module offers the possibility to manually link a partner (supplier) to an expense account. But even better, it automatically "remembers" the expense account entered for a partner on an invoice - unless explicitly told not to do this.

Module might be extended to also cover manual account move entry. And maybe there is a use for having a similar function on sales invoices as well, altough I reasoned that usually you will sell product in the database, for which you can already set an income account.

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

Hi Ronald,

here are my nits:

l.17,42 The module name is not very clear. How about supplier_invoice_default_account?

l.48 Please set 'Therp B.V.' as author, as this prevents multiple 'Therp' entries showing up in the authors sesction of the Apps site and allows people to filter on all our addons by a single click.

l.59..62 init_xml and update_xml are deprecated. Please use the 'data' directive.

l.102 osv.osv is deprecated. Use orm.Model

l.108,109 use "context.get('partner_id')" and "context.get('type')"

l.126 typo 'in_refurd' -> 'in_refund'

l.112,130 You could consider using read() instead of browse(), as it adds less overhead and you only read a single field.

l.203,218 'data' tag at this level no longer necessary from v6.1 on.

review: Needs Fixing

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

l. 53: avoid hyphenation in the module description, as you don't know how the lines will be reformatted when the description is displayed. This is true for docstrings too.

review: Needs Fixing

[Ronald Portier (Therp) (rportier1962)]

I kept the browse instead of the read. According to the famous Technical Mememto from Stephane Wirtel, read() should be used from webservices, but internally browse() should always be used.

I could not find any real performance comparison between the two (which nmight be interesting to do). For the moment I left the browse calls.

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

A few suggested refactorings to improve code maintainability.

line 110: you check on line 112 for the truth of partner_id, so you can safely use the default default value in the call to dict.get()

line 112-113: you can get rid of the 'and invoice_type', since you have an in valid_value check just after this which will return False if invoice_type is None.
context['partner_id']

line 116: no need for another lookup of context['partner_id']: this value is partner_id from line 110

line 117: I personally prefer the attribute access on records, as in
    if partner_obj and partner_obj.property_account_expense:

 * Is there really a case where the partner_id you got from the context can be non existing? Apart from concurrent read / deletion, this sounds far fetched, so I'd be tempted to remove the first part of the test. That remark also applies to line 134.

line 136-137:
  * you are mixing attribute style access and dict style access.
  * the "or 0" bit is not necessary : the and expression will evaluate to False and replacing False by 0 does not change the outcome of the test line 138

line 138: please use "if a != b" rather than "if not a == b"

review: Needs Fixing (code review, no test)

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

Ronald,

you were right about the read/browse performance. Although I get a consistent performance gain of 50% if I do a large number of reads to get a single char field from res.partner when compared to browsing the resource, reading a fields.property using read or a browse record takes almost exactly the same time.

[Ronald Portier (Therp) (rportier1962)]

Stefan, I will take your performance tests in mind when reading single character (what about float and integer, I guess will be about the same?) fields in the future.

Alexandre: thanks for your comments, with one exception I will use them to update the code.

The exception is on the need to test for partner_obj... I never want to make assumptions in my code on the correctness of calling functions. So indeed, I think it is necesarry to handle the case of a non existing partner_id. Or the case that should be even more rare that a valid partner_id will still not result in a valid partner object. For instance when the connection with the database has been lost.

The only thing is what to do about it.

In most cases I would prefer to have an assertion error. partner_obj should exist. If it does not, it is most likely due to a programmer error. In this case, retrieving a default value for a column that a user can change anyway, I don't want to blow up - confront the user with an exception - when something goes wrong.

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

On ven. 01 févr. 2013 10:43:42 CET, Ronald Portier (Therp) wrote:
> Stefan, I will take your performance tests in mind when reading single character (what about float and integer, I guess will be about the same?) fields in the future.
>
> Alexandre: thanks for your comments, with one exception I will use them to update the code.
>
> The exception is on the need to test for partner_obj... I never want to make assumptions in my code on the correctness of calling functions. So indeed, I think it is necesarry to handle the case of a non existing partner_id. Or the case that should be even more rare that a valid partner_id will still not result in a valid partner object. For instance when the connection with the database has been lost.
>
> The only thing is what to do about it.
>
> In most cases I would prefer to have an assertion error. partner_obj should exist. If it does not, it is most likely due to a programmer error. In this case, retrieving a default value for a column that a user can change anyway, I don't want to blow up - confront the user with an exception - when something goes wrong.

Well... I cannot force you :-)

IMO, if you have a supposedly valid partner_id and browsing
model['res.partner'] for that id returns a browse_null instance, not
doing anything is wrong, because you are silencing a bug (you should
not have gotten that partner_id in the first place). At the very least,
this should be logged. And in this specific case, displaying the
exception to the user would be not be inappropriate (because it should
not be happening anyway). Si at the very least if you want to guard
against this, use an assert statement.

--
Alexandre Fayolle
Chef de Projet
Tel : + 33 (0)4 79 26 57 94

Camptocamp France SAS
Savoie Technolac, BP 352
73377 Le Bourget du Lac Cedex
http://www.camptocamp.com

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

LGTM

review: Approve (code review, no test)

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

review: Approve

=== added directory 'account_invoice_line_default_account'
=== added file 'account_invoice_line_default_account/__init__.py'
--- account_invoice_line_default_account/__init__.py	1970-01-01 00:00:00 +0000
+++ account_invoice_line_default_account/__init__.py	2013-02-01 09:54:22 +0000
@@ -0,0 +1,10 @@
+# -*- coding: UTF-8 -*-
+'''
+Created on 30 jan. 2013
+
+@author: Ronald Portier, Therp
+
[e-mail]
+http://www.therp.nl
+'''
+import model
 
=== added file 'account_invoice_line_default_account/__openerp__.py'
--- account_invoice_line_default_account/__openerp__.py	1970-01-01 00:00:00 +0000
+++ account_invoice_line_default_account/__openerp__.py	2013-02-01 09:54:22 +0000
@@ -0,0 +1,49 @@
+# -*- coding: utf-8 -*-
+##############################################################################
+#
+#    OpenERP, Open Source Management Solution
+#    This module copyright (C) 2012 Therp BV (<http://therp.nl>).
+#
+#    This program is free software: you can redistribute it and/or modify
+#    it under the terms of the GNU Affero General Public License as
+#    published by the Free Software Foundation, either version 3 of the
+#    License, or (at your option) any later version.
+#
+#    This program is distributed in the hope that it will be useful,
+#    but WITHOUT ANY WARRANTY; without even the implied warranty of
+#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+#    GNU Affero General Public License for more details.
+#
+#    You should have received a copy of the GNU Affero General Public License
+#    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+##############################################################################
+{
+    'name': 'Account Invoice Line Default Account',
+    'version': '6.1.r004',
+    'depends': [
+        'base',
+        'account'
+    ],
+    'author': 'Therp B.V.',
+    'category': 'Accounting',
+    'description': '''When entering purchase invoices directly, the user has
+to select an account which will be used as a counterpart in the generated
+move lines. However, each supplier will mostly be linked to one account. For
+instance when ordering paper from a supplier that deals in paper, the
+counterpart account will mostly be something like 'office expenses'. 
+This module will add a default counterpart account to a partner (supplier
+only), comparable to the similiar field in product. When a supplier invoice
+is entered, withouth a product, the field from partner will be used as default.
+Also when an expense account is entered on an invoice line (not automatically
+selectd for a product), the expense account will be automatically linked to
+the partner - unless explicitly disabled in the partner record.
+''',
+    'data': [
+        'view/res_partner_view.xml'
+    ],
+    'demo_xml': [],
+    'test': [],
+    'installable': True,
+    'active': False,
+}
 
=== added directory 'account_invoice_line_default_account/model'
=== added file 'account_invoice_line_default_account/model/__init__.py'
--- account_invoice_line_default_account/model/__init__.py	1970-01-01 00:00:00 +0000
+++ account_invoice_line_default_account/model/__init__.py	2013-02-01 09:54:22 +0000
@@ -0,0 +1,11 @@
+# -*- coding: UTF-8 -*-
+'''
+Created on 30 jan. 2013
+
+@author: Ronald Portier, Therp
+
[e-mail]
+http://www.therp.nl
+'''
+import res_partner
+import account_invoice_line
 
=== added file 'account_invoice_line_default_account/model/account_invoice_line.py'
--- account_invoice_line_default_account/model/account_invoice_line.py	1970-01-01 00:00:00 +0000
+++ account_invoice_line_default_account/model/account_invoice_line.py	2013-02-01 09:54:22 +0000
@@ -0,0 +1,52 @@
+# -*- coding: UTF-8 -*-
+'''
+Created on 30 jan. 2013
+
+@author: Ronald Portier, Therp
+
[e-mail]
+http://www.therp.nl
+'''
+from openerp.osv import orm
+
+
+class account_invoice_line(orm.Model):
+    _inherit = 'account.invoice.line'
+    
+    def _account_id_default(self, cr, uid, context=None):
+        if context is None:
+            context = {}
+        partner_id = context.get('partner_id')
+        invoice_type = context.get('type')
+        if  partner_id and invoice_type in ['in_invoice', 'in_refund']:
+            partner_model = self.pool.get('res.partner')
+            partner_obj = partner_model.browse(
+                cr, uid, partner_id, context=context)
+            if  partner_obj and partner_obj.property_account_expense:
+                return partner_obj.property_account_expense.id
+        return False
+
+    _defaults = {
+        'account_id': _account_id_default,
+    }
+    
+    def onchange_account_id(
+            self, cr, uid, ids, product_id, partner_id, inv_type,
+            fposition_id, account_id):
+        if (account_id and partner_id and (not product_id)
+        and inv_type in ['in_invoice', 'in_refund']):
+            # We have an account_id, and is not from a product, so
+            # store it in partner automagically:
+            partner_model = self.pool.get('res.partner')
+            partner_obj = partner_model.browse(cr, uid, partner_id)
+            if  partner_obj and partner_obj.auto_update_account_expense:
+                old_account_id = (
+                    partner_obj.property_account_expense
+                    and partner_obj.property_account_expense.id)
+                if  account_id != old_account_id:
+                    # only write when something really changed
+                    vals = {'property_account_expense': account_id}
+                    partner_obj.write(vals)
+        return super(account_invoice_line, self).onchange_account_id(
+                cr, uid, ids, product_id, partner_id, inv_type,
+                fposition_id, account_id)
 
=== added file 'account_invoice_line_default_account/model/res_partner.py'
--- account_invoice_line_default_account/model/res_partner.py	1970-01-01 00:00:00 +0000
+++ account_invoice_line_default_account/model/res_partner.py	2013-02-01 09:54:22 +0000
@@ -0,0 +1,34 @@
+# -*- coding: UTF-8 -*-
+'''
+Created on 30 jan. 2013
+
+@author: Ronald Portier, Therp
+
[e-mail]
+http://www.therp.nl
+'''
+from openerp.osv import orm
+from openerp.osv import fields
+
+
+class res_partner(orm.Model):
+    _inherit = 'res.partner'
+    
+    _columns = {
+        'property_account_expense': fields.property(
+            'account.account',
+            type='many2one',
+            relation='account.account',
+            string='Default expense account',
+            view_load=True,
+            domain='''[('user_type.report_type', '=', 'expense')]''',
+            help='Default counterpart account for purchases',
+            required=False),
+        'auto_update_account_expense': fields.boolean(
+            'Save account selected on invoice',
+            help='When account selected on invoice, automatically save it'),
+    }
+    
+    _defaults = {
+        'auto_update_account_expense': True,
+    }
 
=== added directory 'account_invoice_line_default_account/view'
=== added file 'account_invoice_line_default_account/view/res_partner_view.xml'
--- account_invoice_line_default_account/view/res_partner_view.xml	1970-01-01 00:00:00 +0000
+++ account_invoice_line_default_account/view/res_partner_view.xml	2013-02-01 09:54:22 +0000
@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="utf-8"?>
+<openerp>
+    <data>
+        <record
+            id="view_partner_expense_property_form"
+            model="ir.ui.view">
+            <field name="name">res.partner.property.form.inherit</field>
+            <field name="model">res.partner</field>
+            <field name="type">form</field>
+            <field name="priority">2</field>
+            <field
+                name="inherit_id"
+                ref="account.view_partner_property_form" />
+            <field
+                name="arch"
+                type="xml">
+                    <field
+                        name="property_account_payable"
+                        position="after">
+                        <group
+                            colspan="2"
+                            col="4">
+                            <field
+                                name="property_account_expense"
+                                groups="account.group_account_invoice" />
+                            <field
+                                name="auto_update_account_expense"
+                                groups="account.group_account_invoice" />
+                        </group>
+                    </field>
+            </field>
+        </record>
+    </data>
+</openerp>