Code review comment for lp:~akretion-team/openobject-addons/hr-expense-small-improvements

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