Merge lp:~jtv/maas/show-commissioning-results into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

UI for displaying node commissioning output (still hidden behind feature flag).

This is something I mentioned last week. It's a bit larger than usual because it's basically the result of a spike. I couldn't take the lack of visibility any more, and hacked up a UI. After that it was a matter of cleaning up and writing tests. It's not quite good enough to be proud of, which is why it's behind a feature flag for now. See ENABLE_NODECOMMISSIONRESULTS. No fancy framework or infrastructure for feature flags, or even a setting or config-file entry; just a dumb variable that we can edit.

Hopefully with this so close to being fully operational, we'll find ourselves motivated to make the final improvements it needs and get something out to our users that will make their lives (and ours) a little better.

With the feature flag enabled, administrators will see a link on the Node page — provided the node in question has commissioning results of course — which shows a count of results, and links to a listing. The listing is actually a generic global listing, but in this case filtered to a specific node. I did not bother building a link to "show all commissioning results," though we could easily do it now if we decide it's useful. A notice of failed commissioning scripts does not link to those results, but of course it would be nice if we could add that. It's just that getting something now was more important to me than getting something good later.

The results listing again is not particularly sophisticated. It sorts by node ID for basic grouping (primitive, but remember the UI right now only ever shows results for one node at a time), then by date so you get to see a reverse chronological listing even if the node has results from multiple commissioning rounds, and finally by the name of the output file — in case two results are reported by the same API call.

Layout of the new code is awkward. The NodeCommissionResult model is defined in the metadataserver app, but the new views are in maasserver because that's where the page lives, and then the templates are in src/maasserver/metadataserver/. Suggestions welcome — though I hope we can do as much as possible in separate branches.

[Raphaël Badin (rvb)]

First of all, thanks a lot for doing this. And congrats for getting something done in such a short time!

[0]

I like the design you've chosen. I like the choice to have a top-level nodecommissioningresults url that you can then filter using query parameters. It's somewhat similar to the API and it's pretty flexible.

> I couldn't take the lack of visibility any more, and hacked up a UI.

It was a problem for our users so it's definitely a worthwhile addition to the UI.

[1]

> Layout of the new code is awkward.

The Django app 'maasserver' depends on the Django app 'metadataserver'; now it happens to provide UI for models defined in the 'metadataserver', that's really fine even if it's a tiny bit awkward. The only thing we could do is more the NodeCommissioning model back into maasserver but I don't think it's worth it.

[2]

+++ src/maasserver/views/nodecommissionresult.py

202 + results = NodeCommissionResult.objects.all()
203 + node = self.request.GET.get('node')
204 + if node is not None:
205 + results = results.filter(node__system_id__in=node)

This can't work: the 'node' object (which I'd rename 'system_ids' if I where you) is the *first* value of the parameter 'node'; what I mean is this: if you use request.GET.get('zz') and the request is "?zz=aa&zz=bb" you'll get 'aa'. You need to use getlist() here (see https://docs.djangoproject.com/en/dev/ref/request-response/#querydict-objects) to get the list of all the values. I'm guessing that's what you had in mind given that you're using that in conjunction with 'system_id__in'.

[3]

Let's add a couple of fake NodeCommissioningResult objects to the sample data (http://paste.ubuntu.com/6947847/). It will make the diff bigger but it will help us a great deal to see if this is working or not (I did that manually and that's how I found the problem I described in [2]).

[Raphaël Badin (rvb)]

Couple of remarks about the listing page:

[4]

It would be nice to see, possibly in the title of the page, a mention of the node used to filter the results. (I know the node is part of the fields listed in the listing itself.)

[5]

It's weird having to click on the status to access the result page. It's weird because I'd expect having to click on the name of the file and it's weird because I'd expect the link to be on the left-hand side, like with all the other listings in MAAS.

Couple of remarks about the result page:

[6]

"Time" says "Result was registered at Feb. 17, 2014, 10:16 a.m.".

I'd get rid of the "Result was registered at" and put it in the title instead. I think it's best to keep the value of the fields to exactly what's in the database for clarity. The explanation of what the field is should be in the title.

Same remark for "Return value" (example value: "Script returned 2").

[Raphaël Badin (rvb)]

Thanks for the fixes, let's get this landed so we can both work on tweaking the display.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Thanks for the review!

=== modified file 'src/maasserver/templates/maasserver/node_view.html'
--- src/maasserver/templates/maasserver/node_view.html	2013-12-18 17:35:45 +0000
+++ src/maasserver/templates/maasserver/node_view.html	2014-02-17 12:29:17 +0000
@@ -188,6 +188,17 @@
         <span>{{ node.routers|join:", " }}</span>
       </li>
     {% endif %}
+    {% if user.is_superuser and nodecommissionresults > 0 %}
+      <li class="block first size5 separate" id="nodecommissionresults">
+        <h4>Commissioning output</h4>
+        <span>
+          <a href="{% url 'nodecommissionresult-list' %}?node={{node.system_id}}">
+            {{ nodecommissionresults }}
+            output file{{ nodecommissionresults|pluralize }}
+          </a>
+        </span>
+      </li>
+    {% endif %}
     {% if probed_details %}
       <li class="block first separate">
         <h2>Raw discovery data</h2>

=== added directory 'src/maasserver/templates/metadataserver'
=== added file 'src/maasserver/templates/metadataserver/nodecommissionresult.html'
--- src/maasserver/templates/metadataserver/nodecommissionresult.html	1970-01-01 00:00:00 +0000
+++ src/maasserver/templates/metadataserver/nodecommissionresult.html	2014-02-17 12:29:17 +0000
@@ -0,0 +1,38 @@
+{% extends "maasserver/base.html" %}
+
+{% block nav-active-settings %}active{% endblock %}
+{% block title %}Commissioning result for node {{ object.node.hostname }}, script {{ object.name }}{% endblock %}
+{% block page-title %}Commissioning result for node: {{ object.node.hostname }}{% endblock %}
+
+{% block content %}
+  <ul class="data-list">
+    <li class="block size7 first" id="name">
+      <h4>Output file</h4>
+      <span>{{ object.name }}</span>
+    </li>
+    <li class="block size8" id="node">
+      <h4>Node</h4>
+      <span>
+        <a href="{% url 'node-view' object.node.system_id %}">
+          {{ object.node.hostname }}
+        </a>
+      </span>
+    </li>
+    <li class="block size7 first" id="time">
+      <h4>Time</h4>
+      <span>Result was registered at {{ object.created }}</span>
+    </li>
+    <li class="block size3" id="script-result">
+      <h4>Return value</h4>
+      <span>Script returned {{ object.script_result }}</span>
+    </li>
+
+    <li class="block first separate" id="output">
+      <h2>Output</h2>
+      <pre>
+{{ object.get_data_as_html }}
+      </pre>
+    </li>
+  </ul>
+
+{% endblock %}

=== added file 'src/maasserver/templates/metadataserver/nodecommissionresult_list.html'
--- src/maasserver/templates/metadataserver/nodecommissionresult_list.html	1970-01-01 00:00:00 +0000
+++ src/maasserver/templates/metadataserver/nodecommissionresult_list.html	2014-02-17 12:29:17 +0000
@@ -0,0 +1,60 @@
+{% extends "maasserver/base.html" %}
+
+{% block nav-active-zone-list %}active{% endblock %}
+{% block title %}Node commissioning results{% endblock %}
+{% block page-title %}
+{{ paginator.count }} result{{ paginator.count|pluralize }}
+in {% include "maasserver/site_title.html" %}
+{% endblock %}
+
+{% block html_includes %}{% include "maasserver/snippets.html" %}
+{% endblock %}
+
+{% block content %}
+    <div id="results">
+      <h2>Commissioning results</h2>
+      {% if paginator.count == 0 %}
+      <p>No matching commissioning results.</p>
+      {% else %}
+      <table class="list">
+        <thead>
+          <tr>
+            <th>Time</th>
+            <th>Node</th>
+            <th>Output file</th>
+            <th>Script result<th>
+          </tr>
+        </thead>
+        <tbody>
+          {% for result_item in results_list %}
+          <tr class="result {% cycle 'even' 'odd' %}">
+            <td>{{ result_item.created }}</td>
+            <td>
+              <a href="{% url 'node-view' result_item.node.system_id %}">
+                {{ result_item.node.system_id }}
+              </a>
+            </td>
+            <td>
+                {{ result_item.name }}
+            </td>
+            <td>
+                <a href="{% url 'nodecommissionresult-view' result_item.id %}">
+                  {% if result_item.script_result == 0 %}
+                  OK: {{ result_item.script_result }}
+                  {% else %}
+                  <span class="warning">
+                    <img src="{{ STATIC_URL }}img/warning.png" title="Failed" />
+                    FAILED: {{ result_item.script_result }}
+                  </span>
+                  {% endif %}
+              </a>
+            </td>
+          </tr>
+          {% endfor %}
+        </tbody>
+      </table>
+      {% endif %}
+      <div class="clear"></div>
+    </div>
+
+{% endblock %}

=== modified file 'src/maasserver/urls.py'
--- src/maasserver/urls.py	2014-02-06 10:14:43 +0000
+++ src/maasserver/urls.py	2014-02-17 12:29:17 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """URL routing configuration."""
@@ -34,6 +34,10 @@
     NetworkListView,
     NetworkView,
     )
+from maasserver.views.nodecommissionresult import (
+    NodeCommissionResultListView,
+    NodeCommissionResultView,
+    )
 from maasserver.views.nodes import (
     enlist_preseed_view,
     MacAdd,
@@ -200,6 +204,14 @@
         r'^commissioning-scripts/add/$',
         CommissioningScriptCreate.as_view(),
         name='commissioning-script-add'),
+    adminurl(
+        r'^commissioning-results/$',
+        NodeCommissionResultListView.as_view(),
+        name='nodecommissionresult-list'),
+    adminurl(
+        r'^commissioning-results/(?P<id>[0-9]+)/$',
+        NodeCommissionResultView.as_view(),
+        name='nodecommissionresult-view'),
 )
 
 # Tag views.

=== added file 'src/maasserver/views/nodecommissionresult.py'
--- src/maasserver/views/nodecommissionresult.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/views/nodecommissionresult.py	2014-02-17 12:29:17 +0000
@@ -0,0 +1,44 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Views for node commissioning results."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = [
+    'NodeCommissionResultListView',
+    ]
+
+from django.shortcuts import get_object_or_404
+from django.views.generic import DetailView
+from maasserver.views import PaginatedListView
+from metadataserver.models import NodeCommissionResult
+
+
+class NodeCommissionResultListView(PaginatedListView):
+
+    template_name = 'maasserver/nodecommissionresult-list.html'
+    context_object_name = 'results_list'
+
+    def get_queryset(self):
+        results = NodeCommissionResult.objects.all()
+        system_ids = self.request.GET.getlist('node')
+        if system_ids is not None and len(system_ids) > 0:
+            results = results.filter(node__system_id__in=system_ids)
+        return results.order_by('node', '-created', 'name')
+
+
+class NodeCommissionResultView(DetailView):
+
+    template_name = 'metadataserver/nodecommissionresult.html'
+
+    def get_object(self):
+        result_id = self.kwargs.get('id')
+        return get_object_or_404(NodeCommissionResult, id=result_id)

=== modified file 'src/maasserver/views/nodes.py'
--- src/maasserver/views/nodes.py	2014-01-21 06:08:12 +0000
+++ src/maasserver/views/nodes.py	2014-02-17 12:29:17 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Nodes views."""
@@ -83,6 +83,7 @@
     HelpfulDeleteView,
     PaginatedListView,
     )
+from metadataserver.models import NodeCommissionResult
 from provisioningserver.tags import merge_details_cleanly
 
 
@@ -399,6 +400,11 @@
 """)
 
 
+# Feature flag: link to node commissioning results?
+# XXX jtv 2014-02-13: Remove this when this UI feature is good enough.
+ENABLE_NODECOMMISSIONRESULTS = False
+
+
 class NodeView(NodeViewMixin, UpdateView):
     """View class to display a node's information and buttons for the actions
     which can be performed on this node.
@@ -440,6 +446,13 @@
         else:
             context["probed_details"] = etree.tostring(
                 probed_details, encoding=unicode, pretty_print=True)
+
+        if ENABLE_NODECOMMISSIONRESULTS:
+            results = NodeCommissionResult.objects.filter(node=node).count()
+        else:
+            results = 0
+        context['nodecommissionresults'] = results
+
         return context
 
     def dispatch(self, *args, **kwargs):

=== added file 'src/maasserver/views/tests/test_nodecommissionresults.py'
--- src/maasserver/views/tests/test_nodecommissionresults.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/views/tests/test_nodecommissionresults.py	2014-02-17 12:29:17 +0000
@@ -0,0 +1,248 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the `NodeCommissionResult` views."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = []
+
+import httplib
+from operator import attrgetter
+from random import randint
+
+from django.core.urlresolvers import reverse
+from django.http.request import QueryDict
+from lxml import html
+from maasserver.testing import extract_redirect
+from maasserver.testing.factory import factory
+from maasserver.testing.testcase import MAASServerTestCase
+from maasserver.utils.orm import get_one
+from maasserver.views.nodecommissionresult import NodeCommissionResultListView
+from mock import Mock
+from testtools.matchers import HasLength
+
+
+class TestNodeCommissionResultView(MAASServerTestCase):
+
+    def request_page(self, result):
+        """Request and parse the  page for the given `NodeCommissionResult`.
+
+        :return: The page's main content as an `lxml.html.HtmlElement`.
+        """
+        link = reverse('nodecommissionresult-view', args=[result.id])
+        response = self.client.get(link)
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        doc = html.fromstring(response.content)
+        return get_one(doc.cssselect('#content'))
+
+    def extract_field(self, doc, field_name, containing_tag='span'):
+        """Get the content text from one of the <li> fields on the page.
+
+        This works on the basis that each of the fields has an `id` attribute
+        which is unique on the page, and contains exactly one tag of the type
+        given as `containing_tag`, which holds the field value.
+        """
+        field = get_one(doc.cssselect('#' + field_name))
+        value = get_one(field.cssselect(containing_tag))
+        return value.text_content().strip()
+
+    def test_requires_admin(self):
+        self.client_log_in(as_admin=False)
+        result = factory.make_node_commission_result()
+        response = self.client.get(
+            reverse('nodecommissionresult-view', args=[result.id]))
+        self.assertEqual(reverse('login'), extract_redirect(response))
+
+    def test_displays_result(self):
+        self.client_log_in(as_admin=True)
+        result = factory.make_node_commission_result(
+            data=factory.getRandomString().encode('ascii'))
+        doc = self.request_page(result)
+
+        self.assertEqual(result.name, self.extract_field(doc, 'name'))
+        self.assertEqual(
+            result.node.hostname,
+            self.extract_field(doc, 'node'))
+        self.assertEqual(
+            "Script returned %d" % result.script_result,
+            self.extract_field(doc, 'script-result'))
+        self.assertEqual(result.data, self.extract_field(doc, 'output', 'pre'))
+
+    def test_escapes_html_in_output(self):
+        self.client_log_in(as_admin=True)
+        # It's actually surprisingly hard to test for this, because lxml
+        # un-escapes on parsing, and is very tolerant of malformed input.
+        # Parsing an un-escaped A<B>C, however, would produce text "AC"
+        # (because the <B> looks like a tag).
+        result = factory.make_node_commission_result(data=b'A<B>C')
+        doc = self.request_page(result)
+        self.assertEqual('A<B>C', self.extract_field(doc, 'output', 'pre'))
+
+    def test_escapes_binary_in_output(self):
+        self.client_log_in(as_admin=True)
+        result = factory.make_node_commission_result(data=b'A\xffB')
+        doc = self.request_page(result)
+        self.assertEqual('A\ufffdB', self.extract_field(doc, 'output', 'pre'))
+
+
+class TestNodeCommissionResultListView(MAASServerTestCase):
+
+    def make_query_string(self, nodes):
+        """Compose a URL query string to filter for the given nodes."""
+        return '&'.join('node=%s' % node.system_id for node in nodes)
+
+    def request_page(self, nodes=None):
+        """Request and parse the  page for the given `NodeCommissionResult`.
+
+        :param node: Optional list of `Node` for which results should be
+            displayed.  If not given, all results are displayed.
+        :return: The page's main content as an `lxml.html.HtmlElement`.
+        """
+        link = reverse('nodecommissionresult-list')
+        if nodes is not None:
+            link += '?' + self.make_query_string(nodes)
+        response = self.client.get(link)
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        return get_one(html.fromstring(response.content).cssselect('#content'))
+
+    def make_view(self, nodes=None):
+        if nodes is None:
+            query_string = ''
+        else:
+            query_string = self.make_query_string(nodes)
+        view = NodeCommissionResultListView()
+        view.request = Mock()
+        view.request.GET = QueryDict(query_string)
+        return view
+
+    def test_requires_admin(self):
+        self.client_log_in(as_admin=False)
+        response = self.client.get(reverse('nodecommissionresult-list'))
+        self.assertEqual(reverse('login'), extract_redirect(response))
+
+    def test_lists_empty_page(self):
+        self.client_log_in(as_admin=True)
+        content = self.request_page()
+        self.assertIn(
+            "No matching commissioning results.",
+            content.text_content().strip())
+        self.assertEqual([], content.cssselect('.result'))
+
+    def test_lists_results(self):
+        self.client_log_in(as_admin=True)
+        result = factory.make_node_commission_result(script_result=0)
+        content = self.request_page()
+        result_row = get_one(content.cssselect('.result'))
+        fields = result_row.cssselect('td')
+
+        [time, node, output_file, script_result] = fields
+        self.assertIn('%d' % result.created.year, time.text_content())
+        self.assertEqual(result.node.system_id, node.text_content().strip())
+        self.assertEqual(
+            reverse('node-view', args=[result.node.system_id]),
+            get_one(node.cssselect('a')).get('href'))
+        self.assertEqual(result.name, output_file.text_content().strip())
+        self.assertEqual('OK: 0', script_result.text_content().strip())
+
+    def test_shows_failure(self):
+        self.client_log_in(as_admin=True)
+        result = factory.make_node_commission_result(
+            script_result=randint(1, 100))
+        content = self.request_page()
+        result_row = get_one(content.cssselect('.result'))
+        fields = result_row.cssselect('td')
+        [_, _, _, script_result] = fields
+        self.assertEqual(
+            "FAILED: %d" % result.script_result,
+            script_result.text_content().strip())
+        self.assertNotEqual([], script_result.find_class('warning'))
+
+    def test_links_to_result(self):
+        self.client_log_in(as_admin=True)
+        result = factory.make_node_commission_result(
+            script_result=randint(1, 100))
+        content = self.request_page()
+        result_row = get_one(content.cssselect('.result'))
+        fields = result_row.cssselect('td')
+        [_, _, _, script_result] = fields
+        link = get_one(script_result.cssselect('a'))
+        self.assertEqual(
+            reverse('nodecommissionresult-view', args=[result.id]),
+            link.get('href'))
+
+    def test_groups_by_node(self):
+        nodes = [factory.make_node() for _ in range(3)]
+        # Create two results per node, but interleave them so the results for
+        # any given node are unlikely to occur side by side by accident.
+        for _ in range(2):
+            for node in nodes:
+                factory.make_node_commission_result(node=node)
+        sorted_results = self.make_view().get_queryset()
+        self.assertEqual(sorted_results[0].node, sorted_results[1].node)
+        self.assertEqual(sorted_results[2].node, sorted_results[3].node)
+        self.assertEqual(sorted_results[4].node, sorted_results[5].node)
+
+    def test_sorts_by_creation_time_for_same_node(self):
+        node = factory.make_node()
+        results = [
+            factory.make_node_commission_result(node=node)
+            for _ in range(3)
+            ]
+        for result in results:
+            result.created -= factory.make_timedelta()
+            result.save()
+
+        self.assertEqual(
+            sorted(results, key=attrgetter('created'), reverse=True),
+            list(self.make_view().get_queryset()))
+
+    def test_sorts_by_name_for_same_node_and_creation_time(self):
+        node = factory.make_node()
+        results = {
+            factory.make_node_commission_result(
+                node=node, name=factory.make_name().lower())
+            for _ in range(5)
+            }
+        self.assertEqual(
+            sorted(results, key=attrgetter('name')),
+            list(self.make_view().get_queryset()))
+
+    def test_filters_by_node(self):
+        factory.make_node_commission_result()
+        node = factory.make_node()
+        node_results = {
+            factory.make_node_commission_result(node=node) for _ in range(3)
+            }
+        factory.make_node_commission_result()
+
+        self.assertEqual(
+            node_results,
+            set(self.make_view(nodes=[node]).get_queryset()))
+
+    def test_combines_node_filters(self):
+        # The nodes are passed as GET parameters, which means there is some
+        # subtlety to how they are passed to the application.  Reading them
+        # naively would ignore all but the first node passed, so make sure we
+        # process all of them.
+        self.client_log_in(as_admin=True)
+        results = [factory.make_node_commission_result() for _ in range(3)]
+        matching_results = results[1:3]
+        content = self.request_page(
+            nodes=[result.node for result in matching_results])
+        rows = content.cssselect('.result')
+        self.assertThat(rows, HasLength(len(matching_results)))
+        matching_names = set()
+        for row in rows:
+            [_, _, name, _] = row.cssselect('td')
+            matching_names.add(name.text_content().strip())
+        self.assertEqual(
+            {result.name for result in matching_results},
+            matching_names)

=== modified file 'src/maasserver/views/tests/test_nodes.py'
--- src/maasserver/views/tests/test_nodes.py	2014-02-11 22:00:31 +0000
+++ src/maasserver/views/tests/test_nodes.py	2014-02-17 12:29:17 +0000
@@ -17,6 +17,7 @@
 import httplib
 from operator import attrgetter
 import os
+from random import randint
 from textwrap import dedent
 from urlparse import (
     parse_qsl,
@@ -947,6 +948,110 @@
         self.assertEqual(params.items(), query_string_params)
 
 
+class NodeCommissionResultsDisplayTest(MAASServerTestCase):
+    """Tests for the link to node commissioning results on the Node page."""
+
+    def enable_display(self):
+        """Enable the feature flag behind which this display is hidden."""
+        # XXX jtv 2014-02-13: Remove this once feature is good enough.
+        self.patch(nodes_views, 'ENABLE_NODECOMMISSIONRESULTS', True)
+
+    def request_results_display(self, node):
+        """Request the page for `node`, and extract the results display.
+
+        Fails if generating, loading or parsing the page failed; or if
+        there was more than one section of commissioning results.
+
+        :return: An `lxml.html.HtmlElement` representing the commissioning
+            results portion of the page; or `None` if it was not present on
+            the page.
+        """
+        node_link = reverse('node-view', args=[node.system_id])
+        response = self.client.get(node_link)
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        doc = fromstring(response.content)
+        results_display = doc.cssselect('#nodecommissionresults')
+        if len(results_display) == 0:
+            return None
+        elif len(results_display) == 1:
+            return results_display[0]
+        else:
+            self.fail("Found more than one matching tag: %s" % results_display)
+
+    def get_results_link(self, display):
+        """Find the results link in `display`.
+
+        :param display: Results display section for a node, as returned by
+            `request_results_display`.
+        :return: `lxml.html.HtmlElement` for the link to the node's
+            commissioning results, as found in `display`; or `None` if it was
+            not present.
+        """
+        links = display.cssselect('a')
+        if len(links) == 0:
+            return None
+        elif len(links) == 1:
+            return links[0]
+        else:
+            self.fail("Found more than one link: %s" % links)
+
+    def normalise_whitespace(self, text):
+        """Return a version of `text` where all whitespace is single spaces."""
+        return ' '.join(text.split())
+
+    def test_view_node_links_to_commissioning_results_if_appropriate(self):
+        self.enable_display()
+        self.client_log_in(as_admin=True)
+        result = factory.make_node_commission_result()
+        section = self.request_results_display(result.node)
+        link = self.get_results_link(section)
+        results_list = reverse('nodecommissionresult-list')
+        self.assertEqual(
+            results_list + '?node=%s' % result.node.system_id,
+            link.get('href'))
+
+    def test_view_node_hides_commissioning_results_by_default(self):
+        # XXX jtv 2014-02-13: Remove this test once we get rid of feature flag.
+        self.client_log_in(as_admin=True)
+        result = factory.make_node_commission_result()
+        self.assertIsNone(self.request_results_display(result.node))
+
+    def test_view_node_shows_commissioning_results_only_if_present(self):
+        self.enable_display()
+        self.client_log_in(as_admin=True)
+        node = factory.make_node()
+        self.assertIsNone(self.request_results_display(node))
+
+    def test_view_node_shows_commissioning_results_only_to_superuser(self):
+        self.enable_display()
+        self.client_log_in(as_admin=False)
+        result = factory.make_node_commission_result()
+        self.assertIsNone(self.request_results_display(result.node))
+
+    def test_view_node_shows_single_commissioning_result(self):
+        self.enable_display()
+        self.client_log_in(as_admin=True)
+        result = factory.make_node_commission_result()
+        section = self.request_results_display(result.node)
+        link = self.get_results_link(section)
+        self.assertEqual(
+            "1 output file",
+            self.normalise_whitespace(link.text_content()))
+
+    def test_view_node_shows_multiple_commissioning_results(self):
+        self.enable_display()
+        self.client_log_in(as_admin=True)
+        node = factory.make_node()
+        num_results = randint(2, 5)
+        for _ in range(num_results):
+            factory.make_node_commission_result(node=node)
+        section = self.request_results_display(node)
+        link = self.get_results_link(section)
+        self.assertEqual(
+            "%d output files" % num_results,
+            self.normalise_whitespace(link.text_content()))
+
+
 class NodeListingSelectionJSControls(SeleniumTestCase):
 
     def test_node_list_js_control_select_all(self):

=== modified file 'src/maastesting/factory.py'
--- src/maastesting/factory.py	2014-02-10 10:15:07 +0000
+++ src/maastesting/factory.py	2014-02-17 12:29:17 +0000
@@ -180,6 +180,12 @@
         stamp = random.randrange(start, end)
         return datetime.datetime.fromtimestamp(stamp)
 
+    def make_timedelta(self):
+        return datetime.timedelta(
+            days=random.randint(0, 3 * 365),
+            seconds=random.randint(0, 24 * 60 * 60 - 1),
+            microseconds=random.randint(0, 999999))
+
     def make_file(self, location, name=None, contents=None):
         """Create a file, and write data to it.
 

=== modified file 'src/metadataserver/models/nodecommissionresult.py'
--- src/metadataserver/models/nodecommissionresult.py	2013-10-07 09:12:40 +0000
+++ src/metadataserver/models/nodecommissionresult.py	2014-02-17 12:29:17 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """:class:`NodeCommissionResult` model."""
@@ -24,6 +24,7 @@
     Manager,
     )
 from django.shortcuts import get_object_or_404
+from django.utils.html import escape
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.timestampedmodel import TimestampedModel
 from metadataserver import DefaultMeta
@@ -97,3 +98,10 @@
     data = BinaryField(
         max_length=1024 * 1024, editable=True, blank=True, default=b'',
         null=False)
+
+    def __unicode__(self):
+        return "%s/%s" % (self.node.system_id, self.name)
+
+    def get_data_as_html(self):
+        """More-or-less human-readable HTML representation of the output."""
+        return escape(self.data.decode('utf-8', 'replace'))

=== modified file 'src/metadataserver/models/tests/test_nodecommissionresult.py'
--- src/metadataserver/models/tests/test_nodecommissionresult.py	2013-10-07 11:08:54 +0000
+++ src/metadataserver/models/tests/test_nodecommissionresult.py	2014-02-17 12:29:17 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the :class:`NodeCommissionResult` model."""
@@ -27,6 +27,12 @@
 class TestNodeCommissionResult(DjangoTestCase):
     """Test the NodeCommissionResult model."""
 
+    def test_unicode_represents_result(self):
+        result = factory.make_node_commission_result()
+        self.assertEqual(
+            '%s/%s' % (result.node.system_id, result.name),
+            unicode(result))
+
     def test_can_store_data(self):
         node = factory.make_node()
         name = factory.getRandomString()
@@ -52,6 +58,27 @@
         ncr2 = factory.make_node_commission_result(node=node2, name="foo")
         self.assertEqual(ncr1.name, ncr2.name)
 
+    def test_get_data_as_html_returns_output(self):
+        output = factory.getRandomString()
+        result = factory.make_node_commission_result(
+            data=output.encode('ascii'))
+        self.assertEqual(output, result.get_data_as_html())
+
+    def test_get_data_as_html_escapes_binary(self):
+        output = b'\x00\xff'
+        result = factory.make_node_commission_result(data=output)
+        html = result.get_data_as_html()
+        self.assertIsInstance(html, unicode)
+        # The nul byte turns into the zero character.  The 0xff is an invalid
+        # character and so becomes the Unicode "replacement character" 0xfffd.
+        self.assertEqual('\x00\ufffd', html)
+
+    def test_get_data_as_html_escapes_for_html(self):
+        output = '<&>'
+        result = factory.make_node_commission_result(
+            data=output.encode('ascii'))
+        self.assertEqual('&lt;&amp;&gt;', result.get_data_as_html())
+
 
 class TestNodeCommissionResultManager(DjangoTestCase):
     """Test the manager utility for NodeCommissionResult."""