Merge lp:~julian-edwards/maas/mac-network-removal into lp:~maas-committers/maas/trunk

[Julian Edwards]

Make it possible to remove the last MAC from the networks edit form.

Because the same form is used for the API and the web view, and the API uses its own operations for manipulating the MAC addresses, the web view was unable to assume it could delete the mac_addresses on the Network.

I've fixed it by defaulting to deleting the mac_addresses if not set, unless a flag is passed, which the API handler now does.

[Jeroen T. Vermeulen (jtv)]

Ugh. Just one thing missing: could you document the parameter on the form's __init__, preferably in such generally clear terms that "this is a horrible kludge for a very specific situation" is more or less an afterthought?

review: Approve

[Julian Edwards (julian-edwards)]

You got it, thanks.

=== modified file 'src/maasserver/api/networks.py'
--- src/maasserver/api/networks.py	2014-09-10 16:20:31 +0000
+++ src/maasserver/api/networks.py	2014-09-17 04:06:58 +0000
@@ -73,7 +73,9 @@
             of users and administrators.
         """
         network = get_object_or_404(Network, name=name)
-        form = NetworkForm(instance=network, data=request.data)
+        form = NetworkForm(
+            instance=network, data=request.data,
+            delete_macs_if_not_present=False)
         if not form.is_valid():
             raise ValidationError(form.errors)
         return form.save()

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2014-09-15 14:28:28 +0000
+++ src/maasserver/forms.py	2014-09-17 04:06:58 +0000
@@ -1933,10 +1933,22 @@
         widget=forms.SelectMultiple(attrs={'size': 10}),
         )
 
-    def __init__(self, data=None, instance=None, **kwargs):
+    def __init__(self, data=None, instance=None,
+                 delete_macs_if_not_present=True, **kwargs):
+        """
+        :param data: The web request.data
+        :param instance: the Network instance
+        :param delete_macs_if_not_present: If there's no mac_addresses present
+            in the data, then assume that the caller wants to delete them.
+            Override with True if you don't want that to happen. Yes, this
+            is a horrible kludge so the same form works in the API and the
+            web view.
+        """
         super(NetworkForm, self).__init__(
             data=data, instance=instance, **kwargs)
+        self.macs_in_request = data.get("mac_addresses") if data else None
         self.set_up_initial_macaddresses(instance)
+        self.delete_macs_if_not_present = delete_macs_if_not_present
 
     def set_up_initial_macaddresses(self, instance):
         """Set the initial value for the field 'macaddresses'.
@@ -1954,7 +1966,13 @@
         """Persist the network into the database."""
         network = super(NetworkForm, self).save(*args, **kwargs)
         macaddresses = self.cleaned_data.get('mac_addresses')
-        if macaddresses is not None:
+        # Because the form is used in the web view AND the API we need a
+        # hack. The API uses separate ops to amend the mac_addresses
+        # list, however the web UI does not. To preserve the API
+        # behaviour, its handler passes delete_macs_if_not_present as False.
+        if self.delete_macs_if_not_present and self.macs_in_request is None:
+            network.macaddress_set.clear()
+        elif macaddresses is not None:
             network.macaddress_set.clear()
             network.macaddress_set.add(*macaddresses)
         return network

=== modified file 'src/maasserver/tests/test_forms_network.py'
--- src/maasserver/tests/test_forms_network.py	2014-09-10 16:20:31 +0000
+++ src/maasserver/tests/test_forms_network.py	2014-09-17 04:06:58 +0000
@@ -120,6 +120,32 @@
         network = reload_object(network)
         self.assertItemsEqual(new_macs, network.macaddress_set.all())
 
+    def test_deletes_macaddresses_by_default_if_not_specified(self):
+        network = factory.make_Network()
+        [factory.make_MACAddress(networks=[network]) for _ in range(3)]
+        form = NetworkForm(
+            data={
+                'name': "foo",
+            },
+            instance=network)
+        form.save()
+        network = reload_object(network)
+        self.assertItemsEqual([], network.macaddress_set.all())
+
+    def test_does_not_delete_unspecified_macaddresses_if_told_not_to(self):
+        network = factory.make_Network()
+        macs = [factory.make_MACAddress(networks=[network]) for _ in range(3)]
+        form = NetworkForm(
+            data={
+                'name': "foo",
+            },
+            instance=network,
+            delete_macs_if_not_present=False,
+            )
+        form.save()
+        network = reload_object(network)
+        self.assertItemsEqual(macs, network.macaddress_set.all())
+
     def test_reports_clashes(self):
         # The uniqueness test on the Network model raises a ValidationError
         # when it finds a clash, but Django is prone to crashing when the