Merge lp:~camptocamp/openerp-fiscal-rules/7.0-wrong-onchange-1255918-revert-hack into lp: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: 67
Merged at revision: 66
Proposed branch: lp:~camptocamp/openerp-fiscal-rules/7.0-wrong-onchange-1255918-revert-hack
Merge into: lp:openerp-fiscal-rules
Diff against target: 20 lines (+2/-3)
1 file modified
account_fiscal_position_rule_sale/sale.py (+2/-3)
To merge this branch: bzr merge lp:~camptocamp/openerp-fiscal-rules/7.0-wrong-onchange-1255918-revert-hack
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.launchpad.net

Description of the change

Follows https://code.launchpad.net/~camptocamp/openerp-fiscal-rules/7.0-wrong-onchange-1255918/+merge/197376

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.

[0] https://code.launchpad.net/~camptocamp/openerp-fiscal-rules/7.0-wrong-onchange-1255918/+merge/197376/comments/458163

To post a comment you must log in.

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

Looking at https://code.launchpad.net/~camptocamp/openerp-fiscal-rules/7.0-wrong-onchange-1255918/+merge/197376/comments/458163 we can suspect it was approved before the change by Raphael

review: Approve (code review, no tests)

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

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

LGTM

review: Approve (code review)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'account_fiscal_position_rule_sale/sale.py'
2--- account_fiscal_position_rule_sale/sale.py 2013-12-04 07:52:33 +0000
3+++ account_fiscal_position_rule_sale/sale.py 2013-12-16 15:26:04 +0000
4@@ -53,14 +53,13 @@
5 return result
6
7 values = result['value']
8- kwargs = context.copy()
9- kwargs.update({
10+ kwargs = {
11 'shop_id': context['shop_id'],
12 'partner_id': partner_id,
13 'partner_invoice_id': values.get('partner_invoice_id', False),
14 'partner_shipping_id': values.get('partner_shipping_id', False),
15 'context': context
16- })
17+ }
18 return self._fiscal_position_map(cr, uid, result, **kwargs)
19
20 def onchange_address_id(self, cr, uid, ids, partner_invoice_id,

Subscribers

People subscribed via source and target branches