[IMP] base: Make it easier to extend Attachment #33392
Conversation
Description of the issue/feature this PR addresses: Current behavior before PR: At the moment it is not easy to add a new storage type without overwriting several methods. The same applies to the method for migrating attachments. Desired behavior after PR is merged: This patch has the goal to improve this. An example that uses these changes is this addition of the storage type Large Object. Large Objec: https://github.com/muk-it/muk_base/blob/12.0/muk_attachment_lobject/models/ir_attachment.py Since the methods: `_get_datas_inital_vals`, `_update_datas_vals`, `_get_datas_clean_vals`, `_clean_datas_after_write` are used it would even work if several storage extensions were installed in the system, because now every module only has to implement its own cleanup method. -- I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr
|
There is another problem that this pull request does not solve: #33456 |
Although I can understand the idea behind this step, it is impossible to call `write` in a subclass without deleting these fields. Maybe it would be a way to control this by context, but since this field is readonly anyway, you could omit it altogether.
if not self.env.context.get('storage', False):
for field in ('file_size', 'checksum'):
vals.pop(field, False)
robodoo
added
CI 🤖
|
Is this ready? You are not only creating overridable methods but also changing the logic on some places which seems odd. |
|
First commit: The only place where the logic should change is in the method odoo/odoo/addons/base/models/ir_attachment.py Line 428 in b1596d1 force_storage and migrate) is basically only for a better overview.
The context ( odoo/odoo/addons/base/models/ir_attachment.py Line 108 in b1596d1 datas triggers that the mimetypes are evaluated again, although nothing has changed. But now only datas are given as value so the recalculation is wrong in many cases.
The splitting of Before: Afterwards: This solves the following scenario: The filestore must be checked each time and the file may have to be deleted. This is not a major problem yet, because an extension could do the same, but if this extension also has a rule like this and there is another extension, it would have to take it into account or know about one another, but in my eyes this is very hard to maintain. Second commit: This commit addresses the problem described here: #33456 I do not find this solution particularly safe from a programming point of view. https://github.com/muk-it/muk_base/blob/12.0/muk_attachment_lobject/models/ir_attachment.py#L111 The super method is also called here, but only works in this one file and not in models inherited from it. https://github.com/odoo/odoo/pull/33392/files#diff-65be9bcf9d09c93a04039c00e1861b9fR248 I hope it is now a little clearer what the background of this pull request is. |
|
@mart-e Did you had the opportunity to read my explanation and is it now clearer what this PR is trying to achieve? I have adapted the PR to the current master branch. |
| @@ -191,31 +231,22 @@ def _compute_datas(self): | |||
| attach.datas = self._file_read(attach.store_fname, bin_size) | |||
| else: | |||
| attach.datas = attach.db_datas | |||
|
|
|||
|
|
|||
| @api.multi | |||
There was a problem hiding this comment.
no need for api.multi it is the default
| record_domain = [ | ||
| '&', ('type', '=', 'binary'), | ||
| '&', storage_domain[self._storage()], | ||
| '|', ('res_field', '=', False), ('res_field', '!=', False) |
There was a problem hiding this comment.
this clause is to avoid the _search override? adding a comment to explain is probably a good idea
| storage = self._storage().upper() | ||
| for index, attach in enumerate(self): | ||
| _logger.info(_("Migrate Attachment %s of %s to %s") % (index + 1, record_count, storage)) | ||
| attach.with_context(migration=True).write({'datas': attach.datas}) |
There was a problem hiding this comment.
and why no giving the existing mimetype in the write values? This way we avoid to introduce a context key to skip verification.
|
Thanks for taking the time to review. For some reason the source repository of this PR is broken and I can't make any more changes to it. So I created a new PR and incorporated the feedback and fixed the merge conflict. PR: #35614 (review) |
Description of the issue/feature this PR addresses:
Fixes #33456
Current behavior before PR:
At the moment it is not easy to add a new storage type without overwriting several methods. The same applies to the method for migrating attachments.
Desired behavior after PR is merged:
This patch has the goal to improve this. An example that uses these changes is this addition of the storage type Large Object.
Large Objec: https://github.com/muk-it/muk_base/blob/12.0/muk_attachment_lobject/models/ir_attachment.py
Since the methods:
_get_datas_inital_vals,_update_datas_vals,_get_datas_clean_vals,_clean_datas_after_writeare used it would even work if several storage extensions were installed in the system, because now every module only has to implement its own cleanup method.--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr