launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06993
[Merge] lp:~jtv/maas/transitions-to-actions into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/transitions-to-actions into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/transitions-to-actions/+merge/100730
It turns out that the coupling between model-side node state transitions and the node actions that we want to offer in the Node UI isn't as tight as expected.
This branch renames the latter to "node actions" so that they're a clear, separate layer on top of the model's checks of permissible status transitions.
There was also a bit of version drift in the documentation: the items in a node-action dict were no longer quite the same as what the comments and docstrings said. In one place I simply replaced the (slightly outdated) details with a reference to the main data structure, which is going to be less clear but also less prone to unnoticed changes.
--
https://code.launchpad.net/~jtv/maas/transitions-to-actions/+merge/100730
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/transitions-to-actions into lp:maas.
=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py 2012-04-03 10:19:47 +0000
+++ src/maasserver/forms.py 2012-04-04 05:33:20 +0000
@@ -11,12 +11,12 @@
__metaclass__ = type
__all__ = [
"CommissioningForm",
- "get_transition_form",
+ "get_action_form",
"HostnameFormField",
"NodeForm",
"MACAddressForm",
"MAASAndNetworkForm",
- "NodeTransitionForm",
+ "NodeActionForm",
"SSHKeyForm",
"UbuntuForm",
"UIAdminNodeEditForm",
@@ -194,20 +194,25 @@
return node
-# Node transitions methods.
-# The format is:
+# Node actions per status.
+#
+# This maps each NODE_STATUS to a list of actions applicable to a node
+# in that state. The actions show up in the user interface as buttons
+# on the node page.
+#
+# Each action is a dict:
+#
# {
-# old_status1: [
-# {
-# 'display': display_string11, # The name of the transition
-# # (to be displayed in the UI).
-# 'name': transition_name11, # The name of the transition.
-# 'permission': permission_required11,
-# },
-# ]
-# ...
+# # Action's display name; will be shown in the button.
+# 'display': "Paint node",
+# # Permission required to perform action.
+# 'permission': NODE_PERMISSION.EDIT,
+# # Callable that performs action. Takes parameters (node, user).
+# 'execute': lambda node, user: paint_node(
+# node, favourite_colour(user)),
+# }
#
-NODE_TRANSITIONS_METHODS = {
+NODE_ACTIONS = {
NODE_STATUS.DECLARED: [
{
'display': "Accept Enlisted node",
@@ -218,55 +223,48 @@
}
-class NodeTransitionForm(forms.Form):
- """A form used to perform a status change on a Node.
+class NodeActionForm(forms.Form):
+ """Base form for performing a node action.
- That form class should not be used directly but through subclasses
- created using `get_transition_form`.
+ This form class should not be used directly but through subclasses
+ created using `get_action_form`.
"""
user = AnonymousUser()
# The name of the input button used with this form.
- input_name = 'node_transition'
+ input_name = 'node_action'
def __init__(self, instance, *args, **kwargs):
- super(NodeTransitionForm, self).__init__(*args, **kwargs)
+ super(NodeActionForm, self).__init__(*args, **kwargs)
self.node = instance
- self.transition_buttons = self.available_transition_methods(
+ self.action_buttons = self.available_action_methods(
self.node, self.user)
- # Create a convenient dict to fetch the transition name and
+ # Create a convenient dict to fetch the action's name and
# the permission to be checked from the button name.
- self.transition_dict = {
- transition['display']: (
- transition['permission'], transition['execute'])
- for transition in self.transition_buttons
+ self.action_dict = {
+ action['display']: (action['permission'], action['execute'])
+ for action in self.action_buttons
}
- def available_transition_methods(self, node, user):
- """Return the transitions that this user is allowed to perform on
- a node.
+ def available_action_methods(self, node, user):
+ """Return the actions that this user is allowed to perform on a node.
:param node: The node for which the check should be performed.
:type node: :class:`maasserver.models.Node`
- :param user: The user used to perform the permission checks. Only the
- transitions available to this user will be returned.
+ :param user: The user who would be performing the action. Only the
+ actions available to this user will be returned.
:type user: :class:`django.contrib.auth.models.User`
- :return: A list of transition dicts (each dict contains 3 values:
- 'name': the name of the transition, 'permission': the permission
- required to perform this transition, 'method': the name of the
- method to execute on the node to perform the transition).
+ :return: Any applicable action dicts, as found in NODE_ACTIONS.
:rtype: Sequence
"""
- node_transitions = NODE_TRANSITIONS_METHODS.get(node.status, ())
return [
- node_transition for node_transition in node_transitions
- if user.has_perm(node_transition['permission'], node)]
+ action for action in NODE_ACTIONS.get(node.status, ())
+ if user.has_perm(action['permission'], node)]
def save(self):
- transition_name = self.data.get(self.input_name)
- permission, execute = self.transition_dict.get(
- transition_name, (None, None))
+ action_name = self.data.get(self.input_name)
+ permission, execute = self.action_dict.get(action_name, (None, None))
if execute is not None:
if not self.user.has_perm(permission, self.node):
raise PermissionDenied()
@@ -275,18 +273,17 @@
raise PermissionDenied()
-def get_transition_form(user):
- """Return a class derived from NodeTransitionForm for a specific user.
+def get_action_form(user):
+ """Return a class derived from NodeActionForm for a specific user.
:param user: The user for which to build a form derived from
- NodeTransitionForm.
+ NodeActionForm.
:type user: :class:`django.contrib.auth.models.User`
- :return: A form class derived from NodeTransitionForm.
+ :return: A form class derived from NodeActionForm.
:rtype: class:`django.forms.Form`
"""
return type(
- str("SpecificNodeTransitionForm"), (NodeTransitionForm,),
- {'user': user})
+ str("SpecificNodeActionForm"), (NodeActionForm,), {'user': user})
class ProfileForm(ModelForm):
=== modified file 'src/maasserver/templates/maasserver/node_view.html'
--- src/maasserver/templates/maasserver/node_view.html 2012-04-03 09:58:17 +0000
+++ src/maasserver/templates/maasserver/node_view.html 2012-04-04 05:33:20 +0000
@@ -14,7 +14,7 @@
</a>
</div>
{% endif %}
- {% if form.transition_buttons or can_delete %}
+ {% if form.action_buttons or can_delete %}
<div class="block size3">
<h4>Actions</h4>
{% if can_delete %}
@@ -22,14 +22,14 @@
Delete node
</a>
{% endif %}
- {% for transition in form.transition_buttons %}
+ {% for action in form.action_buttons %}
{% if forloop.first %}
<form id="node_actions" method="post" action=".">
{% endif %}
<input class="secondary {% if not forloop.first or can_delete %}space-top{% endif %}"
type="submit"
name="{{ form.input_name }}"
- value="{{ transition.display }}" />
+ value="{{ action.display }}" />
{% if forloop.last %}</form>{% endif %}
{% endfor %}
</div>
=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py 2012-04-03 10:19:47 +0000
+++ src/maasserver/tests/test_forms.py 2012-04-04 05:33:20 +0000
@@ -22,11 +22,11 @@
from maasserver.forms import (
ConfigForm,
EditUserForm,
- get_transition_form,
+ get_action_form,
HostnameFormField,
NewUserCreationForm,
- NODE_TRANSITIONS_METHODS,
- NodeTransitionForm,
+ NODE_ACTIONS,
+ NodeActionForm,
NodeWithMACAddressesForm,
ProfileForm,
UIAdminNodeEditForm,
@@ -264,103 +264,100 @@
self.assertItemsEqual(['', user.id], user_ids)
-class NodeTransitionsMethodsTests(TestCase):
- """Test the structure of NODE_TRANSITIONS_METHODS."""
+class NodeActionsTests(TestCase):
+ """Test the structure of NODE_ACTIONS."""
- def test_NODE_TRANSITION_METHODS_initial_states(self):
+ def test_NODE_ACTIONS_initial_states(self):
allowed_states = set(NODE_STATUS_CHOICES_DICT.keys() + [None])
- self.assertTrue(set(NODE_TRANSITIONS_METHODS.keys()) <= allowed_states)
-
- def test_NODE_TRANSITION_METHODS_dict(self):
- transitions = []
- for transition_list in NODE_TRANSITIONS_METHODS.values():
- transitions.extend(transition_list)
-
+ self.assertTrue(set(NODE_ACTIONS.keys()) <= allowed_states)
+
+ def test_NODE_ACTIONS_dict(self):
+ actions = sum(NODE_ACTIONS.values(), [])
self.assertThat(
- [sorted(transition.keys()) for transition in transitions],
+ [sorted(action.keys()) for action in actions],
AllMatch(Equals(sorted(['permission', 'display', 'execute']))))
-class TestNodeTransitionForm(TestCase):
+class TestNodeActionForm(TestCase):
- def test_available_transition_methods_for_declared_node_admin(self):
- # An admin has access to the "Enlist node" transition for a
+ def test_available_action_methods_for_declared_node_admin(self):
+ # An admin has access to the "Accept Enlisted node" action for a
# 'Declared' node.
admin = factory.make_admin()
node = factory.make_node(status=NODE_STATUS.DECLARED)
- form = get_transition_form(admin)(node)
- transitions = form.available_transition_methods(node, admin)
+ form = get_action_form(admin)(node)
+ actions = form.available_action_methods(node, admin)
self.assertEqual(
["Accept Enlisted node"],
- [transition['display'] for transition in transitions])
+ [action['display'] for action in actions])
self.assertEqual(
[NODE_PERMISSION.ADMIN],
- [transition['permission'] for transition in transitions])
+ [action['permission'] for action in actions])
- def test_available_transition_methods_for_declared_node_simple_user(self):
- # A simple user sees no transitions for a 'Declared' node.
+ def test_available_action_methods_for_declared_node_simple_user(self):
+ # A simple user sees no actions for a 'Declared' node.
user = factory.make_user()
node = factory.make_node(status=NODE_STATUS.DECLARED)
- form = get_transition_form(user)(node)
+ form = get_action_form(user)(node)
self.assertItemsEqual(
- [], form.available_transition_methods(node, user))
+ [], form.available_action_methods(node, user))
- def test_get_transition_form_creates_form_class_with_attributes(self):
+ def test_get_action_form_creates_form_class_with_attributes(self):
user = factory.make_admin()
- form_class = get_transition_form(user)
+ form_class = get_action_form(user)
self.assertEqual(user, form_class.user)
- def test_get_transition_form_creates_form_class(self):
+ def test_get_action_form_creates_form_class(self):
user = factory.make_admin()
node = factory.make_node(status=NODE_STATUS.DECLARED)
- form = get_transition_form(user)(node)
+ form = get_action_form(user)(node)
- self.assertIsInstance(form, NodeTransitionForm)
+ self.assertIsInstance(form, NodeActionForm)
self.assertEqual(node, form.node)
- def test_get_transition_form_for_admin(self):
+ def test_get_action_form_for_admin(self):
admin = factory.make_admin()
node = factory.make_node(status=NODE_STATUS.DECLARED)
- form = get_transition_form(admin)(node)
+ form = get_action_form(admin)(node)
self.assertItemsEqual(
{"Accept Enlisted node": (
'accept_enlistment', NODE_PERMISSION.ADMIN)},
- form.transition_dict)
+ form.action_dict)
- def test_get_transition_form_for_user(self):
+ def test_get_action_form_for_user(self):
user = factory.make_user()
node = factory.make_node(status=NODE_STATUS.DECLARED)
- form = get_transition_form(user)(node)
+ form = get_action_form(user)(node)
- self.assertIsInstance(form, NodeTransitionForm)
+ self.assertIsInstance(form, NodeActionForm)
self.assertEqual(node, form.node)
- self.assertItemsEqual({}, form.transition_dict)
+ self.assertItemsEqual({}, form.action_dict)
- def test_get_transition_form_node_for_admin_save(self):
+ def test_get_action_form_node_for_admin_save(self):
admin = factory.make_admin()
node = factory.make_node(status=NODE_STATUS.DECLARED)
- form = get_transition_form(admin)(
- node, {NodeTransitionForm.input_name: "Accept Enlisted node"})
+ form = get_action_form(admin)(
+ node, {NodeActionForm.input_name: "Accept Enlisted node"})
form.save()
self.assertEqual(NODE_STATUS.READY, node.status)
- def test_get_transition_form_for_user_save(self):
+ def test_get_action_form_for_user_save(self):
user = factory.make_user()
node = factory.make_node(status=NODE_STATUS.DECLARED)
- form = get_transition_form(user)(
- node, {NodeTransitionForm.input_name: "Enlist node"})
+ form = get_action_form(user)(
+ node, {NodeActionForm.input_name: "Enlist node"})
self.assertRaises(PermissionDenied, form.save)
- def test_get_transition_form_for_user_save_unknown_trans(self):
+ def test_get_action_form_for_user_save_unknown_trans(self):
user = factory.make_user()
node = factory.make_node(status=NODE_STATUS.DECLARED)
- form = get_transition_form(user)(
- node, {NodeTransitionForm.input_name: factory.getRandomString()})
+ form = get_action_form(user)(
+ node, {NodeActionForm.input_name: factory.getRandomString()})
self.assertRaises(PermissionDenied, form.save)
=== modified file 'src/maasserver/tests/test_views.py'
--- src/maasserver/tests/test_views.py 2012-04-03 12:05:37 +0000
+++ src/maasserver/tests/test_views.py 2012-04-04 05:33:20 +0000
@@ -27,7 +27,7 @@
views,
)
from maasserver.exceptions import NoRabbit
-from maasserver.forms import NodeTransitionForm
+from maasserver.forms import NodeActionForm
from maasserver.models import (
Config,
NODE_AFTER_COMMISSIONING_ACTION,
@@ -586,7 +586,7 @@
doc = fromstring(response.content)
inputs = [
input for input in doc.cssselect('form#node_actions input')
- if input.name == NodeTransitionForm.input_name]
+ if input.name == NodeActionForm.input_name]
self.assertSequenceEqual(
["Accept Enlisted node"], [input.value for input in inputs])
@@ -599,7 +599,7 @@
response = self.client.post(
node_link,
data={
- NodeTransitionForm.input_name: "Accept Enlisted node",
+ NodeActionForm.input_name: "Accept Enlisted node",
})
self.assertEqual(httplib.FOUND, response.status_code)
=== modified file 'src/maasserver/views.py'
--- src/maasserver/views.py 2012-04-03 12:06:24 +0000
+++ src/maasserver/views.py 2012-04-04 05:33:20 +0000
@@ -77,7 +77,7 @@
AddArchiveForm,
CommissioningForm,
EditUserForm,
- get_transition_form,
+ get_action_form,
MAASAndNetworkForm,
NewUserCreationForm,
ProfileForm,
@@ -122,7 +122,7 @@
return node
def get_form_class(self):
- return get_transition_form(self.request.user)
+ return get_action_form(self.request.user)
def get_context_data(self, **kwargs):
context = super(NodeView, self).get_context_data(**kwargs)
