launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/change-field-utility into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/change-field-utility/+merge/117934

This branch adds a utility which leverages the beloved signal system to offer the ability to react to field being changed on models.  It uses that helper to replace the code which was doing exactly that (but in an ad-hoc fashion) in dns.py.  I'm writing this tools because I'll it to listen to the nodegroup's name (i.e. the zone name) being changed.
-- 
https://code.launchpad.net/~rvb/maas/change-field-utility/+merge/117934
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/change-field-utility into lp:maas.

=== modified file 'src/maasserver/dns.py'
--- src/maasserver/dns.py	2012-08-02 08:24:58 +0000
+++ src/maasserver/dns.py	2012-08-02 15:54:36 +0000
@@ -27,7 +27,6 @@
 from django.db.models.signals import (
     post_delete,
     post_save,
-    pre_save,
     )
 from django.dispatch import receiver
 from maasserver.exceptions import MAASException
@@ -42,6 +41,7 @@
     INT_MAX,
     Sequence,
     )
+from maasserver.signals import connect_to_field_change
 from netaddr import (
     IPAddress,
     IPNetwork,
@@ -145,29 +145,15 @@
         change_dns_zones(instance.nodegroup)
 
 
-HOSTNAME_UPDATE_FLAG = '_hostname_updated'
-
-
-@receiver(pre_save, sender=Node)
-def dns_pre_save_Node(sender, instance, **kwargs):
-    """When a Node's hostname is changed, flag that node."""
-    if is_dns_enabled():
-        try:
-            old_node = Node.objects.get(pk=instance.pk)
-        except Node.DoesNotExist:
-            pass  # Node is new, no lease can exist yet.
-        else:
-            if old_node.hostname != instance.hostname:
-                setattr(instance, HOSTNAME_UPDATE_FLAG, True)
-
-
-@receiver(post_save, sender=Node)
-def dns_post_save_Node(sender, instance, **kwargs):
+def dns_post_edit_hostname_Node(instance, old_field):
     """When a Node has been flagged, update the related zone."""
-    if hasattr(instance, HOSTNAME_UPDATE_FLAG):
+    if is_dns_enabled():
         change_dns_zones(instance.nodegroup)
 
 
+connect_to_field_change(dns_post_edit_hostname_Node, Node, 'hostname')
+
+
 def get_zone(nodegroup, serial=None):
     """Create a :class:`DNSZoneConfig` object from a nodegroup.
 

=== added file 'src/maasserver/signals.py'
--- src/maasserver/signals.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/signals.py	2012-08-02 15:54:36 +0000
@@ -0,0 +1,56 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Signal utilities."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'connect_to_field_change',
+    ]
+
+from django.db.models.signals import (
+    post_save,
+    pre_save,
+    )
+
+
+def connect_to_field_change(callback, model, field_name):
+    """Call the provided callback when a field is modified on a model.
+
+    The provided `callback` method will be called when the field named
+    `fieldname` of an object of type `model` is changed.
+
+    The signature of the callback method should be the following:
+
+    >>> def callback(instance, old_field):
+        pass
+
+    Where `instance` is the object which has just being saved to the database
+    and `old_field` is the old value of the field (different from the value of
+    the field in `instance`).
+    """
+    flag = '_field_updated_%s' % field_name
+
+    # Record if the field we're interested in has changed.
+    def pre_save_callback(sender, instance, **kwargs):
+        try:
+            old_object = model.objects.get(pk=instance.pk)
+        except model.DoesNotExist:
+            pass  # object is new.
+        else:
+            old_field = getattr(old_object, field_name)
+            if old_field != getattr(instance, field_name):
+                setattr(instance, flag, old_field)
+    pre_save.connect(pre_save_callback, sender=model, weak=False)
+
+    # Call the `callback` if the field has been marked 'dirty'.
+    def post_save_callback(sender, instance, created, **kwargs):
+        if hasattr(instance, flag):
+            callback(instance, getattr(instance, flag))
+    post_save.connect(post_save_callback, sender=model, weak=False)

=== modified file 'src/maasserver/tests/models.py'
--- src/maasserver/tests/models.py	2012-05-08 07:19:41 +0000
+++ src/maasserver/tests/models.py	2012-08-02 15:54:36 +0000
@@ -12,6 +12,7 @@
 __metaclass__ = type
 __all__ = [
     'JSONFieldModel',
+    'FieldChangeTestModel',
     ]
 
 from django.db.models import (
@@ -35,3 +36,8 @@
     # This model inherits from TimestampedModel so it will have a 'created'
     # field and an 'updated' field.
     pass
+
+
+class FieldChangeTestModel(Model):
+    name1 = CharField(max_length=255, unique=False)
+    name2 = CharField(max_length=255, unique=False)

=== added file 'src/maasserver/tests/test_signals.py'
--- src/maasserver/tests/test_signals.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_signals.py	2012-08-02 15:54:36 +0000
@@ -0,0 +1,53 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for signals helpers."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from maasserver.signals import connect_to_field_change
+from maasserver.testing.factory import factory
+from maasserver.testing.testcase import TestModelTestCase
+from maasserver.tests.models import FieldChangeTestModel
+from maastesting.fakemethod import FakeMethod
+
+
+class ConnectToFieldChangeTest(TestModelTestCase):
+    """Testing for the method `connect_to_field_change`."""
+
+    app = 'maasserver.tests'
+
+    def test_connect_to_field_change_calls_callback(self):
+        old_name1_value = factory.getRandomString()
+        obj = FieldChangeTestModel(name1=old_name1_value)
+        obj.save()
+        callback = FakeMethod()
+        connect_to_field_change(callback, FieldChangeTestModel, 'name1')
+        obj.name1 = factory.getRandomString()
+        obj.save()
+        self.assertEqual(
+            (1, [(obj, old_name1_value)]),
+            (callback.call_count, callback.extract_args()))
+
+    def test_connect_to_field_change_ignores_changes_to_other_fields(self):
+        obj = FieldChangeTestModel(name2=factory.getRandomString())
+        obj.save()
+        callback = FakeMethod()
+        connect_to_field_change(callback, FieldChangeTestModel, 'name1')
+        obj.name2 = factory.getRandomString()
+        obj.save()
+        self.assertEqual(0, callback.call_count)
+
+    def test_connect_to_field_change_ignores_object_creation(self):
+        callback = FakeMethod()
+        connect_to_field_change(callback, FieldChangeTestModel, 'name1')
+        obj = FieldChangeTestModel(name1=factory.getRandomString())
+        obj.save()
+        self.assertEqual(0, callback.call_count)