Merge lp:~camptocamp/openerp-fiscal-rules/7.0-wrong-onchange-1255918 into lp:openerp-fiscal-rules

[Guewen Baconnier @ Camptocamp]

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

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

[Sébastien BEAU - http://www.akretion.com (sebastien.beau)]

LGTM, I think too that it's better to no transform the context into kwargs.

review: Approve (no test, code review)

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

Hello Guewen,

I agree with the 2 first points.

However I have some doubts about your point 3) (about the context):
I see that in our Brazilian localization we were using the trick of injecting context values into the _fiscal_position_map **kwargs so that we could inject our required extra fiscal parameters without the need to change the signature and hence risk incompatibility with many modules (that would rely on the normal signature).
see https://github.com/openerpbrasil/l10n_br_core/blob/7.0/l10n_br_sale/sale_view.xml#L71

About if this is "hazardous": well, this get possibly unexpected values into the kwargs, but I would say these would be keys we don't care, just like you get many params in an HTTP request but care only about some parameters for some function. My whole point is that OpenERP on_change should be dictionaries instead of position args, so I'm trying here to take advantage of the existing kwargs system here even if it's not 100% clean.
And in the case, there are parameters we care about, we can take control back by forcing a defined values in this code before calling _fiscal_position_map. So I'm not sure if it's that dangerous practically.

On the contrary, I'm absolutely sure that changing the signature of this onchange in our localization is a bomb, making it incompatible with many modules unless we build a combinatorial explosion of glue modules (we already suffer a lot of the situation with the other on_changes not passing a hackable context).

Indeed, may module would override onchange_partner_id without touching the signature in the view. Today it just works. If we shouldn't not use the context anymore for wrapping our params, tomorrow we will need to work around every single case to ensure our override with more params will be called first.

So what do you think?
if the most annoying thing is the recursive context, couldn't we just avoid it by copying the context before getting it into the kwargs?

Regards.

review: Needs Information

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

Hello,

On 12/03/2013 09:12 PM, Raphaël Valyi - http://www.akretion.com wrote:
> Review: Needs Information
>
> Hello Guewen,
>
> I agree with the 2 first points.
>
> However I have some doubts about your point 3) (about the context):
> I see that in our Brazilian localization we were using the trick of injecting context values into the _fiscal_position_map **kwargs so that we could inject our required extra fiscal parameters without the need to change the signature and hence risk incompatibility with many modules (that would rely on the normal signature).
> see https://github.com/openerpbrasil/l10n_br_core/blob/7.0/l10n_br_sale/sale_view.xml#L71
>

Are your sure it was used? This code was not reached when it should be,
and was reached when it shouldn't but was failing. I don't want to keep
a such a hack if no longer necessary.

> About if this is "hazardous": well, this get possibly unexpected values into the kwargs, but I would say these would be keys we don't care, just like you get many params in an HTTP request but care only about some parameters for some function. My whole point is that OpenERP on_change should be dictionaries instead of position args, so I'm trying here to take advantage of the existing kwargs system here even if it's not 100% clean.
> And in the case, there are parameters we care about, we can take control back by forcing a defined values in this code before calling _fiscal_position_map. So I'm not sure if it's that dangerous practically.
>
> On the contrary, I'm absolutely sure that changing the signature of this onchange in our localization is a bomb, making it incompatible with many modules unless we build a combinatorial explosion of glue modules (we already suffer a lot of the situation with the other on_changes not passing a hackable context).
>
> Indeed, may module would override onchange_partner_id without touching the signature in the view. Today it just works. If we shouldn't not use the context anymore for wrapping our params, tomorrow we will need to work around every single case to ensure our override with more params will be called first.
>
> So what do you think?
> if the most annoying thing is the recursive context, couldn't we just avoid it by copying the context before getting it into the kwargs?
>
> Regards.
>

I understand why you did that and that's really awful, but I don't see
any good solution, since we can't pass kwargs in the views...

So if really you need that, I think we have no choice to keep that.

--
Guewen Baconnier
Business Solutions Software Developer

Camptocamp SA
PSE A, CH-1015 Lausanne
Phone: [phone]
Office: [phone]
http://www.camptocamp.com/

[lp:~camptocamp/openerp-fiscal-rules/7.0-wrong-onchange-1255918]

66. By Guewen Baconnier @ Camptocamp on 2013-12-04

[CHG] reintroduce the ugly thing of the context muted in kwargs, used by the BR localization to pass arbitrary values in the onchange

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

Hello Guewen,

yes, you are right, I'll double check, but it rather look this code wasn't reached at all and that the fiscal rule is applied as expected just because onchange_address_id was called later in the chain (with an annoying signature change in our localization this time BTW)...

Eventually, what we could do is: clean the base code just as you suggest. And in our localization, we do the nasty context to kwargs transfer. We can actually do it without changing the method signature (the extra args will be transferred at any point in the super chain).

Let me do some testing and then I confirm we can still live with that and you do the whole merge.

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

OK,

so after looking further at the code, I confirm we can merge your change. We will deal with injecting the required extra params in the context in our localization, an unwrap them in the _fiscal_position_map method that we already override.
In any case, the way we were overriding the context in the view was broken (it was forgetting all previous context). Eventually we will use an extra web module doing that kind of hack to get our parameters injected in the context: https://gist.github.com/rvalyi/7789846

In any case you can proceed with the merge.

review: Approve

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

LGTM

review: Approve (code review, no tests)

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

Just as a post-mortem comment for documentation:

the dead code that Gueween was telling was never reached, was actually reached in our specific case in the Brazilian Localization because we were overriding the context in such a way in the view and we had no more shop_id... (we overrided the context view attribute to pass extra params but missed shop_id)

I really believe the usage of web_context_tunnel instead https://code.launchpad.net/~akretion-team/server-env-tools/web-context-tunnel/+merge/198599 will allow to inject extra parameters in the context (instead of making son_change signatures incompatibles) without risking to just bypass the original context as it was occurring in this specific case and made us miss the bug.

Regards.

=== 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-04 07:52:44 +0000
@@ -43,21 +43,22 @@
         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 = context.copy()
+        kwargs.update({
+            '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)