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_asset_management #3271

Closed

Conversation

odooNextev
Copy link
Contributor

Ho provato a migrare il modulo e facendo alcuni test non mi da errori. Qualcuno mi potrebbe aiutare con una review funzionale?
Grazie

@francesco-ooops
Copy link
Contributor

si può aggiungere alla issue di migrazione?

@odooNextev odooNextev changed the title 16.0 mig l10n it assets management [16.0] [MIG] l10n_it_assets_management May 25, 2023
@OpenCode OpenCode mentioned this pull request May 25, 2023
80 tasks
@OpenCode
Copy link
Contributor

si può aggiungere alla issue di migrazione?

Fatto

@sergiocorato
Copy link
Contributor

Grazie, consiglierei di integrare le modifiche proposte sulla 14.0 (riassunte qui #3351 ) prima di proseguire.

@sergiocorato
Copy link
Contributor

Promemoria: rinominando il modulo c'è da definire se va inserita una funzione per farlo qui o inserirla in OpenUpgrade qui https://github.com/OCA/OpenUpgrade/blob/952813dd83ed5cf7bda224310bb717db62bd4f07/openupgrade_scripts/apriori.py#L23 , oltre agli script di migrazione.

@odooNextev
Copy link
Contributor Author

@sergiocorato Va bene, quando sarà mergiata integriamo le modifiche. Non dovrebbe essere necessario lo script di migrazione perchè i nomi dei modelli non cambiano. Va bene solo aggiungerlo alla lista?

@sergiocorato
Copy link
Contributor

@sergiocorato Va bene, quando sarà mergiata integriamo le modifiche.

Come andate meglio.

Non dovrebbe essere necessario lo script di migrazione perchè i nomi dei modelli non cambiano. Va bene solo aggiungerlo alla lista?

Ne stiamo discutendo in PSC, appena ne esce una decisione sarà resa pubblica.

@primes2h primes2h changed the title [16.0] [MIG] l10n_it_assets_management [16.0] [MIG] l10n_it_asset_management Jun 9, 2023
Copy link
Contributor

@Borruso Borruso left a comment

Choose a reason for hiding this comment

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

Grazie per la PR

Comment on lines 99 to 101
@api.model
def create(self, vals):
# Add depreciation if it's missing while category is set
Copy link
Contributor

Choose a reason for hiding this comment

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

Dovresti usare il decoratore @api.model_create_multi e modificare la funzione per warning is not overriding the create method in batch

Copy link
Contributor Author

@odooNextev odooNextev Jul 6, 2023

Choose a reason for hiding this comment

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

Fatto, ma devo cambiare il recupero dei vals

Copy link
Contributor

Choose a reason for hiding this comment

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

ora la create ritorna solo un asset ma nel create_multi possono esserci puoi record da creare

l10n_it_asset_management/models/asset_accounting_info.py Outdated Show resolved Hide resolved
l10n_it_asset_management/models/asset_depreciation.py Outdated Show resolved Hide resolved
l10n_it_asset_management/models/asset_depreciation_line.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Borruso Borruso 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
ma solo quando lancio il wizard delle previsioni mi appare questo errore
photo_2023-06-16_08-51-07

@eLBati
Copy link
Member

eLBati commented Jul 6, 2023

/ocabot rebase

@OCA-git-bot
Copy link
Contributor

Congratulations, PR rebased to 16.0.

@OCA-git-bot OCA-git-bot force-pushed the 16.0-mig-l10n_it_assets_management branch from 66ec237 to ddbf359 Compare July 6, 2023 08:29
@AmePal
Copy link

AmePal commented Jul 6, 2023

Buon pomeriggio,

ho testato il modulo, direi che a parte il problema già segnalato in precedenza (anch'io riscontro l'errore in allegato quando provo a cliccare sul pulsante "vedi" presente nella stampa del registro cespiti) mi sembra tutto corretto e funzionante come sulla versione 14.
Segnalo solamente che la stampa del registro cespiti taglia il numero delle pagine sulla destra (invio anche la stampa in allegato dove sottolineo in giallo il taglio sul layout della prima pagina del registro).
L'unica differenza che noto rispetto alla versione 14 del modulo è relativa alle registrazioni contabili, che mentre nella versione precedente si confermavano automaticamente, nella versione 16 sembrerebbero restare in bozza.

Registro beni ammortizzabili - HTML (21).pdf

Allegato1

@odooNextev
Copy link
Contributor Author

Comincio a rivedere le segnalazioni e cercare di sistemarle

@AmePal
Copy link

AmePal commented Jul 6, 2023

Buonasera,

adesso riscontro un altro problema alla creazione del cespite (allego dettaglio dell'errore), in precedenza non si verificava.
Credo che probabilmente sia dovuto all'aggiunta di: "@api.model_create_multi", ho modificato la funzione create su un mio ambiente di test e il tutto ha funzionato nuovamente in maniera corretta.
Potreste fare una verifica?
Vi ringrazio

errore

@odooNextev
Copy link
Contributor Author

Buonasera,

adesso riscontro un altro problema alla creazione del cespite (allego dettaglio dell'errore), in precedenza non si verificava. Credo che probabilmente sia dovuto all'aggiunta di: "@api.model_create_multi", ho modificato la funzione create su un mio ambiente di test e il tutto ha funzionato nuovamente in maniera corretta. Potreste fare una verifica? Vi ringrazio

errore

Hai ragione
ho corretto l'errore in locale, ma stavo aspettando di risolvere anche quello dei report prima di pubblicare le correzioni

@odooNextev
Copy link
Contributor Author

@AmePal @Borruso ho risolto alcuni problemi ed ora è nuovamente testabile in runboat
Manca la migrazione della funzionalità di visualizzazione dei report diretta senza esportazione in un file, ma mi sa che sarà lunga perchè si avvaleva di un componente di account_financial_reports che ora è stato stravolto.
Indago ancora un po', ma si potrebbe anche rimuovere la funzionalità e lasciare solo la stampa xlsx e pdf

@AmePal
Copy link

AmePal commented Jul 7, 2023

Buongiorno @Borruso,

adesso il pulsante "vedi" sembrerebbe funzionare, mentre riscontro un errore quando clicco sul pulsante "esporta dati registro" (allego errore).
In ogni caso secondo me, data la complicazione segnalata sul componente di account_financial_report, si potrebbe anche rimuovere la funzionalità della visualizzazione senza esportazione e lasciare solo la possibilità di esportare pdf ed excel.
Resterebbe solo da modificare leggermente il layout in modo che i numeri sulla destra non vengano troncati.

Cattura errore

@odooNextev
Copy link
Contributor Author

Buongiorno @Borruso,

adesso il pulsante "vedi" sembrerebbe funzionare, mentre riscontro un errore quando clicco sul pulsante "esporta dati registro" (allego errore). In ogni caso secondo me, data la complicazione segnalata sul componente di account_financial_report, si potrebbe anche rimuovere la funzionalità della visualizzazione senza esportazione e lasciare solo la possibilità di esportare pdf ed excel. Resterebbe solo da modificare leggermente il layout in modo che i numeri sulla destra non vengano troncati.

Ti rispondo io perchè sto facendo io le modifiche.
Cerco di risolvere il problema che hai segnalato, mentre per il report ho applicato la PR che ha fatto Sergio per la v14 #3348

@AmePal
Copy link

AmePal commented Jul 10, 2023

Buon pomeriggio,

confermo che adesso tutti e tre i pulsanti presenti nella stampa del registro funzionano e non restituiscono alcun errore.
Resterebbe solo quel problemino sul layout di stampa, per il resto mi sembra che il modulo funzioni correttamente come nella versione 14.

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.

Dando un rapido sguardo noto alcune cose mancanti/bloccanti:

  • rivedere la history dei commit
  • aggiungere rename del modulo tramite opportuno hook / script di migrazione contestuale al commit in cui il modulo viene rinominato

@stenext
Copy link

stenext commented Sep 7, 2023

Dando un rapido sguardo noto alcune cose mancanti/bloccanti:

  • rivedere la history dei commit

che comando lanceresti per fare un rebase dei soli commit di questo modulo?
ho provato con git rebase -i origin/16.0 o git rebase -i _ID_PRIMO_COMMIT_, ma include anche gli altri commit non solo quelli che ci interessano

  • aggiungere rename del modulo tramite opportuno hook / script di migrazione contestuale al commit in cui il modulo viene rinominato

dovrebbe bastare questo:
openupgrade.rename_tables(cr, [('assets_management', 'l10n_it_asset_management')])
è una buona soluzione per modificare un vecchio commit? https://stackoverflow.com/questions/2719579/how-to-add-a-changed-file-to-an-older-not-last-commit-in-git
però non dovrebbe servire perchè tutti i commit post pre-commit nella migrazione verranno squashati perciò non conta se inserisco lo script prima o dopo gli altri

@odooNextev odooNextev force-pushed the 16.0-mig-l10n_it_assets_management branch from a1ff644 to 3fabee7 Compare September 11, 2023 07:50
@odooNextev odooNextev force-pushed the 16.0-mig-l10n_it_assets_management branch from e77f352 to 08a7065 Compare September 19, 2023 08:53
@odooNextev odooNextev force-pushed the 16.0-mig-l10n_it_assets_management branch from f838749 to 1626e30 Compare November 17, 2023 09:59
@odooNextev
Copy link
Contributor Author

@SirAionTech ho fatto il rebase, spero di averlo fatto bene-
Fammi sapere che è probabilmente l'ultimo modulo che resta da migrare.

@odooNextev
Copy link
Contributor Author

@tafaRU puoi aggiornare la review?

@SirAionTech
Copy link
Contributor

@SirAionTech ho fatto il rebase, spero di averlo fatto bene- Fammi sapere che è probabilmente l'ultimo modulo che resta da migrare.

Grazie mille, ora è più fattibile guardare le differenze di codice; quando ho fatto aggiorno la revisione.

A occhio comunque è rimasto ancora qualche commit consecutivo di traduzioni dello stesso autore che sarebbero da schiacciare:
image,
image.

In teoria si potrebbero anche riordinare e schiacciare per autore le traduzioni non consecutive tipo
image
, ci guadagneremmo un po' di commit, però c'è il rischio di conflitti quindi secondo me si può evitare.

@odooNextev odooNextev force-pushed the 16.0-mig-l10n_it_assets_management branch from 1626e30 to c0db61c Compare November 17, 2023 10:53
@odooNextev
Copy link
Contributor Author

@SirAionTech sì, mi erano sfuggiti quei commit che hai segnalato... ce ne sono talmente tanti...
Adesso ho aggiornato

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 è che io sia esperto di cespiti, tutt'altro, volevo solo fare un giro di prova ma ottengo un errore quando provo a creare un cespite:

Odoo.-.Cespiti.mp4

Nota che non vedo i campi Azienda/Valuta quindi non ho i gruppi multi-company/currency rispettivamente.

Potresti verificare?

class AccountAccount(models.Model):
_inherit = "account.account"

def unlink(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Questo metodo e gli altri override di unlink che sollevano un'eccezione sono buoni candidati per diventare metodi ondelete, vedi https://www.odoo.com/documentation/16.0/developer/reference/backend/orm.html#odoo.api.ondelete (il codice è https://github.com/odoo/odoo/blob/32f74b1bd6f84d8ac5b86eb1e4b9754864efb7d1/odoo/api.py#L140).

In breve, il problema di lasciarli così è che se sollevano un errore durante la disinstallazione del modulo, possono rimanere dati inconsistenti nel DB.


@api.model
def get_default_company_id(self):
return self.env.user.company_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Qui e negli altri punti dove questo approccio è ripetuto: se, come immagino, si vuole ottenere l'azienda corrente dell'utente, bisogna usare self.env.company.
Se è loggato in più aziende, verrà ritornata semplicemente la prima.

self.env.user.company_id è invece l'azienda predefinita dell'utente: quella attiva in automatico al login, non è detto che sia l'azienda dove è loggato quando viene eseguito questo codice.

Comment on lines 79 to 80
sale_date = fields.Date(string="Sale Date")

dismiss_date = fields.Date()
sale_date = fields.Date()

sale_move_id = fields.Many2one("account.move", string="Sale Move")

sold = fields.Boolean(string="Sold")
dismissed = fields.Boolean(string="Dismissed")
sold = fields.Boolean()
Copy link
Contributor

Choose a reason for hiding this comment

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

Come mai sono stati rimossi i campi dismiss*?

num_lines = dep.line_ids.filtered("requires_depreciation_nr")
if num_lines:
num_lines.normalize_depreciation_nr()
for d in dep:
Copy link
Contributor

Choose a reason for hiding this comment

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

Sarebbe forse più leggibile for dep in deps ma non ci sono problemi a lasciarlo così

Comment on lines +37 to +40
_(
"Cannot remove type {}: there is some depreciation"
" line linked to it."
).format(line_type.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Qui e negli altri punti dove viene usato format: la funzione _ è stata migliorata in modo da essere più robusta per quanto riguarda la formattazione delle stringhe, sarebbe meglio quindi usare la nuova sintassi _("This %(arg)s", arg="value").

Altrimenti se in una traduzione manca un argomento viene sollevato un errore.

A memoria mi pare sia una novità della 16.0 quindi sarebbe da fare nella migrazione, se così non è possiamo lasciare stare.

Copy link
Contributor

Choose a reason for hiding this comment

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

Senza considerare che usare format per le traduzioni è un rischio per la sicurezza come spiegato in OCA/pylint-odoo#302.

Comment on lines +89 to +90
t-att-data-domain="domain"
t-att-data-res-model="'asset.asset'"
Copy link
Contributor

Choose a reason for hiding this comment

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

Qui e nelle altre occorrenze

Suggested change
t-att-data-domain="domain"
t-att-data-res-model="'asset.asset'"
t-att-domain="domain"
res-model="asset.asset"

Vedi le modifiche ai report in OCA/account-financial-reporting@b2d4672

t-att-data-res-model="'asset.asset'"
class="o_account_financial_reports_web_action_multi"
>
<t t-raw="category_section.category_name" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Qui e nelle altre occorrenze

Suggested change
<t t-raw="category_section.category_name" />
<t t-out="category_section.category_name" />

t-raw è deprecato, vedi https://github.com/odoo/odoo/blob/11704a57349c04e579096489b346c5fa72c93f73/odoo/addons/base/models/ir_qweb.py#L2039-L2048

<span style="margin: 5px;">
<span>Asset: </span>
<a
t-att-data-active-id="active_id"
Copy link
Contributor

Choose a reason for hiding this comment

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

Qui e nelle altre occorrenze

Suggested change
t-att-data-active-id="active_id"
t-att-res-id="active_id"

Vedi le modifiche ai report in OCA/account-financial-reporting@b2d4672

assets += dep_lines.mapped("asset_id")
move.update(
{
"asset_ids": [(6, 0, assets.ids)],
Copy link
Contributor

Choose a reason for hiding this comment

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

I vari (6, 0, ...) e simili possono essere sostituiti dai metodi di odoo.fields.Command.
Non ci sono comunque problemi a lasciare com'è.

<field name="model_id" ref="model_asset_asset" />
<field name="global" eval="True" />
<field name="domain_force">
['|',('company_id','=',False),('company_id','child_of',[user.company_id.id])]
Copy link
Contributor

Choose a reason for hiding this comment

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

Qui e nelle altre occorrenze

Suggested change
['|',('company_id','=',False),('company_id','child_of',[user.company_id.id])]
['|',('company_id','=',False),('company_id','in',company_ids)]

Vedi odoo/odoo@a5b6f31

@SirAionTech
Copy link
Contributor

Prendo in carico per risolvere #3271 (review).

@filipposaviori
Copy link

Servono verifiche funzionali?

@SirAionTech
Copy link
Contributor

Servono verifiche funzionali?

Se vuoi fai pure, ma a breve (oggi o domani) aggiungerò nuove modifiche a questa PR o ne pubblicherò direttamente un'altra per risolvere #3271 (review).

@filipposaviori
Copy link

Ok, no allora attendo la nuova PR e poi mi metto a testare su quella.

Grazie,
FS

@SirAionTech
Copy link
Contributor

Chiuderei per andare avanti in #3776.

@francesco-ooops
Copy link
Contributor

@odooNextev chiudiamo?

@odooNextev
Copy link
Contributor Author

@odooNextev chiudiamo?

La posso chiudere però pensavo di trovare un po' del lavoro che avevo fatto qui nella nuova, invece mi sembra si sia ripartiti da nuovo

@francesco-ooops
Copy link
Contributor

@odooNextev ok, per questioni specifiche direi di confrontarvi con @SirAionTech nell'altra PR, qui chiudo.

Grazie!

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.