← Back to team overview

savoirfairelinux-openerp team mailing list archive

Re: lp:~savoirfairelinux-openerp/banking-addons/loose-coupling into lp:banking-addons/bank-statement-reconcile-7.0

 

Review: Needs Fixing code review, no test

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
-- 
https://code.launchpad.net/~savoirfairelinux-openerp/banking-addons/loose-coupling/+merge/185033
Your team Savoir-faire Linux' OpenERP is subscribed to branch lp:~savoirfairelinux-openerp/banking-addons/loose-coupling.


References