Merge lp:~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting into lp:~maas-committers/maas/trunk

[Dimiter Naydenov]

Fix bug #994887: Nodes list view now supports sorting by hostname(mac) or status columns.

This is still here for reference only and history. The same bug is fixed with a simpler solution in https://code.launchpad.net/~dimitern/maas/bug-994887-alternate-nodes-list-sorting/+merge/131177. So I'm marking this as Rejected.

-------------------

Implemented sorting for /MAAS/nodes/ list - by hostname or status (asc/desc). As advised, I pulled only what was needed from the project django-sortable and I'm using it in the views/templates. Since django-sortable is not in main archive, we cannot use it, but it's only a tiny package, so I extracted what was needed and added it to maasserver (also did some style formatting so it'll conform to our code).

There are a couple of very minor UI changes, which I was advised also to request a separate review from Huw Wilkins. The added elements are 2 up/down arrows appearing on the right of the two table columns - MAC and Status. Here are screenshots how it looks in all states:

http://people.canonical.com/~dimitern/maas-nodes-sort-none.png
http://people.canonical.com/~dimitern/maas-nodes-sort-mac-asc.png
http://people.canonical.com/~dimitern/maas-nodes-sort-mac-desc.png
http://people.canonical.com/~dimitern/maas-nodes-sort-status-asc.png
http://people.canonical.com/~dimitern/maas-nodes-sort-status-desc.png

[John A Meinel (jameinel)]

I haven't gone through this in detail, but my first concern is how this
interacts with pagination. Does this just sort the visible nodes, or does
it re do the query with a new sort order? (It is nice to nit have to reload
the page, but with >1k nodes, we won't display them all at once, and then
sorting wouldn't be very useful)

John
=:->
On Oct 19, 2012 8:31 PM, "Dimiter Naydenov" <email address hidden>
wrote:

> The proposal to merge
> lp:~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting into
> lp:maas has been updated.
>
> Description changed to:
>
> Implemented sorting for /MAAS/nodes/ list - by hostname or status
> (asc/desc). As advised, I pulled only what was needed from the project
> django-sortable and I'm using it in the views/templates. Since
> django-sortable is not in main archive, we cannot use it, but it's only a
> tiny package, so I extracted what was needed and added it to maasserver
> (also did some style formatting so it'll conform to our code).
>
> There are a couple of very minor UI changes, which I was advised also to
> request a separate review from Huw Wilkins. The added elements are 2
> up/down arrows appearing on the right of the two table columns - MAC and
> Status. Here are screenshots how it looks in all states:
>
> http://people.canonical.com/~dimitern/maas-nodes-sort-none.png
> http://people.canonical.com/~dimitern/maas-nodes-sort-mac-asc.png
> http://people.canonical.com/~dimitern/maas-nodes-sort-mac-desc.png
> http://people.canonical.com/~dimitern/maas-nodes-sort-status-asc.png
> http://people.canonical.com/~dimitern/maas-nodes-sort-status-desc.png
>
> For more details, see:
>
> https://code.launchpad.net/~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting/+merge/130600
> --
>
> https://code.launchpad.net/~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting/+merge/130600
> You are subscribed to branch lp:maas.
>

[John A Meinel (jameinel)]

I guess my specific point is, people expect inline controls like a triangle
to be cheap, but at this point to do it accurately we have to reload the
page. Should we present this in a way to make it clearer what will happen?
Would Ajax be enough to make the reload lightweight?. We should probably
test with a large dataset and see how long it takes to reload.

John
=:->
On Oct 20, 2012 7:52 AM, "John A Meinel" <email address hidden> wrote:

> I haven't gone through this in detail, but my first concern is how this
> interacts with pagination. Does this just sort the visible nodes, or does
> it re do the query with a new sort order? (It is nice to nit have to reload
> the page, but with >1k nodes, we won't display them all at once, and then
> sorting wouldn't be very useful)
>
> John
> =:->
> On Oct 19, 2012 8:31 PM, "Dimiter Naydenov" <
> <email address hidden>>
> wrote:
>
> > The proposal to merge
> > lp:~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting into
> > lp:maas has been updated.
> >
> > Description changed to:
> >
> > Implemented sorting for /MAAS/nodes/ list - by hostname or status
> > (asc/desc). As advised, I pulled only what was needed from the project
> > django-sortable and I'm using it in the views/templates. Since
> > django-sortable is not in main archive, we cannot use it, but it's only a
> > tiny package, so I extracted what was needed and added it to maasserver
> > (also did some style formatting so it'll conform to our code).
> >
> > There are a couple of very minor UI changes, which I was advised also to
> > request a separate review from Huw Wilkins. The added elements are 2
> > up/down arrows appearing on the right of the two table columns - MAC and
> > Status. Here are screenshots how it looks in all states:
> >
> > http://people.canonical.com/~dimitern/maas-nodes-sort-none.png
> > http://people.canonical.com/~dimitern/maas-nodes-sort-mac-asc.png
> > http://people.canonical.com/~dimitern/maas-nodes-sort-mac-desc.png
> > http://people.canonical.com/~dimitern/maas-nodes-sort-status-asc.png
> > http://people.canonical.com/~dimitern/maas-nodes-sort-status-desc.png
> >
> > For more details, see:
> >
> >
> https://code.launchpad.net/~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting/+merge/130600
> > --
> >
> >
> https://code.launchpad.net/~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting/+merge/130600
> > You are subscribed to branch lp:maas.
> >
>
> --
>
> https://code.launchpad.net/~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting/+merge/130600
> You are subscribed to branch lp:maas.
>

[Dimiter Naydenov (dimitern)]

It works quite well with paging - transparently, and because it modifies the queryset, it actually works for both visible and invisible nodes in the list. Paging links include the ?sort= and dir= parameters, so sorting spans pages properly. I tested this manually by setting paginate_by = 2, and then realized it'll be good to have a test for that. So I added test_node_list_paginates_while_sorted, which does just that.

Also, I noticed a small omission on my part - the tags view shows matching nodes in a list, using the same template, so I modified the included snippet to skip header paging links when not explicitly required (only /nodes/ list uses it now).

With regards to speed, I don't think it'll be any slower than sorting by -created by default, provided we have indexes on both hostname and status columns in the DB. I thought about implementing the sort using AJAX/in-line with JS, but what I did seemed like a better option.

Thanks for the feedback and I'm eager to see the review :)

[Raphaël Badin (rvb)]

I've run the tests in test_views_nodes.py and the test coverage report says that 25% of the module sortable.py is untested. This means that some tests are missing or that some of the code in sortable.py is unnecessary and thus you should consider removing it.

$ ./bin/maas test src/maasserver/tests/test_views_nodes.py --with-coverage --cover-package=maasserver.templatetags
[...]
........................................................................S...
Name Stmts Miss Cover Missing
------------------------------------------------------------------
maasserver.templatetags 0 0 100%
maasserver.templatetags.field_type 8 0 100%
maasserver.templatetags.sortable 83 21 75% 47, 51-55, 65-66, 68-69, 124-125, 133-134, 141-142, 151-152, 156-157, 161-162
------------------------------------------------------------------
TOTAL 91 21 77%
----------------------------------------------------------------------
Ran 76 tests in 23.961s

[Dimiter Naydenov (dimitern)]

As discussed on IRC, I decided it's better to have the complete functionality of sortable, if we need to use it for other views. So I added separate unit tests src/maasserver/templatetags/tests/test_sortable.py with all the needed test cases to ensure 100% coverage.

[Martin Packman (gz)]

The project much of this code comes from only has a license specified in setup.py (GPLv3, which is okay to redistribute as AGPLv3), but we probably want the copyright header in that file to read "Drew Yeaton" instead of/as well as Canonical.

A fair bit of that code wiggles my wtf-meter, is the best way of doing this really writing a custom template snippet parser (parse_tag_token... no wonder django templating is fragile), then constructing the link (SortableLinkNode.make_link) and html (SortableLinkNode.render) with string interpolation? I wouldn't trust this to, for instance, correctly escape '&' -> '&amp;', but maybe I underestimate django magic.

Unlike John I don't worry about the headers being page reloads for sorting rather than fast JS things, this way it at least combines correctly the pagination, and has predictable behaviour.

review: Abstain

[John A Meinel (jameinel)]

As long as the reload speed is reasonable, I'm happy to have it done by an extra round trip. 100ms is reasonable, 10s is not, somewhere inbetween is the cutoff :).

Hence why I was mentioning measuring it, rather than just saying 'it works fine when you have 5 nodes'.

[Huw Wilkins (huwshimi)]

The arrows look fine. To fit in with the styles they should probably be a shade of our warm grey, but I'll fix that up in a further branch.

review: Approve (ui)

=== 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-10-22 09:32:21 +0000
@@ -30,6 +30,17 @@
     border-color: #dd4814;
     cursor: pointer;
     }
+.sort-none, .sort-asc, .sort-desc {
+    background-repeat: no-repeat;
+    background-position: right 0.4em;
+    padding-right: 1.2em;
+    }
+.sort-asc {
+    background-image: url('/static/img/up_arrow_dark.png');
+    }
+.sort-desc {
+    background-image: url('/static/img/down_arrow_dark.png');
+    }
 /* ul list */
 ul.list {
     list-style: none;

=== added file 'src/maasserver/static/img/down_arrow_dark.png'
Binary files src/maasserver/static/img/down_arrow_dark.png	1970-01-01 00:00:00 +0000 and src/maasserver/static/img/down_arrow_dark.png	2012-10-22 09:32:21 +0000 differ
=== added file 'src/maasserver/static/img/up_arrow_dark.png'
Binary files src/maasserver/static/img/up_arrow_dark.png	1970-01-01 00:00:00 +0000 and src/maasserver/static/img/up_arrow_dark.png	2012-10-22 09:32:21 +0000 differ
=== modified file 'src/maasserver/templates/maasserver/node_list.html'
--- src/maasserver/templates/maasserver/node_list.html	2012-10-12 13:33:09 +0000
+++ src/maasserver/templates/maasserver/node_list.html	2012-10-22 09:32:21 +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 sortable="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-10-10 08:13:01 +0000
+++ src/maasserver/templates/maasserver/nodes_listing.html	2012-10-22 09:32:21 +0000
@@ -1,9 +1,16 @@
+{% load sortable %}
+
 {% if node_list|length %}
   <table class="list">
     <thead>
       <tr>
-        <th>MAC</th>
-        <th>Status</th>
+	{% if sortable == "true" %}
+        <th>{% sortable_link hostname "MAC" %}</th>
+        <th>{% sortable_link status "Status" %}</th>
+	{% else %}
+	<th>MAC</th>
+	<td>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-10-18 11:26:02 +0000
+++ src/maasserver/templates/maasserver/tag_view.html	2012-10-22 09:32:21 +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 sortable="false" %}
     {% include "maasserver/pagination.html" %}
   </div>
 {% endblock %}

=== added file 'src/maasserver/templatetags/sortable.py'
--- src/maasserver/templatetags/sortable.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/templatetags/sortable.py	2012-10-22 09:32:21 +0000
@@ -0,0 +1,168 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+# Most of this code is taken from django-sortable project:
+# https://github.com/drewyeaton/django-sortable
+# and formatted to fit our style guide
+
+"""Field type template tag."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+        "sortable_link",
+        "sortable_header",
+        "sortable_url",
+        "sortable_class",
+    ]
+
+from django.conf import settings
+from django import template
+
+
+register = template.Library()
+
+# CSS classes, used by the sortable_class tag
+SORT_ASC_CLASS = getattr(settings, 'SORT_ASC_CLASS', 'sort-asc')
+SORT_DESC_CLASS = getattr(settings, 'SORT_DESC_CLASS', 'sort-desc')
+SORT_NONE_CLASS = getattr(settings, 'SORT_DESC_CLASS', 'sort-none')
+
+directions = {
+    'asc': {'class': SORT_ASC_CLASS, 'inverse': 'desc'},
+    'desc': {'class': SORT_DESC_CLASS, 'inverse': 'asc'},
+    }
+
+
+def parse_tag_token(token):
+    """Parses a tag that's supposed to be in this format:
+    {% sortable_link field title %}
+    """
+    bits = [b.strip('"\'') for b in token.split_contents()]
+    if len(bits) < 2:
+        raise template.TemplateSyntaxError(
+            "anchor tag takes at least 1 argument")
+    try:
+        title = bits[2]
+    except IndexError:
+        if bits[1].startswith(('+', '-')):
+            title = bits[1][1:].capitalize()
+        else:
+            title = bits[1].capitalize()
+
+    return (bits[1].strip(), title.strip())
+
+
+class SortableLinkNode(template.Node):
+    """Build sortable link based on query params."""
+
+    def __init__(self, field_name, title):
+        if field_name.startswith('-'):
+            self.field_name = field_name[1:]
+            self.default_direction = 'desc'
+        elif field_name.startswith('+'):
+            self.field_name = field_name[1:]
+            self.default_direction = 'asc'
+        else:
+            self.field_name = field_name
+            self.default_direction = 'asc'
+
+        self.title = title
+
+    def build_link(self, context):
+        """Prepare link for rendering based on context."""
+        get_params = context['request'].GET.copy()
+
+        field_name = get_params.get('sort', None)
+        if field_name:
+            del(get_params['sort'])
+
+        direction = get_params.get('dir', None)
+        if direction:
+            del(get_params['dir'])
+        direction = direction if direction in ('asc', 'desc') else 'asc'
+
+        # if is current field, and sort isn't defined,
+        # assume asc otherwise desc
+        direction = direction or (
+            (self.field_name == field_name) and 'asc' or 'desc')
+
+        # if current field and it's sorted, make link inverse,
+        # otherwise defalut to asc
+        if self.field_name == field_name:
+            get_params['dir'] = directions[direction]['inverse']
+        else:
+            get_params['dir'] = self.default_direction
+
+        if self.field_name == field_name:
+            css_class = directions[direction]['class']
+        else:
+            css_class = SORT_NONE_CLASS
+
+        params = (
+            "&%s" % (get_params.urlencode(),)
+            if len(get_params.keys()) > 0 else '')
+        url = '%s?sort=%s%s' % (
+            context['request'].path, self.field_name, params)
+
+        return (url, css_class)
+
+    def render(self, context):
+        url, css_class = self.build_link(context)
+        return '<a href="%s" class="%s" title="%s">%s</a>' % (
+            url, css_class, self.title, self.title)
+
+
+class SortableTableHeaderNode(SortableLinkNode):
+    """Build sortable link header based on query params."""
+
+    def render(self, context):
+        url, css_class = self.build_link(context)
+        return '<th class="%s"><a href="%s" title="%s">%s</a></th>' % (
+            css_class, url, self.title, self.title)
+
+
+class SortableURLNode(SortableLinkNode):
+    """Build sortable link header based on query params."""
+
+    def render(self, context):
+        url, css_class = self.build_link(context)
+        return url
+
+
+class SortableClassNode(SortableLinkNode):
+    """Build sortable link header based on query params."""
+
+    def render(self, context):
+        url, css_class = self.build_link(context)
+        return css_class
+
+
+def sortable_link(parser, token):
+    field, title = parse_tag_token(token)
+    return SortableLinkNode(field, title)
+
+
+def sortable_header(parser, token):
+    field, title = parse_tag_token(token)
+    return SortableTableHeaderNode(field, title)
+
+
+def sortable_url(parser, token):
+    field, title = parse_tag_token(token)
+    return SortableURLNode(field, title)
+
+
+def sortable_class(parser, token):
+    field, title = parse_tag_token(token)
+    return SortableClassNode(field, title)
+
+
+sortable_link = register.tag(sortable_link)
+sortable_header = register.tag(sortable_header)
+sortable_url = register.tag(sortable_url)
+sortable_class = register.tag(sortable_class)

=== added directory 'src/maasserver/templatetags/tests'
=== added file 'src/maasserver/templatetags/tests/__init__.py'
=== added file 'src/maasserver/templatetags/tests/test_sortable.py'
--- src/maasserver/templatetags/tests/test_sortable.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/templatetags/tests/test_sortable.py	2012-10-22 09:32:21 +0000
@@ -0,0 +1,130 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Sortable template tags tests."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+
+from django.test import TestCase
+from django.test.client import RequestFactory
+from django.template import (
+    Template,
+    Context,
+    TemplateSyntaxError,
+    )
+
+
+class SortableTemplateTagsTests(TestCase):
+
+    def prepare_request(self, url, **kwargs):
+        return RequestFactory().get(
+            url,
+            data=kwargs,
+            )
+
+    def render_template(self, template, request):
+        return Template(
+            "{% load sortable %}" + template
+            ).render(Context({
+                'request': request,
+            }))
+
+    def test_sortable_link_tag_default_sort_with_extra_param(self):
+        request = self.prepare_request("some_url", param1="a")
+        out = self.render_template(
+            '{% sortable_link field "Title" %}',
+             request)
+        self.assertEqual(
+            out,
+            '<a href="some_url?sort=field&dir=asc&param1=a" '
+            'class="sort-none" title="Title">Title</a>')
+
+    def test_sortable_link_tag_with_sort_and_dir(self):
+        request = self.prepare_request(
+            "some_url",
+            sort="field",
+            dir="desc")
+        out = self.render_template(
+            '{% sortable_link field "Title" %}',
+             request)
+        self.assertEqual(
+            out,
+            '<a href="some_url?sort=field&dir=asc" '
+            'class="sort-desc" title="Title">Title</a>')
+
+    def test_sortable_link_tag_desc_sort(self):
+        request = self.prepare_request("some_url")
+        out = self.render_template(
+            '{% sortable_link -field "Title" %}',
+             request)
+        self.assertEqual(
+            out,
+            '<a href="some_url?sort=field&dir=desc" '
+            'class="sort-none" title="Title">Title</a>')
+
+    def test_sortable_link_tag_with_invalid_syntax_raises(self):
+        request = self.prepare_request("some_url")
+        self.assertRaises(
+            TemplateSyntaxError,
+            self.render_template,
+            "{% sortable_link %}", request)
+
+    def test_sortable_link_tag_without_title_capitalizes_field(self):
+        request = self.prepare_request("some_url")
+        out = self.render_template(
+            "{% sortable_link fieldname %}",
+            request)
+        self.assertEqual(
+            out,
+            '<a href="some_url?sort=fieldname&dir=asc" '
+            'class="sort-none" title="Fieldname">Fieldname</a>')
+
+        # Now test with explicit ascending order
+        out = self.render_template(
+            "{% sortable_link +fieldname %}",
+            request)
+        self.assertEqual(
+            out,
+            '<a href="some_url?sort=fieldname&dir=asc" '
+            'class="sort-none" title="Fieldname">Fieldname</a>')
+
+    def test_sortable_header_tag(self):
+        request = self.prepare_request(
+            "some_url",
+            sort="field",
+            dir="desc")
+        out = self.render_template(
+            '{% sortable_header other "Info" %}',
+            request)
+        self.assertEqual(
+            out,
+            '<th class="sort-none">'
+            '<a href="some_url?sort=other&dir=asc" '
+            'title="Info">Info</a></th>')
+
+    def test_sortable_url_tag(self):
+        request = self.prepare_request("some_url")
+        out = self.render_template(
+            '{% sortable_url field "Name" %}',
+            request)
+        self.assertEqual(
+            out,
+            "some_url?sort=field&dir=asc")
+
+    def test_sortable_class_tag(self):
+        request = self.prepare_request(
+            "some_url",
+            sort="field",
+            dir="desc")
+        out = self.render_template(
+            '<span class="{% sortable_class field %}">Name</span>',
+            request)
+        self.assertEqual(
+            out,
+            '<span class="sort-desc">Name</span>')

=== modified file 'src/maasserver/tests/test_views_nodes.py'
--- src/maasserver/tests/test_views_nodes.py	2012-10-15 17:39:31 +0000
+++ src/maasserver/tests/test_views_nodes.py	2012-10-22 09:32:21 +0000
@@ -79,6 +79,78 @@
         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 creare some nodes to have something in the list
+        for i in xrange(3):
+            factory.make_node()
+        response = self.client.get(reverse('node-list'))
+        sort_hostname = reverse('node-list') + '?sort=hostname&dir=asc'
+        sort_status = reverse('node-list') + '?sort=status&dir=asc'
+        self.assertIn(sort_hostname, get_content_links(response))
+        self.assertIn(sort_status, get_content_links(response))
+
+    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=lambda x: x.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
+        sorted_nodes = sorted(
+            nodes, key=lambda x: x.hostname, reverse=True)
+        response = self.client.get(
+            reverse('node-list') + '?sort=hostname&dir=desc')
+        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')])
+
+    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=lambda x: x.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
+        sorted_nodes = sorted(
+            nodes, key=lambda x: x.status, reverse=True)
+        response = self.client.get(
+            reverse('node-list') + '?sort=status&dir=desc')
+        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')])
+
     def test_node_list_displays_sorted_list_of_nodes(self):
         # Nodes are sorted on the node list page, newest first.
         nodes = [factory.make_node() for i in range(3)]
@@ -421,7 +493,10 @@
             {"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")
+        # Exclude header sorting links
+        node_links = filter(
+            lambda x: 'sort=' not in x,
+            document.xpath("//div[@id='nodes']/table//a/@href"))
         self.assertEqual([node2_link], node_links)
 
     def test_node_list_paginates(self):
@@ -439,15 +514,18 @@
         # Fetch first page, should link newest two nodes and page 2
         response = self.client.get(reverse('node-list'))
         page1 = fromstring(response.content)
-        self.assertEqual(node_links[:page_size], expr_node_links(page1))
+        # Exclude header sorting links
+        self.assertEqual(node_links[:page_size],
+            filter(lambda x: 'sort=' not in x, expr_node_links(page1)))
         self.assertEqual([("next", "?page=2"), ("last", "?page=3")],
             [(a.text.lower(), a.get("href"))
                 for a in expr_page_anchors(page1)])
         # Fetch second page, should link next nodes and adjacent pages
         response = self.client.get(reverse('node-list'), {"page": 2})
         page2 = fromstring(response.content)
+        # Exclude header sorting links
         self.assertEqual(node_links[page_size:page_size * 2],
-            expr_node_links(page2))
+            filter(lambda x: 'sort=' not in x, expr_node_links(page2)))
         self.assertEqual([("first", "."), ("previous", "."),
                 ("next", "?page=3"), ("last", "?page=3")],
             [(a.text.lower(), a.get("href"))
@@ -455,10 +533,15 @@
         # Fetch third page, should link oldest node and node list page
         response = self.client.get(reverse('node-list'), {"page": 3})
         page3 = fromstring(response.content)
-        self.assertEqual(node_links[page_size * 2:], expr_node_links(page3))
+        # Exclude header sorting links
+        page3_node_links = filter(
+            lambda x: 'sort=' not in x, expr_node_links(page3))
+        page3_anchors = filter(
+            lambda x: 'sort=' not in x, expr_page_anchors(page3))
+        self.assertEqual(node_links[page_size * 2:], page3_node_links)
         self.assertEqual([("first", "."), ("previous", "?page=2")],
             [(a.text.lower(), a.get("href"))
-                for a in expr_page_anchors(page3)])
+                for a in page3_anchors])
 
     def test_node_list_query_paginates(self):
         """Node list query subset is split across multiple pages with links"""
@@ -474,13 +557,35 @@
             {"query": "maas-tags=odd", "page": 3})
         document = fromstring(response.content)
         self.assertIn("5 matching nodes", document.xpath("string(//h1)"))
+        # Exclude header sorting links
         self.assertEqual([last_node_link],
-            document.xpath("//div[@id='nodes']/table//a/@href"))
+            filter(
+                lambda x: 'sort=' not in x,
+                document.xpath("//div[@id='nodes']/table//a/@href")))
         self.assertEqual([("first", "?query=maas-tags%3Dodd"),
                 ("previous", "?query=maas-tags%3Dodd&page=2")],
             [(a.text.lower(), a.get("href"))
                 for a in document.xpath("//div[@class='pagination']//a")])
 
+    def test_node_list_paginates_while_sorted(self):
+        # Set a very small page size to save creating lots of nodes
+        self.patch(nodes_views.NodeListView, 'paginate_by', 2)
+        nodes = [factory.make_node(hostname=name)
+            for name in ('zero', 'four', 'two', 'one', 'five')]
+        page1_links = [
+            reverse('node-view', args=[nodes[4].system_id]),
+            reverse('node-view', args=[nodes[1].system_id])]
+        response = self.client.get(
+            reverse('node-list') + '?sort=hostname&dir=asc')
+        document = fromstring(response.content)
+        self.assertIn("5 nodes in", document.xpath("string(//h1)"))
+        # Exclude header sorting links
+        self.assertEqual(
+            page1_links,
+            filter(
+                lambda x: 'sort=' not in x,
+                document.xpath("//div[@id='nodes']/table//a/@href")))
+
 
 class NodePreseedViewTest(LoggedInTestCase):
 

=== added directory 'src/maasserver/utils/sortable'
=== added file 'src/maasserver/utils/sortable/__init__.py'
--- src/maasserver/utils/sortable/__init__.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/utils/sortable/__init__.py	2012-10-22 09:32:21 +0000
@@ -0,0 +1,25 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+# Most of this code is taken from django-sortable project:
+# https://github.com/drewyeaton/django-sortable
+# and formatted to fit our style guide
+
+"""Sortable - helper to make sorting table columns in templates easier."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'SortableInvalidObjectsException',
+    'Sortable',
+    ]
+
+from maasserver.utils.sortable.sortable import (
+    SortableInvalidObjectsException,
+    Sortable,
+    )

=== added file 'src/maasserver/utils/sortable/sortable.py'
--- src/maasserver/utils/sortable/sortable.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/utils/sortable/sortable.py	2012-10-22 09:32:21 +0000
@@ -0,0 +1,161 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+# Most of this code is taken from django-sortable project:
+# https://github.com/drewyeaton/django-sortable
+# and formatted to fit our style guide
+
+"""Sortable - helper to make sorting table columns in templates easier."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'SortableInvalidObjectsException',
+    'Sortable',
+    ]
+
+from operator import itemgetter, attrgetter
+
+
+class SortableInvalidObjectsException(Exception):
+    pass
+
+
+class Sortable(object):
+    """Takes a QuerySet and allows sorting by fields."""
+
+    def __init__(self, objects, fields):
+        super(Sortable, self).__init__()
+        self.objects = objects
+        self.fields = None
+        self.set_normalized_fields(fields)
+
+    def set_normalized_fields(self, fields):
+        """Takes name-to-field mapping tuple, normalizes it,
+        and sets field.
+        """
+        if fields is None:
+            return
+
+        field_list = []
+        for f in fields:
+            if isinstance(f, basestring):
+                field_list.append((f, (f,)))
+            elif isinstance(f[1], basestring):
+                field_list.append((f[0], (f[1],)))
+            else:
+                field_list.append(f)
+        self.fields = dict(field_list)
+
+    def sorted(self, field_name, direction='asc'):
+        """Returns QuerySet with order_by applied
+        or sorted list of dictionary.
+        """
+
+        if self.fields:
+            try:
+                fields = self.fields[field_name]
+            except KeyError:
+                return self.objects
+        else:
+            fields = (field_name,)
+
+        if direction not in ('asc', 'desc'):
+            return self.objects
+
+        fields = Sortable.prepare_fields(fields, direction)
+
+        if hasattr(self.objects, 'order_by'):
+            result = self.objects.order_by(*fields)
+        elif isinstance(self.objects, (list, tuple)):
+            if len(self.objects) < 2:
+                return self.objects
+
+            comparers = []
+            if isinstance(self.objects[0], dict):
+                getter = itemgetter
+            else:
+                getter = attrgetter
+
+            for f in fields:
+                field = f[1:] if f.startswith('-') else f
+                comparers.append((getter(field), 1 if field == f else -1))
+
+            def comparer(left, right):
+                for func, polarity in comparers:
+                    result = cmp(func(left), func(right))
+                    return 0 if not result else polarity * result
+
+            result = sorted(self.objects, cmp=comparer)
+        else:
+            raise SortableInvalidObjectsException(
+                'An object of this type can not be sorted.')
+
+        return result
+
+    def sql_predicate(self, field_name, direction='asc', default=None):
+        """Returns a predicate for use in a SQL ORDER BY clause."""
+
+        if self.fields:
+            try:
+                fields = self.fields[field_name]
+            except KeyError:
+                fields = default
+        else:
+            fields = field_name
+
+        if direction not in ('asc', 'desc'):
+            fields = default
+
+        fields = Sortable.prepare_fields(
+            fields, direction, sql_predicate=True)
+        return ', '.join(fields)
+
+    @staticmethod
+    def prepare_fields(fields, direction, sql_predicate=False):
+        """Given a list or tuple of fields and direction,
+        return a list of fields with their appropriate
+        order_by direction applied.
+
+        >>> fields = ['++one', '--two', '+three', 'four', '-five']
+        >>> Sortable.prepare_fields(fields, 'asc')
+        ['one', '-two', 'three', 'four', '-five']
+        >>> Sortable.prepare_fields(fields, 'desc')
+        ['one', '-two', '-three', '-four', 'five']
+        >>> Sortable.prepare_fields(fields, 'not_asc_or_desc')
+        ['one', '-two', 'three', 'four', '-five']
+        >>> Sortable.prepare_fields(fields, 'desc', True)
+        ['one ASC', 'two DESC', 'three DESC', 'four DESC', 'five ASC']
+        """
+
+        if direction not in ('asc', 'desc'):
+            direction = 'asc'
+
+        fields = list(fields)
+        for i, field in enumerate(fields):
+            if field.startswith('--'):
+                fields[i] = field[1:]
+            elif field.startswith('++'):
+                fields[i] = field[2:]
+            elif field.startswith('-'):
+                fields[i] = (direction == 'asc' and '-' or '') + field[1:]
+            else:
+                field = field[1:] if field.startswith('+') else field
+                fields[i] = (direction == 'desc' and '-' or '') + field
+
+        if not sql_predicate:
+            return fields
+
+        # determine sql predicates from list
+        fields = list(fields)
+        for i, field in enumerate(fields):
+            if field.startswith('-'):
+                fields[i] = '%s DESC' % (field[1:],)
+            else:
+                fields[i] = '%s ASC' % (field,)
+        return fields

=== modified file 'src/maasserver/views/nodes.py'
--- src/maasserver/views/nodes.py	2012-10-19 13:55:51 +0000
+++ src/maasserver/views/nodes.py	2012-10-22 09:32:21 +0000
@@ -67,6 +67,7 @@
     HelpfulDeleteView,
     PaginatedListView,
     )
+from maasserver.utils.sortable import Sortable
 
 
 def get_longpoll_context():
@@ -110,6 +111,8 @@
     def get(self, request, *args, **kwargs):
         self.query = request.GET.get("query")
         self.query_error = None
+        self.sort_field = request.GET.get('sort', '')
+        self.sort_direction = request.GET.get('dir', 'asc')
         return super(NodeListView, self).get(request, *args, **kwargs)
 
     def get_queryset(self):
@@ -117,6 +120,9 @@
         nodes = Node.objects.get_nodes(
             user=self.request.user, prefetch_mac=True,
             perm=NODE_PERMISSION.VIEW,).order_by('-created')
+        sortable = Sortable(nodes, ('hostname', 'status'))
+        nodes = sortable.sorted(self.sort_field, self.sort_direction)
+
         if self.query:
             try:
                 return constrain_nodes(nodes, _parse_constraints(self.query))