Code review comment for lp:~dimitern/maas/bug-994887-alternate-nodes-list-sorting

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