Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[11.0][FIX] l10n_es_account_invoice_sequence: Compute correct next number of sequence #1052

Merged
merged 1 commit into from
Apr 26, 2019

Conversation

jffernandez
Copy link
Member

Cuando se crea un nuevo diario propone como número siguiente del contador el 1.

El número por defecto correcto es el número actual del contador que se establece forzosamente (el de los demás diarios de la misma empresa, si se cumplen las condiciones, tal como está en el método create sobreescrito en el módulo)

De este modo se evita sobreescribir el valor del contador si el usuario no tiene el cuidado de consultar el valor actual y cambiar el valor incorrecto propuesto como valor por defecto.

@pedrobaeza pedrobaeza added this to the 11.0 milestone Apr 2, 2019
@pedrobaeza
Copy link
Member

Buenas, @jffernandez gracias por el PR y bienvenido a la comunidad!

Entiendo el problema que describes y desde luego es algo real, pero que como casi todos creamos los diarios al principio, ni somos conscientes de ello, o bien luego al renumerar es como si nada.

Pero el parche que me parece más adecuado para ese problema es:

  • Ocultar el campo de siguiente número cuando la compañía sea española. Para eso se puede poner un campo calculado y un attrs al campo de siguiente número.
  • Inhibir la escritura del number next en ese caso.

¿Por qué lo considero más adecuado? Porque así evitamos cambiar un número a mitad de ejercicio cuando la numeración ya está más que establecida. ¿Tú qué opinas?

Si te resulta difícil, laborioso o tedioso hacer los cambios, puedo hacerlos yo si quieres.

@jffernandez
Copy link
Member Author

Gracias Pedro.

Puedo implementarlo como comentas, el problema que veo, es que con el diseño de esa vista debería ocultar también el campo del contador, porque están los dos en el mismo div, con la etiqueta del campo sequence_number_next, o quedaría la etiqueta de "Próximo número" y se vería el contador... no queda muy elegante. Y creo que el contador no deberíamos ocultarlo, aunque no se puede modificar, creo que es importante que se vea qué contador se está empleando.

Que se pueda modificar el contador de asientos desde ahí... bueno, creo que no es un problema añadido, ya se podía hacer y si lo ocultamos estamos quitando una funcionalidad (aunque no sea necesaria/recomendable).

¿Qué opinas? ¿Dejamos los campos visibles? ¿Oculto el del próximo número y dejo el del contador con la etiqueta incorrecta? (se me ocurre ahora que igual puedo añadir una segunda etiqueta y jugar con la visibilidad, pero no se si dará problemas con el script de renderizado al encontrarse con dos etiquetas)

Gracias por tu tiempo!

@pedrobaeza
Copy link
Member

Buenas, José, no termino de entenderte a lo que te refieres. ¿Me puedes poner alguna captura de pantalla o mockup de lo que propones?

@jffernandez
Copy link
Member Author

Ocultar el campo no es tan sencillo, porque bajo la etiqueta de ese campo está también el campo del contador:
Captura1

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, con lo que me cuentas, volvemos a tu plan original, pero mira los comentarios que te hago y además respeta las 80 cols del PEP8.

@jffernandez jffernandez force-pushed the 11.0-fix-account_invoice_sequence branch from d6bc1b0 to 24724f6 Compare April 16, 2019 13:12
@jffernandez
Copy link
Member Author

Gracias por los comentarios Pedro.
He aprovechado para cambiar la implementación, porque he comprobado que si proponemos como valor por defecto el contador que se establece forzosamente en el create se consigue el mismo resultado de una forma más limpia.

De ésta forma es redundante establecerlo en el create, porque el campo es de solo lectura, o prefieres dejarlo igualmente? porque además se limpia el valor de 'refund_sequence' (creo que ese campo también habría que revisarlo, porque nunca se va a tener una numeración diferente para los asientos de los abonos... pero ya es harina de otro FIX).

@pedrobaeza
Copy link
Member

Perfecto, sí, así es más limpio. ¡Buen cambio!

Sobre lo del refund, efectivamente resulta a veces confuso por la terminología. Lo suyo sería:

  • Tener un campo calculado con depends del company_id que diga si es plan contable español.
  • Añadir attrs="{'invisible': ...}" a ese campo que lo oculte si es plan contable español.
  • Ya con esto podrías también cambiar la implementación de esto para así aprovechar ese mismo campo calculado (que tiene la ventaja de cachearse).

@jffernandez jffernandez force-pushed the 11.0-fix-account_invoice_sequence branch from 24724f6 to 9927dd8 Compare April 18, 2019 12:12
@jffernandez
Copy link
Member Author

jffernandez commented Apr 18, 2019

Del refund estoy de acuerdo con lo que indicas, pero creo que es mejor hacer otro PR para eso.

En éste FIX no se emplearía el campo calculado porque creo que para verificar si la empresa que viene en los valores por defecto tiene el plan contable español habría que mantener el código que he añadido en la función default_get, para obtener el objeto a partir del id que viene en el diccionario de valores por defecto calculados previamente (en la llamada a super).

En el código existente que emplea esa condición, en el método create, creo que tampoco se podría usar el campo calculado porque ahí todavía no tenemos el objeto creado, solamente un diccionario con los valores de los campos.

Corrígeme si me equivoco.

Muchas gracias!

@pedrobaeza
Copy link
Member

@jffernandez llevas razón con lo del default, y el otro sí que podría ser con el compute, pero tampoco merece la pena. Échale un vistazo al rojo de Travis, y cuando esté verde, fusionamos esto y ya vemos si hacemos lo de los refund.

@jffernandez
Copy link
Member Author

Gracias Pedro.

Los errores de Travis es por los certificados del módulo facturae... no sé exactamente qué tengo que hacer al respecto, disculpa porque no me tengo peleado mucho con GitHub ni Travis.

He actualizado los cambios de upstream en mi fork, pero igual lo hice peor, porque ahora aparecen dos commit en el PR.

@etobella
Copy link
Member

@pedrobaeza Hay un error relacionado con Facturae. Lo detecté esta semana, pero aún no pude revisarlo bien, ya que pensaba que era un problema mio de servidor.
Parece relacionado con la cadena de certificados de https://www.facturae.gob.es/politica_de_firma_formato_facturae/politica_de_firma_formato_facturae_v3_1.pdf

En cuanto tenga la incidencia resuelta haré el PR.

@pedrobaeza
Copy link
Member

OK, @etobella, a ver si puedes solucionarlo lo antes posible 👼

@etobella
Copy link
Member

Bueno, parece que la incidencia está relacionada con el Subject Alternative Name del certificado. Se considera que para que un certificado sea válido para una web debe incluir el DNS en el Subject Alternative Name, el certificado de la web que os decia no está bien formado y el Subject Alternative Name está vacio. La mayoría de webs utilizan el CN del certificado si no tiene Subject Alternative Name, pero como según el RFC no debería ser válido, el openssl no lo considera correcto y falla.

Llegados a este punto, creo que solo podemos quitar la verificación en esta url, ya que será imposible que el Gobierno de España modifique el certificado (Que se puede probar, pero lo veo difícil).

¿Os parece bien?

@pedrobaeza
Copy link
Member

Haz por favor rebase para tener Travis verde

@jffernandez jffernandez force-pushed the 11.0-fix-account_invoice_sequence branch from bf12790 to 17b4d2c Compare April 24, 2019 14:14
@jffernandez
Copy link
Member Author

@pedrobaeza Listo, todo verde.

@pedrobaeza pedrobaeza merged commit ce4cb40 into OCA:11.0 Apr 26, 2019
@pedrobaeza
Copy link
Member

Cherry-pick para v12 en 855dfff

@jffernandez jffernandez deleted the 11.0-fix-account_invoice_sequence branch May 24, 2019 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants