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

[ADD] new module l10n_it_fatturapa_auto_downpayment #4230

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

TheMule71
Copy link
Contributor

@TheMule71 TheMule71 commented Jun 25, 2024

Se la fattura è un acconto, setta automaticamente TD02 come tipo documento fiscale.
Tale comportamento è simile a l10n_it_edi

Rimpiazza #4222

Fixes: #4221

@francesco-ooops
Copy link
Contributor

Non so dare una valutazione su questo caso specifico, ma credo che un obiettivo di oca-italy dovrebbe essere cercare di evitare la proliferazione di nuovi moduli per la contabilità... che ne pensate?

@TheMule71 TheMule71 force-pushed the 16.0-new-l10n_it_fatturapa_auto_downpayment branch from 0e68b98 to 9f96f96 Compare June 25, 2024 13:09
@TheMule71
Copy link
Contributor Author

Non so dare una valutazione su questo caso specifico, ma credo che un obiettivo di oca-italy dovrebbe essere cercare di evitare la proliferazione di nuovi moduli per la contabilità... che ne pensate?

#4222 (comment)

Ho fatto sia una PR come modifica a modulo esistente e sia una PR come modulo nuovo. Il motivo per avere un nuovo modulo è solo per evitare di introdurre la dipendenza dal modulo sale su l10n_it_fiscal_document_type.

@TheMule71 TheMule71 force-pushed the 16.0-new-l10n_it_fatturapa_auto_downpayment branch 2 times, most recently from fd91b78 to 5c5592b Compare June 25, 2024 14:02
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.

Functional test: OK

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 mettere [ADD] invece di [IMP] nel messaggio del commit?

[ADD] for adding new modules

(da https://www.odoo.com/documentation/16.0/contributing/development/git_guidelines.html#tag-and-module-name)

Grande che hai messo un test 😄 fammi sapere cosa ne pensi delle note qui sotto, le uniche cose bloccanti per me sono nell'override

def _get_document_fiscal_type(
self, move_type=None, partner=None, fiscal_position=None, journal=None
):
self.ensure_one()
Copy link
Contributor

Choose a reason for hiding this comment

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

Il metodo in super

non impone questo vincolo, quindi in teoria può gestire un recordset.
È possibile spostare questo vincolo e il resto del codice che ne ha bisogno dentro un if len(self) == 1 o qualcosa di analogo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

L'unico punto in cui è chiamata è la _compute_set_document_fiscal_type(), all'interno di un loop fattura per fattura.

Piu di quello però, il valore di ritorno è un alista di possibili TDxx validi per una sola fattura. La funziona ignora self di fatto (potrebbe essere @api.model), ma gli vengono passati un solo partner, una sola posizione fiscale, un solo journal, relativi ad una sola fattura.

In pratica la funzione originale è usata come se fosse self.ensure_one() e scritta come se fosse @api.model . Non credo possa essere chiamata su un recordset, visto che per avere un risultato sensato dovresti assicurarti a priori che sono fatture con lo stesso partner, stessa fpos, stesso journal.

Io ho aggiunto self.ensure_one() perché effettivamente uso self.

Copy link
Contributor

Choose a reason for hiding this comment

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

È vero che il metodo in super si comporta come un api.model e il risultato dipende solo dai suoi parametri, però credo che questo sia un motivo in più perché l'override aggiunto in questa PR non debba aggiungere vincoli a self.

Attualmente potrebbero esserci altri moduli che chiamano _get_document_fiscal_type su un recordset e funzionano; dopo l'installazione di questo modulo invece verrebbe sollevato un errore, credo sia meglio evitarlo e rimanere compatibili con il codice esistente.
Non è detto che lo facciano perché si aspettano un risultato in base a self, ma semplicemente perché è possibile farlo (come è possibile farlo per i metodi api.model).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

La _get_document_fiscal_type viene chiamata da una compute

def _compute_set_document_fiscal_type(self):
for invoice in self:
# Edit only draft invoices
# or invoices that do not have a document type
if invoice.state != "draft" and invoice.fiscal_document_type_id:
continue
# If there is already a fitting document type, do not change it
accepted_document_type_ids = invoice._get_document_fiscal_type(
invoice.move_type,
invoice.partner_id,
invoice.fiscal_position_id,
invoice.journal_id,
)
if invoice.fiscal_document_type_id.id in accepted_document_type_ids:
continue
document_type = False
if accepted_document_type_ids:
document_type = accepted_document_type_ids[0]
invoice.fiscal_document_type_id = document_type

fattura per fattura.

Per qualche motivo, chi l'ha scritta ha deciso di passare uno ad uno i campi usati per decidere, data una
fattura, l'elenco dei tipi documento disponibili per la scelta.

Si tratta di scelta discutibile, un metodo che li legga da self avrebbe avuto molto più senso. Ma rimane che la logica della funzione - sia per come è definita, sia per come è usata - è quella di fornire i possibili valore del document type di una singola fattura.

La scelta ha fatto si che sia tecnicamente possibile chiamarla su in recordset, ma logicamente non ha senso, visto che comunque o gestisci il fatto che fatture diverse hatto tutte gli stessi valori per i campi in questione, oppure semplicemente la chiami coi valori fattura per fattura, oppure ottieni una risposta che non si puó applicare ad un recordset, ma solo alla fattura di cui hai passato i valori.

In altre parole, il metodo prende comunue i dati specifici singoli e non supporta in ogni caso un recordset, sul quale funziona per caso e non fornisce la risposta giusta. Visto che stiamo parlando di un ipotetico modulo la usasse in quel modo, funzionerebbe solo per caso e non farebbe quello che dovrebbe comunque, visto che non lavora sul recordset ma sui singoli valori. Sarebbe comunque un bug, che non tira eccezione solo grazie a un secondo bug (o scelta di design molto discutibile).

Per altro, non c'è soluzione... se _get_document_fiscal_type() fornisce l'elenco dei possibili valore del document type di una specifica fattura, se la si vuole estendere a criteri che vanno oltre agli argomenti già previsti, bisogna accedere a self. E a quel punto, che ci sia ensure_one in testa o dia errore dopo cambia poco. La posso anche togliere, ma l'eccezione la tirerebbe dopo comunque.

Posso anche riscrivere il metodo per loopare sui record, controllare che siano tutte fatture di downpayment, e restituire un elenco di valori, per poi tirare un'eccezione se le fatture non sono omogenee (o tutte acconti o tutte no). Ma non cambia il fatto che comunque l'ipotetico modulo che chiama il metodo su recordset non omogenei si troverebbe un'eccezione che ieri non c'era.

E poi se parliamo di ipotetici moduli là fuori, ogni volta che si fixa un bug si va a rischio di "rompere" un qualche modulo che implementi una fix o un workaround del bug. La cosa non ci deve impedire di fixare bug, e nel modo migliore.

@TheMule71
Copy link
Contributor Author

TheMule71 commented Jul 19, 2024

Grazie della PR! Potresti mettere [ADD] invece di [IMP] nel messaggio del commit?

[ADD] for adding new modules

Corretto. Grazie per la review!

@TheMule71 TheMule71 force-pushed the 16.0-new-l10n_it_fatturapa_auto_downpayment branch 2 times, most recently from b79d2be to 8cbb8df Compare July 19, 2024 08:27
@sergiocorato
Copy link
Contributor

sergiocorato commented Aug 28, 2024

@TheMule71 ho aggiunto la PR che avevo proposto in precedenza sulla 14.0, erede di quella proposta sulla 12.0, che hanno un approccio diverso al problema.

@matteoopenf
Copy link
Contributor

ci sono novità in questo modulo?

@TheMule71 TheMule71 force-pushed the 16.0-new-l10n_it_fatturapa_auto_downpayment branch from 8cbb8df to 02b2b34 Compare October 21, 2024 13:22
@TheMule71 TheMule71 requested a review from SirAionTech October 21, 2024 13:23
@TheMule71
Copy link
Contributor Author

ci sono novità in questo modulo?

Dovrebbe essere tutto a posto.

@francesco-ooops francesco-ooops changed the title [IMP] new module l10n_it_fatturapa_auto_downpayment [ADD] new module l10n_it_fatturapa_auto_downpayment Oct 21, 2024
def _get_document_fiscal_type(
self, move_type=None, partner=None, fiscal_position=None, journal=None
):
self.ensure_one()
Copy link
Contributor

Choose a reason for hiding this comment

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

È vero che il metodo in super si comporta come un api.model e il risultato dipende solo dai suoi parametri, però credo che questo sia un motivo in più perché l'override aggiunto in questa PR non debba aggiungere vincoli a self.

Attualmente potrebbero esserci altri moduli che chiamano _get_document_fiscal_type su un recordset e funzionano; dopo l'installazione di questo modulo invece verrebbe sollevato un errore, credo sia meglio evitarlo e rimanere compatibili con il codice esistente.
Non è detto che lo facciano perché si aspettano un risultato in base a self, ma semplicemente perché è possibile farlo (come è possibile farlo per i metodi api.model).

Comment on lines 47 to 50
Installation
============


Copy link
Contributor

Choose a reason for hiding this comment

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

Puoi eliminare INSTALL.md? Altrimenti viene creata questa sezione vuota.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fatto.

Comment on lines 3 to 4
Alla creazione di una fattura di downpayment, viene settato
TD02 come fiscal document type.
Copy link
Contributor

Choose a reason for hiding this comment

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

Visto che non c'è nulla da configurare, penso sia meglio rimuovere questo file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fatto.

Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Feb 23, 2025
@TheMule71 TheMule71 force-pushed the 16.0-new-l10n_it_fatturapa_auto_downpayment branch from 02b2b34 to 6c0ce83 Compare February 24, 2025 19:21
@TheMule71
Copy link
Contributor Author

@TheMule71 ho aggiunto la PR che avevo proposto in precedenza sulla 14.0, erede di quella proposta sulla 12.0, che hanno un approccio diverso al problema.

Visto, ma passi dalla creazione fattura via sale order. Io per es. ho implementato un modo diverso di creare le fatture di downpayment per un cliente, che vedeva proprio una limitazione nel dover passare dal SO. Gli acconti che preparano sono slegati da un singolo SO e fatture di SO diversi possono andare a scontare dalla stessa fattura di acconto.

È compatibile con il resto di Odoo perche _is_downpayment() fa la cosa giusta e riconosce sia le fatture di acconto standard Odoo sia quelle create direttamente senza passare da SO.

Abbiamo già una funzione che prende:

                invoice.move_type,
                invoice.partner_id,
                invoice.fiscal_position_id,
                invoice.journal_id,

e propone un elenco di possibili TD* da presentare all'utente. Il che è un po' diverso da impostare un default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
16.0 new module stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TD02 come default per il tipo documento nelle fatture di acconto
6 participants