launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #13664
[Merge] lp:~dimitern/maas/bug-994887-alternate-nodes-list-sorting into lp:maas
Dimiter Naydenov has proposed merging lp:~dimitern/maas/bug-994887-alternate-nodes-list-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-alternate-nodes-list-sorting/+merge/131177
Implemented sorting for /MAAS/nodes/ list - by hostname or status (asc/desc). This is an alternative, simpler solution, compared to https://code.launchpad.net/~dimitern/maas/bug-994887-nodes-listing-does-not-support-sorting/+merge/130600 - not using third party modules (django-sortable) anymore. UI changes (sorting up/down arrows) were already approved, so just the code changes need approval.
--
https://code.launchpad.net/~dimitern/maas/bug-994887-alternate-nodes-list-sorting/+merge/131177
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~dimitern/maas/bug-994887-alternate-nodes-list-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-24 12:56:22 +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-24 12:56:22 +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-24 12:56:22 +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-24 12:56:22 +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-10-10 08:13:01 +0000
+++ src/maasserver/templates/maasserver/nodes_listing.html 2012-10-24 12:56:22 +0000
@@ -2,8 +2,15 @@
<table class="list">
<thead>
<tr>
+ {% if sorting == "true" %}
+ <th><a href="{{ sort_links.hostname }}"
+ class="{{ sort_classes.hostname }}">MAC</a></th>
+ <th><a href="{{ sort_links.status }}"
+ class="{{ sort_classes.status }}">Status</a></th>
+ {% else %}
<th>MAC</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-10-18 11:26:02 +0000
+++ src/maasserver/templates/maasserver/tag_view.html 2012-10-24 12:56:22 +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-10-15 17:39:31 +0000
+++ src/maasserver/tests/test_views_nodes.py 2012-10-24 12:56:22 +0000
@@ -79,6 +79,113 @@
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_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")
+ for link in header_links:
+ self.assertIn('page=1', link)
+ self.assertIn('query=maas-tags%3Dshiny', link)
+ self.assertIn('sort=', link)
+ self.assertIn('dir=', link)
+
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 +528,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 +549,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 +568,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 +592,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"))
=== 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-24 12:56:22 +0000
@@ -110,13 +110,25 @@
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", "asc")
+
+ # Default sorting is descending by creation date
+ if self.sort_by is None:
+ self.sort_by = "created"
+ self.sort_dir = "desc"
+
return super(NodeListView, self).get(request, *args, **kwargs)
def get_queryset(self):
- # Return node list sorted, newest first.
+ order_by = self.sort_by
+ if self.sort_dir == 'desc':
+ order_by = '-' + order_by
+ # Return node list sorted, newest first by default,
+ # unless sorting params are present.
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))
@@ -125,11 +137,38 @@
return Node.objects.none()
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')
+ links = {field: self.request.path + '?' 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'] = self.sort_dir
+ classes[field] = 'sort-%s' % self.sort_dir
+ else:
+ params['dir'] = reverse_dir
+
+ 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
