From: Olivier Dony Date: Thu, 27 Nov 2014 11:36:28 +0000 (+0100) Subject: [FIX] product: remove unnecessary UoM rounding step, add missing test X-Git-Url: http://git.inspyration.org/?a=commitdiff_plain;h=fc85a7ee5c33eadc22652b6b26478c320e35b283;p=odoo%2Fodoo.git [FIX] product: remove unnecessary UoM rounding step, add missing test Remove the intermediate rounding inside _compute_qty(), as it is not necessary after rev. fa2f7b86 and has undesired side-effects. An extra float_round() operation inside _compute_qty() had been added at rev. 311c77bb to avoid a float representation error in UoM factors that could bias the ceiling() operation done as the last conversion step. Example 1: Dozen has a factor of 1/12, which was previously stored in the database with a decimal accuracy of 12 significant decimal digits. This meant the factor was exactly stored as 0.08333333333333. When reading this back into a Python float, the precision was not sufficient, and the UoM conversion of 1 Dozen to Units gave a result of 12.00000000000047961... After the final ceiling() operation to Unit's rounding, the converted value ended up as 13. This problem was initially solved using an extra rounding. However at revision fa2f7b86 the decimal precision used to store UoM factors was increased to preserve all significant digits. This added the extra precision necessary to read the Dozen factor back into an accurate float value of 1/12, and the conversion of 1 Dozen now gives 12.0 Units, even without the intermediate rounding operation. (Works for other factor values too) At the same time that extra rounding operation has undesired side-effects, as it requires a fixed precision derived from the rounding precisions of the UoMs. But there is no given precision that would work in all cases for this intermediate value. It is always possible to find a valid combination of UoM roundings that breaks that intermediate step, e.g. by forcing integer roundings. Example 2: Let Grams have a rounding precision set to 1 because no smaller quantities are allowed, and Kilograms a rounding of 0.001 to allow representing 1 Gram. (gram factor = 1000 and kilogram rounding = .001 by default) If we try to convert 1234 Grams into Kilograms, the extra rounding introduced in 311c77bb will cause a rounding of 1234.0/1000.0 at the precision of Grams (1), which gives 1.0 as a result. The net result of this conversion gives 1234.0 Gram = 1.0 Kilogram, while the correct result (1.234 Kilogram) is perfectly compatible with the UoM settings. Similar errors could be triggered with various rounding settings, as long as the intermediate rounding needs a finite precision. Two extra tests have been added to cover Example 1 and Example 2. -- Related to #2072, #1125, #1126, #2672 Closes #2495, #2498 --- diff --git a/addons/product/product.py b/addons/product/product.py index abdbcaf..69dc23b 100644 --- a/addons/product/product.py +++ b/addons/product/product.py @@ -178,10 +178,7 @@ class product_uom(osv.osv): raise osv.except_osv(_('Error!'), _('Conversion from Product UoM %s to Default UoM %s is not possible as they both belong to different Category!.') % (from_unit.name,to_unit.name,)) else: return qty - # First round to the precision of the original unit, so that - # float representation errors do not bias the following ceil() - # e.g. with 1 / (1/12) we could get 12.0000048, ceiling to 13! - amount = float_round(qty/from_unit.factor, precision_rounding=from_unit.rounding) + amount = qty/from_unit.factor if to_unit: amount = ceiling(amount * to_unit.factor, to_unit.rounding) return amount diff --git a/addons/product/product_data.xml b/addons/product/product_data.xml index 49362e3..4e62e1e 100644 --- a/addons/product/product_data.xml +++ b/addons/product/product_data.xml @@ -47,6 +47,7 @@ kg + diff --git a/addons/product/tests/test_uom.py b/addons/product/tests/test_uom.py index 3d3ba04..e6ac605 100644 --- a/addons/product/tests/test_uom.py +++ b/addons/product/tests/test_uom.py @@ -12,7 +12,10 @@ class TestUom(TransactionCase): def test_10_conversion(self): cr, uid = self.cr, self.uid gram_id = self.imd.get_object_reference(cr, uid, 'product', 'product_uom_gram')[1] + kg_id = self.imd.get_object_reference(cr, uid, 'product', 'product_uom_kgm')[1] tonne_id = self.imd.get_object_reference(cr, uid, 'product', 'product_uom_ton')[1] + unit_id = self.imd.get_object_reference(cr, uid, 'product','product_uom_unit')[1] + dozen_id = self.imd.get_object_reference(cr, uid, 'product','product_uom_dozen')[1] qty = self.uom._compute_qty(cr, uid, gram_id, 1020000, tonne_id) self.assertEquals(qty, 1.02, "Converted quantity does not correspond.") @@ -20,6 +23,20 @@ class TestUom(TransactionCase): price = self.uom._compute_price(cr, uid, gram_id, 2, tonne_id) self.assertEquals(price, 2000000.0, "Converted price does not correspond.") + # If the conversion factor for Dozens (1/12) is not stored with sufficient precision, + # the conversion of 1 Dozen into Units will give e.g. 12.00000000000047 Units + # and the Unit rounding will round that up to 13. + # This is a partial regression test for rev. 311c77bb, which is further improved + # by rev. fa2f7b86. + qty = self.uom._compute_qty(cr, uid, dozen_id, 1, unit_id) + self.assertEquals(qty, 12.0, "Converted quantity does not correspond.") + + # Regression test for side-effect of commit 311c77bb - converting 1234 Grams + # into Kilograms should work even if grams are rounded to 1. + self.uom.write(cr, uid, gram_id, {'rounding': 1}) + qty = self.uom._compute_qty(cr, uid, gram_id, 1234, kg_id) + self.assertEquals(qty, 1.234, "Converted quantity does not correspond.") + def test_20_rounding(self): cr, uid = self.cr, self.uid unit_id = self.imd.get_object_reference(cr, uid, 'product', 'product_uom_unit')[1]