launchpad-reviewers team mailing list archive

[John A Meinel <john@xxxxxxxxxxxxxxxxx>]

John A Meinel has proposed merging lp:~jameinel/maas/tag-view into lp:maas with lp:~jameinel/maas/node-view-has-tags as a prerequisite.

Commit message:
Add a basic view for Tags. Update the Node view to include a link to the Tag view.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jameinel/maas/tag-view/+merge/127152

This is built on my previous patch to expose tags on the Node view. It updates the node view to include the URL to the new Tag view. And then it shows the Tag with links back to the nodes.

I ended up just copying the 'node_list' listing for the nodes, which seems to be reasonable. I don't know that we want to show 'status' on the tag page, but I want to put something in the table, and moving '(hostname)' to another column means it doesn't match the expected nodes view.

-- 
https://code.launchpad.net/~jameinel/maas/tag-view/+merge/127152
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jameinel/maas/tag-view into lp:maas.

=== modified file 'src/maasserver/templates/maasserver/node_view.html'
--- src/maasserver/templates/maasserver/node_view.html	2012-09-30 13:16:30 +0000
+++ src/maasserver/templates/maasserver/node_view.html	2012-09-30 13:16:30 +0000
@@ -54,7 +54,8 @@
       <h4>Tags</h4>
       <span>
           {% for tag in node.tags.all %}
-          {{ tag }}{% if not forloop.last %}, {% endif %}
+            <a href="{% url 'tag-view' tag.name %}">{{ tag }}</a>
+            {% if not forloop.last %}, {% endif %}
           {% endfor %}
       </span>
     </li>

=== added file 'src/maasserver/templates/maasserver/tag_view.html'
--- src/maasserver/templates/maasserver/tag_view.html	1970-01-01 00:00:00 +0000
+++ src/maasserver/templates/maasserver/tag_view.html	2012-09-30 13:16:30 +0000
@@ -0,0 +1,71 @@
+{% extends "maasserver/base.html" %}
+
+{% block nav-active-settings %}active{% endblock %}
+{% block title %}Tag: {{ tag.name}}{% endblock %}
+{% block page-title %}Tag: {{ tag.name}}{% endblock %}
+
+{% block content %}
+  <ul class="data-list">
+    <li class="block size2 first">
+      <h4>Name</h4>
+      <span>{{ tag.name }}</span>
+    </li>
+    <li class="block size2">
+      <h4>Comment</h4>
+      <span>{{ tag.comment }}</span>
+    </li>
+    <li class="block size2">
+      <h4>Definition</h4>
+      <span>{{ tag.definition }}</span>
+    </li>
+    <li class="block size2 first">
+    </li>
+    {% if error_text %}
+    <li class="block first">
+      <h4>Error output</h4>
+      <span>{{ error_text }}</span>
+    </li>
+    {% endif %}
+    {% if status_text %}
+    <li class="block first">
+      <h4>Console output</h4>
+      <span>{{ status_text }}</span>
+    </li>
+    {% endif %}
+    <li class="block first">
+    <div id="nodes">
+      <h3>Nodes</h3>
+      {% 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 %}
+      {% if node_list|length %}
+        <table class="list">
+          <thead>
+            <tr>
+              <th>MAC</th>
+              <th>Status</th>
+            </tr>
+          </thead>
+          {% for node in node_list %}
+            <tr class="node {% cycle 'even' 'odd' %}">
+              <td>
+                <a href="{% url 'node-view' node.system_id %}">
+                  {% for macaddress in node.macaddress_set.reverse %}
+                    {{ macaddress }}{% if not forloop.last %},{% endif %}
+                  {% endfor %}
+                </a>
+                ({{ node.hostname }})
+              </td>
+              <td>{{ node.display_status }}</td>
+            </tr>
+          {% endfor %}
+        </table>
+      {% endif%}
+    </div>
+    </li>
+  </ul>
+{% endblock %}
+

=== modified file 'src/maasserver/tests/test_views_nodes.py'
--- src/maasserver/tests/test_views_nodes.py	2012-09-30 13:16:30 +0000
+++ src/maasserver/tests/test_views_nodes.py	2012-09-30 13:16:30 +0000
@@ -119,8 +119,10 @@
         content_text = doc.cssselect('#content')[0].text_content()
         self.assertIn(tag_a.name, content_text)
         self.assertIn(tag_b.name, content_text)
-        # tag_link = reverse('tags', args=[tag.name])
-        # self.assertIn(tag_link, get_content_links(response))
+        self.assertItemsEqual(
+            [reverse('tag-view', args=[t.name]) for t in (tag_a, tag_b)],
+            [link for link in get_content_links(response)
+                if link.startswith('/tags/')])
 
     def test_view_node_displays_node_info_no_owner(self):
         # If the node has no owner, the Owner 'slot' does not exist.

=== modified file 'src/maasserver/tests/test_views_tags.py'
--- src/maasserver/tests/test_views_tags.py	2012-09-30 13:16:30 +0000
+++ src/maasserver/tests/test_views_tags.py	2012-09-30 13:16:30 +0000
@@ -35,4 +35,43 @@
 
 class TagViewsTest(LoggedInTestCase):
 
-    pass
+    def test_view_tag_displays_tag_info(self):
+        # The tag page features the basic information about the tag.
+        tag = factory.make_tag(name='the-named-tag',
+                               comment='Human description of the tag',
+                               definition='//xpath')
+        tag_link = reverse('tag-view', args=[tag.name])
+        response = self.client.get(tag_link)
+        doc = fromstring(response.content)
+        content_text = doc.cssselect('#content')[0].text_content()
+        self.assertIn(tag.name, content_text)
+        self.assertIn(tag.comment, content_text)
+        self.assertIn(tag.definition, content_text)
+
+    def test_view_tag_includes_node_links(self):
+        tag = factory.make_tag()
+        node = factory.make_node(set_hostname=True)
+        node.tags.add(tag)
+        mac = factory.make_mac_address(node=node).mac_address
+        tag_link = reverse('tag-view', args=[tag.name])
+        node_link = reverse('node-view', args=[node.system_id])
+        response = self.client.get(tag_link)
+        doc = fromstring(response.content)
+        content_text = doc.cssselect('#content')[0].text_content()
+        self.assertIn('(%s)' % node.hostname, content_text)
+        self.assertIn(mac, content_text)
+        self.assertNotIn(node.system_id, content_text)
+        self.assertIn(node_link, get_content_links(response))
+
+    def test_view_tag_hides_private_nodes(self):
+        tag = factory.make_tag()
+        node = factory.make_node(set_hostname=True)
+        node2 = factory.make_node(owner=factory.make_user(), set_hostname=True)
+        node.tags.add(tag)
+        node2.tags.add(tag)
+        tag_link = reverse('tag-view', args=[tag.name])
+        response = self.client.get(tag_link)
+        doc = fromstring(response.content)
+        content_text = doc.cssselect('#content')[0].text_content()
+        self.assertIn(node.hostname, content_text)
+        self.assertNotIn(node2.hostname, content_text)

=== modified file 'src/maasserver/views/nodes.py'
--- src/maasserver/views/nodes.py	2012-09-30 13:16:30 +0000
+++ src/maasserver/views/nodes.py	2012-06-22 09:45:30 +0000
@@ -165,7 +165,6 @@
             node.error if node.status == NODE_STATUS.FAILED_TESTS else None)
         context['status_text'] = (
             node.error if node.status != NODE_STATUS.FAILED_TESTS else None)
-        context['tag_names'] = node.tags
         return context
 
     def dispatch(self, *args, **kwargs):

=== modified file 'src/maasserver/views/tags.py'
--- src/maasserver/views/tags.py	2012-09-30 13:16:30 +0000
+++ src/maasserver/views/tags.py	2012-09-30 13:16:30 +0000
@@ -15,19 +15,31 @@
     ]
 
 from maasserver.models import Tag
-
-
-class TagView:
+from django.views.generic import (
+    # ListView,
+    UpdateView,
+    )
+
+
+class TagView(UpdateView):
     """Basic view of a tag.
     """
 
+    template_name = 'maasserver/tag_view.html'
     context_object_name = 'tag'
 
     def get_object(self):
-        system_id = self.kwargs.get('name', None)
+        name = self.kwargs.get('name', None)
         tag = Tag.objects.get_tag_or_404(
             name=name, user=self.request.user,
             to_edit=False)
         return tag
 
+    def get_context_data(self, **kwargs):
+        context = super(TagView, self).get_context_data(**kwargs)
+        context['node_list'] = Tag.objects.get_nodes(self.kwargs['name'],
+                                                     self.request.user)
+        return context
 
+    def get_success_url(self):
+        return reverse('tag-view', args=[self.get_object().name])