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

[16.0][IMP] l10n_es_vat_book: Added missing taxes to mapping book #3813

Merged

Conversation

edlopen
Copy link
Member

@edlopen edlopen commented Nov 19, 2024

Copy link

@fcvalgar fcvalgar left a comment

Choose a reason for hiding this comment

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

Revisión funcional.

LGTM, gracias @edlopen

@fcvalgar
Copy link

@edlopen fallan los tests. He probado sin problemas pero si lo puedes revisar. Gracias.

@pedrobaeza
Copy link
Member

El fallo en los tests es por esto: odoo/odoo#187250 (review)

Lo están solucionando en odoo/odoo#187606

@pedrobaeza pedrobaeza added this to the 16.0 milestone Nov 19, 2024
@fcvalgar
Copy link

El fallo en los tests es por esto: odoo/odoo#187250 (review)

Lo están solucionando en odoo/odoo#187606
Gracias por el aviso @pedrobaeza .

@yajo
Copy link
Member

yajo commented Nov 20, 2024

¿Puedes rebasar @edlopen porfa? Ya fusionaron el fix que comenta @pedrobaeza ayer.

@pedrobaeza
Copy link
Member

Espera, OCB no tiene el parche. Estoy haciendo un pull + push manual.

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.

Por la hora, sí que había entrado finalmente, pero en cualquier caso, he hecho push con todo lo existente.

Lo que no estoy de acuerdo es en añadiros como mantenedores ni tampoco añadir dos contribuidores para este parche.

Copy link
Contributor

@Christian-RB Christian-RB left a comment

Choose a reason for hiding this comment

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

El cambio parece correcto. Desconozco el protocolo específico para convertirse en maintainer de un módulo, pero intuyo que es un tema delicado, especialmente considerando que incluye su imagen de perfil de GitHub en el README. En cuanto a añadirse como contribuidor, pese a no ser un gran cambio no me parece mal que el author del PR se añada a la lista. Igualmente dejo la resolución en manos de los más veteranos.

@pedrobaeza
Copy link
Member

De los contribuidores, ahora veo que en realidad no se están añadiendo dos, si no uno y "reordenando" el otro. Eso puede valer, aunque por parte de este equipo en concreto siempre se intente introducir nombres al más mínimo cambio. Hay contribuidores que hacen más sin buscar introducirse siempre, pero lo dicho, no es algo que se prohíba.

Pero la cuestión de mantenedores sí que no, ya que no es un módulo que hayan mantenido ellos activamente en toda su historia.

@edlopen edlopen force-pushed the 16.0-imp-l10n_es_vat_book-add_taxes_to_book branch from df99e71 to 39ccf7e Compare November 20, 2024 14:17
@edlopen
Copy link
Member Author

edlopen commented Nov 20, 2024

@pedrobaeza tienes razón, acabo de modificar el manifest.

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.

Gracias por considerar los cambios.

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-3813-by-pedrobaeza-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 474d012 into OCA:16.0 Nov 20, 2024
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 354716b. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants