Merge lp:~julian-edwards/maas/better-search into lp:~maas-committers/maas/trunk

[Julian Edwards]

Add an extra search box to the node listing page that allows users to do substring searches that match on hostname, architecture and tags.

I suck at CSS and it shows, so please help me out here! There's a couple of obvious problems with the new layout of the boxes (please run up the branch to see).

 1. The magnifying glass icon moves up a pixel when you hover over it! But only on one of the search boxes... :/

 2. The cross-to-cancel click doesn't work on the "test constraints" box, I think I messed up with the layout somehow but I don't know how.

Otherwise, this does exactly what it says on the tin.

[Julian Edwards (julian-edwards)]

Pretty please someone?

[Raphaël Badin (rvb)]

The new search box is really badly positioned on Firefox (it overlaps with the other search box): http://people.canonical.com/~rvb/search_overlap.png

[Raphaël Badin (rvb)]

This is the result of searching for 'u' on the sample data: http://people.canonical.com/~rvb/multiple_nodes.png. Some lines are duplicated!

review: Needs Fixing

[Raphaël Badin (rvb)]

Not sure if it's a regression this branch introduced or not but the sorting parameters are not preserved when you use the search box:
- sort on a field
- search for something
- the ordering is lost

[Christian Reis (kiko)]

Is this a part of the UX work that is being planned post-1.7?

review: Needs Information

[Julian Edwards (julian-edwards)]

On Thursday 30 Oct 2014 11:50:31 you wrote:
> Review: Needs Information
>
> Is this a part of the UX work that is being planned post-1.7?

No, it was a bug that Andres wanted fixed.

[Julian Edwards (julian-edwards)]

On Thursday 30 Oct 2014 11:10:40 you wrote:
> Review: Needs Fixing
>
> This is the result of searching for 'u' on the sample data:
> http://people.canonical.com/~rvb/multiple_nodes.png. Some lines are
> duplicated!

Woops, needs a unique! Thanks for spotting.

[Julian Edwards (julian-edwards)]

On Thursday 30 Oct 2014 10:49:42 you wrote:
> The new search box is really badly positioned on Firefox (it overlaps with
> the other search box): http://people.canonical.com/~rvb/search_overlap.png

I hate CSS. I mean, I really hate it. :/

[Christian Reis (kiko)]

> No, it was a bug that Andres wanted fixed.

Okay, then we shouldn't do this without aligning with the wider UX plan.

[Julian Edwards (julian-edwards)]

On Thursday 30 Oct 2014 13:25:30 you wrote:
> > No, it was a bug that Andres wanted fixed.
>
> Okay, then we shouldn't do this without aligning with the wider UX plan.

I really don't think that should block this, we have had people screaming for
a proper search for a long time. The aesthetics are orthogonal to the
underlying search and can be trivially tweaked later - by someone who knows
what they're doing with CSS of course :)

[Julian Edwards (julian-edwards)]

On Thursday 30 October 2014 11:10:40 you wrote:
> Review: Needs Fixing
>
> This is the result of searching for 'u' on the sample data:
> http://people.canonical.com/~rvb/multiple_nodes.png. Some lines are
> duplicated!

FWIW, I can't get the sampledata to work when I load it from fresh, it looks
like it needs fixing, it causes the appserver to crash when clicking on nodes,
and the names in the node list are all "#### INVALID STRING ####"

[Julian Edwards (julian-edwards)]

Also regarding sorting being preserved, it's like that in trunk AFAICT.

[Julian Edwards (julian-edwards)]

Ok Raph, I've fixed the duplication, but I'll be buggered if I can get the CSS working. When I change it to fix one browser, the other screws up. I think I need some help here, if you are able?

[Christian Reis (kiko)]

Julian, have you looked at Blake's search work which he shared via the list? Looks like the two of you should sync.

[Raphaël Badin (rvb)]

> Woops, needs a unique! Thanks for spotting.

It needs a unique indeed. But it has nothing to do with the union:
This is a way to trigger the problem: http://paste.ubuntu.com/8803036/ (fwiw, this is the query this generates http://paste.ubuntu.com/8803039/).

The problem is that querying across manytomany relationships gives you an effective join. Hence the duplication. This makes perfect sense from an SQL point of view but from an ORM point of view (i.e. Nodes.objects.filter()), it's a bit weird, I'll give you that.

Also, you can make the code is a tad nicer: http://paste.ubuntu.com/8803384/

[Raphaël Badin (rvb)]

> When I change it to fix one browser, the other screws up. I think I need some help here, if you are able?

I don't see the overlap I was seeing last week, I guess you've fixed this.

The only UI problem that I see is the jumping magnifying glass… and the only way I managed to fix the problem is by doing this: http://paste.ubuntu.com/8804644/.

[Raphaël Badin (rvb)]

This looks good to me. This is a massive improvement to the usability of MAAS. I'm not entirely sure we should have the two search boxes though. It's looks weird and I'm not sure anyone uses the constraints search box since it isn't documented anywhere.

So I'd be +1 on removing it after we make sure its usage is non-existant. But this is something we can do after this lands.

>> Okay, then we shouldn't do this without aligning with the wider UX plan.

> [...] The aesthetics are orthogonal to the underlying search.

Agreed. I even think it's best if we land this now to force the UX redesign to take this into account.

review: Approve

[Christian Reis (kiko)]

On Mon, Nov 03, 2014 at 04:06:07PM -0000, Raphaël Badin wrote:
> Agreed. I even think it's best if we land this now to force the UX
> redesign to take this into account.

Won't this directly conflict with what Blake is working on?

[Julian Edwards (julian-edwards)]

On Monday 03 November 2014 12:50:53 you wrote:
> Julian, have you looked at Blake's search work which he shared via the list?
> Looks like the two of you should sync.

I did. My change is vastly simpler and I'd like to land this now because it
solves a specific problem without requiring a new JS toolkit and UI redesign.

It doesn't preclude any work that Blake is doing, however.

[Julian Edwards (julian-edwards)]

On Monday 03 November 2014 18:18:21 you wrote:
> On Mon, Nov 03, 2014 at 04:06:07PM -0000, Raphaël Badin wrote:
> > Agreed. I even think it's best if we land this now to force the UX
> > redesign to take this into account.
>
> Won't this directly conflict with what Blake is working on?

Not really, his is nowhere near landing.

[Julian Edwards (julian-edwards)]

On Monday 03 November 2014 16:00:34 you wrote:
> > When I change it to fix one browser, the other screws up. I think I need
> > some help here, if you are able?
> I don't see the overlap I was seeing last week, I guess you've fixed this.

I did. Copious overriding of CSS properties!

> The only UI problem that I see is the jumping magnifying glass… and the only
> way I managed to fix the problem is by doing this:

> http://paste.ubuntu.com/8804644/.

I worked out what's causing it in the end, I can just remove the padding in
the input that it's inheriting from some top-level CSS class. The hover makes
the padding move! Weird.

Also the other problem is that the input boxes are different lengths on FF and
Chrom*. FF increases the size of the box when you left-pad the text to make
room for the icon; Chrom* does not. Consistency eh?!

I need to fix one more thing too, the margins are different sizes on each box
(again, css inheritance...).

[Julian Edwards (julian-edwards)]

On Monday 03 November 2014 16:06:07 you wrote:
> Review: Approve

Thanks.

> This looks good to me. This is a massive improvement to the usability of
> MAAS. I'm not entirely sure we should have the two search boxes though.
> It's looks weird and I'm not sure anyone uses the constraints search box
> since it isn't documented anywhere.

I'm not sure either, but I left it *just in case*.

> So I'd be +1 on removing it after we make sure its usage is non-existant.
> But this is something we can do after this lands.

Yeah, let's ask around.

> >> Okay, then we shouldn't do this without aligning with the wider UX plan.
> >
> > [...] The aesthetics are orthogonal to the underlying search.
>
> Agreed. I even think it's best if we land this now to force the UX redesign
> to take this into account.

+1

[Julian Edwards (julian-edwards)]

Bouncing icon fixed - the CSS was being overridden in the HTML page but not for the hover attribute... fun!

Now, I've sorted all of that and the FF page looks great. Chrom* looks good, except the shorter input boxes and that the cursor is now off centre in the box until you start typing in it!

You just can't f'in win.

=== modified file 'src/maasserver/static/css/components/search_box.css'
--- src/maasserver/static/css/components/search_box.css	2014-02-25 07:27:05 +0000
+++ src/maasserver/static/css/components/search_box.css	2014-11-04 01:15:28 +0000
@@ -2,21 +2,42 @@
     position: absolute;
     top: 0;
     right: 0;
+    width: 210px;
+    height: 33px;
+    }
+
+form.search-test {
+    position: absolute;
+    top: 0;
+    right: 220px;
     width: 420px;
-    height: 26px;
+    height: 33px;
     }
 
 #header form.search {
     position: relative;
-    width: 228px;
+    width: 420px;
     margin: 18px 15px 0 0;
     }
 
+input.search-test {
+    position: relative;
+    top: 0;
+    left: -20px;
+    width: 420px;
+    height: 33px;
+    padding: 0;
+    padding-left: 22px;
+    background-color: #fff;
+    }
+
 input.search-input {
-    position: absolute;
+    position: relative;
     top: 0;
-    left: 0;
-    width: 420px;
+    left: 0px;
+    width: 186px;
+    height: 33px;
+    padding: 0;
     padding-left: 22px;
     background-color: #fff;
     }
@@ -29,10 +50,34 @@
     border-radius: 4px;
     }
 
+#header input.search-test-input {
+    width: 225px;
+    border: none;
+    -moz-border-radius: 4px;
+    -webkit-border-radius: 4px;
+    border-radius: 4px;
+    }
+
+input.search-test-submit,
+input.search-test-submit:hover {
+    position: absolute;
+    top: 4px;
+    margin-top: 3px;
+    left: -19px;
+    width: 24px;
+    height: 24px;
+    padding: 0;
+    margin: 0;
+    background: transparent url(../?img/search_icon.png) no-repeat center center;
+    border: none;
+    font-size: 0;
+    cursor: pointer;
+    }
+
 input.search-submit,
 input.search-submit:hover {
     position: absolute;
-    top: 1px;
+    top: 4px;
     left: 1px;
     width: 24px;
     height: 24px;
@@ -41,8 +86,5 @@
     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/templates/maasserver/node_list.html'
--- src/maasserver/templates/maasserver/node_list.html	2014-10-08 08:37:42 +0000
+++ src/maasserver/templates/maasserver/node_list.html	2014-11-04 01:15:28 +0000
@@ -2,7 +2,7 @@
 
 {% block nav-active-node-list %}active{% endblock %}
 {% block title %}Nodes{% endblock %}
-{% block page-title %}{{ paginator.count }}{% if input_query %} matching{% endif %} node{{ paginator.count|pluralize }} in {% include "maasserver/site_title.html" %}{% endblock %}
+{% block page-title %}{{ paginator.count }}{% if input_query or input_test %} matching{% endif %} node{{ paginator.count|pluralize }} in {% include "maasserver/site_title.html" %}{% endblock %}
 {% block header-search %}{% endblock %}
 
 {% block html_includes %}{% include "maasserver/snippets.html" %}
@@ -27,14 +27,13 @@
       display:none;
   }
 
-  input.search-input {
+  input.search-input,
+  input.search-test {
     line-height: 23px;
     outline: none;
   }
 
-  input.search-submit {
-    margin-top: 3px;
-  }
+
   </style>
   <script type="text/javascript">
   <!--
@@ -101,12 +100,20 @@
 
 {% block content %}
   <div id="nodes" style="position: relative;">
+    <form id="node_listing_constraint_form"
+          action="{% url 'node-list' %}?{{preserved_query}}"
+          method="get" class="search-test">
+      <input type="search" name="test" placeholder="Test Juju constraints"
+             class="search-test" value="{{input_test|default_if_none:''}}" />
+      <input id="search_button" type="submit"
+             class="search-test-submit" />
+    </form>
     <form id="node_listing_search_form"
           action="{% url 'node-list' %}?{{preserved_query}}"
           method="get" class="search">
       <input type="search" name="query" placeholder="Search nodes"
              class="search-input" value="{{input_query|default_if_none:''}}" />
-      <input id="search_button" type="submit" value="Search"
+      <input id="search_button" type="submit"
              class="search-submit" />
     </form>
 

=== modified file 'src/maasserver/views/nodes.py'
--- src/maasserver/views/nodes.py	2014-10-31 07:11:08 +0000
+++ src/maasserver/views/nodes.py	2014-11-04 01:15:28 +0000
@@ -35,6 +35,7 @@
 from django.contrib import messages
 from django.core.exceptions import PermissionDenied
 from django.core.urlresolvers import reverse
+from django.db.models import Q
 from django.http import QueryDict
 from django.shortcuts import (
     get_object_or_404,
@@ -228,6 +229,7 @@
 
     def populate_modifiers(self, request):
         self.query = request.GET.get("query")
+        self.test_query = request.GET.get("test")
         self.query_error = None
         self.sort_by = request.GET.get("sort")
         self.sort_dir = request.GET.get("dir")
@@ -253,7 +255,7 @@
         These are sorting and search option we want a POST request to
         preserve so that the display after a POST request is similar
         to the display before the request."""
-        return ["dir", "query", "page", "sort"]
+        return ["dir", "query", "test", "page", "sort"]
 
     def get_preserved_query(self):
         params = {
@@ -323,7 +325,7 @@
         :param nodes_query: A query set of nodes.
         :return: A query set of nodes that returns a subset of `nodes_query`.
         """
-        data = _parse_constraints(self.query)
+        data = _parse_constraints(self.test_query)
         form = AcquireNodeForm.Strict(data=data)
         # Change the field names of the AcquireNodeForm object to
         # conform to Juju's naming.
@@ -336,12 +338,30 @@
                  for field, errors in form.errors.items()])
             return Node.objects.none()
 
+    def _search_nodes(self, nodes_query):
+        """Filter the given nodes query by searching field data.
+
+        The search query is substring matched non-case sensitively
+        against some fields in the nodes.
+
+        :param nodes_query: A queryset of nodes.
+        :return: A query set of nodes that returns a subset of `nodes_query`.
+        """
+        name_clause = Q(hostname__icontains=self.query)
+        arch_clause = Q(architecture__icontains=self.query)
+        tag_clause = Q(tags__name__icontains=self.query)
+
+        query = nodes_query.filter(name_clause | arch_clause | tag_clause)
+        return query.distinct()
+
     def get_queryset(self):
         nodes = Node.objects.get_nodes(
             user=self.request.user, perm=NODE_PERMISSION.VIEW)
         nodes = nodes.order_by(*self._compose_sort_order())
+        if self.test_query:
+            nodes = self._constrain_nodes(nodes)
         if self.query:
-            nodes = self._constrain_nodes(nodes)
+            nodes = self._search_nodes(nodes)
         nodes = prefetch_nodes_listing(nodes)
         return configure_macs(nodes)
 
@@ -391,6 +411,7 @@
         context["preserved_query"] = self.get_preserved_query()
         context["form"] = form
         context["input_query"] = self.query
+        context["input_test"] = self.test_query
         context["input_query_error"] = self.query_error
         links, classes = self._prepare_sort_links()
         context["sort_links"] = links

=== modified file 'src/maasserver/views/tests/test_nodes.py'
--- src/maasserver/views/tests/test_nodes.py	2014-10-31 07:11:08 +0000
+++ src/maasserver/views/tests/test_nodes.py	2014-11-04 01:15:28 +0000
@@ -94,7 +94,9 @@
 from provisioningserver.utils.enum import map_enum
 from provisioningserver.utils.text import normalise_whitespace
 from testtools.matchers import (
+    Contains,
     ContainsAll,
+    Equals,
     HasLength,
     Not,
     )
@@ -1029,31 +1031,31 @@
             "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):
+    def test_node_list_constraint_test_query_includes_current(self):
         self.client_log_in()
         qs = factory.make_string()
-        response = self.client.get(reverse('node-list'), {"query": qs})
+        response = self.client.get(reverse('node-list'), {"test": qs})
         query_value = fromstring(response.content).xpath(
-            "string(//div[@id='nodes']//input[@name='query']/@value)")
+            "string(//div[@id='nodes']//input[@name='test']/@value)")
         self.assertIn(qs, query_value)
 
-    def test_node_list_query_error_on_missing_tag(self):
+    def test_node_list_constraint_test_query_error_on_missing_tag(self):
         self.client_log_in()
         response = self.client.get(
-            reverse('node-list'), {"query": "maas-tags=missing"})
+            reverse('node-list'), {"test": "maas-tags=missing"})
         error_string = fromstring(response.content).xpath(
             "string(//div[@id='nodes']//p[@class='form-errors'])")
         self.assertIn("No such tag(s): 'missing'", error_string)
 
-    def test_node_list_query_error_on_unknown_constraint(self):
+    def test_node_list_constraint_test_query_error_on_unknown_constraint(self):
         self.client_log_in()
         response = self.client.get(
-            reverse('node-list'), {"query": "color=red"})
+            reverse('node-list'), {"test": "color=red"})
         error_string = fromstring(response.content).xpath(
             "string(//div[@id='nodes']//p[@class='form-errors'])")
         self.assertEqual("color: No such constraint.", error_string.strip())
 
-    def test_node_list_query_selects_subset(self):
+    def test_node_list_constraint_test_query_selects_subset(self):
         self.client_log_in()
         tag = factory.make_Tag("shiny")
         node1 = factory.make_Node(cpu_count=1)
@@ -1063,7 +1065,7 @@
         node2.tags = [tag]
         node3.tags = []
         response = self.client.get(
-            reverse('node-list'), {"query": "maas-tags=shiny cpu=2"})
+            reverse('node-list'), {"test": "maas-tags=shiny cpu=2"})
         node2_link = reverse('node-view', args=[node2.system_id])
         document = fromstring(response.content)
         node_links = document.xpath(
@@ -1116,7 +1118,7 @@
             [(a.text.lower(), a.get("href"))
              for a in expr_page_anchors(page3)])
 
-    def test_node_list_query_paginates(self):
+    def test_node_list_constraint_test_query_paginates(self):
         """Node list query subset is split across multiple pages with links"""
         self.client_log_in()
         # Set a very small page size to save creating lots of nodes
@@ -1130,7 +1132,7 @@
         last_node_link = reverse('node-view', args=[nodes[0].system_id])
         response = self.client.get(
             reverse('node-list'),
-            {"query": "maas-tags=odd", "page": 3})
+            {"test": "maas-tags=odd", "page": 3})
         document = fromstring(response.content)
         self.assertIn("5 matching nodes", document.xpath("string(//h1)"))
         self.assertEqual(
@@ -1138,8 +1140,8 @@
             document.xpath("//div[@id='nodes']/form/table/tr/td[3]/a/@href"))
         self.assertEqual(
             [
-                ("first", "?query=maas-tags%3Dodd"),
-                ("previous", "?query=maas-tags%3Dodd&page=2")
+                ("first", "?test=maas-tags%3Dodd"),
+                ("previous", "?test=maas-tags%3Dodd&page=2")
             ],
             [
                 (a.text.lower(), a.get("href"))
@@ -1369,6 +1371,128 @@
         self.assertIn(node_event_list, get_content_links(response))
 
 
+class TestNodesViewSearch(MAASServerTestCase):
+
+    def test_node_list_search_query_includes_current_clause(self):
+        self.client_log_in()
+        qs = factory.make_string()
+        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_search_query_finds_by_hostname(self):
+        self.client_log_in()
+        [factory.make_Node() for i in range(10)]
+        hostname = factory.make_name("hostname")
+        node = factory.make_Node(hostname=hostname)
+        node2 = factory.make_Node(hostname=hostname[:-1])
+        node_link = reverse('node-view', args=[node.system_id])
+        node2_link = reverse('node-view', args=[node2.system_id])
+        response = self.client.get(reverse('node-list'), {"query": hostname})
+        document = fromstring(response.content)
+        node_links = document.xpath(
+            "//div[@id='nodes']/form/table/tr/td[3]/a/@href")
+        self.expectThat(node_links, Equals([node_link]))
+
+        # A substring search also matches.  Here it also picks up the
+        # other node with the exact name match.
+        response = self.client.get(
+            reverse('node-list'), {"query": hostname[:-1]})
+        document = fromstring(response.content)
+        node_links = document.xpath(
+            "//div[@id='nodes']/form/table/tr/td[3]/a/@href")
+        self.expectThat(node_links, Equals([node_link, node2_link]))
+
+    def test_node_list_search_query_finds_by_arch(self):
+        self.client_log_in()
+        amd64_node = factory.make_Node(architecture="amd64/generic")
+        factory.make_Node(architecture="i386/generic")
+        factory.make_Node(architecture="armhf/generic")
+
+        node_link = reverse('node-view', args=[amd64_node.system_id])
+        response = self.client.get(reverse('node-list'), {"query": "amd64"})
+        document = fromstring(response.content)
+        node_links = document.xpath(
+            "//div[@id='nodes']/form/table/tr/td[3]/a/@href")
+        self.expectThat(node_links, Equals([node_link]))
+
+    def test_node_list_search_doesnt_show_duplicates(self):
+        # Unioning several query sets can result in dupes, check that
+        # the query uses distinct.
+        self.client_log_in()
+        node = factory.make_Node(
+            hostname="arthur", architecture="amd64/generic")
+        node.tags = [factory.make_Tag() for i in range(3)]
+        node.save()
+        nodes = [node]
+        nodes.append(factory.make_Node(
+            hostname="andrew", architecture="i386/generic"))
+        response = self.client.get(reverse('node-list'), {"query": "a"})
+        document = fromstring(response.content)
+        expected_node_links = [
+            reverse('node-view', args=[n.system_id]) for n in nodes]
+        doc_node_links = document.xpath(
+            "//div[@id='nodes']/form/table/tr/td[3]/a/@href")
+        self.assertItemsEqual(expected_node_links, doc_node_links)
+
+    def test_node_list_search_query_finds_by_tags(self):
+        self.client_log_in()
+        nodes = [factory.make_Node() for i in range(10)]
+        tag = factory.make_Tag("odd")
+        for node in nodes[::2]:
+            node.tags = [tag]
+        response = self.client.get(
+            reverse('node-list'), {"query": "odd"})
+        document = fromstring(response.content)
+        node_links = document.xpath(
+            "//div[@id='nodes']/form/table/tr/td[3]/a/@href")
+        expected_node_links = [
+            reverse('node-view', args=[node.system_id])
+            for node in nodes[::2]]
+        self.expectThat(node_links, ContainsAll(expected_node_links))
+
+        # Substrings match tags too.
+        response = self.client.get(
+            reverse('node-list'), {"query": "od"})
+        document = fromstring(response.content)
+        node_links = document.xpath(
+            "//div[@id='nodes']/form/table/tr/td[3]/a/@href")
+        self.expectThat(node_links, ContainsAll(expected_node_links))
+
+    def test_node_list_search_query_paginates(self):
+        """Node list query subset is split across multiple pages with links"""
+        self.client_log_in()
+        # Set a very small page size to save creating lots of nodes
+        self.patch(nodes_views.NodeListView, 'paginate_by', 2)
+        nodes = [
+            factory.make_Node(created="2012-10-12 12:00:%02d" % i)
+            for i in range(10)]
+        tag = factory.make_Tag("odd")
+        for node in nodes[::2]:
+            node.tags = [tag]
+        last_node_link = reverse('node-view', args=[nodes[0].system_id])
+        response = self.client.get(
+            reverse('node-list'), {"query": "odd", "page": 3})
+        document = fromstring(response.content)
+        self.expectThat(
+            document.xpath("string(//h1)"), Contains("5 matching nodes"))
+        self.expectThat(
+            [last_node_link],
+            Equals(
+                document.xpath(
+                    "//div[@id='nodes']/form/table/tr/td[3]/a/@href")))
+        self.assertEqual(
+            [
+                ("first", "?query=odd"),
+                ("previous", "?query=odd&page=2")
+            ],
+            [
+                (a.text.lower(), a.get("href"))
+                for a in document.xpath("//div[@class='pagination']//a")
+            ])
+
+
 class TestWarnUnconfiguredIPAddresses(MAASServerTestCase):
 
     def test__warns_for_IPv6_address_on_non_ubuntu_OS(self):