-
-
Notifications
You must be signed in to change notification settings - Fork 160
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
[13.0][IMP] Attachment Integration #9
Conversation
Thanks for the improvement, but not sure if this should go in the base module. @etobella what do you think? In any case, better documentation about this (in the README) and UI tips are needed. |
Hi, This feature is related with my comments in #2 I was talking to @luismalta a few hours ago about this feature and he has already developed this proof of concept to open this PR so that you can also suggest ideas. We are thinking about the following features:
Any suggestions are welcome. Best regards |
@luismalta interesting approach, but I think this should go on a new module, like When we integrated the module However, what will happen if two categories or two directories exists for this model? If you are interested, we are currently working on a new The main idea would be to relate one or more storages to a model, so, each root folder of the storage will be related to a single record, now, we will be able to show all the related folders in a simple way. So, a record could have different folders related to it and a user will be able to see the files that its permissions allow. |
The only problem is that we are developing it on 12.0, but it will be ported to 13.0 as soon as it is approved 😉 |
|
Sorry for the misunderstanding, we crossed the information between muk_dms_file and muk_dms_attachment. Our module has nothing to do with muk_dms_field, it is a different approach of how to relate dms.files and records. |
Hi Guys, Talking with @luismalta I found that I made a mistake. I specified it to be implemented in the dms.category model instead of the dms.directory model. @etobella did you see my comment about the roadmap of this feature? Interesting your idea of verifying the user's permissions to the directory model, we can implement it. Do attachments no longer do this automatically? @luismalta is going to test this case. |
About the multiple directories per model, in my opinion, this should not be allowed. This will keep the feature very simple and minimalist avoiding we to have to create one module per model. We can create a default behavior to create a directory per model, with model _description automatically too. Do you think we really need a separate module for this feature? Is it not simpler to leave it in the same DMS and create some settings. |
e396dcc
to
c3c142e
Compare
06f3789
to
e99f61e
Compare
The last commits put the PR in the way suggested by @mileo, missing only the errors of Travis' tests, which I'm working on at the moment. Therefore, I would like to request the review of the PR again. |
We tried a different approach in order to make a similar behavior on #12. Now, we can assign one or more directories to a record and we can use the inherited structure there. I think this simplifies a lot how to manage it. Can you check? Also, a new view has been added in order to make it easy enough |
Maybe we can create a unified view, like: |
|
Yes, being optional fields, if you want, you don't fill those fields and let that integration. You are right about permissions, but the aim of this is to fill flows that are done by lots of persons in one place, and summarize them automatically in the DMS by others. |
I created a migration to dms_field: PR # 17. |
Hi, Maybe need to make some changes to aprrove this PR:
This functionallity is very interesting, good job!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this changes, travis OK
dms/models/dms_file.py
Outdated
default_directory_id = self.env["dms.directory"].search( | ||
[("id", "=", self._context["default_directory_id"])] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change is necessary to solved travis
default_directory_id = self.env["dms.directory"].with_user(SUPERUSER_ID).browse(self._context["default_directory_id"])
IMO this is a problem with _search function in directory that is not correct (isn't override and is needed) because not apply groups correctlly (not in this PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why searching instead of a browse
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why searching instead of a
browse
?
Thanks, update my comments
dms/models/dms_file.py
Outdated
directory_id = self.env["dms.directory"].search( | ||
[("id", "=", vals["directory_id"])] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change is necessary to solved travis
directory_id = self.env["dms.directory"].with_user(SUPERUSER_ID).browse(vals["directory_id"])
IMO this is a problem with _search function in directory that is not correct (isn't override and is needed) because not apply groups correctlly (not in this PR).
@victoralmau Thanks a lot for the help. I will work on these changes to make PR approvable |
f335162
to
82772c9
Compare
Add exactly same functionallity in V12: #24 |
Add test in similar PR #24 and maybe is interesting to add it and increase codecov results. |
directory_fields = { | ||
"name": attachment_id.res_name.replace("/", "-"), | ||
"is_root_directory": True, | ||
"parent_id": False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldnt it be better to add all these directories under a common model directory Contacts Root
for example instead of making every record root directory?
if self.storage_id.save_type == "file": | ||
new_vals["content_file"] = self.content | ||
elif self.storage_id.save_type == "attachment": | ||
new_vals["attachment_id"] = self.content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's very strange because in V12 don't work #24 (comment).
Solved (V12) with this changes:
if self.storage_id.save_type == "file": | |
new_vals["content_file"] = self.content | |
elif self.storage_id.save_type == "attachment": | |
new_vals["attachment_id"] = self.content | |
if self.storage_id.save_type in ["file", "attachment"]: | |
new_vals["content_file"] = self.content |
if dms_field and dms_field.state == "installed": | ||
directory_fields["res_id"] = attachment_id.res_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's unnecessary because of (in the future) other addon use it, it's not necessary to add new field res_id
in directory because record_ref
contain it already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the standard is to have res_model
and res_id
field. The record_ref
can be a computed one, composed over the other 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but actually don't exists this fields.
Do you think that it's necessary to remove record_ref
and add: res_model
and res_id
fields?
It's necessary to add this fields only in directory or in file too? (now group all attachment in only one directory, it's enought?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to go with res_model
and res_id
fields, and the other one computed (stored). If res_model
can be computed stored as well from the dms.directory
, perfect. I understand that dms.directory
can contain res_model
and optionally res_id
, isn't it?
def _create_record_dir(self, storage_id, attachment_id): | ||
dms_field = self.env["ir.module.module"].search([("name", "=", "dms_field")]) | ||
|
||
directory_fields = { | ||
"name": attachment_id.res_name.replace("/", "-"), | ||
"is_root_directory": True, | ||
"parent_id": False, | ||
"root_storage_id": storage_id.id, | ||
} | ||
|
||
if dms_field and dms_field.state == "installed": | ||
directory_fields["res_id"] = attachment_id.res_id | ||
|
||
return self.env["dms.directory"].create(directory_fields) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's unnecessary but need to change some things in _create
@api.model | ||
def create(self, vals): | ||
if "dms_file" in vals and vals["dms_file"]: | ||
del vals["dms_file"] | ||
return super(IrAttachment, self).create(vals) | ||
else: | ||
attachment_id = super(IrAttachment, self).create(vals) | ||
if "res_model" in vals: | ||
storage_id = self.env["dms.storage"].search( | ||
[("model_id", "=", vals["res_model"])] | ||
) | ||
|
||
if storage_id and storage_id.save_type == "attachment": | ||
record_directory = self.env["dms.directory"].search( | ||
[("complete_name", "=", attachment_id.res_name)] | ||
) | ||
if not record_directory: | ||
record_directory = self._create_record_dir( | ||
storage_id, attachment_id | ||
) | ||
|
||
attachments_dir_id = record_directory.child_directory_ids.filtered( | ||
lambda r: r.name == "Attachments" | ||
) | ||
|
||
if not attachments_dir_id: | ||
attachments_dir_id = self.env["dms.directory"].create( | ||
{ | ||
"name": "Attachments", | ||
"parent_id": record_directory.id, | ||
"storage_id": storage_id.id, | ||
"record_ref": "{},{}".format( | ||
attachment_id.res_model, attachment_id.res_id | ||
), | ||
} | ||
) | ||
file_name = self._dms_file_name(attachments_dir_id, vals["name"]) | ||
|
||
self.env["dms.file"].create( | ||
{ | ||
"name": file_name, | ||
"directory_id": attachments_dir_id.id, | ||
"attachment_id": attachment_id.id, | ||
"record_ref": "{},{}".format( | ||
attachment_id.res_model, attachment_id.res_id | ||
), | ||
} | ||
) | ||
|
||
return attachment_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In V12 change it for some reasons:
- It's possible to exists some attachment storage
- It's necessary to auto-create root directory related to model and storage
- It's necessary to create sub directory (related to root directory previously auto-create) related to model and res_id with record_ref field.
- Dms fIeld need to related to previous directory.
@api.model | |
def create(self, vals): | |
if "dms_file" in vals and vals["dms_file"]: | |
del vals["dms_file"] | |
return super(IrAttachment, self).create(vals) | |
else: | |
attachment_id = super(IrAttachment, self).create(vals) | |
if "res_model" in vals: | |
storage_id = self.env["dms.storage"].search( | |
[("model_id", "=", vals["res_model"])] | |
) | |
if storage_id and storage_id.save_type == "attachment": | |
record_directory = self.env["dms.directory"].search( | |
[("complete_name", "=", attachment_id.res_name)] | |
) | |
if not record_directory: | |
record_directory = self._create_record_dir( | |
storage_id, attachment_id | |
) | |
attachments_dir_id = record_directory.child_directory_ids.filtered( | |
lambda r: r.name == "Attachments" | |
) | |
if not attachments_dir_id: | |
attachments_dir_id = self.env["dms.directory"].create( | |
{ | |
"name": "Attachments", | |
"parent_id": record_directory.id, | |
"storage_id": storage_id.id, | |
"record_ref": "{},{}".format( | |
attachment_id.res_model, attachment_id.res_id | |
), | |
} | |
) | |
file_name = self._dms_file_name(attachments_dir_id, vals["name"]) | |
self.env["dms.file"].create( | |
{ | |
"name": file_name, | |
"directory_id": attachments_dir_id.id, | |
"attachment_id": attachment_id.id, | |
"record_ref": "{},{}".format( | |
attachment_id.res_model, attachment_id.res_id | |
), | |
} | |
) | |
return attachment_id | |
@api.model | |
def create(self, vals): | |
if "dms_file" in vals and vals["dms_file"]: | |
del vals["dms_file"] | |
return super(IrAttachment, self).create(vals) | |
else: | |
attachment_id = super(IrAttachment, self).create(vals) | |
if attachment_id.res_model and attachment_id.res_id: | |
storage_ids = self.env["dms.storage"].search( | |
[ | |
("model_id.model", "=", attachment_id.res_model), | |
("save_type", "=", "attachment"), | |
] | |
) | |
if storage_ids: | |
record_ref = "{},{}".format( | |
attachment_id.res_model, attachment_id.res_id, | |
) | |
directory_ids = self.env["dms.directory"].search( | |
[ | |
("storage_id", "in", storage_ids.ids), | |
("record_ref", "=", record_ref), | |
] | |
) | |
if directory_ids: | |
directory_id = directory_ids[0] | |
else: | |
root_directory_ids = self.env["dms.directory"].search( | |
[ | |
("storage_id", "=", storage_ids[0].id), | |
("is_root_directory", "=", True), | |
] | |
) | |
if root_directory_ids: | |
root_directory_id = root_directory_ids[0] | |
else: | |
root_directory_id = self.env["dms.directory"].create( | |
{ | |
"name": attachment_id.res_model, | |
"is_root_directory": True, | |
"storage_id": storage_ids[0].id, | |
"root_storage_id": storage_ids[0].id, | |
} | |
) | |
# Create directory related to record_ref | |
directory_id = self.env["dms.directory"].create( | |
{ | |
"name": attachment_id.res_name, | |
"parent_id": root_directory_id.id, | |
"storage_id": root_directory_id.storage_id.id, | |
"record_ref": record_ref, | |
} | |
) | |
# File operations + dms_file create | |
file_name = self._dms_file_name(directory_id, vals["name"]) | |
self.env["dms.file"].create( | |
{ | |
"name": file_name, | |
"directory_id": directory_id.id, | |
"attachment_id": attachment_id.id, | |
"record_ref": record_ref, | |
} | |
) | |
return attachment_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally need to create test about it with some checks:
-
Migration process
-
Create
dms_file
and check if attachment create it and related it -
Create attachment related to model previously configured in storage and check if
dms_file
work fine
In V12 #24 exists test about it.
I was just going to ask in contributors mailing list about existence of module that would allow to categorize and/or add tags to attachments. This PR serves nicely for that purpose I guess. What I miss would be integration back to attachment list. What I mean is that I would like to have let's say product.template's attachments displayed in a tree view with category and tags visible/editable. I'd like to use the category and/or tags later to selectively display some of the attachments to e-commerce for example (which would be up to its own module). Is this feasible? |
If i understand it correctly this PR don't allow it. Maybe, if you prefer create a new issue about it and explain a lot and everybody comment about it and the best way to do. |
I guess @victoralmau is right. What do you think @luismalta ? |
@luismalta @mileo do you agree then to supersed this with #37 ? |
Yes, Would it be possible to put @luismalta as a contributor on #37? |
It's already included with all its content on the first commit of that PR 😃 Closing then this as continuing in #37 Thanks all |
This feature aims to integrate dms with ir.attachment. That way, every time a new attachment is created, a dms.file is also created with a category according to the model the attachment is in.