openerp-expert-accounting team mailing list archive

["Olivier Dony \(OpenERP\)" <odo@xxxxxxxxxxx>]

Review: Needs Fixing

Hello Lionel,

This looks like a very important addition to the l10n_fr modules, so I'm in favor of merging it :-)

Have you perhaps got the chance to ask other French OpenERP integrators to review it? Shouldn't we ask them?

A few remarks, nevertheless (line numbers are those of the current diff @r.5755):

- l.61: It would be great if the module manifest contained a more complete description, explaining where users can configure its features, and how to use them
- *: the source files should presumably bear your own copyright, rather than being copyright "OpenERP SA" ;-)
- l.139: assuming that every domain item in the list will be iterable is not correct, what about OR/AND/NOT operators? And actually if you only wanted to have name_search match on the `rib` column in addition to the _rec_name, you could simply override name_search or _name_search, without impacting the normal search().
- l.171: why do you check the constraint again when computing the error message? This will duplicate the check every time...
- l.202: this call to search() is actually a normal name_search() call except you're hardcoding the 'name' field. It'd be slightly better to call super.name_search() (which calls name_get for you, so you need a bit of refactoring)


Thanks!


PS: marking this MP as ``Work In Progress``, when you're ready don't forget to make it ``Needs Review`` again
-- 
https://code.launchpad.net/~numerigraphe/openobject-addons/trunk-rib/+merge/82886
Your team OpenERP Accounting Experts is requested to review the proposed merge of lp:~numerigraphe/openobject-addons/trunk-rib into lp:openobject-addons.