openerp-mexico-localization

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

pac-facturalo
Merge into 7.0

Revision history for this message

Agustín (atin81) wrote on 2013-12-14:

Antes que nada muchas gracias por la revisión Nhomar. Trataré de
responder lo mejor que pueda a todas tus dudas.

On 14/12/13 02:47, Nhomar - Vauxoo wrote:
> Review: Needs Fixing
>
> 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?
Nosotros estamos trabajando en conjunto con la empresa de factura-lo.mx
en la realización de este código y para ofrecer la solución a nuestros
clientes. Voy a ponerme en contacto con ellos para ver cual es el nombre
que tienen registrado.
>
> 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>
Mismo comentario anterior, no se exactamente que problema tuvieron en
días pasados con su web.
>
> 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.
No intentaban ser créditos sino más bien lugares donde acudir en caso de
dudas, pero los vamos a retirar para dejar sólo la página web.
>
> 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.
Ok, los moveremos a demo y consideraremos la realización del wizard.
>
> 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.
Intentamos empezar con un módulo nuevo pero no pudimos hacer que
funcionara por el desconocimiento del trabajo interno del módulo. Había
datos que no se creaban y cosas así. Así que decidimos hacer copy/paste
del módulo de pac_sf y de ahí ir refactorizando las funciones como las
íbamos necesitando. Esa es la razón del código comentado, irá
desapareciendo conforme el módulo vaya siendo pulido.
>
> 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.
Ok, ya tomamos nota y trabajaremos en eso.
>
> 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.
También tomamos nota de esto
>
> 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.
Perfecto, quedamos a la espera de las unificaciones y mientras
seguiremos refactorizando sólo aquellas funciones que sean únicas para
este pac en particular de forma que se pueda realizar una unificación
del resto de las funciones en el lugar que se considere más conveniente.
>
> 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".
La verdad aunque hace tiempo que venimos considerando lo de los test
unitarios, todavía no nos hemos dado la oportunidad de empezar a
trabajar con un desarrollo basado en pruebas. Creo que esta va a ser la
oportunidad perfecta para hacerlo, pero si tendrá que esperar a enero :D
>
> 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.
Perfecto, creo yo el bug y les paso el link? O al revés?
>
> 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.
De momento lo único que hemos modificado al código es la función para
realizar el envío de los correos electrónicos con las facturas, porque
no lográbamos hacer que se enviaran de forma consistente. A veces se
enviaban y a veces no. Entonces tomamos como base el módulo de mensajes
y reescribimos algunas partes. Ya lo propondré en otro MR para no revolver.

En cuanto a la propuesta, pues me gustaría esperar a que conociéramos
bien a fondo como está estructurada toda la localización mexicana para
poder opinar. Si creo que algunas funciones quedaría mejor si se
unificaran, pero todavía siento que no tenemos la visión global
necesaria para opinar.

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

Claro que es muy útil, mejor largo y bien explicado, significa que le
dedicaste tiempo a revisar y eso es importante para nosotros. Muchas
gracias.

Seguimos en contacto

Antes que nada muchas gracias por la revisión Nhomar. Trataré de 
responder lo mejor que pueda a todas tus dudas.

On 14/12/13 02:47, Nhomar - Vauxoo wrote:
> Review: Needs Fixing
>
> 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?
Nosotros estamos trabajando en conjunto con la empresa de factura-lo.mx 
en la realización de este código y para ofrecer la solución a nuestros 
clientes. Voy a ponerme en contacto con ellos para ver cual es el nombre 
que tienen registrado.
>
> 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>
Mismo comentario anterior, no se exactamente que problema tuvieron en 
días pasados con su web.
>
> 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.
No intentaban ser créditos sino más bien lugares donde acudir en caso de 
dudas, pero los vamos a retirar para dejar sólo la página web.
>
> 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.
Ok, los moveremos a demo y consideraremos la realización del wizard.
>
> 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.
Intentamos empezar con un módulo nuevo pero no pudimos hacer que 
funcionara por el desconocimiento del trabajo interno del módulo. Había 
datos que no se creaban y cosas así. Así que decidimos hacer copy/paste 
del módulo de pac_sf y de ahí ir refactorizando las funciones como las 
íbamos necesitando. Esa es la razón del código comentado, irá 
desapareciendo conforme el módulo vaya siendo pulido.
>
> 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.
Ok, ya tomamos nota y trabajaremos en eso.
>
> 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.
También tomamos nota de esto
>
> 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.
Perfecto, quedamos a la espera de las unificaciones y mientras 
seguiremos refactorizando sólo aquellas funciones que sean únicas para 
este pac en particular de forma que se pueda realizar una unificación 
del resto de las funciones en el lugar que se considere más conveniente.
>
> 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".
La verdad aunque hace tiempo que venimos considerando lo de los test 
unitarios, todavía no nos hemos dado la oportunidad de empezar a 
trabajar con un desarrollo basado en pruebas. Creo que esta va a ser la 
oportunidad perfecta para hacerlo, pero si tendrá que esperar a enero :D
>
> 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.
Perfecto, creo yo el bug y les paso el link? O al revés?
>
> 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.
De momento lo único que hemos modificado al código es la función para 
realizar el envío de los correos electrónicos con las facturas, porque 
no lográbamos hacer que se enviaran de forma consistente. A veces se 
enviaban y a veces no. Entonces tomamos como base el módulo de mensajes 
y reescribimos algunas partes. Ya lo propondré en otro MR para no revolver.

En cuanto a la propuesta, pues me gustaría esperar a que conociéramos 
bien a fondo como está estructurada toda la localización mexicana para 
poder opinar. Si creo que algunas funciones quedaría mejor si se 
unificaran, pero todavía siento que no tenemos la visión global 
necesaria para opinar.

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

Claro que es muy útil, mejor largo y bien explicado, significa que le 
dedicaste tiempo a revisar y eso es importante para nosotros. Muchas 
gracias.

Seguimos en contacto

« Back to merge proposal