Fix #101: Introduce an ordered tree node base class and recommend using it

matthiask · matthiask · commit 6be8a54f128b · 2025-11-27T20:52:50.000+01:00
diff --git a/README.rst b/README.rst
@@ -111,6 +111,8 @@ Tree node with ordering among siblings
 Nodes with the same parent may be ordered among themselves. The default is to
 order siblings by their primary key but that's not always very useful.
 
+**Manual position management:**
+
 .. code-block:: python
 
     from tree_queries.models import TreeNode
@@ -122,6 +124,54 @@ order siblings by their primary key but that's not always very useful.
         class Meta:
             ordering = ["position"]
 
+**Automatic position management:**
+
+For automatic position management, use ``OrderableTreeNode`` which automatically
+assigns sequential position values to new nodes:
+
+.. code-block:: python
+
+    from tree_queries.models import OrderableTreeNode
+
+    class Category(OrderableTreeNode):
+        name = models.CharField(max_length=100)
+        # position field and ordering are inherited from OrderableTreeNode
+
+When creating new nodes without an explicit position, ``OrderableTreeNode``
+automatically assigns a position value 10 units higher than the maximum position
+among siblings. The increment of 10 (rather than 1) makes it explicit that the
+position values themselves have no inherent meaning - they are purely for relative
+ordering, not a sibling counter or index.
+
+If you need to customize the Meta class (e.g., to add verbose names or additional
+ordering fields), inherit from ``OrderableTreeNode.Meta``:
+
+.. code-block:: python
+
+    from tree_queries.models import OrderableTreeNode
+
+    class Category(OrderableTreeNode):
+        name = models.CharField(max_length=100)
+
+        class Meta(OrderableTreeNode.Meta):
+            verbose_name = "category"
+            verbose_name_plural = "categories"
+            # ordering = ["position"] is inherited from OrderableTreeNode.Meta
+
+.. code-block:: python
+
+    # Create nodes - positions are assigned automatically
+    root = Category.objects.create(name="Root")  # position=10
+    child1 = Category.objects.create(name="Child 1", parent=root)  # position=10
+    child2 = Category.objects.create(name="Child 2", parent=root)  # position=20
+    child3 = Category.objects.create(name="Child 3", parent=root)  # position=30
+
+    # Manual reordering is still possible
+    child3.position = 15  # Move between child1 and child2
+    child3.save()
+
+This approach is identical to the pattern used in feincms3's ``AbstractPage``.
+
 
 Add custom methods to queryset
 ------------------------------
@@ -538,16 +588,63 @@ To use the admin functionality, install with the ``admin`` extra:
 Usage
 -----
 
+**With automatic position management:**
+
+For the best admin experience with proper ordering, use ``OrderableTreeNode``:
+
 .. code-block:: python
 
     from django.contrib import admin
     from tree_queries.admin import TreeAdmin
-    from .models import Category
+    from tree_queries.models import OrderableTreeNode
+
+    class Category(OrderableTreeNode):
+        name = models.CharField(max_length=100)
+        # position field and ordering are inherited from OrderableTreeNode
+
+    @admin.register(Category)
+    class CategoryAdmin(TreeAdmin):
+        list_display = [*TreeAdmin.list_display, "name"]
+        position_field = "position"  # Enables sibling ordering controls
+
+**With manual position management:**
+
+If you prefer to manage positions yourself:
+
+.. code-block:: python
+
+    from django.contrib import admin
+    from django.db.models import Max
+    from tree_queries.admin import TreeAdmin
+    from tree_queries.models import TreeNode
+
+    class Category(TreeNode):
+        name = models.CharField(max_length=100)
+        position = models.PositiveIntegerField(default=0)
+
+        class Meta:
+            ordering = ["position"]
+
+        def save(self, *args, **kwargs):
+            # Custom position logic here
+            if not self.position:
+                self.position = (
+                    10
+                    + (
+                        self.__class__._default_manager.filter(parent_id=self.parent_id)
+                        .order_by()
+                        .aggregate(p=Max("position"))["p"]
+                        or 0
+                    )
+                )
+            super().save(*args, **kwargs)
+
+        save.alters_data = True
 
     @admin.register(Category)
     class CategoryAdmin(TreeAdmin):
-        list_display = [*TreeAdmin.list_display, "name", "is_active"]
-        position_field = "position"  # Optional: field used for sibling ordering
+        list_display = [*TreeAdmin.list_display, "name"]
+        position_field = "position"
 
 The ``TreeAdmin`` provides:
 
@@ -617,5 +714,6 @@ maintain their relative order after the migration.
 
 Note that the position field is used purely for ordering siblings and is not an
 index. By default, django-tree-queries' admin interface starts with a position
-value of 10 and increments by 10 (10, 20, 30, etc.) to make it clear that the
-value is not an index, but just something to order siblings by.
+value of 10 and increments by 10 (10, 20, 30, etc.) to make it explicit that the
+position values themselves have no inherent meaning - they are purely for relative
+ordering, not a sibling counter or index.
diff --git a/tests/testapp/models.py b/tests/testapp/models.py
@@ -2,7 +2,7 @@
 
 from django.db import models
 
-from tree_queries.models import TreeNode
+from tree_queries.models import OrderableTreeNode, TreeNode
 from tree_queries.query import TreeQuerySet
 
 
@@ -139,3 +139,10 @@ class OneToOneRelatedOrder(models.Model):
 
     def __str__(self):
         return ""
+
+
+class OrderedModel(OrderableTreeNode):
+    name = models.CharField(max_length=100)
+
+    def __str__(self):
+        return self.name
diff --git a/tests/testapp/test_ordered.py b/tests/testapp/test_ordered.py
@@ -0,0 +1,150 @@
+from django.test import TestCase
+
+from .models import OrderedModel
+
+
+class OrderableTreeNodeTestCase(TestCase):
+    def test_automatic_position_assignment(self):
+        """Test that position is automatically assigned to new nodes"""
+        root = OrderedModel.objects.create(name="Root")
+        assert root.position == 10
+
+        child1 = OrderedModel.objects.create(name="Child 1", parent=root)
+        assert child1.position == 10
+
+        child2 = OrderedModel.objects.create(name="Child 2", parent=root)
+        assert child2.position == 20
+
+        child3 = OrderedModel.objects.create(name="Child 3", parent=root)
+        assert child3.position == 30
+
+    def test_manual_position_respected(self):
+        """Test that manually set positions are not overwritten"""
+        root = OrderedModel.objects.create(name="Root", position=100)
+        assert root.position == 100
+
+        child = OrderedModel.objects.create(name="Child", parent=root, position=50)
+        assert child.position == 50
+
+    def test_position_increments_from_max(self):
+        """Test that positions increment from the current maximum"""
+        root = OrderedModel.objects.create(name="Root")
+
+        # Create children with custom positions
+        OrderedModel.objects.create(name="Child 1", parent=root, position=100)
+        OrderedModel.objects.create(name="Child 2", parent=root, position=200)
+
+        # Next auto-assigned position should be max + 10
+        child3 = OrderedModel.objects.create(name="Child 3", parent=root)
+        assert child3.position == 210
+
+    def test_siblings_ordered_by_position(self):
+        """Test that siblings are correctly ordered by position"""
+        root = OrderedModel.objects.create(name="Root")
+
+        child1 = OrderedModel.objects.create(name="Child 1", parent=root)
+        child2 = OrderedModel.objects.create(name="Child 2", parent=root)
+        child3 = OrderedModel.objects.create(name="Child 3", parent=root)
+
+        siblings = list(root.children.all())
+        assert siblings[0] == child1
+        assert siblings[1] == child2
+        assert siblings[2] == child3
+
+    def test_reordering_siblings(self):
+        """Test that siblings can be manually reordered"""
+        root = OrderedModel.objects.create(name="Root")
+
+        child1 = OrderedModel.objects.create(name="Child 1", parent=root)  # position=10
+        child2 = OrderedModel.objects.create(name="Child 2", parent=root)  # position=20
+        child3 = OrderedModel.objects.create(name="Child 3", parent=root)  # position=30
+
+        # Move child3 between child1 and child2
+        child3.position = 15
+        child3.save()
+
+        siblings = list(root.children.all())
+        assert siblings[0] == child1
+        assert siblings[1] == child3
+        assert siblings[2] == child2
+
+    def test_position_per_parent(self):
+        """Test that positions are assigned per parent"""
+        root1 = OrderedModel.objects.create(name="Root 1")
+        root2 = OrderedModel.objects.create(name="Root 2")
+
+        child1_1 = OrderedModel.objects.create(name="Child 1-1", parent=root1)
+        child2_1 = OrderedModel.objects.create(name="Child 2-1", parent=root2)
+
+        # Both should get position 10 since they have different parents
+        assert child1_1.position == 10
+        assert child2_1.position == 10
+
+        child1_2 = OrderedModel.objects.create(name="Child 1-2", parent=root1)
+        child2_2 = OrderedModel.objects.create(name="Child 2-2", parent=root2)
+
+        # Both should get position 20
+        assert child1_2.position == 20
+        assert child2_2.position == 20
+
+    def test_ordering_with_tree_queries(self):
+        """Test that ordering works correctly with tree queries"""
+        root = OrderedModel.objects.create(name="Root")
+        child1 = OrderedModel.objects.create(name="Child 1", parent=root)
+        OrderedModel.objects.create(name="Grandchild 1", parent=child1)
+        OrderedModel.objects.create(name="Grandchild 2", parent=child1)
+        OrderedModel.objects.create(name="Child 2", parent=root)
+
+        # Get tree with tree fields
+        nodes = list(OrderedModel.objects.with_tree_fields())
+
+        # Verify depth-first order respects position ordering
+        assert nodes[0].name == "Root"
+        assert nodes[1].name == "Child 1"
+        assert nodes[2].name == "Grandchild 1"
+        assert nodes[3].name == "Grandchild 2"
+        assert nodes[4].name == "Child 2"
+
+    def test_update_preserves_position(self):
+        """Test that updating a node doesn't change its position"""
+        root = OrderedModel.objects.create(name="Root")
+        child = OrderedModel.objects.create(name="Child", parent=root)
+
+        original_position = child.position
+        assert original_position == 10
+
+        # Update the name
+        child.name = "Updated Child"
+        child.save()
+
+        # Position should remain the same
+        assert child.position == original_position
+
+    def test_zero_position_is_replaced(self):
+        """Test that position=0 triggers auto-assignment"""
+        root = OrderedModel.objects.create(name="Root")
+
+        # Even if we explicitly set position=0, it should be replaced on create
+        child = OrderedModel(name="Child", parent=root, position=0)
+        child.save()
+
+        assert child.position == 10
+
+        # Create another child to verify positions increment correctly
+        child2 = OrderedModel.objects.create(name="Child 2", parent=root)
+        assert child2.position == 20
+
+    def test_ordering_inherited_from_meta(self):
+        """Test that ordering is inherited from OrderableTreeNode.Meta"""
+        # This test verifies that the Meta.ordering is properly inherited
+        root = OrderedModel.objects.create(name="Root")
+        OrderedModel.objects.create(name="Child 2", parent=root, position=20)
+        OrderedModel.objects.create(name="Child 1", parent=root, position=10)
+        OrderedModel.objects.create(name="Child 3", parent=root, position=30)
+
+        # Query without explicit ordering should use Meta.ordering
+        children = list(OrderedModel.objects.filter(parent=root))
+
+        assert children[0].name == "Child 1"
+        assert children[1].name == "Child 2"
+        assert children[2].name == "Child 3"
diff --git a/tree_queries/models.py b/tree_queries/models.py
@@ -1,5 +1,6 @@
 from django.core.exceptions import ValidationError
 from django.db import models
+from django.db.models import Max
 from django.utils.translation import gettext_lazy as _
 
 from tree_queries.fields import TreeNodeForeignKey
@@ -55,3 +56,48 @@ def clean(self):
             )
         ):
             raise ValidationError(_("A node cannot be made a descendant of itself."))
+
+
+class OrderableTreeNode(TreeNode):
+    """
+    A TreeNode with automatic position management for consistent sibling ordering.
+
+    This mixin provides automatic position value assignment when creating new nodes,
+    ensuring siblings are properly ordered. When a node is saved without an explicit
+    position value, it automatically receives a position 10 units higher than the
+    maximum position among its siblings.
+
+    Usage:
+        class Category(OrderableTreeNode):
+            name = models.CharField(max_length=100)
+            # position field and ordering are provided by OrderableTreeNode
+
+    The position field increments by 10 (rather than 1) to make it explicit that
+    the position values themselves have no inherent meaning - they are purely for
+    relative ordering, not a sibling counter or index. This approach is identical
+    to the one used in feincms3's AbstractPage.
+    """
+
+    position = models.PositiveIntegerField(default=0, db_index=True)
+
+    class Meta:
+        abstract = True
+        ordering = ["position"]
+
+    def save(self, *args, **kwargs):
+        """
+        Automatically assigns a position value if not set.
+
+        If the position is 0 (the default), calculates a new position by finding
+        the maximum position among siblings and adding 10.
+        """
+        if not self.position:
+            self.position = 10 + (
+                self.__class__._default_manager.filter(parent_id=self.parent_id)
+                .order_by()
+                .aggregate(p=Max("position"))["p"]
+                or 0
+            )
+        super().save(*args, **kwargs)
+
+    save.alters_data = True
