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] FIX l10n_it_vat_registries: use consistent order in report summary #2305

Merged

Conversation

TheMule71
Copy link
Contributor

Descrizione del problema o della funzionalità:
Nel riepilogo del report, le varie imposte vengono elencate nell'ordine in cui appiaiono nelle singole righe, il che cambia da stampa a stampa.

Comportamento attuale prima di questa PR:
L'ordinamento dipende dalle righe stampate in precedenza.

Comportamento desiderato dopo questa PR:
L'ordinamento è determinato dall'ordine in cui le imposte vengono specificate in configurazione, e non cambia da stampa a stampa.

--
Confermo di aver firmato il CLA https://odoo-community.org/page/cla e di aver letto le linee guida su https://odoo-community.org/page/contributing

@TheMule71 TheMule71 changed the title FIX l10n_it_vat_registries: use consistent order in report summary [14.0] FIX l10n_it_vat_registries: use consistent order in report summary Jun 1, 2021
@eLBati
Copy link
Member

eLBati commented Jun 3, 2021

@TheMule71 non c'era la parallela per v12?

@TheMule71
Copy link
Contributor Author

@TheMule71 non c'era la parallela per v12?

#2306

@eLBati
Copy link
Member

eLBati commented Jun 10, 2021

@TheMule71 potresti creare la issue di tracciamento?
(vedi https://github.com/OCA/l10n-italy/wiki/Team-di-sviluppo )

@TheMule71
Copy link
Contributor Author

@TheMule71 potresti creare la issue di tracciamento?
(vedi https://github.com/OCA/l10n-italy/wiki/Team-di-sviluppo )

È giusta così? #2325

@eLBati
Copy link
Member

eLBati commented Jun 17, 2021

👍

@@ -223,7 +223,7 @@
</tr>
</thead>
<t
t-foreach="total_used_taxes"
t-foreach="total_used_taxes.sorted(key='sequence')"
Copy link
Member

Choose a reason for hiding this comment

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

Grazie della PR!

Suggested change
t-foreach="total_used_taxes.sorted(key='sequence')"
t-foreach="total_used_taxes.sorted()"

Senza parametri viene preso l'ordinamento definito nel model (vedi https://github.com/odoo/odoo/blob/cb996aefb4aeeac9c41359ead523d52c66d21e29/odoo/models.py#L5387-L5389) che se ho ben capito dovrebbe essere quello che si vuole ottenere da questa PR.

Specificando l'ordinamento qui per sequence si rischia di doverlo cambiare quando verrà modificato l'ordinamento nel model che attualmente è per sequence,id: https://github.com/odoo/odoo/blob/c8c68a07199124091e8d49946a63fb8c705c2417/addons/account/models/account_tax.py#L40

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Grazie per la review!

Il problema portato dal cliente era l'impossibilità di controllare l'ordine del riepilogo, oltre al fatto che cambiava da stampa a stampa. Quello che vogliono otterene è una corrispondenza 1:1 con le stampe che fa il loro commercialista col suo sw, compreso l'ordine con cui le imposte vengono riepilogate!

Quindi in realtà, il problema non è solo avere un ordinamento, e l'ordinamento di default, specie se prendiamo in considerazione l'ipotesi (oggettivamente abbastanza improbabile) che possa cambiare, non è quello ideale: quello ideale è quello impostato da interfaccia in fase di configurazione (e quindi basato su sequence, e non su altro).

In altre parole, usando quello di default avremmo il problema opposto, mettere esplicitamernte sequence come key se dovesse cambiare.

Sebbene probabilmente ai più basterebbe un ordinamento qualunque, basta che sia coerente da stampa a stampa, l'idea di poterlo controllare precisamente da configurazione non è sbagliata.

Copy link
Member

Choose a reason for hiding this comment

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

Ah quindi non importa che la visualizzazione nel backend (determinata da https://github.com/odoo/odoo/blob/c8c68a07199124091e8d49946a63fb8c705c2417/addons/account/models/account_tax.py#L40) sia coerente con la visualizzazione nel report?
Se mi confermi che è così allora va bene; secondo me può risultare inaspettato ma non lo vedo come un problema.

Per avere un ordinamento univoco aggiungerei comunque sorted('id') altrimenti a parità di sequence (ad esempio quando le crei hanno tutte lo stesso default) potresti avere report con le imposte in ordini diversi.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah quindi non importa che la visualizzazione nel backend (determinata da https://github.com/odoo/odoo/blob/c8c68a07199124091e8d49946a63fb8c705c2417/addons/account/models/account_tax.py#L40) sia coerente con la visualizzazione nel report?

Non è quello il problema riportato. Alle fine stiamo un po' discutendo del sesso degli angeli, non è che sia poi così probabile che l'ordinamento di default cambi, per cui anche mettere .sorted() e basta al 99,99% non farà mai alcuna differenza. Era più per spiegare perché avevo fatto con sequence esplicita.

@TheMule71 TheMule71 force-pushed the 14.0-fix-l10n_it_vat_registries-sorted branch from 6250bbb to 4609c9b Compare June 17, 2021 10:17
@TheMule71
Copy link
Contributor Author

Azz nella 12.0 è stata già mergiata :)

Copy link
Member

@SimoRubi SimoRubi left a comment

Choose a reason for hiding this comment

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

Review del codice, per me è ok

TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jun 18, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jun 18, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jun 18, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jul 2, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jul 2, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jul 9, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jul 9, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jul 9, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jul 9, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jul 17, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jul 20, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jul 30, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jul 30, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Aug 6, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Aug 20, 2021
@SimoRubi
Copy link
Member

@OCA/local-italy-maintainers si può mergiare o servono altre modifiche?
Grazie!

TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Sep 3, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Sep 17, 2021
@tafaRU
Copy link
Member

tafaRU commented Sep 24, 2021

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-2305-by-tafaRU-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 08b4852 into OCA:14.0 Sep 24, 2021
@OCA-git-bot
Copy link
Contributor

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

TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Oct 1, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Oct 1, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Oct 8, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Oct 8, 2021
@TheMule71 TheMule71 deleted the 14.0-fix-l10n_it_vat_registries-sorted branch October 15, 2021 16:19
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Oct 15, 2021
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Oct 15, 2021
TheMule71 added a commit to TheMule71/l10n-italy that referenced this pull request Jan 21, 2022
TheMule71 added a commit to TheMule71/l10n-italy that referenced this pull request Jan 21, 2022
TheMule71 added a commit to TheMule71/l10n-italy that referenced this pull request Jan 21, 2022
TheMule71 added a commit to TheMule71/l10n-italy that referenced this pull request Jan 21, 2022
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jun 29, 2022
TheMule71 added a commit to odoo-italia/l10n-italy that referenced this pull request Jun 29, 2022
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.

5 participants