Merge lp:~dimitern/maas/bug-994887-alternate-nodes-list-sorting into lp:~maas-committers/maas/trunk

Proposed by Dimiter Naydenov
Status: Merged
Approved by: Dimiter Naydenov
Approved revision: no longer in the source branch.
Merged at revision: 1294
Proposed branch: lp:~dimitern/maas/bug-994887-alternate-nodes-list-sorting
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 309 lines (+178/-9)
6 files modified
src/maasserver/static/css/components/table_list.css (+11/-0)
src/maasserver/templates/maasserver/node_list.html (+1/-1)
src/maasserver/templates/maasserver/nodes_listing.html (+7/-0)
src/maasserver/templates/maasserver/tag_view.html (+1/-1)
src/maasserver/tests/test_views_nodes.py (+115/-5)
src/maasserver/views/nodes.py (+43/-2)
To merge this branch: bzr merge lp:~dimitern/maas/bug-994887-alternate-nodes-list-sorting
Reviewer Review Type Date Requested Status
Martin Packman (community) Approve
John A Meinel (community) Approve
Review via email: mp+131177@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :
Download full text (3.5 KiB)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/24/2012 02:57 PM, Dimiter Naydenov wrote:
> 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.
>

+ # Just creare some nodes to have something in the list

typo 'create'

However, why do we need the nodes to actually get the links? I would
tend to have no more than 1 'make_node()' call.

...
+ node_links = [
+ reverse('node-view', args=[node.system_id])
+
+ for node in sorted_nodes]

The spacing in this test looks wrong. You probably want to run 'make
lint' and handle anything that it brings up as a problem.

+ # Now check the reverse order
+ sorted_nodes = sorted(
+ nodes, key=lambda x: x.status, reverse=True)

I don't think you need to re-do the sort, if anything just do
"sorted_nodes = reversed(sorted_nodes)".
However, I think all you actually need to do is:
node_links = reversed(node_links) and not do anything with re-sorting
the node objects themselves.

That should simplify the test a bit.

+ # 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")))

I'm hesitant about using 'filter' in these cases. I'm wondering if
there is a better way to write the tests. Perhaps giving a specific
class id for the node links, or one for the sorting links?

         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)

I kind of feel like we want to always have a sort field that is
unique. What I mean, is that if you sort by 'status' then all nodes
with the same status can occur in arbitrary order. (It is probably
reasonably static because the db isn't changing a lot in the
background, but it isn't guaranteed). Certainly an unrelated action
that would otherwise not affect the nodes being shown could cause a
node that was on page 1 to suddenly swap places with a node on page 2.

I'm not sure about the specific order of operations, but I think we
want something li...

Read more...

review: Approve
Revision history for this message
Martin Packman (gz) wrote :

        # Just creare some nodes to have something in the list

Tyop, 'create'. Hm, we should really make a helper for multi-node creation on these views, but that can happen later.

+ sorted_nodes = sorted(nodes, key=lambda x: x.hostname)

Always nice to use operator.attrgetter('hostname') for this.

+ 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)

Rather than having multiple string contains assertions like this, I'd use urlparse.parse_qsl and testtools matchers against all the key/values you expect, so a failure has the full context. Something like:

    self.assertThat(urlparse.parse_qsl(link), ContainsAll([('page', '1'), ...]))

- 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"))

For these, I'd adjust the xpath instead, ie, .../table//td//a/...

+ links = {field: self.request.path + '?' for field in fields}

Give relative links rather than including the path in them, the link should just start with the '?', note tests will need changing to expect this too.

review: Needs Fixing
Revision history for this message
Dimiter Naydenov (dimitern) wrote :

Thanks for the review! I made the requested modifications and also simplified the tests (fixing a minor issue I found with the initial sort order).

Revision history for this message
Martin Packman (gz) wrote :

+ perm=NODE_PERMISSION.VIEW,).order_by(order_by, '-created')

Having the default order_by here being ('-created', '-created') but all the other changes look good.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 16:02:22 +0000
@@ -30,6 +30,17 @@
30 border-color: #dd4814;30 border-color: #dd4814;
31 cursor: pointer;31 cursor: pointer;
32 }32 }
33.sort-none, .sort-asc, .sort-desc {
34 background-repeat: no-repeat;
35 background-position: right 0.4em;
36 padding-right: 1.2em;
37 }
38.sort-asc {
39 background-image: url('/static/img/up_arrow_dark.png');
40 }
41.sort-desc {
42 background-image: url('/static/img/down_arrow_dark.png');
43 }
33/* ul list */44/* ul list */
34ul.list {45ul.list {
35 list-style: none;46 list-style: none;
3647
=== added file 'src/maasserver/static/img/down_arrow_dark.png'
37Binary 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 16:02:22 +0000 differ48Binary 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 16:02:22 +0000 differ
=== added file 'src/maasserver/static/img/up_arrow_dark.png'
38Binary 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 16:02:22 +0000 differ49Binary 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 16:02: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 16:02:22 +0000
@@ -38,7 +38,7 @@
38 {% if input_query_error %}38 {% if input_query_error %}
39 <p class="form-errors">{{input_query_error}}</p>39 <p class="form-errors">{{input_query_error}}</p>
40 {% endif %}40 {% endif %}
41 {% include "maasserver/nodes_listing.html" %}41 {% include "maasserver/nodes_listing.html" with sorting="true" %}
42 {% include "maasserver/pagination.html" %}42 {% include "maasserver/pagination.html" %}
43 <a id="addnode" href="#" class="button right space-top">+ Add node</a>43 <a id="addnode" href="#" class="button right space-top">+ Add node</a>
44 <div class="clear"></div>44 <div class="clear"></div>
4545
=== 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 16:02:22 +0000
@@ -2,8 +2,15 @@
2 <table class="list">2 <table class="list">
3 <thead>3 <thead>
4 <tr>4 <tr>
5 {% if sorting == "true" %}
6 <th><a href="{{ sort_links.hostname }}"
7 class="{{ sort_classes.hostname }}">MAC</a></th>
8 <th><a href="{{ sort_links.status }}"
9 class="{{ sort_classes.status }}">Status</a></th>
10 {% else %}
5 <th>MAC</th>11 <th>MAC</th>
6 <th>Status</th>12 <th>Status</th>
13 {% endif %}
7 </tr>14 </tr>
8 </thead>15 </thead>
9 {% for node in node_list %}16 {% for node in node_list %}
1017
=== 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 16:02:22 +0000
@@ -34,7 +34,7 @@
34 </ul>34 </ul>
35 <div id="nodes" class="block first pad-top">35 <div id="nodes" class="block first pad-top">
36 <h2 class="block first line-top pad-top">{{ paginator.count }} Node{{ paginator.count|pluralize }}</h2>36 <h2 class="block first line-top pad-top">{{ paginator.count }} Node{{ paginator.count|pluralize }}</h2>
37 {% include "maasserver/nodes_listing.html" %}37 {% include "maasserver/nodes_listing.html" with sorting="false" %}
38 {% include "maasserver/pagination.html" %}38 {% include "maasserver/pagination.html" %}
39 </div>39 </div>
40{% endblock %}40{% endblock %}
4141
=== 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 16:02:22 +0000
@@ -13,7 +13,12 @@
13__all__ = []13__all__ = []
1414
15import httplib15import httplib
16from operator import attrgetter
16from unittest import skip17from unittest import skip
18from urlparse import (
19 parse_qsl,
20 urlparse,
21 )
1722
18from django.conf import settings23from django.conf import settings
19from django.core.urlresolvers import reverse24from django.core.urlresolvers import reverse
@@ -79,6 +84,108 @@
79 enlist_preseed_link = reverse('enlist-preseed-view')84 enlist_preseed_link = reverse('enlist-preseed-view')
80 self.assertIn(enlist_preseed_link, get_content_links(response))85 self.assertIn(enlist_preseed_link, get_content_links(response))
8186
87 def test_node_list_contains_column_sort_links(self):
88 # Just create a node to have something in the list
89 factory.make_node()
90 response = self.client.get(reverse('node-list'))
91 sort_hostname = '?sort=hostname&dir=asc'
92 sort_status = '?sort=status&dir=asc'
93 self.assertIn(sort_hostname, get_content_links(response))
94 self.assertIn(sort_status, get_content_links(response))
95
96 def test_node_list_sorts_by_hostname(self):
97 names = ['zero', 'one', 'five']
98 nodes = [factory.make_node(hostname=n) for n in names]
99
100 # First check the ascending sort order
101 sorted_nodes = sorted(nodes, key=attrgetter('hostname'))
102 response = self.client.get(
103 reverse('node-list'), {
104 'sort': 'hostname',
105 'dir': 'asc'})
106 node_links = [
107 reverse('node-view', args=[node.system_id])
108 for node in sorted_nodes]
109 self.assertEqual(
110 node_links,
111 [link for link in get_content_links(response)
112 if link.startswith('/nodes/node')])
113
114 # Now check the reverse order
115 node_links = list(reversed(node_links))
116 response = self.client.get(
117 reverse('node-list'), {
118 'sort': 'hostname',
119 'dir': 'desc'})
120 self.assertEqual(
121 node_links,
122 [link for link in get_content_links(response)
123 if link.startswith('/nodes/node')])
124
125 def test_node_list_sorts_by_status(self):
126 statuses = {
127 NODE_STATUS.READY,
128 NODE_STATUS.DECLARED,
129 NODE_STATUS.FAILED_TESTS,
130 }
131 nodes = [factory.make_node(status=s) for s in statuses]
132
133 # First check the ascending sort order
134 sorted_nodes = sorted(nodes, key=attrgetter('status'))
135 response = self.client.get(
136 reverse('node-list'), {
137 'sort': 'status',
138 'dir': 'asc'})
139 node_links = [
140 reverse('node-view', args=[node.system_id])
141 for node in sorted_nodes]
142 self.assertEqual(
143 node_links,
144 [link for link in get_content_links(response)
145 if link.startswith('/nodes/node')])
146
147 # Now check the reverse order
148 node_links = list(reversed(node_links))
149 response = self.client.get(
150 reverse('node-list'), {
151 'sort': 'status',
152 'dir': 'desc'})
153 self.assertEqual(
154 node_links,
155 [link for link in get_content_links(response)
156 if link.startswith('/nodes/node')])
157
158 def test_node_list_sort_preserves_other_params(self):
159 # Set a very small page size to save creating lots of nodes
160 page_size = 2
161 self.patch(nodes_views.NodeListView, 'paginate_by', page_size)
162
163 nodes = []
164 tag = factory.make_tag('shiny')
165 for name in ('bbb', 'ccc', 'ddd', 'aaa'):
166 node = factory.make_node(hostname=name)
167 node.tags = [tag]
168 nodes.append(node)
169
170 params = {
171 'sort': 'hostname',
172 'dir': 'asc',
173 'page': '1',
174 'query': 'maas-tags=shiny'}
175 response = self.client.get(reverse('node-list'), params)
176 document = fromstring(response.content)
177 header_links = document.xpath("//div[@id='nodes']/table//th/a/@href")
178 fields = iter(('hostname', 'status'))
179 field_dirs = iter(('desc', 'asc'))
180 for link in header_links:
181 self.assertThat(
182 parse_qsl(urlparse(link).query),
183 ContainsAll([
184 ('page', '1'),
185 ('query', 'maas-tags=shiny'),
186 ('sort', next(fields)),
187 ('dir', next(field_dirs))]))
188
82 def test_node_list_displays_sorted_list_of_nodes(self):189 def test_node_list_displays_sorted_list_of_nodes(self):
83 # Nodes are sorted on the node list page, newest first.190 # Nodes are sorted on the node list page, newest first.
84 nodes = [factory.make_node() for i in range(3)]191 nodes = [factory.make_node() for i in range(3)]
@@ -421,7 +528,8 @@
421 {"query": "maas-tags=shiny cpu=2"})528 {"query": "maas-tags=shiny cpu=2"})
422 node2_link = reverse('node-view', args=[node2.system_id])529 node2_link = reverse('node-view', args=[node2.system_id])
423 document = fromstring(response.content)530 document = fromstring(response.content)
424 node_links = document.xpath("//div[@id='nodes']/table//a/@href")531 node_links = document.xpath(
532 "//div[@id='nodes']/table//td/a/@href")
425 self.assertEqual([node2_link], node_links)533 self.assertEqual([node2_link], node_links)
426534
427 def test_node_list_paginates(self):535 def test_node_list_paginates(self):
@@ -434,7 +542,7 @@
434 # Order node links with newest first as the view is expected to542 # Order node links with newest first as the view is expected to
435 node_links = [reverse('node-view', args=[node.system_id])543 node_links = [reverse('node-view', args=[node.system_id])
436 for node in reversed(nodes)]544 for node in reversed(nodes)]
437 expr_node_links = XPath("//div[@id='nodes']/table//a/@href")545 expr_node_links = XPath("//div[@id='nodes']/table//td/a/@href")
438 expr_page_anchors = XPath("//div[@class='pagination']//a")546 expr_page_anchors = XPath("//div[@class='pagination']//a")
439 # Fetch first page, should link newest two nodes and page 2547 # Fetch first page, should link newest two nodes and page 2
440 response = self.client.get(reverse('node-list'))548 response = self.client.get(reverse('node-list'))
@@ -446,7 +554,8 @@
446 # Fetch second page, should link next nodes and adjacent pages554 # Fetch second page, should link next nodes and adjacent pages
447 response = self.client.get(reverse('node-list'), {"page": 2})555 response = self.client.get(reverse('node-list'), {"page": 2})
448 page2 = fromstring(response.content)556 page2 = fromstring(response.content)
449 self.assertEqual(node_links[page_size:page_size * 2],557 self.assertEqual(
558 node_links[page_size:page_size * 2],
450 expr_node_links(page2))559 expr_node_links(page2))
451 self.assertEqual([("first", "."), ("previous", "."),560 self.assertEqual([("first", "."), ("previous", "."),
452 ("next", "?page=3"), ("last", "?page=3")],561 ("next", "?page=3"), ("last", "?page=3")],
@@ -474,8 +583,9 @@
474 {"query": "maas-tags=odd", "page": 3})583 {"query": "maas-tags=odd", "page": 3})
475 document = fromstring(response.content)584 document = fromstring(response.content)
476 self.assertIn("5 matching nodes", document.xpath("string(//h1)"))585 self.assertIn("5 matching nodes", document.xpath("string(//h1)"))
477 self.assertEqual([last_node_link],586 self.assertEqual(
478 document.xpath("//div[@id='nodes']/table//a/@href"))587 [last_node_link],
588 document.xpath("//div[@id='nodes']/table//td/a/@href"))
479 self.assertEqual([("first", "?query=maas-tags%3Dodd"),589 self.assertEqual([("first", "?query=maas-tags%3Dodd"),
480 ("previous", "?query=maas-tags%3Dodd&page=2")],590 ("previous", "?query=maas-tags%3Dodd&page=2")],
481 [(a.text.lower(), a.get("href"))591 [(a.text.lower(), a.get("href"))
482592
=== 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 16:02:22 +0000
@@ -110,13 +110,26 @@
110 def get(self, request, *args, **kwargs):110 def get(self, request, *args, **kwargs):
111 self.query = request.GET.get("query")111 self.query = request.GET.get("query")
112 self.query_error = None112 self.query_error = None
113 self.sort_by = request.GET.get("sort")
114 self.sort_dir = request.GET.get("dir")
115
113 return super(NodeListView, self).get(request, *args, **kwargs)116 return super(NodeListView, self).get(request, *args, **kwargs)
114117
115 def get_queryset(self):118 def get_queryset(self):
116 # Return node list sorted, newest first.119 # Default sort - newest first, unless sorting params are
120 # present. In addition, to ensure order consistency, when
121 # sorting by non-unique fields (like status), we always
122 # sort by the unique creation date as well
123 if self.sort_by is not None:
124 prefix = '-' if self.sort_dir == 'desc' else ''
125 order_by = (prefix + self.sort_by, '-created')
126 else:
127 order_by = ('-created', )
128
129 # Return the sorted node list
117 nodes = Node.objects.get_nodes(130 nodes = Node.objects.get_nodes(
118 user=self.request.user, prefetch_mac=True,131 user=self.request.user, prefetch_mac=True,
119 perm=NODE_PERMISSION.VIEW,).order_by('-created')132 perm=NODE_PERMISSION.VIEW,).order_by(*order_by)
120 if self.query:133 if self.query:
121 try:134 try:
122 return constrain_nodes(nodes, _parse_constraints(self.query))135 return constrain_nodes(nodes, _parse_constraints(self.query))
@@ -125,11 +138,39 @@
125 return Node.objects.none()138 return Node.objects.none()
126 return nodes139 return nodes
127140
141 def _prepare_sort_links(self):
142 """Returns 2 dicts, with sort fields as keys and
143 links and CSS classes for the that field.
144 """
145
146 fields = ('hostname', 'status')
147 # Build relative URLs for the links, just with the params
148 links = {field: '?' for field in fields}
149 classes = {field: 'sort-none' for field in fields}
150
151 params = self.request.GET.copy()
152 reverse_dir = 'asc' if self.sort_dir == 'desc' else 'desc'
153
154 for field in fields:
155 params['sort'] = field
156 if field == self.sort_by:
157 params['dir'] = reverse_dir
158 classes[field] = 'sort-%s' % self.sort_dir
159 else:
160 params['dir'] = 'asc'
161
162 links[field] += params.urlencode()
163
164 return links, classes
165
128 def get_context_data(self, **kwargs):166 def get_context_data(self, **kwargs):
129 context = super(NodeListView, self).get_context_data(**kwargs)167 context = super(NodeListView, self).get_context_data(**kwargs)
130 context.update(get_longpoll_context())168 context.update(get_longpoll_context())
131 context["input_query"] = self.query169 context["input_query"] = self.query
132 context["input_query_error"] = self.query_error170 context["input_query_error"] = self.query_error
171 links, classes = self._prepare_sort_links()
172 context["sort_links"] = links
173 context["sort_classes"] = classes
133 return context174 return context
134175
135176