Merge lp:~rvb/maas/bug-1234853 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Fix the add-new-interface UI so that it doesn't blow up when the nodegroup is already 'managed' and one is trying to add another 'managed' interface.

Instead of using a custom save() method on NodeGroupInterfaceForm, simply use Django's built-in 'instance' parameter to give the form an instance initialized with the right nodegroup. It means that the instance's nodegroup is set right from the start (as opposed to just before save() is called). This allows Django to perform the model validation (when the existence of another managed interface is checked) as part of the form validation.

[Jeroen T. Vermeulen (jtv)]

Looks sensible. In src/maasserver/views/settings_clusters.py, is there any need for get_form_kwargs() to check whether 'instance' might already be set?

review: Approve

[Raphaël Badin (rvb)]

> Looks sensible. In src/maasserver/views/settings_clusters.py, is there any
> need for get_form_kwargs() to check whether 'instance' might already be set?

Well, not really, this is a method on ClusterInterfaceCreate(). That view is there to handle the *creation* of an interface so it's guaranteed that no instance will be there already. I'll put an assertion.

Thanks for the review!

[lp:~rvb/maas/bug-1234853]

1659. By Raphaël Badin on 2013-10-04

Add assertion.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2013-09-25 12:34:45 +0000
+++ src/maasserver/api.py	2013-10-04 10:46:18 +0000
@@ -1659,10 +1659,10 @@
         :type ip_range_high: basestring (IP Address)
         """
         nodegroup = get_object_or_404(NodeGroup, uuid=uuid)
-        form = NodeGroupInterfaceForm(request.data)
+        instance = NodeGroupInterface(nodegroup=nodegroup)
+        form = NodeGroupInterfaceForm(request.data, instance=instance)
         if form.is_valid():
-            return form.save(
-                nodegroup=nodegroup)
+            return form.save()
         else:
             raise ValidationError(form.errors)
 

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2013-09-16 05:13:13 +0000
+++ src/maasserver/forms.py	2013-10-04 10:46:18 +0000
@@ -674,14 +674,6 @@
             'ip_range_high',
             )
 
-    def save(self, nodegroup=None, commit=True, *args, **kwargs):
-        interface = super(NodeGroupInterfaceForm, self).save(commit=False)
-        if nodegroup is not None:
-            interface.nodegroup = nodegroup
-        if commit:
-            interface.save(*args, **kwargs)
-        return interface
-
 
 INTERFACES_VALIDATION_ERROR_MESSAGE = (
     "Invalid json value: should be a list of dictionaries, each containing "
@@ -766,8 +758,9 @@
     def save(self):
         nodegroup = super(NodeGroupWithInterfacesForm, self).save()
         for interface in self.cleaned_data['interfaces']:
-            form = NodeGroupInterfaceForm(data=interface)
-            form.save(nodegroup=nodegroup)
+            instance = NodeGroupInterface(nodegroup=nodegroup)
+            form = NodeGroupInterfaceForm(data=interface, instance=instance)
+            form.save()
         if self.status is not None:
             nodegroup.status = self.status
             nodegroup.save()

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2013-09-16 05:13:13 +0000
+++ src/maasserver/tests/test_forms.py	2013-10-04 10:46:18 +0000
@@ -673,14 +673,6 @@
 
 class TestNodeGroupInterfaceForm(MAASServerTestCase):
 
-    def test_NodeGroupInterfaceForm_save_sets_nodegroup(self):
-        nodegroup = factory.make_node_group(
-            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
-        form = NodeGroupInterfaceForm(data=make_interface_settings())
-        self.assertTrue(form.is_valid(), form._errors)
-        interface = form.save(nodegroup=nodegroup)
-        self.assertEqual(nodegroup, interface.nodegroup)
-
     def test_NodeGroupInterfaceForm_validates_parameters(self):
         form = NodeGroupInterfaceForm(data={'ip': factory.getRandomString()})
         self.assertFalse(form.is_valid())
@@ -692,9 +684,10 @@
         settings['management'] = NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED
         for field_name in nullable_fields:
             del settings[field_name]
-        form = NodeGroupInterfaceForm(data=settings)
         nodegroup = factory.make_node_group()
-        interface = form.save(nodegroup=nodegroup)
+        form = NodeGroupInterfaceForm(
+            data=settings, instance=NodeGroupInterface(nodegroup=nodegroup))
+        interface = form.save()
         field_values = [
             getattr(interface, field_name) for field_name in nullable_fields]
         self.assertThat(field_values, AllMatch(Equals('')))

=== modified file 'src/maasserver/tests/test_views_settings_clusters.py'
--- src/maasserver/tests/test_views_settings_clusters.py	2013-02-08 17:16:07 +0000
+++ src/maasserver/tests/test_views_settings_clusters.py	2013-10-04 10:46:18 +0000
@@ -198,6 +198,21 @@
             reload_object(interface),
             MatchesStructure.byEquality(**data))
 
+    def test_rejects_interface_creation_if_cluster_already_managed(self):
+        nodegroup = factory.make_node_group(
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)
+        create_link = reverse(
+            'cluster-interface-create', args=[nodegroup.uuid])
+        # nodegroup already has a 'managed' interface, try adding another
+        # one, also 'managed'.
+        data = factory.get_interface_fields(
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)
+        response = self.client.post(create_link, data)
+        self.assertEqual(httplib.OK, response.status_code)
+        error_message = (
+            "Another managed interface already exists for this cluster.")
+        self.assertThat(response.content, Contains(error_message))
+
 
 # XXX: rvb 2012-10-08 bug=1063881: apache transforms '//' into '/' in
 # the urls it passes around and this happens when an interface has an empty

=== modified file 'src/maasserver/views/settings_clusters.py'
--- src/maasserver/views/settings_clusters.py	2012-10-15 09:32:06 +0000
+++ src/maasserver/views/settings_clusters.py	2013-10-04 10:46:18 +0000
@@ -126,12 +126,18 @@
     form_class = NodeGroupInterfaceForm
     context_object_name = 'interface'
 
+    def get_form_kwargs(self):
+        kwargs = super(ClusterInterfaceCreate, self).get_form_kwargs()
+        assert kwargs.get('instance', None) is None
+        kwargs['instance'] = NodeGroupInterface(nodegroup=self.get_nodegroup())
+        return kwargs
+
     def get_success_url(self):
         uuid = self.kwargs.get('uuid', None)
         return reverse('cluster-edit', args=[uuid])
 
     def form_valid(self, form):
-        self.object = form.save(nodegroup=self.get_nodegroup())
+        self.object = form.save()
         messages.info(self.request, "Interface created.")
         return super(ClusterInterfaceCreate, self).form_valid(form)