Merge lp:~akretion-team/openobject-addons/hr-expense-small-improvements into lp:openobject-addons

[Alexis de Lattre]

This merge proposal contains a number of small usability and technical improvements in hr_expense. These improvements are all generic and have been implemented following my experience the use of hr_expense on a day-to-day basis at Anevia :

1. Usability improvement : Add 'total_amount' field in the form view of the expense lines, so that the employee has a better control when entering his expense note.

2. Usability improvement : Some fields of hr_expense_expense should always be read-only (date_valid, date_confirm, user_valid, ...) Other fields should be readonly when state != draft (currency_id, employee_id, ...). Some fields should be readonly only when state = invoice or paid (journal_id)

3. Technical improvement : function fields are stored, with the appropriate invalid functions, to get better performances.

I hope that this merge proposal will be accepted in trunk. Don't hesitate to ask me any question about this proposal.

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

Hi Alexis,

Thanks for submitting your patch, and sorry for the review delay. I'm a bit surprised no-one from the community had a look at it yet though (we'll try to do something about that, will post about it later)

The changes concerning the readonly state of the fields look good, but I have a few questions about the rest:

- Is there any particular performance problem that triggered the need to store the function fields? In general it is better to avoid storing function fields that require complex store triggers, especially if they're not critical for searching and do not represent an identified performance issue. The main reason for this being to avoid bugs and keep the code smaller and simpler (that store "plumbing" is dull and requires extra boilerplate functions in case of complex triggers).

- Why did you drop all the "select=True" attributes of the fields? This attribute is still used to tell the ORM to create an index on the column, so they're actually very useful most of the time. Any specific reason?

+ Note: you might want to have a look at openerp.tools.float_utils.float_round() in order to properly round the total_amount that is returned by your on_change. See how it's used [1][2].

Wrapping up, it seems to me that if you keep the "select=True" attributes and remove all the storing plumbing, your patch will stay nice, minimalist and less risky, not even altering the database schema (which is best if you'd like to see it included in 7.0)

Thanks!

PS: you'll probably get a few conflicts if you update your branch with trunk, due to the way most views where changed for v7, but if it's annoying the few changes you have should be easy to reapply on top of the new code, discarding the conflicts.

[1] example use in account: http://bazaar.launchpad.net/~openerp/openobject-addons/trunk/view/head:/account/account.py#L2092
[2] reference: http://bazaar.launchpad.net/~openerp/openobject-server/trunk/view/head:/openerp/tools/float_utils.py#L32

review: Needs Information

[lp:~akretion-team/openobject-addons/hr-expense-small-improvements]

6605. By Alexis de Lattre on 2012-12-20

Merge with up-to-date addons-trunk revno 8422 and update to code accordingly.
Take into account the remarks of Olivier Dony :
- select=True are back (I though it was useless)
- remove the store for the "amount" field of expense (keep it only for expense lines, because the store is very simple for expense lines)
Remove field account_move_id, which is not used any more.

[Alexis de Lattre (alexis-via)]

Dear Olivier,

Thank you for your review ; I must say that the merge proposal was submitted so long ago (when we were preparing OpenERP 6.1 !) that I didn't expect a review/merge of it any more.

Hopefully, I found some motivation to updated my branch hr-expense-small-improvements this morning and I took your remarks into account :

1) About the storing of function fields, I must say that my motivation is not only about performance or search functionnality, it is also about the fact that I want to do some "Business Intelligence" on the expense note (with Pentaho), and for that I want to have the fields in the database. But, in fact, only the amount_total field of the expense line is usefull for my Business Intelligence analysis, and, for this field, the store trigger is very simple. So the "amount" field of expense.expense is NOT stored any more ; only the "amount_total" field of expense.line is stored.

2) I was not aware that select=True was usefull ; I though it was an old thing of the past ! So I have put it back.

I have made another change : I removed the field "account_move_id", which is not used at all any more : it is not set/read in the code and not displayed in the views (If you want to keep it, you can do it in your merge).

Some remarks :

- The fields "date_valid" and "date_confirm" are still set in the code, but they are not displayed in the views. Why ?

- The on_change on editable tree views doesn't set the value of function fields (this works fine in the form views). You can see that on the "amount_total" field on expense lines. I think it's a bug so I will report it.

[lp:~akretion-team/openobject-addons/hr-expense-small-improvements]

6606. By Alexis de Lattre on 2012-12-20

Clean-up my previous commit.

[Alexis de Lattre (alexis-via)]

I forgot to mention that, in my commit 6605, I updated the order of the fields in the tree view of expense line to make it more logicial (same order as in account invoice line), i.e. :

Qty - UoM - Unit price - Subtotal

Unmerged revisions

6606. By Alexis de Lattre on 2012-12-20

Clean-up my previous commit.

6605. By Alexis de Lattre on 2012-12-20

Merge with up-to-date addons-trunk revno 8422 and update to code accordingly.
Take into account the remarks of Olivier Dony :
- select=True are back (I though it was useless)
- remove the store for the "amount" field of expense (keep it only for expense lines, because the store is very simple for expense lines)
Remove field account_move_id, which is not used any more.

6604. By Alexis de Lattre on 2012-02-16

[Usability] Add 'total_amount' field in the form view of the expense lines, so that the employee has a better control when entering his expense note.
Add the related on_change code.

6603. By Alexis de Lattre on 2012-02-16

total_amount on hr.expense.line is now a stored field.

6602. By Alexis de Lattre on 2012-02-16

Journal field should be writable until the expense is invoiced.

6601. By Alexis de Lattre on 2012-02-15

'amount' field of hr.expense.expense is now store=True with invalidate function, for better performances.

6600. By Alexis de Lattre on 2012-02-15

Some fields of hr_expense_expense should always be read-only (date_valid, date_confirm, user_valid, ...)
Some fields should be readonly when state != draft (currency_id, employee_id, ...)
Remove useless Select=True
Coding style fixes

=== modified file 'hr_expense/hr_expense.py'
--- hr_expense/hr_expense.py	2012-12-20 11:47:30 +0000
+++ hr_expense/hr_expense.py	2012-12-20 14:52:44 +0000
@@ -60,6 +60,11 @@
         else:
             return self.pool.get('res.currency').search(cr, uid, [('rate','=',1.0)], context=context)[0]
 
+    def _get_expense_from_line(self, cr, uid, ids, context=None):
+        """Invalidate function of the 'amount' field based on the hr.expense.line obj"""
+        return self.pool.get('hr.expense.expense').search(cr, uid,
+            [('line_ids', 'in', ids)], context=context)
+
     _name = "hr.expense.expense"
     _inherit = ['mail.thread']
     _description = "Expense"
@@ -76,20 +81,21 @@
         'name': fields.char('Description', size=128, required=True, readonly=True, states={'draft':[('readonly',False)], 'confirm':[('readonly',False)]}),
         'id': fields.integer('Sheet ID', readonly=True),
         'date': fields.date('Date', select=True, readonly=True, states={'draft':[('readonly',False)], 'confirm':[('readonly',False)]}),
-        'journal_id': fields.many2one('account.journal', 'Force Journal', help = "The journal used when the expense is done."),
+        'journal_id': fields.many2one('account.journal', 'Force Journal',
+            states={'done':[('readonly', True)]},
+            help = "The journal used when the expense is done."),
         'employee_id': fields.many2one('hr.employee', "Employee", required=True, readonly=True, states={'draft':[('readonly',False)], 'confirm':[('readonly',False)]}),
         'user_id': fields.many2one('res.users', 'User', required=True),
-        'date_confirm': fields.date('Confirmation Date', select=True, help="Date of the confirmation of the sheet expense. It's filled when the button Confirm is pressed."),
-        'date_valid': fields.date('Validation Date', select=True, help="Date of the acceptation of the sheet expense. It's filled when the button Accept is pressed."),
+        'date_confirm': fields.date('Confirmation Date', select=True, readonly=True, help="Date of the confirmation of the sheet expense. It's filled when the button Confirm is pressed."),
+        'date_valid': fields.date('Validation Date', select=True, readonly=True, help="Date of the acceptation of the sheet expense. It's filled when the button Accept is pressed."),
         'user_valid': fields.many2one('res.users', 'Validation By', readonly=True, states={'draft':[('readonly',False)], 'confirm':[('readonly',False)]}),
-        'account_move_id': fields.many2one('account.move', 'Ledger Posting'),
         'line_ids': fields.one2many('hr.expense.line', 'expense_id', 'Expense Lines', readonly=True, states={'draft':[('readonly',False)]} ),
         'note': fields.text('Note'),
         'amount': fields.function(_amount, string='Total Amount', digits_compute=dp.get_precision('Account')),
-        'voucher_id': fields.many2one('account.voucher', "Employee's Receipt"),
+        'voucher_id': fields.many2one('account.voucher', "Employee's Receipt", readonly=True),
         'currency_id': fields.many2one('res.currency', 'Currency', required=True, readonly=True, states={'draft':[('readonly',False)], 'confirm':[('readonly',False)]}),
-        'department_id':fields.many2one('hr.department','Department', readonly=True, states={'draft':[('readonly',False)], 'confirm':[('readonly',False)]}),
-        'company_id': fields.many2one('res.company', 'Company', required=True),
+        'department_id':fields.many2one('hr.department', 'Department', readonly=True, states={'draft':[('readonly',False)], 'confirm':[('readonly',False)]}),
+        'company_id': fields.many2one('res.company', 'Company', required=True, readonly=True, states={'draft':[('readonly',False)], 'confirm':[('readonly',False)]}),
         'state': fields.selection([
             ('draft', 'New'),
             ('cancelled', 'Refused'),
@@ -97,9 +103,14 @@
             ('accepted', 'Approved'),
             ('done', 'Done'),
             ],
+<<<<<<< TREE
             'Status', readonly=True, track_visibility='onchange',
             help='When the expense request is created the status is \'Draft\'.\n It is confirmed by the user and request is sent to admin, the status is \'Waiting Confirmation\'.\
             \nIf the admin accepts it, the status is \'Accepted\'.\n If a receipt is made for the expense request, the status is \'Done\'.'),
+=======
+            'Status', readonly=True, help="""When the expense request is created the status is 'Draft'.\n It is confirmed by the user and request is sent to admin, the status is 'Waiting Confirmation'.\
+            \nIf the admin accepts it, the status is 'Accepted'.\n If a receipt is made for the expense request, the status is 'Done'."""),
+>>>>>>> MERGE-SOURCE
     }
     _defaults = {
         'company_id': lambda s, cr, uid, c: s.pool.get('res.company')._company_default_get(cr, uid, 'hr.employee', context=c),
@@ -263,13 +274,15 @@
         'name': fields.char('Expense Note', size=128, required=True),
         'date_value': fields.date('Date', required=True),
         'expense_id': fields.many2one('hr.expense.expense', 'Expense', ondelete='cascade', select=True),
-        'total_amount': fields.function(_amount, string='Total', digits_compute=dp.get_precision('Account')),
+        'total_amount': fields.function(_amount, string='Total', digits_compute=dp.get_precision('Account'), store={
+            'hr.expense.line': (lambda self, cr, uid, ids, c={}: ids, ['unit_amount', 'unit_quantity'], 10)
+            }),
         'unit_amount': fields.float('Unit Price', digits_compute=dp.get_precision('Product Price')),
         'unit_quantity': fields.float('Quantities', digits_compute= dp.get_precision('Product Unit of Measure')),
         'product_id': fields.many2one('product.product', 'Product', domain=[('hr_expense_ok','=',True)]),
         'uom_id': fields.many2one('product.uom', 'Unit of Measure', required=True),
         'description': fields.text('Description'),
-        'analytic_account': fields.many2one('account.analytic.account','Analytic account'),
+        'analytic_account': fields.many2one('account.analytic.account', 'Analytic account'),
         'ref': fields.char('Reference', size=32),
         'sequence': fields.integer('Sequence', select=True, help="Gives the sequence order when displaying a list of expense lines."),
         }
@@ -280,7 +293,7 @@
     }
     _order = "sequence, date_value desc"
 
-    def onchange_product_id(self, cr, uid, ids, product_id, context=None):
+    def onchange_product_id(self, cr, uid, ids, product_id, unit_quantity, context=None):
         res = {}
         if product_id:
             product = self.pool.get('product.product').browse(cr, uid, product_id, context=context)
@@ -288,8 +301,12 @@
             amount_unit = product.price_get('standard_price')[product.id]
             res['unit_amount'] = amount_unit
             res['uom_id'] = product.uom_id.id
+            res['total_amount'] = amount_unit * unit_quantity
         return {'value': res}
 
+    def onchange_amount_qty(self, cr, uid, ids, unit_amount, unit_quantity, context=None):
+        return {'value': {'total_amount': unit_amount * unit_quantity}}
+
     def onchange_uom(self, cr, uid, ids, product_id, uom_id, context=None):
         res = {'value':{}}
         if not uom_id or not product_id:

=== modified file 'hr_expense/hr_expense_view.xml'
--- hr_expense/hr_expense_view.xml	2012-12-06 11:12:08 +0000
+++ hr_expense/hr_expense_view.xml	2012-12-20 14:52:44 +0000
@@ -90,32 +90,33 @@
                                 <form string="Expense Lines" version="7.0">
                                     <group>
                                         <group>
-                                            <field name="product_id" on_change="onchange_product_id(product_id, context)" context="{'default_hr_expense_ok':1}"/>
+                                            <field name="product_id" on_change="onchange_product_id(product_id, unit_quantity, context)" context="{'default_hr_expense_ok':1}"/>
                                             <field name="name"/>
                                             <field name="ref"/>
                                             <field domain="[('type','=','normal')]" name="analytic_account" groups="analytic.group_analytic_accounting"/>
                                         </group>
                                         <group>
-                                            <field name="unit_amount"/>
+                                            <field name="unit_amount" on_change="onchange_amount_qty(unit_amount, unit_quantity, context)"/>
                                             <label for="unit_quantity"/>
                                             <div>
-                                                <field name="unit_quantity" class="oe_inline"/> 
+                                                <field name="unit_quantity" class="oe_inline" on_change="onchange_amount_qty(unit_amount, unit_quantity, context)"/>
                                                 <field name="uom_id" on_change="onchange_uom(product_id, uom_id, context)" class="oe_inline"/>
                                             </div>
+                                            <field name="total_amount"/>
                                             <field name="date_value" />
                                         </group>
                                     </group>
                                 </form>
                                 <tree string="Expense Lines" editable="bottom">
                                     <field name="sequence" invisible="1"/>
-                                    <field name="product_id" on_change="onchange_product_id(product_id, context)" context="{'default_hr_expense_ok':1}"/>
+                                    <field name="product_id" on_change="onchange_product_id(product_id, unit_quantity, context)" context="{'default_hr_expense_ok':1}"/>
                                     <field name="date_value" string="Expense Date"/>
                                     <field name="name"/>
                                     <field name="ref"/>
                                     <field domain="[('type','in',['normal','contract']), ('parent_id','!=',False)]" name="analytic_account" groups="analytic.group_analytic_accounting"/>
+                                    <field name="unit_quantity" on_change="onchange_amount_qty(unit_amount, unit_quantity, context)"/>
                                     <field name="uom_id" on_change="onchange_uom(product_id, uom_id, context)"/>
-                                    <field name="unit_amount"/>
-                                    <field name="unit_quantity"/>
+                                    <field name="unit_amount" on_change="onchange_amount_qty(unit_amount, unit_quantity, context)"/>
                                     <field name="total_amount" sum="Total"/>
                                 </tree>
                             </field>