From 62a62d2ae4ceb31c7d843f8dd6ce536e148ac309 Mon Sep 17 00:00:00 2001 From: Aran Keyhan-Rad Date: Fri, 30 Oct 2026 14:16:19 +0000 Subject: [PATCH] Fix move_before to the leftmost child position (#68) move_before on a parent's first child returned early without moving, and the left-sibling lookup could pick a same-level cousin from another subtree. Restrict the lookup to the same parent, order by lft, and insert as the leftmost child when there is no left sibling. Co-Authored-By: Claude Opus 5.5 --- sqlalchemy_mptt/events.py | 11 +++++-- sqlalchemy_mptt/tests/cases/move_node.py | 40 ++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/sqlalchemy_mptt/events.py b/sqlalchemy_mptt/events.py index 718c315..e97ec19 100644 --- a/sqlalchemy_mptt/events.py +++ b/sqlalchemy_mptt/events.py @@ -198,6 +198,7 @@ def mptt_before_update(mapper, connection, instance): mptt_move_inside = None left_sibling = None left_sibling_tree_id = None + move_to_leftmost = False if hasattr(instance, 'mptt_move_inside'): mptt_move_inside = instance.mptt_move_inside @@ -230,9 +231,10 @@ def mptt_before_update(mapper, connection, instance): and_( table.c.level == right_sibling_level, table.c.tree_id == right_sibling_tree_id, + table.c.parent_id == right_sibling_parent, table.c.lft < right_sibling_left ) - ) + ).order_by(table.c.lft) ).fetchall() if current_lvl_nodes: ( @@ -250,6 +252,10 @@ def mptt_before_update(mapper, connection, instance): # if move_before to top level elif not right_sibling_parent: left_sibling_tree_id = right_sibling_tree_id - 1 + # move_before the first child: insert as leftmost child (#68) + else: + instance.parent_id = right_sibling_parent + move_to_leftmost = True # if placed after a particular node if hasattr(instance, 'mptt_move_after'): @@ -321,7 +327,8 @@ def mptt_before_update(mapper, connection, instance): # XXX why this str() around parent_id comparison? if not left_sibling \ and str(node_parent_id) == str(instance.parent_id) \ - and not mptt_move_inside: + and not mptt_move_inside \ + and not move_to_leftmost: if left_sibling_tree_id is None: return diff --git a/sqlalchemy_mptt/tests/cases/move_node.py b/sqlalchemy_mptt/tests/cases/move_node.py index 1c1a20e..225f9f6 100644 --- a/sqlalchemy_mptt/tests/cases/move_node.py +++ b/sqlalchemy_mptt/tests/cases/move_node.py @@ -197,6 +197,46 @@ def test_move_before_function(self): ], self.result.all()) + def test_move_before_leftmost_child(self): + """ Move node(6) before node(5), the first child of node(4). + + Regression test for #68: moving to the leftmost position did + nothing, and node(3) (a cousin at the same level) must not be + mistaken for the left sibling. + """ + node = self.session.query(self.model)\ + .filter(self.model.get_pk_column() == 6).one() + node.move_before("5") + _level = node.get_default_level() + self.assertEqual( + [ + # id lft rgt lvl parent tree + (1, 1, 22, _level + 0, None, 1), + (2, 2, 5, _level + 1, 1, 1), + (3, 3, 4, _level + 2, 2, 1), + (4, 6, 11, _level + 1, 1, 1), + (5, 9, 10, _level + 2, 4, 1), + (6, 7, 8, _level + 2, 4, 1), + (7, 12, 21, _level + 1, 1, 1), + (8, 13, 16, _level + 2, 7, 1), + (9, 14, 15, _level + 3, 8, 1), + (10, 17, 20, _level + 2, 7, 1), + (11, 18, 19, _level + 3, 10, 1), + + (12, 1, 22, _level + 0, None, 2), + (13, 2, 5, _level + 1, 12, 2), + (14, 3, 4, _level + 2, 13, 2), + (15, 6, 11, _level + 1, 12, 2), + (16, 7, 8, _level + 2, 15, 2), + (17, 9, 10, _level + 2, 15, 2), + (18, 12, 21, _level + 1, 12, 2), + (19, 13, 16, _level + 2, 18, 2), + (20, 14, 15, _level + 3, 19, 2), + (21, 17, 20, _level + 2, 18, 2), + (22, 18, 19, _level + 3, 21, 2) + ], + self.result.all()) + def test_move_one_tree_before_other_tree(self): self.fixture.add( self.model,