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

[Eric Caudal - www.elico-corp.com]

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

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)]

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

Regards.

review: Approve (code review)

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

> 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)]

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)]

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

=== modified file 'purchase_landed_costs/purchase.py'
--- purchase_landed_costs/purchase.py	2013-12-12 15:34:53 +0000
+++ purchase_landed_costs/purchase.py	2014-02-28 10:15:58 +0000
@@ -281,7 +281,7 @@
             apply_on = 'order'
             po_obj = self.pool.get('purchase.order')
             po = po_obj.browse(cr, uid, purchase_order_id, context=context)
-            fiscal_position = po.fiscal_position or False
+	    fiscal_position = po.fiscal_position and po.fiscal_position.id or False 
         else:
             apply_on = 'line'
         if not product_id:
@@ -550,7 +550,7 @@
         po = (landed_cost.purchase_order_id or
               landed_cost.purchase_order_line_id.order_id)
         currency_id = landed_cost.purchase_order_id.pricelist_id.currency_id.id
-        fiscal_position = po.fiscal_position or False
+	fiscal_position = po.fiscal_position and po.fiscal_position.id or False 
         journal_obj = self.pool.get('account.journal')
         journal_ids = journal_obj.search(
             cr, uid,
@@ -587,7 +587,7 @@
         vals_inv = self._prepare_landed_cost_inv(cr, uid, landed_cost,
                                                  context=context)
         inv_id = invoice_obj.create(cr, uid, vals_inv, context=context)
-        fiscal_position = (po.fiscal_position or False)
+	fiscal_position = po.fiscal_position and po.fiscal_position.id or False 
         exp_account_id = prod_obj._choose_exp_account_from(
             cr, uid,
             landed_cost.product_id,