Merge lp:~jtv/maas/bug-1379816 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Make the “disable IPv4 by default” checkbox visible in the cluster UI.

This includes the UI trick we needed to get the “disable IPv4” checkbox working in the node UI, which Raphaël generalised into a reusable pattern. Without it, once checked, the box can never be unchecked.

That should explain why the tests may seem to have an unhealthy obsession with changing the setting from True to False, instead of just trying it either way. I hope the comments make the situation clear throughout.

In a next step we could hide this button if the cluster does not have any IPv6 configured, like we do in the node UI.

Jeroen

[Christian Reis (kiko)]

Let's hide the button if there's no IPv6 configured -- and in fact, what happens if it is selected and there is no IPv6 on the cluster?

Pending that change and review, approved for 1.7.

review: Needs Fixing

[Jeroen T. Vermeulen (jtv)]

Branch to hide the field is at https://code.launchpad.net/~jtv/maas/hide-cluster-default-disable-ipv4-if-no-ipv6/+merge/238254

[Raphaël Badin (rvb)]

Looks good to me, with the additional lp:~jtv/maas/hide-cluster-default-disable-ipv4-if-no-ipv6/ branch.

review: Approve

=== modified file 'src/maasserver/api/tests/test_nodegroup.py'
--- src/maasserver/api/tests/test_nodegroup.py	2014-10-03 14:49:13 +0000
+++ src/maasserver/api/tests/test_nodegroup.py	2014-10-10 15:15:47 +0000
@@ -128,7 +128,9 @@
 
     def test_PUT_updates_nodegroup(self):
         # The api allows the updating of a NodeGroup.
-        nodegroup = factory.make_NodeGroup()
+        old_disable_ipv4 = factory.pick_bool()
+        nodegroup = factory.make_NodeGroup(
+            default_disable_ipv4=old_disable_ipv4)
         self.become_admin()
         new_name = factory.make_name("new-name")
         new_cluster_name = factory.make_name("new-cluster-name")
@@ -140,6 +142,7 @@
                 'name': new_name,
                 'cluster_name': new_cluster_name,
                 'status': new_status,
+                'default_disable_ipv4': not old_disable_ipv4,
             })
 
         self.assertEqual(httplib.OK, response.status_code, response.content)
@@ -147,6 +150,7 @@
         self.assertEqual(
             (new_name, new_cluster_name, new_status),
             (nodegroup.name, nodegroup.cluster_name, nodegroup.status))
+        self.assertEqual(not old_disable_ipv4, nodegroup.default_disable_ipv4)
 
     def test_PUT_updates_nodegroup_validates_data(self):
         nodegroup, _ = factory.make_unrenamable_NodeGroup_with_Node()
@@ -163,6 +167,18 @@
             "Can't rename DNS zone",
             parsed_result['name'][0])
 
+    def test_PUT_without_default_disable_ipv4_leaves_field_unchanged(self):
+        old_value = factory.pick_bool()
+        nodegroup = factory.make_NodeGroup(default_disable_ipv4=old_value)
+        self.become_admin()
+        response = self.client_put(
+            reverse('nodegroup_handler', args=[nodegroup.uuid]),
+            {'name': nodegroup.name})
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        self.assertEqual(
+            old_value,
+            reload_object(nodegroup).default_disable_ipv4)
+
     def test_accept_accepts_nodegroup(self):
         nodegroups = [factory.make_NodeGroup() for _ in range(3)]
         uuids = [nodegroup.uuid for nodegroup in nodegroups]

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2014-10-10 04:55:03 +0000
+++ src/maasserver/forms.py	2014-10-10 15:15:47 +0000
@@ -1710,6 +1710,7 @@
             'cluster_name',
             'status',
             'name',
+            'default_disable_ipv4',
             )
 
     def clean_name(self):
@@ -1732,6 +1733,16 @@
 
         return new_name
 
+    def clean_default_disable_ipv4(self):
+        data = self.submitted_data
+        if 'ui_submission' in data and 'default_disable_ipv4' not in data:
+            # In the UI, where all fields are submitted except checkboxes in
+            # the "off" state, a missing boolean field means False.
+            # (In the API, as enforced by MAASModelForm, a missing boolean
+            # field means "unchanged").
+            self.cleaned_data['default_disable_ipv4'] = False
+        return self.cleaned_data['default_disable_ipv4']
+
 
 class TagForm(MAASModelForm):
 

=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py	2014-09-30 18:13:21 +0000
+++ src/maasserver/models/nodegroup.py	2014-10-10 15:15:47 +0000
@@ -183,8 +183,8 @@
         default=False,
         verbose_name="Disable IPv4 by default when deploying nodes",
         help_text=(
-            "Default setting for setting on new nodes: disable IPv4 when "
-            "deploying, on operating systems where this is supported."))
+            "Default setting for new nodes: disable IPv4 when deploying, on "
+            "operating systems where this is supported."))
 
     @property
     def api_credentials(self):

=== modified file 'src/maasserver/views/clusters.py'
--- src/maasserver/views/clusters.py	2014-10-07 21:18:44 +0000
+++ src/maasserver/views/clusters.py	2014-10-10 15:15:47 +0000
@@ -143,6 +143,15 @@
     form_class = NodeGroupEdit
     context_object_name = 'cluster'
 
+    def get_form_kwargs(self):
+        kwargs = super(ClusterEdit, self).get_form_kwargs()
+        # The cluster form has a boolean checkbox.  For those we need to know
+        # whether a submission came in from the UI (where omitting the field
+        # means "set to False") or from the API (where it means "leave
+        # unchanged").
+        kwargs['ui_submission'] = True
+        return kwargs
+
     def get_context_data(self, **kwargs):
         context = super(ClusterEdit, self).get_context_data(**kwargs)
         context['interfaces'] = (

=== modified file 'src/maasserver/views/tests/test_clusters.py'
--- src/maasserver/views/tests/test_clusters.py	2014-10-08 08:34:46 +0000
+++ src/maasserver/views/tests/test_clusters.py	2014-10-10 15:15:47 +0000
@@ -397,6 +397,18 @@
         self.assertIn(
             reverse('cluster-interface-create', args=[nodegroup.uuid]), links)
 
+    def test_admin_can_disable_default_disable_ipv4_flag(self):
+        self.client_log_in(as_admin=True)
+        nodegroup = factory.make_NodeGroup(default_disable_ipv4=True)
+        edit_link = reverse('cluster-edit', args=[nodegroup.uuid])
+        # In a UI submission, omitting a boolean means setting it to False.
+        data = {
+            'ui_submission': True,
+            }
+        response = self.client.post(edit_link, data)
+        self.assertEqual(httplib.FOUND, response.status_code)
+        self.assertFalse(reload_object(nodegroup).default_disable_ipv4)
+
 
 class ClusterInterfaceDeleteTest(MAASServerTestCase):