openerp-fiscal-rules

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

Proposed by Guewen Baconnier @ Camptocamp on 2013-12-16

Status:

Merged

Approved by:

Yannick Vaucher @ Camptocamp on 2013-12-18

Approved revision:

Merged at revision:

Proposed branch:

Merge into:

Diff against target:

1 file modified

account_fiscal_position_rule_sale/sale.py (+2/-3)

To merge this branch:

bzr merge lp://qastaging/~camptocamp/openerp-fiscal-rules/7.0-wrong-onchange-1255918-revert-hack

Related bugs:

Bug #1255918: kwargs always NoneType	Undecided	Fix Released
Bug #1265067: [7.0] _fiscal_position_map() got multiple values for keyword argument 'uid'	Undecided	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Sandy Carter (http://www.savoirfairelinux.com)	code review		Approve on 2013-12-31
Raphaël Valyi - http://www.akretion.com			Approve on 2013-12-16
Yannick Vaucher @ Camptocamp	code review, no tests	2013-12-16	Approve on 2013-12-16
Review via email: mp+199144@code.qastaging.launchpad.net

Since Raphaël stated that the context being mutated in kwargs is not necessary [0], I propose to revert the change.

Note that the current code is failing is some conditions, when a 'uid' key is present in the context for instance.

Revision history for this message

Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote on 2013-12-16:

Unless we make the hack uglier it seems necessary to do this revert.

review: Approve (code review, no tests)

Revision history for this message

Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote on 2013-12-16:

disambiguation:
I meant
-> we can suspect it was approved by Raphael, before the change

Revision history for this message

Raphaël Valyi - http://www.akretion.com (rvalyi) wrote on 2013-12-16:

thanks guys.

In any case, for our localization we will try to wrap the extra params in the context more cleanly using:
https://code.launchpad.net/~akretion-team/server-env-tools/web-context-tunnel/+merge/198599
(You are welcome to review BTW)

And we will unwrap our extra params ourselves from the context in our localization override.

Thanks.

review: Approve

Revision history for this message

Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote on 2013-12-31:

LGTM

review: Approve (code review)