avanzosc team mailing list archive

[Alexandre Fayolle - camptocamp <alexandre.fayolle@xxxxxxxxxxxxxx>]

Review: Needs Fixing code review, no test

when porting to openerp v7, we try to ensure the following:

* in __openerp__.py, merge 'update_xml' and 'init__xml' in 'data', and "demo_xml" in "demo"
* use the full package name for openerp imports (e.g. from tools.translate import _ becomes from openerp.tools.translate import _)
* replace osv.osv (resp osv.osv_memory) deprecated base class with orm.Model (resp. orm.TransientModel)
* remove instantiation of model classes
* the default values, when constant, can be defined without using a lambda (e.g. l. 352)

Also: in english, there is no space before an exclamation mark (and I'm personnaly not fond of exclamation marks in error messages). 

Specific comments regarding the code (lines refer to the line numbers in the diff below):

l. 144: method get_lotlist: why are you using strings for keys? Using the id numbers would simplify the code in that function and also in fifo_lot and lifo_lot

l. 182-188: I have not checked the whole execution path. But "manual" seems to be a legal value for lot_type_in, in which case you will call lot_list[cur_lot] with cur_lot == 0 on line 190, which will raise a KeyError. 


l 200: methode fifo_lot: can be rewritten in more maintainable way as following (I'm supposing you follow my advice on using ints in lot_list):

def fifo_lot(self, cr, uid, lot_list, qty, context):
    """get the lot with a non null qty and the earliest date"""
    lot_obj = self.pool.get('stock.production.lot')
    lot_ids = [id for id, qty in lot_list.iteritems() if qty > 0] # keep only the ids with a qty
    if lot_ids:
        domain = [('id', in, lot_ids)]
        earliest_lot_id = lot_obj.search, cr, uid, domain, limit=1, order='date asc', context=context)[0]
    else:
        earlist_lot_id = False
    return earliest_lot_id

Note the docstring, the use of search with a limit and an order (which will speedup things a lot). Same refactoring applies to lifo_lot. 

l.224: remove print statement. Use logging if necessary. 

I did not proof read the po files. 
-- 
https://code.launchpad.net/~avanzosc/openerp-manufacturing/7.0/+merge/147316
Your team Avanzosc Developers is subscribed to branch lp:~avanzosc/openerp-manufacturing/7.0.