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

[14.0][FIX]: l10n_it_fatturapa_out: create attachment with res_id not null #3337

Closed
wants to merge 1 commit into from

Conversation

TheMule71
Copy link
Contributor

Fixes: #3336

@TheMule71
Copy link
Contributor Author

Si tratta certamente di un hack. Del resto, l'allegato si riferisce a più fatture e non abbiamo un model al quale attaccarlo direttamente.

Un'alternativa potrebbe essere quella di attaccarlo alla company, ma si rischia di avere migliaia di allegati e l'impatto della cosa non è chiaro.

@@ -53,6 +53,8 @@ def saveAttachment(self, fatturapa, number):

attach_str = fatturapa.to_xml(self.env)
attach_vals = {
"res_model": "fatturapa.attachment.out",
Copy link
Member

Choose a reason for hiding this comment

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

Puoi aggiungere un commento che spieghi l'hack?

@@ -53,6 +53,8 @@ def saveAttachment(self, fatturapa, number):

attach_str = fatturapa.to_xml(self.env)
attach_vals = {
"res_model": "fatturapa.attachment.out",
"res_id": -1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Grazie della PR!
Così com'è non la vedo come una modifica facilmente mantenibile: ad esempio per sapere se è necessaria nella altre versioni (comprese migrazioni future), bisogna provare a mano i passi che hai descritto nella issue.

Per renderla più mantenibile, puoi aggiungere un test?

Una modifica meno hack potrebbe essere fare override del metodo ir.attachment.check per gestire il caso in cui l'allegato è collegato a una fattura elettronica.

Potresti chiarire come mai res_id deve essere proprio -1?
Normalmente è l'ID del record cui è collegato l'allegato.

Copy link
Contributor

Choose a reason for hiding this comment

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

Una modifica meno hack potrebbe essere fare override del metodo ir.attachment.check per gestire il caso in cui l'allegato è collegato a una fattura elettronica.

+1 ma quelle righe sono atomiche nel metodo, faresti un override completo ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Una modifica meno hack potrebbe essere fare override del metodo ir.attachment.check per gestire il caso in cui l'allegato è collegato a una fattura elettronica.

+1 ma quelle righe sono atomiche nel metodo, faresti un override completo ?

Cosa sarebbe un override completo?

Intendevo una modifica del tipo:

if `è un allegato di fattura elettronica`:
    # Fai i controlli necessari in questo caso
else:
    return super()...

Copy link
Contributor

Choose a reason for hiding this comment

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

Intendo che non so se è possibile bypassare solo questo controllo chiamando super() , devi andare a modificare il metodo nel suo insieme o sbaglio?

Copy link
Contributor

Choose a reason for hiding this comment

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

Intendo che non so se è possibile bypassare solo questo controllo chiamando super() , devi andare a modificare il metodo nel suo insieme o sbaglio?

Fare override vuol dire ridefinire un certo metodo chiamando super per eseguire il codice del metodo ridefinito, che io sappia non esistono override che permettono di modificare il codice solo di parte del metodo.

Ci si può andare a complicare la vita con il context, ma eviterei.

Copy link
Contributor

Choose a reason for hiding this comment

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

@SirAionTech Si mi sono espresso male, stavo cercando di capire se si potesse tentare una soluzione di quel tipo, ma diventa macchinoso correggere il metodo, forse conviene tenere la soluzione attuale

Copy link
Member

Choose a reason for hiding this comment

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

Una modifica meno hack potrebbe essere fare override del metodo ir.attachment.check per gestire il caso in cui l'allegato è collegato a una fattura elettronica

Sequendo questa idea ho fatto #3827

@francesco-ooops
Copy link
Contributor

francesco-ooops commented Jul 4, 2023

Ciao, qualche aggiornamento su questa PR? è piuttosto bloccante che la maggior parte degli utenti non possano accedere alla lista delle fatture elettroniche

@@ -53,6 +53,8 @@ def saveAttachment(self, fatturapa, number):

attach_str = fatturapa.to_xml(self.env)
attach_vals = {
"res_model": "fatturapa.attachment.out",
"res_id": -1,
Copy link

Choose a reason for hiding this comment

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

toglierei il res_id a -1

Suggested change
"res_id": -1,

Per mettere il res_id corretto si potrebbe fare una doppia scrittura create -> update, sostituendo la riga di return con:

attach_obj = attach_obj.create(attach_vals)
attach_obj.write({"res_id": attach_obj.id})
return attach_obj

Copy link
Member

Choose a reason for hiding this comment

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

@jackom83 cosa intendi con "res_id corretto"?

Choose a reason for hiding this comment

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

Quello dell'allegato che stai creando (l'XML della fattura)

Copy link
Member

Choose a reason for hiding this comment

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

Ma questo comporterebbe far comparire l'allegato, come allegato standard, sull'oggetto XML della fattura, giusto? E probabilmente non lo vogliamo.

Comunque vedi #3827

Choose a reason for hiding this comment

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

Non mi è chiaro che intendi nn compare gia ? nell'scheda fattura elettronica, forse mi sfugge dove altro comparirebbe ?

Copy link
Contributor Author

@TheMule71 TheMule71 Jan 13, 2024

Choose a reason for hiding this comment

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

res_id è l'id dell'oggetto a cui l'allegato è... allegato. Se l'allegato fosse un PDF, res_model sarebbe il modello del documento e res_id il suo ID.

Mettere come res_id l'id dell'allegato stesso creerebbe una sorta di loop, diventerebbe un allegato di sé stesso, la vedo come una soluzione con più potenziali problemi rispetto a -1, che non è NULL nel senso che non è un allegato orfano perché è stato cancellato l'oggetto a cui era collegato, ma allo stesso tempo Odoo non lo collega mai a nessuno oggetto visto che hanno tutti ID > 1.

Choose a reason for hiding this comment

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

@TheMule71 mi convince la tua osservazione, ma allora mi chiedo perche non mettere il res_id dell'oggetto fattura a cui l'allegato si riferisce, è perche altrimenti verrebbe trattato come allegato standard come indicava @eLBati ? quali controindicazioni ci sono nel consentire cio ? se l'allegato diventa visibile anche tra gli allegati standard di "account.move" si potrebbe agire sulla vista per nascondere gli allegati di tipo "fatturapa.attachment.out"

Chiaramente è un ipotesi, anche la soluzione in #3827 mi sembra buona, pero vedo che è solo per la 16 se nn erro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Non so perché mi sono perso il commento, comunque un allegato può contenere più fatture.

@francesco-ooops
Copy link
Contributor

@eLBati @TheMule71 (e @SirAionTech che aveva proposto un diverso approccio) dunque alla fine né la soluzione proposta qui né quella in #3827 sono da adottare?

@eLBati
Copy link
Member

eLBati commented Jan 18, 2024

Per me si può seguire la #3827 , ho scritto chiarimenti su #3827 (comment)

Però se qualcuno vuole implementare la valorizzazione di res_model e res_id non mi oppongo.
Attenzione che vanno valorizzati sia per le fatture in uscita che per quelle in ingresso.
Inoltre servono gli script di upgrade per valorizzarli per tutte le e-fatture presenti nel DB.

@francesco-ooops
Copy link
Contributor

@TheMule71 si può chiudere a seguito di #4194 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

La lista delle fatture elettroniche non è sempre visibile agli utenti non amministratori
9 participants