launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/pre-987585-cosmetics-and-test-helper into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #987585 in MAAS: "Start Node button does not allocate node"
  https://bugs.launchpad.net/maas/+bug/987585

For more details, see:
https://code.launchpad.net/~jtv/maas/pre-987585-cosmetics-and-test-helper/+merge/103329

This is still to keep the diff for my main branch small and focused.  In the diff you'll find:

 * Removal of an unnecessary export.

 * Some docstrings that were previously missing.

 * A bit of cosmetic rearrangement of code: remove unnecessary negation / flatten nesting structure.

 * A genuine bug fix for a test (the test for an Allocated node used a Ready one instead).

 * Improvements to a test helper that POSTs to a page and then GETs the result.

 * Some changes in wording: keep sentences short / don't use commas to concatenate what could be stand-alone sentences / use dashes to disambiguate nontrivial adjective-noun sequences / don't say “details” when offering more generic information rather than details about the situation at hand.

The test helper deserves some explanation.  It would simulate a form submission from the user's point of view: a POST is answered with a redirect to a GET.  The helper did not verify the response from the POST, thus hiding errors during testing.  The client requests are not transactional, so changes saved in an abortive POST would still show up in the database!


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/pre-987585-cosmetics-and-test-helper/+merge/103329
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/pre-987585-cosmetics-and-test-helper into lp:maas.

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-04-23 05:57:31 +0000
+++ src/maasserver/forms.py	2012-04-24 17:17:20 +0000
@@ -17,7 +17,6 @@
     "NodeForm",
     "MACAddressForm",
     "MAASAndNetworkForm",
-    "NodeActionForm",
     "SSHKeyForm",
     "UbuntuForm",
     "UIAdminNodeEditForm",
@@ -334,20 +333,21 @@
             if user.has_perm(action['permission'], node)]
 
     def display_message(self, message):
+        """Show `message` as feedback after performing an action."""
         if self.request is not None:
             messages.add_message(self.request, messages.INFO, message)
 
     def save(self):
+        """An action was requested.  Perform it."""
         action_name = self.data.get(self.input_name)
         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()
+        if execute is None:
+            raise PermissionDenied()
+        if not self.user.has_perm(permission, self.node):
+            raise PermissionDenied()
+        execute(self.node, self.user)
+        self.display_message(message)
 
 
 def get_action_form(user, request=None):

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2012-04-23 05:57:31 +0000
+++ src/maasserver/tests/test_forms.py	2012-04-24 17:17:20 +0000
@@ -351,7 +351,7 @@
 
     def test_start_action_starts_allocated_node_for_owner(self):
         node = factory.make_node(
-            status=NODE_STATUS.READY, owner=factory.make_user())
+            status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
         form = get_action_form(node.owner)(
             node, {NodeActionForm.input_name: "Start node"})
         form.save()

=== modified file 'src/maasserver/tests/test_views_nodes.py'
--- src/maasserver/tests/test_views_nodes.py	2012-04-23 14:02:51 +0000
+++ src/maasserver/tests/test_views_nodes.py	2012-04-24 17:17:20 +0000
@@ -13,6 +13,7 @@
 __all__ = []
 
 import httplib
+from urlparse import urlparse
 
 from django.conf import settings
 from django.core.urlresolvers import reverse
@@ -264,16 +265,24 @@
             NODE_STATUS.COMMISSIONING, reload_object(node).status)
 
     def perform_action_and_get_node_page(self, node, action_name):
+        """POST to perform a node action, then load the resulting page."""
         node_link = reverse('node-view', args=[node.system_id])
-        self.client.post(
+        response = self.client.post(
             node_link,
             data={
                 NodeActionForm.input_name: action_name,
             })
-        response = self.client.get(node_link)
-        return response
+        if response.status_code != httplib.FOUND:
+            self.fail(
+                "POST failed with code %d: '%s'"
+                % (response.status_code, response.content))
+        redirect = urlparse(response['Location']).path
+        if redirect != node_link:
+            self.fail(
+                "Odd: POST on %s redirected to %s." % (node_link, redirect))
+        return self.client.get(node_link)
 
-    def test_start_commisionning_displays_message(self):
+    def test_start_commisioning_displays_message(self):
         self.become_admin()
         node = factory.make_node(status=NODE_STATUS.DECLARED)
         response = self.perform_action_and_get_node_page(

=== modified file 'src/maasserver/views/nodes.py'
--- src/maasserver/views/nodes.py	2012-04-23 10:57:09 +0000
+++ src/maasserver/views/nodes.py	2012-04-24 17:17:20 +0000
@@ -76,11 +76,10 @@
 # Info message displayed on the node page for COMMISSIONING
 # or READY nodes.
 NODE_BOOT_INFO = mark_safe("""
-You can boot this node using Avahi enabled boot media or an
-adequately configured dhcp server, see
+You can boot this node using Avahi-enabled boot media or an adequately
+configured dhcp server.  See
 <a href="https://wiki.ubuntu.com/ServerTeam/MAAS/AvahiBoot";>
-https://wiki.ubuntu.com/ServerTeam/MAAS/AvahiBoot</a> for
-details.
+https://wiki.ubuntu.com/ServerTeam/MAAS/AvahiBoot</a> for instructions.
 """)