[FIX] l10n_it_fatturapa: link fatturapa.attachments

[micheledic]

Seguendo questa PR #4107 il problema rimane sugli eventuali allegati contenuti nella fattura elettronica ( fatturapa.attachments)
Riuso lo stesso abstract per allineare correttamente fatturapa.attachments

Risolve #4264

[francesco-ooops]

@SirAionTech puoi verificare?

[SirAionTech]

@SirAionTech puoi verificare?

Non so come riprodurre il problema quindi non posso verificare

[micheledic]

@SirAionTech puoi verificare?

Non so come riprodurre il problema quindi non posso verificare

Per simulare il problema:

• Importa un XML fattura con degli allegati all'interno dell'XML con utente A così che crea fatturapa.attachments (che sono gli allegati in base64 contenuti all'interno dell'XML)
• Entra con utente B nella fattura importata ed esce errore perchè fatturapa.attachments ha un ir.attachment senza res_id e res_model e quindi per behaviour nativo di Odoo, se un ir.attachment non ha res_id/res_model (su cui fa il check dei permessi) , può accedervi SOLO l'utente che l'ha creato ... Lo stesso problema c'era prima del merge della PR [FIX] Import zip file e-fatture non possibile se non si hanno i diritti da Amministratore. Il sistema rimporta errore generico #4107 su fatturapa.attachment.in/fatturapa.attachment.out

[SirAionTech]

@SirAionTech puoi verificare?

Non so come riprodurre il problema quindi non posso verificare

Per simulare il problema:

• Importa un XML fattura con degli allegati all'interno dell'XML con utente A così che crea fatturapa.attachments (che sono gli allegati in base64 contenuti all'interno dell'XML)
• Entra con utente B nella fattura importata ed esce errore perchè fatturapa.attachments ha un ir.attachment senza res_id e res_model e quindi per behaviour nativo di Odoo, se un ir.attachment non ha res_id/res_model (su cui fa il check dei permessi) , può accedervi SOLO l'utente che l'ha creato ... Lo stesso problema c'era prima del merge della PR [FIX] Import zip file e-fatture non possibile se non si hanno i diritti da Amministratore. Il sistema rimporta errore generico #4107 su fatturapa.attachment.in/fatturapa.attachment.out

Grazie, puoi riportare il tutto in una issue come descritto in https://github.com/OCA/l10n-italy/wiki/Team-di-sviluppo#apertura-issue?

[micheledic]

@SirAionTech puoi verificare?

Non so come riprodurre il problema quindi non posso verificare

Per simulare il problema:

• Importa un XML fattura con degli allegati all'interno dell'XML con utente A così che crea fatturapa.attachments (che sono gli allegati in base64 contenuti all'interno dell'XML)
• Entra con utente B nella fattura importata ed esce errore perchè fatturapa.attachments ha un ir.attachment senza res_id e res_model e quindi per behaviour nativo di Odoo, se un ir.attachment non ha res_id/res_model (su cui fa il check dei permessi) , può accedervi SOLO l'utente che l'ha creato ... Lo stesso problema c'era prima del merge della PR [FIX] Import zip file e-fatture non possibile se non si hanno i diritti da Amministratore. Il sistema rimporta errore generico #4107 su fatturapa.attachment.in/fatturapa.attachment.out

Grazie, puoi riportare il tutto in una issue come descritto in https://github.com/OCA/l10n-italy/wiki/Team-di-sviluppo#apertura-issue?

#4264 fatto

[SirAionTech]

Ho provato in runboat a seguire i passi descritti in #4264 e non ottengo errori:

Screencast.from.11-07-2024.16.40.36.webm

la fattura creata è in http://oca-l10n-italy-16-0-79f19bb8345f.runboat.odoo-community.org/web#id=47&cids=1&model=account.move&view_type=form&menu_id=140

[micheledic]

Ho provato in runboat a seguire i passi descritti in #4264 e non ottengo errori:

Screencast.from.11-07-2024.16.40.36.webm
la fattura creata è in http://oca-l10n-italy-16-0-79f19bb8345f.runboat.odoo-community.org/web#id=47&cids=1&model=account.move&view_type=form&menu_id=140

Cliccando con user b sull'allegato :

[SirAionTech]

Ho provato in runboat a seguire i passi descritti in #4264 e non ottengo errori:
Screencast.from.11-07-2024.16.40.36.webm
la fattura creata è in http://oca-l10n-italy-16-0-79f19bb8345f.runboat.odoo-community.org/web#id=47&cids=1&model=account.move&view_type=form&menu_id=140

Cliccando con user b sull'allegato :

Ah ecco, dalla descrizione della issue avevo capito che veniva fuori un errore appena si apriva la fattura, puoi aggiungere nella issue che è questo l'errore cui ti riferisci?
Ora riprovo

[SirAionTech]

Grazie della PR!
Ho verificato che il problema esiste in 16.0 e viene corretto da queste modifiche.
Ho provato anche la migrazione e risolve il problema sui dati esistenti.
Stranamente, non sono riuscito a riprodurre il problema quando c'è installato solo l10n_it_fatturapa_in 🤷.
Sai mica come mai?

Puoi aggiungere un test che fallisce senza questa fix? Ci sono diversi esempi nella PR #4107 che hai linkato in descrizione.
Non credo possa andare in questo modulo perché qui non puoi creare fatture elettroniche, però puoi aggiungerlo in un altro modulo che dipende da questo; preferibilmente in un commit separato come fatto in #4107.

[micheledic]

Grazie della PR! Ho verificato che il problema esiste in 16.0 e viene corretto da queste modifiche. Ho provato anche la migrazione e risolve il problema sui dati esistenti. Stranamente, non sono riuscito a riprodurre il problema quando c'è installato solo l10n_it_fatturapa_in 🤷. Sai mica come mai?

Puoi aggiungere un test che fallisce senza questa fix? Ci sono diversi esempi nella PR #4107 che hai linkato in descrizione. Non credo possa andare in questo modulo perché qui non puoi creare fatture elettroniche, però puoi aggiungerlo in un altro modulo che dipende da questo; preferibilmente in un commit separato come fatto in #4107.

Il problema probabilmente si presenta con l10n_it_fatturapa_out installato per questa compute che accede a doc_attachments e non avendo i permessi esce errore , anche se in caso di fattura IN nono dovrebbe capitare...

A questo field ci accede un altra compute presente su account.move

has_pdf_invoice_print = fields.Boolean(
        related="fatturapa_attachment_out_id.has_pdf_invoice_print", readonly=True
    )

 @api.depends("out_invoice_ids.fatturapa_doc_attachments.is_pdf_invoice_print")
    def _compute_has_pdf_invoice_print(self):
        """Check if all the invoices related to this attachment
        have at least one attachment containing
        the PDF report of the invoice"""
        for attachment_out in self:
            attachment_out.has_pdf_invoice_print = False
            for invoice in attachment_out.out_invoice_ids:
                invoice_attachments = invoice.fatturapa_doc_attachments`

[micheledic]

@SirAionTech quel test in fail di cosa si tratta ? devo occuparmene o possiamo non considerarlo?

[SirAionTech]

@SirAionTech quel test in fail di cosa si tratta ? devo occuparmene o possiamo non considerarlo?

I check del coverage non sono obbligatori, non serve che ci fai nulla

[SirAionTech]

Quando hai fatto ricorda di chiedermi di aggiornare al review:

(da https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/requesting-a-pull-request-review)

[micheledic]

Quando hai fatto ricorda di chiedermi di aggiornare al review:

(da https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/requesting-a-pull-request-review)

fatto!

[SirAionTech]

A parte la modifica minima suggerita sotto, il problema è che il test aggiunto non verifica la fix: il test aggiunto dovrebbe fallire quando la fix non è presente.
Potrebbe dipendere da quello che scrivevi in #4250 (comment)? Se è così forse ti conviene mettere un test invece in l10n_it_fatturapa_out.

[micheledic]

A parte la modifica minima suggerita sotto, il problema è che il test aggiunto non verifica la fix: il test aggiunto dovrebbe fallire quando la fix non è presente. Potrebbe dipendere da quello che scrivevi in #4250 (comment)? Se è così forse ti conviene mettere un test invece in l10n_it_fatturapa_out.

Ho provato a vedere nella PR #4107 e i test verificano solo che è possibile accedere dopo il fix con assertTrue...
Considerando che il fix allinea res_id e res_model alla create del ir.attachment, non saprei come testarlo visto che è un change proprio facendo l'override della create del mixin( e quindi includendolo nel commit, si verifica sempre )...
Il mixin è presente dalla #4107 e ho fatto l'inherit in fatturapa.attachments così che si triggeri ad ogni create

Per il piccolo cambio richiesto sotto invece , l'ho fatto

[SirAionTech]

A parte la modifica minima suggerita sotto, il problema è che il test aggiunto non verifica la fix: il test aggiunto dovrebbe fallire quando la fix non è presente. Potrebbe dipendere da quello che scrivevi in #4250 (comment)? Se è così forse ti conviene mettere un test invece in l10n_it_fatturapa_out.

Ho provato a vedere nella PR #4107 e i test verificano solo che è possibile accedere dopo il fix con assertTrue... Considerando che il fix allinea res_id e res_model alla create del ir.attachment, non saprei come testarlo visto che è un change proprio facendo l'override della create del mixin( e quindi includendolo nel commit, si verifica sempre )... Il mixin è presente dalla #4107 e ho fatto l'inherit in fatturapa.attachments così che si triggeri ad ogni create

I test di #4107 però fallivano senza la loro fix.
Per aggiungere un test che fallisca senza la fix, puoi rimuoverla in locale (in questo caso ti basta commentare l'inherit che hai aggiunto) e verificare che il tuo test fallisca.
Poi ripristinando la fix (togli il commento che hai aggiunto), il test deve passare.

Qualcuno inizierebbe addirittura a sviluppare facendo prima il test e poi la correzione (vedi https://en.wikipedia.org/wiki/Test-driven_development).

[micheledic]

A parte la modifica minima suggerita sotto, il problema è che il test aggiunto non verifica la fix: il test aggiunto dovrebbe fallire quando la fix non è presente. Potrebbe dipendere da quello che scrivevi in #4250 (comment)? Se è così forse ti conviene mettere un test invece in l10n_it_fatturapa_out.

Ho provato a vedere nella PR #4107 e i test verificano solo che è possibile accedere dopo il fix con assertTrue... Considerando che il fix allinea res_id e res_model alla create del ir.attachment, non saprei come testarlo visto che è un change proprio facendo l'override della create del mixin( e quindi includendolo nel commit, si verifica sempre )... Il mixin è presente dalla #4107 e ho fatto l'inherit in fatturapa.attachments così che si triggeri ad ogni create

I test di #4107 però fallivano senza la loro fix. Per aggiungere un test che fallisca senza la fix, puoi rimuoverla in locale (in questo caso ti basta commentare l'inherit che hai aggiunto) e verificare che il tuo test fallisca. Poi ripristinando la fix (togli il commento che hai aggiunto), il test deve passare.

Qualcuno inizierebbe addirittura a sviluppare facendo prima il test e poi la correzione (vedi https://en.wikipedia.org/wiki/Test-driven_development).

ora è ok

[SirAionTech]

Oltre alle verifiche di #4250 (review):

Ho verificato che il problema esiste in 16.0 e viene corretto da queste modifiche.
Ho provato anche la migrazione e risolve il problema sui dati esistenti.

Ho anche verificato che il test fallisce senza la correzione, quindi per me è ok.
La nota qui sotto non la vedo come bloccante per il merge.

Grazie!

[SirAionTech]

e_invoice può essere fuorviante perché in realtà questo è un allegato della fattura elettronica, lo si potrebbe chiamare ad esempio e_attachment?

[SirAionTech]

/ocabot merge minor

[OCA-git-bot]

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-4250-by-SirAionTech-bump-minor, awaiting test results.

[OCA-git-bot]

Congratulations, your PR was merged at ba26d48. Thanks a lot for contributing to OCA. ❤️

[SirAionTech]

Ah dovevo usare nobump perché c'è una migrazione, mea culpa, la migrazione 16.0.1.1.1 comunque parte lo stesso quando si aggiorna da 16.0.1.1.0 a 16.0.1.2.0.