launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07340
[Merge] lp:~jtv/maas/doris-would-you-like-nicer-node-actions into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/doris-would-you-like-nicer-node-actions into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/doris-would-you-like-nicer-node-actions/+merge/103742
As discussed with Julian: node actions have gotten perhaps a little out of hand. This branch offers a new, more strongly-typed implementation. It has a (very short) list of node-action classes, each of which defines such things as what permission you need on a node to perform it, and what state the node needs to be in. That single short list also defines the order in which the actions will appear on a page.
With this design our tests no longer need to specify the actions' names as literals such as “Start node.” Instead they can use symbols such as StartNode.display (“the displayed name for the start-node action”). In another change, the confirmation message is now built into the action's code, so that it can be customized on the fly if desired — and so that we have one less field to fill out when we define a new action.
Oh, and the new design is much easier to unit-test. The tests run it through its detailed paces at quite a low level, and the form and view tests can be reduced to one example case per behaviour.
The view will receive a container of action objects instantiated from these action classes, filtered down to only those that apply to the given node and user. But that's all in the next branch. The combined change is huge and I could think of no better way to split it up than to add the new module in one, and make it replace the old one in another branch.
Put together, my two branches delete about as many lines as they add (less than 10% difference). I think that's not bad, given that I introduce a whole new module with its own documentation etc. It suggests that simple, anonymous data structures don't always scale better than a classic type-based approach. NODE_ACTIONS was a dict containing lists of dicts mapping keys to attributes and functions, very much like you'd get in a JavaScript program.
Jeroen
--
https://code.launchpad.net/~jtv/maas/doris-would-you-like-nicer-node-actions/+merge/103742
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/doris-would-you-like-nicer-node-actions into lp:maas.
=== added file 'src/maasserver/node_action.py'
--- src/maasserver/node_action.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/node_action.py 2012-04-26 17:52:18 +0000
@@ -0,0 +1,225 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Node actions.
+
+These are actions that appear as buttons no the UI's Node page, depending
+on the node's state, the user's privileges etc.
+
+To define a new node action, derive a class for it from :class:`NodeAction`,
+provide the missing pieces documented in the class, and add it to
+`ACTION_CLASSES`. The actions will always appear on the page in the same
+order as they do in `ACTION_CLASSES`.
+"""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = [
+ 'compile_node_actions',
+ ]
+
+from abc import (
+ ABCMeta,
+ abstractmethod,
+ abstractproperty,
+ )
+from collections import OrderedDict
+from textwrap import dedent
+
+from django.core.urlresolvers import reverse
+from maasserver.enum import (
+ NODE_PERMISSION,
+ NODE_STATUS,
+ NODE_STATUS_CHOICES_DICT,
+ )
+from maasserver.exceptions import Redirect
+from maasserver.models import (
+ Node,
+ SSHKey,
+ )
+
+# All node statuses.
+ALL_STATUSES = set(NODE_STATUS_CHOICES_DICT.keys())
+
+
+class NodeAction:
+ """Base class for node actions."""
+
+ __metaclass__ = ABCMeta
+
+ display = abstractproperty("""
+ Action name.
+
+ Will be used as the label for the action's button.
+ """)
+
+ statuses = abstractproperty("""
+ Node states that this action makes sense for.
+
+ A collection of NODE_STATUS values. The action will be available
+ only if `node.status in action.statuses`.
+ """)
+
+ permission = abstractproperty("""
+ Required permission.
+
+ A NODE_PERMISSION value. The action will be available only if the
+ user has this given permission on the subject node.
+ """)
+
+ def __init__(self, node, user, request=None):
+ """Initialize a node action.
+
+ All node actions' initializers must accept these same arguments,
+ without variations.
+ """
+ self.node = node
+ self.user = user
+ self.request = request
+
+ def inhibit(self):
+ """Overridable: is there any reason not to offer this action?
+
+ This property may return a reason to inhibit this action, in which
+ case its button may still be visible in the UI, but disabled. A
+ tooltip will provide the reason, as returned by this method.
+
+ :return: A human-readable reason to inhibit the action, or None if
+ the action is valid.
+ """
+ return None
+
+ @abstractmethod
+ def perform(self):
+ """Perform this action.
+
+ Even though this is not the API, the action may raise
+ :class:`MAASAPIException` exceptions. When this happens, the view
+ will return to the client an http response reflecting the exception.
+
+ :return: A human-readable message confirming that the action has been
+ performed. It will be shown as an informational notice on the
+ Node page.
+ """
+
+ def is_permitted(self):
+ """Does the current user have the permission required?"""
+ return self.user.has_perm(self.permission, self.node)
+
+ # Uninitialized inhibititions cache.
+ _cached_inhibition = object()
+
+ @property
+ def inhibition(self):
+ """Caching version of `inhibit`."""
+ if self._cached_inhibition == NodeAction._cached_inhibition:
+ self._cached_inhibition = self.inhibit()
+ return self._cached_inhibition
+
+
+class Delete(NodeAction):
+ """Delete a node."""
+ display = "Delete node"
+ statuses = ALL_STATUSES
+ permission = NODE_PERMISSION.ADMIN
+
+ def inhibit(self):
+ if self.node.owner is not None:
+ return "You cannot delete this node because it's in use."
+ return None
+
+ def perform(self):
+ """Redirect to the delete view's confirmation page.
+
+ The rest of deletion is handled by a specialized deletion view.
+ All that the action really does is get you to its are-you-sure
+ page.
+ """
+ raise Redirect(reverse('node-delete', args=[self.node.system_id]))
+
+
+class AcceptAndCommission(NodeAction):
+ """Accept a node into the MAAS, and start the commissioning process."""
+ display = "Accept & commission"
+ statuses = (NODE_STATUS.DECLARED, )
+ permission = NODE_PERMISSION.ADMIN
+
+ def perform(self):
+ self.node.start_commissioning(self.user)
+ return "Node commissioning started."
+
+
+class RetryCommissioning(NodeAction):
+ """Retry commissioning of a node that failed previously."""
+ display = "Retry commissioning"
+ statuses = (NODE_STATUS.FAILED_TESTS, )
+ permission = NODE_PERMISSION.ADMIN
+
+ def perform(self):
+ self.node.start_commissioning(self.user)
+ return "Started a new attempt to commission this node."
+
+
+class StartNode(NodeAction):
+ """Acquire and start a node."""
+ display = "Start node"
+ statuses = (NODE_STATUS.READY, )
+ permission = NODE_PERMISSION.VIEW
+
+ def inhibit(self):
+ """The user must have an SSH key, so that they access the node."""
+ if not SSHKey.objects.get_keys_for_user(self.user).exists():
+ return dedent("""
+ You have no means of accessing the node after starting it.
+ Register an SSH key first. Do this on your Preferences
+ screen: click on the menu with your name at the top of the
+ page, select Preferences, and look for the "SSH keys" section.
+ """)
+ return None
+
+ def perform(self):
+ # Avoid circular imports.
+ from maasserver.api import get_oauth_token
+
+ # Be sure to acquire before starting, or start_nodes will think
+ # the node ineligible based on its un-acquired status.
+ self.node.acquire(get_oauth_token(self.request))
+ Node.objects.start_nodes([self.node.system_id], self.user)
+ return dedent("""
+ This node is now allocated to you.
+ It has been asked to start up.
+ """)
+
+
+ACTION_CLASSES = (
+ Delete,
+ AcceptAndCommission,
+ RetryCommissioning,
+ StartNode,
+ )
+
+
+def compile_node_actions(node, user, request=None, classes=ACTION_CLASSES):
+ """Provide :class:`NodeAction` objects for given request.
+
+ :param node: The :class:`Node` that the request pertains to.
+ :param user: The :class:`User` making the request.
+ :param request: The :class:`HttpRequest` being serviced. It may be used
+ to obtain information about the OAuth token being used.
+ :return: An :class:`OrderedDict` mapping applicable actions' display names
+ to corresponding :class:`NodeAction` instances. The dict is ordered
+ for consistent display.
+ """
+ applicable_actions = (
+ action_class(node, user, request)
+ for action_class in classes
+ if node.status in action_class.statuses)
+ return OrderedDict(
+ (action.display, action)
+ for action in applicable_actions
+ if action.is_permitted())
=== added file 'src/maasserver/tests/test_node_action.py'
--- src/maasserver/tests/test_node_action.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_node_action.py 2012-04-26 17:52:18 +0000
@@ -0,0 +1,232 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for node actions."""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = []
+
+from urlparse import urlparse
+
+from django.core.urlresolvers import reverse
+import maasserver.api
+from maasserver.enum import (
+ NODE_PERMISSION,
+ NODE_STATUS,
+ NODE_STATUS_CHOICES_DICT,
+ )
+from maasserver.exceptions import Redirect
+from maasserver.node_action import (
+ AcceptAndCommission,
+ compile_node_actions,
+ Delete,
+ NodeAction,
+ RetryCommissioning,
+ StartNode,
+ )
+from maasserver.provisioning import get_provisioning_api_proxy
+from maasserver.testing.factory import factory
+from maasserver.testing.testcase import TestCase
+
+
+ALL_STATUSES = NODE_STATUS_CHOICES_DICT.keys()
+
+
+class FakeNodeAction(NodeAction):
+ display = "Action label"
+ statuses = ALL_STATUSES
+ permission = NODE_PERMISSION.VIEW
+
+ # For testing: an inhibition for inhibit() to return.
+ fake_inhibition = None
+
+ def inhibit(self):
+ return self.fake_inhibition
+
+ def perform(self):
+ pass
+
+
+class TestNodeAction(TestCase):
+
+ def test_compile_node_actions_returns_available_actions(self):
+
+ class MyAction(FakeNodeAction):
+ display = factory.getRandomString()
+
+ actions = compile_node_actions(
+ factory.make_node(), factory.make_admin(), classes=[MyAction])
+ self.assertEqual([MyAction.display], actions.keys())
+
+ def test_compile_node_actions_checks_node_status(self):
+
+ class MyAction(FakeNodeAction):
+ statuses = (NODE_STATUS.READY, )
+
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ actions = compile_node_actions(
+ node, factory.make_admin(), classes=[MyAction])
+ self.assertEqual({}, actions)
+
+ def test_compile_node_actions_checks_permission(self):
+
+ class MyAction(FakeNodeAction):
+ permission = NODE_PERMISSION.EDIT
+
+ node = factory.make_node(status=NODE_STATUS.COMMISSIONING)
+ actions = compile_node_actions(
+ node, factory.make_user(), classes=[MyAction])
+ self.assertEqual({}, actions)
+
+ def test_compile_node_actions_includes_inhibited_actions(self):
+
+ class MyAction(FakeNodeAction):
+ fake_inhibition = factory.getRandomString()
+
+ actions = compile_node_actions(
+ factory.make_node(), factory.make_admin(), classes=[MyAction])
+ self.assertEqual([MyAction.display], actions.keys())
+
+ def test_compile_node_actions_maps_display_names(self):
+
+ class Action1(FakeNodeAction):
+ display = factory.getRandomString()
+
+ class Action2(FakeNodeAction):
+ display = factory.getRandomString()
+
+ actions = compile_node_actions(
+ factory.make_node(), factory.make_admin(),
+ classes=[Action1, Action2])
+ for label, action in actions.items():
+ self.assertEqual(label, action.display)
+
+ def test_compile_node_actions_maintains_order(self):
+ labels = [factory.getRandomString() for counter in range(4)]
+ classes = [
+ type(b"Action%d" % counter, (FakeNodeAction,), {'display': label})
+ for counter, label in enumerate(labels)]
+ actions = compile_node_actions(
+ factory.make_node(), factory.make_admin(), classes=classes)
+ self.assertSequenceEqual(labels, actions.keys())
+ self.assertSequenceEqual(
+ labels, [action.display for action in actions.values()])
+
+ def test_is_permitted_allows_if_user_has_permission(self):
+
+ class MyAction(FakeNodeAction):
+ permission = NODE_PERMISSION.EDIT
+
+ node = factory.make_node(
+ status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+ self.assertTrue(MyAction(node, node.owner).is_permitted())
+
+ def test_is_permitted_disallows_if_user_lacks_permission(self):
+
+ class MyAction(FakeNodeAction):
+ permission = NODE_PERMISSION.EDIT
+
+ node = factory.make_node(
+ status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+ self.assertFalse(MyAction(node, factory.make_user()).is_permitted())
+
+ def test_inhibition_wraps_inhibit(self):
+ inhibition = factory.getRandomString()
+ action = FakeNodeAction(factory.make_node(), factory.make_user())
+ action.fake_inhibition = inhibition
+ self.assertEqual(inhibition, action.inhibition)
+
+ def test_inhibition_caches_inhibition(self):
+ # The inhibition property will call inhibit() only once. We can
+ # prove this by changing the string inhibit() returns; it won't
+ # affect the value of the property.
+ inhibition = factory.getRandomString()
+ action = FakeNodeAction(factory.make_node(), factory.make_user())
+ action.fake_inhibition = inhibition
+ self.assertEqual(inhibition, action.inhibition)
+ action.fake_inhibition = factory.getRandomString()
+ self.assertEqual(inhibition, action.inhibition)
+
+ def test_inhibition_caches_None(self):
+ # An inhibition of None is also faithfully cached. In other
+ # words, it doesn't get mistaken for an uninitialized cache or
+ # anything.
+ action = FakeNodeAction(factory.make_node(), factory.make_user())
+ action.fake_inhibition = None
+ self.assertIsNone(action.inhibition)
+ action.fake_inhibition = factory.getRandomString()
+ self.assertIsNone(action.inhibition)
+
+ def test_Delete_inhibit_allows_if_node_has_no_owner(self):
+ unowned_node = factory.make_node(status=NODE_STATUS.READY)
+ self.assertIsNone(
+ Delete(unowned_node, factory.make_admin()).inhibit())
+
+ def test_Delete_inhibit_disallows_if_node_has_owner(self):
+ owned_node = factory.make_node(
+ status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+ action = Delete(owned_node, factory.make_admin())
+ inhibition = action.inhibit()
+ self.assertIsNotNone(inhibition)
+ self.assertEqual(
+ "You cannot delete this node because it's in use.", inhibition)
+
+ def test_Delete_redirects_to_node_delete_view(self):
+ node = factory.make_node()
+ action = Delete(node, factory.make_admin())
+ try:
+ action.perform()
+ except Redirect as e:
+ pass
+ self.assertEqual(
+ reverse('node-delete', args=[node.system_id]),
+ urlparse(unicode(e)).path)
+
+ def test_AcceptAndCommission_starts_commissioning(self):
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ action = AcceptAndCommission(node, factory.make_admin())
+ action.perform()
+ self.assertEqual(NODE_STATUS.COMMISSIONING, node.status)
+ self.assertEqual(
+ 'start',
+ get_provisioning_api_proxy().power_status.get(node.system_id))
+
+ def test_RetryCommissioning_starts_commissioning(self):
+ node = factory.make_node(status=NODE_STATUS.FAILED_TESTS)
+ action = RetryCommissioning(node, factory.make_admin())
+ action.perform()
+ self.assertEqual(NODE_STATUS.COMMISSIONING, node.status)
+ self.assertEqual(
+ 'start',
+ get_provisioning_api_proxy().power_status.get(node.system_id))
+
+ def test_StartNode_inhibit_allows_user_with_SSH_key(self):
+ user_with_key = factory.make_user()
+ factory.make_sshkey(user_with_key)
+ self.assertIsNone(
+ StartNode(factory.make_node(), user_with_key).inhibit())
+
+ def test_StartNode_inhibit_disallows_user_without_SSH_key(self):
+ user_without_key = factory.make_user()
+ action = StartNode(factory.make_node(), user_without_key)
+ inhibition = action.inhibit()
+ self.assertIsNotNone(inhibition)
+ self.assertIn("SSH key", inhibition)
+
+ def test_StartNode_acquires_and_starts_node(self):
+ node = factory.make_node(status=NODE_STATUS.READY)
+ user = factory.make_user()
+ consumer, token = user.get_profile().create_authorisation_token()
+ self.patch(maasserver.api, 'get_oauth_token', lambda request: token)
+ StartNode(node, user).perform()
+ self.assertEqual(NODE_STATUS.ALLOCATED, node.status)
+ self.assertEqual(user, node.owner)
+ self.assertEqual(
+ 'start',
+ get_provisioning_api_proxy().power_status.get(node.system_id))
