Merge lp:~julian-edwards/maas/backport-r1294-1299 into lp:maas/1.2

[Julian Edwards]

Fix bug #994887: Nodes list view now supports sorting by hostname(mac) or status columns. (backport r1294 and r1299 from trunk)

[Julian Edwards (julian-edwards)]

review: Approve

=== modified file 'src/maasserver/static/css/components/table_list.css'
--- src/maasserver/static/css/components/table_list.css	2012-04-05 06:23:59 +0000
+++ src/maasserver/static/css/components/table_list.css	2012-12-03 07:15:29 +0000
@@ -30,6 +30,19 @@
     border-color: #dd4814;
     cursor: pointer;
     }
+.sort-none,
+.sort-asc,
+.sort-desc {
+    background-repeat: no-repeat;
+    background-position: right center;
+    padding-right: 16px;
+    }
+.sort-asc {
+    background-image: url('../?img/sort_ascending.png');
+    }
+.sort-desc {
+    background-image: url('../?img/sort_descending.png');
+    }
 /* ul list */
 ul.list {
     list-style: none;
 
=== added file 'src/maasserver/static/img/sort_ascending.png'
 Binary files src/maasserver/static/img/sort_ascending.png	1970-01-01 00:00:00 +0000 and src/maasserver/static/img/sort_ascending.png	2012-12-03 07:15:29 +0000 differ
=== added file 'src/maasserver/static/img/sort_descending.png'
 Binary files src/maasserver/static/img/sort_descending.png	1970-01-01 00:00:00 +0000 and src/maasserver/static/img/sort_descending.png	2012-12-03 07:15:29 +0000 differ
=== modified file 'src/maasserver/templates/maasserver/node_list.html'
--- src/maasserver/templates/maasserver/node_list.html	2012-10-17 05:20:13 +0000
+++ src/maasserver/templates/maasserver/node_list.html	2012-12-03 07:15:29 +0000
@@ -38,7 +38,7 @@
     {% if input_query_error %}
     <p class="form-errors">{{input_query_error}}</p>
     {% endif %}
-    {% include "maasserver/nodes_listing.html" %}
+    {% include "maasserver/nodes_listing.html" with sorting="true" %}
     {% include "maasserver/pagination.html" %}
     <a id="addnode" href="#" class="button right space-top">+ Add node</a>
     <div class="clear"></div>
 
=== modified file 'src/maasserver/templates/maasserver/nodes_listing.html'
--- src/maasserver/templates/maasserver/nodes_listing.html	2012-11-16 13:30:09 +0000
+++ src/maasserver/templates/maasserver/nodes_listing.html	2012-12-03 07:15:29 +0000
@@ -2,9 +2,25 @@
   <table class="list">
     <thead>
       <tr>
+      {% if sorting == "true" %}
+      <th><a href="{{ sort_links.hostname }}"
+             class="{{ sort_classes.hostname }}">
+        <acronym title="Fully Qualified Domain Name">FQDN</acronym>
+      </a></th>
+      <th>
+        <acronym
+           title="Media Access Control addresses">MAC</acronym>
+      </th>
+      <th>
+        <a href="{{ sort_links.status }}"
+	       class="{{ sort_classes.status }}">Status</a>
+      </th>
+      {% else %}
         <th><acronym title="Fully Qualified Domain Name">FQDN</acronym></th>
         <th><acronym
           title="Media Access Control addresses">MAC</acronym></th>
+        <th>Status</th>
+      {% endif %}
       </tr>
     </thead>
     {% for node in node_list %}
 
=== modified file 'src/maasserver/templates/maasserver/tag_view.html'
--- src/maasserver/templates/maasserver/tag_view.html	2012-11-07 11:32:16 +0000
+++ src/maasserver/templates/maasserver/tag_view.html	2012-12-03 07:15:29 +0000
@@ -34,7 +34,7 @@
   </ul>
   <div id="nodes" class="block first pad-top">
     <h2 class="block first line-top pad-top">{{ paginator.count }} Node{{ paginator.count|pluralize }}</h2>
-    {% include "maasserver/nodes_listing.html" %}
+    {% include "maasserver/nodes_listing.html" with sorting="false" %}
     {% include "maasserver/pagination.html" %}
   </div>
 {% endblock %}
 
=== modified file 'src/maasserver/tests/test_views_nodes.py'
--- src/maasserver/tests/test_views_nodes.py	2012-11-29 13:22:20 +0000
+++ src/maasserver/tests/test_views_nodes.py	2012-12-03 07:15:29 +0000
@@ -13,7 +13,12 @@
 __all__ = []
 
 import httplib
+from operator import attrgetter
 from unittest import skip
+from urlparse import (
+    parse_qsl,
+    urlparse,
+    )
 
 from django.conf import settings
 from django.core.urlresolvers import reverse
@@ -81,6 +86,15 @@
         enlist_preseed_link = reverse('enlist-preseed-view')
         self.assertIn(enlist_preseed_link, get_content_links(response))
 
+    def test_node_list_contains_column_sort_links(self):
+        # Just create a node to have something in the list
+        factory.make_node()
+        response = self.client.get(reverse('node-list'))
+        sort_hostname = '?sort=hostname&dir=asc'
+        sort_status = '?sort=status&dir=asc'
+        self.assertIn(sort_hostname, get_content_links(response))
+        self.assertIn(sort_status, get_content_links(response))
+
     def test_node_list_lists_nodes_from_different_nodegroups(self):
         # Bug 1084443.
         nodegroup1 = factory.make_node_group()
@@ -91,6 +105,99 @@
         response = self.client.get(reverse('node-list'))
         self.assertEqual(httplib.OK, response.status_code)
 
+    def test_node_list_sorts_by_hostname(self):
+        names = ['zero', 'one', 'five']
+        nodes = [factory.make_node(hostname=n) for n in names]
+
+        # First check the ascending sort order
+        sorted_nodes = sorted(nodes, key=attrgetter('hostname'))
+        response = self.client.get(
+            reverse('node-list'), {
+                'sort': 'hostname',
+                'dir': 'asc'})
+        node_links = [
+             reverse('node-view', args=[node.system_id])
+             for node in sorted_nodes]
+        self.assertEqual(
+            node_links,
+            [link for link in get_content_links(response)
+                if link.startswith('/nodes/node')])
+
+        # Now check the reverse order
+        node_links = list(reversed(node_links))
+        response = self.client.get(
+            reverse('node-list'), {
+                'sort': 'hostname',
+                'dir': 'desc'})
+        self.assertEqual(
+            node_links,
+            [link for link in get_content_links(response)
+                if link.startswith('/nodes/node')])
+
+    def test_node_list_sorts_by_status(self):
+        statuses = {
+            NODE_STATUS.READY,
+            NODE_STATUS.DECLARED,
+            NODE_STATUS.FAILED_TESTS,
+            }
+        nodes = [factory.make_node(status=s) for s in statuses]
+
+        # First check the ascending sort order
+        sorted_nodes = sorted(nodes, key=attrgetter('status'))
+        response = self.client.get(
+            reverse('node-list'), {
+                'sort': 'status',
+                'dir': 'asc'})
+        node_links = [
+             reverse('node-view', args=[node.system_id])
+             for node in sorted_nodes]
+        self.assertEqual(
+            node_links,
+            [link for link in get_content_links(response)
+                if link.startswith('/nodes/node')])
+
+        # Now check the reverse order
+        node_links = list(reversed(node_links))
+        response = self.client.get(
+            reverse('node-list'), {
+                'sort': 'status',
+                'dir': 'desc'})
+        self.assertEqual(
+            node_links,
+            [link for link in get_content_links(response)
+                if link.startswith('/nodes/node')])
+
+    def test_node_list_sort_preserves_other_params(self):
+        # Set a very small page size to save creating lots of nodes
+        page_size = 2
+        self.patch(nodes_views.NodeListView, 'paginate_by', page_size)
+
+        nodes = []
+        tag = factory.make_tag('shiny')
+        for name in ('bbb', 'ccc', 'ddd', 'aaa'):
+            node = factory.make_node(hostname=name)
+            node.tags = [tag]
+            nodes.append(node)
+
+        params = {
+                'sort': 'hostname',
+                'dir': 'asc',
+                'page': '1',
+                'query': 'maas-tags=shiny'}
+        response = self.client.get(reverse('node-list'), params)
+        document = fromstring(response.content)
+        header_links = document.xpath("//div[@id='nodes']/table//th/a/@href")
+        fields = iter(('hostname', 'status'))
+        field_dirs = iter(('desc', 'asc'))
+        for link in header_links:
+            self.assertThat(
+                parse_qsl(urlparse(link).query),
+                ContainsAll([
+                    ('page', '1'),
+                    ('query', 'maas-tags=shiny'),
+                    ('sort', next(fields)),
+                    ('dir', next(field_dirs))]))
+
     def test_node_list_displays_fqdn_dns_not_managed(self):
         nodes = [factory.make_node() for i in range(3)]
         response = self.client.get(reverse('node-list'))
@@ -448,7 +555,8 @@
             {"query": "maas-tags=shiny cpu=2"})
         node2_link = reverse('node-view', args=[node2.system_id])
         document = fromstring(response.content)
-        node_links = document.xpath("//div[@id='nodes']/table//a/@href")
+        node_links = document.xpath(
+            "//div[@id='nodes']/table//td/a/@href")
         self.assertEqual([node2_link], node_links)
 
     def test_node_list_paginates(self):
@@ -461,7 +569,7 @@
         # Order node links with newest first as the view is expected to
         node_links = [reverse('node-view', args=[node.system_id])
             for node in reversed(nodes)]
-        expr_node_links = XPath("//div[@id='nodes']/table//a/@href")
+        expr_node_links = XPath("//div[@id='nodes']/table//td/a/@href")
         expr_page_anchors = XPath("//div[@class='pagination']//a")
         # Fetch first page, should link newest two nodes and page 2
         response = self.client.get(reverse('node-list'))
@@ -473,7 +581,8 @@
         # Fetch second page, should link next nodes and adjacent pages
         response = self.client.get(reverse('node-list'), {"page": 2})
         page2 = fromstring(response.content)
-        self.assertEqual(node_links[page_size:page_size * 2],
+        self.assertEqual(
+            node_links[page_size:page_size * 2],
             expr_node_links(page2))
         self.assertEqual([("first", "."), ("previous", "."),
                 ("next", "?page=3"), ("last", "?page=3")],
@@ -501,8 +610,9 @@
             {"query": "maas-tags=odd", "page": 3})
         document = fromstring(response.content)
         self.assertIn("5 matching nodes", document.xpath("string(//h1)"))
-        self.assertEqual([last_node_link],
-            document.xpath("//div[@id='nodes']/table//a/@href"))
+        self.assertEqual(
+            [last_node_link],
+            document.xpath("//div[@id='nodes']/table//td/a/@href"))
         self.assertEqual([("first", "?query=maas-tags%3Dodd"),
                 ("previous", "?query=maas-tags%3Dodd&page=2")],
             [(a.text.lower(), a.get("href"))
 
=== modified file 'src/maasserver/views/nodes.py'
--- src/maasserver/views/nodes.py	2012-11-29 13:22:20 +0000
+++ src/maasserver/views/nodes.py	2012-12-03 07:15:29 +0000
@@ -110,13 +110,26 @@
     def get(self, request, *args, **kwargs):
         self.query = request.GET.get("query")
         self.query_error = None
+        self.sort_by = request.GET.get("sort")
+        self.sort_dir = request.GET.get("dir")
+
         return super(NodeListView, self).get(request, *args, **kwargs)
 
     def get_queryset(self):
-        # Return node list sorted, newest first.
+        # Default sort - newest first, unless sorting params are
+        # present. In addition, to ensure order consistency, when
+        # sorting by non-unique fields (like status), we always
+        # sort by the unique creation date as well
+        if self.sort_by is not None:
+            prefix = '-' if self.sort_dir == 'desc' else ''
+            order_by = (prefix + self.sort_by, '-created')
+        else:
+            order_by = ('-created', )
+
+        # Return the sorted node list
         nodes = Node.objects.get_nodes(
             user=self.request.user, prefetch_mac=True,
-            perm=NODE_PERMISSION.VIEW,).order_by('-created')
+            perm=NODE_PERMISSION.VIEW,).order_by(*order_by)
         if self.query:
             try:
                 return constrain_nodes(nodes, _parse_constraints(self.query))
@@ -127,11 +140,39 @@
         nodes = nodes.prefetch_related('nodegroup__nodegroupinterface_set')
         return nodes
 
+    def _prepare_sort_links(self):
+        """Returns 2 dicts, with sort fields as keys and
+        links and CSS classes for the that field.
+        """
+
+        fields = ('hostname', 'status')
+        # Build relative URLs for the links, just with the params
+        links = {field: '?' for field in fields}
+        classes = {field: 'sort-none' for field in fields}
+
+        params = self.request.GET.copy()
+        reverse_dir = 'asc' if self.sort_dir == 'desc' else 'desc'
+
+        for field in fields:
+            params['sort'] = field
+            if field == self.sort_by:
+                params['dir'] = reverse_dir
+                classes[field] = 'sort-%s' % self.sort_dir
+            else:
+                params['dir'] = 'asc'
+
+            links[field] += params.urlencode()
+
+        return links, classes
+
     def get_context_data(self, **kwargs):
         context = super(NodeListView, self).get_context_data(**kwargs)
         context.update(get_longpoll_context())
         context["input_query"] = self.query
         context["input_query_error"] = self.query_error
+        links, classes = self._prepare_sort_links()
+        context["sort_links"] = links
+        context["sort_classes"] = classes
         return context