-----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 like: .order_by(id).order_by(order_by) Or .order_by(order_by).order_by(id) Since the existing order was '-created' we might keep that one? John =:-> review: approve -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iEYEARECAAYFAlCH8TYACgkQJdeBCYSNAAOqvgCgkd3RV3BCLh/Ledkqfm8t/rZE Jn0Anj3W6PSxQGG1azGvPsYLuH8CSoqG =YOlj -----END PGP SIGNATURE-----