launchpad-reviewers team mailing list archive

[John A Meinel <john@xxxxxxxxxxxxxxxxx>]

John A Meinel has proposed merging lp:~jameinel/maas/tag-auto-populate into lp:maas.

Commit message:
Change Tag.save() to call self.populate_nodes() if the definition changed.

This makes it much easier to DTRT when using Tag, rather than having to think 'ok, did I change something that needs to rebuild?'.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jameinel/maas/tag-auto-populate/+merge/126603

This makes Tag call self.populate_nodes during save(). Should simplify things a bit for using Tag.

I followed the advice here: http://stackoverflow.com/questions/1355150/django-when-saving-how-can-you-check-if-a-field-has-changed

I almost wonder if this should be done in a 'clean' method rather than a 'save' method. Though on initial creation, you have to do it after save, because otherwise you don't have an ID for foreign-key references.

-- 
https://code.launchpad.net/~jameinel/maas/tag-auto-populate/+merge/126603
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jameinel/maas/tag-auto-populate into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-09-26 10:00:14 +0000
+++ src/maasserver/api.py	2012-09-27 05:51:21 +0000
@@ -1280,8 +1280,6 @@
             try:
                 new_tag = form.save(commit=False)
                 new_tag.save()
-                if new_tag.definition != old_definition:
-                    new_tag.populate_nodes()
                 form.save_m2m()
             except DatabaseError as e:
                 raise ValidationError(e)
@@ -1348,11 +1346,7 @@
         raise PermissionDenied()
     form = TagForm(request.data)
     if form.is_valid():
-        new_tag = form.save(commit=False)
-        new_tag.save()
-        new_tag.populate_nodes()
-        form.save_m2m()
-        return new_tag
+        return form.save()
     else:
         raise ValidationError(form.errors)
 

=== modified file 'src/maasserver/models/tag.py'
--- src/maasserver/models/tag.py	2012-09-26 10:00:14 +0000
+++ src/maasserver/models/tag.py	2012-09-27 05:51:21 +0000
@@ -94,6 +94,17 @@
 
     objects = TagManager()
 
+    def __init__(self, *args, **kwargs):
+        super(Tag, self).__init__(*args, **kwargs)
+        # Track what the original definition is, so we can detect when it
+        # changes and we need to repopulate the node<=>tag mapping.
+        # We have to check for self.id, otherwise we don't see the creation of
+        # a new definition.
+        if self.id is None:
+            self._original_definition = None
+        else:
+            self._original_definition = self.definition
+
     def __unicode__(self):
         return self.name
 
@@ -111,3 +122,9 @@
             # Is there an efficiency difference between doing
             # 'node.tags.add(self)' and 'self.node_set.add(node)' ?
             node.tags.add(self)
+
+    def save(self, *args, **kwargs):
+        super(Tag, self).save(*args, **kwargs)
+        if self.definition != self._original_definition:
+            self.populate_nodes()
+        self._original_definition = self.definition

=== modified file 'src/maasserver/tests/test_tag.py'
--- src/maasserver/tests/test_tag.py	2012-09-26 12:26:54 +0000
+++ src/maasserver/tests/test_tag.py	2012-09-27 05:51:21 +0000
@@ -43,41 +43,42 @@
         self.assertEqual([tag.id], [t.id for t in node.tags.all()])
         self.assertEqual([node.id], [n.id for n in tag.node_set.all()])
 
-    def test_populate_nodes_applies_tags_to_nodes(self):
+    def test_applies_tags_to_nodes(self):
         node1 = factory.make_node()
         node1.set_hardware_details('<node><child /></node>')
         node2 = factory.make_node()
         node2.set_hardware_details('<node />')
         tag = factory.make_tag(definition='/node/child')
-        tag.populate_nodes()
         self.assertItemsEqual([tag.name], node1.tag_names())
         self.assertItemsEqual([], node2.tag_names())
 
-    def test_populate_nodes_removes_old_values(self):
+    def test_removes_old_values(self):
         node1 = factory.make_node()
         node1.set_hardware_details('<node><foo /></node>')
         node2 = factory.make_node()
         node2.set_hardware_details('<node><bar /></node>')
         tag = factory.make_tag(definition='/node/foo')
-        tag.populate_nodes()
         self.assertItemsEqual([tag.name], node1.tag_names())
         self.assertItemsEqual([], node2.tag_names())
         tag.definition = '/node/bar'
-        tag.populate_nodes()
+        tag.save()
         self.assertItemsEqual([], node1.tag_names())
         self.assertItemsEqual([tag.name], node2.tag_names())
+        # And we notice if we change it *again* and then save.
+        tag.definition = '/node/foo'
+        tag.save()
+        self.assertItemsEqual([tag.name], node1.tag_names())
+        self.assertItemsEqual([], node2.tag_names())
 
-    def test_populate_nodes_doesnt_touch_other_tags(self):
+    def test_doesnt_touch_other_tags(self):
         node1 = factory.make_node()
         node1.set_hardware_details('<node><foo /></node>')
         node2 = factory.make_node()
         node2.set_hardware_details('<node><bar /></node>')
         tag1 = factory.make_tag(definition='/node/foo')
-        tag1.populate_nodes()
         self.assertItemsEqual([tag1.name], node1.tag_names())
         self.assertItemsEqual([], node2.tag_names())
         tag2 = factory.make_tag(definition='/node/bar')
-        tag2.populate_nodes()
         self.assertItemsEqual([tag1.name], node1.tag_names())
         self.assertItemsEqual([tag2.name], node2.tag_names())
 
@@ -118,13 +119,12 @@
 
 class TestTagTransactions(TransactionTestCase):
 
-    def test_populate_nodes_rollsback_invalid_xpath(self):
+    def test_rollsback_invalid_xpath(self):
         @transaction.commit_manually
         def setup():
             node = factory.make_node()
             node.set_hardware_details('<node><foo /></node>')
             tag = factory.make_tag(definition='/node/foo')
-            tag.populate_nodes()
             self.assertItemsEqual([tag.name], node.tag_names())
             transaction.commit()
             return tag, node
@@ -132,7 +132,7 @@
         @transaction.commit_manually
         def trigger_invalid():
             tag.definition = 'invalid::tag'
-            self.assertRaises(DatabaseError, tag.populate_nodes)
+            self.assertRaises(DatabaseError, tag.save)
             transaction.rollback()
         # Because the definition is invalid, the db should not have been
         # updated