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] FIX access error to e-invoices for users who did not create the attachment #3827

Closed
wants to merge 1 commit into from

Conversation

eLBati
Copy link
Member

@eLBati eLBati commented Jan 9, 2024

Risolve #3623 e #3336

Copy link
Contributor

@mrcast mrcast left a comment

Choose a reason for hiding this comment

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

Functional test ok!

@TheMule71
Copy link
Contributor

TheMule71 commented Jan 13, 2024

In generale mi piaceve di più l'idea di creare gli allegati correttamente, e non con res_id e res_model NULL, anche per https://github.com/odoo/odoo/blob/e8834cb00b82e0bee7f6338aef940037a3e49471/odoo/addons/base/models/ir_attachment.py#L444C1-L444C13
che suggerisce in odoo li considerano dei left-over.

Non vorrei che un giorno qualcuno in odoo / OCA / * pensasse che sia una buona idea rimuovere tutti gli allegati "orfani".

@matteoopenf
Copy link
Contributor

In generale mi piaceve di più l'idea di creare gli allegati correttamente, e non con res_id e res_model NULL, anche per https://github.com/odoo/odoo/blob/e8834cb00b82e0bee7f6338aef940037a3e49471/odoo/addons/base/models/ir_attachment.py#L444C1-L444C13 che suggerisce in odoo li considerano dei left-over.

Non vorrei che un giorno qualcuno in odoo / OCA / * pensasse che sia una buona idea rimuovere tutti gli allegati "orfani".

si in effetti e' pericoloso lasciarli orfani perche' diventerebbero soggeti a eliminazione

@eLBati
Copy link
Member Author

eLBati commented Jan 18, 2024

Non vorrei che un giorno qualcuno in odoo / OCA / * pensasse che sia una buona idea rimuovere tutti gli allegati "orfani".

Questo lo vedo difficile, perchè di ir.attachment non collegati a alcun record ce ne sono parecchi (immagini, CSS...)

@eLBati
Copy link
Member Author

eLBati commented Jan 18, 2024

si in effetti e' pericoloso lasciarli orfani perche' diventerebbero soggeti a eliminazione

Non mi risulta, vedi sopra.

Provate a cercare in un DB di produzione, magari con website, attachment dove res_model non è valorizzato

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.

Grazie della PR!
Come mai il commit non rispetta le linee guida? Vedi https://github.com/OCA/odoo-community.org/blob/8afdb5a5ebfd3314e2fc836c9b9f644468ee044e/website/Contribution/CONTRIBUTING.rst#L1114.

Puoi aggiungere un test? Così si evitano regressioni

Comment on lines +10 to +12
@api.model
def check(self, mode, values=None):
for attachment in self:
Copy link
Contributor

Choose a reason for hiding this comment

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

Giusto una nota per dire che normalmente nei metodi api.model self è un recordset vuoto, questo metodo però viene anche chiamato su dei record, ad esempio in https://github.com/odoo/odoo/blob/d752abb24d9b25bb6d10e411961447c8c8e28bba/odoo/addons/base/models/ir_attachment.py#L503

ftpa_attachment = (
self.env["fatturapa.attachment.in"]
.sudo()
.search([("ir_attachment_id", "=", attachment.id)])
Copy link
Contributor

Choose a reason for hiding this comment

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

Cercare all'interno di un loop è molto dispendioso, si può spostare la ricerca fuori dal loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

In questo caso non saprei come

class Attachment(models.Model):
_inherit = "ir.attachment"

@api.model
Copy link
Contributor

Choose a reason for hiding this comment

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

Tutto questo è duplicato di quanto sopra, non sono tante righe ma dici che è possibile ridurre la duplicazione del codice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Modificato

@eLBati eLBati force-pushed the 16.0-fix-3623 branch 2 times, most recently from 1504a9c to 27d81f2 Compare March 1, 2024 13:14
… e-invoices for users who did not create the attachment
@eLBati
Copy link
Member Author

eLBati commented Mar 1, 2024

Come mai il commit non rispetta le linee guida?

modificato

@eLBati
Copy link
Member Author

eLBati commented Mar 1, 2024

Sposto in #3785 a causa del refactoring fatturapa.attachment

@eLBati eLBati closed this Mar 1, 2024
@SirAionTech
Copy link
Contributor

Sposto in #3785 a causa del refactoring fatturapa.attachment

Ok, ma credo abbia più speranze di essere mergiata se resta indipendente.
Come mai il refactoring fatturapa.attachment (immagino tu intenda il commit 3ed6b63 di #3785) impone di spostarla in #3785?

@eLBati
Copy link
Member Author

eLBati commented Mar 4, 2024

Perchè le 2 PR toccavano gli stessi file e da separate, se mergiate, non funzionavano, vedi ad es _check_access_ftpa

@SirAionTech
Copy link
Contributor

Perchè le 2 PR toccavano gli stessi file e da separate, se mergiate, non funzionavano, vedi ad es _check_access_ftpa

Capito grazie, quindi le PR di per sé non hanno problemi: ci sono problemi solo quando cerchi di usarle entrambe

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.

5 participants