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

[12.0][MIG] l10n-italy_l10n_it_intrastat #1314

Merged
merged 7 commits into from
Nov 21, 2019
Merged

[12.0][MIG] l10n-italy_l10n_it_intrastat #1314

merged 7 commits into from
Nov 21, 2019

Conversation

glauco70
Copy link

Descrizione del problema o della funzionalità:
Porting alla 12.0 gestione intrastat : moduli l10n_it_intrastat e l10n_it_intrastat_statement

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

@labaggio labaggio changed the title 12.0 mig l10n italy l10n it intrastat [12.0][MIG] l10n-italy_l10n_it_intrastat Jun 26, 2019
Copy link
Member

@SimoRubi SimoRubi left a comment

Choose a reason for hiding this comment

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

Provando a creare una dichiarazione intrastat, quando cambio il periodo ottengo:

Errore:
[...]
l10n_it_intrastat_statement/models/intrastat.py", line 345, in onchange_period
    statement.date_start = period_date_start
[...]
TypeError: 2019-03-01 00:00:00 (field account.intrastat.statement.date_start) must be string or date, not datetime.

@glauco70
Copy link
Author

Provando a creare una dichiarazione intrastat, quando cambio il periodo ottengo:

Errore:
[...]
l10n_it_intrastat_statement/models/intrastat.py", line 345, in onchange_period
    statement.date_start = period_date_start
[...]
TypeError: 2019-03-01 00:00:00 (field account.intrastat.statement.date_start) must be string or date, not datetime.

sistemato

Copy link
Member

@SimoRubi SimoRubi left a comment

Choose a reason for hiding this comment

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

Ho creato una dichiarazione intrastat, provando a fare "Export file" e poi (nel wizard che appare) "Export file invio" ottengo:

l10n_it_intrastat_statement/models/intrastat.py", line 428, in _get_file_name
    date_obj = datetime.strptime(self.date, '%Y-%m-%d')
TypeError: strptime() argument 1 must be str, not datetime.date

@glauco70
Copy link
Author

Ho creato una dichiarazione intrastat, provando a fare "Export file" e poi (nel wizard che appare) "Export file invio" ottengo:

l10n_it_intrastat_statement/models/intrastat.py", line 428, in _get_file_name
    date_obj = datetime.strptime(self.date, '%Y-%m-%d')
TypeError: strptime() argument 1 must be str, not datetime.date

sistemto la gestione date

@SimoRubi
Copy link
Member

@glauco70 per proseguire con le review ho bisogno di sapere da quali moduli parte questo porting.

Sono un pò confuso perché so che esistono dei report riferiti ai moduli intrastat (o almeno ci sono nei moduli v10 in https://github.com/openerp-italia/data-modules/tree/10.0) e senza i report, i moduli intrastat hanno poca utilità.

Vedo che i dati del modulo https://github.com/openerp-italia/data-modules/tree/10.0/l10n_it_intrastat_data sono stati inclusi in l10n_it_intrastat ma mancano invece i report del modulo https://github.com/openerp-italia/data-modules/tree/10.0/l10n_it_report_intrastat.

Mi potresti riassumere la situazione di questi moduli rispetto a quelli v10?

@labaggio
Copy link
Contributor

@glauco70 per proseguire con le review ho bisogno di sapere da quali moduli parte questo porting.

Sono un pò confuso perché so che esistono dei report riferiti ai moduli intrastat (o almeno ci sono nei moduli v10 in https://github.com/openerp-italia/data-modules/tree/10.0) e senza i report, i moduli intrastat hanno poca utilità.

Vedo che i dati del modulo https://github.com/openerp-italia/data-modules/tree/10.0/l10n_it_intrastat_data sono stati inclusi in l10n_it_intrastat ma mancano invece i report del modulo https://github.com/openerp-italia/data-modules/tree/10.0/l10n_it_report_intrastat.

Mi potresti riassumere la situazione di questi moduli rispetto a quelli v10?

Ciao @SimoRubi, questo porting nasce dal porting sulla 11 fatte da me.
Quello sulla 11 parte, come hai indicato tu, da qui https://github.com/openerp-italia/.
Lì tutti i moduli erano sparsi in due repo diversi: modules e data-modules
Aveva poco senso avere due repo e 5 moduli, quindi tempo fa con @alessandrocamilli (se non ricorda male) aveva ridistribuito tutto quello che serve nei unici due moduli che trovi in questa PR.
Se vuoi vedere dove sono i vari pezzi, puoi guardare la PR in cui ho aggiunto il modulo: #1283. Ha due commit, il primo contiene la versione originale, suddivisa nei 5 moduli, il secondo come sono state redistribuite le informazioni.

@SimoRubi
Copy link
Member

Grazie @labaggio del chiarimento, concordo che avete fatto bene a riunire tutto in 2 moduli.
Nel secondo commit però vedo che vengono eliminati i file dei report, ad esempio:
image

Mi sapresti dire dove posso trovare ad esempio il report che ha id report_intrastat_mod1_bis (qui sotto)?
image

@labaggio
Copy link
Contributor

Grazie @labaggio del chiarimento, concordo che avete fatto bene a riunire tutto in 2 moduli.
Nel secondo commit però vedo che vengono eliminati i file dei report, ad esempio:
image

Mi sapresti dire dove posso trovare ad esempio il report che ha id report_intrastat_mod1_bis (qui sotto)?
image

Ops.. è stata una bella svista.. grazie della segnalazione. sistemiamo la 11 e subito dopo questa.

@tafaRU
Copy link
Member

tafaRU commented Jul 1, 2019

@labaggio ci sono novità relative a #1314 (comment) ?

@labaggio
Copy link
Contributor

labaggio commented Jul 1, 2019

@labaggio ci sono novità relative a #1314 (comment) ?

Abbiamo prima fatto le modifiche sulla 11. In giornata sarà aggiornata anche questa PR

@glauco70
Copy link
Author

glauco70 commented Jul 1, 2019

@labaggio ci sono novità relative a #1314 (comment) ?

aggiornata la PR con i report

@tafaRU
Copy link
Member

tafaRU commented Jul 1, 2019

aggiornata la PR con i report

ok grazie, come mai un commit di merge?

@glauco70
Copy link
Author

glauco70 commented Jul 1, 2019

aggiornata la PR con i report

ok grazie, come mai un commit di merge?

era un mio errore, sistemato

Copy link
Member

@SimoRubi SimoRubi left a comment

Choose a reason for hiding this comment

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

In una dichiarazione intrastat, ho fatto i passi:

  • Imposto Period = 6
  • Imposto Period Type = Trimestre

Ottengo:

l10n_it_intrastat_statement/models/intrastat.py", line 345, in onchange_period
    statement.date_start = fields.Date.to_date(period_date_start)
UnboundLocalError: local variable 'period_date_start' referenced before assignment

L'errore andrebbe gestito.

l10n_it_intrastat_statement/models/intrastat.py Outdated Show resolved Hide resolved
Copy link
Member

@tafaRU tafaRU left a comment

Choose a reason for hiding this comment

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

Ciao, grazie della PR!
Ho fatto solo code review (sul diff globale), qui sotto qualche correzione/domanda.

l10n_it_intrastat/models/__init__.py Outdated Show resolved Hide resolved
l10n_it_intrastat/models/config.py Outdated Show resolved Hide resolved
l10n_it_intrastat/models/config.py Show resolved Hide resolved
l10n_it_intrastat/models/config.py Show resolved Hide resolved
l10n_it_intrastat/models/config.py Outdated Show resolved Hide resolved
l10n_it_intrastat/models/account.py Outdated Show resolved Hide resolved
l10n_it_intrastat_statement/models/intrastat.py Outdated Show resolved Hide resolved
l10n_it_intrastat/demo/product_demo.xml Outdated Show resolved Hide resolved
l10n_it_intrastat_statement/report/intrastat_mod1_bis.xml Outdated Show resolved Hide resolved
l10n_it_intrastat_statement/security/rules.xml Outdated Show resolved Hide resolved
Copy link
Member

@SimoRubi SimoRubi left a comment

Choose a reason for hiding this comment

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

Controlla gli errori pylint di Travis

@tafaRU
Copy link
Member

tafaRU commented Jul 10, 2019

@glauco70 grazie, mi pare ci siano ancora dei miei commenti non risolti. Mi puoi avvisare quando hai terminato?

@glauco70
Copy link
Author

Controlla gli errori pylint di Travis

sistemati

Copy link
Member

@SimoRubi SimoRubi left a comment

Choose a reason for hiding this comment

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

Le date di start/stop ora cambiano correttamente con l'onchange, ma al salvataggio vengono resettate (vedi video https://drive.google.com/file/d/1K85rJpcTRypfu4Kchang2yiYDNQNz0Ym).

Inoltre, potresti dare un nome alle dichiarazioni?
Ti suggerisco di implementare il metodo account.intrastat.statement.name_get (per come usarlo vedi https://github.com/odoo/odoo/blob/94a2c54fb50e39e3343f3183ead2121940ed0878/odoo/models.py#L1575), altrimenti quando si apre una dichiarazione, in alto c'è ad esempio 'account.intrastat.statement,2' che è il comportamento standard ma ha poco significato per l'utente.

@glauco70
Copy link
Author

@glauco70 grazie, mi pare ci siano ancora dei miei commenti non risolti. Mi puoi avvisare quando hai terminato?

@tafaRU terminati !

Copy link
Member

@SimoRubi SimoRubi left a comment

Choose a reason for hiding this comment

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

Grazie delle modifiche! Ora la review funzionale mi sembra ok, ho iniziato la review del codice

l10n_it_intrastat_statement/models/intrastat.py Outdated Show resolved Hide resolved
l10n_it_intrastat_statement/models/intrastat.py Outdated Show resolved Hide resolved
l10n_it_intrastat_statement/models/intrastat.py Outdated Show resolved Hide resolved
l10n_it_intrastat_statement/models/intrastat.py Outdated Show resolved Hide resolved
l10n_it_intrastat_statement/models/intrastat.py Outdated Show resolved Hide resolved
l10n_it_intrastat_statement/models/intrastat.py Outdated Show resolved Hide resolved
l10n_it_intrastat_statement/models/intrastat.py Outdated Show resolved Hide resolved
@labaggio
Copy link
Contributor

@SimoRubi grazie mille per l'aiuto.
Nelle prossime settimane non riusciremo a prendere in carico queste altre modifiche.
Ti è possibile darci una mano facendo un PR a noi? Grazie

@SimoRubi
Copy link
Member

@SimoRubi grazie mille per l'aiuto.
Nelle prossime settimane non riusciremo a prendere in carico queste altre modifiche.
Ti è possibile darci una mano facendo un PR a noi? Grazie

ok

@SimoRubi
Copy link
Member

@SimoRubi grazie mille per l'aiuto.
Nelle prossime settimane non riusciremo a prendere in carico queste altre modifiche.
Ti è possibile darci una mano facendo un PR a noi? Grazie

Ho fatto https://github.com/linkitspa/l10n-italy/pull/8

@glauco70
Copy link
Author

@SimoRubi grazie mille per l'aiuto.
Nelle prossime settimane non riusciremo a prendere in carico queste altre modifiche.
Ti è possibile darci una mano facendo un PR a noi? Grazie

Ho fatto linkitspa#8

Merge effettuato !

@glauco70
Copy link
Author

Ho fatto linkitspa#8

@SimoRubi ho sistemato alcune segnalazioni di flake8

@SimoRubi
Copy link
Member

@glauco70 ho fatto https://github.com/linkitspa/l10n-italy/pull/9, perlopiù refactoring.
Prima c'era un monte di codice duplicato, ora ce n'è mezzo monte.. Dopo il merge controllo ancora una volta cosa si può fare ma sono ottimista

@glauco70
Copy link
Author

glauco70 commented Nov 4, 2019

@glauco70
Mi pare che manchi il porting del README dalla 11.0, era stato rivisto anche quello.

Sistemato il readme

@glauco70
Copy link
Author

glauco70 commented Nov 4, 2019

Grazie @glauco70.
Potresti controllare gli errori sollevati da test_pylint?
https://travis-ci.org/OCA/l10n-italy/jobs/605604019#L505-L508

Sistemati

Copy link
Contributor

@primes2h primes2h left a comment

Choose a reason for hiding this comment

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

@glauco70
Ho effettuato un confronto pari pari con la 11.0, ci sono ancora alcune correzioni per allineare le versioni, le trovi a seguire.

Ho anche notato due cose:

  1. nel modulo l10n_it_intrastat_statement della 12.0 c'è un README che non era presente nella 11.0. Sembra essere la vecchia versione del README di l10n_it_intrastat(quello prima della revisione per intendersi). Ho l'impressione che sia stato inserito per errore, puoi verificare per cortesia?

  2. Per poter utilizzare i moduli Intrastat nella 12.0 è necessario abilitare le funzionalità contabili complete, altrimenti le voci sono nascoste. Nella 11.0 non è così, le voci sono visibili.
    Ci sono motivazioni particolari per questa differenza?
    Un utente potrebbe voler gestire le dichiarazioni Intrastat usando solo il modulo fatturazione e magazzino.

l10n_it_intrastat/views/intrastat.xml Outdated Show resolved Hide resolved
l10n_it_intrastat/__manifest__.py Show resolved Hide resolved
l10n_it_intrastat/models/intrastat.py Outdated Show resolved Hide resolved
l10n_it_intrastat/models/intrastat.py Outdated Show resolved Hide resolved
l10n_it_intrastat/models/intrastat.py Outdated Show resolved Hide resolved
l10n_it_intrastat/models/account.py Outdated Show resolved Hide resolved
l10n_it_intrastat/readme/CONFIGURE.rst Outdated Show resolved Hide resolved
l10n_it_intrastat/readme/CONFIGURE.rst Outdated Show resolved Hide resolved
l10n_it_intrastat/views/account.xml Outdated Show resolved Hide resolved
l10n_it_intrastat_statement/__manifest__.py Show resolved Hide resolved
@glauco70
Copy link
Author

glauco70 commented Nov 7, 2019

  1. nel modulo l10n_it_intrastat_statement della 12.0 c'è un README che non era presente nella 11.0. Sembra essere la vecchia versione del README di l10n_it_intrastat(quello prima della revisione per intendersi). Ho l'impressione che sia stato inserito per errore, puoi verificare per cortesia?

@primes2h
Si è vero, lo sistemo.
Riguardando il README del modulo l10n_it_intrastat secondo me ci sarebbe da spostare la parte "Usage" al README del modulo l10n_it_intrastat_statement, questa funzionalità si ha solo installano questo modulo; oppure secondo te è comunque corretto lasciare tutto nel README del modulo l10n_it_intrastat ?

@primes2h
Copy link
Contributor

primes2h commented Nov 8, 2019

@primes2h
Si è vero, lo sistemo.
Riguardando il README del modulo l10n_it_intrastat secondo me ci sarebbe da spostare la parte "Usage" al README del modulo l10n_it_intrastat_statement, questa funzionalità si ha solo installano questo modulo; oppure secondo te è comunque corretto lasciare tutto nel README del modulo l10n_it_intrastat ?

@glauco70
In generale è sempre meglio tenere separate le documentazioni dei rispettivi moduli per non creare confusione all'utente.
Considera che l10n_it_intrastat potrebbe essere installato anche senza l10n_it_intrastat_statement.

Oltre a USAGE ci sarebbe da rivedere anche DESCRIPTION.

Eventualmente potresti anche aggiungere una nota in l10n_it_intrastatindicando che per la creazione/generazione delle dichiarazioni è necessario installare l10n_it_intrastat_statement.

@glauco70
Copy link
Author

@glauco70
Ho effettuato un confronto pari pari con la 11.0, ci sono ancora alcune correzioni per allineare le versioni, le trovi a seguire.

Sistemate tutte le segnalazioni

Ho anche notato due cose:

  1. nel modulo l10n_it_intrastat_statement della 12.0 c'è un README che non era presente nella 11.0. Sembra essere la vecchia versione del README di l10n_it_intrastat(quello prima della revisione per intendersi). Ho l'impressione che sia stato inserito per errore, puoi verificare per cortesia?

Sistemati i README dei due moduli (e di conseguenza USAGE e DESCRIPTION), spostata la parte relativa al modulo l10n_it_intrastat_statement in quel modulo.

  1. Per poter utilizzare i moduli Intrastat nella 12.0 è necessario abilitare le funzionalità contabili complete, altrimenti le voci sono nascoste. Nella 11.0 non è così, le voci sono visibili.
    Ci sono motivazioni particolari per questa differenza?
    Un utente potrebbe voler gestire le dichiarazioni Intrastat usando solo il modulo fatturazione e magazzino.

Questi controlli c'erano già nel modulo 10.0 che @labaggio ha migrato alla 11.0 e che abbiamo portato alla 12.0; nel README è specificato che vanno abilitate le funzionalità contabili complete.

@primes2h
Copy link
Contributor

  1. Per poter utilizzare i moduli Intrastat nella 12.0 è necessario abilitare le funzionalità contabili complete, altrimenti le voci sono nascoste. Nella 11.0 non è così, le voci sono visibili.
    Ci sono motivazioni particolari per questa differenza?
    Un utente potrebbe voler gestire le dichiarazioni Intrastat usando solo il modulo fatturazione e magazzino.

Questi controlli c'erano già nel modulo 10.0 che @labaggio ha migrato alla 11.0 e che abbiamo portato alla 12.0; nel README è specificato che vanno abilitate le funzionalità contabili complete.

Nella 10.0 il problema non si poneva perché la parte di contabilità non era nascosta.
Quello che ancora non mi è chiaro è perché nella 11.0 quasi tutte le voci Intrastat sono visibili senza abilitare le funzionalità contabili complete mentre nella 12.0 no.

In ogni caso, a meno che non sia strettamente necessario per il funzionamento, non vincolerei l'uso dei moduli Intrastat alle funzionalità contabili nascoste. (o perlomeno allineerei la 12.0 alla 11.0)
Imho, se a un utente non interessa la contabilità ma vuole usare i moduli Intrastat con fatturazione e magazzino dovrebbe poterlo fare in modo semplice.
Magari vediamo anche cosa ne pensano @OCA/local-italy-developers

@glauco70
Copy link
Author

  1. Per poter utilizzare i moduli Intrastat nella 12.0 è necessario abilitare le funzionalità contabili complete, altrimenti le voci sono nascoste. Nella 11.0 non è così, le voci sono visibili.
    Ci sono motivazioni particolari per questa differenza?
    Un utente potrebbe voler gestire le dichiarazioni Intrastat usando solo il modulo fatturazione e magazzino.

Questi controlli c'erano già nel modulo 10.0 che @labaggio ha migrato alla 11.0 e che abbiamo portato alla 12.0; nel README è specificato che vanno abilitate le funzionalità contabili complete.

Nella 10.0 il problema non si poneva perché la parte di contabilità non era nascosta.

Nella 10 bisognava avere l'accesso "Adviser" per poter visualizzare il menù "Intrastat Statement", nella 11.0 e 12.0 questo accesso è stato tolto sostituito dalle funzioni contabili complete

Quello che ancora non mi è chiaro è perché nella 11.0 quasi tutte le voci Intrastat sono visibili senza abilitare le funzionalità contabili complete mentre nella 12.0 no.

Non mi risulta, funziona come la 12; comunque ho appena aggiornato anche la 11.0 (sistemando un errore in fase di salvataggio fattura) e inserendo le voci di menu "Intrastat lines" e "Intrastat Statement" nello stesso menu

In ogni caso, a meno che non sia strettamente necessario per il funzionamento, non vincolerei l'uso dei moduli Intrastat alle funzionalità contabili nascoste. (o perlomeno allineerei la 12.0 alla 11.0)
Imho, se a un utente non interessa la contabilità ma vuole usare i moduli Intrastat con fatturazione e magazzino dovrebbe poterlo fare in modo semplice.
Magari vediamo anche cosa ne pensano @OCA/local-italy-developers

Non è necessario per il funzionamento, se si decide di cambiare la logica nessun problema

@tafaRU
Copy link
Member

tafaRU commented Nov 20, 2019

@primes2h mergiamo?

@labaggio
Copy link
Contributor

@primes2h mergiamo?

Si vi prego.. :)
Il funzionamento non mi sembra più messo in discussione, l'unico dubbio rimasto è sul discorso del flag per abilitare le voci di menu, ma come @glauco70 ha scritto, prima era un accesso (Adviser) ora un flag, quindi non mi pare bloccante.
Al massimo apriamo una issue e discutiamone lì e quando si sarà presa una decisione faremo una PR, ma mergiamo questa PR che è aperta veramente da un sacco di tempo.

@tafaRU
Copy link
Member

tafaRU commented Nov 20, 2019

Al massimo apriamo una issue e discutiamone lì e quando si sarà presa una decisione faremo una PR, ma mergiamo questa PR che è aperta veramente da un sacco di tempo.

Per me è 🆗 Vediamo cosa dice @primes2h.

Copy link
Contributor

@primes2h primes2h left a comment

Choose a reason for hiding this comment

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

Si vi prego.. :)
Il funzionamento non mi sembra più messo in discussione, l'unico dubbio rimasto è sul discorso del flag per abilitare le voci di menu, ma come @glauco70 ha scritto, prima era un accesso (Adviser) ora un flag, quindi non mi pare bloccante.
Al massimo apriamo una issue e discutiamone lì e quando si sarà presa una decisione faremo una PR, ma mergiamo questa PR che è aperta veramente da un sacco di tempo.

Per me va bene!
A questo punto però prima del merge è meglio specificare di abilitare le funzionalità contabili anche in l10n_it_intrastat (in l10n_it_intrastat_statement è già indicato).
Inoltre ho notato che nella 12.0 i percorsi sono diversi rispetto alla 11.0, sotto trovate tutti i suggerimenti per correggere i readme.

Dopo queste ultime piccole modifiche per me è 👍

l10n_it_intrastat_statement/readme/USAGE.rst Outdated Show resolved Hide resolved
l10n_it_intrastat_statement/readme/USAGE.rst Outdated Show resolved Hide resolved
l10n_it_intrastat_statement/readme/USAGE.rst Outdated Show resolved Hide resolved
l10n_it_intrastat/readme/CONFIGURE.rst Show resolved Hide resolved
l10n_it_intrastat_statement/readme/DESCRIPTION.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@primes2h primes2h left a comment

Choose a reason for hiding this comment

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

Grazie!

@labaggio
Copy link
Contributor

/ocabot merge

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 12.0-ocabot-merge-pr-1314-by-labaggio-bump-no, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Nov 21, 2019
Signed-off-by labaggio
@OCA-git-bot OCA-git-bot merged commit 10fb5dd into OCA:12.0 Nov 21, 2019
@OCA-git-bot
Copy link
Contributor

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

9 participants