Code review comment for lp:~rvalyi/openobject-addons/extensible-mrp

Revision history for this message
Raphaƫl Valyi - http://www.akretion.com (rvalyi) wrote :

Hello,

This an almost backward compatible refactoring. But I grepped, no addons module is broken by it at least.

mrp action_produce_assign_product and action_po_assign methods where respectively creating several production orders and purchase orders while not judging it useful to link them to their original procurement, and passing only the last id at the end of the method. As a consequence, no method could override them properly, for instance to pass some custom options from a procurement to a production order or a purchase order.
A concrete illustration is to be found in the mrp_bom_configuration module (stable extra addons 5.0) we made when I was still at Smile: action_produce_assign_product couldn't be overriden properly and has thus been copied/pasted/hacked. So if two modules do that, then they are incompatible which is bad.

Design consideration:
There are basically two options to give the required info to potential overriders: those hopefully smart hooks such as in https://code.launchpad.net/~akretion-team/openobject-addons/extensible-inventory/+merge/20293 or return 'res' hash of associated objects, such as sale_order#action_invoice_create. I'm not sure which method is best (I the method extension point offers more performance because it optimize database access when overriden, at least when it's done thge way I did). I here opted for the {} thing because returning the id of the last created resource only was just to so plain wrong, so at least we had to remove that and the {} was the only coherent choice here. But I'm very open if somebody suggests to also add some hook like in https://code.launchpad.net/~akretion-team/openobject-addons/extensible-inventory/+merge/20293 or add it and just return True.

Still I wonder if ideally that wouldn't be better to have only one style instead of both as it is currently the case. May be a longer term reflexion for OpenERP SA for future version (though it's sad to let develop modules on a non homogeneous API meanwhile).

Don't hesitate to ask if you have any question.

« Back to merge proposal