launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #08670
[Merge] lp:~rvb/maas/forms-cleanup into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/forms-cleanup into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/forms-cleanup/+merge/109577
This branch cleans up code in src/maasserver/forms.py. The main goal here is to remove form code duplication and unify the forms used to create and edit nodes. This branch is a preparation for a follow-up branch which adds the possibility to set a Node's power_parameters when adding it via the API.
Please note that this branch adds lots of docstring and adds only one SLOC.
= Notes =
This branch:
- removes duplicated forms used when editing and adding a node (and renames the forms accordingly).
- removes the 'api' parameter in get_node_edit_form: it's a left over that's no longer needed.
- adds 'architecture' to the edit form (this field is on the 'add node' form and even if it might seem silly to change the architecture, one might want to correct a wrong architecture).
- reorders the fields on the "Add node" js form to match the order of the same fields on the edit form.
--
https://code.launchpad.net/~rvb/maas/forms-cleanup/+merge/109577
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/forms-cleanup into lp:maas.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-06-07 15:43:56 +0000
+++ src/maasserver/api.py 2012-06-11 08:48:32 +0000
@@ -110,8 +110,8 @@
)
from maasserver.fields import validate_mac
from maasserver.forms import (
+ get_node_create_form,
get_node_edit_form,
- NodeWithMACAddressesForm,
)
from maasserver.models import (
Config,
@@ -434,7 +434,7 @@
node = Node.objects.get_node_or_404(
system_id=system_id, user=request.user, perm=NODE_PERMISSION.EDIT)
data = get_overrided_query_dict(model_to_dict(node), request.data)
- Form = get_node_edit_form(request.user, api=True)
+ Form = get_node_edit_form(request.user)
form = Form(data, instance=node)
if form.is_valid():
return form.save()
@@ -520,7 +520,8 @@
:rtype: :class:`maasserver.models.Node`.
:raises: ValidationError
"""
- form = NodeWithMACAddressesForm(request.data)
+ Form = get_node_create_form(request.user)
+ form = Form(request.data)
if form.is_valid():
return form.save()
else:
=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py 2012-06-07 15:57:28 +0000
+++ src/maasserver/forms.py 2012-06-11 08:48:32 +0000
@@ -11,17 +11,20 @@
__metaclass__ = type
__all__ = [
+ "AdminNodeWithMACAddressesForm",
"CommissioningForm",
"get_action_form",
"get_node_edit_form",
+ "get_node_create_form",
"HostnameFormField",
"NodeForm",
"MACAddressForm",
"MAASAndNetworkForm",
+ "NodeWithMACAddressesForm",
"SSHKeyForm",
"UbuntuForm",
- "UIAdminNodeEditForm",
- "UINodeEditForm",
+ "AdminNodeForm",
+ "NodeForm",
]
from django import forms
@@ -88,10 +91,6 @@
class NodeForm(ModelForm):
- system_id = forms.CharField(
- widget=forms.TextInput(attrs={'readonly': 'readonly'}),
- required=False)
-
after_commissioning_action = forms.TypedChoiceField(
label="After commissioning",
choices=NODE_AFTER_COMMISSIONING_ACTION_CHOICES, required=False,
@@ -106,41 +105,8 @@
model = Node
fields = (
'hostname',
- 'system_id',
'after_commissioning_action',
'architecture',
- 'power_type',
- )
-
-
-class UINodeEditForm(ModelForm):
-
- after_commissioning_action = forms.ChoiceField(
- label="After commissioning",
- choices=NODE_AFTER_COMMISSIONING_ACTION_CHOICES,
- required=False)
-
- class Meta:
- model = Node
- fields = (
- 'hostname',
- 'after_commissioning_action',
- )
-
-
-class UIAdminNodeEditForm(ModelForm):
-
- after_commissioning_action = forms.ChoiceField(
- label="After commissioning",
- choices=NODE_AFTER_COMMISSIONING_ACTION_CHOICES,
- required=False)
-
- class Meta:
- model = Node
- fields = (
- 'hostname',
- 'after_commissioning_action',
- 'power_type',
)
@@ -169,32 +135,39 @@
super(APIEditMixin, self)._post_clean()
-class APIAdminNodeEditForm(APIEditMixin, UIAdminNodeEditForm):
+class AdminNodeForm(APIEditMixin, NodeForm):
+ """A version of NodeForm with adds the fields 'power_type' and
+ 'power_parameters'.
+ """
class Meta:
model = Node
fields = (
- 'hostname',
- 'after_commissioning_action',
- 'power_type',
- 'power_parameters',
- )
+ 'hostname',
+ 'after_commissioning_action',
+ 'architecture',
+ 'power_type',
+ 'power_parameters',
+ )
def __init__(self, data=None, files=None, instance=None, initial=None):
- super(APIAdminNodeEditForm, self).__init__(
+ super(AdminNodeForm, self).__init__(
data=data, files=files, instance=instance, initial=initial)
self.set_up_power_parameters_field(data, instance)
def set_up_power_parameters_field(self, data, node):
- if data is None:
- data = {}
- power_type = data.get('power_type', self.initial.get('power_type'))
- if power_type not in dict(POWER_TYPE_CHOICES):
- power_type = node.power_type
- self.fields['power_parameters'] = POWER_TYPE_PARAMETERS[power_type]
+ if node is not None:
+ if data is None:
+ data = {}
+ power_type = data.get(
+ 'power_type', self.initial.get('power_type'))
+ if power_type not in dict(POWER_TYPE_CHOICES):
+ power_type = node.power_type
+ self.fields['power_parameters'] = (
+ POWER_TYPE_PARAMETERS[power_type])
def clean(self):
- cleaned_data = super(APIAdminNodeEditForm, self).clean()
+ cleaned_data = super(AdminNodeForm, self).clean()
# If power_type is DEFAULT and power_parameters_skip_check is not
# on, reset power_parameters (set it to the empty string).
is_default = cleaned_data['power_type'] == POWER_TYPE.DEFAULT
@@ -204,15 +177,12 @@
cleaned_data['power_parameters'] = ''
return cleaned_data
- def clean_power_type_power_param(self, cleaned_data):
- return cleaned_data
-
-
-def get_node_edit_form(user, api=False):
+
+def get_node_edit_form(user):
if user.is_superuser:
- return APIAdminNodeEditForm
+ return AdminNodeForm
else:
- return UINodeEditForm
+ return NodeForm
class MACAddressForm(ModelForm):
@@ -281,17 +251,25 @@
return []
-class NodeWithMACAddressesForm(NodeForm):
+class WithMACAddressesMixin:
+ """A form mixin which dynamically adds a MultipleMACAddressField to the
+ list of fields. This mixin also overrides the 'save' method to persist
+ the list of MAC addresses and is intended to be used with a class
+ inheriting from NodeForm.
+ """
def __init__(self, *args, **kwargs):
- super(NodeWithMACAddressesForm, self).__init__(*args, **kwargs)
+ super(WithMACAddressesMixin, self).__init__(*args, **kwargs)
+ self.set_up_mac_addresses_field()
+
+ def set_up_mac_addresses_field(self):
macs = [mac for mac in self.data.getlist('mac_addresses') if mac]
self.fields['mac_addresses'] = MultipleMACAddressField(len(macs))
self.data = self.data.copy()
self.data['mac_addresses'] = macs
def is_valid(self):
- valid = super(NodeWithMACAddressesForm, self).is_valid()
+ valid = super(WithMACAddressesMixin, self).is_valid()
# If the number of MAC address fields is > 1, provide a unified
# error message if the validation has failed.
reformat_mac_address_error = (
@@ -312,7 +290,7 @@
return data
def save(self):
- node = super(NodeWithMACAddressesForm, self).save()
+ node = super(WithMACAddressesMixin, self).save()
for mac in self.cleaned_data['mac_addresses']:
node.add_mac_address(mac)
if self.cleaned_data['hostname'] == "":
@@ -320,6 +298,24 @@
return node
+class AdminNodeWithMACAddressesForm(WithMACAddressesMixin, AdminNodeForm):
+ """A version of the AdminNodeForm which includes the multi-MAC address
+ field.
+ """
+
+
+class NodeWithMACAddressesForm(WithMACAddressesMixin, NodeForm):
+ """A version of the NodeForm which includes the multi-MAC address field.
+ """
+
+
+def get_node_create_form(user):
+ if user.is_superuser:
+ return AdminNodeWithMACAddressesForm
+ else:
+ return NodeWithMACAddressesForm
+
+
class NodeActionForm(forms.Form):
"""Base form for performing a node action.
=== modified file 'src/maasserver/static/js/node_add.js'
--- src/maasserver/static/js/node_add.js 2012-04-27 08:11:29 +0000
+++ src/maasserver/static/js/node_add.js 2012-06-11 08:48:32 +0000
@@ -140,10 +140,10 @@
.set('method', 'post')
.append(global_error)
.append(operation)
+ .append(Y.Node.create(this.add_node))
+ .append(Y.Node.create(this.add_architecture))
.append(Y.Node.create(this.add_macaddress))
.append(macaddress_add_link)
- .append(Y.Node.create(this.add_architecture))
- .append(Y.Node.create(this.add_node))
.append(buttons);
return addnodeform;
},
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-06-08 11:40:51 +0000
+++ src/maasserver/tests/test_api.py 2012-06-11 08:48:32 +0000
@@ -251,19 +251,6 @@
system_id=json.loads(response.content)['system_id'])
self.assertEqual(POWER_TYPE.DEFAULT, node.power_type)
- def test_POST_new_sets_power_type(self):
- architecture = factory.getRandomChoice(ARCHITECTURE_CHOICES)
- response = self.client.post(
- self.get_uri('nodes/'), {
- 'op': 'new',
- 'architecture': architecture,
- 'power_type': POWER_TYPE.WAKE_ON_LAN,
- 'mac_addresses': ['00:11:22:33:44:55'],
- })
- node = Node.objects.get(
- system_id=json.loads(response.content)['system_id'])
- self.assertEqual(POWER_TYPE.WAKE_ON_LAN, node.power_type)
-
def test_POST_new_associates_mac_addresses(self):
# The API allows a Node to be created and associated with MAC
# Addresses.
@@ -517,6 +504,18 @@
class AdminLoggedInEnlistmentAPITest(APIv10TestMixin, AdminLoggedInTestCase):
# Enlistment tests specific to admin users.
+ def test_POST_new_sets_power_type_if_admin(self):
+ response = self.client.post(
+ self.get_uri('nodes/'), {
+ 'op': 'new',
+ 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
+ 'power_type': POWER_TYPE.WAKE_ON_LAN,
+ 'mac_addresses': ['00:11:22:33:44:55'],
+ })
+ node = Node.objects.get(
+ system_id=json.loads(response.content)['system_id'])
+ self.assertEqual(POWER_TYPE.WAKE_ON_LAN, node.power_type)
+
def test_POST_admin_creates_node_in_commissioning_state(self):
# When an admin user enlists a node, it goes into the
# Commissioning state.
@@ -1007,12 +1006,11 @@
{'power_parameters_unknown_param': factory.getRandomString()})
self.assertEqual(
- (httplib.BAD_REQUEST, json.loads(response.content)),
(
- response.status_code,
- {'power_parameters':
- ["Unknown parameter(s): unknown_param."]}
- ))
+ httplib.BAD_REQUEST,
+ {'power_parameters': ["Unknown parameter(s): unknown_param."]}
+ ),
+ (response.status_code, json.loads(response.content)))
self.assertEqual(
power_parameters, reload_object(node).power_parameters)
@@ -1052,11 +1050,11 @@
node = reload_object(node)
self.assertEqual(
- (httplib.BAD_REQUEST, json.loads(response.content)),
(
- response.status_code,
- {'power_parameters': ['Unknown parameter(s): address.']}
- ))
+ httplib.BAD_REQUEST,
+ {'power_parameters': ["Unknown parameter(s): address."]}
+ ),
+ (response.status_code, json.loads(response.content)))
self.assertEqual(
power_parameters, reload_object(node).power_parameters)
=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py 2012-06-07 08:22:26 +0000
+++ src/maasserver/tests/test_forms.py 2012-06-11 08:48:32 +0000
@@ -20,24 +20,26 @@
from django.http import QueryDict
from maasserver.enum import (
ARCHITECTURE,
+ ARCHITECTURE_CHOICES,
NODE_AFTER_COMMISSIONING_ACTION_CHOICES,
NODE_STATUS,
)
from maasserver.forms import (
- APIAdminNodeEditForm,
+ AdminNodeForm,
+ AdminNodeWithMACAddressesForm,
ConfigForm,
EditUserForm,
get_action_form,
+ get_node_create_form,
get_node_edit_form,
HostnameFormField,
MACAddressForm,
NewUserCreationForm,
NodeActionForm,
+ NodeForm,
NodeWithMACAddressesForm,
ProfileForm,
remove_None_values,
- UIAdminNodeEditForm,
- UINodeEditForm,
validate_hostname,
)
from maasserver.models import (
@@ -185,25 +187,27 @@
class NodeEditForms(TestCase):
- def test_UINodeEditForm_contains_limited_set_of_fields(self):
- form = UINodeEditForm()
+ def test_NodeForm_contains_limited_set_of_fields(self):
+ form = NodeForm()
self.assertEqual(
[
'hostname',
'after_commissioning_action',
+ 'architecture',
], list(form.fields))
- def test_UINodeEditForm_changes_node(self):
+ def test_NodeForm_changes_node(self):
node = factory.make_node()
hostname = factory.getRandomString()
after_commissioning_action = factory.getRandomChoice(
NODE_AFTER_COMMISSIONING_ACTION_CHOICES)
- form = UINodeEditForm(
+ form = NodeForm(
data={
'hostname': hostname,
'after_commissioning_action': after_commissioning_action,
+ 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
},
instance=node)
form.save()
@@ -212,28 +216,32 @@
self.assertEqual(
after_commissioning_action, node.after_commissioning_action)
- def test_UIAdminNodeEditForm_contains_limited_set_of_fields(self):
- form = UIAdminNodeEditForm()
+ def test_AdminNodeForm_contains_limited_set_of_fields(self):
+ node = factory.make_node()
+ form = AdminNodeForm(instance=node)
self.assertEqual(
[
'hostname',
'after_commissioning_action',
+ 'architecture',
'power_type',
+ 'power_parameters',
],
list(form.fields))
- def test_UIAdminNodeEditForm_changes_node(self):
+ def test_AdminNodeForm_changes_node(self):
node = factory.make_node()
hostname = factory.getRandomString()
after_commissioning_action = factory.getRandomChoice(
NODE_AFTER_COMMISSIONING_ACTION_CHOICES)
power_type = factory.getRandomChoice(POWER_TYPE_CHOICES)
- form = UIAdminNodeEditForm(
+ form = AdminNodeForm(
data={
'hostname': hostname,
'after_commissioning_action': after_commissioning_action,
'power_type': power_type,
+ 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
},
instance=node)
form.save()
@@ -255,29 +263,18 @@
def test_remove_None_values_leaves_empty_dict_untouched(self):
self.assertEqual({}, remove_None_values({}))
- def test_APIAdminNodeEditForm_contains_limited_set_of_fields(self):
- form = APIAdminNodeEditForm({}, instance=factory.make_node())
-
- self.assertEqual(
- [
- 'hostname',
- 'after_commissioning_action',
- 'power_type',
- 'power_parameters',
- ],
- list(form.fields))
-
- def test_APIAdminNodeEditForm_changes_node(self):
+ def test_AdminNodeForm_changes_node_with_skip_check(self):
node = factory.make_node()
hostname = factory.getRandomString()
after_commissioning_action = factory.getRandomChoice(
NODE_AFTER_COMMISSIONING_ACTION_CHOICES)
power_type = factory.getRandomChoice(POWER_TYPE_CHOICES)
power_parameters_field = factory.getRandomString()
- form = APIAdminNodeEditForm(
+ form = AdminNodeForm(
data={
'hostname': hostname,
'after_commissioning_action': after_commissioning_action,
+ 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
'power_type': power_type,
'power_parameters_field': power_parameters_field,
'power_parameters_skip_check': True,
@@ -291,17 +288,23 @@
(node.hostname, node.after_commissioning_action, node.power_type,
node.power_parameters))
- def test_get_node_edit_form_returns_UINodeEditForm_if_non_admin(self):
- user = factory.make_user()
- self.assertEqual(UINodeEditForm, get_node_edit_form(user))
-
- def test_get_node_edit_form_returns_APIAdminNodeEdit_if_admin_api(self):
- admin = factory.make_admin()
- self.assertEqual(APIAdminNodeEditForm, get_node_edit_form(admin, True))
-
- def test_get_node_edit_form_returns_UINodeEditForm_if_non_admin_api(self):
- user = factory.make_user()
- self.assertEqual(UINodeEditForm, get_node_edit_form(user, True))
+ def test_get_node_edit_form_returns_NodeForm_if_non_admin(self):
+ user = factory.make_user()
+ self.assertEqual(NodeForm, get_node_edit_form(user))
+
+ def test_get_node_edit_form_returns_APIAdminNodeEdit_if_admin(self):
+ admin = factory.make_admin()
+ self.assertEqual(AdminNodeForm, get_node_edit_form(admin))
+
+ def test_get_node_create_form_if_non_admin(self):
+ user = factory.make_user()
+ self.assertEqual(
+ NodeWithMACAddressesForm, get_node_create_form(user))
+
+ def test_get_node_create_form_if_admin(self):
+ admin = factory.make_admin()
+ self.assertEqual(
+ AdminNodeWithMACAddressesForm, get_node_create_form(admin))
class TestNodeActionForm(TestCase):
=== modified file 'src/maasserver/tests/test_views_nodes.py'
--- src/maasserver/tests/test_views_nodes.py 2012-05-22 12:27:18 +0000
+++ src/maasserver/tests/test_views_nodes.py 2012-06-11 08:48:32 +0000
@@ -20,6 +20,7 @@
from maasserver import messages
import maasserver.api
from maasserver.enum import (
+ ARCHITECTURE_CHOICES,
NODE_AFTER_COMMISSIONING_ACTION,
NODE_STATUS,
)
@@ -208,6 +209,7 @@
node_edit_link = reverse('node-edit', args=[node.system_id])
params = {
'hostname': factory.getRandomString(),
+ 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
'after_commissioning_action': factory.getRandomEnum(
NODE_AFTER_COMMISSIONING_ACTION),
}
@@ -424,6 +426,7 @@
'after_commissioning_action': factory.getRandomEnum(
NODE_AFTER_COMMISSIONING_ACTION),
'power_type': factory.getRandomChoice(POWER_TYPE_CHOICES),
+ 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
}
response = self.client.post(node_edit_link, params)
