Code review comment for lp:~yajo/openerp-spain/add-cnae-module

Revision history for this message
Pedro Manuel Baeza (pedro.baeza) wrote :

Buenas, Jairot,

Gracias por la contribución. Te menciono varias cosas para mejorar el módulo:

- En la definición de los campos parent_left y parent_right, mejor ponles el parámetro select=1 para que se cree un índice, y se aproveche mejor toda la ventaja del _parent_store.

- Para las nuevas prácticas de v7/v8, define "from osv import fields, osv" como "from openerp.osv import fields, orm", y los modelos como orm.Model en lugar de osv.Model.

- Para cumplir con el estándar PEP8, tienes que corregir estas cosas (obtenidas de la utilidad flake8):

l10n_es_partner_cnae.py:21:1: E302 expected 2 blank lines, found 1
l10n_es_partner_cnae.py:62:1: E302 expected 2 blank lines, found 1
__openerp__.py:25:1: E122 continuation line missing indentation or outdented
__openerp__.py:39:80: E501 line too long (84 > 79 characters)
__openerp__.py:59:14: E203 whitespace before ':'
__openerp__.py:60:11: E203 whitespace before ':'

- Las nuevas convenciones que se están empezando a aplicar indican de colocar los modelos en la carpeta models y las vistas en la carpeta views, por si quieres seguirlas. Además, también se está empezando utilizar la convención CapsWords del PEP8 en los nombres de clases.

- El icono del módulo no puede ser el logo de la empresa desarrolladora. Puedes utilizar esta imagen (redimensionada al tamaño adecuado) combinada con la bandera española: http://www.clasificaciondeempresas.com.es/wp-content/uploads/2013/06/cnae-codigo-lista-719166.jpg.

- ¿Por qué se permite poner código CNAE a los contactos?

- Es mejor meter el menú de los códigos CNAE dentro de 'Libreta de direcciones'.

- Con la definición de los grupos de seguridad poniendo la regla "access_cnae_codes_manager" con el grupo "base.group_no_one", al final, todo el mundo puede editar los códigos.

- ¿Merece la pena tener dos vistas para lo mismo? Creo que con la vista de árbol podría ser suficiente, aunque se pierda la editabilidad directa.

Un saludo.

review: Needs Fixing (code review and test)

« Back to merge proposal