8 module attachment metadata

[bealdav]

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:

• ecommerce: exchange with marketplace: ebay, amazon, etc.
• logistics:
  • send delivery order to logistics center, receive a delivery reply,
  • send a notification of goods send to this center and collect a confirmation of good receiving of goods by the logistics
  • collect inventory of the center
• banking: receiving all documents ...

[pedrobaeza]

Why not putting it in OCA/knowledge?

[lmignon]

Can you add OCA as author?

[nhomar]

@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.

[lmignon]

in v8, base is no more required. Moreover, the dependency on mail seems also not required.

[lmignon]

@bealdav Thank you for the contribution. Can you replace the ir_attachment module by modules and move you views into a viewsdirectory to be conform to the expected directory structure?(https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#directories)

[lmignon]

you can remove these lines

[bealdav]

@nhomar good answer.

@lmignon thanks for the review

[pedrobaeza]

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.

[bealdav]

@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

[pedrobaeza]

In the project README, it says:

* provide access to knowledge/documents

so I think it can fit. It's for avoiding the big vault that server-tools is nowadays.

[lmignon]

@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.

[nhomar]

@pedrobaeza about

'''

• provide access to knowledge/documents

'''

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.
Metadata == Technical extention to manipulate attachments (like the less-css converted embebed in odoo for example, it do not depend of document but it manipulate ir.attachment objects).

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.

[pedrobaeza]

@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:

• Subjective
• Overlapping
• Not perfect at all

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

[nhomar]

@pedrobaeza

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.

[pedrobaeza]

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?

[bealdav]

@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.

[nhomar]

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 :-)

[pedrobaeza]

@nhomar, I also read the README, but as you have said about the only feature is the hash, that confused me. I read:

This module extend ir.attachment model with some new fields for a better control
for import and export of files

Do what you want then, as I have said, this is very subjected, so go away what this repo if you want.

[bealdav]

@pedrobaeza you said

why not calling it attachment_hash

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)

[nhomar]

@pedrobaeza

About the readme, yes, there you are right:
@bealdav can you explain the redme a little better in rder to be more clear, Please?

What is a hash and why it is necesary can help the first readers :-)

[pedrobaeza]

@bealdav, thanks for the clarification

[nhomar]

@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 ..

[bealdav]

@nhomar @pedrobaeza If these lines can reconcile you, it'll make me an happy day ;-)

[nhomar]

@bealdav jajaja do not worry... In spanish we both sound worst :-)

[bealdav]

@nhomar maybe you can add a +1 ? Thanks

[tremlin]

Now that the politics seems to have been settled, I add my 👍 cents for code review.

[coveralls]

Coverage increased (+1.5%) to 57.345% when pulling f9984fc on akretion:8-attachm-meta into 5571138 on OCA:8.0.

[coveralls]

Coverage increased (+2.2%) to 58.078% when pulling 5391d08 on akretion:8-attachm-meta into 5571138 on OCA:8.0.

[moylop260]

Could you add a test using a create with 2 records to check if is valid use self.datas instead of attachment.datas?

[hparfr]

👍

[coveralls]

Coverage increased (+2.2%) to 58.078% when pulling 1e3757a on akretion:8-attachm-meta into 5571138 on OCA:8.0.

[sebastienbeau]

👍 LGTM (code review)