Merge lp:~openerp-dev/openobject-addons/6.0-bug_725908-ach into lp:openobject-addons/6.0

Proposed by Anup(SerpentCS)
Status: Merged
Merged at revision: 4534
Proposed branch: lp:~openerp-dev/openobject-addons/6.0-bug_725908-ach
Merge into: lp:openobject-addons/6.0
Diff against target: 139 lines (+59/-51)
1 file modified
stock/wizard/stock_fill_inventory.py (+59/-51)
To merge this branch: bzr merge lp:~openerp-dev/openobject-addons/6.0-bug_725908-ach
Reviewer Review Type Date Requested Status
Jay Vora (Serpent Consulting Services) (community) Approve
Review via email: mp+56554@code.launchpad.net

Description of the change

Hello,

   The problem with the Stock Fill Inventory has been fixed by this solution. Now all the stock moves will be imported to the Inventory with production lots.

Thanks.

To post a comment you must log in.
Revision history for this message
Rucha (Open ERP) (rpa-openerp) wrote :
Revision history for this message
Jay Vora (Serpent Consulting Services) (jayvora) wrote :

Anup,

Thanks to you and Kirti for the fix. It was really a good one.

Kindly go through these important points to improve here:

1. Variable naming convention.(Do not opt for i,j,k,v).
2. Wrong indentation on your proposal Line 21.
3. Just a note : Whenever we use child_of operator, go for the right operant with [] of ids. This doesn't create any problem as this has been covered well recently in expression.py, but its a good practice.(Refer to line 31)
4. Does the order='id' give you a different result when order='parent_left'? (Refer to Line 31)
5. Line 61 should be location_dest_id = location. Think over it.
6. Use a shorthand operator. (Refer to line 63)
7. Improve the notification, it could be notification or warning(Line 70).
8. You don't need line 73.
9. Why do we need len(line_ids)? (Line 90).
10. Regarding last 2 for loops, I am not 100% sure whether its an optimized solution or not.
11. Regarding Line 75, are you sure we always have one inventory to check? I doubt because, we can use this wizard for multiple inventories. And its always wise to check before writing list[0].

Correct me If I missed anything.

I am investigating more in order to get the best suited fix.

Thanks.

review: Needs Fixing
Revision history for this message
Jay Vora (Serpent Consulting Services) (jayvora) wrote :

Corrections:

4. Does the order='id' give you a different result than order='parent_left'? (Refer to Line 31)

Thanks.

Revision history for this message
Anup(SerpentCS) (anup-serpent) wrote :

Hello Jay,

   Thanks a lot for a detailed investigation. I have fixed many of the issues mentioned by you like indentations and naming conventions, child_of, shorthand operator,notifications etc.

4.Currently order = 'parent_left' is giving the same result as id. But IMHO it's better to keep id rather than parent_left.

5. You are right we don't need that line. It saves many read calls and we don't even need that variable.

11. Well The wizard should be used with a single inventory only. You can not use it with multiple inventories. I have kept a notification if more than one inventory is selected. So it solves the issue.

We even don't need the lines 38-42. When will it be the case when ids will not be there?

Please share your views.

Thanks.

Revision history for this message
Jay Vora (Serpent Consulting Services) (jayvora) wrote :

Thanks for the corrections Anup.

However, I am investigating 3 changes here:
1. Return True would be good for view_init.
2. ids = ids[0] (Line 44)
3. order='id' (Line 56).

Thanks again.

review: Approve
Revision history for this message
Jay Vora (Serpent Consulting Services) (jayvora) wrote :

Anup,

Corrections have been made and the branch has been merged to stable 6.0.2.

Thanks for all your efforts.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'stock/wizard/stock_fill_inventory.py'
--- stock/wizard/stock_fill_inventory.py 2011-01-14 00:11:01 +0000
+++ stock/wizard/stock_fill_inventory.py 2011-04-18 07:33:37 +0000
@@ -39,14 +39,18 @@
39 @param context: A standard dictionary39 @param context: A standard dictionary
40 @return: New arch of view with new columns.40 @return: New arch of view with new columns.
41 """41 """
42 if context==None:42 if context is None:
43 context={}43 context = {}
44 res = super(stock_fill_inventory, self).view_init(cr, uid, fields_list, context=context)44 super(stock_fill_inventory, self).view_init(cr, uid, fields_list, context=context)
45
46 if len(context['active_ids']) > 1:
47 raise osv.except_osv(_('Error!'), _('You can not perform this operation on more than one Stock Inventories'))
48
45 if context.get('active_id', False):49 if context.get('active_id', False):
46 stock = self.pool.get('stock.inventory').browse(cr, uid, context.get('active_id', False))50 stock = self.pool.get('stock.inventory').browse(cr, uid, context.get('active_id', False))
51
47 if stock.state=='done':52 if stock.state=='done':
48 raise osv.except_osv(_('Error!'), _('Stock Inventory is done'))53 raise osv.except_osv(_('Error!'), _('Stock Inventory is already Validated.'))
49 True
5054
51 def fill_inventory(self, cr, uid, ids, context=None):55 def fill_inventory(self, cr, uid, ids, context=None):
52 """ To Import stock inventory according to products available in the selected locations.56 """ To Import stock inventory according to products available in the selected locations.
@@ -58,61 +62,65 @@
58 @return:62 @return:
59 """63 """
60 if context is None:64 if context is None:
61 context = {} 65 context = {}
66
62 inventory_line_obj = self.pool.get('stock.inventory.line')67 inventory_line_obj = self.pool.get('stock.inventory.line')
63 location_obj = self.pool.get('stock.location')68 location_obj = self.pool.get('stock.location')
64 product_obj = self.pool.get('product.product')69 product_obj = self.pool.get('product.product')
65 stock_location_obj = self.pool.get('stock.location')70 move_obj = self.pool.get('stock.move')
66 if ids and len(ids): 71 ids = ids[0]
67 ids = ids[0]72
68 else:
69 return {'type': 'ir.actions.act_window_close'}
70 fill_inventory = self.browse(cr, uid, ids, context=context)73 fill_inventory = self.browse(cr, uid, ids, context=context)
71 res = {}74 res = {}
72 res_location = {}75 res_location = {}
76
73 if fill_inventory.recursive :77 if fill_inventory.recursive :
74 location_ids = location_obj.search(cr, uid, [('location_id',78 location_ids = location_obj.search(cr, uid, [('location_id',
75 'child_of', fill_inventory.location_id.id)])79 'child_of', [fill_inventory.location_id.id])], order="id")
76 for location in location_ids :
77 res = location_obj._product_get(cr, uid, location)
78 res_location[location] = res
79 else:80 else:
80 context.update({'compute_child': False})81 location_ids = [fill_inventory.location_id.id]
81 res = location_obj._product_get(cr, uid,82
82 fill_inventory.location_id.id, context=context)83 res = {}
83 res_location[fill_inventory.location_id.id] = res84 flag = False
84 85
85 product_ids = []86 for location in location_ids:
86 for location in res_location.keys():87 datas = {}
87 res = res_location[location]88 res[location] = {}
88 for product_id in res.keys():89 move_ids = move_obj.search(cr, uid, [('location_dest_id','=',location),('state','=','done')], context=context)
89 prod = product_obj.browse(cr, uid, product_id, context=context)90
90 uom = prod.uom_id.id91 for move in move_obj.browse(cr, uid, move_ids, context=context):
91 context.update(uom=uom, compute_child=False)92 lot_id = move.prodlot_id.id
92 amount = stock_location_obj._product_get(cr, uid,93 prod_id = move.product_id.id
93 location, [product_id], context=context)[product_id]94 qty = move.product_qty
94 if(amount):95
95 if fill_inventory.set_stock_zero:96 if datas.get((prod_id, lot_id)):
96 amount = 0 97 qty += datas[(prod_id, lot_id)]['product_qty']
97 line_ids=inventory_line_obj.search(cr, uid,98
98 [('inventory_id', '=', context['active_ids']),99 datas[(prod_id, lot_id)] = {'product_id': prod_id, 'location_id': location, 'product_qty': qty, 'product_uom': move.product_id.uom_id.id, 'prod_lot_id': lot_id}
99 ('location_id', '=', location),100
100 ('product_id', '=', product_id),101 if datas:
101 ('product_uom', '=', uom),102 flag = True
102 ('product_qty', '=', amount)])103 res[location] = datas
103 if not len(line_ids):104
104 inventory_line = {105 if not flag:
105 'inventory_id': context['active_ids'][0],106 raise osv.except_osv(_('Warning !'), _('No product in this location.'))
106 'location_id': location,107
107 'product_id': product_id,108 for stock_move in res.values():
108 'product_uom': uom,109 for stock_move_details in stock_move.values():
109 'product_qty': amount110 stock_move_details.update({'inventory_id': context['active_ids'][0]})
110 }111 domain = []
111 inventory_line_obj.create(cr, uid, inventory_line)112
112 product_ids.append(product_id)113 if fill_inventory.set_stock_zero:
113114 stock_move_details.update({'product_qty': 0})
114 if(len(product_ids) == 0):115
115 raise osv.except_osv(_('Message !'), _('No product in this location.'))116 for field, value in stock_move_details.items():
117 domain.append((field, '=', value))
118
119 line_ids = inventory_line_obj.search(cr, uid, domain, context=context)
120
121 if not line_ids:
122 inventory_line_obj.create(cr, uid, stock_move_details, context=context)
123
116 return {'type': 'ir.actions.act_window_close'}124 return {'type': 'ir.actions.act_window_close'}
117125
118stock_fill_inventory()126stock_fill_inventory()