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][MIG] l10n_es_aeat_sii_oss: Migration to 16.0 #3112

Merged
merged 8 commits into from
Jun 28, 2023

Conversation

JavierIniesta
Copy link

@JavierIniesta JavierIniesta mentioned this pull request Jun 20, 2023
54 tasks
@JavierIniesta JavierIniesta changed the title [MIG] l10n_es_aeat_sii_oss: Migration to 16.0 [WIP] [MIG] l10n_es_aeat_sii_oss: Migration to 16.0 Jun 20, 2023
@JavierIniesta JavierIniesta force-pushed the 16.0-mig-l10n_es_aeat_sii_oss branch 3 times, most recently from d8d496c to 14ee322 Compare June 20, 2023 14:38
@JavierIniesta JavierIniesta changed the title [WIP] [MIG] l10n_es_aeat_sii_oss: Migration to 16.0 [WIP] [16.0] [MIG] l10n_es_aeat_sii_oss: Migration to 16.0 Jun 21, 2023
@JavierIniesta JavierIniesta changed the title [WIP] [16.0] [MIG] l10n_es_aeat_sii_oss: Migration to 16.0 [WIP][16.0][MIG] l10n_es_aeat_sii_oss: Migration to 16.0 Jun 21, 2023
Copy link
Contributor

@RodrigoBM RodrigoBM left a comment

Choose a reason for hiding this comment

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

Code review

@ACheung-FactorLibre
Copy link
Contributor

LGTM.

@pedrobaeza
Copy link
Member

/ocabot migration l10n_es_aeat_sii_oss

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Jun 26, 2023
@pedrobaeza
Copy link
Member

Está aún como draft y [WIP]. Si ya está terminado, quitad esa etiqueta.

@JavierIniesta
Copy link
Author

Buenos días @pedrobaeza

Vamos a realizar algunas pruebas funcionales más. En cuanto las hayamos pasado, quito las etiquetas y aviso por aquí.

Muchas gracias!

@JavierIniesta JavierIniesta changed the title [WIP][16.0][MIG] l10n_es_aeat_sii_oss: Migration to 16.0 [16.0][MIG] l10n_es_aeat_sii_oss: Migration to 16.0 Jun 26, 2023
@JavierIniesta JavierIniesta marked this pull request as ready for review June 26, 2023 11:42
@JavierIniesta
Copy link
Author

Tras varias pruebas funcionales, marcamos como listo.

@@ -35,6 +35,7 @@ def setUpClass(cls):

def test_invoice_sii_oss(self):
self.partner.sii_simplified_invoice = True
self.env.clear()
Copy link
Member

Choose a reason for hiding this comment

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

No tengo claro por qué necesitáis hacer esto. Puede estar tapando un problema en otro lado.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aquí @pedrobaeza estuvimos viendo que si no borrábamos la cache estaba intentando crear X registros nuevos (NewID) y nos saltaba error al hacer el sorted del metodo _compute_name.

https://github.com/OCA/OCB/blob/289e62d730435f7c9017e7f5f08788d6938ad5cb/addons/account/models/account_move.py#L683

Es posible que haya un problema con el Form y que no este borrando la cache o que este manteniendo registros en cache que no tendría que darse, pero no hemos dado con ello.

Sabes si a alguien le ha pasado esto en los test con el Form, ya que también cambiamos el Form por un create del registro y el test paso bien.

Un saludo

Copy link
Member

Choose a reason for hiding this comment

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

Ahora las facturas ya no se deberían crear con Form en v16. Se volvió a tocar para que con los mínimos vals, fuera creable desde código. Echa un vistazo a este método y si acaso, cambiad el código para utilizarlo de forma similar:

https://github.com/odoo/odoo/blob/f73c214efdbd49865ff3f0db915fadc80b4403b0/addons/account/tests/common.py#L674

Copy link
Author

Choose a reason for hiding this comment

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

Buenos días.

He modificado el test para hacerlo con create directamente. El test no era hijo de AccountTestInvoicingCommon por eso al final he hecho el create directamente. El número de líneas es mayor que cuando se hacía con Form pero no es nada significativo.

RodrigoBM and others added 7 commits June 28, 2023 08:34
- Standard procedure
- Conversion from account.invoice to account.move
- Test adaptation

TT31556
Steps to reproduce:
- Have partner 1 with fiscal position FP1 with registration key 01.
- Have partner 2 with fiscal position FP2 with registration key 02.
- Create customer invoice with partner 1 as customer.
- Select partner 2 as delivery address.

Expected result:
Fiscal position is changed to FP2 and the registration key is changed
to 02.

Current behavior:
The fiscal position is changed, but not the registration key.

That's because the recursive onchanges are explicitly disabled in
account move object:

https://github.com/odoo/odoo/blob/2f817a7b36cc7e5ab235829eecd61a1d71ce546e/addons/account/models/account_move.py#L1190

We workaround this limitation converting the registration key into a
computed writable field that is computed even if the recursive onchange
is disabled.
@JavierIniesta JavierIniesta force-pushed the 16.0-mig-l10n_es_aeat_sii_oss branch from 14ee322 to f341fae Compare June 28, 2023 06:34
}
invoice = (
self.env["account.move"]
.with_context(default_move_type="out_invoice")
Copy link
Member

Choose a reason for hiding this comment

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

Sigue haciendo falta pasar este contexto?

Copy link
Author

Choose a reason for hiding this comment

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

Lo he metido porque en el _create_invoice:

A mí tampoco me parece necesario, pero como lo meten ahí, por hacerlo de manera similar. Si me dices de quitarlo, lo quito.

Copy link
Member

Choose a reason for hiding this comment

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

Creo que ahí lo han hecho por el "por si acaso". Prueba a quitarlo a ver si sigue todo verde.

Copy link
Author

Choose a reason for hiding this comment

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

Hecho!

@JavierIniesta JavierIniesta force-pushed the 16.0-mig-l10n_es_aeat_sii_oss branch from f341fae to 3b66181 Compare June 28, 2023 07:14
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.

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-3112-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit b1fa178 into OCA:16.0 Jun 28, 2023
@OCA-git-bot
Copy link
Contributor

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

@RodrigoBM RodrigoBM deleted the 16.0-mig-l10n_es_aeat_sii_oss branch June 29, 2023 08:48
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.

7 participants