launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #08424
[Merge] lp:~rvb/maas/multifield-api into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/multifield-api into lp:maas with lp:~rvb/maas/multifield-power_parameters as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/multifield-api/+merge/108195
This branch updates the forms used to edit a node via the API to dynamically (depending on the value of the node's effective power_type) add a DictCharField used to edit a node's power_parameters.
= Note =
There is a catch: Django model forms (i.e. forms generated from models) were clearly designed to handle post request from HTML forms rather than API calls. The problem I had to fix with the APIEditMixin trick is that the standard validation in Django always assumes that all the fields will be present in the data submitted to the form (which would be the case if the data would come from a post request of an HTML form created by the current values for all the fields of the edited object). In an API call, non provided values should simply be ignored (instead of changing the field's value to the default). That's what APIEditMixin does: it removes None values from the processed data before the object instance if modified.
--
https://code.launchpad.net/~rvb/maas/multifield-api/+merge/108195
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/multifield-api into lp:maas.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-05-29 10:46:26 +0000
+++ src/maasserver/api.py 2012-05-31 16:12:28 +0000
@@ -414,12 +414,25 @@
:param power_type: The new power type for this node (see
vocabulary `POWER_TYPE`). Only available to admin users.
:type power_type: basestring
+ :param power_parameters_{param1}: The new value for the 'param1'
+ power parameter. Note that this is dynamic as the available
+ parameters depend on the selected value of the Node's power_type.
+ For instance, if the power_type is 'ether_wake', the only valid
+ parameter is 'power_address' so one would want to pass:
+ power_type='ether_wake'&&power_parameters_power_address='myaddress'
+ Only available to admin users.
+ :type power_parameters_{param1}: basestring
+ :param power_parameters_skip_check: Whether or not the new power
+ parameters for this node should be checked against the expected
+ power parameters for the node's power type ('True' or 'False').
+ The default is 'False'.
+ :type power_parameters_skip_validation: basestring
"""
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)
+ Form = get_node_edit_form(request.user, api=True)
form = Form(data, instance=node)
if form.is_valid():
return form.save()
=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py 2012-05-28 11:58:22 +0000
+++ src/maasserver/forms.py 2012-05-31 16:12:28 +0000
@@ -58,6 +58,8 @@
SSHKey,
)
from maasserver.node_action import compile_node_actions
+from maasserver.power_parameters import POWER_TYPE_PARAMETERS
+from provisioningserver.enum import POWER_TYPE_CHOICES
def compose_invalid_choice_text(choice_of_what, valid_choices):
@@ -138,9 +140,53 @@
)
-def get_node_edit_form(user):
+def remove_None_values(data):
+ """Returns a new dictionary without the keys corresponding to None values
+ """
+ return {key: value for key, value in data.items() if value is not None}
+
+
+class APIEditMixin:
+ """A mixin that clears None values after the cleaning phase
+ This is useful when one wants to avoid the edited object with None values
+ created by a form when the submitted data has some fields missing.
+ """
+
+ def _post_clean(self):
+ """Override Django's private hook _post_save to remove None values
+ from 'self.cleaned_data'.
+ The cleanup needs to happen before Django's _post_clean method because
+ that's where the fields of the instance get set with the data from
+ self.cleaned_data."""
+ self.cleaned_data = remove_None_values(self.cleaned_data)
+ super(APIEditMixin, self)._post_clean()
+
+
+class APIAdminNodeEditForm(APIEditMixin, UIAdminNodeEditForm):
+
+ class Meta:
+ model = Node
+ fields = (
+ 'hostname',
+ 'after_commissioning_action',
+ 'power_type',
+ 'power_parameters',
+ )
+
+ def __init__(self, data, instance):
+ super(APIAdminNodeEditForm, self).__init__(data, instance=instance)
+ self.setup_power_parameters_field(data, instance)
+
+ def setup_power_parameters_field(self, data, node):
+ power_type = data.get('power_type', None)
+ if power_type is None or power_type not in dict(POWER_TYPE_CHOICES):
+ power_type = node.get_effective_power_type()
+ self.fields['power_parameters'] = POWER_TYPE_PARAMETERS[power_type]
+
+
+def get_node_edit_form(user, api=False):
if user.is_superuser:
- return UIAdminNodeEditForm
+ return APIAdminNodeEditForm if api else UIAdminNodeEditForm
else:
return UINodeEditForm
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-05-29 10:46:26 +0000
+++ src/maasserver/tests/test_api.py 2012-05-31 16:12:28 +0000
@@ -978,6 +978,57 @@
self.assertEqual(httplib.NOT_FOUND, response.status_code)
+ def test_PUT_updates_power_parameters_field(self):
+ # The api allows the updating of a Node's power_parameters field.
+ self.become_admin()
+ node = factory.make_node(
+ owner=self.logged_in_user,
+ power_type=POWER_TYPE.WAKE_ON_LAN)
+ # Create a power_parameter valid for the selected power_type.
+ new_power_address = factory.getRandomString()
+ response = self.client.put(
+ self.get_node_uri(node),
+ {'power_parameters_power_address': new_power_address})
+
+ self.assertEqual(httplib.OK, response.status_code)
+ self.assertEqual(
+ {'power_address': new_power_address},
+ reload_object(node).power_parameters)
+
+ def test_PUT_updates_power_parameters_ignores_unknown_param(self):
+ self.become_admin()
+ power_parameters = factory.getRandomString()
+ node = factory.make_node(
+ owner=self.logged_in_user,
+ power_type=POWER_TYPE.WAKE_ON_LAN,
+ power_parameters=power_parameters)
+
+ response = self.client.put(
+ self.get_node_uri(node),
+ {'power_parameters_unknown_param': factory.getRandomString()})
+
+ self.assertEqual(httplib.OK, response.status_code)
+ self.assertEqual(
+ power_parameters, reload_object(node).power_parameters)
+
+ def test_PUT_updates_power_parameters_field_arbitrary(self):
+ # With power_parameters_skip_check, arbitrary data
+ # can be put in a Node's power_parameter field.
+ self.become_admin()
+ node = factory.make_node(owner=self.logged_in_user)
+ new_param = factory.getRandomString()
+ new_value = factory.getRandomString()
+ response = self.client.put(
+ self.get_node_uri(node),
+ {
+ 'power_parameters_%s' % new_param: new_value,
+ 'power_parameters_skip_check': True,
+ })
+
+ self.assertEqual(httplib.OK, response.status_code)
+ self.assertEqual(
+ {new_param: new_value}, reload_object(node).power_parameters)
+
def test_DELETE_deletes_node(self):
# The api allows to delete a Node.
self.become_admin()
=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py 2012-05-28 11:58:22 +0000
+++ src/maasserver/tests/test_forms.py 2012-05-31 16:12:28 +0000
@@ -24,6 +24,7 @@
NODE_STATUS,
)
from maasserver.forms import (
+ APIAdminNodeEditForm,
ConfigForm,
EditUserForm,
get_action_form,
@@ -34,6 +35,7 @@
NodeActionForm,
NodeWithMACAddressesForm,
ProfileForm,
+ remove_None_values,
UIAdminNodeEditForm,
UINodeEditForm,
validate_hostname,
@@ -49,7 +51,10 @@
)
from maasserver.testing.factory import factory
from maasserver.testing.testcase import TestCase
-from provisioningserver.enum import POWER_TYPE_CHOICES
+from provisioningserver.enum import (
+ POWER_TYPE,
+ POWER_TYPE_CHOICES,
+ )
from testtools.testcase import ExpectedException
@@ -241,6 +246,81 @@
after_commissioning_action, node.after_commissioning_action)
self.assertEqual(power_type, node.power_type)
+ def test_remove_None_values(self):
+ random_input = factory.getRandomString()
+ inputs = [
+ {},
+ {random_input: random_input, factory.getRandomString(): None},
+ {random_input:None}
+ ]
+ expected = [{}, {random_input: random_input}, {}]
+ self.assertEqual(expected, map(remove_None_values, inputs))
+
+ 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):
+ 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(
+ data={
+ 'hostname': hostname,
+ 'after_commissioning_action': after_commissioning_action,
+ 'power_type': power_type,
+ 'power_parameters_field': power_parameters_field,
+ 'power_parameters_skip_check': True,
+ },
+ instance=node)
+ form.save()
+
+ self.assertEqual(
+ (hostname, after_commissioning_action, power_type,
+ {'field': power_parameters_field}),
+ (node.hostname, node.after_commissioning_action, node.power_type,
+ node.power_parameters))
+
+ def test_APIAdminNodeEditForm_uses_effective_power_type_to_validate(self):
+ # The effective power_type (i.e. the power type defined on the node
+ # itself or the global default) to validate the power_parameters.
+ node = factory.make_node(power_type=POWER_TYPE.DEFAULT)
+ Config.objects.set_config('node_power_type', POWER_TYPE.VIRSH)
+ address = factory.getRandomString()
+ after_commissioning_action = factory.getRandomChoice(
+ NODE_AFTER_COMMISSIONING_ACTION_CHOICES)
+ form = APIAdminNodeEditForm(
+ data={
+ 'after_commissioning_action': after_commissioning_action,
+ 'power_parameters_power_address': address,
+ },
+ instance=node)
+
+ # The effective power_type is POWER_TYPE.VIRSH, the power_parameters
+ # data was validated using the DictCharField corresponding to this
+ # power_parameters (as defined by POWER_TYPE_PARAMETERS).
+ self.assertFalse(form.is_valid())
+ self.assertEqual(
+ {'power_parameters':
+ [
+ 'Driver: This field is required.',
+ 'Username: This field is required.',
+ 'Power ID: This field is required.'
+ ]
+ },
+ form.errors)
+
def test_get_node_edit_form_returns_UIAdminNodeEditForm_if_admin(self):
admin = factory.make_admin()
self.assertEqual(UIAdminNodeEditForm, get_node_edit_form(admin))
@@ -249,6 +329,14 @@
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))
+
class TestNodeActionForm(TestCase):
