launchpad-reviewers team mailing list archive

[Dimiter Naydenov <dimiter.naydenov@xxxxxxxxxxxxx>]

Dimiter Naydenov has proposed merging lp:~dimitern/maas/no-api-to-mass-release-nodes into lp:maas.

Commit message:
Fixed #1061870: Implemented NodesHandler.release() API call to release multiple nodes at once.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~dimitern/maas/no-api-to-mass-release-nodes/+merge/130372

Implemented NodesHandler.release() API to enable mass releasing of nodes in a single call (fixes bug #1061870).
Not sure what should be the complete description in the docstring though :(
-- 
https://code.launchpad.net/~dimitern/maas/no-api-to-mass-release-nodes/+merge/130372
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~dimitern/maas/no-api-to-mass-release-nodes into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-14 22:58:09 +0000
+++ src/maasserver/api.py	2012-10-18 14:33:21 +0000
@@ -803,6 +803,52 @@
         nodes = [node.accept_enlistment(request.user) for node in nodes]
         return filter(None, nodes)
 
+    @operation(idempotent=False)
+    def release(self, request):
+        """Release multiple nodes.
+
+        TBD: not sure about the proper description..
+
+        :param nodes: system_ids of the nodes which are to be released.
+           (An empty list is acceptable).
+        :return: The system_ids of any nodes that have their status
+            changed by this call. Thus, nodes that were already released
+            are excluded from the result.
+        """
+        system_ids = set(request.POST.getlist('nodes'))
+         # Check the existence of these nodes first.
+        self._check_system_ids_exist(system_ids)
+        # Make sure that the user has the required permission.
+        nodes = Node.objects.get_nodes(
+            request.user, perm=NODE_PERMISSION.EDIT, ids=system_ids)
+        if len(nodes) < len(system_ids):
+            permitted_ids = set(node.system_id for node in nodes)
+            raise PermissionDenied(
+                "You don't have the required permission to release the "
+                "following node(s): %s." % (
+                    ', '.join(system_ids - permitted_ids)))
+
+        released = []
+        failed = []
+        for node in nodes:
+            if node.status == NODE_STATUS.READY:
+                # Nothing to do.
+                pass
+            elif node.status in [NODE_STATUS.ALLOCATED, NODE_STATUS.RESERVED]:
+                node.release()
+                released.append(node)
+            else:
+                failed.append(
+                    "%s ('%s')"
+                    % (node.system_id, node.display_status()))
+
+        if any(failed):
+            raise NodeStateViolation(
+                "Node(s) cannot be released in their current state: %s."
+                % ', '.join(failed))
+        
+        return released
+        
     @operation(idempotent=True)
     def list(self, request):
         """List Nodes visible to the user, optionally filtered by criteria.

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-10-15 07:52:46 +0000
+++ src/maasserver/tests/test_api.py	2012-10-18 14:33:21 +0000
@@ -2126,6 +2126,109 @@
             [node.system_id for node in acceptable_nodes], accepted_ids)
         self.assertNotIn(accepted_node.system_id, accepted_ids)
 
+    def test_POST_quietly_releases_empty_set(self):
+        response = self.client.post(self.get_uri('nodes/'), {'op': 'release'})
+        self.assertEqual(
+            (httplib.OK, "[]"), (response.status_code, response.content))
+
+    def test_POST_release_rejects_request_from_unauthorized_user(self):
+        node = factory.make_node(
+            status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+        response = self.client.post(
+            self.get_uri('nodes/'), {
+                'op': 'release',
+                'nodes': [node.system_id],
+                })
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
+        self.assertEqual(NODE_STATUS.ALLOCATED, reload_object(node).status)
+
+    def test_POST_release_fails_if_nodes_do_not_exits(self):
+         # Make sure there is a node, it just isn't among the ones to release
+        factory.make_node()
+        node_ids = set(factory.getRandomString() for i in xrange(5))
+        response = self.client.post(
+            self.get_uri('nodes/'), {
+                'op': 'release',
+                'nodes': node_ids
+                })
+        node_ids = ', '.join(node_ids)
+        self.assertEqual(
+            (httplib.BAD_REQUEST, "Unknown node(s): %s." % node_ids),
+            (response.status_code, response.content))
+
+    def test_POST_release_forbidden_if_user_cannot_edit_node(self):
+        # Create a bunch of nodes, owned by the logged in user
+        node_ids = set(
+            factory.make_node(
+                status=NODE_STATUS.ALLOCATED,
+                owner=self.logged_in_user).system_id
+            for i in xrange(3)
+            )
+        # And one with no owner
+        another_node = factory.make_node(status=NODE_STATUS.RESERVED)
+        node_ids.add(another_node.system_id)
+        
+        response = self.client.post(
+            self.get_uri('nodes/'), {
+                'op': 'release',
+                'nodes': node_ids
+                })
+        self.assertEqual(
+            (httplib.FORBIDDEN,
+                "You don't have the required permission to release the "
+                "following node(s): %s." % another_node.system_id),
+            (response.status_code, response.content))
+        
+    def test_POST_release_rejects_impossible_state_changes(self):
+        acceptable_states = set([
+            NODE_STATUS.ALLOCATED,
+            NODE_STATUS.RESERVED,
+            NODE_STATUS.READY,
+            ])
+        unacceptable_states = (
+            set(map_enum(NODE_STATUS).values()) - acceptable_states)
+        owner = self.logged_in_user
+        nodes = [
+            factory.make_node(status=status, owner=owner)
+            for status in unacceptable_states]
+        response = self.client.post(
+            self.get_uri('nodes/'), {
+                'op': 'release',
+                'nodes': [node.system_id for node in nodes],
+                })
+        expected = ', '.join(
+            "%s ('%s')" % (node.system_id, node.display_status())
+            for node in reversed(nodes)   # the result is in reverse order
+            if node.status not in acceptable_states)
+        self.assertEqual(
+            (httplib.CONFLICT,
+             "Node(s) cannot be released in their current state: %s."
+             % expected),
+            (response.status_code, response.content))
+
+    def test_POST_release_returns_modified_nodes(self):
+        owner = self.logged_in_user
+        acceptable_states = set([
+            NODE_STATUS.READY,
+            NODE_STATUS.ALLOCATED,
+            NODE_STATUS.RESERVED,
+            ])
+        nodes = [
+            factory.make_node(status=status, owner=owner)
+            for status in acceptable_states
+            ]
+        response = self.client.post(
+            self.get_uri('nodes/'), {
+                'op': 'release',
+                'nodes': [node.system_id for node in nodes],
+                })
+        parsed_result = json.loads(response.content)
+        self.assertEqual(httplib.OK, response.status_code)
+        # The first node is READY, so shouldn't be touched
+        self.assertItemsEqual(
+            [nodes[1].system_id, nodes[2].system_id],
+            extract_system_ids(parsed_result))
+        
 
 class MACAddressAPITest(APITestCase):