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

Proposed by Julian Edwards
Status: Merged
Approved by: Julian Edwards
Approved revision: no longer in the source branch.
Merged at revision: 3005
Proposed branch: lp:~julian-edwards/maas/mac-network-removal
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 94 lines (+49/-3)
3 files modified
src/maasserver/api/networks.py (+3/-1)
src/maasserver/forms.py (+20/-2)
src/maasserver/tests/test_forms_network.py (+26/-0)
To merge this branch: bzr merge lp:~julian-edwards/maas/mac-network-removal
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) Approve
Review via email: mp+234907@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

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
Revision history for this message
Julian Edwards (julian-edwards) wrote :

You got it, thanks.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/api/networks.py'
2--- src/maasserver/api/networks.py 2014-09-10 16:20:31 +0000
3+++ src/maasserver/api/networks.py 2014-09-17 04:06:58 +0000
4@@ -73,7 +73,9 @@
5 of users and administrators.
6 """
7 network = get_object_or_404(Network, name=name)
8- form = NetworkForm(instance=network, data=request.data)
9+ form = NetworkForm(
10+ instance=network, data=request.data,
11+ delete_macs_if_not_present=False)
12 if not form.is_valid():
13 raise ValidationError(form.errors)
14 return form.save()
15
16=== modified file 'src/maasserver/forms.py'
17--- src/maasserver/forms.py 2014-09-15 14:28:28 +0000
18+++ src/maasserver/forms.py 2014-09-17 04:06:58 +0000
19@@ -1933,10 +1933,22 @@
20 widget=forms.SelectMultiple(attrs={'size': 10}),
21 )
22
23- def __init__(self, data=None, instance=None, **kwargs):
24+ def __init__(self, data=None, instance=None,
25+ delete_macs_if_not_present=True, **kwargs):
26+ """
27+ :param data: The web request.data
28+ :param instance: the Network instance
29+ :param delete_macs_if_not_present: If there's no mac_addresses present
30+ in the data, then assume that the caller wants to delete them.
31+ Override with True if you don't want that to happen. Yes, this
32+ is a horrible kludge so the same form works in the API and the
33+ web view.
34+ """
35 super(NetworkForm, self).__init__(
36 data=data, instance=instance, **kwargs)
37+ self.macs_in_request = data.get("mac_addresses") if data else None
38 self.set_up_initial_macaddresses(instance)
39+ self.delete_macs_if_not_present = delete_macs_if_not_present
40
41 def set_up_initial_macaddresses(self, instance):
42 """Set the initial value for the field 'macaddresses'.
43@@ -1954,7 +1966,13 @@
44 """Persist the network into the database."""
45 network = super(NetworkForm, self).save(*args, **kwargs)
46 macaddresses = self.cleaned_data.get('mac_addresses')
47- if macaddresses is not None:
48+ # Because the form is used in the web view AND the API we need a
49+ # hack. The API uses separate ops to amend the mac_addresses
50+ # list, however the web UI does not. To preserve the API
51+ # behaviour, its handler passes delete_macs_if_not_present as False.
52+ if self.delete_macs_if_not_present and self.macs_in_request is None:
53+ network.macaddress_set.clear()
54+ elif macaddresses is not None:
55 network.macaddress_set.clear()
56 network.macaddress_set.add(*macaddresses)
57 return network
58
59=== modified file 'src/maasserver/tests/test_forms_network.py'
60--- src/maasserver/tests/test_forms_network.py 2014-09-10 16:20:31 +0000
61+++ src/maasserver/tests/test_forms_network.py 2014-09-17 04:06:58 +0000
62@@ -120,6 +120,32 @@
63 network = reload_object(network)
64 self.assertItemsEqual(new_macs, network.macaddress_set.all())
65
66+ def test_deletes_macaddresses_by_default_if_not_specified(self):
67+ network = factory.make_Network()
68+ [factory.make_MACAddress(networks=[network]) for _ in range(3)]
69+ form = NetworkForm(
70+ data={
71+ 'name': "foo",
72+ },
73+ instance=network)
74+ form.save()
75+ network = reload_object(network)
76+ self.assertItemsEqual([], network.macaddress_set.all())
77+
78+ def test_does_not_delete_unspecified_macaddresses_if_told_not_to(self):
79+ network = factory.make_Network()
80+ macs = [factory.make_MACAddress(networks=[network]) for _ in range(3)]
81+ form = NetworkForm(
82+ data={
83+ 'name': "foo",
84+ },
85+ instance=network,
86+ delete_macs_if_not_present=False,
87+ )
88+ form.save()
89+ network = reload_object(network)
90+ self.assertItemsEqual(macs, network.macaddress_set.all())
91+
92 def test_reports_clashes(self):
93 # The uniqueness test on the Network model raises a ValidationError
94 # when it finds a clash, but Django is prone to crashing when the