openerp-expert-framework team mailing list archive

[Parthiv Patel <parthivgls@xxxxxxxxx>]

Hello Experts,

Recently me and my team mates were testing HR and related module of OpenERP,
and we found few annoying things in the code, so I would like to share my
views for the same. At many places on_change even is written in various
ways. There was NO unified way or structure to write such method. Most of
them were written as ease of dev. and causing the bugs.

For example : in Trunk Addons Rev. 4172 (if any one from committer/core team
reading. please fix it, given u a patch on
https://bugs.launchpad.net/openobject-addons/+bug/653690 )

*hr_payroll / hr_payroll.py *

*        def on_change_employee_id(self, cr, uid, ids, employee_id):
        v = {}
        passport = self.pool.get('hr.employee').browse(cr, uid,
employee_id).passport_id
        if passport:
            v['passport_id'] = passport
        return {'value': v}

*
As it can be seen from code, what will be if some one removes "employee_id",
it will raise the error. why this kind of code ? what is the purpose of
using return variable name "v".
no standard format of on_change is been followed. In same file another
on_change event.* *

*hr_payroll / hr_payroll.py *

 * def onchange_company_id(self, cr, uid, ids, company_id=False,
context=None):*
*        res = {}*
*        if context is None:*
*            context = {}*
*        if company_id:*
*            company = self.pool.get('res.company').browse(cr, uid,
company_id, context=context)*
*            if company.partner_id.bank_ids:*
*                res.update({'bank': company.partner_id.bank_ids[0].
bank.name})*
*        return {*
*            'value':res*
*        }*

I would suggest we define a structure for this kind of functions which can
be easily followed to avoid unnecessary bugs like above.
I have tried to define something, but need your precious views.

onchange_field (cr, uid, ids, field, context=None):
 # initialize the return dictionary.

res = { }

 # initialize the return with null

res['value'] = { 'field_1' : False }

# check for the value of "field"

if not field:
return res
else:
# declare the required POOL, OBJECT, etc...
return res



Please feel free to comment, suggest your views on it.


-- 
Thanks & Regards,
Parthiv Patel,
CEO,
Tech Receptives,