search

OCA / l10n-spain

Odoo Spain Localization
https://www.aeodoo.org/estado-localizacion
GNU Affero General Public License v3.0
293 stars 522 forks source link

[17.0][MIG] l10n_es_vat_prorate: Migration to 17.0 #3583

Closed manuelregidor closed 6 months ago

manuelregidor commented 6 months ago

Standard migration + test adicional

T-6084

HaraldPanten commented 6 months ago

/ocabot migration l10n_es_vat_prorate

pedrobaeza commented 6 months ago

Según veo en tu enlace, no es un error real que provoque un rojo (eso lo provoca otra cosa del otro PR), pero bueno, entendido ahora. A ver, creo que podría considerarse error del otro módulo: si no es IBAN, no debería evaluarse, o si lo que se hace es un try, hacer un mute_logger para evitar eso tan feo en los logs. ¿Lo cambiáis?

ValentinVinagre commented 6 months ago

@manuelregidor el error si no me equivoco viene dado por el archivo de "test/test_prorate_sii.py" que en los imports no está contemplado que el módulo del SII no esté disponible en la rama, se da por supuesto que si está. Es una dependencia soft y por eso no está indicado como dependencia el módulo "l10n_es_aeat_sii_oca" en el manifest. Se tendría que tener controlada esta dependencia soft en el archivo de TEST para que no de error con un try. https://github.com/OCA/l10n-spain/blob/01ee6bb711d95200794dfe22604915fcdedefe7a/l10n_es_vat_prorate/tests/test_prorate_sii.py#L5

HaraldPanten commented 6 months ago

Según veo en tu enlace, no es un error real que provoque un rojo (eso lo provoca otra cosa del otro PR), pero bueno, entendido ahora. A ver, creo que podría considerarse error del otro módulo: si no es IBAN, no debería evaluarse, o si lo que se hace es un try, hacer un mute_logger para evitar eso tan feo en los logs. ¿Lo cambiáis?

Correcto. Por un lado está el tema del IBAN (que es solo un aviso), pero lo que falla es lo que comenta Valentín.

manuelregidor commented 6 months ago

@pedrobaeza @ValentinVinagre @HaraldPanten Se podría modificar el init.py de la carpeta tests para dejarlo de la siguiente manera:

from . import test_vat_prorate
try:
    from . import test_prorate_sii
except ImportError:
    pass
pedrobaeza commented 6 months ago

Por qué no poner el try dentro del archivo test_prorate_sii?

manuelregidor commented 6 months ago

@pedrobaeza Si ponemos el try en la línea de import de test_prorate_sii.py sigue saltando un error por la siguiente línea:

class TestSIIVatProrate(test_l10n_es_aeat_sii.TestL10nEsAeatSiiBase):

pedrobaeza commented 6 months ago

Hay que hacer lo siguiente:

try:
    from odoo.addons.l10n_es_aeat_sii_oca.tests.test_l10n_es_aeat_sii import TestL10nEsAeatSiiBase
except ImportError:
    TestL10nEsAeatSiiBase = object

@tagged("-at_install", "post_install")
class TestSIIVatProrate(TestL10nEsAeatSiiBase):
manuelregidor commented 6 months ago

@pedrobaeza Hecho y funcionando. Muchas gracias. @HaraldPanten Puedes revisar funcionalmente, por favor?

pedrobaeza commented 6 months ago

OK, aunque faltaría por probar la parte del SII. Creo que sería mejor esperar a fusionar el otro y entonces hacer rebase aquí para que se ejecuten los tests.

HaraldPanten commented 6 months ago

OK, aunque faltaría por probar la parte del SII. Creo que sería mejor esperar a fusionar el otro y entonces hacer rebase aquí para que se ejecuten los tests.

Y eso? Cuando se vaya a fusionar el SII (una vez esté acabado) no lanzará los tests de toda la rama? Eso no comprobará que sea correcto?

O lo dices porque quieres hacer revisión funcional, etc.?

pedrobaeza commented 6 months ago

Sí, al fusionar saldrá rojo si no es correcto, pero qué pasa si éste está mal en lugar del SII? Tocará hacer cambios de nuevo en este módulo. Lo decía por evitar eso.

HaraldPanten commented 6 months ago

Sí, al fusionar saldrá rojo si no es correcto, pero qué pasa si éste está mal en lugar del SII? Tocará hacer cambios de nuevo en este módulo. Lo decía por evitar eso.

En este caso, según como lo veo yo, se dará una situación que a nosotros (y sé que a vosotros también) se nos da en el día a día. "Mi PR falla por los tests de otro módulo, o directamente por otro módulo que nada tienen que ver con el mío". Entonces tocará arreglarlo por la persona que esté migrando el SII (en este caso también nosotros 😂) .

El caso es que este módulo puede ser utilizado sin el SII, por lo que (a priori) no veo que el SII deba "parar" la fusión de este PR. ¿Sabes qué te quiero decir?

De todas formas, si crees que lo más conveniente es esperar, por nuestra parte no hay problema. Lo único es que vamos a empezar con la prorrata del modelo 303 también, y entonces tendremos esta dependencia no satisfecha.

pedrobaeza commented 6 months ago

Venga, pues reviso y fusionamos si procede

OCA-git-bot commented 6 months ago

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

manuelregidor commented 6 months ago

@pedrobaeza @HaraldPanten A la espera del OK de los últimos cambios para fusionar commits.

manuelregidor commented 6 months ago

@pedrobaeza Mensaje del commit cambiado

manuelregidor commented 6 months ago

@pedrobaeza @HaraldPanten Tengo el problema localizado, subo cambios en un momento.

manuelregidor commented 6 months ago

@pedrobaeza @HaraldPanten cambios aplicados

HaraldPanten commented 6 months ago

Yo lo veo bien. En cuanto se valide técnicamente por parte de Pedro, fusionamos

OCA-git-bot commented 6 months ago

What a great day to merge this nice PR. Let's do it! Prepared branch 17.0-ocabot-merge-pr-3583-by-pedrobaeza-bump-nobump, awaiting test results.

OCA-git-bot commented 6 months ago

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

OCA-git-bot commented 6 months ago

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