Review: Needs Fixing code review, no test 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. -- https://code.launchpad.net/~savoirfairelinux-openerp/knowledge-addons/document_multiple_records/+merge/206960 Your team Savoir-faire Linux' OpenERP is subscribed to branch lp:~savoirfairelinux-openerp/knowledge-addons/document_multiple_records. -- Mailing list: https://launchpad.net/~savoirfairelinux-openerp Post to : [email protected] Unsubscribe : https://launchpad.net/~savoirfairelinux-openerp More help : https://help.launchpad.net/ListHelp
