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

Proposed by Eric Caudal - 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/ (+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:
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.


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


review: Disapprove
Leonardo Pistone (lepistone) wrote :

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

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.


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/'
2--- purchase_landed_costs/ 2013-12-12 15:34:53 +0000
3+++ purchase_landed_costs/ 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 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 =
17- fiscal_position = po.fiscal_position or False
18+ fiscal_position = po.fiscal_position and or False
19 journal_obj = self.pool.get('account.journal')
20 journal_ids =
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 or False
28 exp_account_id = prod_obj._choose_exp_account_from(
29 cr, uid,
30 landed_cost.product_id,


People subscribed via source and target branches