Code review comment for lp:~rvb/maas/default-zone

Good stuff, though I tried to squeeze out a few notes:

.

Having an entire "Is this the default zone" column with a "Yes" in exactly one row seems a bit officious. Normally I'd say: just append "(default)" to the name. But with the default zone's name fixed to "default," that would look ridiculous as well. How about we just leave it out? We could make the default zone look different by italicising its displayed name.
.

Similarly, it looks a bit... clerical to say "Default / No" on every zone's details page and "Default / Yes" on the default zone's. Shouldn't we just have a notice like "This is the default zone. It cannot be renamed or deleted." on the default zone's page, and nothing on the rest? There may not even be a way to click through to that page without first noticing that there is a default zone.

.

Typo — test_updates_default_zone_description_works should lose either the "_works" suffix, of the first "s".

.

Finally, several things about test_rejects_deletion_of_default_zone:

    def test_rejects_deletion_of_default_zone(self):
        try:
            self.client.post(
                reverse('zone-del', args=[DEFAULT_ZONE_NAME]),
                {'post': 'yes'})
        except ValidationError:
            # XXX: Right now, this generates an error because the deletion

If you're going to have an XXX here, add your name and the date. Frankly though I'm not sure it's worth one — we have so many bigger fish to fry!

            # is prevented in the model code and not at the form level.
            # This is not so bad because we make sure that the deletion link
            # for the default zone isn't showed anywhere.

It's "shown," not "showed."

            # Still, ideally, once the validation happens at the form level,
            # this try/except statement should be removed and this test
            # should be extended further to check that the reponse to
            # the above self.client.post indicates a failure to validate
            # the data.
            pass

English has the extra "s" in "response." Keep technical English direct, and avoid the passive voice where possible! For example, I think this version takes less reading to understand: "If we move validation to the form level, this exception goes away and we'll have to check the HTTP response for a validation failure."

        # TODO: check that the page failed to validate the data.

If you want to verify that we actually got the ValidationError, use ExpectedException:

        with ExpectedException(ValidationError):
            self.client.post(
                ...)

This will make the test fail if the expected exception is not raised. Arguably that makes the test more brittle if we ever abolish the ValidationError, but in that case the comments will need revisiting anyway so it's appropriate for the test to call attention to itself.

Julian has been saying he would like us to use TODO markers only for reminders to ourselves, and never land them. It may be a little late for that though.

Anyway, thanks for the branch. I'll be happy to see this item closed!