launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

[0]

344	-        return Node.objects.get_nodes(
345	+        nodes = Node.objects.get_nodes(
346	             user=self.request.user,
347	             perm=NODE_PERMISSION.VIEW).order_by('-created')
348	+        if self.query:
349	+            try:
350	+                return constrain_nodes(nodes, _parse_constraints(self.query))
351	+            except InvalidConstraint as e:
352	+                self.query_error = e
353	+                return Node.objects.none()
354	+        return nodes

Not sure why you decided to do this in the view code instead of passing a 'constraints' parameter to get_nodes().  This way, get_available_node_for_acquisition could also pass on the 'constraints' argument to get_nodes().  Don't you think it would be better?

[1]

> I futzed around a fair bit trying to work out a sane way of doing this in django, but in the end picked the 
> simplest option, which is not using a Form class

I agree that using a form would not give you much here.  It would just be an encapsulation but this this seems to be used by the view code only (not the API) so it's not really useful.

[2]

303	+# Hey look, it's another mapping like the one in maasserver.api
304	+_non_tag_constraints = {

This might be obvious but why can't this be shared with maasserver.api somehow?  I guess I'm probably missing some context here.
-- 
https://code.launchpad.net/~gz/maas/node_search_view/+merge/128296
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gz/maas/node_search_view into lp:maas.