Code review comment for lp:~camptocamp/openerp-fiscal-rules/7.0-wrong-onchange-1255918

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: +41 21 619 10 39
Office: +41 21 619 10 10
http://www.camptocamp.com/