Code review comment for lp:~numerigraphe-team/stock-logistic-warehouse/7.0-inventory-location

Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :


Thanks for this contribution, the functionality is great and I'd love to see it in OCA. And thanks for including tests.

A few minor changes are needed to bring the module to the coding standards

* use orm.Model / orm.TransientModel as base classes instead of osv.osv and osv.osv_memory
* model column default values: in OpenERP 7.0, not need to use a lambda for a constant value, just use the constant


* in, I think you should use 'Warehouse Management' for the category (i.e. the same one as the official 'stock' addon
* no space before '!' in english (I'd remove the exclamation marks in exception messages altogether, that the UI job to display an icon)
* in confirm_uninventoried_location_wizard: you should add a 'if context is None: context = {}' at the beginning.
* in confirm_uninventoried_locations: you are ignoring an osv_except exception. Could you add a comment in the code explaining why this is safe? If this is about the "No product in this location" warning raised in the reimplementation of _fill_location_lines, then I think a check that we are ignoring the correct exception should be added. You could for instance subclass except_osv and raise + ignore the subclass. Otherwise I think the code may hide things we do not want to hide (or even fail because of an except_osv

review: Needs Fixing (code review, test)

« Back to merge proposal