Code review comment for lp://qastaging/~rvb/maas/maas-node-api

Thanks for the thorough review Gavin!

> [1]

Okay.

> [2]
> + for k, v in error.message_dict.items():
> s/items/iteritems/

Nice.

> [3]
> + if type(v) is list:
> if isinstance(v, list):

k

> [4]
> + return get_object_or_404(Node, system_id=system_id)
> Django is a massive layer violation isn't it? ;)

Well, you'll admit it's convenient and avoids boiler plate code…

> [5]
> + def read(self, request, system_id=None):
> + if system_id is not None:
> + return get_object_or_404(Node, system_id=system_id)
> + else:
> + return Node.objects.all().order_by('id')
>
> Does it make sense to s/Node/self.model/ above (and elsewhere)?

I think it's clearer like it is.

> [6]
>
> + node = Node(status = 'NEW', **dict(request.data.items()))
>
> Pyflakes ought to be moaning about the spaces around the =, bit it's
> curiously silent. Ah, maybe it's pep8 that ought to be moaning...

Good spot. I wonder why I use pyflake when I have pynella :)

> [7]
> + if 'status' in request.data:
> + return bad_request('Cannot set the status for a node.')
>
> This worries me. Do we need to check every bit of data coming in by
> hand? I don't want to have to do that, otherwise we're going to have
> to check handlers every time we update the model.

Well, this is only required because this is not checked at the model level, but I suppose we could put that in the model.

If you don't mind, I'll leave it like this for now because I think we will have to revisit that shortly anyway.

> [8]
> How do we layer authorization onto this?

That's my next task :)

> [9]
>
> - hostname = models.CharField(max_length=255)
> + hostname = models.CharField(max_length=255, null=True, default=None)
>
> Is there a min_length too? A hostname of "" is as good as null, so
> it's probably better to have just one or the other.

Good point, Django agrees with you: https://docs.djangoproject.com/en/dev/ref/models/fields/#django.db.models.Field.null

> [10]
>
> + mac = MACAddress(mac_address=mac_address, node=self)
> + mac.full_clean()
> + mac.save()
>
> Does mac.save() not imply full_clean()? Seems like a common pattern,
> and it's strange that Django doesn't just do it.

I agree. Model validation was only added in Django 1.2. Before that the validation was done using Forms. I think the reason for the manual call to full_clean is historic… but I might be wrong.

> [11]
>
> + def addMACAddress(self, mac_address):
> ...
> + def removeMACAddress(self, mac_address):
>
> We have a chance to adopt PEP8 naming (i.e. add_mac_address,
> remove_mac_address). Perhaps we ought to.

True, I've been infected by the lp way to name methods.

> [12]
>
> === added file 'src/maasserver/tests/factory.py'
>
> This should probably like in testing/factory.py.
>
>
> [13]
>
> +class Factory():
> ...
> + def createNode(self, hostname=None, set_hostname=False, status=None,
>
> Should we maintain the Launchpad naming here? i.e. makeSomething (or
> make_something if we're going PEP8). createSomething feels like proper
> API, if you know what I mean.

Okay, let's do this properly since we are starting fresh.

> [14]
>
> + self.assertEqual(2, len(parsed_result))
> + self.assertEqual(node1.hostname, parsed_result[0]['hostname'])
> + self.assertEqual(node1.system_id, parsed_result[0]['system_id'])
> + self.assertEqual(node2.hostname, parsed_result[1]['hostname'])
> + self.assertEqual(node2.system_id, parsed_result[1]['system_id'])
> This might be clearer and simpler as:
> expected = [
> {"hostname": node1.hostname, "system_id": node1.system_id},
> {"hostname": node2.hostname, "system_id": node2.system_id},
> ]
> self.assertEqual(expected, parsed_result)

ok.

> [15]
>
> + """
> + The api allows to fetch the list of Nodes.
>
> s/allows to fetch/allows for fetching/
>
> (Elsewhere too.)

Done.

> [16]
>
> + def test_node_DELETE(self):
> ...
> + self.assertEqual(
> + 0,
> + Node.objects.filter(system_id = system_id).count())
>
> The following might be clearer:
>
> + self.assertEqual(
> + [], list(Node.objects.filter(system_id=system_id)))
>
> (Is the list() needed?)

Okay. yes, lazy evaluation.

> [17]
> + def setUp(self):
> + self.node = factory.createNode()
> + self.mac1 = self.node.addMACAddress('aa:bb:cc:dd:ee:ff')
> + self.mac2 = self.node.addMACAddress('22:bb:cc:dd:aa:ff')
>
> Don't forget to up-call.

Right.

> [18]
> + self.assertEqual(
> + nb_macs + 1,
> + MACAddress.objects.filter(node = self.node).count())
>
> Like above, show the MACs instead of just the count.

Nah, I don't like macs :)

> [19]
>
> + def xtest_macs_DELETE_mac(self):
>
> s/xtest/test/ ?

Yep, en route for the 31th test…

> [20]
>
> + When deleting a MAC Address, the api returns a 'Bad Request' (400)
> + error the provided MAC Address is not valid.
>
> s/error/error if/

Done.

> [21]
>
> Why all the trailing slashes in the URL configuration?

Well, spotted, this is actually intentional: see https://docs.djangoproject.com/en/dev/ref/middleware/#django.middleware.common.CommonMiddleware.