[16.0][IMP] database_cleanup: purge orphaned attachments#3566
[16.0][IMP] database_cleanup: purge orphaned attachments#3566
Conversation
Aldeigja
force-pushed
the
16.0-t5258-database_cleanup-orphaned-attachments-clean
branch
from
699d8f0 to
ab70051
Compare
StefanRijnhart
left a comment
StefanRijnhart
left a comment
There was a problem hiding this comment.
Thanks! Judging from the name of the PR and the patterns from this module, I would have expected signalling attachments where the model no longer exists or the res_id is not present as a record. This makes sense as well, but to make it explicit what the issue is with each attachment (and to prepare for extension with other possible error conditions), would you mind adding a reason field to the wizard line model as is done in cleanup.purge.line, with for now a fixed reason like 'File missing in filestore'?
| wizard = env["cleanup.purge.wizard.attachment"].create( | ||
| { | ||
| "purge_line_ids": [ | ||
| ( |
There was a problem hiding this comment.
Can you use the more modern fields.Command.create?
There was a problem hiding this comment.
Thanks for pointing that out. Fixed.
| IrModel = env.registry["ir.attachment"] | ||
| original_unlink = IrModel.unlink | ||
|
|
||
| def patched_unlink(self): |
There was a problem hiding this comment.
Why does unlink need to be patched? Isn't the exists check enough for the validation of the test?
There was a problem hiding this comment.
The patch is necessary because this test validates the error-resilience path in the purge(). Regardless of whether unlink() raises a UserError for one attachment, the remaining attachments should still be purged. Without the patch, both attachments would be successfully deleted, and we would miss the exception branch entirely.
Aldeigja
force-pushed
the
16.0-t5258-database_cleanup-orphaned-attachments-clean
branch
from
b372147 to
95fb498
Compare
StefanRijnhart
left a comment
StefanRijnhart
left a comment
There was a problem hiding this comment.
Thanks for the updates! Please squash commits.
| attachment_id = fields.Many2one("ir.attachment") | ||
| reason = fields.Selection( | ||
| [ | ||
| (REASON_MISSING_FILE, ("File missing in filestore")), |
There was a problem hiding this comment.
| (REASON_MISSING_FILE, ("File missing in filestore")), | |
| (REASON_MISSING_FILE, "File missing in filestore"), |
| "Attachment #%s cannot be deleted: %s", | ||
| attach.id, | ||
| str(exc), | ||
| ) |
There was a problem hiding this comment.
Given that you built this resilience, you will want to visualize this as well. Can you store the error message in a new column on the line model, and add it to the list view with optional="hide".
There was a problem hiding this comment.
Thank you, I've added the field and updated the view as well.
| res.append( | ||
| ( | ||
| 0, | ||
| 0, |
Add wizard to list ir.attachment records whose store_fname points to a missing file on disk when storage is "file", and allow purging those rows. task 5258
Aldeigja
force-pushed
the
16.0-t5258-database_cleanup-orphaned-attachments-clean
branch
from
95fb498 to
e8b0061
Compare
2 participants
Add wizard to list ir.attachment records whose store_fname points to a missing file on disk when storage is "file", and allow purging those rows.