Code review comment for lp:~savoirfairelinux-openerp/knowledge-addons/document_multiple_records

Please fix according to https://code.launchpad.net/~savoirfairelinux-openerp/knowledge-addons/add_document_multiple_records/+merge/205249/comments/481345

Holger Brunn (Therp) (hbrunn) wrote on 2014-02-10:

I think you should follow the rule to put models into the subdir model, with one file per model.

#29 why no local import here?
#135ff could you explain those lines? To me it seems we leave stuff in the database there that we don't want
#363 comment
#377 this text seems quite misleading. How about 'Add existing document/attachment'?
$470 ir.attachment.wizard seems a very generic name, I think you should use something more specific to what you do.

Further I don't see where you do any refcount. How do you take care that if attachment A is linked to records R1, R2 and R3, A stays if I delete R1 and R2, but will be deleted when I delete R3?

And how do you deal with attachments being only visible to people who have read permissions on the document given in res_model/res_id? I think you'll have to override that to also take the other linked records into account.

For usability, I'd also add a function field that combines res_model and res_id to a reference field.