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 ricevute bancarie #3131

Closed

Conversation

TonyMasciI
Copy link
Contributor

No description provided.

@TonyMasciI TonyMasciI mentioned this pull request Jan 13, 2023
79 tasks
@tafaRU
Copy link
Member

tafaRU commented Jan 13, 2023

/ocabot migration l10n_it_ricevute_bancarie

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Jan 13, 2023
@TonyMasciI TonyMasciI force-pushed the 16.0-mig-l10n_it_ricevute_bancarie branch from 6a25095 to 5887c7a Compare January 13, 2023 11:31
@TonyMasciI
Copy link
Contributor Author

@tafaRU @eLBati Vi torna l'errore che è presente in pre-commit, se vi torna mi spiegate cosa devo fare?

@TonyMasciI
Copy link
Contributor Author

Ho degli errori nei test in questa PR sono legati al fato che le imposte DEMO del modulo l10n_it_fatturapa non è settato il campo country_id con Italia nel FILE facendo scattare questo pezzo di CODICE, dato che è stato già mergiato penso di aprire una ISSUE per poter mandare avanti i test in questa PR, @TheMule71 dato che ti sei occupato della migrazione del modulo l10n_it_fatturapa ti volevo chiedere se posso procedere con Issue e PR o c'è da considerare altro?

@TheMule71
Copy link
Contributor

Ho degli errori nei test in questa PR sono legati al fato che le imposte DEMO del modulo l10n_it_fatturapa non è settato il campo country_id con Italia nel FILE facendo scattare questo pezzo di CODICE, dato che è stato già mergiato penso di aprire una ISSUE per poter mandare avanti i test in questa PR, @TheMule71 dato che ti sei occupato della migrazione del modulo l10n_it_fatturapa ti volevo chiedere se posso procedere con Issue e PR o c'è da considerare altro?

Perche le riba dipendono da l10n_it_fatturapa_out? In teoria non dovresti nemmeno accedere alle imposte definite da l10n_it_fatturapa.

Il fatto che le imposte DEMO di l10n_it_fatturapa debbano essere create con country_id Italia e non USA potrei anche essere d'accordo anche se non so che impatto possa avere la cosa su tutti i test degli altri moduli, ma se dobbiamo cambiarla tanto vale cambiarla adesso.

Trovo strana la dipendenza di questo modulo da l10n_it_fatturapa_out, e ne segue che non dovrebbe essere questa la ragione per la modifica a l10n_it_fatturapa (al di là che possa avere una ragione a sè stante).

I test di questo modulo dovrebbero funzionare anche senza l10n_it_fatturapa. E se dipendono da dati demo esterni:

  1. dovrebbe essere esplicita (non c'è alcun riferimento a l10n_it_fatturapa nel codice);
  2. i dati demo in questione andrebbero spostati magari su l10n_it_account se non hanno ha che fare solo con la fatturazione elettronica.

@TonyMasciI
Copy link
Contributor Author

l10n_it_fatturapa_out

@TheMule71 non dipende direttamente da l10n_it_fatturapa ma da l10n_it_fatturapa_out che dipende da l10n_it_fatturapa, cosa ho notato, quando esegue il codice di TEST e cerca delle imposte per usarle nei TEST UNITARI ovviamente trova quelle generate nei dati demo del modulo l10n_it_fatturapa che però non hanno la country_id italiana. Quindi non fa riferimento direttamente ad un record generato in l10n_it_fatturapa ma tramite search le trova e quindi poi va in errore in questo pezzo di CODICE.

Quindi non ha senso per me far riferimento al l10n_it_fatturapa direttamente sarebbe errato, i Dati demo sono correttamente creati in l10n_it_fatturapa ma bisogna solo aggiungere la country_id anche perché il pezzo di CODICE non è presente nella V14 ma solo nella V16, quindi va corretta come cosa. Fammi sapere se sei d'accordo!

@TheMule71
Copy link
Contributor

TheMule71 commented Jan 16, 2023

@TheMule71 non dipende direttamente da l10n_it_fatturapa ma da l10n_it_fatturapa_out

Infatti avevo chiesto:

Perche le riba dipendono da l10n_it_fatturapa_out?

e non da l10n_it_fatturapa. Il punto è che nel codice non si fa riferimento a l10n_it_fatturapa_out per cui quella dipendenza andrebbe eliminata, a meno che non ci sia qualcosa di implicito che mi sfugge.

Fammi sapere se sei d'accordo!

Ho capito tutto la prima volta. Quello che sto dicendo è che questo modulo non dovrebbe dipendere da l10n_it_fatturapa per i suoi test. In altre parole, se i test hanno bisogno di imposte, dovrebbero crearle loro invece di cercare imposte create da altri moduli dai quali questo non dipende.

Uno deve poter creare un db, installare l10n_it_ricevute_bancarie, lanciare i test, e questi devono funzionare anche se l10n_it_fatturapa e i suoi dati demo non sono installati. Quindi aggiungere la country_id ai dati demo di l10n_it_fatturapa non è il modo corretto di fixare questi test.

Potrebbe essere il caso di farlo comunque e per altre ragioni (che al momento non ci sono in realtà a parte una questione "estetica", quello sono imposte che esistono in Italia), ma rimane il punto fondamentale: questi test devono continuare a funzionare anche quando l10n_it_fatturapa non è installato, quindi la soluzione è creare le imposte che servono dove servono nei test.

Se le imposte dovessero servire ad altri moduli, allora ok aggiungere country_id corretta ma vanno spostate nei dati demo di un altro modulo più "a monte" nell'albero delle dipendenze, tipo l10n_it_account.

@SirTakobi
Copy link
Contributor

@tafaRU @eLBati Vi torna l'errore che è presente in pre-commit, se vi torna mi spiegate cosa devo fare?

Riporto l'errore:

Please check-in the following files:
setup/l10n_it_ricevute_bancarie/odoo/addons/l10n_it_ricevute_bancarie

(da https://github.com/OCA/l10n-italy/actions/runs/3910908926/jobs/6683663263#step:8:14)

Dovresti avere in locale un file nel path indicato setup/l10n_it_ricevute_bancarie/odoo/addons/l10n_it_ricevute_bancarie: per risolvere devi aggiungerlo (git add) e includerlo nel commit contenente le modifiche fatte da pre-commit.

Dovresti avere il file perché è generato da pre-commit con l'hook setuptools-odoo-make-default, se così non fosse puoi generarlo manualmente seguendo https://github.com/acsone/setuptools-odoo#setuptools-odoo-make-default-helper-script

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.

2023-01-21 14:30:49,438 1 WARNING fm16 odoo.fields: Redundant default on account.move.is_riba_payment
messaggio se installo il modulo

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.

2023-01-23 12:23:30,878 3097887 WARNING fm16 odoo.api.create: The model odoo.addons.l10n_it_ricevute_bancarie.models.account is not overriding the create method in batch
devi usare @api.model_create_multi

@TonyMasciI TonyMasciI force-pushed the 16.0-mig-l10n_it_ricevute_bancarie branch 2 times, most recently from 56c8cb9 to 373a080 Compare January 27, 2023 14:34
@TonyMasciI TonyMasciI marked this pull request as ready for review January 27, 2023 16:40
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.

Il filtro riba_filter in views/account_view.xml va in conflitto con account_due_list,
suggerirei di mettere nelle dipendenze account_due_list ed estendere quello come sotto:

<record id="riba_filter" model="ir.ui.view">
    <field name="name">C/O filter</field>
    <field name="model">account.move.line</field>
    <field name="inherit_id" ref="account_due_list.view_payments_filter" />
    <field name="arch" type="xml">
        <xpath expr="//search" position="inside">
            <filter
                name="da_emettere"
                string="RIBA To Issue"
                domain="[('distinta_line_ids', '=', False)]"
            />
            <filter
                name="emesse"
                string="RIBA Issued"
                domain="[('distinta_line_ids', '!=', False)]"
            />
            <filter
                name="reconciled"
                string="Reconciled"
                domain="[('reconciled', '!=', False)]"
            />
            <filter
                name="to_reconcile"
                string="To Reconcile"
                domain="[('reconciled', '=', False)]"
            />
            <filter
                name="unsolved"
                string="Past Due"
                domain="[('unsolved_invoice_ids', '!=', False)]"
            />
            <filter
                name="sale_journal"
                string="Sale Journals"
                domain="[('journal_id.type', '=', 'sale')]"
            />
            <field name="account_id" />
            <field name="partner_id" />
            <field name="move_id" />
            <field name="date_maturity" />
            <group expand="0" string="Group By...">
                <filter
                    name="filter_customer"
                    string="Group By Customer"
                    context="{'group_by':'partner_id'}"
                />
            </group>
        </xpath>
    </field>
</record>

@TonyMasciI
Copy link
Contributor Author

Il filtro riba_filter in views/account_view.xml va in conflitto con account_due_list, suggerirei di mettere nelle dipendenze account_due_list ed estendere quello come sotto:

<record id="riba_filter" model="ir.ui.view">
    <field name="name">C/O filter</field>
    <field name="model">account.move.line</field>
    <field name="inherit_id" ref="account_due_list.view_payments_filter" />
    <field name="arch" type="xml">
        <xpath expr="//search" position="inside">
            <filter
                name="da_emettere"
                string="RIBA To Issue"
                domain="[('distinta_line_ids', '=', False)]"
            />
            <filter
                name="emesse"
                string="RIBA Issued"
                domain="[('distinta_line_ids', '!=', False)]"
            />
            <filter
                name="reconciled"
                string="Reconciled"
                domain="[('reconciled', '!=', False)]"
            />
            <filter
                name="to_reconcile"
                string="To Reconcile"
                domain="[('reconciled', '=', False)]"
            />
            <filter
                name="unsolved"
                string="Past Due"
                domain="[('unsolved_invoice_ids', '!=', False)]"
            />
            <filter
                name="sale_journal"
                string="Sale Journals"
                domain="[('journal_id.type', '=', 'sale')]"
            />
            <field name="account_id" />
            <field name="partner_id" />
            <field name="move_id" />
            <field name="date_maturity" />
            <group expand="0" string="Group By...">
                <filter
                    name="filter_customer"
                    string="Group By Customer"
                    context="{'group_by':'partner_id'}"
                />
            </group>
        </xpath>
    </field>
</record>

@andreampiovesana Ma che errore ti da? e in che momento?

@andreampiovesana
Copy link
Contributor

form di configurazione da correggere
image

@TonyMasciI TonyMasciI force-pushed the 16.0-mig-l10n_it_ricevute_bancarie branch from 2c1ec26 to d9f5510 Compare March 20, 2023 10:40
@primes2h
Copy link
Contributor

primes2h commented Mar 29, 2023

@TonyMasciI
Ti propongo saydigital#2 per completare il lavoro di revisione iniziato nella 12.0 qui (tafaRU#8) e proseguito nella 14.0 qui.

Il tutto anche in vista di una eventuale futura convergenza e integrazione tra i moduli della comunità e di Odoo SA.

In particolare:

P.S.: mancano gli script di migrazione.

@tafaRU
Copy link
Member

tafaRU commented Jun 13, 2023

#3352

@tafaRU
Copy link
Member

tafaRU commented Jun 20, 2023

#3396

@sergiocorato
Copy link
Contributor

/ocabot rebase

@OCA-git-bot
Copy link
Contributor

@sergiocorato The rebase process failed, because command git push --force saydigital tmp-pr-3131:16.0-mig-l10n_it_ricevute_bancarie failed with output:

remote: Permission to saydigital/l10n-italy.git denied to OCA-git-bot.
fatal: unable to access 'https://github.com/saydigital/l10n-italy/': The requested URL returned error: 403

@filipposaviori
Copy link

@TonyMasciI volevo testare la PR nel runbot per il merge, ma il runbot è nello stato "Pending" e non mi permette di testare. Come fare per risolvere?

Grazie,
FS

@TonyMasciI TonyMasciI force-pushed the 16.0-mig-l10n_it_ricevute_bancarie branch from d9f5510 to 3145f31 Compare June 22, 2023 15:06
@TonyMasciI
Copy link
Contributor Author

@TonyMasciI volevo testare la PR nel runbot per il merge, ma il runbot è nello stato "Pending" e non mi permette di testare. Come fare per risolvere?

Grazie, FS

Vediamo se ora ti permette di usare la build

@TonyMasciI TonyMasciI force-pushed the 16.0-mig-l10n_it_ricevute_bancarie branch from 3145f31 to 153796c Compare June 23, 2023 08:04
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!
Non ho ancora guardato il codice perché i problemi con i commit non sono risolti:

  • Nel commit di migrazione viene modificato il modulo l10n_it_declaration_of_intent:
    image
    è possibile rimuovere queste modifiche?

  • Grazie di aver modificato la storia dei commit 💪
    sono rimasti però ancora dei commit di oca-ci, tipo 154b7d9, è possibile rimuoverli?

EDIT: ho anche notato che il modulo è stato rinominato in l10n_it_riba ma manca lo script di migrazione, è possibile aggiungerlo? ho proposto una bozza in saydigital#5.

@filipposaviori
Copy link

Testando sono arrivato al punto di segnare la riba come Pagata o Insoluta.
Quando provo a cliccare su "segna come pagata" non accade nulla.
image

Quando provo a cliccare su "segna come insoluta" invece mi apre il menù corretto.
image

@TonyMasciI
Copy link
Contributor Author

Testando sono arrivato al punto di segnare la riba come Pagata o Insoluta. Quando provo a cliccare su "segna come pagata" non accade nulla. image

Quando provo a cliccare su "segna come insoluta" invece mi apre il menù corretto. image

@filipposaviori per favore mi puoi girare le configurazioni della riba e la fattura così replico in locale il problema? grazie

@scigghia
Copy link
Contributor

scigghia commented Sep 8, 2023

Perchè abbiamo optato per il cambio di nome del modulo ?

@TonyMasciI
Copy link
Contributor Author

@TonyMasciI Ti propongo saydigital#2 per completare il lavoro di revisione iniziato nella 12.0 qui (tafaRU#8) e proseguito nella 14.0 qui.

Il tutto anche in vista di una eventuale futura convergenza e integrazione tra i moduli della comunità e di Odoo SA.

In particolare:

* i nomi in italiano (viste, modelli ecc.) sono stati portati in inglese.

* i nomi in inglese che contenevano "false friends" sono stati corretti (non erano comprensibili per uno straniero)

* ho optato per l'utilizzo dell'acronimo RiBa anche per le stringhe in inglese. Le RiBa sono una peculiarità italiana e qualsiasi altro termine inglese non rispecchia appieno le sue caratteristiche. Credo sia la soluzione migliore per cercare di risolvere il problema (vedi [[14.0] Migration: l10n_it_ricevute_bancarie #2138](https://github.com/OCA/l10n-italy/pull/2138)).
  Allo stesso modo ho cambiato il nome del modulo in `l10n_it_riba` (vedi anche [[12.0][ADD] l10n_it_riba_sale_commission #3236 (comment)](https://github.com/OCA/l10n-italy/pull/3236#issuecomment-1482936024) e [[14.0][ADD] l10n_it_riba_sale_commission  #3230](https://github.com/OCA/l10n-italy/pull/3230))

P.S.: mancano gli script di migrazione.

@scigghia per questo motivo

@filipposaviori
Copy link

@TonyMasciI ecco la configurazione della Ri.Ba.:
image
e qua invece ti metto la configurazione della fattura di esempio creata:

image

@TonyMasciI
Copy link
Contributor Author

@filipposaviori perdonami ti posso chiedere anche come hai configurato queste record:

  • 888000 salvo buon fine
  • 888001 ricevute bancarie
  • 888002 spese di incasso
  • 121000 Account Receivable
  • 211100 Bills to receive
  • BE74126201326907

così da avere proprio il test completeaente uguale!

grazie!

@filipposaviori
Copy link

@TonyMasciI certo.
Allora i conti son fatti in questo modo:
888000 - image
888001 - image
888002 - image
121000 (già presente a sistema) - image
211100 (già presente a sistema) - image

La banca invece, anch'essa già presente e configurata, è configurata così:
image

Spero ti sia utile.

Grazie,
FS

@TonyMasciI
Copy link
Contributor Author

@filipposaviori Ho configurato come te ma mi blocco a livello funzionale, potremmo sentirci in settimana sul discord per parlarne a voce così da arrivare allo stesso tuo punto?

@filipposaviori
Copy link

@TonyMasciI si certo nessun problema

Not tested because `account` module cannot be migrated yet with OpenUpgrade
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

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

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.

mi spiace veder cambiare il nome al modulo (solo per una questione emotiva eh... 🥲 )
ma comunque LGTM 👍

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.

Non ho ancora guardato il codice perché questi punti di #3131 (review) non sono risolti:

  • Nel commit di migrazione viene modificato il modulo l10n_it_declaration_of_intent:
    image
    è possibile rimuovere queste modifiche?
  • Grazie di aver modificato la storia dei commit 💪
    sono rimasti però ancora dei commit di oca-ci, tipo 154b7d9, è possibile rimuoverli?

Inoltre:

  • i commit di traduzione consecutivi, se dello stesso autore, sono da unire.
    Ad esempio:
    image
  • mancano diversi commit presenti in 14.0, ad esempio 7ae8a5d.

Tutti questi punti vengono dalle istruzioni per la migrazione in https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-16.0.
Ad esempio:

Squash administrative commits (if any) with the previous commit for reducing commit noise. Check https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests#mergesquash-the-commits-generated-by-bots-or-weblate for details.

@francesco-ooops
Copy link
Contributor

@TonyMasciI puoi aggiungere #3411?

@filipposaviori
Copy link

Ciao a tutti, come mai la PR non è stata ancora mergiata?

Ci sono ulteriori controlli da eseguire?

@francesco-ooops
Copy link
Contributor

@TonyMasciI riesci ad applicare le richieste di @SirAionTech ?

@TonyMasciI
Copy link
Contributor Author

@TonyMasciI riesci ad applicare le richieste di @SirAionTech ?

Ciao Francesco appena ho del tempo da dedicargli certamente è in coda!

@SirAionTech
Copy link
Contributor

In accordo con @TonyMasciI (https://discordapp.com/channels/753902328494424064/753902328494424070/1156855058382737408) prendo in carico la PR, ne farò un'altra a breve

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.