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_sct_cbi #4367

Merged
merged 7 commits into from
Oct 22, 2024

Conversation

toita86
Copy link

@toita86 toita86 commented Sep 10, 2024

Backport of #4189 that depends on OCA/bank-payment#1344

added the backport of #4379

@francesco-ooops
Copy link
Contributor

/ocabot migration l10n_it_sepa_bonifici

@OCA-git-bot OCA-git-bot added this to the 14.0 milestone Sep 13, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request Sep 13, 2024
76 tasks
@toita86 toita86 force-pushed the 14.0-MIG-l10n_it_sct_cbi-toita86 branch from 4785ca6 to 6bd9793 Compare September 13, 2024 08:43
@SirAionTech
Copy link
Contributor

Sto correggendo un problema nel modulo 16.0 in #4379, probabilmente sarà da includere anche qui

Copy link

@MaurizioConte MaurizioConte left a comment

Choose a reason for hiding this comment

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

da un test effettuato il file generato contiene il tag ReqdExctnDt che ha un tag **<Dt/>** che non è corretto è un di più che non ci deve essere infatti correggendo la stringa sul file e ripassando per il controllo sul sito CBI funziona.

<ReqdExctnDt><Dt>2024-09-26</Dt><Dt/></ReqdExctnDt>

@francesco-ooops
Copy link
Contributor

da un test effettuato il file generato contiene il tag ReqdExctnDt che ha un tag **<Dt/>** che non è corretto è un di più che non ci deve essere infatti correggendo la stringa sul file e ripassando per il controllo sul sito CBI funziona.

<ReqdExctnDt><Dt>2024-09-26</Dt><Dt/></ReqdExctnDt>

@SirAionTech ti risulta anche sulla v16 questo problema?

@SirAionTech
Copy link
Contributor

da un test effettuato il file generato contiene il tag ReqdExctnDt che ha un tag **<Dt/>** che non è corretto è un di più che non ci deve essere infatti correggendo la stringa sul file e ripassando per il controllo sul sito CBI funziona.
<ReqdExctnDt><Dt>2024-09-26</Dt><Dt/></ReqdExctnDt>

@SirAionTech ti risulta anche sulla v16 questo problema?

I file prodotti da #4189 erano validi

su https://www.cbiservice.com/correttoresepa.php, selezionando l'opzione: SCT - CBIBdyPaymentRequest 00.04.01 (new).

Non so se si sta parlando di un altro controllo o se magari si riproduce solo con certi passi, se devo verificarlo secondo me servono più info.

@MaurizioConte
Copy link

MaurizioConte commented Sep 27, 2024 via email

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.

Test funzionale: OK

Copy link

@MaurizioConte MaurizioConte left a comment

Choose a reason for hiding this comment

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

Test OK

@francesco-ooops
Copy link
Contributor

@SirAionTech si può mergiare?

Copy link
Contributor

@aleuffre aleuffre 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: LGTM. Però mi sembra che i test non vengano eseguiti, puoi verificare?

@francesco-ooops
Copy link
Contributor

/ocabot rebase

SirAionTech and others added 4 commits October 16, 2024 09:21
Use new standards
Added namespace to pain descendants
IBAN is mandatory
Initiating party is mandatory
FinInstd/Othr is not allowed
ReqdExctnDt has new structure
BICFI in PaymentRequest instead of BIC
Manage multiple priorities
Added test
Updated README
Updated translations
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: l10n-italy-16.0/l10n-italy-16.0-l10n_it_sct_cbi
Translate-URL: https://translation.odoo-community.org/projects/l10n-italy-16-0/l10n-italy-16-0-l10n_it_sct_cbi/
@OCA-git-bot
Copy link
Contributor

Congratulations, PR rebased to 14.0.

@OCA-git-bot OCA-git-bot force-pushed the 14.0-MIG-l10n_it_sct_cbi-toita86 branch from 14df0d6 to 20fac5d Compare October 16, 2024 09:21
@toita86 toita86 force-pushed the 14.0-MIG-l10n_it_sct_cbi-toita86 branch from 20fac5d to 6fcf603 Compare October 21, 2024 08:13
@SirAionTech SirAionTech added the needs fixing Has conflicts or is failing mandatory CI checks label Oct 21, 2024
@toita86 toita86 force-pushed the 14.0-MIG-l10n_it_sct_cbi-toita86 branch from 6fcf603 to f05f170 Compare October 21, 2024 14:01
@SirAionTech SirAionTech removed the needs fixing Has conflicts or is failing mandatory CI checks label Oct 21, 2024
Copy link
Contributor

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

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

Tutto il resto mi sembra ok, ho solo un dubbio su uno degli ultimi cambiamenti.

Comment on lines +113 to +114
if None in namespaces:
namespaces["default"] = namespaces.pop(None)
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Non sono sicuro di questa aggiunta (e l'analoga qualche riga sotto)

@SirAionTech cosa ne pensi?

Copy link
Contributor

Choose a reason for hiding this comment

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

Non so se un find con questi nuovi namespaces possa trovare i tag del namespace incriminato (namespaces[None]), però è anche vero che per ora vengono solo cercati i tag nel namespace PMRQ quindi il test per ora credo sia rimasto valido

Copy link
Contributor

Choose a reason for hiding this comment

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

Confermo che in locale i test passano anche senza questa modifica. Se non da problemi sulle pipeline OCA, sarei per rimuoverlo.

Per info, nel test test_multiple_payment_priority la variabile namespaces contiene questo:

{
    None: 'urn:CBI:xsd:CBIBdyPaymentRequest.00.04.01', 
    'PMRQ': 'urn:CBI:xsd:CBIPaymentRequest.00.04.01'
}

Copy link
Author

@toita86 toita86 Oct 22, 2024

Choose a reason for hiding this comment

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

Confermo che anche nel mio ambiante doodba non ci sono errori di nessuna tipo ma su OCA si. Mi sono confrontato un po anche con @SirAionTech su Discord e dovrebbe essere il fatto che usa una versione di lxml diversa. Su oca è la 3.7.1 https://github.com/OCA/l10n-italy/actions/runs/11435980402/job/31812452602?pr=4367#step:4:228 mentre sul mio ambiente e sulla 16.0 è la 4.6.5

Con il debugger ho capito che il name space che viene fornito a find rompe la xpath che non può prendere argomenti nulli a quanto pare sulla versione 4.6.5 questo non è un problema.
Da questo ne è uscita la mia "rinomina" della voce None.

Copy link
Contributor

@aleuffre aleuffre 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, LGTM

@francesco-ooops
Copy link
Contributor

@SirAionTech mergiamo?

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!
Ho fatto revisione del codice e controllato la storia dei commit: c'è qualche piccola discrepanza rispetto alla storia originale, ad esempio 842c43b è diverso dal commit originale ee35db5 e manca qualche traduzione, poi andrebbero schiacciati i commit dei bot come indicato in https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests#mergesquash-the-commits-generated-by-bots-or-weblate, ma secondo me niente di bloccante soprattutto perché si tratta di un backporting, e la storia che andrà avanti sarà quella in 16.0 che è corretta.

TL;DR: /ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-4367-by-SirAionTech-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 8152d4e into OCA:14.0 Oct 22, 2024
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@toita86 toita86 deleted the 14.0-MIG-l10n_it_sct_cbi-toita86 branch October 22, 2024 14:29
@francesco-ooops francesco-ooops mentioned this pull request Nov 8, 2024
2 tasks
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.

8 participants