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.

Sign up for GitHub

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

Jump to bottom

[13.0][MIG]l10n_es_aeat_mod190 #1701

Merged
merged 14 commits into from Jun 8, 2021
Merged

[13.0][MIG]l10n_es_aeat_mod190 #1701

merged 14 commits into from Jun 8, 2021

Conversation

manuelregidor
Copy link
Contributor

supersedes #1504

@HaraldPanten HaraldPanten mentioned this pull request May 21, 2021
Closed
35 tasks
@manuelcalerosolis
Copy link

@manuelregidor please check this error in the test

File "/home/travis/build/OCA/l10n-spain/l10n_es_aeat_mod303/tests/test_l10n_es_aeat_mod303.py", line 547, in test_model_303
self.env.ref("l10n_es_aeat.res_partner_aeat"),
AssertionError: res.partner() != res.partner(44,)

@etobella
Copy link
Member

Has podido revisarlo @manuelregidor ?

@manuelregidor
Copy link
Contributor Author

@manuelcalerosolis @etobella Lo he revisado pero no he conseguido solucionarlo. Cuando se fuerza la llamada a _onchange_partner_id desde el método create de AccountMove del módulo del modelo del 190, fallan los tests del 303 porque se pierde el valor de partner_id de line_ids. Le he dado bastantes vueltas pero no sé cómo arreglarlo. Cualquier sugerencia será bienvenida. Muchas gracias y un saludo.

@manuelcalerosolis
Copy link

Lo voy a mirar a ver si puedo saber q está pasando.

@manuelcalerosolis
Copy link

@manuelregidor de momento tan solo he observado un comportamiento a mi parecer extraño.

  1. Monto una base de datos nueva.
  2. Paso los test del modulo 303, pasan todos correctamente.
  3. Paso los test del modulo 190, pasan todos correctamente.
  4. Vuelvo a pasar los test del modulo 303, y me da el mismo fallo que a ti.

Ahora seguiré mirando a ver si puedo ir aportar algo mas.

@manuelregidor
Copy link
Contributor Author

@manuelcalerosolis Sí, el problema se presenta al utilizar el create de AccountMove que hereda el módulo 190. Lo que intentaré es prescindir de la función _onchange_partner_id y crear una función aislada adicional que se lance al cambiar el partner_id pero que no llame a ningún super, a ver si así funciona.

@pedrobaeza
Copy link
Member

Me decís si no conseguís nada.

@manuelregidor manuelregidor force-pushed the 13.0-mig-l10n_es_aeat_mod190 branch 2 times, most recently from 1e952aa to 7b2ffd3 Compare May 31, 2021 14:08
@HaraldPanten HaraldPanten mentioned this pull request May 31, 2021
Closed
HaraldPanten
HaraldPanten approved these changes May 31, 2021
View reviewed changes
Copy link
Contributor

@HaraldPanten HaraldPanten 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. ¿Qué te parece @etobella ?

etobella
etobella requested changes May 31, 2021
View reviewed changes
l10n_es_aeat_mod190/models/account_move_line.py Outdated Show resolved Hide resolved
l10n_es_aeat_mod190/models/account_move_line.py Outdated Show resolved Hide resolved
l10n_es_aeat_mod190/models/l10n_es_aeat_mod190_report.py Outdated Show resolved Hide resolved
l10n_es_aeat_mod190/models/l10n_es_aeat_mod190_report.py
@@ -72,7 +69,7 @@ def button_confirm(self):
self._check_report_lines()
return super(L10nEsAeatMod190Report, self).button_confirm()

@api.multi
# flake8: noqa: C901
Copy link
Member

Choose a reason for hiding this comment

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

Por que quitas el flake8?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hola @etobella, en la función compute, el flake8 indica que la función es demasiado compleja con el warning C901 (únicamente he desactivado este aviso). En los demás modelos, esta función también es muy compleja, por lo que lo he dejado así y no la he refactorizado.

@HaraldPanten
Copy link
Contributor

@etobella ¿qué tal lo ves ahora?

etobella
etobella approved these changes Jun 8, 2021
View reviewed changes
Copy link
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@OCA-git-bot
Copy link
Contributor

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). 🤖

@HaraldPanten
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 13.0-ocabot-merge-pr-1701-by-HaraldPanten-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jun 8, 2021
@OCA-git-bot
Merge PR #1701 into 13.0
5d7e03c 
Signed-off-by HaraldPanten
@OCA-git-bot
Copy link
Contributor

It looks like something changed on 13.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 13.0-ocabot-merge-pr-1701-by-HaraldPanten-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 59805c4 into OCA:13.0 Jun 8, 2021
@OCA-git-bot
Copy link
Contributor

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

@HaraldPanten HaraldPanten deleted the 13.0-mig-l10n_es_aeat_mod190 branch June 8, 2021 16:51
pedrobaeza
pedrobaeza reviewed Jun 10, 2021
View reviewed changes
l10n_es_aeat_mod190/data/res_country_state.xml
@@ -0,0 +1,159 @@
<?xml version="1.0" encoding="UTF-8" ?>
<odoo>
<record id="base.state_es_vi" model="res.country.state">
Copy link
Member

Choose a reason for hiding this comment

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

Revisando esto a posteriori, no debería existir. Es una sobrecarga no necesaria. Tenemos https://github.com/OCA/l10n-spain/blob/13.0/l10n_es_aeat/models/spanish_states_mapping.py para ello.

@HaraldPanten
Copy link
Contributor

@manuelregidor ¿puedes revisar lo que comenta Pedro?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedrobaeza pedrobaeza pedrobaeza left review comments

@etobella etobella etobella approved these changes

@HaraldPanten HaraldPanten HaraldPanten approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@manuelregidor @manuelcalerosolis @etobella @pedrobaeza @HaraldPanten @OCA-git-bot @jvpascual @oca-travis @angelmoya @cubells