openerp-community team mailing list archive

["Guewen Baconnier @ Camptocamp" <guewen.baconnier@xxxxxxxxxxxxxx>]

Review: Needs Fixing

Quick eyeball review, I did not reviewed the business aspect.

dubious indentation at l. 647

837	+ 'audit_ids': fields.many2many('mgmtsystem.audit','mgmtsystem_audit_nonconformity_rel','mgmtsystem_audit_id','mgmtsystem_action_id','Related Audits'),
Just a remark, the relation name and column names are no longer necessary, so you can just write:
'audit_ids': fields.many2many('mgmtsystem.audit', 'Related Audits'),
No need to change that though.

1646+ 'description': fields.text('Description', translation=True),
2801+ 'description': fields.text('Description', translation=True),
typo: s/translation/translate/

The context propagation is often missing.

1795+ vals = {'evaluation_date': time.strftime('%Y-%m-%d %H:%M'), 'evaluation_date_user_id': uid }
It would be better if the date time formatting use the constant openerp.tools.DEFAULT_SERVER_DATETIME_FORMAT

1655	+_STATES = [
1656	+ ('d', _('Draft')),
1657	+ ('a', _('Analysis')),
1658	+ ('p', _('Pending Approval')),
1659	+ ('o', _('In Progress')),
1660	+ ('c', _('Closed')),
1661	+ ('x', _('Cancelled')),
1662	+ ]
The state codes are meaningless.
How do a developer knows what will be filtered in this domain? [('state','=','d')]
This lack of meaning seems to be common on the fields.selection of different classes across the merge proposal.

On the usage of namespaces, classes and instanciation of models:
2591	+from osv import fields, osv
2592	+
2593	+class mgmtsystem_nonconformity(osv.osv):
2594	+ _inherit = "mgmtsystem.nonconformity"
2595	+ _columns = {
2596	+ 'analytic_account_id': fields.many2one('account.analytic.account', 'Contract'),
2597	+ }
2598	+mgmtsystem_nonconformity()
You better have to use the complete namespace (by the way the import of osv is now useless if we just need Model) :
  from openerp.osv import fields, orm
and inherit of the new classes:
  class mgmtsystem_nonconformity(orm.Model):
The instanciation  (l.2598) after the class declaration is no longer required.


Unrelated to the code, the review type "Resubmit" is to be used by a reviewer to ask you to resubmit, not the other way. You can see in the reviewer list on top of the page that you are now part of the reviewers for your proposal, and you ask yourself a resubmit ;-)
-- 
https://code.launchpad.net/~openerp-community/openerp-mgmtsystem/nc-extend/+merge/139534
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openerp-mgmtsystem/nc-extend.