openerp-community-reviewer team mailing list archive

["Guewen Baconnier @ Camptocamp" <guewen.baconnier@xxxxxxxxxxxxxx>]

Guewen Baconnier @ Camptocamp has proposed merging lp:~camptocamp/openerp-fiscal-rules/7.0-wrong-onchange-1255918 into lp:openerp-fiscal-rules.

Commit message:
[FIX] kwargs is always None, fiscal position rule never applied

Requested reviews:
  Account Core Editors (account-core-editors)
Related bugs:
  Bug #1255918 in openerp-fiscal-rules: "kwargs always NoneType"
  https://bugs.launchpad.net/openerp-fiscal-rules/+bug/1255918

For more details, see:
https://code.launchpad.net/~camptocamp/openerp-fiscal-rules/7.0-wrong-onchange-1255918/+merge/197376

Fixes lp:1255918

The onchange_partner_id method had a couple of errors:

* The condition on shop_id to not apply the rules was reversed [0]
* {}.update() returns None, so kwargs was always None
* context was set as a value of context, resulting in a recursive data structure:
{'context': <Recursion on dict with id=120528752>, ...}

I took the decision here to no longer transform the context into **kwargs because it seems to me a hazardous operation. I think that the kwargs should be build solely with determined keys. If someone has a good reason to keep passing the context as **kwargs, please let me know.

[0] http://bazaar.launchpad.net/~account-core-editors/openerp-fiscal-rules/oerp6.1-stable/view/head:/account_fiscal_position_rule_sale/sale.py#L35
-- 
https://code.launchpad.net/~camptocamp/openerp-fiscal-rules/7.0-wrong-onchange-1255918/+merge/197376
Your team Account Core Editors is requested to review the proposed merge of lp:~camptocamp/openerp-fiscal-rules/7.0-wrong-onchange-1255918 into lp:openerp-fiscal-rules.

=== modified file 'account_fiscal_position_rule_sale/sale.py'
--- account_fiscal_position_rule_sale/sale.py	2013-10-18 08:59:22 +0000
+++ account_fiscal_position_rule_sale/sale.py	2013-12-02 14:15:41 +0000
@@ -43,23 +43,23 @@
         return fp_rule_obj.apply_fiscal_mapping(cr, uid, result, **kwargs)
 
     def onchange_partner_id(self, cr, uid, ids, partner_id, context=None):
-
         if not context:
             context = {}
 
         result = super(sale_order, self).onchange_partner_id(
-            cr, uid, ids, partner_id, context)
+            cr, uid, ids, partner_id, context=context)
 
-        if context.get('shop_id', False):
+        if not context.get('shop_id'):
             return result
 
-        kwargs = context.update({
-            'shop_id': context.get('shop_id', False),
+        values = result['value']
+        kwargs = {
+            'shop_id': context['shop_id'],
             'partner_id': partner_id,
-            'partner_invoice_id': result['value'].get('partner_invoice_id', False),
-            'partner_shipping_id': result['value'].get('partner_shipping_id', False),
+            'partner_invoice_id': values.get('partner_invoice_id', False),
+            'partner_shipping_id': values.get('partner_shipping_id', False),
             'context': context
-        })
+        }
         return self._fiscal_position_map(cr, uid, result, **kwargs)
 
     def onchange_address_id(self, cr, uid, ids, partner_invoice_id,