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

[14.0] [MIG] l10n_it_account_balance_report > l10n_it_financial_statements_report #3343

Merged

Conversation

SirTakobi
Copy link
Contributor

@SirTakobi SirTakobi commented May 30, 2023

@TonyMasciI per capire le modifiche fatte in #3169, non ho trovato altro modo che provare in prima persona la migrazione.
Manca ancora:

@SirTakobi SirTakobi force-pushed the 14.0-mig-l10n_it_account_balance_report branch from 57f522b to 39a34f6 Compare May 30, 2023 13:14
@TonyMasciI
Copy link
Contributor

@TonyMasciI per capire le modifiche fatte in #3169, non ho trovato altro modo che provare in prima persona la migrazione. Manca ancora:

* [ ]  pulire storia dei commit

* [ ]  verificare risultati report HTML/PDF

* [ ]  report XLSX

* [ ]  test automatici

* [ ]  rebase nel caso di modifiche in `12.0`

Vogliamo brasare l'altra PR e lasciare questa?

@SirTakobi
Copy link
Contributor Author

Vogliamo brasare l'altra PR e lasciare questa?

Lascerei aperta la tua finché non so se posso portare a termine questa.
cc @eLBati

@SirTakobi SirTakobi force-pushed the 14.0-mig-l10n_it_account_balance_report branch 2 times, most recently from 3708dbc to 4ace700 Compare June 23, 2023 14:35
SilvioGregorini and others added 15 commits June 23, 2023 17:36
Currently translated at 66.4% (85 of 128 strings)

Translation: l10n-italy-12.0/l10n-italy-12.0-l10n_it_account_balance_report
Translate-URL: https://translation.odoo-community.org/projects/l10n-italy-12-0/l10n-italy-12-0-l10n_it_account_balance_report/it/
Currently translated at 92.2% (118 of 128 strings)

Translation: l10n-italy-12.0/l10n-italy-12.0-l10n_it_account_balance_report
Translate-URL: https://translation.odoo-community.org/projects/l10n-italy-12-0/l10n-italy-12-0-l10n_it_account_balance_report/it/
Currently translated at 92.9% (119 of 128 strings)

Translation: l10n-italy-12.0/l10n-italy-12.0-l10n_it_account_balance_report
Translate-URL: https://translation.odoo-community.org/projects/l10n-italy-12-0/l10n-italy-12-0-l10n_it_account_balance_report/it/
Currently translated at 95.3% (122 of 128 strings)

Translation: l10n-italy-12.0/l10n-italy-12.0-l10n_it_account_balance_report
Translate-URL: https://translation.odoo-community.org/projects/l10n-italy-12-0/l10n-italy-12-0-l10n_it_account_balance_report/it/
Usando il modulo l10n_it_account_balance_report, il cron di Pulizia dati interni diventa molto lento e in alcuni casi va in blocco.

L'autovacuum della tabella report_trial_balance diventa molto lenta per via delle cascade presenti nel modulo.
Inserendo gli indici nei campi collegati il problema si risolve, come già realizzato nel repo OCA/account-financial-reporting#750.
Currently translated at 93.7% (121 of 129 strings)

Translation: l10n-italy-12.0/l10n-italy-12.0-l10n_it_account_balance_report
Translate-URL: https://translation.odoo-community.org/projects/l10n-italy-12-0/l10n-italy-12-0-l10n_it_account_balance_report/it/
@SirTakobi SirTakobi force-pushed the 14.0-mig-l10n_it_account_balance_report branch 3 times, most recently from ade1a2f to 4a0a1db Compare June 27, 2023 10:55
@SirTakobi SirTakobi marked this pull request as ready for review June 27, 2023 11:21
@SirTakobi
Copy link
Contributor Author

@TonyMasciI ho avuto modo di andare avanti, direi che puoi chiudere la tua e, se vuoi, fammi sapere cosa ne pensi di questa.

@francesco-ooops @elvise grazie del supporto per lo sviluppo (ho aggiunto Ooops nei contributors).

Magari anche qualcuno dei revisori di #3169 è interessato? @GSLabIt @andreampiovesana @stefano-ooops

cc @eLBati

@SirTakobi SirTakobi force-pushed the 14.0-mig-l10n_it_account_balance_report branch from 4a0a1db to c9530ba Compare June 27, 2023 11:30
@SirTakobi SirTakobi force-pushed the 14.0-mig-l10n_it_account_balance_report branch from c9530ba to 0056968 Compare June 27, 2023 11:36
@francesco-ooops
Copy link
Contributor

@TonyMasciI mentre facciamo la review funzionale puoi dare un'occhiata?

@francesco-ooops
Copy link
Contributor

@andreampiovesana puoi fare review? grazie!

@SirTakobi
Copy link
Contributor Author

SirTakobi commented Jul 13, 2023

Vedi anche #3064 (comment)

Alla fine ho dovuto fare il renaming 😄

Non ho usato il meccanismo di OpenUpgrade (https://oca.github.io/OpenUpgrade/use_cases/module_renaming.html) perché credo dia problemi con la versione enterprise, ho chiesto lumi in https://discord.com/channels/737652535149592587/766180676373446657/1129044284906164224.

@OCA/local-italy-maintainers si potrebbe segnare in #1905 che il modulo ha cambiato nome, che ne dite?

UPDATE: dopo https://github.com/OCA/l10n-italy/compare/c62e6014c50094fa8b7e3338c798b7a579f98d06..6de9e6abf3ad3b3b2decbf292b467b030e5919bf dovrebbe supportare sia Enterprise (dove va installato manualmente il nuovo modulo) che OpenUpgrade (dove il modulo risulterà installato automaticamente)

@eLBati eLBati mentioned this pull request Jul 14, 2023
76 tasks
@eLBati
Copy link
Member

eLBati commented Jul 14, 2023

@OCA/local-italy-maintainers si potrebbe segnare in #1905 che il modulo ha cambiato nome, che ne dite?

Fatto

@SirTakobi SirTakobi force-pushed the 14.0-mig-l10n_it_account_balance_report branch from c62e601 to 6de9e6a Compare July 14, 2023 08:54
],
"external_dependencies": {
"python": [
"openupgradelib",

Choose a reason for hiding this comment

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

This is not needed AFAIK, right @pedrobaeza ? I mean, it's just for migrations scripts, not for proper module code.

Copy link
Member

Choose a reason for hiding this comment

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

Exactly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this because it is used in pre_init_hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I am trying to support both the migration flows that I described yesterday in https://discord.com/channels/737652535149592587/766180676373446657/1129044284906164224.

That is why the migration happens both in pre_init_hook and in migrations folder.
Maybe I am a bit confused on how migrations work, I would very much appreciate your points of view.

cc @sergiocorato

By the way, big thanks for having a look!



def migrate_old_module(cr):
remove_models(

Choose a reason for hiding this comment

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

no need, odoo does it automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When this method is executed in the pre_init_hook, without this change I have WARNING messages like:

odoo.addons.base.models.ir_model: The model account_balance_report_account could not be dropped because it did not exist in the registry.

Choose a reason for hiding this comment

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

Ok, sorry, I understand that some things have to be done different when you are calling migrations scripts in a pre-init-hook. That's something unusual so I am not sure which differences encompass this approach. Feel free!

@eLBati eLBati changed the title [MIG] l10n_it_account_balance_report [14.0] [MIG] l10n_it_account_balance_report > l10n_it_financial_statements_report Jul 20, 2023
@SirTakobi SirTakobi marked this pull request as draft July 20, 2023 14:09
Lines are now hidden by super when the balance is 0, so we don't need any more overrides that do that.
hierarchy_on -> show_hierarchy
string_types now is str
XLSX class variables cannot be assigned because not in __slots__
ir.actions.report fields renames
Propagate wizard and report data to report model for printing; in super all this is done directly in the wizard.
The reports model name must be report.<module_name> and action/views must adapt.
Adapt report and code to different report data structure.
Adapt report to new HTML structure.
account.account.account_balance_report_section: selection attribute will be ignored as the field is related
Hack super's JS for report action to use our actions
Use HTML Report instead of client action
Remove inactive Maintainer
New translations format
Update italian translations
Improved coverage
Balance is too generic, Financial statements is what this module is producing
@SirTakobi SirTakobi force-pushed the 14.0-mig-l10n_it_account_balance_report branch from 6de9e6a to 1194fb2 Compare July 24, 2023 09:34
@SirTakobi SirTakobi marked this pull request as ready for review July 24, 2023 09:58
Copy link

@stefano-ooops stefano-ooops left a comment

Choose a reason for hiding this comment

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

testato ok

@francesco-ooops
Copy link
Contributor

@TonyMasciI @andreampiovesana che ne dite?

Copy link
Member

@eLBati eLBati left a comment

Choose a reason for hiding this comment

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

Tested

@francesco-ooops
Copy link
Contributor

@primes2h a livello di terminologia può andare?

Copy link
Contributor

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

@eLBati
Copy link
Member

eLBati commented Jul 28, 2023

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-3343-by-eLBati-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 15925ce into OCA:14.0 Jul 28, 2023
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at efd6632. 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.