launchpad-reviewers team mailing list archive

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

Raphaël Badin has proposed merging lp:~rvb/maas/maas-aciton-feedback-bug-973272 into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #973272 in MAAS: "No UI feedback after node action"
  https://bugs.launchpad.net/maas/+bug/973272

For more details, see:
https://code.launchpad.net/~rvb/maas/maas-aciton-feedback-bug-973272/+merge/100846

This branch adds the ability to publish a message when an action is performed.

I could have made this optional but I think it's really a good habit to publish a message each time an action is performed.

Note: technically I could have passed only the request to get_action_form but I'd rather pass the user *and* the request and have the request only used for message publishing.
-- 
https://code.launchpad.net/~rvb/maas/maas-aciton-feedback-bug-973272/+merge/100846
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/maas-aciton-feedback-bug-973272 into lp:maas.

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-04-04 15:11:34 +0000
+++ src/maasserver/forms.py	2012-04-04 17:58:18 +0000
@@ -24,6 +24,7 @@
     ]
 
 from django import forms
+from django.contrib import messages
 from django.contrib.auth.forms import (
     UserChangeForm,
     UserCreationForm,
@@ -193,9 +194,6 @@
 
 def start_node(node, user):
     """Start a node from the UI.  It will have no meta_data."""
-    # TODO: Provide some kind of feedback to the user.  Starting a node
-    # is fire-and-forget.  From the user's point of view, it's as if all
-    # that happens is the page reloading.
     Node.objects.start_nodes([node.system_id], user)
 
 
@@ -231,11 +229,13 @@
             'display': "Accept Enlisted node",
             'permission': NODE_PERMISSION.ADMIN,
             'execute': lambda node, user: Node.accept_enlistment(node),
+            'message': "Node accepted into the pool."
         },
         {
             'display': "Commission node",
             'permission': NODE_PERMISSION.ADMIN,
             'execute': lambda node, user: Node.start_commissioning(node, user),
+            'message': "Node commissioned."
         },
     ],
     NODE_STATUS.READY: [
@@ -243,6 +243,7 @@
             'display': "Start node",
             'permission': NODE_PERMISSION.EDIT,
             'execute': start_node,
+            'message': "Node started."
         },
     ],
     NODE_STATUS.ALLOCATED: [
@@ -250,6 +251,7 @@
             'display': "Start node",
             'permission': NODE_PERMISSION.EDIT,
             'execute': start_node,
+            'message': "Node started."
         },
     ],
 }
@@ -263,6 +265,7 @@
     """
 
     user = AnonymousUser()
+    request = None
 
     # The name of the input button used with this form.
     input_name = 'node_action'
@@ -275,7 +278,8 @@
         # Create a convenient dict to fetch the action's name and
         # the permission to be checked from the button name.
         self.action_dict = {
-            action['display']: (action['permission'], action['execute'])
+            action['display']: (
+                action['permission'], action['execute'], action['message'])
             for action in self.action_buttons
         }
 
@@ -294,28 +298,37 @@
             action for action in NODE_ACTIONS.get(node.status, ())
             if user.has_perm(action['permission'], node)]
 
+    def display_message(self, message):
+        if self.request is not None:
+            messages.add_message(self.request, messages.INFO, message)
+
     def save(self):
         action_name = self.data.get(self.input_name)
-        permission, execute = self.action_dict.get(action_name, (None, None))
+        permission, execute, message = (
+            self.action_dict.get(action_name, (None, None, None)))
         if execute is not None:
             if not self.user.has_perm(permission, self.node):
                 raise PermissionDenied()
             execute(self.node, self.user)
+            self.display_message(message)
         else:
             raise PermissionDenied()
 
 
-def get_action_form(user):
+def get_action_form(user, request=None):
     """Return a class derived from NodeActionForm for a specific user.
 
     :param user: The user for which to build a form derived from
         NodeActionForm.
     :type user: :class:`django.contrib.auth.models.User`
+    :param request: An optional request object to publish action messages.
+    :type request: django.http.HttpRequest
     :return: A form class derived from NodeActionForm.
     :rtype: class:`django.forms.Form`
     """
     return type(
-        str("SpecificNodeActionForm"), (NodeActionForm,), {'user': user})
+        str("SpecificNodeActionForm"), (NodeActionForm,),
+        {'user': user, 'request': request})
 
 
 class ProfileForm(ModelForm):

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2012-04-04 15:11:34 +0000
+++ src/maasserver/tests/test_forms.py	2012-04-04 17:58:18 +0000
@@ -242,9 +242,10 @@
 
     def test_NODE_ACTIONS_dict(self):
         actions = sum(NODE_ACTIONS.values(), [])
+        keys = ['permission', 'display', 'execute', 'message']
         self.assertThat(
             [sorted(action.keys()) for action in actions],
-            AllMatch(Equals(sorted(['permission', 'display', 'execute']))))
+            AllMatch(Equals(sorted(keys))))
 
 
 class TestNodeActionForm(TestCase):
@@ -292,11 +293,7 @@
         form = get_action_form(admin)(node)
 
         self.assertItemsEqual(
-            {"Accept Enlisted node": (
-                'accept_enlistment', NODE_PERMISSION.ADMIN),
-             "Commission node": (
-                'start_commissioning', NODE_PERMISSION.ADMIN),
-            },
+            ["Accept Enlisted node", "Commission node"],
             form.action_dict)
 
     def test_get_action_form_for_user(self):
@@ -311,7 +308,7 @@
     def test_get_action_form_node_for_admin_save(self):
         admin = factory.make_admin()
         node = factory.make_node(status=NODE_STATUS.DECLARED)
-        form = get_action_form(admin)(
+        form = get_action_form(admin, self.client.request)(
             node, {NodeActionForm.input_name: "Accept Enlisted node"})
         form.save()
 

=== modified file 'src/maasserver/tests/test_provisioning.py'
--- src/maasserver/tests/test_provisioning.py	2012-04-04 16:20:39 +0000
+++ src/maasserver/tests/test_provisioning.py	2012-04-04 17:58:18 +0000
@@ -20,7 +20,6 @@
 from maasserver.exceptions import MAASAPIException
 from maasserver.models import (
     ARCHITECTURE,
-    ARCHITECTURE_CHOICES,
     Config,
     Node,
     NODE_AFTER_COMMISSIONING_ACTION,

=== modified file 'src/maasserver/tests/test_views.py'
--- src/maasserver/tests/test_views.py	2012-04-04 14:45:23 +0000
+++ src/maasserver/tests/test_views.py	2012-04-04 17:58:18 +0000
@@ -610,8 +610,7 @@
         self.assertAttributes(node, params)
 
     def test_view_node_admin_has_button_to_accept_enlistement(self):
-        self.logged_in_user.is_superuser = True
-        self.logged_in_user.save()
+        self.become_admin()
         node = factory.make_node(status=NODE_STATUS.DECLARED)
         node_link = reverse('node-view', args=[node.system_id])
         response = self.client.get(node_link)
@@ -624,8 +623,7 @@
             "Accept Enlisted node", [input.value for input in inputs])
 
     def test_view_node_POST_admin_can_enlist_node(self):
-        self.logged_in_user.is_superuser = True
-        self.logged_in_user.save()
+        self.become_admin()
         node = factory.make_node(status=NODE_STATUS.DECLARED)
         node_link = reverse('node-view', args=[node.system_id])
         response = self.client.post(
@@ -638,6 +636,21 @@
         self.assertEqual(
             NODE_STATUS.READY, reload_object(node).status)
 
+    def test_view_node_POST_admin_enlist_node_sees_message(self):
+        self.become_admin()
+        node = factory.make_node(status=NODE_STATUS.DECLARED)
+        node_link = reverse('node-view', args=[node.system_id])
+        self.client.post(
+            node_link,
+            data={
+                NodeActionForm.input_name: "Accept Enlisted node",
+            })
+        response = self.client.get(node_link)
+
+        self.assertEqual(
+            ['Node accepted into the pool.'],
+            [message.message for message in response.context['messages']])
+
     def test_view_node_has_button_to_accept_enlistement_for_user(self):
         # A simple user can't see the button to enlist a declared node.
         node = factory.make_node(status=NODE_STATUS.DECLARED)

=== modified file 'src/maasserver/views.py'
--- src/maasserver/views.py	2012-04-04 14:45:23 +0000
+++ src/maasserver/views.py	2012-04-04 17:58:18 +0000
@@ -124,7 +124,7 @@
         return node
 
     def get_form_class(self):
-        return get_action_form(self.request.user)
+        return get_action_form(self.request.user, self.request)
 
     def get_context_data(self, **kwargs):
         context = super(NodeView, self).get_context_data(**kwargs)