launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #13572
lp:~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting into lp:maas
Dimiter Naydenov has proposed merging lp:~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting into lp:maas.
Commit message:
Fix bug #994887: Nodes list view now supports sorting by hostname(mac) or status columns.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #994887 in MAAS: "Nodes listing does not support sorting"
https://bugs.launchpad.net/maas/+bug/994887
For more details, see:
https://code.launchpad.net/~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting/+merge/130600
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.
--
https://code.launchpad.net/~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting/+merge/130600
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting into lp:maas.
=== 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-19 16:21: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-19 16:21: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-19 16:21:21 +0000 differ
=== 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-19 16:21:21 +0000
@@ -1,9 +1,11 @@
+{% load sortable %}
+
{% if node_list|length %}
<table class="list">
<thead>
<tr>
- <th>MAC</th>
- <th>Status</th>
+ <th>{% sortable_link hostname "MAC" %}</th>
+ <th>{% sortable_link status "Status" %}</th>
</tr>
</thead>
{% for node in node_list %}
=== 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-19 16:21: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)
=== 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-19 16:21: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,8 +557,11 @@
{"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"))
=== 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-19 16:21: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-19 16:21: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-19 16:21: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))
Follow ups
