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

[Jeroen T. Vermeulen]

Tweaks for the commissioning-results UI, as requested in previous review: indicate whether nodes listing is being filtered by node (and which), make leftmost column in listing (rather than rightmost) link to the details page, and make the "time" and "script result" fields on the details page less verbose.

These cover the remaining review notes from the original review for this UI: https://code.launchpad.net/~jtv/maas/show-commissioning-results/+merge/206661

We have functions similar to normalise_whitespace elsewhere. I figured it wasn't _quite_ worth centralising, given that "normalise" is a bit vague and only takes on meaning in the context of the module, but it's getting awfully close to that point where we want something reusable.

Jeroen

[Gavin Panella (allenap)]

Looks good. I have lots of comments, but they're mostly around the UI,
and not your changes. Pick out those ones that you want to fix; the rest
is just my commentary on the feature.

[1]

From IRC:

<jtv> allenap: if you want to play with the new feature, edit
  src/maasserver/views/nodes.py and change ENABLE_NODECOMMISSIONRESULTS
  to be True.

Why are we using feature flags in trunk?

[2]

+              {% if result_item.script_result == 0 %}
+              <span>OK: {{ result_item.script_result }}</span>
+              {% else %}

I know this probably comes under "systemtic UI review", but I don't
think this needs to include the return code. Just "OK" is enough.

+              <span class="warning">
+                <img src="{{ STATIC_URL }}img/warning.png" title="Failed" />
+                FAILED: {{ result_item.script_result }}
+              </span>
+              {% endif %}

Likewise, this doesn't need the return code (it ought to be on the
details page though).

[3]

On the details page, instead of having the 2-column thing at the top,
which takes up a lot of space without telling us much, how about
integrating it into the output:

  <breadcrumbs>
    Nodes > moon > Commissioning results > 00-maas-01-lshw.out
  </breadcrumbs>

  <heading>
    FAILED
  </heading>

  <output>
    <italics>
      Recorded 12:34 on February 17th, 2014.
    </italics>
    Error while installing lldp: Network unreachable.
    <italics>
      Exited 1
    </italics>
  </output>

Maybe that's no good either. I just know that I dislike that 2-column
layout; it's too sparse and light on useful information.

[4]

On http://localhost:5240/commissioning-results/?node=... I expected to
be able to click on the "/!\ FAILED" bit to see the logs.  If I had
wanted to see the logs from an "OK" result I would have wanted to click
in the same column; it is called "Script result" after all.

[4.1]

Same goes for http://localhost:5240/nodes/; I want to be able to click
the "Failed tests" status line or the warning triangle to see what's
wrong.

[4.2]

The URL http://localhost:5240/commissioning-results/?node=... is
slightly disorientating. It would be good to have a view under
/nodes/$id/ to show a page with that node's own results.

[5]

I'm also not sure I care what file it wrote to:
00-maas-02-virtuality.out means little to me, and I know the
implementation.

[6]

I'm also not sure the Node column is useful on that page. I see that
it's a general commissioning results page, narrowed to that particular
node, but I got there by finding "moon" then clicking "3 output files".
To be told that it's for node-29d7ad70-4671-11e1-93b8-00225f89f211 is
confusing. Once I understood, it was superfluous.

I guess it needs to be there when showing more than just a single node's
results, but it should then show the node's given name, rather than its
system-generated name.

[7]

On http://localhost:5240/nodes/.../view/ the "Commissioning output" cell
should probably show if commissioning failed too, but perhaps that's
coming later.

[8]

It's not a change you made, but the <pre> block for the output has
excess whitespace within it. <pre> is so anal that it's necessary to
foreg...

review: Approve

[Julian Edwards (julian-edwards)]

On Monday 17 Feb 2014 15:59:06 you wrote:
> <jtv> allenap: if you want to play with the new feature, edit
> src/maasserver/views/nodes.py and change ENABLE_NODECOMMISSIONRESULTS
> to be True.
>
> Why are we using feature flags in trunk?

Let's get rid of the flag. The feature will be useful to people, even if it's
not polished to the shine of Gavin's head. We'll also get early feedback from
users before release time.

cheers.

[Jeroen T. Vermeulen (jtv)]

> Looks good. I have lots of comments, but they're mostly around the UI,
> and not your changes. Pick out those ones that you want to fix; the rest
> is just my commentary on the feature.

Thanks for going through it so thoroughly! I made lots of changes in response, and they brought me to the point where I'm confident enough to drop the feature flag.

> [1]
>
> From IRC:
>
> <jtv> allenap: if you want to play with the new feature, edit
>  src/maasserver/views/nodes.py and change ENABLE_NODECOMMISSIONRESULTS
>  to be True.
>
> Why are we using feature flags in trunk?

Because the change would have been much more contentious otherwise. It was a mad-dash personal initiative, not a carefully planned feature. Planned as a feature it would have taken longer, and therefore, been lower on the priority list and impossible to fit in before the sprint. As a mad dash, it was at risk of bogging down in design/review changes, and we might still have ended up getting something great later instead of something good now. The feature flag made it possible to land the initial branch and make changes later.

> [2]
>
> +              {% if result_item.script_result == 0 %}
> +              <span>OK: {{ result_item.script_result }}</span>
> +              {% else %}
>
> I know this probably comes under "systemtic UI review", but I don't
> think this needs to include the return code. Just "OK" is enough.

Exactly the sort of feedback I needed. Done.

> +              <span class="warning">
> +                <img src="{{ STATIC_URL }}img/warning.png" title="Failed" />
> +                FAILED: {{ result_item.script_result }}
> +              </span>
> +              {% endif %}
>
> Likewise, this doesn't need the return code (it ought to be on the
> details page though).

Also done.

> [3]
>
> On the details page, instead of having the 2-column thing at the top,
> which takes up a lot of space without telling us much, how about
> integrating it into the output:
>
>  <breadcrumbs>
>    Nodes > moon > Commissioning results > 00-maas-01-lshw.out
>  </breadcrumbs>
>
>  <heading>
>    FAILED
>  </heading>
>
>  <output>
>    <italics>
>      Recorded 12:34 on February 17th, 2014.
>    </italics>
>    Error while installing lldp: Network unreachable.
>    <italics>
>      Exited 1
>    </italics>
>  </output>
>
> Maybe that's no good either. I just know that I dislike that 2-column
> layout; it's too sparse and light on useful information.

I agree, but it's what we had available... Maybe we can have a proper redesign for this sometime after the sprint. It'd be nice if it didn't look quite so much like a Node page. For now though I hope this will remain a fairly obscure page.

> [4]
>
> On http://localhost:5240/commissioning-results/?node=... I expected to
> be able to click on the "/!\ FAILED" bit to see the logs.  If I had
> wanted to see the logs from an "OK" result I would have wanted to click
> in the same column; it is called "Script result" after all.

Ironically, that's what I had in my original branch. But Raphaël preferred the link on the left-hand column, so I changed it. And now I have a solution that I hope will ...

=== modified file 'src/maasserver/templates/metadataserver/nodecommissionresult.html'
--- src/maasserver/templates/metadataserver/nodecommissionresult.html	2014-02-17 07:34:05 +0000
+++ src/maasserver/templates/metadataserver/nodecommissionresult.html	2014-02-18 04:37:34 +0000
@@ -19,19 +19,21 @@
       </span>
     </li>
     <li class="block size7 first" id="time">
-      <h4>Time</h4>
-      <span>Result was registered at {{ object.created }}</span>
+      <h4>Result registered at</h4>
+      <span>{{ object.created }}</span>
     </li>
     <li class="block size3" id="script-result">
-      <h4>Return value</h4>
-      <span>Script returned {{ object.script_result }}</span>
+      <h4>Script return value</h4>
+      <span>{{ object.script_result }}</span>
     </li>
 
     <li class="block first separate" id="output">
+      {% if object.data %}
       <h2>Output</h2>
-      <pre>
+<pre>
 {{ object.get_data_as_html }}
-      </pre>
+</pre>
+      {% endif %}
     </li>
   </ul>
 

=== modified file 'src/maasserver/templates/metadataserver/nodecommissionresult_list.html'
--- src/maasserver/templates/metadataserver/nodecommissionresult_list.html	2014-02-17 04:25:33 +0000
+++ src/maasserver/templates/metadataserver/nodecommissionresult_list.html	2014-02-18 04:37:34 +0000
@@ -1,6 +1,6 @@
 {% extends "maasserver/base.html" %}
 
-{% block nav-active-zone-list %}active{% endblock %}
+{% block nav-active-result-list %}active{% endblock %}
 {% block title %}Node commissioning results{% endblock %}
 {% block page-title %}
 {{ paginator.count }} result{{ paginator.count|pluralize }}
@@ -12,41 +12,52 @@
 
 {% block content %}
     <div id="results">
-      <h2>Commissioning results</h2>
+      <h2 id="header">
+        Commissioning results
+        {% if nodes_filter %}
+        for {{ nodes_filter }}
+        {% endif %}
+      </h2>
       {% if paginator.count == 0 %}
       <p>No matching commissioning results.</p>
       {% else %}
       <table class="list">
         <thead>
           <tr>
-            <th>Time</th>
+            <th>Script result</th>
+            <th>Output file</th>
+            <th>Registered at</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>
+            <!-- Script result -->
+            <td>
+              <a href="{% url 'nodecommissionresult-view' result_item.id %}">
+                {% if result_item.script_result == 0 %}
+                <span>OK</span>
+                {% else %}
+                <span class="warning">
+                  <img src="{{ STATIC_URL }}img/warning.png" title="Failed" />
+                  FAILED
+                </span>
+                {% endif %}
+              </a>
+            </td>
+            <!-- Output file -->
+            <td>
+                {{ result_item.name }}
+            </td>
+            <!-- Registered at -->
+            <td>
+              {{ result_item.created }}
+            </td>
+            <!-- Node -->
             <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 %}
+                {{ result_item.node.hostname }}
               </a>
             </td>
           </tr>

=== modified file 'src/maasserver/views/nodecommissionresult.py'
--- src/maasserver/views/nodecommissionresult.py	2014-02-17 12:28:55 +0000
+++ src/maasserver/views/nodecommissionresult.py	2014-02-18 04:37:34 +0000
@@ -18,6 +18,7 @@
 
 from django.shortcuts import get_object_or_404
 from django.views.generic import DetailView
+from maasserver.models import Node
 from maasserver.views import PaginatedListView
 from metadataserver.models import NodeCommissionResult
 
@@ -27,9 +28,22 @@
     template_name = 'maasserver/nodecommissionresult-list.html'
     context_object_name = 'results_list'
 
+    def get_filter_system_ids(self):
+        """Return the list of nodes that were selected for filtering."""
+        return self.request.GET.getlist('node')
+
+    def get_context_data(self, **kwargs):
+        context = super(NodeCommissionResultListView, self).get_context_data()
+        system_ids = self.get_filter_system_ids()
+        if system_ids is not None and len(system_ids) > 0:
+            nodes = Node.objects.filter(system_id__in=system_ids)
+            context['nodes_filter'] = ', '.join(
+                sorted(node.hostname for node in nodes))
+        return context
+
     def get_queryset(self):
         results = NodeCommissionResult.objects.all()
-        system_ids = self.request.GET.getlist('node')
+        system_ids = self.get_filter_system_ids()
         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')

=== modified file 'src/maasserver/views/nodes.py'
--- src/maasserver/views/nodes.py	2014-02-13 10:29:27 +0000
+++ src/maasserver/views/nodes.py	2014-02-18 04:37:34 +0000
@@ -400,11 +400,6 @@
 """)
 
 
-# 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.
@@ -447,10 +442,7 @@
             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
+        results = NodeCommissionResult.objects.filter(node=node).count()
         context['nodecommissionresults'] = results
 
         return context

=== modified file 'src/maasserver/views/tests/test_nodecommissionresults.py'
--- src/maasserver/views/tests/test_nodecommissionresults.py	2014-02-17 12:28:55 +0000
+++ src/maasserver/views/tests/test_nodecommissionresults.py	2014-02-18 04:37:34 +0000
@@ -30,6 +30,11 @@
 from testtools.matchers import HasLength
 
 
+def normalise_whitespace(text):
+    """Replace any whitespace sequence with just a single space."""
+    return ' '.join(text.split())
+
+
 class TestNodeCommissionResultView(MAASServerTestCase):
 
     def request_page(self, result):
@@ -72,7 +77,7 @@
             result.node.hostname,
             self.extract_field(doc, 'node'))
         self.assertEqual(
-            "Script returned %d" % result.script_result,
+            "%d" % result.script_result,
             self.extract_field(doc, 'script-result'))
         self.assertEqual(result.data, self.extract_field(doc, 'output', 'pre'))
 
@@ -92,6 +97,13 @@
         doc = self.request_page(result)
         self.assertEqual('A\ufffdB', self.extract_field(doc, 'output', 'pre'))
 
+    def test_hides_output_if_empty(self):
+        self.client_log_in(as_admin=True)
+        result = factory.make_node_commission_result(data=b'')
+        doc = self.request_page(result)
+        field = get_one(doc.cssselect('#output'))
+        self.assertEqual('', field.text_content().strip())
+
 
 class TestNodeCommissionResultListView(MAASServerTestCase):
 
@@ -143,26 +155,23 @@
         result_row = get_one(content.cssselect('.result'))
         fields = result_row.cssselect('td')
 
-        [time, node, output_file, script_result] = fields
+        [script_result, output_file, time, node] = fields
+        self.assertEqual('OK', script_result.text_content().strip())
         self.assertIn('%d' % result.created.year, time.text_content())
-        self.assertEqual(result.node.system_id, node.text_content().strip())
+        self.assertEqual(result.node.hostname, 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))
+        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())
+        [script_result, _, _, _] = fields
+        self.assertEqual("FAILED", script_result.text_content().strip())
         self.assertNotEqual([], script_result.find_class('warning'))
 
     def test_links_to_result(self):
@@ -172,7 +181,7 @@
         content = self.request_page()
         result_row = get_one(content.cssselect('.result'))
         fields = result_row.cssselect('td')
-        [_, _, _, script_result] = fields
+        [script_result, _, _, _] = fields
         link = get_one(script_result.cssselect('a'))
         self.assertEqual(
             reverse('nodecommissionresult-view', args=[result.id]),
@@ -241,8 +250,35 @@
         self.assertThat(rows, HasLength(len(matching_results)))
         matching_names = set()
         for row in rows:
-            [_, _, name, _] = row.cssselect('td')
+            [_, name, _, _] = row.cssselect('td')
             matching_names.add(name.text_content().strip())
         self.assertEqual(
             {result.name for result in matching_results},
             matching_names)
+
+    def test_does_not_show_node_if_not_filtering_by_node(self):
+        self.client_log_in(as_admin=True)
+        doc = self.request_page()
+        header = get_one(doc.cssselect('#header'))
+        self.assertEqual(
+            "Commissioning results",
+            normalise_whitespace(header.text_content()))
+
+    def test_shows_node_if_filtering_by_node(self):
+        self.client_log_in(as_admin=True)
+        node = factory.make_node()
+        doc = self.request_page(nodes=[node])
+        header = get_one(doc.cssselect('#header'))
+        self.assertEqual(
+            "Commissioning results for %s" % node.hostname,
+            normalise_whitespace(header.text_content()))
+
+    def test_shows_nodes_if_filtering_by_multiple_nodes(self):
+        self.client_log_in(as_admin=True)
+        names = [factory.make_name('node').lower() for _ in range(2)]
+        nodes = [factory.make_node(hostname=name) for name in names]
+        doc = self.request_page(nodes=nodes)
+        header = get_one(doc.cssselect('#header'))
+        self.assertEqual(
+            "Commissioning results for %s" % ', '.join(sorted(names)),
+            normalise_whitespace(header.text_content()))

=== modified file 'src/maasserver/views/tests/test_nodes.py'
--- src/maasserver/views/tests/test_nodes.py	2014-02-13 10:53:27 +0000
+++ src/maasserver/views/tests/test_nodes.py	2014-02-18 04:37:34 +0000
@@ -951,11 +951,6 @@
 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.
 
@@ -1000,7 +995,6 @@
         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)
@@ -1010,26 +1004,17 @@
             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)
@@ -1039,7 +1024,6 @@
             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)