Code review comment for lp:~jtv/maas/show-commissioning-results

First of all, thanks a lot for doing this. And congrats for getting something done in such a short time!

[0]

I like the design you've chosen. I like the choice to have a top-level nodecommissioningresults url that you can then filter using query parameters. It's somewhat similar to the API and it's pretty flexible.

> I couldn't take the lack of visibility any more, and hacked up a UI.

It was a problem for our users so it's definitely a worthwhile addition to the UI.

[1]

> Layout of the new code is awkward.

The Django app 'maasserver' depends on the Django app 'metadataserver'; now it happens to provide UI for models defined in the 'metadataserver', that's really fine even if it's a tiny bit awkward. The only thing we could do is more the NodeCommissioning model back into maasserver but I don't think it's worth it.

[2]

+++ src/maasserver/views/nodecommissionresult.py

202 + results = NodeCommissionResult.objects.all()
203 + node = self.request.GET.get('node')
204 + if node is not None:
205 + results = results.filter(node__system_id__in=node)

This can't work: the 'node' object (which I'd rename 'system_ids' if I where you) is the *first* value of the parameter 'node'; what I mean is this: if you use request.GET.get('zz') and the request is "?zz=aa&zz=bb" you'll get 'aa'. You need to use getlist() here (see https://docs.djangoproject.com/en/dev/ref/request-response/#querydict-objects) to get the list of all the values. I'm guessing that's what you had in mind given that you're using that in conjunction with 'system_id__in'.

[3]

Let's add a couple of fake NodeCommissioningResult objects to the sample data (http://paste.ubuntu.com/6947847/). It will make the diff bigger but it will help us a great deal to see if this is working or not (I did that manually and that's how I found the problem I described in [2]).