launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-1077075 into lp:maas.

Commit message:
Fix oops when renaming accepted nodegroup without interfaces.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1077075 in MAAS: "Oops when renaming nodegroup w/o interface"
  https://bugs.launchpad.net/maas/+bug/1077075

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-1077075/+merge/133718

This came up during Q/A.  Minimal discussion with Raphaël.  The “clean” method for NodeGroup.name dereferences the result of NodeGroup.get_managed_interface() — but the result could be None.  It's not an easy situation to get into, but it's possible.

If there's no interface on a nodegroup, there's no reason to stop the user from renaming the thing.  So I changed the method to permit this scenario.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/bug-1077075/+merge/133718
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-1077075 into lp:maas.

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-11-09 09:11:51 +0000
+++ src/maasserver/forms.py	2012-11-09 17:18:25 +0000
@@ -857,6 +857,11 @@
             return new_name
 
         interface = self.instance.get_managed_interface()
+        if interface is None:
+            # No network interfaces.  It's weird, but there certainly
+            # won't be a problem with the name change.
+            return new_name
+
         if interface.management != NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS:
             # MAAS is not managing DNS on this network, so the user can
             # rename the zone at will.

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2012-11-09 09:11:51 +0000
+++ src/maasserver/tests/test_forms.py	2012-11-09 17:18:25 +0000
@@ -57,6 +57,7 @@
     MACAddress,
     Node,
     NodeGroup,
+    NodeGroupInterface,
     )
 from maasserver.models.config import DEFAULT_CONFIG
 from maasserver.node_action import (
@@ -981,3 +982,13 @@
         self.assertTrue(form.is_valid())
         form.save()
         self.assertEqual(data['name'], reload_object(nodegroup).name)
+
+    def test_accepts_name_change_if_nodegroup_has_no_interface(self):
+        nodegroup, node = make_unrenamable_nodegroup_with_node()
+        NodeGroupInterface.objects.filter(nodegroup=nodegroup).delete()
+        data = self.make_form_data(nodegroup)
+        data['name'] = factory.make_name('new-name')
+        form = NodeGroupEdit(instance=nodegroup, data=data)
+        self.assertTrue(form.is_valid())
+        form.save()
+        self.assertEqual(data['name'], reload_object(nodegroup).name)