Code review comment for lp:~atin81/openerp-mexico-localization/pac-facturalo

Hola Agustin.

Tengo varias dudas, pero ante todo muchas gracias por el aporte.

Dudas Operativas:

1.- No veo a factura-lo aquí:
 http://www.sat.gob.mx/sitio_internet/asistencia_contribuyente/principiantes/comprobantes_fiscales/66_19264.html

Debido a la apertura del software y su fácil auditoría debemon colocar los nombres "Registrados" en la página del SAT, ¿crees que nos ayudas puedas apoyar con colocar eso por favor?

2.- Al momento de revisar el PAC con los dominios suministrados en los parámetros de conexión tenía un virus la página, ¿Me comentas cómo es eso posible? creo que como usuarios Linux pensar en Virus es nocivo para nuestra Salud </joke>

3.- Por favor los créditos no deben ir en el description del __openerp__.py, para eso es el campo website en el descriptor, lo puedes apuntar a tu página y los correos ya están en los comentarios del código junto con la licencia.

Dudas Técnicas:

3.- l10n_mx_facturae_pac_facturalo/data/l10n_mx_facturae_pac_facturalo.xml debería estar en "demo" no en "data" en todo caso debería configurarse con un Wizard al momento de colocar la data, no cableada como se encuentra en éste momento. ¿Por qué? sucede que los valores necesarios para la operación en una base de datos para producción por buenas prácticas no deberían pre-configurarse de forma errónea. Te puedes basar en los wizards ir.config para mayor información, o verifica los wizards de configuración del módulo de pacsf.

4.- Lo mismo para l10n_mx_facturae_pac_facturalo/data/l10n_mx_facturae_seq.xml son datos contables que afectarían en 1 click una base de datos de producción.

5.- Hay código comentado, en python eso no se hace ;-) (no es PHP) puedes removerlo y comentar en el commit el TODO, después puedes volverlo a colocar solo leyendo un bzr diff.

6.- Yo sé que nuestros módulos tienen comentarios en español, pero es debido a que son nuestra evolución y no han sido limpiados, me apoyas colocandolos en Inglés, de ésta manera nos ahorramos muchísimos errores con unicode y podemos usar tu caso de código como ejemplo cuando discutimos propuestas al core oficial.

7.- Sucede lo mismo con las variables (pero éstas déjalas así) las variables en español son inconsistentes es decir, leer browse.algoenespanol.del.ano suena feo verdad? siempre es buena práctica colocar las variables en inglés. NOTA. Si tienes esto en producción éste cambio específico lo puedes hacer en un branch diferente al que uses hasta que quede mergeado.

8.- Hay muchísimo código copiado y pegado, creo que tenemos que hacer una limpieza antes de éste merge para poder unificar los criterios y darle respuesta a tus preguntas.

9.- El módulo no tiene un solo test unitario, te comento que estabilizar los otros nos ha costado un monton que marque verde en los servidores de integración contínua, veo sumamante riesgoso colocar un módulo sin tests "Tienes en planes trabajar en ésto".

10.- Sobre el reporte nosotros hemos hecho varias iteraciones, podemos discutirlas en un bug sobre ese tema específico (sin trancar esta MP por ese concepto esppecífico) ya que tenemos que diseñarlo.

Sobre tus dudas.

11.- Si nos propones alguna clase de diseño podemos hacer un sprint, nostros estamos planeando reescribir algunas cosas importantes para efectos de la firma de nómina y código comentado y algunas cosas que te comento acá, si tienes alguna crítica al respecto, es bienvenida como merge o como comentario en algún bug, con todo gusto la checamos y la mergeamos caso a caso.

Ui... creo que me quedo largo ésto, espero te sea de utilidad en tu proceso de terminarlo.

Saludos.