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

Proposed by Ronald Portier (Therp) on 2013-02-01
Status: Merged
Merged at revision: 101
Proposed branch: lp:~therp-nl/account-financial-tools/6.1-account-purchase-default
Merge into: lp:~account-core-editors/account-financial-tools/6.1
Diff against target: 222 lines (+190/-0)
6 files modified
account_invoice_line_default_account/__init__.py (+10/-0)
account_invoice_line_default_account/__openerp__.py (+49/-0)
account_invoice_line_default_account/model/__init__.py (+11/-0)
account_invoice_line_default_account/model/account_invoice_line.py (+52/-0)
account_invoice_line_default_account/model/res_partner.py (+34/-0)
account_invoice_line_default_account/view/res_partner_view.xml (+34/-0)
To merge this branch: bzr merge lp:~therp-nl/account-financial-tools/6.1-account-purchase-default
Reviewer Review Type Date Requested Status
Stefan Rijnhart (Opener) 2013-02-01 Approve on 2013-02-01
Alexandre Fayolle - camptocamp code review, no test 2013-02-01 Approve on 2013-02-01
Review via email: mp+146082@code.launchpad.net

This proposal supersedes a proposal from 2013-02-01.

Description of the change

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.

To post a comment you must log in.
Stefan Rijnhart (Opener) (stefan-opener) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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

LGTM

review: Approve (code review, no test)
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added directory 'account_invoice_line_default_account'
2=== added file 'account_invoice_line_default_account/__init__.py'
3--- account_invoice_line_default_account/__init__.py 1970-01-01 00:00:00 +0000
4+++ account_invoice_line_default_account/__init__.py 2013-02-01 09:54:22 +0000
5@@ -0,0 +1,10 @@
6+# -*- coding: UTF-8 -*-
7+'''
8+Created on 30 jan. 2013
9+
10+@author: Ronald Portier, Therp
11+
12+rportier@therp.nl
13+http://www.therp.nl
14+'''
15+import model
16
17=== added file 'account_invoice_line_default_account/__openerp__.py'
18--- account_invoice_line_default_account/__openerp__.py 1970-01-01 00:00:00 +0000
19+++ account_invoice_line_default_account/__openerp__.py 2013-02-01 09:54:22 +0000
20@@ -0,0 +1,49 @@
21+# -*- coding: utf-8 -*-
22+##############################################################################
23+#
24+# OpenERP, Open Source Management Solution
25+# This module copyright (C) 2012 Therp BV (<http://therp.nl>).
26+#
27+# This program is free software: you can redistribute it and/or modify
28+# it under the terms of the GNU Affero General Public License as
29+# published by the Free Software Foundation, either version 3 of the
30+# License, or (at your option) any later version.
31+#
32+# This program is distributed in the hope that it will be useful,
33+# but WITHOUT ANY WARRANTY; without even the implied warranty of
34+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
35+# GNU Affero General Public License for more details.
36+#
37+# You should have received a copy of the GNU Affero General Public License
38+# along with this program. If not, see <http://www.gnu.org/licenses/>.
39+#
40+##############################################################################
41+{
42+ 'name': 'Account Invoice Line Default Account',
43+ 'version': '6.1.r004',
44+ 'depends': [
45+ 'base',
46+ 'account'
47+ ],
48+ 'author': 'Therp B.V.',
49+ 'category': 'Accounting',
50+ 'description': '''When entering purchase invoices directly, the user has
51+to select an account which will be used as a counterpart in the generated
52+move lines. However, each supplier will mostly be linked to one account. For
53+instance when ordering paper from a supplier that deals in paper, the
54+counterpart account will mostly be something like 'office expenses'.
55+This module will add a default counterpart account to a partner (supplier
56+only), comparable to the similiar field in product. When a supplier invoice
57+is entered, withouth a product, the field from partner will be used as default.
58+Also when an expense account is entered on an invoice line (not automatically
59+selectd for a product), the expense account will be automatically linked to
60+the partner - unless explicitly disabled in the partner record.
61+''',
62+ 'data': [
63+ 'view/res_partner_view.xml'
64+ ],
65+ 'demo_xml': [],
66+ 'test': [],
67+ 'installable': True,
68+ 'active': False,
69+}
70
71=== added directory 'account_invoice_line_default_account/model'
72=== added file 'account_invoice_line_default_account/model/__init__.py'
73--- account_invoice_line_default_account/model/__init__.py 1970-01-01 00:00:00 +0000
74+++ account_invoice_line_default_account/model/__init__.py 2013-02-01 09:54:22 +0000
75@@ -0,0 +1,11 @@
76+# -*- coding: UTF-8 -*-
77+'''
78+Created on 30 jan. 2013
79+
80+@author: Ronald Portier, Therp
81+
82+rportier@therp.nl
83+http://www.therp.nl
84+'''
85+import res_partner
86+import account_invoice_line
87
88=== added file 'account_invoice_line_default_account/model/account_invoice_line.py'
89--- account_invoice_line_default_account/model/account_invoice_line.py 1970-01-01 00:00:00 +0000
90+++ account_invoice_line_default_account/model/account_invoice_line.py 2013-02-01 09:54:22 +0000
91@@ -0,0 +1,52 @@
92+# -*- coding: UTF-8 -*-
93+'''
94+Created on 30 jan. 2013
95+
96+@author: Ronald Portier, Therp
97+
98+rportier@therp.nl
99+http://www.therp.nl
100+'''
101+from openerp.osv import orm
102+
103+
104+class account_invoice_line(orm.Model):
105+ _inherit = 'account.invoice.line'
106+
107+ def _account_id_default(self, cr, uid, context=None):
108+ if context is None:
109+ context = {}
110+ partner_id = context.get('partner_id')
111+ invoice_type = context.get('type')
112+ if partner_id and invoice_type in ['in_invoice', 'in_refund']:
113+ partner_model = self.pool.get('res.partner')
114+ partner_obj = partner_model.browse(
115+ cr, uid, partner_id, context=context)
116+ if partner_obj and partner_obj.property_account_expense:
117+ return partner_obj.property_account_expense.id
118+ return False
119+
120+ _defaults = {
121+ 'account_id': _account_id_default,
122+ }
123+
124+ def onchange_account_id(
125+ self, cr, uid, ids, product_id, partner_id, inv_type,
126+ fposition_id, account_id):
127+ if (account_id and partner_id and (not product_id)
128+ and inv_type in ['in_invoice', 'in_refund']):
129+ # We have an account_id, and is not from a product, so
130+ # store it in partner automagically:
131+ partner_model = self.pool.get('res.partner')
132+ partner_obj = partner_model.browse(cr, uid, partner_id)
133+ if partner_obj and partner_obj.auto_update_account_expense:
134+ old_account_id = (
135+ partner_obj.property_account_expense
136+ and partner_obj.property_account_expense.id)
137+ if account_id != old_account_id:
138+ # only write when something really changed
139+ vals = {'property_account_expense': account_id}
140+ partner_obj.write(vals)
141+ return super(account_invoice_line, self).onchange_account_id(
142+ cr, uid, ids, product_id, partner_id, inv_type,
143+ fposition_id, account_id)
144
145=== added file 'account_invoice_line_default_account/model/res_partner.py'
146--- account_invoice_line_default_account/model/res_partner.py 1970-01-01 00:00:00 +0000
147+++ account_invoice_line_default_account/model/res_partner.py 2013-02-01 09:54:22 +0000
148@@ -0,0 +1,34 @@
149+# -*- coding: UTF-8 -*-
150+'''
151+Created on 30 jan. 2013
152+
153+@author: Ronald Portier, Therp
154+
155+rportier@therp.nl
156+http://www.therp.nl
157+'''
158+from openerp.osv import orm
159+from openerp.osv import fields
160+
161+
162+class res_partner(orm.Model):
163+ _inherit = 'res.partner'
164+
165+ _columns = {
166+ 'property_account_expense': fields.property(
167+ 'account.account',
168+ type='many2one',
169+ relation='account.account',
170+ string='Default expense account',
171+ view_load=True,
172+ domain='''[('user_type.report_type', '=', 'expense')]''',
173+ help='Default counterpart account for purchases',
174+ required=False),
175+ 'auto_update_account_expense': fields.boolean(
176+ 'Save account selected on invoice',
177+ help='When account selected on invoice, automatically save it'),
178+ }
179+
180+ _defaults = {
181+ 'auto_update_account_expense': True,
182+ }
183
184=== added directory 'account_invoice_line_default_account/view'
185=== added file 'account_invoice_line_default_account/view/res_partner_view.xml'
186--- account_invoice_line_default_account/view/res_partner_view.xml 1970-01-01 00:00:00 +0000
187+++ account_invoice_line_default_account/view/res_partner_view.xml 2013-02-01 09:54:22 +0000
188@@ -0,0 +1,34 @@
189+<?xml version="1.0" encoding="utf-8"?>
190+<openerp>
191+ <data>
192+ <record
193+ id="view_partner_expense_property_form"
194+ model="ir.ui.view">
195+ <field name="name">res.partner.property.form.inherit</field>
196+ <field name="model">res.partner</field>
197+ <field name="type">form</field>
198+ <field name="priority">2</field>
199+ <field
200+ name="inherit_id"
201+ ref="account.view_partner_property_form" />
202+ <field
203+ name="arch"
204+ type="xml">
205+ <field
206+ name="property_account_payable"
207+ position="after">
208+ <group
209+ colspan="2"
210+ col="4">
211+ <field
212+ name="property_account_expense"
213+ groups="account.group_account_invoice" />
214+ <field
215+ name="auto_update_account_expense"
216+ groups="account.group_account_invoice" />
217+ </group>
218+ </field>
219+ </field>
220+ </record>
221+ </data>
222+</openerp>

Subscribers

People subscribed via source and target branches