Merge lp:~elicoidal/purchase-wkfl/purchase-wkfl-fix-0001 into lp:~purchase-core-editors/purchase-wkfl/7.0

Proposed by Eric Caudal - www.elico-corp.com on 2014-02-28
Status: Rejected
Rejected by: Guewen Baconnier @ Camptocamp on 2014-03-03
Proposed branch: lp:~elicoidal/purchase-wkfl/purchase-wkfl-fix-0001
Merge into: lp:~purchase-core-editors/purchase-wkfl/7.0
Diff against target: 30 lines (+3/-3)
1 file modified
purchase_landed_costs/purchase.py (+3/-3)
To merge this branch: bzr merge lp:~elicoidal/purchase-wkfl/purchase-wkfl-fix-0001
Reviewer Review Type Date Requested Status
Pedro Manuel Baeza Disapprove on 2014-03-03
Leonardo Pistone code review Disapprove on 2014-03-03
Guewen Baconnier @ Camptocamp Disapprove on 2014-03-03
Alexandre Fayolle - camptocamp code review, no test 2014-02-28 Needs Information on 2014-02-28
Review via email: mp+208758@code.launchpad.net
To post a comment you must log in.

Can I suggest using the "value if condition else other_value" construct rather than "condition and value or other_value"

the former is more explicit and does not suffer from the possible unintended behavior you get when value is False (you get other_value in that case even if condition is True).

review: Needs Information (code review, no test)
Pedro Manuel Baeza (pedro.baeza) wrote :

I think there's no side effect on the expression. It's also used everywhere on OpenERP core.

Regards.

review: Approve (code review)

> I think there's no side effect on the expression. It's also used everywhere on
> OpenERP core.
>
> Regards.

Being used everywhere on OpenERP is not a good rationale.

Eric, the changes on the lines 9 and 27 of the diff are not correct: `prod_obj._choose_exp_account_from` expects a browse_record.

I proposed a correct fix [0] hence I reject this patch.

Thanks anyway

[0] https://code.launchpad.net/~camptocamp/purchase-wkfl/7.0-browse_record_error-1287159/+merge/209056

review: Disapprove
Leonardo Pistone (lepistone) wrote :

To back Alexandre's point, here is a bug caused by the and-or trick:

https://bugs.launchpad.net/openobject-addons/+bug/1190592

review: Disapprove (code review)
Pedro Manuel Baeza (pedro.baeza) wrote :

OK, I see what's your point. It's convenient to rename them variables with the suffix _id to differentiate browse_record instances from id variables.

Regards.

review: Disapprove

Unmerged revisions

29. By <elicoidal <email address hidden>> <email address hidden> on 2014-02-28

[FIX] fiscal position

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'purchase_landed_costs/purchase.py'
2--- purchase_landed_costs/purchase.py 2013-12-12 15:34:53 +0000
3+++ purchase_landed_costs/purchase.py 2014-02-28 10:15:58 +0000
4@@ -281,7 +281,7 @@
5 apply_on = 'order'
6 po_obj = self.pool.get('purchase.order')
7 po = po_obj.browse(cr, uid, purchase_order_id, context=context)
8- fiscal_position = po.fiscal_position or False
9+ fiscal_position = po.fiscal_position and po.fiscal_position.id or False
10 else:
11 apply_on = 'line'
12 if not product_id:
13@@ -550,7 +550,7 @@
14 po = (landed_cost.purchase_order_id or
15 landed_cost.purchase_order_line_id.order_id)
16 currency_id = landed_cost.purchase_order_id.pricelist_id.currency_id.id
17- fiscal_position = po.fiscal_position or False
18+ fiscal_position = po.fiscal_position and po.fiscal_position.id or False
19 journal_obj = self.pool.get('account.journal')
20 journal_ids = journal_obj.search(
21 cr, uid,
22@@ -587,7 +587,7 @@
23 vals_inv = self._prepare_landed_cost_inv(cr, uid, landed_cost,
24 context=context)
25 inv_id = invoice_obj.create(cr, uid, vals_inv, context=context)
26- fiscal_position = (po.fiscal_position or False)
27+ fiscal_position = po.fiscal_position and po.fiscal_position.id or False
28 exp_account_id = prod_obj._choose_exp_account_from(
29 cr, uid,
30 landed_cost.product_id,

Subscribers

People subscribed via source and target branches