Code review comment for lp://qastaging/~rvb/maas/maas-security-model

> > [4]
...
>
> Good point I've used https://docs.djangoproject.com/en/dev/ref/utils/#django.u
> tils.http.urlquote_plus.

That's nice. I'll try and remember that one.

> > [5]
> >
> > + status = models.CharField(
> > + max_length=10, choices=NODE_STATUS_CHOICES, editable=False,
> > + default=DEFAULT_STATUS)
> >
> > This ought to be an enum, stored as a INT, like in Launchpad.
> > PostgreSQL does support ENUM fields, but they make upgrading more
> > complicated. I don't know what the Django way of doing this is.
>
> Django doesn't want to know about this… and python does not provide an enum
> type so I've changed the status to an IntegerField and did that:
>
> class NODE_STATUS:
> DEFAULT_STATUS = 0
> NEW = 0
> READY = 1
> DEPLOYED = 2
> COMMISSIONED = 3
> DECOMMISSIONED = 4
>
>
> NODE_STATUS_CHOICES = (
> (NODE_STATUS.NEW, u'New'),
> (NODE_STATUS.READY, u'Ready to Commission'),
> (NODE_STATUS.DEPLOYED, u'Deployed'),
> (NODE_STATUS.COMMISSIONED, u'Commissioned'),
> (NODE_STATUS.DECOMMISSIONED, u'Decommissioned'),
> )

Consider using flufl.enum (in a later branch perhaps). It's one of
Barry Warsaw's libraries. I've never used it but I doubt it's on
crack.

http://packages.python.org/flufl.enum/

> > [7]
> >
> > + def test_anon_nodes_GET(self):
> > + response = self.client.get('/api/nodes/')
> > + self.assertEqual(401, response.status_code)
> >
> > Comment/docstring missing.
> >
> > Most of our tests don't explain themselves :-/
>
> I'm going to do it because it's you but I think this is rather self-
> explanatory.

I agree, but I spent a long time in the Landscape test suite the year
before last. They have tons of small test methods without explanations
and it's actually quite hard going to move around in there. I also
think it's good to write a sentence about what this test is meant to
be doing; I think it helps avoid testing the wrong thing or testing
too many things.

> > [8]
> >
> > + self.user = factory.make_user(username='user', password='test')
> > + self.client = Client()
> > + self.client.login(username='user', password='test')
> >
> > This appears twice. I imagine it's going to appear many more
> > times. Consider extracting this to a get_authenticated_client()
> > function or something like that, and chucking it in the testing
> > package somewhere.
>
> This is really so simple that I'm reluctant to add yet another utility
> function… I've refactored the code avoid the duplication.

I kind of agree, but...

A line of code that reads "client = get_authenticated_client()"
quickly conveys its intent. Also, someone else might want to
parameterize the function to choose a different user name or password,
and what started out as only three lines becomes much more, but
remains short in the test.