8 module attachment metadata #376
Conversation
|
Why not putting it in OCA/knowledge? |
|
|
||
| {'name': 'attachment_metadata', | ||
| 'version': '0.0.1', | ||
| 'author': 'Akretion', |
There was a problem hiding this comment.
Can you add OCA as author?
|
@pedrobaeza ir.attachment model is a core feature and this module can be used for several things different from the Knowledge context. I think it is fine here. Regards. +1 for be here. No functionally tested yet! just CPR. |
| internal and external hash for coherence verification | ||
| """, | ||
| 'depends': [ | ||
| 'base', |
There was a problem hiding this comment.
in v8, base is no more required. Moreover, the dependency on mail seems also not required.
|
@bealdav Thank you for the contribution. Can you replace the |
| :alt: Try me on Runbot | ||
| :target: https://runbot.odoo-community.org/runbot/{repo_id}/8.0 | ||
|
|
||
| .. repo_id is available in https://github.com/OCA/maintainer-tools/blob/master/tools/repos_with_ids.txt |
There was a problem hiding this comment.
you can remove these lines
19fcf0f to
32c3fb6
Compare
|
Being a base model doesn't prevent to be ordered by "area". Modules like https://github.com/OCA/knowledge/tree/8.0/attachment_preview or https://github.com/OCA/knowledge/tree/8.0/attachments_to_filesystem, that also touch this model, are in OCA/knowledge. Please reconsider the project where being hosted. |
|
@pedrobaeza when I read knowledge, I think https://en.wikipedia.org/wiki/Knowledge_management. I think other people could think of the same way than me. This module doesn't provide any knowledge nor tool for knowledge. It's confusing, I think. But It's not a problem for me to move to another repo (I've just done it)), if it makes sense for whole OCA. Thanks |
|
In the project README, it says: so I think it can fit. It's for avoiding the big vault that server-tools is nowadays. |
|
@pedrobaeza ir.attachment is a support for knowledge management not the way to do knowledge management 😏 . Nevertheless if all the modules that extends ir.attachment are in knowledge I'm not against to put this module in knowledge. |
|
@pedrobaeza about '''
''' Again. I can need the meta-data in ir.attachment without any feature of document management system and knowledge management system. I can be agree because it is so tired this discussions but conceptually this is a technical tool not a knowledge feature: Knowledge == Content. Let me give an example where this module should not need knowledge: Electronic Invoices can need meta data, but I do not need all the knowledge repository declared to manipulate such electronic documents (And I do not want to) our ir.attachment are linked directly from the invoice. I hope Ii explained better the topic. Knowledge != All ir.attachment related features. |
|
@nhomar, I'm not asking you to install all the knowledge modules. It's only a way to classify modules according an "area". We all know that this classification is:
But in this case, we can avoid to have another more module for this bloated module. Anyway, if you only want some metadata in the object, I prefer this approach: #313 |
|
Whatever, you are the expert, but my point is not subjective you are talking about 2 absolutly different topic. MEtadata of a file is the Hash information (unique identifier for a file) used internally in odoo already this module is only enabling it to be shown in the frontend. Your arguments in this topic are pointless and completelly based on a misunderstood of the feature itself. BTW: In -NO- place we said that we will divide the repositories per object (which is what you are proposing) the repositories are divided by functional approach (and this module is NOT a knowledge related functional approach and the one you mentioned in #313 is not related at all with this feature). But again my friend "you are the expert.". EOF on my side here this discusion is simply a power fight ;-) this for me is a technical feature which extend the original behavior of the ir.attachment object to show the sha.... nothing related with knowledge. |
|
No, I'm not trying to force anyone, and it's not a question of who's the man with the bigger eggs 😛 . Just giving my reasonings, but I'm only guessing about the module name. If the feature is only a hash, why not calling it attachment_hash? |
|
@pedrobaeza notice the first purpose (not the last) of this module is to be used with #377, then it's a small module which can be extended by other needs. |
|
because it is a "metadata" for a file I supose, but if you did not read the code how can you be opinated? Some times I did not understand jeje ;-) For me the name was clear (with simpl read the name) I read the name and inmediatly look for the information I just described and it was there perfectly done :-) |
|
@nhomar, I also read the README, but as you have said about the only feature is the hash, that confused me. I read: Do what you want then, as I have said, this is very subjected, so go away what this repo if you want. |
|
@pedrobaeza you said
Here is the reason https://github.com/akretion/server-tools/blob/8-file-loc/external_file_location/attachment.py I don't want to rename the module each time I add a field (metadata field) |
|
About the readme, yes, there you are right: What is a hash and why it is necesary can help the first readers :-) |
|
@bealdav, thanks for the clarification |
|
@bealdav Yes it can be good. Just FYI, we made internally a PoC to sync with S3 (not finished just a PoC) and we include exactly the same feature inside the module, that's why I think it brings some good value to everybody as a base module. An example with your other module can help everybody understand correctly the need of the approach. Thanks .. |
|
@nhomar @pedrobaeza If these lines can reconcile you, it'll make me an happy day ;-) |
|
@bealdav jajaja do not worry... In spanish we both sound worst :-) |
|
@nhomar maybe you can add a +1 ? Thanks |
|
Now that the politics seems to have been settled, I add my 👍 cents for code review. |
| for attachment in self: | ||
| if attachment.datas: | ||
| attachment.internal_hash = hashlib.md5( | ||
| b64decode(self.datas)).hexdigest() |
There was a problem hiding this comment.
Could you add a test using a create with 2 records to check if is valid use self.datas instead of attachment.datas?
|
👍 |
|
👍 LGTM (code review) |
Syncing from upstream OCA/server-tools (12.0)
This module add some fields to standard ir.attachment with a new model to ensure to a good quality in exchange of files (import/export).
Then it's used by external_file_location module define here #377
Example of kind files which can use this module: