google-code-export / django-mptt

Automatically exported from code.google.com/p/django-mptt
Other
0 stars 0 forks source link

insert_node using last-child does not work correctly. #78

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
For example:

>>> root = Node(name='Top')
>>> root.save()
>>> node_a = Node(name='Node A')
>>> Node.tree.insert_node(node_a, root, position='last-child', save=True)
>>> node_b = Node(name='Node B')
>>> Node.tree.insert_node(node_b, root, position='last-child', save=True)

Looking at the table, the nodes are in reverse order. Node A should have a 
"lft" value of 2.

# SELECT * FROM myapp_node ORDER BY lft;
 id | parent_id |   name    | lft | rght | tree_id | level 
----+-----------+-----------+-----+------+---------+-------
 1  |           | Root      |   1 |    6 |       1 |     0
 2  |         1 | Node B    |   2 |    3 |       1 |     1
 3  |         1 | Node A    |   4 |    5 |       1 |     1

Original issue reported on code.google.com by bendavi...@gmail.com on 30 Oct 2010 at 5:48

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
Never mind, I realized I needed to get a fresh copy of root before inserting so 
that left/right values could be calculated correctly.

Original comment by bendavi...@gmail.com on 30 Oct 2010 at 6:04

GoogleCodeExporter commented 9 years ago
That's true. This issue has been raised a few times, it's because the parent 
node is still in memory and it doesn't get updated when we do our bulk update 
on the database.

However I think we probably can solve this (for this common use case at least).

insert_node() and move_node() take a parent node parameter, I guess they could 
just update the mptt fields on the parent node.

I don't think there are any adverse consequences to doing this. It doesn't 
require any more database queries. 

I think this can be done by modifying _calculate_inter_tree_move_values() to 
return modified parent lft/rght fields, and changing insert_node() and the 
_move_*() methods to use them.

Original comment by craig.ds@gmail.com on 1 Nov 2010 at 8:44

GoogleCodeExporter commented 9 years ago
Issue 66 has been merged into this issue.

Original comment by craig.ds@gmail.com on 13 Nov 2010 at 6:36

GoogleCodeExporter commented 9 years ago
Issue 32 has been merged into this issue.

Original comment by craig.ds@gmail.com on 13 Nov 2010 at 6:49

GoogleCodeExporter commented 9 years ago
I've created a branch to deal to this, and I think it looks promising:

https://github.com/django-mptt/django-mptt/tree/gc78-update-parents-when-inserti
ng

This only fixes the problem for the specific case of inserting new nodes into a 
tree. I think it's quite hard / impossible to fix this for moving existing 
nodes around, since node.rght can't be easily calculated without knowing the 
origin of the move. (not sure though, it might be doable).

This does introduce some potential backwards incompatibility, since people may 
be relying on the old behaviour. For this reason I think it's best to put it in 
0.5 rather than another 0.4.x

I also have slight reservations about making the behaviour of inserts/moves 
different, but I think it might be worth it since this is a very common problem 
and the current behaviour is a pain to use.

Feedback please :)

Original comment by craig.ds@gmail.com on 13 Nov 2010 at 8:52

GoogleCodeExporter commented 9 years ago
Migrated to https://github.com/django-mptt/django-mptt/issues/#issue/101

Original comment by craig.ds@gmail.com on 19 Dec 2010 at 1:10