From da12f2b809d74ef5637153b1fe67c9fc9b741fa4 Mon Sep 17 00:00:00 2001
From: Olivier Dony <odo@openerp.com>
Date: Tue, 9 Dec 2014 15:28:37 +0100
Subject: [PATCH] [FIX] product: pricelist: multiple corrections in UoM-related
 conversions

Pricelist computations need to consider 2 different Units
of Measure:
 - The default product UoM (product.uom_id), used as reference
   for the various quantities and amounts specified in each
   pricelist rules.
 - The `context UoM` is the UoM in which the result is requested,
   that is the list price UoM.

For example the 'price_min_margin' amount is meant for the unit
price of 1 x default UoM. When the context UoM is not the default
product UoM, it can be any UoM of the same UoM Category, and the
various quantities and amounts specified on the rule need to
be adapted accordingly:
  - min_quantity (expressed in terms of the default UoM)
  - price_surcharge (specified for 1 x default UoM)
  - price_min_margin (specified for 1 x default UoM)
  - price_max_margin (specified for 1 x default UoM)

The UoM corrections were not done consistently and resulted in
wrong prices when computing the price using a non-default UoM.
The cases were a conversion was needed or not were not properly
identified within the _price_rule_get_multi().

After this commit, the various code branches in _price_rule_get_multi
always ensures that:
 - price requested for: `qty` of `qty_uom_id`
 - `qty_in_product_uom` is the requested `qty` converted to default UoM
 - current (intermediary) price: `price` for  `price_uom_id`

Therefore `price` and `price_uom_id` are always in sync, and `price_uom_id`
can always be compared with `qty_uom_id' in order to know whether
a conversion is still needed.

This patch also corrects and extends the regression tests
introduced at revision 79ebe10.
---
 addons/product/pricelist.py            | 78 +++++++++++++++-----------
 addons/product/product.py              |  3 +-
 addons/product/tests/test_pricelist.py | 47 +++++++++++++++-
 3 files changed, 93 insertions(+), 35 deletions(-)

diff --git a/addons/product/pricelist.py b/addons/product/pricelist.py
index 49b518a8078..5881099a13c 100644
--- a/addons/product/pricelist.py
+++ b/addons/product/pricelist.py
@@ -247,19 +247,27 @@ class product_pricelist(osv.osv):
 
         results = {}
         for product, qty, partner in products_by_qty_by_partner:
-            uom_price_already_computed = False
             results[product.id] = 0.0
-            price = False
             rule_id = False
+            price = False
+
+            # Final unit price is computed according to `qty` in the `qty_uom_id` UoM.
+            # An intermediary unit price may be computed according to a different UoM, in
+            # which case the price_uom_id contains that UoM.
+            # The final price will be converted to match `qty_uom_id`.
+            qty_uom_id = context.get('uom') or product.uom_id.id
+            price_uom_id = product.uom_id.id
+            qty_in_product_uom = qty
+            if qty_uom_id != product.uom_id.id:
+                try:
+                    qty_in_product_uom = product_uom_obj._compute_qty(
+                        cr, uid, context['uom'], qty, product.uom_id.id or product.uos_id.id)
+                except except_orm:
+                    # Ignored - incompatible UoM in context, use default product UoM
+                    pass
+
             for rule in items:
-                if 'uom' in context and product.uom_id and context['uom'] != product.uom_id.id:
-                    try:
-                        qty_in_product_uom = product_uom_obj._compute_qty(cr, uid, context['uom'], qty, product.uom_id.id, dict(context.items() + [('raise-exception', False)]))
-                    except except_orm:
-                        qty_in_product_uom = qty
-                else:
-                    qty_in_product_uom = qty
-                if rule.min_quantity and qty_in_product_uom<rule.min_quantity:
+                if rule.min_quantity and qty_in_product_uom < rule.min_quantity:
                     continue
                 if is_product_template:
                     if rule.product_tmpl_id and product.id != rule.product_tmpl_id.id:
@@ -287,7 +295,7 @@ class product_pricelist(osv.osv):
                                 rule.base_pricelist_id, [(product,
                                 qty, False)], context=context)[product.id]
                         ptype_src = rule.base_pricelist_id.currency_id.id
-                        uom_price_already_computed = True
+                        price_uom_id = qty_uom_id
                         price = currency_obj.compute(cr, uid,
                                 ptype_src, pricelist.currency_id.id,
                                 price_tmp, round=False,
@@ -302,12 +310,10 @@ class product_pricelist(osv.osv):
                         seller = product.seller_ids[0]
                     if seller:
                         qty_in_seller_uom = qty
-                        from_uom = context.get('uom') or product.uom_id.id
-                        seller_uom = seller.product_uom and seller.product_uom.id or False
-                        if seller_uom and from_uom and from_uom != seller_uom:
-                            qty_in_seller_uom = product_uom_obj._compute_qty(cr, uid, from_uom, qty, to_uom_id=seller_uom)
-                        else:
-                            uom_price_already_computed = True
+                        seller_uom = seller.product_uom.id
+                        if qty_uom_id != seller_uom:
+                            qty_in_seller_uom = product_uom_obj._compute_qty(cr, uid, qty_uom_id, qty, to_uom_id=seller_uom)
+                        price_uom_id = seller_uom
                         for line in seller.pricelist_ids:
                             if line.min_quantity <= qty_in_seller_uom:
                                 price = line.price
@@ -317,34 +323,40 @@ class product_pricelist(osv.osv):
                         price_types[rule.base] = price_type_obj.browse(cr, uid, int(rule.base))
                     price_type = price_types[rule.base]
 
-                    uom_price_already_computed = True
-                    price = currency_obj.compute(cr, uid,
+                    # price_get returns the price in the context UoM, i.e. qty_uom_id
+                    price_uom_id = qty_uom_id
+                    price = currency_obj.compute(
+                            cr, uid,
                             price_type.currency_id.id, pricelist.currency_id.id,
-                            product_obj._price_get(cr, uid, [product],
-                            price_type.field, context=context)[product.id], round=False, context=context)
+                            product_obj._price_get(cr, uid, [product], price_type.field, context=context)[product.id],
+                            round=False, context=context)
 
                 if price is not False:
                     price_limit = price
                     price = price * (1.0+(rule.price_discount or 0.0))
                     if rule.price_round:
                         price = tools.float_round(price, precision_rounding=rule.price_round)
-                    if context.get('uom'):
-                        # compute price_surcharge based on reference uom
-                        factor = product_uom_obj.browse(cr, uid, context.get('uom'), context=context).factor
-                    else:
-                        factor = 1.0
-                    price += (rule.price_surcharge or 0.0) / factor
+
+                    convert_to_price_uom = (lambda price: product_uom_obj._compute_price(
+                                                cr, uid, product.uom_id.id,
+                                                price, price_uom_id))
+                    if rule.price_surcharge:
+                        price_surcharge = convert_to_price_uom(rule.price_surcharge)
+                        price += price_surcharge
+
                     if rule.price_min_margin:
-                        price = max(price, price_limit+rule.price_min_margin)
+                        price_min_margin = convert_to_price_uom(rule.price_min_margin)
+                        price = max(price, price_limit + price_min_margin)
+
                     if rule.price_max_margin:
-                        price = min(price, price_limit+rule.price_max_margin)
+                        price_max_margin = convert_to_price_uom(rule.price_max_margin)
+                        price = min(price, price_limit + price_max_margin)
+
                     rule_id = rule.id
                 break
 
-            if price:
-                if 'uom' in context and not uom_price_already_computed:
-                    uom = product.uos_id or product.uom_id
-                    price = product_uom_obj._compute_price(cr, uid, uom.id, price, context['uom'])
+            # Final price conversion to target UoM
+            price = product_uom_obj._compute_price(cr, uid, price_uom_id, price, qty_uom_id)
 
             results[product.id] = (price, rule_id)
         return results
diff --git a/addons/product/product.py b/addons/product/product.py
index 253bae7a3ee..618d1503afc 100644
--- a/addons/product/product.py
+++ b/addons/product/product.py
@@ -189,7 +189,8 @@ class product_uom(osv.osv):
         return amount
 
     def _compute_price(self, cr, uid, from_uom_id, price, to_uom_id=False):
-        if not from_uom_id or not price or not to_uom_id:
+        if (not from_uom_id or not price or not to_uom_id
+                or (to_uom_id == from_uom_id)):
             return price
         from_unit, to_unit = self.browse(cr, uid, [from_uom_id, to_uom_id])
         if from_unit.category_id.id != to_unit.category_id.id:
diff --git a/addons/product/tests/test_pricelist.py b/addons/product/tests/test_pricelist.py
index 4e61cb11b75..37d80360cff 100644
--- a/addons/product/tests/test_pricelist.py
+++ b/addons/product/tests/test_pricelist.py
@@ -15,6 +15,8 @@ class TestPricelist(TransactionCase):
         self.datacard_id = self.ir_model_data.get_object_reference(cr, uid, 'product', 'product_product_46')[1]
         self.unit_id = self.ir_model_data.get_object_reference(cr, uid, 'product', 'product_uom_unit')[1]
         self.dozen_id = self.ir_model_data.get_object_reference(cr, uid, 'product', 'product_uom_dozen')[1]
+        self.tonne_id = self.ir_model_data.xmlid_to_res_id(cr, uid, 'product.product_uom_ton')
+        self.kg_id = self.ir_model_data.xmlid_to_res_id(cr, uid, 'product.product_uom_kgm')
 
         self.public_pricelist_id = self.ir_model_data.get_object_reference(cr, uid, 'product', 'list0')[1]
         self.sale_pricelist_id = self.product_pricelist.create(cr, uid, {
@@ -64,7 +66,50 @@ class TestPricelist(TransactionCase):
         usb_adapter_unit = self.product_product.browse(cr, uid, self.usb_adapter_id, context=unit_context)
         usb_adapter_dozen = self.product_product.browse(cr, uid, self.usb_adapter_id, context=dozen_context)
         self.assertAlmostEqual(usb_adapter_unit.price*12, usb_adapter_dozen.price)
-
         datacard_unit = self.product_product.browse(cr, uid, self.datacard_id, context=unit_context)
         datacard_dozen = self.product_product.browse(cr, uid, self.datacard_id, context=dozen_context)
+        # price_surcharge applies to product default UoM, here "Units", so surcharge will be multiplied
         self.assertAlmostEqual(datacard_unit.price*12, datacard_dozen.price)
+
+    def test_20_pricelist_uom(self):
+        # Verify that the pricelist rules are correctly using the product's default UoM
+        # as reference, and return a result according to the target UoM (as specific in the context)
+        cr, uid = self.cr, self.uid
+        kg, tonne = self.kg_id, self.tonne_id
+        tonne_price = 100
+
+        # make sure 'tonne' resolves down to 1 'kg'.
+        self.uom.write(cr, uid, tonne, {'rounding': 0.001})
+
+        # setup product stored in 'tonnes', with a discounted pricelist for qty > 3 tonnes
+        spam_id = self.product_product.copy(cr, uid, self.usb_adapter_id,
+                                            { 'name': '1 tonne of spam',
+                                              'uom_id': self.tonne_id,
+                                              'uos_id': self.tonne_id,
+                                              'uom_po_id': self.tonne_id,
+                                              'list_price': tonne_price,
+                                            })
+        pricelist_version_id = self.ir_model_data.xmlid_to_res_id(cr, uid, 'product.ver0')
+        self.registry('product.pricelist.item').create(cr, uid,
+                                                      { 'price_version_id': pricelist_version_id,
+                                                        'sequence': 10,
+                                                        'name': '3+ tonnes: -10 EUR discount/t',
+                                                        'base': 1, # based on public price
+                                                        'min_quantity': 3, # min = 3 tonnes
+                                                        'price_surcharge': -10, # -10 EUR / tonne
+                                                        'product_id': spam_id,
+                                                      })
+        pricelist_id = self.public_pricelist_id
+
+        def test_unit_price(qty, uom, expected_unit_price):
+            unit_price = self.registry('product.pricelist').price_get(cr, uid, [pricelist_id],
+                                                                      spam_id, qty,
+                                                                      context={'uom': uom})[pricelist_id]
+            self.assertAlmostEqual(unit_price, expected_unit_price, msg='Computed unit price is wrong')
+
+        # Test prices - they are *per unit*, the quantity is only here to match the pricelist rules!
+        test_unit_price(2, kg, tonne_price / 1000.0)
+        test_unit_price(2000, kg, tonne_price / 1000.0)
+        test_unit_price(3500, kg, (tonne_price - 10) / 1000.0)
+        test_unit_price(2, tonne, tonne_price)
+        test_unit_price(3, tonne, tonne_price - 10)