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_it_split_payment #3387

Merged
merged 35 commits into from
Sep 29, 2023

Conversation

Borruso
Copy link
Contributor

@Borruso Borruso commented Jun 16, 2023

Migration l10n_it_split_payment from 14.0 to 16.0

--
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

@tafaRU
Copy link
Member

tafaRU commented Jun 16, 2023

/ocabot migration l10n_it_split_payment

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Jun 16, 2023
@OCA-git-bot
Copy link
Contributor

The migration issue (#2967) has been updated to reference the current pull request.
however, a previous pull request was referenced : #3132.
Perhaps you should check that there is no duplicate work.
CC : @TheMule71

@OCA-git-bot OCA-git-bot mentioned this pull request Jun 16, 2023
79 tasks
@Borruso Borruso force-pushed the 16.0-mig-l10n_it_split_payment branch 4 times, most recently from f31ec42 to 513d6bd Compare June 16, 2023 15:53
@Borruso Borruso marked this pull request as ready for review June 16, 2023 15:53
@Borruso Borruso force-pushed the 16.0-mig-l10n_it_split_payment branch 2 times, most recently from 4bf5438 to c04fb94 Compare July 6, 2023 12:16
Copy link
Contributor

@MarcoCalcagni MarcoCalcagni left a comment

Choose a reason for hiding this comment

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

Test Funzionali OK .

go to merge

@MarcoCalcagni
Copy link
Contributor

@tafaRU @eLBati
profumo di merge ...

Copy link
Contributor

@scigghia scigghia left a comment

Choose a reason for hiding this comment

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

👍

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

Copy link

@MaurizioPellegrinet MaurizioPellegrinet left a comment

Choose a reason for hiding this comment

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

LGTM

@Borruso Borruso force-pushed the 16.0-mig-l10n_it_split_payment branch from c04fb94 to 4cc56ba Compare September 1, 2023 08:33
@Borruso
Copy link
Contributor Author

Borruso commented Sep 1, 2023

@eLBati aggiunto maintainers

@tafaRU
Copy link
Member

tafaRU commented Sep 1, 2023

@Borruso la richiesta di essere aggiunti come maintainer è preferibile farla successiva al merge di questa PR in una PR separata. Anche perché non avrebbe effetto immediato, cioè non avresti già i permessi di potere eseguire OCA Bot sulla PR corrente. Grazie.

@Borruso
Copy link
Contributor Author

Borruso commented Sep 1, 2023

@Borruso la richiesta di essere aggiunti come maintainer è preferibile farla successiva al merge di questa PR in una PR separata. Anche perché non avrebbe effetto immediato, cioè non avresti già i permessi di potere eseguire OCA Bot sulla PR corrente. Grazie.

mi aveva detto @eLBati di farlo che poi mergiava la PR ora lo tolgo

@Borruso Borruso force-pushed the 16.0-mig-l10n_it_split_payment branch from 4cc56ba to 2cdc16b Compare September 1, 2023 10:44
Copy link
Contributor

@SirAionTech SirAionTech left a comment

Choose a reason for hiding this comment

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

Grazie della PR!
Potresti modificare la storia dei commit come indicato in https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests?

</field>
<field name="amount_tax" position="after">
<field
name="amount_sp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Come mai non viene più mostrato il totale dello split tra i totali della fattura?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

si mostrava il totale delle imposte sulla fattura cosa che sulla 16 mostra anche senza questo pezzo di codice

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, quando hai finito le modifiche per favore ricordati di chiedermi di aggiornare la review così ricontrollo

Copy link
Contributor

Choose a reason for hiding this comment

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

si mostrava il totale delle imposte sulla fattura cosa che sulla 16 mostra anche senza questo pezzo di codice

Dov'è l'importo dello split payment?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

vedi in basso a destra Taxes: $ 22.00

Copy link
Contributor

Choose a reason for hiding this comment

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

alla 14 il campo di odoo non si vedeva perché era a 0
alla 16 il campo di odoo della tassa è valorizzato in altro modo e ora è visibile

Ripensandoci: quindi oltre all'interfaccia diversa ci sono anche dati diversi nella 16.0 rispetto alla 14.0?
Da quanto hai scritto sembra che il totale delle imposte fosse 0 in 14.0 mentre in 16.0 non è più così

Copy link
Member

Choose a reason for hiding this comment

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

Configurando il tax group correttamente:
image

si ottiene:

image

Non è sufficiente?

Copy link
Contributor

Choose a reason for hiding this comment

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

Configurando il tax group correttamente: image

si ottiene:

image

Non è sufficiente?

Nel README c'è scritto che il gruppo deve chiamarsi Taxes:
image
(da https://github.com/OCA/l10n-italy/blob/e36407bb8b31e175be3c84470193b4687d8b1b27/l10n_it_split_payment/static/SP2.png)
ma dall'immagine in realtà sembra essere un Many2One..?
Quindi se la configurazione va fatta in quel modo c'è da correggere il README.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aggirnato readme

Copy link
Contributor

Choose a reason for hiding this comment

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

Grazie, ora configurando il modulo come spiegato nel README è possibile visualizzare l'importo dello split payment tra i totali della fattura:
image

C'è qualche incoerenza nella nomenclatura negli step di configurazione ma niente di bloccante.

alla 14 il campo di odoo non si vedeva perché era a 0
alla 16 il campo di odoo della tassa è valorizzato in altro modo e ora è visibile

Ripensandoci: quindi oltre all'interfaccia diversa ci sono anche dati diversi nella 16.0 rispetto alla 14.0? Da quanto hai scritto sembra che il totale delle imposte fosse 0 in 14.0 mentre in 16.0 non è più così

Ho anche verificato che il totale delle imposte (amount_tax) è 0 come era in 14.0.
Quello che viene mostrato adesso nei totali è il totale del gruppo di imposte Split Payment (creato dal modulo con le ultime modifiche) che però non va a contribuire al totale delle imposte per la fattura.

@Borruso Borruso force-pushed the 16.0-mig-l10n_it_split_payment branch from b55d19d to 3504091 Compare September 29, 2023 07:45
@Borruso
Copy link
Contributor Author

Borruso commented Sep 29, 2023

Anche i commit consecutivi dello stesso traduttore si possono schiacciare:

The same may happen if a translation is done in Weblate in several time frames, generating several commits across time.
You can merge all those commits together or with the commit that generates them.

(da https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests#mergesquash-the-commits-generated-by-bots-or-weblate)

Quindi puoi schiacciare questi commit? image

fatto

@tafaRU
Copy link
Member

tafaRU commented Sep 29, 2023

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-3387-by-tafaRU-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Sep 29, 2023
Signed-off-by tafaRU
@OCA-git-bot
Copy link
Contributor

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

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

@OCA-git-bot OCA-git-bot merged commit 7f05438 into OCA:16.0 Sep 29, 2023
6 checks passed
@OCA-git-bot
Copy link
Contributor

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