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

[17.0][MIG] l10n_es_pos: Migration to 17.0 #3548

Open
wants to merge 67 commits into
base: 17.0
Choose a base branch
from

Conversation

peluko00
Copy link
Contributor

@peluko00 peluko00 commented Apr 23, 2024

Module migrated to version 17.0

cc https://github.com/APSL 154343
@miquelalzanillas @lbarry-apsl @javierobcn @mpascuall please review

@peluko00 peluko00 mentioned this pull request Apr 23, 2024
47 tasks
@pedrobaeza
Copy link
Member

/ocabot migration l10n_es_pos

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone Apr 23, 2024
Copy link
Member

@lbarry-apsl lbarry-apsl left a comment

Choose a reason for hiding this comment

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

Validated functionality in Runboat

@peluko00
Copy link
Contributor Author

peluko00 commented May 9, 2024

This module is available on Odoo core

@peluko00 peluko00 closed this May 9, 2024
@pedrobaeza
Copy link
Member

Es correcto que ahora hay un l10n_es_pos en Odoo core, pero mi compañero @chienandalu tiene que confirmar que cubre correctamente todas las casuísticas. Os decimos luego.

@peluko00
Copy link
Contributor Author

peluko00 commented May 9, 2024

Es correcto que ahora hay un l10n_es_pos en Odoo core, pero mi compañero @chienandalu tiene que confirmar que cubre correctamente todas las casuísticas. Os decimos luego.

Perfecto Pedro, vamos comentando!

@peluko00 peluko00 reopened this May 9, 2024
@NestorAgut
Copy link

NestorAgut commented May 16, 2024

Es correcto que ahora hay un l10n_es_pos en Odoo core, pero mi compañero @chienandalu tiene que confirmar que cubre correctamente todas las casuísticas. Os decimos luego.

Perfecto Pedro, vamos comentando!

Nosotros hemos testeado el módulo y no nos genera la secuencia en los tickets.

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Varios comentarios:

Lo primero que hay que hacer es analizar el gap entre lo que propone Odoo, ya que usó de base este mismo desarrollo para luego divergir en algunas cosas de forma bastante significativa.

Después de eso, hay dos formas de enfocar esta situación:

  • Extendiendo sobre lo que propone Odoo para cubrir necesidades.
  • Proponiendo una alternativa completamente distinta.

Y en cualquier caso, el nombre del módulo debe cambiar, ya que no pueden solaparse. Ya sea l10n_es_pos_oca o l10n_es_pos_extension.

Yo particularmente exploraría la posibilidad de acoplarse al estándar porque siempre es deseable reducir en lo posible la base de código a mantener por OCA. Esto, claro está, es más que probable que requiriese además scripts de migración.

Y si el gap resulta ser grande (por ejemplo, el requisito del offline es considerable) exploraría las vías de la extensión.

@pedrobaeza
Copy link
Member

David, según parece, en el estándar se crea una factura por cada ticket, lo que lo hace totalmente inviable por la cantidad de apuntes contables que generaría, volviendo a la situación pre-14.

@chienandalu
Copy link
Member

Sí, ese fue mi principal punto de crítica en su día (odoo/odoo#126268 (comment)).

@pedrobaeza
Copy link
Member

Ese punto lo hace deshecharlo totalmente en mi opinión.

@chienandalu
Copy link
Member

Ya, a mí tampoco me entusiasma 😅 pero entiendo que el enfoque puede ser válido si el número de transacciones no es desorbitado.

He vuelto a darle un repaso al código por encima y no se acopla muy bien con la alternativa OCA. De modo que la opción l10n_es_pos_oca sería la que tendría más peso.

@miquelalzanillas
Copy link
Contributor

miquelalzanillas commented Sep 23, 2024

Hola @pedrobaeza ,

para ir avanzado en este módulo, proponemos los siguientes cambios:

  • Renombrar el módulo (l10n_es_pos_oca) que a su vez dependerá del módulo del core (l10n_es_pos)
  • Utilizar al máximo la funcionalidad nativa.
  • Saltar la creación de un asiento contable para cada factura simplificada y contabilitzar los tickets en el cierre de la sesión tal y como se hace en v16.

Si no nos dejamos nada, entendemos que con estos cambios recuperamos lo que teníamos en versiones anteriores.

Los ves bien?

@chienandalu
Copy link
Member

Hola, @miquelalzanillas . Si crees que merece la pena acoplarlo al módulo nativo y no es muy costoso, adelante. Si no, tampoco pasa nada si siguen independientes, lo cuál probablemente sea menos trabajo

@pedrobaeza
Copy link
Member

Mejor no depender del módulo original. Para qué tener dos instalados si uno anula el otro.

@miquelalzanillas
Copy link
Contributor

Ok, entonces seria suficiente con renombrar el módulo.

Nos ponemos a ello en este mismo PR.

@pedrobaeza
Copy link
Member

Para renombrar, utilizad estos comandos para mantener todo el historial en futuras versiones:

git filter-branch --tree-filter 'if [ -d l10n_es_pos ]; then mv l10n_es_pos l10n_es_pos_oca; fi' HEAD
git rebase origin/17.0

License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl).
*/

/** @odoo-module */
Copy link
Member

Choose a reason for hiding this comment

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

Lo mismo con este

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sabes porque es debido este error:
9:1 error Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

Copy link
Member

Choose a reason for hiding this comment

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

Sí, los modulos js deben estar nombrados con la extensión .esm.js para que el linter los repase adecuadamente

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hecho, gracias!

@peluko00 peluko00 force-pushed the 17.0-mig-l10n_es_pos branch 5 times, most recently from 5250ec2 to 4b70f1d Compare September 26, 2024 06:49
@miquelalzanillas
Copy link
Contributor

Hola @peluko00 ,

He probado el módulo con los últimos cambios, pero sucede algo extraño con las secuencias.

Primero he abierto una sesión del POS y he creado un ticket de venta y otro de reembolso. Ha utilizado una secuencia especifica para el ticket de reeembolso. Yo quería usar la misma secuencia para ventas y reembolsos, así que he desactivado la opción "Usar secuencia específica para notas de crédito" en el diario de facturas simplicadas.

Después, he cerrado la sesión anterior, y he abierto una segunda sesión de POS. He vuelto a crear un ticket de venta y otro de reembolso, pero la secuencia simplificada de ambos tickets ahora es incorrecta, ha utilizado la secuencia de reembolso para ambos.

image

@peluko00 peluko00 force-pushed the 17.0-mig-l10n_es_pos branch 7 times, most recently from c16f7c7 to e756a54 Compare September 26, 2024 11:13
@peluko00
Copy link
Contributor Author

peluko00 commented Sep 26, 2024

Hola @peluko00 ,

He probado el módulo con los últimos cambios, pero sucede algo extraño con las secuencias.

Primero he abierto una sesión del POS y he creado un ticket de venta y otro de reembolso. Ha utilizado una secuencia especifica para el ticket de reeembolso. Yo quería usar la misma secuencia para ventas y reembolsos, así que he desactivado la opción "Usar secuencia específica para notas de crédito" en el diario de facturas simplicadas.

Después, he cerrado la sesión anterior, y he abierto una segunda sesión de POS. He vuelto a crear un ticket de venta y otro de reembolso, pero la secuencia simplificada de ambos tickets ahora es incorrecta, ha utilizado la secuencia de reembolso para ambos.

image

Gracias por la anotación @miquelalzanillas, lo he resuelto. Lo podras revisar otra vez cuando puedas, gracias!

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

@peluko00 Convendría que probases mínimamente los cambios antes de pedir revisiones. Ahora mismo ni siquiera se cargan los assets correctamente...

"depends": ["point_of_sale"],
"data": ["views/pos_views.xml", "views/res_config_settings_views.xml"],
"assets": {
"point_of_sale.assets": [
Copy link
Member

Choose a reason for hiding this comment

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

Esto simplemente no puede funcionar, porque estos assets ya no existen: https://github.com/odoo/odoo/pull/120070/files

"l10n_es_pos_oca/static/src/app/screens/ticket_screen/ticket_screen.xml",
"l10n_es_pos_oca/static/src/app/screens/payment_screen/payment_screen.esm.js",
"l10n_es_pos_oca/static/src/app/store/models.esm.js",
"l10n_es_pos_oca/static/src/app/screens/ticket_screen/ticket_screen.js",
Copy link
Member

Choose a reason for hiding this comment

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

Esta ruta está mal

@peluko00 peluko00 force-pushed the 17.0-mig-l10n_es_pos branch 2 times, most recently from d0d4432 to c0b04e6 Compare September 30, 2024 15:31
@peluko00
Copy link
Contributor Author

@peluko00 Convendría que probases mínimamente los cambios antes de pedir revisiones. Ahora mismo ni siquiera se cargan los assets correctamente...

Perdona @chienandalu se me olvido, he hecho una refactorización porque ha cambiado bastante en relación a la v16. Lo he probado y creo que funciona como deberia, le podrias hechar un ojo cuando puedas. Puede ser que algunas cosas se puedan mejorar. Gracias de antemano!

@peluko00
Copy link
Contributor Author

@miquelalzanillas Traducciones realizadas, lo puedes revisar cuando puedas. Gracias

@peluko00
Copy link
Contributor Author

Buenas @pedrobaeza @miquelalzanillas @chienandalu podreis revisarlo cuando podais asi ya lo cerramos. Gracias!

@miquelalzanillas
Copy link
Contributor

Hola @peluko00 ,

He vuelto a probar esto. Ahora todo funciona según lo esperado con la única excepción de que cuando el importe del pedido supera el limite de la factura simplificada, si no se ha especificado un cliente antes de validar el pedido, se establece automáticamente el partner "Simplified Invoice Partner (ES)".

Yo creo que esto no debería establecerse automáticamente. Debería mantener lo mismo que en la v16, es decir, debería mostrar una advertencia al usuario para obligarle a establcer un partner antes de poder validar el ticket.

Por mi parte, una vez esto esté corregido, estaría listo para mergear.

@peluko00
Copy link
Contributor Author

Hola @peluko00 ,

He vuelto a probar esto. Ahora todo funciona según lo esperado con la única excepción de que cuando el importe del pedido supera el limite de la factura simplificada, si no se ha especificado un cliente antes de validar el pedido, se establece automáticamente el partner "Simplified Invoice Partner (ES)".

Yo creo que esto no debería establecerse automáticamente. Debería mantener lo mismo que en la v16, es decir, debería mostrar una advertencia al usuario para obligarle a establcer un partner antes de poder validar el ticket.

Por mi parte, una vez esto esté corregido, estaría listo para mergear.

Corregido, gracias!

Copy link
Contributor

@miquelalzanillas miquelalzanillas left a comment

Choose a reason for hiding this comment

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

Por mi parte todo ok.

@chienandalu crees que podríamos darle un empujón?

Gracias @peluko00 !

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.