savoirfairelinux-openerp team mailing list archive

["Stefan Rijnhart \(Therp\)" <stefan@xxxxxxxx>]

Review: Needs Information

I must agree that the features of this module, while useful, don't seem to necessarily belong together. Apart from that, the code does raise some questions with me:

- By ISP, do you mean internet service provider?
- What do you mean by 'representative'?
- How is code different from the regular partner reference 'ref'? Why not fill this field automatically instead of creating a new field?
- Can you explain, what onchange_name does (preferably in a docstring) and how the code is assembled (preferably by refactoring the code, it would be nice if this piece of the code became a little bit more readable)

You override the char field 'birthdate' from the base module by a date field. I would suspect that the installation of this module would make you 'lose' any existing birhdates in char format (orm moves the database column to birthdate_moved_nnn), and that an upgrade of the base module would move your date type column, and so on. Or am I missing something?

l.207: you catch just any exception, which you should generally avoid doing. I think the whole try/catch block can be substituted by checking if 'name' is in values, right?

-- 
https://code.launchpad.net/~savoirfairelinux-openerp/partner-contact-management/partner_isp/+merge/185095
Your team Savoir-faire Linux' OpenERP is subscribed to branch lp:~savoirfairelinux-openerp/partner-contact-management/partner_isp.