launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/power-param-api3 into lp:maas with lp:~rvb/maas/power-param-api2 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/power-param-api3/+merge/107627

Use node edit forms in API's node update method.
-- 
https://code.launchpad.net/~rvb/maas/power-param-api3/+merge/107627
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/power-param-api3 into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-05-24 15:56:29 +0000
+++ src/maasserver/api.py	2012-05-28 16:30:30 +0000
@@ -83,9 +83,11 @@
     PermissionDenied,
     ValidationError,
     )
+from django.forms.models import model_to_dict
 from django.http import (
     HttpResponse,
     HttpResponseBadRequest,
+    QueryDict,
     )
 from django.shortcuts import (
     get_object_or_404,
@@ -107,7 +109,10 @@
     Unauthorized,
     )
 from maasserver.fields import validate_mac
-from maasserver.forms import NodeWithMACAddressesForm
+from maasserver.forms import (
+    get_node_edit_form,
+    NodeWithMACAddressesForm,
+    )
 from maasserver.models import (
     Config,
     FileStorage,
@@ -362,13 +367,6 @@
     )
 
 
-EDITABLE_NODE_FIELDS = (
-    'hostname',
-    'architecture',
-    'power_type',
-     )
-
-
 @api_operations
 class NodeHandler(BaseHandler):
     """Manage individual Nodes."""
@@ -390,21 +388,28 @@
             vocabulary `ARCHITECTURE`).
         :type architecture: basestring
         :param power_type: The new power type for this node (see
-            vocabulary `POWER_TYPE`).
+            vocabulary `POWER_TYPE`).  Only available to admin users.
         :type power_type: basestring
         """
 
         node = Node.objects.get_node_or_404(
             system_id=system_id, user=request.user, perm=NODE_PERMISSION.EDIT)
-        unknown_fields = set(request.data).difference(EDITABLE_NODE_FIELDS)
-        if len(unknown_fields) != 0:
-            raise PermissionDenied(
-                "Unable to set field(s): %s. Allowed fields are: %s." % (
-                    ','.join(unknown_fields), ','.join(EDITABLE_NODE_FIELDS)))
-        for key, value in request.data.items():
-            setattr(node, key, value)
-        node.save()
-        return node
+        # Create a writable query dict.
+        data = QueryDict('').copy()
+        # Missing fields will be taken from the node's current values.  This
+        # is to circumvent Django's ModelForm (form created from a model)
+        # default behaviour that requires all the fields to be defined.
+        data.update(model_to_dict(node))
+        # We can't use update here because data is a QueryDict and updates
+        # does not replaces the old values with the new as one would expect.
+        for k, v in request.data.items():
+            data[k] = v
+        Form = get_node_edit_form(request.user)
+        form = Form(data, instance=node)
+        if form.is_valid():
+            return form.save()
+        else:
+            raise ValidationError(form.errors)
 
     def delete(self, request, system_id):
         """Delete a specific Node."""

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-04-27 10:56:37 +0000
+++ src/maasserver/forms.py	2012-05-28 16:30:30 +0000
@@ -13,6 +13,7 @@
 __all__ = [
     "CommissioningForm",
     "get_action_form",
+    "get_node_edit_form",
     "HostnameFormField",
     "NodeForm",
     "MACAddressForm",
@@ -110,7 +111,8 @@
 
     after_commissioning_action = forms.ChoiceField(
         label="After commissioning",
-        choices=NODE_AFTER_COMMISSIONING_ACTION_CHOICES)
+        choices=NODE_AFTER_COMMISSIONING_ACTION_CHOICES,
+        required=False)
 
     class Meta:
         model = Node
@@ -124,7 +126,8 @@
 
     after_commissioning_action = forms.ChoiceField(
         label="After commissioning",
-        choices=NODE_AFTER_COMMISSIONING_ACTION_CHOICES)
+        choices=NODE_AFTER_COMMISSIONING_ACTION_CHOICES,
+        required=False)
 
     class Meta:
         model = Node
@@ -135,6 +138,13 @@
             )
 
 
+def get_node_edit_form(user):
+    if user.is_superuser:
+        return UIAdminNodeEditForm
+    else:
+        return UINodeEditForm
+
+
 class MACAddressForm(ModelForm):
     class Meta:
         model = MACAddress

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-05-24 16:10:47 +0000
+++ src/maasserver/tests/test_api.py	2012-05-28 16:30:30 +0000
@@ -35,7 +35,6 @@
 from fixtures import Fixture
 from maasserver import api
 from maasserver.api import (
-    EDITABLE_NODE_FIELDS,
     extract_constraints,
     extract_oauth_key,
     extract_oauth_key_from_auth_header,
@@ -73,7 +72,10 @@
     NodeUserData,
     )
 from metadataserver.nodeinituser import get_node_init_user
-from provisioningserver.enum import POWER_TYPE
+from provisioningserver.enum import (
+    POWER_TYPE,
+    POWER_TYPE_CHOICES,
+    )
 
 
 class APIv10TestMixin:
@@ -866,20 +868,55 @@
         self.assertEqual(0, Node.objects.filter(hostname='diane').count())
         self.assertEqual(1, Node.objects.filter(hostname='francis').count())
 
-    def test_PUT_rejects_unknown_fields(self):
-        node = factory.make_node(owner=self.logged_in_user)
+    def test_PUT_ignores_unknown_fields(self):
+        node = factory.make_node(
+            owner=self.logged_in_user,
+            after_commissioning_action=(
+                NODE_AFTER_COMMISSIONING_ACTION.DEFAULT))
         field = factory.getRandomString()
         response = self.client.put(
             self.get_node_uri(node),
-            {field: factory.getRandomString}
-            )
-
-        error_msg = (
-            "Unable to set field(s): %s. Allowed fields are: %s." % (
-                (field, ','.join(EDITABLE_NODE_FIELDS))))
-        self.assertEqual(
-            (httplib.FORBIDDEN, error_msg),
-            (response.status_code, response.content))
+            {field: factory.getRandomString()}
+            )
+
+        self.assertEqual(httplib.OK, response.status_code)
+
+    def test_PUT_admin_can_change_power_type(self):
+        self.become_admin()
+        original_power_type = factory.getRandomChoice(
+            POWER_TYPE_CHOICES)
+        new_power_type = factory.getRandomChoice(
+            POWER_TYPE_CHOICES, but_not=original_power_type)
+        node = factory.make_node(
+            owner=self.logged_in_user,
+            power_type=original_power_type,
+            after_commissioning_action=(
+                NODE_AFTER_COMMISSIONING_ACTION.DEFAULT))
+        self.client.put(
+            self.get_node_uri(node),
+            {'power_type': new_power_type}
+            )
+
+        self.assertEqual(
+            new_power_type, reload_object(node).power_type)
+
+    def test_PUT_non_admin_cannot_change_power_type(self):
+        original_power_type = factory.getRandomChoice(
+            POWER_TYPE_CHOICES)
+        new_power_type = factory.getRandomChoice(
+            POWER_TYPE_CHOICES, but_not=original_power_type)
+        node = factory.make_node(
+            owner=self.logged_in_user,
+            power_type=original_power_type,
+            after_commissioning_action=(
+                NODE_AFTER_COMMISSIONING_ACTION.DEFAULT))
+        self.client.put(
+            self.get_node_uri(node),
+            {'power_type': new_power_type}
+            )
+
+        self.assertEqual(
+            original_power_type, reload_object(node).power_type)
 
     def test_resource_uri_points_back_at_node(self):
         # When a Node is returned by the API, the field 'resource_uri'

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2012-05-05 09:47:03 +0000
+++ src/maasserver/tests/test_forms.py	2012-05-28 16:30:30 +0000
@@ -27,6 +27,7 @@
     ConfigForm,
     EditUserForm,
     get_action_form,
+    get_node_edit_form,
     HostnameFormField,
     MACAddressForm,
     NewUserCreationForm,
@@ -240,6 +241,14 @@
             after_commissioning_action, node.after_commissioning_action)
         self.assertEqual(power_type, node.power_type)
 
+    def test_get_node_edit_form_returns_UIAdminNodeEditForm_if_admin(self):
+        admin = factory.make_admin()
+        self.assertEqual(UIAdminNodeEditForm, get_node_edit_form(admin))
+
+    def test_get_node_edit_form_returns_UINodeEditForm_if_non_admin(self):
+        user = factory.make_user()
+        self.assertEqual(UINodeEditForm, get_node_edit_form(user))
+
 
 class TestNodeActionForm(TestCase):
 

=== modified file 'src/maasserver/views/nodes.py'
--- src/maasserver/views/nodes.py	2012-04-27 08:11:29 +0000
+++ src/maasserver/views/nodes.py	2012-05-28 16:30:30 +0000
@@ -41,9 +41,8 @@
     )
 from maasserver.forms import (
     get_action_form,
+    get_node_edit_form,
     MACAddressForm,
-    UIAdminNodeEditForm,
-    UINodeEditForm,
     )
 from maasserver.messages import messaging
 from maasserver.models import (
@@ -151,10 +150,7 @@
         return node
 
     def get_form_class(self):
-        if self.request.user.is_superuser:
-            return UIAdminNodeEditForm
-        else:
-            return UINodeEditForm
+        return get_node_edit_form(self.request.user)
 
     def get_success_url(self):
         return reverse('node-view', args=[self.get_object().system_id])