[FIX] orm.read_group: improve test coverage and corner case handling:

- allow leading spaces in orderby spec for m2o columns
- extra test for read_group on multiple columns
- proper handling of groupby on numeric column with default order

bzr revid: odo@openerp.com-20140304152927-havybom9x1jgdzae

diff --git a/openerp/addons/base/tests/test_base.py b/openerp/addons/base/tests/test_base.py
index ebdb406..42ba21a 100644 (file)
--- a/openerp/addons/base/tests/test_base.py
+++ b/openerp/addons/base/tests/test_base.py
@@ -299,7 +299,16 @@ class test_base(common.TransactionCase):
         ids = [self.res_users.create(cr, uid, u) for u in test_users]
         domain = [('id', 'in', ids)]
 
-        # group on local char field, aggregate on int field (second groupby ignored on purpose)
+        # group on local char field without domain and without active_test (-> empty WHERE clause)
+        groups_data = self.res_users.read_group(cr, uid, [], fields=['login'], groupby=['login'], orderby='login DESC', context={'active_test': False})
+        self.assertGreater(len(groups_data), 6, "Incorrect number of results when grouping on a field")
+
+        # group on local char field with limit
+        groups_data = self.res_users.read_group(cr, uid, domain, fields=['login'], groupby=['login'], orderby='login DESC', limit=3, offset=3)
+        self.assertEqual(len(groups_data), 3, "Incorrect number of results when grouping on a field with limit")
+        self.assertEqual(['bob', 'alice2', 'alice'], [g['login'] for g in groups_data], 'Result mismatch')
+
+        # group on inherited char field, aggregate on int field (second groupby ignored on purpose)
         groups_data = self.res_users.read_group(cr, uid, domain, fields=['name', 'color', 'function'], groupby=['function', 'login'])
         self.assertEqual(len(groups_data), 3, "Incorrect number of results when grouping on a field")
         self.assertEqual(['5$ Wrench', 'Eavesdropper', 'Friend'], [g['function'] for g in groups_data], 'incorrect read_group order')
@@ -307,11 +316,20 @@ class test_base(common.TransactionCase):
             self.assertIn('color', group_data, "Aggregated data for the column 'color' is not present in read_group return values")
             self.assertEqual(group_data['color'], 3, "Incorrect sum for aggregated data for the column 'color'")
 
-        # group on local char field, reverse order
+        # group on inherited char field, reverse order
         groups_data = self.res_users.read_group(cr, uid, domain, fields=['name', 'color'], groupby='name', orderby='name DESC')
         self.assertEqual(['Nab', 'Eve', 'Bob', 'Alice'], [g['name'] for g in groups_data], 'Incorrect ordering of the list')
 
-        # group on local char field, multiple orders with directions
+        # group on int field, default ordering
+        groups_data = self.res_users.read_group(cr, uid, domain, fields=['color'], groupby='color')
+        self.assertEqual([-3, 0, 1, 2, 3, 6], [g['color'] for g in groups_data], 'Incorrect ordering of the list')
+
+        # multi group, second level is int field, should still be summed in first level grouping
+        groups_data = self.res_users.read_group(cr, uid, domain, fields=['name', 'color'], groupby=['name', 'color'], orderby='name DESC')
+        self.assertEqual(['Nab', 'Eve', 'Bob', 'Alice'], [g['name'] for g in groups_data], 'Incorrect ordering of the list')
+        self.assertEqual([3, 3, 2, 1], [g['color'] for g in groups_data], 'Incorrect ordering of the list')
+
+        # group on inherited char field, multiple orders with directions
         groups_data = self.res_users.read_group(cr, uid, domain, fields=['name', 'color'], groupby='name', orderby='color DESC, name')
         self.assertEqual(len(groups_data), 4, "Incorrect number of results when grouping on a field")
         self.assertEqual(['Eve', 'Nab', 'Bob', 'Alice'], [g['name'] for g in groups_data], 'Incorrect ordering of the list')
@@ -345,6 +363,14 @@ class test_base(common.TransactionCase):
         self.assertEqual([2, 4], [g['title_count'] for g in groups_data], 'Incorrect number of results')
         self.assertEqual([-1, 10], [g['color'] for g in groups_data], 'Incorrect aggregation of int column')
 
+        # group on inherited many2one (res_partner.title), multiple orders with m2o in second position
+        groups_data = self.res_users.read_group(cr, uid, domain, fields=['function', 'color', 'title'], groupby=['title'], orderby="color desc, title desc")
+        self.assertEqual(len(groups_data), 2, "Incorrect number of results when grouping on a field")
+        # m2o is returned as a (id, label) pair
+        self.assertEqual([(title_lady, 'Lady'), (title_sir, 'Sir')], [g['title'] for g in groups_data], 'Incorrect ordering of the result')
+        self.assertEqual([4, 2], [g['title_count'] for g in groups_data], 'Incorrect number of results')
+        self.assertEqual([10, -1], [g['color'] for g in groups_data], 'Incorrect aggregation of int column')
+
         # group on inherited many2one (res_partner.title), ordered by other inherited field (color)
         groups_data = self.res_users.read_group(cr, uid, domain, fields=['function', 'color', 'title'], groupby=['title'], orderby='color')
         self.assertEqual(len(groups_data), 2, "Incorrect number of results when grouping on a field")
@@ -353,6 +379,7 @@ class test_base(common.TransactionCase):
         self.assertEqual([2, 4], [g['title_count'] for g in groups_data], 'Incorrect number of results')
         self.assertEqual([-1, 10], [g['color'] for g in groups_data], 'Incorrect aggregation of int column')
 
+
 class test_partner_recursion(common.TransactionCase):
 
     def setUp(self):

diff --git a/openerp/osv/orm.py b/openerp/osv/orm.py
index a033e3f..f9cfbe1 100644 (file)
--- a/openerp/osv/orm.py
+++ b/openerp/osv/orm.py
@@ -76,7 +76,7 @@ _schema = logging.getLogger(__name__ + '.schema')
 # List of etree._Element subclasses that we choose to ignore when parsing XML.
 from openerp.tools import SKIPPED_ELEMENT_TYPES
 
-regex_order = re.compile('^(([a-z0-9_]+|"[a-z0-9_]+")( *desc| *asc)?( *, *|))+$', re.I)
+regex_order = re.compile('^( *([a-z0-9_]+|"[a-z0-9_]+")( *desc| *asc)?( *, *|))+$', re.I)
 regex_object_name = re.compile(r'^[a-z0-9_.]+$')
 
 # TODO for trunk, raise the value to 1000
@@ -2698,7 +2698,7 @@ class BaseModel(object):
 
         aggregated_fields = [
             f for f in fields
-            if f not in ('id', 'sequence')
+            if f not in ('id', 'sequence', groupby)
             if fget[f]['type'] in ('integer', 'float')
             if (f in self._all_columns and getattr(self._all_columns[f].column, '_classic_write'))]
         for f in aggregated_fields:
@@ -2716,6 +2716,7 @@ class BaseModel(object):
             count_field = groupby
 
         prefix_terms = lambda prefix, terms: (prefix + " " + ",".join(terms)) if terms else ''
+        prefix_term = lambda prefix, term: ('%s %s' % (prefix, term)) if term else ''
 
         query = """
             SELECT min(%(table)s.id) AS id, count(%(table)s.id) AS %(count_field)s_count
@@ -2731,11 +2732,11 @@ class BaseModel(object):
             'count_field': count_field,
             'extra_fields': prefix_terms(',', select_terms),
             'from': from_clause,
-            'where': prefix_terms('WHERE', [where_clause]),
+            'where': prefix_term('WHERE', where_clause),
             'groupby': prefix_terms('GROUP BY', groupby_terms),
             'orderby': prefix_terms('ORDER BY', orderby_terms),
-            'limit': prefix_terms('LIMIT', [str(int(limit))] if limit else []),
-            'offset': prefix_terms('OFFSET', [str(int(offset))] if offset else []),
+            'limit': prefix_term('LIMIT', int(limit) if limit else None),
+            'offset': prefix_term('OFFSET', int(offset) if limit else None),
         }
         cr.execute(query, where_clause_params)
         alldata = {}