Merge lp:~rvb/maas/maas-1.7-bug-1437094 into lp:maas/1.7

Proposed by Raphaël Badin on 2015-03-30
Status: Merged
Approved by: Raphaël Badin on 2015-03-30
Approved revision: 3357
Merged at revision: 3356
Proposed branch: lp:~rvb/maas/maas-1.7-bug-1437094
Merge into: lp:maas/1.7
Diff against target: 136 lines (+57/-23)
2 files modified
src/maasserver/views/nodes.py (+12/-7)
src/maasserver/views/tests/test_nodes.py (+45/-16)
To merge this branch: bzr merge lp:~rvb/maas/maas-1.7-bug-1437094
Reviewer Review Type Date Requested Status
Blake Rouse 2015-03-30 Approve on 2015-03-30
Review via email: mp+254548@code.launchpad.net

Commit message

Fix sorting by MAC address on the node listing page.

Description of the change

The 'late_sort_fields' mechanism was trying to access the 'primary_mac' property object on the node and it doesn't exist.

I fixed this by extending what the view supports so that we can add anything we want in the 'late_fields' instead of having the view too strictly tied to the methods defined on the model.

To post a comment you must log in.
Blake Rouse (blake-rouse) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/views/nodes.py'
2--- src/maasserver/views/nodes.py 2014-12-18 13:51:10 +0000
3+++ src/maasserver/views/nodes.py 2015-03-30 09:30:50 +0000
4@@ -28,7 +28,6 @@
5 from cgi import escape
6 import json
7 import logging
8-from operator import attrgetter
9 import re
10 from textwrap import dedent
11 from urllib import urlencode
12@@ -297,7 +296,14 @@
13 sort_fields = (
14 'hostname', 'status', 'owner', 'cpu_count',
15 'memory', 'storage', 'zone')
16- late_sort_fields = ('primary_mac', )
17+ late_sort_fields = {
18+ 'primary_mac': (
19+ lambda node1, node2: cmp(
20+ unicode(node1.get_primary_mac()),
21+ unicode(node2.get_primary_mac()),
22+ )
23+ ),
24+ }
25
26 def populate_modifiers(self, request):
27 self.query = request.GET.get("query")
28@@ -510,7 +516,7 @@
29 """
30
31 # Build relative URLs for the links, just with the params
32- fields = self.sort_fields + self.late_sort_fields
33+ fields = self.sort_fields + tuple(self.late_sort_fields.keys())
34 links = {field: '?' for field in fields}
35 classes = {field: 'sort-none' for field in fields}
36
37@@ -535,10 +541,9 @@
38 """
39 node_list = context['node_list']
40 reverse = (self.sort_dir == 'desc')
41- if self.sort_by in self.late_sort_fields:
42- node_list = sorted(
43- node_list, key=attrgetter(self.sort_by),
44- reverse=reverse)
45+ cmp_func = self.late_sort_fields.get(self.sort_by)
46+ if cmp_func is not None:
47+ node_list = sorted(node_list, cmp=cmp_func, reverse=reverse)
48 context['node_list'] = node_list
49 return context
50
51
52=== modified file 'src/maasserver/views/tests/test_nodes.py'
53--- src/maasserver/views/tests/test_nodes.py 2015-03-02 17:23:17 +0000
54+++ src/maasserver/views/tests/test_nodes.py 2015-03-30 09:30:50 +0000
55@@ -93,6 +93,7 @@
56 message_from_form_stats,
57 node_to_dict,
58 NodeEventListView,
59+ NodeListView,
60 NodeView,
61 )
62 from maastesting.djangotestcase import count_queries
63@@ -283,22 +284,18 @@
64 self.client_log_in()
65 factory.make_Node()
66 response = self.client.get(reverse('node-list'))
67- sort_hostname = '?sort=hostname&dir=asc'
68- sort_status = '?sort=status&dir=asc'
69- sort_owner = '?sort=owner&dir=asc'
70- sort_cpu_count = '?sort=cpu_count&dir=asc'
71- sort_memory = '?sort=memory&dir=asc'
72- sort_storage = '?sort=storage&dir=asc'
73- sort_primary_mac = '?sort=primary_mac&dir=asc'
74- sort_zone = '?sort=zone&dir=asc'
75- self.assertIn(sort_hostname, get_content_links(response))
76- self.assertIn(sort_status, get_content_links(response))
77- self.assertIn(sort_owner, get_content_links(response))
78- self.assertIn(sort_cpu_count, get_content_links(response))
79- self.assertIn(sort_memory, get_content_links(response))
80- self.assertIn(sort_storage, get_content_links(response))
81- self.assertIn(sort_primary_mac, get_content_links(response))
82- self.assertIn(sort_zone, get_content_links(response))
83+ sort_fields = (
84+ NodeListView.sort_fields +
85+ tuple(NodeListView.late_sort_fields.keys())
86+ )
87+ expected_sort_fields = [
88+ 'hostname', 'status', 'owner', 'cpu_count', 'memory', 'storage',
89+ 'primary_mac', 'zone'
90+ ]
91+ self.assertItemsEqual(expected_sort_fields, sort_fields)
92+ sorting_links = ['?sort=%s&dir=asc' % field for field in sort_fields]
93+ self.assertThat(
94+ get_content_links(response), ContainsAll(sorting_links))
95
96 def test_node_list_ignores_unknown_sort_param(self):
97 self.client_log_in()
98@@ -385,6 +382,38 @@
99 [link for link in get_content_links(response)
100 if link.startswith('/nodes/node')])
101
102+ def test_node_list_sorts_by_primary_mac(self):
103+ self.client_log_in()
104+ [factory.make_Node(mac=True) for _ in range(8)]
105+
106+ # First check the ascending sort order.
107+ node_macs = sorted(
108+ (unicode(n.get_primary_mac()), n) for n in Node.objects.all())
109+ sorted_nodes = [item[1] for item in node_macs]
110+ response = self.client.get(
111+ reverse('node-list'), {
112+ 'sort': 'primary_mac',
113+ 'dir': 'asc'})
114+ node_links = [
115+ reverse('node-view', args=[node.system_id])
116+ for node in sorted_nodes
117+ ]
118+ self.assertEqual(
119+ node_links,
120+ [link for link in get_content_links(response)
121+ if link.startswith('/nodes/node')])
122+
123+ # Now check the reverse order.
124+ node_links = list(reversed(node_links))
125+ response = self.client.get(
126+ reverse('node-list'), {
127+ 'sort': 'primary_mac',
128+ 'dir': 'desc'})
129+ self.assertEqual(
130+ node_links,
131+ [link for link in get_content_links(response)
132+ if link.startswith('/nodes/node')])
133+
134 def test_node_list_sorts_by_zone(self):
135 self.client_log_in()
136 zones = [factory.make_Zone(sortable_name=True) for _ in range(5)]

Subscribers

People subscribed via source and target branches

to all changes: