launchpad-reviewers team mailing list archive

[Martin Packman <martin.packman@xxxxxxxxxxxxx>]

Martin Packman has proposed merging lp:~gz/maas/node_search_view into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~gz/maas/node_search_view/+merge/128296

Adds the ability to view a subset of nodes on the node list page, using constraints to filter which nodes are displayed.

The search query is treated as a space separated list of tags, which a little special handling to make "mem:4096" and so on work as well. This is similar to the early designs for the feature, rather than being more along the lines of the juju constraints argument. This is mostly because having to use "tags=one,two,three mem=1" is less convenient and harder to handle than just treating everything as a tag by default. Unifying the code that does constraint mapping a little better would still be worthwhile.

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 and just parsing the search mini-language late and reporting any errors inline on the page. Doing validation separate from the query is not trivial, and would not bring much benefit. A little more poking on the design and layout would be nice still.
-- 
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.

=== modified file 'src/maasserver/static/css/components/search_box.css'
--- src/maasserver/static/css/components/search_box.css	2012-03-01 06:22:38 +0000
+++ src/maasserver/static/css/components/search_box.css	2012-10-05 17:28:40 +0000
@@ -1,5 +1,42 @@
-input.search-box {
+form.search {
+    position: relative;
+    width: 228px;
+    height: 26px;
+    }
+#header form.search {
+    position: relative;
+    width: 228px;
+    margin: 18px 15px 0 0;
+    }
+input.search-input {
+    position: absolute;
+    top: 0;
+    left: 0;
+    width: 450px;
     padding-left: 22px;
     color: #aea79f;
-    background: #fff url(../../img/search_icon.png) no-repeat 5px center;
-    }
+    background-color: #fff;
+    }
+#header input.search-input {
+    width: 225px;
+    border: none;
+    -moz-border-radius: 4px;
+    -webkit-border-radius: 4px;
+    border-radius: 4px;
+    }
+input.search-submit,
+input.search-submit:hover {
+    position: absolute;
+    top: 1px;
+    left: 1px;
+    width: 24px;
+    height: 24px;
+    padding: 0;
+    margin: 0;
+    background: transparent url(../?img/search_icon.png) no-repeat center center;
+    border: none;
+    font-size: 0;
+    }
+input.search-submit:hover {
+    cursor: pointer;
+}

=== modified file 'src/maasserver/static/css/forms.css'
--- src/maasserver/static/css/forms.css	2012-06-06 16:01:55 +0000
+++ src/maasserver/static/css/forms.css	2012-10-05 17:28:40 +0000
@@ -35,6 +35,7 @@
     margin-right: 3px;
     }
 textarea,
+input[type="search"],
 input[type="password"],
 input[type="text"] {
     width: 350px;
@@ -47,6 +48,7 @@
     background: #FFF;
     }
 textarea:focus,
+input[type="search"]:focus,
 input[type="password"]:focus,
 input[type="text"]:focus {
     border-color: #000;

=== modified file 'src/maasserver/static/css/layout.css'
--- src/maasserver/static/css/layout.css	2012-06-29 10:04:44 +0000
+++ src/maasserver/static/css/layout.css	2012-10-05 17:28:40 +0000
@@ -68,26 +68,6 @@
 #right-nav li.active a {
     background-color: transparent;
     }
-#right-nav li.divider {
-    height: 30px;
-    margin-top: 17px;
-    border-left: 1px solid #c53f11;
-    }
-
-
-/******************************************************************************
-    Header search
-*/
-#header-search {
-    margin: 18px 15px 0 0;
-    }
-
-#header-search input[type="text"] {
-    width: 200px;
-    -moz-border-radius: 4px;
-    -webkit-border-radius: 4px;
-    border-radius: 4px;
-    }
 
 
 /******************************************************************************

=== modified file 'src/maasserver/templates/maasserver/base.html'
--- src/maasserver/templates/maasserver/base.html	2012-08-03 16:36:26 +0000
+++ src/maasserver/templates/maasserver/base.html	2012-10-05 17:28:40 +0000
@@ -45,21 +45,15 @@
           </div>
           <div id="header" class="center-page-content">
               <ul id="right-nav" class="nav">
-                {% comment %}
-                  <!-- To be enabled when we have search functionality -->
                   <li class="search">
                     {% block header-search %}
-                      <form action="{% url 'node-list' %}" method="get" id="header-search">
-                        <input type="text" name="query" value="Search nodes" class="search-box" />
+                      <form action="{% url 'node-list' %}" method="get" class="search">
+                        <input type="search" name="query" placeholder="Search nodes" class="search-input" />
+                        <input type="submit" value="Search" class="search-submit" />
                       </form>
                     {% endblock %}
                   </li>
-                {% endcomment %}
                 {% if user.is_superuser %}
-                  {% comment %}
-                    <!-- To be enabled when we have search functionality -->
-                    <li class="divider"></li>
-                  {% endcomment %}
                   <li class="icon {% block nav-active-settings %}{% endblock %}">
                     <a href="{% url 'settings' %}">
                       <img src="{{ STATIC_URL }}img/settings.png" alt="Settings" />

=== modified file 'src/maasserver/templates/maasserver/node_list.html'
--- src/maasserver/templates/maasserver/node_list.html	2012-10-02 08:12:10 +0000
+++ src/maasserver/templates/maasserver/node_list.html	2012-10-05 17:28:40 +0000
@@ -31,12 +31,13 @@
 
 {% block content %}
   <div id="nodes">
-    {% comment %}
-      <!-- To be enabled when we have search functionality -->
-      <form action="" method="get" class="block full-width inline-form">
-        <input type="text" name="query" class="search-box" value="Search nodes" />
-      </form>
-    {% endcomment %}
+    <form action="{% url 'node-list' %}" method="get" class="block full-width search">
+      <input type="search" name="query" placeholder="Search nodes" class="search-input" value="{{input_query|default_if_none:''}}" />
+      <input type="submit" value="Search" class="search-submit" />
+    </form>
+    {% if input_query_error %}
+    <p class="form-errors">{{input_query_error}}</p>
+    {% endif %}
     {% include "maasserver/nodes_listing.html" %}
     <a id="addnode" href="#" class="button right space-top">+ Add node</a>
     <div class="clear"></div>

=== modified file 'src/maasserver/tests/test_views_nodes.py'
--- src/maasserver/tests/test_views_nodes.py	2012-10-05 03:17:59 +0000
+++ src/maasserver/tests/test_views_nodes.py	2012-10-05 17:28:40 +0000
@@ -361,6 +361,41 @@
             "This node is now allocated to you.",
             '\n'.join(msg.message for msg in response.context['messages']))
 
+    def test_node_list_query_includes_current(self):
+        qs = factory.getRandomString()
+        response = self.client.get(reverse('node-list'), {"query": qs})
+        query_value = fromstring(response.content).xpath(
+            "string(//div[@id='nodes']//input[@name='query']/@value)")
+        self.assertIn(qs, query_value)
+
+    def test_node_list_query_error_on_missing_tag(self):
+        response = self.client.get(reverse('node-list'), {"query": "missing"})
+        error_string = fromstring(response.content).xpath(
+            "string(//div[@id='nodes']//p[@class='form-errors'])")
+        self.assertRegexpMatches(error_string, "Invalid .* No such tag")
+
+    def test_node_list_query_error_on_unknown_constraint(self):
+        response = self.client.get(reverse('node-list'),
+            {"query": "color:red"})
+        error_string = fromstring(response.content).xpath(
+            "string(//div[@id='nodes']//p[@class='form-errors'])")
+        self.assertRegexpMatches(error_string, "Invalid .* 'color:red'")
+
+    def test_node_list_query_selects_subset(self):
+        tag = factory.make_tag("shiny")
+        node1 = factory.make_node(cpu_count=1)
+        node2 = factory.make_node(cpu_count=2)
+        node3 = factory.make_node(cpu_count=2)
+        node1.tags = [tag]
+        node2.tags = [tag]
+        node3.tags = []
+        response = self.client.get(reverse('node-list'),
+            {"query": "shiny cpu:2"})
+        node2_link = reverse('node-view', args=[node2.system_id])
+        document = fromstring(response.content)
+        node_links = document.xpath("//div[@id='nodes']/table//a/@href")
+        self.assertEqual([node2_link], node_links)
+
 
 class NodePreseedViewTest(LoggedInTestCase):
 

=== modified file 'src/maasserver/views/nodes.py'
--- src/maasserver/views/nodes.py	2012-09-30 11:48:37 +0000
+++ src/maasserver/views/nodes.py	2012-10-05 17:28:40 +0000
@@ -43,6 +43,7 @@
     NODE_STATUS,
     )
 from maasserver.exceptions import (
+    InvalidConstraint,
     MAASAPIException,
     NoRabbit,
     )
@@ -56,6 +57,7 @@
     MACAddress,
     Node,
     )
+from maasserver.models.node_constraint_filter import constrain_nodes
 from maasserver.preseed import (
     get_enlist_preseed,
     get_preseed,
@@ -78,19 +80,63 @@
         return {}
 
 
+# Hey look, it's another mapping like the one in maasserver.api
+_non_tag_constraints = {
+    "cpu": "cpu_count",
+    "cpu_count": "cpu_count",
+    "mem": "memory",
+    "memory": "memory",
+    "arch": "architecture",
+    "name": "hostname",
+}
+
+
+def _parse_constraints(query_string):
+    """Turn query string from user into constraints dict
+
+    Ideally this would be pretty much the same as the juju constraints, but
+    there are a few things that don't fit nicely across the two models.
+    """
+    constraints = {}
+    tags = []
+    for word in query_string.strip().split():
+        parts = word.split(":", 1)
+        if len(parts) == 2 and parts[0] in _non_tag_constraints:
+            constraints[_non_tag_constraints[parts[0]]] = parts[1]
+        else:
+            tags.append(word)
+    if tags:
+        constraints["tags"] = " ".join(tags)
+    return constraints
+
+
 class NodeListView(ListView):
 
     context_object_name = "node_list"
 
+    def get(self, request, *args, **kwargs):
+        self.query = request.GET.get("query")
+        self.query_error = None
+        return super(NodeListView, self).get(request, *args, **kwargs)
+
     def get_queryset(self):
         # Return node list sorted, newest first.
-        return Node.objects.get_nodes(
+        nodes = Node.objects.get_nodes(
             user=self.request.user,
             perm=NODE_PERMISSION.VIEW).order_by('-created')
+        if self.query:
+            try:
+                return constrain_nodes(nodes, _parse_constraints(self.query))
+            except InvalidConstraint as e:
+                self.query_error = e
+                return Node.objects.none()
+        return nodes
 
     def get_context_data(self, **kwargs):
         context = super(NodeListView, self).get_context_data(**kwargs)
         context.update(get_longpoll_context())
+        context["input_query"] = self.query
+        context["input_query_error"] = self.query_error
         return context