[14.0] FIX l10n_it_fatturapa: missing dep on l10n_generic_coa

[TheMule71]

Currently referenced by demo data (accounts from l10n_generic_coa)

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

[eLBati]

Non si può costringere a installare dei conti generici (tantomeno se è solo per far funzionare i dati demo ), piuttosto l10n_it

[TheMule71]

Non si può nemmeno costringere ad installare l10n_it finché non si risolve la questione l10n_it_edi.

Poi siamo sicuri che già questo non installi l10n_generic_coa già lui? https://github.com/odoo/odoo/blob/14.0/addons/account/__init__.py#L48
non mi sembra di vedere lo specifico per IT. E se è già installato semplicemente installando account, non stiamo constringendo nessuno a installare nulla che non sia già installato.

La dipendenza comunque serve solo per i dati demo. Anche se account installasse automaticamente l10n_generic_coa comunque dovremmo dichiarare la dipendenza solo per risolvere l'id dei conti.

Alternativamente, dobbiamo riunciare ad usare conti pre-made nei dati demo e creare noi i nostri per i dati demo.

[SimoRubi]

@TheMule71 puoi descriviere qual'è il problema che hai incontrato?
Da https://github.com/odoo/odoo/blob/f997bc8f6e9d5138de6faed80e9774d359bfbe8d/addons/account/__init__.py#L48 pare che l10n_generic_coa si installi se non c'è già qualche paese selezionato da cui prendere la contabilità.

Infatti poi nei test viene referenziato in
https://github.com/odoo/odoo/blob/b6c37754532ae6a7cc1f68a345fde6258abdc893/addons/account/tests/common.py#L30
pur senza essere tra le dipendenze del modulo.

[TheMule71]

Mi ha riportato @Borruso che dà errore da quando abbiamo inserito i conti nei dati demo:
https://github.com/OCA/l10n-italy/blob/14.0/l10n_it_fatturapa/demo/account_invoice_fatturapa.xml#L20

Non ho preferenze particolari per una soluzione o per l'altra.

Nei test il problema è minore, sei in python e puoi controllare quale piano dei conti usare:
https://github.com/odoo/odoo/blob/b6c37754532ae6a7cc1f68a345fde6258abdc893/addons/account/tests/common.py#L27-L30

La soluzione ideale sarebbe quella di usare le imposte definite qui: https://github.com/odoo/odoo/blob/b6c37754532ae6a7cc1f68a345fde6258abdc893/addons/l10n_it/data/account_tax_template.xml#L22
o almeno i conti di l10n_it_chart_template_generic.

Tuttavia, non possiamo creare la dipendenza da l10n_it senza tirarci dietro l10n_it_edi.

Quindi, se non mettiamo tiriamo dientro l10n_it, ci troviamo che dobbiamo creare delle aliquote, e per quello ci servono dei conti, e se non ha caricato l10n_it, allora ha caricato l10n_generic_coa, quindi quei conti dobbiamo usare oppure ne creiamo altri noi ad hoc.

Oppure non creiamo imposte nei dati demo.

[SimoRubi]

Mi ha riportato @Borruso che dà errore da quando abbiamo inserito i conti nei dati demo

Dà errore facendo cosa? E qual'è l'errore?

[TheMule71]

Mi ha riportato @Borruso che dà errore da quando abbiamo inserito i conti nei dati demo

Dà errore facendo cosa? E qual'è l'errore?

I dettagli per riprodurlo non mi li ricordo, dovremmo chiedere direttamente a @Borruso .

Tuttavia, vista la natura del problema, con un minimo di immaginazione, mi sono inventato questa procedura:

• installi odoo base
• (da web) installi l10n_it
• rimuovi l10n_it_edi
• installi l10n_it_fatturapa

2021-06-07 12:02:55,757 7670 INFO odoo14-oca14 odoo.modules.loading: Module l10n_it_fatturapa: loading demo 
2021-06-07 12:02:55,757 7670 INFO odoo14-oca14 odoo.modules.loading: loading l10n_it_fatturapa/demo/account_invoice_fatturapa.xml 
2021-06-07 12:02:55,819 7670 ERROR odoo14-oca14 odoo.tools.convert.init: Could not eval([(5, 0, 0),                 (0,0, {                     'factor_percent': 100,                     'repartition_type': 'base',                 }),                 (0,0, {                     'factor_percent': 100,                     'repartition_type': 'tax',                     'account_id': ref('l10n_generic_coa.tax_received'),                 }),             ]) for invoice_repartition_line_ids in {} 
2021-06-07 12:02:55,820 7670 WARNING odoo14-oca14 odoo.modules.loading: Module l10n_it_fatturapa demo data failed to install, installed without demo data 
Traceback (most recent call last):
  File "/home/marco/src/odoo/14/odoo/odoo/odoo/tools/cache.py", line 85, in lookup
    r = d[key]
  File "/home/marco/src/odoo/14/odoo/odoo/odoo/tools/func.py", line 71, in wrapper
    return func(self, *args, **kwargs)
  File "/home/marco/src/odoo/14/odoo/odoo/odoo/tools/lru.py", line 34, in __getitem__
    a = self.d[obj]
KeyError: ('ir.model.data', <function IrModelData.xmlid_lookup at 0x7f9178550050>, 'l10n_generic_coa.tax_received')

Più sotto ovviamente c'è:

ValueError: External ID not found in the system: l10n_generic_coa.tax_received

Non so se è esattamente questo che ha trovato @Borruso però mi sembra una procedura assolutamente ragionevole che fallisce inaspettatamente.

I test travis non falliscono solo perché non ci tiriamo dietro l10n_it come dipendenza (potenzialmente di l10n_it_account direi), e questo per la questione l10n_it_edi (che mi pare ad un punto fermo).

Quello che succede: normalmente, odoo standard installa un CoA specifico per una nazione se questa fa parte di un elenco predeterminato nel modulo account standard (vd. sopra). In effetti installa automaticamente il modulo l10n_xx. IT non fa parte di questo elenco, ergo, account installa l10n_generic_coa. Questo fa si che l'installazione travis non fallisca (perché l10n_generic_coa viene installato prima di l10n_it_fatturapa sempre e comunque).

Questo per travis.

Se a mano però - e non c'è nulla di sbagliato logicamente - installi l10n_it, rimuovi l10n_it_edi, e poi provi ad installare l10n_it_fatturapa, questa si tira dietro account, il quale, trovando già il CoA italiano installato, non installa più l10n_generic_coa.

L'installazione fallisce perché i dati demo di l10n_fatturapa fanno riferimento a dei conti presi da l10n_generic_coa.

In pratica, se non dichiariamo nessuna dipendenza, come adesso, funziona travis ma qualsiasi procedura che installi il CoA italiano prima di account (by-passando l'installazione di l10n_generic_coa) fallisce.

Se dichiariamo una dipendenza da l10n_it e usiamo per i dati demo conti dal CoA italiano, dobbiamo convincere travis a rimuovere l10n_it_edi dopo l'installazione dei l10n_it. Dico travis ma potrebbe benissimo essere l'init di l10n_it_account a farlo, rimane che va rimosso automaticamente.

Alla fine, aggiungere una riga di dipendenza da l10n_generic_coa, che non altera il comportamento in tutti i casi in cui account viene installato prima di l10n_it (e quindi installa comunque l10n_generic_coa), e risolve il caso in cui l'ordine sia contrario (prima l10n_it, poi account), sembrava la cosa meno invasiva.

In pratica:

1. travis: NON installa l10n_it, forza l'installazione di l10n_generic_coa, aggiungendo la dipendenza non alteri il risultato finale;

2. installazione con prima account, poi l10n_it: ti trovi installati entrambi i CoA, l'installazione di l10n_it_fatturapa funziona, aggiugendo la dipendenza non cambia il risultato;

3. installazione con prima l10n_it, poi account: ti trovi installato solo il CoA italiano, l'installazione di l10n_it_fatturapa fallisce;

dopo questa PR:

3. installazione con prima l10n_it, poi account: l'installazione di l10n_fatturapa funziona, ti trovi installati entrambi i CoA, come nelle scenario 2.

[TheMule71]

A proposito, ribadisco che per me una soluzione vale l'altra. Se decidiamo di rimuovere le imposte dai dati demo (il che ci forza a crearle da python a livello di test), va bene anche quello.

Come ho scritto sopra, da python si ha certo più libertà nel verificare quale CoA è installato e andare a beccarsi i conti relativi. Genericamente, non mi piace l'idea di test che cambiano il comportamento in base alla presenza o meno di altri moduli, ma nello specifico servono un paio di conti dal CoA per la creazione delle imposte.

[SimoRubi]

ok grazie ora mi è un più chiaro qual'è il problema :)

Come anticipato da @eLBati in #2311 (review) non credo però che questa sia la soluzione giusta per i motivi indicati nel suo commento.

Per risolvere penso sia più corretta una di:

• Creare le imposte nella parte comune dei test automatici, come fatto in https://github.com/odoo/odoo/blob/b6c37754532ae6a7cc1f68a345fde6258abdc893/addons/account/tests/common.py#L285
• Ricercare dei conti opportuni per le imposte direttamente durante la loro creazione, vedi ad esempio come viene utilizzato obj.search in https://github.com/odoo/odoo/blob/600a1da6c327064e83feed122092c5d0ce1e7894/addons/l10n_ar/demo/account_supplier_invoice_demo.xml#L146

Mettere l10n_it come dipendenza penso abbia delle controindicazioni perché se ne parlò in #1980 (comment) e alla fine si decise di non aggiungere la dipendenza ma non ho capito del tutto le motivazioni, penso che @eLBati e @sergiocorato ne sappiano qualcosa di più

[TheMule71]

@Borruso per cortesia puoi controllare se con queste ultime modifiche anche da te tutto funziona?

[TheMule71]

Come non detto. Travis fallisce. Curiosamente, i test non falliscono da me con un db nuovo. Mah, domani ci riguardo.

[eLBati]

Poi siamo sicuri che già questo non installi l10n_generic_coa già lui? https://github.com/odoo/odoo/blob/14.0/addons/account/__init__.py#L48

Sì, siamo sicuri. E sarebbe un bel problema se lo facesse.

l10n_generic_coa viene installato solo se env.company.country_id.code e se non esiste alcun modulo chiamato 'l10n_' + country_code.lower()

[TheMule71]

Poi siamo sicuri che già questo non installi l10n_generic_coa già lui? https://github.com/odoo/odoo/blob/14.0/addons/account/__init__.py#L48

Sì, siamo sicuri. E sarebbe un bel problema se lo facesse.

l10n_generic_coa viene installato solo se env.company.country_id.code e se non esiste alcun modulo chiamato 'l10n_' + country_code.lower()

Sicuri o sicuri sicuri sicuri? :)
https://travis-ci.com/github/OCA/l10n-italy/jobs/512554116#L1495

Bisognerebbe convincere travis allora. In realtà, env.company.country_id.code è popolato solo se installi la base, configuri, poi installi gli altri moduli (in particolare, account).

In altre parole, solo se fai una procedura specifica per evitare di installare account prima di aver specificato la country. Se per caso installi account (o un modulo che se lo tira dietro) prima di aver specificato la country ti trovi l10n_generic_coa installato. Quello che non capisco è perché sarebbe un problema, i CoA si switchano se non hai fatture in ballo. Anch'io preferirei tirare dentro l10n_it e il CoA italiano, ma quella è un'altro discorso.

E comunque - in generale - ha senso fare tutti i test con travis con l10n_generic_coa? Per dire, l10n_it_account è risultato verde per il merge, con l10n_generic_coa installato, già a marzo:
https://travis-ci.com/github/OCA/l10n-italy/jobs/490381574#L1047

Se la nostra policy è "l10n_generic_coa = brutto", ok, mi va anche bene, poi noi non dovremmo fare tutti i test di tutti i nostri moduli con quello.

[eLBati]

LGTM

[SimoRubi]

Per riprodurre l'errore, far partire Odoo con i parametri:

• -d <nome_db>, nota che il DB non deve già esistere: verrà creato da Odoo
• --init l10n_it_fatturapa

Risultato (prima di questa PR):

ValueError: <class 'ValueError'>: "External ID not found in the system: l10n_generic_coa.tax_received" while evaluating
"[(5, 0, 0),                 (0,0, {                     'factor_percent': 100,                     'repartition_type': 'base',                 }),                 (0,0, {                     'factor_percent': 100,                     'repartition_type': 'tax',                     'account_id': ref('l10n_generic_coa.tax_received'),                 }),             ]"

Con le modifiche fatte in questa PR ottengo invece un altro errore:

ValueError: <class 'ValueError'>: "External ID not found in the system: account.account" while evaluating
"[(5, 0, 0),                 (0,0, {                     'factor_percent': 100,                     'repartition_type': 'base',                 }),                 (0,0, {                     'factor_percent': 100,                     'repartition_type': 'tax',                     'account_id': ref('l10n_it.2601', raise_if_not_found=False) or ref('l10n_generic_coa.tax_payable', raise_if_not_found=False) or ref('account.account').search([('user_type_id', '=', 'account.data_account_type_current_liabilities')], limit=1),                 }),             ]"

che ho corretto in TheMule71#20, puoi controllare se ti torna?

[SimoRubi]

Se riesci a togliere il commit di merge (non so se se ne andrà con il merge in 14.0), poi per me è ok