stock.picking unlink() regression due to context update

[Albert Cervera i Areny - http://www.NaN-tic.com]

In stock module, stock.picking unlink() function has been changed in revision 2529 introducing a regression if context is not specified. The following diff fixes the problem.

Also note that I created a new context called "ctx" instead of using "context" itself. Modifying the original context is not a good idea because it may have undesired consequences very difficult to track.

     def unlink(self, cr, uid, ids, context=None):
+ if context is None:
+ context = {}
         for pick in self.browse(cr, uid, ids, context=context):
             if pick.state in ['done','cancel']:
                 raise osv.except_osv(_('Error'), _('You cannot remove the picking which is in %s state !')%(pick.state,))
             elif pick.state in ['confirmed','assigned']:
                 ids2 = [move.id for move in pick.move_lines]
- context.update({'call_unlink':True})
- self.pool.get('stock.move').action_cancel(cr, uid, ids2, context)
+ ctx = context.copy()
+ ctx['call_unlink'] = True
+ self.pool.get('stock.move').action_cancel(cr, uid, ids2, ctx)
             else:
                 continue
         return super(stock_picking, self).unlink(cr, uid, ids, context=context)

[Jay Vora (Serpent Consulting Services) (jayvora)]

It has been fixed by revision 2642 <email address hidden>.
Thanks.

[Albert Cervera i Areny - http://www.NaN-tic.com (albert-nan)]

In recent commit 2642, trying to fix this issue you have introduced the same problem somewhere else:

Please, ensure that context is a dictionary before calling ".get()" on it!

Here's the offending portion of commit 2642.

@@ -1344,7 +1351,7 @@

     def unlink(self, cr, uid, ids, context=None):
         for move in self.browse(cr, uid, ids, context=context):
- if move.state != 'draft':
+ if move.state != 'draft' and not context.get('call_unlink',False):
                 raise osv.except_osv(_('UserError'),
                         _('You can only delete draft moves.'))
         return super(stock_move, self).unlink(

[Jay Vora (Serpent Consulting Services) (jayvora)]

Thank you for pointing this out.
It has been improved by revision 2642 <email address hidden>.

[Albert Cervera i Areny - http://www.NaN-tic.com (albert-nan)]

This new patch uses "if not context:" instead of "if context is None:". The latter is the correct one.

Please see https://bugs.launchpad.net/openobject-addons/+bug/525808 for an explanation.

[Jay Vora (Serpent Consulting Services) (jayvora)]

Hello Albert,

Can't we get rid of the similar problems by simply defining this ?

def unlink(self, cr, uid, ids, context={}):

Thanks.

[Albert Cervera i Areny - http://www.NaN-tic.com (albert-nan)]

No we can't. Please, read the bug report I referred to for a full description
of the problem.

A Dimecres, 10 de març de 2010, Jay (Open ERP) va escriure:
> Hello Albert,
>
> Can't we get rid of the similar problems by simply defining this ?
>
> def unlink(self, cr, uid, ids, context={}):
>
> Thanks.
>

--
Albert Cervera i Areny
http://www.NaN-tic.com
Mòbil: [phone]

[Jay Vora (Serpent Consulting Services) (jayvora)]

Hello,

It is possible that context may get no value(so context=None) or some value(context={} or dictionary with keyfrom called function.

At that time, if context is None won't help us.

Better we do like, 'if context is None or (not context):'.

Let me know your views.

Thanks.

[Albert Cervera i Areny - http://www.NaN-tic.com (albert-nan)]

As I already said, everything related to this issue can be found in this bug
report and "if context is None:" is the correct expression:

 https://bugs.launchpad.net/openobject-addons/+bug/525808

It is a matter of correctness to *not* use "not context", because the user may
provide a context he might want to see modified. I think we all agree we don't
want to modify the context, but let's do things the right way.

A Dijous, 11 de març de 2010, Jay (Open ERP) va escriure:
> Hello,
>
> It is possible that context may get no value(so context=None) or some
> value(context={} or dictionary with keyfrom called function.
>
> At that time, if context is None won't help us.
>
> Better we do like, 'if context is None or (not context):'.
>
> Let me know your views.
>
> Thanks.
>
> ** Changed in: openobject-addons
> Status: New => Confirmed
>

--
Albert Cervera i Areny
http://www.NaN-tic.com
Mòbil: [phone]

[Jay Vora (Serpent Consulting Services) (jayvora)]

I agree Albert.
It has been fixed by revision 2654 <email address hidden>.
Thanks for discussing this issue.