Merge lp:~cedric-lebrouster/ocb-server/ocb-7.0-bug-1253052-parent-order into lp:ocb-server

[Cedric Le Brouster(OpenFire)]

Fixes parent_left and parent_right recomputation in write/create methods ( bug lp:1253052 )

[lp:~cedric-lebrouster/ocb-server/ocb-7.0-bug-1253052-parent-order]

5280. By Cedric Le Brouster(OpenFire) on 2014-03-07

Add tests for _parent_order

[Holger Brunn (Therp) (hbrunn)]

This patch looks a bit scary at first, mainly because you have to read it closely to convince yourself that it doesn't trigger a recomputation on every write().

#123-133 can be done in one SQL statement I think, given that we touch that code anyway such an optimization wouldn't hurt imho.

review: Approve (code review)

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

This patch breaks the yaml_import tests.

Extract from the logs:

TEST test_ocb70 openerp.tools.yaml_import: 2. Duplicate the parent category and verify that the children have been duplicated too and are below the new parent
ERROR test_ocb70 openerp.tools.yaml_import: AssertionError in Python code : After duplication, previous record must have old childs records only
TEST test_ocb70 openerp.tools.yaml_import: 3. Duplicate the children then reassign them to the new parent (1st method) and check the parent_store structure.
ERROR test_ocb70 openerp.tools.yaml_import: AssertionError in Python code : After duplication, previous record must have old childs records only
TEST test_ocb70 openerp.tools.yaml_import: 4. Duplicate the children then reassign them to the new parent (2nd method) and check the parent_store structure.
ERROR test_ocb70 openerp.tools.yaml_import: AssertionError in Python code : After duplication, previous record must have old childs records only
TEST test_ocb70 openerp.tools.yaml_import: 5. Duplicate the children then reassign them to the new parent (3rd method) and make sure the parent_store structure is still right.
ERROR test_ocb70 openerp.tools.yaml_import: AssertionError in Python code : After duplication, previous record must have old childs records only

Can you please check this, and adapt the tests (which seem to have some assumptions broken by your changes) or fix the code in yaml_import?

review: Needs Fixing (code review, test)

[lp:~cedric-lebrouster/ocb-server/ocb-7.0-bug-1253052-parent-order]

5281. By Cedric Le Brouster(OpenFire) on 2014-04-25

[FIX] orm: fix miscalculation of _parent_left and _parent_right in create() method for some special case.
(i.e. when new object parent's _parent_left value is 0)

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

LGTM, thanks for the fix

review: Approve (code review, test)

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

https://github.com/odoo/odoo/pull/829

[Holger Brunn (Therp) (hbrunn)]

Development for 7.0 has moved to github on https://github.com/OCA/ocb - please move your merge proposal there if it is still valid.

(I close and reject this in order to have a cleaner overview for 6.1 MPs which indeed have to be done on launchpad)

review: Disapprove

Unmerged revisions

5281. By Cedric Le Brouster(OpenFire) on 2014-04-25

[FIX] orm: fix miscalculation of _parent_left and _parent_right in create() method for some special case.
(i.e. when new object parent's _parent_left value is 0)

5280. By Cedric Le Brouster(OpenFire) on 2014-03-07

Add tests for _parent_order

5279. By Cedric Le Brouster(OpenFire) on 2014-03-06

[FIX] orm: wrong parent_left and parent_right recomputation in write/create methods (see bug 1253052)

=== modified file 'openerp/addons/base/test/base_test.yml'
--- openerp/addons/base/test/base_test.yml	2014-04-16 11:22:39 +0000
+++ openerp/addons/base/test/base_test.yml	2014-04-25 11:42:41 +0000
@@ -137,7 +137,19 @@
         assert len(old_struct) == 4, "After duplication, previous record must have old childs records only"
         assert (not set(new_struct).intersection(old_struct)), "After duplication, nodes should not be mixed"
 -
-    6. Restore pool state after the test
+    6. Tests on _parent_order.
+-
+    !python {model: res.partner.category }: |
+        another_root = self.create(cr, uid, {'name':'Root categ'})
+        struct = self.search(cr, uid, [('id','in',(ref('test_categ_root'),another_root))])
+        assert struct[0]==another_root, "Root partner categories should be sorted."
+        struct = self.search(cr, uid, [('id','in',(ref('test_categ_1'),ref('test_categ_2')))])
+        assert struct[0]==ref('test_categ_1'), "Partner categories should be sorted."
+        self.write(cr, uid, ref('test_categ_1'), {'name':'Child 3'})
+        struct = self.search(cr, uid, [('id','in',(ref('test_categ_1'),ref('test_categ_2')))])
+        assert struct[0]==ref('test_categ_2'), "Partner categories should be re-sorted after name update."
+-
+    7. Restore pool state after the test
 -
     !python {model: res.partner.category}: |
         self.pool._init = True
 
=== modified file 'openerp/osv/orm.py'
--- openerp/osv/orm.py	2014-04-07 10:57:40 +0000
+++ openerp/osv/orm.py	2014-04-25 11:42:41 +0000
@@ -4173,22 +4173,31 @@
 
         parents_changed = []
         parent_order = self._parent_order or self._order
-        if self._parent_store and (self._parent_name in vals):
+        if self._parent_store:
             # The parent_left/right computation may take up to
             # 5 seconds. No need to recompute the values if the
-            # parent is the same.
+            # parent and order field are the same.
             # Note: to respect parent_order, nodes must be processed in
             # order, so ``parents_changed`` must be ordered properly.
-            parent_val = vals[self._parent_name]
-            if parent_val:
-                query = "SELECT id FROM %s WHERE id IN %%s AND (%s != %%s OR %s IS NULL) ORDER BY %s" % \
-                                (self._table, self._parent_name, self._parent_name, parent_order)
-                cr.execute(query, (tuple(ids), parent_val))
-            else:
-                query = "SELECT id FROM %s WHERE id IN %%s AND (%s IS NOT NULL) ORDER BY %s" % \
-                                (self._table, self._parent_name, parent_order)
-                cr.execute(query, (tuple(ids),))
-            parents_changed = map(operator.itemgetter(0), cr.fetchall())
+            query = "SELECT id FROM %s WHERE id IN %%s" % (self._table,)
+            query_params = [tuple(ids)]
+            query_clause = ""
+            parent_params = [self._parent_name]
+            for param in parent_order.split(','):
+                parent_params.append(param.strip())
+            for parent_param in parent_params:
+                if parent_param in vals:
+                    parents_changed = True
+                    parent_val = vals[parent_param]
+                    if parent_val:
+                        query_clause += (query_clause and ' OR ' or '') + "%s != %%s OR %s IS NULL" % (parent_param,parent_param)
+                        query_params.append(parent_val)
+                    else:
+                        query_clause += (query_clause and ' OR ' or '') + "%s IS NOT NULL" % (parent_param,)
+            if parents_changed:
+                query += (query_clause and (' AND (' + query_clause + ')') or '') + " ORDER BY %s" % (self._parent_order,)
+                cr.execute(query, tuple(query_params))
+                parents_changed = map(operator.itemgetter(0), cr.fetchall())
 
         upd0 = []
         upd1 = []
@@ -4286,18 +4295,16 @@
             if self.pool._init:
                 self.pool._init_parent[self._name] = True
             else:
-                order = self._parent_order or self._order
-                parent_val = vals[self._parent_name]
-                if parent_val:
-                    clause, params = '%s=%%s' % (self._parent_name,), (parent_val,)
-                else:
-                    clause, params = '%s IS NULL' % (self._parent_name,), ()
-
                 for id in parents_changed:
-                    cr.execute('SELECT parent_left, parent_right FROM %s WHERE id=%%s' % (self._table,), (id,))
-                    pleft, pright = cr.fetchone()
+                    cr.execute('SELECT parent_left, parent_right, %s FROM %s WHERE id=%%s' % (self._parent_name, self._table,), (id,))
+                    pleft, pright, parent_val = cr.fetchone()
                     distance = pright - pleft + 1
 
+                    if parent_val:
+                        clause, params = '%s=%%s' % (self._parent_name,), (parent_val,)
+                    else:
+                        clause, params = '%s IS NULL' % (self._parent_name,), ()
+
                     # Positions of current siblings, to locate proper insertion point;
                     # this can _not_ be fetched outside the loop, as it needs to be refreshed
                     # after each update, in case several nodes are sequentially inserted one
@@ -4517,21 +4524,25 @@
                 self.pool._init_parent[self._name] = True
             else:
                 parent = vals.get(self._parent_name, False)
+                where_clause = ' where '+self._parent_name
                 if parent:
-                    cr.execute('select parent_right from '+self._table+' where '+self._parent_name+'=%s order by '+(self._parent_order or self._order), (parent,))
-                    pleft_old = None
-                    result_p = cr.fetchall()
-                    for (pleft,) in result_p:
-                        if not pleft:
-                            break
-                        pleft_old = pleft
-                    if not pleft_old:
-                        cr.execute('select parent_left from '+self._table+' where id=%s', (parent,))
-                        pleft_old = cr.fetchone()[0]
-                    pleft = pleft_old
-                else:
-                    cr.execute('select max(parent_right) from '+self._table)
-                    pleft = cr.fetchone()[0] or 0
+                    where_clause += '=%s'
+                    where_params = (parent,)
+                else:
+                    where_clause += ' is null'
+                    where_params = tuple()
+                cr.execute('select parent_right from '+self._table + where_clause+' order by '+(self._parent_order or self._order), where_params)
+                pleft_old = None
+                result_p = cr.fetchall()
+                for (pleft,) in result_p:
+                    if not pleft:
+                        break
+                    pleft_old = pleft
+                if not pleft_old and parent:
+                    cr.execute('select parent_left from '+self._table+' where id=%s', (parent,))
+                    pleft = cr.fetchone()[0]
+                else:
+                    pleft = pleft_old or -1
                 cr.execute('update '+self._table+' set parent_left=parent_left+2 where parent_left>%s', (pleft,))
                 cr.execute('update '+self._table+' set parent_right=parent_right+2 where parent_right>%s', (pleft,))
                 cr.execute('update '+self._table+' set parent_left=%s,parent_right=%s where id=%s', (pleft+1, pleft+2, id_new))