Permalink
Browse files

Porting acacia code to use django-mptt as the base.

This is the bulk of the code changes, excising any use of
django-treebeard and replacing it all with django-mptt. All of the
tests, bar one (which is a bad test anyway) pass.

Documentation hasn't been updated and there are still a couple of FIXME
items in the code.
  • Loading branch information...
1 parent 986f4c0 commit 72aeebd1045a7ee055452841eb7685597ea38b71 Malcolm Tredinnick committed Oct 1, 2009
Showing with 99 additions and 315 deletions.
  1. +62 −68 acacia/models.py
  2. +34 −88 acacia/tests.py
  3. +0 −156 acacia/treebeard_mods.py
  4. +2 −2 testing/runtests.py
  5. +1 −1 testing/test_settings.py
View
@@ -7,7 +7,7 @@
from django.db import models
-import treebeard_mods
+import mptt
class TopicManager(models.Manager):
@@ -24,23 +24,28 @@ def get_by_full_name(self, full_name):
# Ensure foo//bar is the same as foo/bar. Nice to have.
sep = self.model.separator
pieces = [o for o in full_name.split(sep) if o]
- normalised_path = sep.join(pieces)
- return self.get(full_name=normalised_path)
+ for candidate in self.filter(level=len(pieces)-1, name=pieces[-1]):
+ parents = candidate.get_ancestors().values_list("name", flat=True)
+ if list(parents) == pieces[:-1]:
+ return candidate
+ raise self.model.DoesNotExist
def get_subtree(self, full_name):
"""
Returns a list containing the tag with the given full name and all tags
with this tag as an ancestor. The first item in the list will be the
tag with the passed in long name.
- Raises Tag.DoesNotExist if there is no tag with 'long_name'.
+ Raises Topic.DoesNotExist if there is no tag with 'long_name'.
"""
- return self.model.get_tree(self.get_by_full_name(full_name))
+ return self.get_by_full_name(full_name).get_descendants(True)
def get_or_create_by_full_name(self, full_name):
"""
Retrieves a topic with the given full_name. If the topic doesn't exist,
- it is created (along with all the necessary parent topics).
+ it is created (along with all the necessary parent topics). This is
+ analogous to Django's get_or_create() manager method, with additional
+ logic to handle long names.
Returns a pair: the topic object and a boolean flag indicating whether
or not a new object was created.
@@ -51,19 +56,19 @@ def get_or_create_by_full_name(self, full_name):
except self.model.DoesNotExist:
pass
+ # TODO: Feels like I should be able to do this with less queries.
pieces = full_name.rsplit(self.model.separator, 1)
if len(pieces) == 1:
return self.model.create(name=pieces[0]), True
parent, created = self.get_or_create_by_full_name(pieces[0])
if not pieces[1]:
# full_name ended with a trailing separator (e.g. /foo/bar/).
return parent, created
- node = self.model(name=pieces[-1])
- parent.add_child(node)
+ node = self.create(name=pieces[-1], parent=parent)
return node, True
-class AbstractTopic(treebeard_mods.MP_Node):
+class AbstractTopic(models.Model):
"""
A node in a hierarchical storage tree, representing topics of some kind.
The name of the topic is the name of the parent, followed by the node's
@@ -73,71 +78,58 @@ class AbstractTopic(treebeard_mods.MP_Node):
just the name via inheritance.
"""
name = models.CharField(max_length=50)
- # Denormalise things a bit so that full name lookups are fast.
- full_name = models.CharField(max_length=512, unique=True)
-
- node_order_by = ["name"]
- separator = "/"
+ parent = models.ForeignKey("self", null=True, blank=True,
+ related_name="children")
+ # The level of this node in the tree it belongs to (used by mptt).
+ level = models.PositiveIntegerField()
objects = TopicManager()
+ separator = u"/"
- class Meta(treebeard_mods.MP_Node.Meta):
+ class Meta:
abstract = True
def __unicode__(self):
- if not hasattr(self, "full_name") or not self.full_name:
- self._set_full_name()
- return self.full_name
-
- def _set_full_name(self):
- """
- Sets the full_name attribute to the correct value. Generally called as
- part of saving the class, but can also be called by other class methods
- that need an accurate value before displaying the __unicode__ output,
- for example.
- """
- if self.depth == 1:
- self.full_name = self.name
- else:
- parent = self.separator.join(
- list(self.get_ancestors().values_list("name", flat=True)))
- self.full_name = "%s%s%s" % (parent, self.separator, self.name)
-
- def save(self, *args, **kwargs):
- """
- Updates the full_name attribute prior to saving (incurs an extra lookup
- query on each save, but saving is much less common than retrieval).
- """
- # FIXME: Need to handle the case of creating a duplicate node.
- self._set_full_name()
- return super(AbstractTopic, self).save(*args, **kwargs)
-
- def move(self, target, pos=None, update_funcs=()):
- """
- Moves current node and all descendents to a new position in the tree.
-
- If "update_funcs" is given, it's a list of callables that are passed
- child nodes as the first argument. Subclasses can use this list to add
- their own hooks for things needing updating after moves.
- """
- # A little runtime efficiency is sacrificed here for the sake of code
- # simplicity. The default treebeard move() method uses custom SQL to
- # update the paths on all the children. This class has to also update
- # all the full_name attributes. The latter is done as a separate query
- # for each child, on the grounds that moving is far less common than
- # reading for these types of hierarchies.
- super(AbstractTopic, self).move(target, pos)
-
- # After call to super's move(), "self" no longer has valid path
- # information, so have to refetch the data from the database.
- for node in self.__class__.objects.get(pk=self.pk).get_tree():
- # FIXME: Since I already know the ancestor names at this point,
- # being able to pass them into _set_full_name() would be an
- # improvement.
- node._set_full_name()
- for func in update_funcs:
- func(node)
- node.save()
+ return self.full_name()
+
+ def full_name(self):
+ if not hasattr(self, "_full_name_cache"):
+ if self.level:
+ parents = self.separator.join(
+ self.get_ancestors().values_list("name", flat=True))
+ self._full_name_cache = u"%s%s%s" % (parents, self.separator,
+ self.name)
+ else:
+ self._full_name_cache = self.name
+ return self._full_name_cache
+
+ # FIXME: Implement this in a reasonable fashion for mptt-based trees.
+ ##def move_to(self, target, pos=None, update_funcs=()):
+ ## """
+ ## Moves current node and all descendents to a new position in the tree.
+
+ ## If "update_funcs" is given, it's a list of callables that are passed
+ ## child nodes as the first argument. Subclasses can use this list to add
+ ## their own hooks for things needing updating after moves.
+ ## """
+ ## # A little runtime efficiency is sacrificed here for the sake of code
+ ## # simplicity. The default treebeard move() method uses custom SQL to
+ ## # update the paths on all the children. This class has to also update
+ ## # all the full_name attributes. The latter is done as a separate query
+ ## # for each child, on the grounds that moving is far less common than
+ ## # reading for these types of hierarchies.
+ ## super(AbstractTopic, self).move(target, pos)
+
+ ## # After call to super's move(), "self" no longer has valid path
+ ## # information, so have to refetch the data from the database.
+ ## for node in self.__class__.objects.get(pk=self.pk).get_tree():
+ ## # FIXME: Since I already know the ancestor names at this point,
+ ## # being able to pass them into _set_full_name() would be an
+ ## # improvement.
+ ## node._set_full_name()
+ ## for func in update_funcs:
+ ## func(node)
+ ## node.save()
class Topic(AbstractTopic):
"""
@@ -146,3 +138,5 @@ class Topic(AbstractTopic):
"""
pass
+mptt.register(Topic, order_insertion_by=["name"])
+
View
@@ -1,6 +1,5 @@
"""
-Tests for both the local treebeard overrides and the whole hierarchical topic
-structure.
+Tests for the hierarchical topic structure.
"""
from django import db, test
@@ -12,46 +11,22 @@ class BaseTestSetup(object):
"""
def setUp(self):
"""
- Load some potentially confusing topic data. There are four topics here
- are:
- - a/b/c
- - a/x/c
- - c/b/d
- - x/y/c
+ Load some potentially confusing topic data.
- The tests can use the fact that node "c" appears as the leaf of three
+ The tests can use the fact that node "c" appears as the leaf of multiple
similar, but different paths and that some nodes appear multiple times
(and node "d" only appears once).
-
"""
- content = [
- {"data": {"name": "a"},
- "children": [
- {"data": {"name": "b"},
- "children": [
- {"data": {"name": "c"}},
- ]},
- {"data": {"name": "x"},
- "children": [
- {"data": {"name": "c"}},
- ]},
- ]},
- {"data": {"name": "c"},
- "children": [
- {"data": {"name": "b"},
- "children": [
- {"data": {"name": "d"}},
- ]},
- ]},
- {"data": {"name": "x"},
- "children": [
- {"data": {"name": "y"},
- "children": [
- {"data": {"name": "c"}},
- ]},
- ]},
- ]
- models.Topic.load_bulk(content)
+ nodes = ["a/b/c",
+ "a/x/c",
+ "c/b/d",
+ "x/y/c"]
+ for node in nodes:
+ pieces = node.split("/")
+ obj = models.Topic.objects.get_or_create(name=pieces[0], level=0)[0]
+ for piece in pieces[1:]:
+ obj = models.Topic.objects.get_or_create(name=piece,
+ parent=obj)[0]
class TopicTest(BaseTestSetup, test.TestCase):
"""
@@ -91,9 +66,9 @@ def test_get_by_full_name2(self):
def test_get_by_full_name3(self):
"""
- Tests that we can correctly retrieve a node given its full name. When
- the fulli name only contains a single component, there's a third
- processing path involved and this is what's tested here.
+ Tests that we can correctly retrieve a node given its full name. In
+ this case, we're testing the potential edge-case where the name has
+ only a single component.
"""
# Use "c" as the test here, since there's more than one Topic instance
# with a name of "c", but only one topic whose full name is "c".
@@ -104,7 +79,7 @@ def test_get_by_full_name3(self):
def test_get_subtree(self):
"""
Tests that the subtree rooted at a particular node (given by full name)
- is retrieved correctly.
+ is retrieved (and sorted) correctly.
"""
result = [unicode(obj) for obj in models.Topic.objects.get_subtree("a")]
expected = [u"a", u"a/b", u"a/b/c", u"a/x", u"a/x/c"]
@@ -120,15 +95,15 @@ def test_name_normalisation(self):
topic3 = models.Topic.objects.get_by_full_name("c/b/d/")
topic4 = models.Topic.objects.get_by_full_name("/c/b/d/")
self.assertEquals(topic1, topic2)
- self.assertEquals(topic3, topic2)
+ self.assertEquals(topic2, topic3)
self.assertEquals(topic3, topic4)
def test_unicode(self):
"""
Tests that the __unicode__() method displays the correct thing.
"""
- topic = models.Topic.objects.get(name="x", depth=1)
- tree = models.Topic.get_tree(topic)
+ topic = models.Topic.objects.get(name="x", level=0)
+ tree = topic.get_descendants(True)
result = [unicode(obj) for obj in tree]
expected = [u"x", u"x/y", u"x/y/c"]
self.assertEquals(result, expected)
@@ -141,7 +116,7 @@ def test_change_parent(self):
# Move the "x" in "a/x" to be a child of "a/b" (so it becomes "a/b/x").
target = models.Topic.objects.get_by_full_name("a/b")
node = models.Topic.objects.get_by_full_name("a/x")
- node.move(target, "sorted-child")
+ node.move_to(target)
# The "a/x/c" node should have been moved to "a/b/x/c" as part of this.
models.Topic.objects.get_by_full_name("a/b/x/c")
@@ -150,17 +125,19 @@ def test_change_parent(self):
self.assertRaises(models.Topic.DoesNotExist,
models.Topic.objects.get_by_full_name, "a/x/c")
- def test_create_duplicate_entry(self):
- """
- Tests that an appropriate error is raised if the tree is manipulated in
- such a way that a duplicate full name would be created.
- """
- target = models.Topic.objects.get_by_full_name("a")
- node = models.Topic.objects.get_by_full_name("x")
- # Attempting to move "x" under "a" raises an error at the database
- # level because we already have an "a/x" node.
- self.assertRaises(db.IntegrityError, node.move, target,
- "sorted-child")
+ # TODO: merging nodes with common names isn't implemented yet. The
+ # behaviour of this test is incorrect in any case (they shoul merge, not
+ # error out).
+ #def test_create_duplicate_entry(self):
+ # """
+ # Tests that an appropriate error is raised if the tree is manipulated in
+ # such a way that a duplicate full name would be created.
+ # """
+ # target = models.Topic.objects.get_by_full_name("a")
+ # node = models.Topic.objects.get_by_full_name("x")
+ # # Attempting to move "x" under "a" raises an error at the database
+ # # level because we already have an "a/x" node.
+ # self.assertRaises(db.IntegrityError, node.move_to, target)
def test_create_by_full_name1(self):
"""
@@ -202,34 +179,3 @@ def test_create_by_full_name4(self):
self.failUnlessEqual(created, True)
self.failUnlessEqual(unicode(node), u"a/b/e")
-class TreebeardTests(BaseTestSetup, test.TestCase):
- """
- Tests for my local modification/overrides to the default treebeard
- functionality.
- """
- def test_add_instance_as_child(self):
- """
- Tests that an unsaved Topic instance can be inserted as a child of some
- other node in the tree.
- """
- root = models.Topic.get_first_root_node()
- node = models.Topic(name="new child")
- root.add_child(node)
- # The instance should be saved as part of this process.
- self.failIf(node.pk is None)
- children = [unicode(obj) for obj in root.get_children()]
- expected = [u"a/b", u"a/new child", u"a/x"]
- self.assertEquals(children, expected)
-
- def test_add_instance_as_root(self):
- """
- Tests that an unsaved Topic instance can be inserted as a new root node.
- """
- node = models.Topic(name="new root")
- models.Topic.add_root(node)
- # The instance should be saved as part of this process.
- self.failIf(node.pk is None)
- root = [unicode(obj) for obj in models.Topic.get_root_nodes()]
- expected = [u"a", u"c", u"new root", u"x"]
- self.assertEquals(root, expected)
-
Oops, something went wrong.

0 comments on commit 72aeebd

Please sign in to comment.