Code review comment for lp:~savoirfairelinux-openerp/banking-addons/loose-coupling

Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

Nice piece of work!

Some concerns:

l.192ff I still see calls to the ORM methods without propagation of the context
l.199,1668 instead of round, I think that you should use res_currency.is_zero()

235 + for handler_func in registry.SEND_PAYMENT_FUNCS:
I don't know if this can be an issue here but you have to be cautious with this global registry mechanism: you always have to consider that OpenERP may import the python code from uninstalled modules. Meaning that you will end up with entries in your registry that are not part of an installed module.

241 + handler_func(self, cr, uid, cprs, context=context)
If I understand this line, you'll call the send method for each handler registered, so if you have handler functions for 3 banks, you'll do a call to the 3 banks? Couldn't it be a problem?

1643 + company = company_obj.browse(cr, uid, 1, context=context)
1 should not be hard-coded, at least use the xmlid, ideally, get it from the user

review: Needs Fixing (code review, no test)

« Back to merge proposal