l10n-italy icon indicating copy to clipboard operation
l10n-italy copied to clipboard

[MIG] assets_management -> l10n_it_asset_management: Migration to 16.0

Open SirAionTech opened this issue 1 year ago • 42 comments

Migrazione da https://github.com/OCA/l10n-italy/tree/2f71528a4b249e2adf33d18108247c80f15f55b1/assets_management (14.0). Sostituisce https://github.com/OCA/l10n-italy/pull/3271.

Il commit successivo alla migrazione risolve https://github.com/OCA/l10n-italy/issues/3775 per 16.0. Risolve https://github.com/OCA/l10n-italy/issues/3009 per 16.0. Risolve https://github.com/OCA/l10n-italy/issues/3318 per 16.0. Risolve https://github.com/OCA/l10n-italy/issues/3851 per 16.0. Risolve https://github.com/OCA/l10n-italy/issues/3874 per 16.0. Risolve https://github.com/OCA/l10n-italy/issues/3170 per 16.0.

SirAionTech avatar Dec 12 '23 15:12 SirAionTech

Domani faccio test.

filipposaviori avatar Dec 12 '23 16:12 filipposaviori

lo rinimini in l10n_it_asset_management?

Ah grazie mi sono dimenticato di scriverlo: rispetto a https://github.com/OCA/l10n-italy/pull/3271 non c'è più la rinomina del modulo. Non l'ho aggiunta perché non ho trovato nessuna discussione o motivazione per aggiungerla. Tu sai mica chi l'ha deciso e perché è stato deciso di rinominarlo?

SirAionTech avatar Dec 13 '23 16:12 SirAionTech

credo per sottolineare la specificità italiana del modulo, visto che la precedente PR faceva così manterrei la rinomina del modulo.

andreampiovesana avatar Dec 13 '23 17:12 andreampiovesana

@SirAionTech era stato discusso qua https://github.com/OCA/l10n-italy/pull/3155#issuecomment-1429345028 ed anche io lo ritengo corretto (anche solo per non buttare via il lavoro di @odooNextev )

francesco-ooops avatar Dec 13 '23 18:12 francesco-ooops

@SirAionTech era stato discusso qua #3155 (comment)

Esatto, nello specifico la discussione era partita qui.

primes2h avatar Dec 13 '23 18:12 primes2h

Aggiunta rinomina del modulo, provata con https://github.com/OCA/OpenUpgrade

SirAionTech avatar Dec 14 '23 11:12 SirAionTech

@SirAionTech potrebbe essere la buona occasione di aggiungere una copertura di test?

francesco-ooops avatar Dec 14 '23 11:12 francesco-ooops

@SirAionTech potrebbe essere la buona occasione di aggiungere una copertura di test?

Sì, se/quando ho tempo li aggiungo.

A occhio la mancanza grossa di copertura è sul codice dei report, quindi potrei partire dal porting di https://github.com/OCA/l10n-italy/pull/3010.

SirAionTech avatar Dec 14 '23 12:12 SirAionTech

@SirAionTech potrebbe essere la buona occasione di aggiungere una copertura di test?

Sì, se/quando ho tempo li aggiungo.

A occhio la mancanza grossa di copertura è sul codice dei report, quindi potrei partire dal porting di #3010.

@francesco-ooops migliorata la copertura grazie al porting di #3010, anche se non è cambiata quanto speravo. Ora mi limiterei a fare modifiche solo se ci sono problemi dovuti alla migrazione in sé.

SirAionTech avatar Dec 19 '23 14:12 SirAionTech

Ciao @SirTakobi quando creo un nuovo cespite a partire da una fattura di acquisto erroneamente riporta la data anche nel campo "dismiss date" Screenshot 2024-01-15 153257

Marianna-Marasco avatar Jan 15 '24 14:01 Marianna-Marasco

Ciao @SirTakobi quando creo un nuovo cespite a partire da una fattura di acquisto erroneamente riporta la data anche nel campo "dismiss date" Screenshot 2024-01-15 153257

Grazie della revisione! Abbiamo visto poco fa che il problema esiste anche in 14.0, creo la issue (https://github.com/OCA/l10n-italy/issues/3851) e correggo.

SirAionTech avatar Jan 15 '24 15:01 SirAionTech

I test falliscono per https://github.com/OCA/l10n-italy/issues/3801.

SirAionTech avatar Jan 15 '24 16:01 SirAionTech

Il modulo è incompatibile con account_asset di Odoo Enterprise? Se sì, conviene scriverlo da qualche parte

eLBati avatar Feb 08 '24 16:02 eLBati

Il modulo è incompatibile con account_asset di Odoo Enterprise? Se sì, conviene scriverlo da qualche parte

Mi rispondo: è incompatibile. Entrambi aggiungono asset_ids a account.move.line e in account_asset punta a account.asset mentre in l10n_it_asset_management punta a asset.asset

eLBati avatar Feb 08 '24 16:02 eLBati

Domani faccio test.

@filipposaviori quando riesci :)

francesco-ooops avatar Feb 08 '24 16:02 francesco-ooops

Il modulo è incompatibile con account_asset di Odoo Enterprise? Se sì, conviene scriverlo da qualche parte

Mi rispondo: è incompatibile. Entrambi aggiungono asset_ids a account.move.line e in account_asset punta a account.asset mentre in l10n_it_asset_management punta a asset.asset

Ho aperto la issue https://github.com/OCA/l10n-italy/issues/3952 per chiarire che il problema esisteva già in 14.0.

Visto che è un problema indipendente dalla migrazione e si può correggere anche dopo, sarei per mergiare la migrazione così com'è se non ci sono problemi legati alla migrazione in sé, cosa ne pensi? Anche perché questa migrazione (contando anche https://github.com/OCA/l10n-italy/pull/3271) è in ballo da quasi un anno.

In un secondo momento poi si potrà

scriverlo da qualche parte

o anche risolverlo (in https://discord.com/channels/753902328494424064/753902328494424070/1205446526319591435 hai scritto che vi organizzate per farlo) per 16.0 e magari anche per 14.0, come soluzione della issue https://github.com/OCA/l10n-italy/issues/3952.

SirAionTech avatar Feb 13 '24 14:02 SirAionTech

@OCA/local-italy-maintainers potete fare review?

francesco-ooops avatar Feb 13 '24 14:02 francesco-ooops

Visto che è un problema indipendente dalla migrazione e si può correggere anche dopo, sarei per mergiare la migrazione così com'è se non ci sono problemi legati alla migrazione in sé, cosa ne pensi? Anche perché questa migrazione (contando anche #3271) è in ballo da quasi un anno.

A proposito potresti aggiungere anche "Nextev Srl [email protected]" nei contributori?

odooNextev avatar Feb 13 '24 14:02 odooNextev

Visto che è un problema indipendente dalla migrazione e si può correggere anche dopo, sarei per mergiare la migrazione così com'è se non ci sono problemi legati alla migrazione in sé, cosa ne pensi? Anche perché questa migrazione (contando anche #3271) è in ballo da quasi un anno.

A proposito potresti aggiungere anche "Nextev Srl [email protected]" nei contributori?

Come hai notato in https://github.com/OCA/l10n-italy/pull/3271#issuecomment-1866010598

@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

sono ripartito da nuovo a fare la migrazione, perché l'ho trovato più semplice che correggere una per una tutte le cose che avevo segnalato in https://github.com/OCA/l10n-italy/pull/3271#pullrequestreview-1736629246.

Ho comunque aggiunto un commit per metterti tra i contributori :smile:

SirAionTech avatar Feb 13 '24 15:02 SirAionTech

@SirAionTech ok grazie 👍 scusa, ma infatti non ricordavo, perchè eri ripartito più o meno da 0? C'erano troppi cherrypick da fare per allineare la 14.0?

odooNextev avatar Feb 13 '24 15:02 odooNextev

perchè eri ripartito più o meno da 0? C'erano troppi cherrypick da fare per allineare la 14.0?

No, sono ripartito da 0 a fare la migrazione

perché l'ho trovato più semplice che correggere una per una tutte le cose che avevo segnalato in https://github.com/OCA/l10n-italy/pull/3271#pullrequestreview-1736629246.

SirAionTech avatar Feb 13 '24 15:02 SirAionTech

Ciao @SirAionTech, come l'altra volta non riesco a creare una PR sul tuo branch 😅 Il branch è questo, ho apportato delle modifiche per assicurare la compatibilità di account_asset di Enterprise

LorenzoC0 avatar Feb 15 '24 11:02 LorenzoC0

Ciao @SirAionTech, come l'altra volta non riesco a creare una PR sul tuo branch 😅 Il branch è questo, ho apportato delle modifiche per assicurare la compatibilità di account_asset di Enterprise

ma se sono incompatibili, perche' li rendi compatibili?

matteoopenf avatar Feb 15 '24 11:02 matteoopenf

Ciao @SirAionTech, come l'altra volta non riesco a creare una PR sul tuo branch 😅 Il branch è questo, ho apportato delle modifiche per assicurare la compatibilità di account_asset di Enterprise

In https://github.com/SirAionTech/l10n-italy/compare/16.0-mig-assets_management...LorenzoC0:l10n-italy:16.0-mig-assets_management non hai il pulsante per creare la pull request? image

SirAionTech avatar Feb 15 '24 13:02 SirAionTech

@SirAionTech Non vedo la tua repository nel dropdown dei fork disponibili 🤔 immagine

LorenzoC0 avatar Feb 15 '24 15:02 LorenzoC0

@SirAionTech Non vedo la tua repository nel dropdown dei fork disponibili 🤔 immagine

Che strano, il link che ho messo in https://github.com/OCA/l10n-italy/pull/3776#issuecomment-1946094498 dovrebbe selezionare già tutto. Anche aprendolo da incognito vedo il branch selezionato: image

SirAionTech avatar Feb 15 '24 15:02 SirAionTech

@SirAionTech Non vedo la tua repository nel dropdown dei fork disponibili 🤔 immagine

il filtro su "base: 14.0" invece che 16.0 non ha nulla a che vedere?

francesco-ooops avatar Feb 15 '24 15:02 francesco-ooops

@SirAionTech Non vedo la tua repository nel dropdown dei fork disponibili 🤔 immagine

il filtro su "base: 14.0" invece che 16.0 non ha nulla a che vedere?

Nemmeno così, non capisco 🤔 Vedo comunque tutte le altre repo forkate se lascio la ricerca vuota immagine

LorenzoC0 avatar Feb 15 '24 15:02 LorenzoC0

ma se sono incompatibili, perche' li rendi compatibili?

Perchè nei casi di DB multi country e multi localizzazioni per l'Italia serve l10n_it_asset_management e per altre nazioni serve account_asset, quindi devono poter funzionare insieme

eLBati avatar Feb 16 '24 08:02 eLBati

ma se sono incompatibili, perche' li rendi compatibili?

Perchè nei casi di DB multi country e multi localizzazioni per l'Italia serve l10n_it_asset_management e per altre nazioni serve account_asset, quindi devono poter funzionare insieme

Capito, grazie mille della spiegazione

matteoopenf avatar Feb 16 '24 08:02 matteoopenf