launchpad-reviewers team mailing list archive

[Martin Packman <martin.packman@xxxxxxxxxxxxx>]

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

Use same pagination logic on tag view page as the nodes listing view has.

Note this changes the view to be oriented around Node, with Tag as an extra, rather than the reverse. In practice this is fine as none of the features of the old UpdateView.
-- 
https://code.launchpad.net/~gz/maas/tag_view_pagination/+merge/130322
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gz/maas/tag_view_pagination into lp:maas.

=== modified file 'src/maasserver/templates/maasserver/tag_view.html'
--- src/maasserver/templates/maasserver/tag_view.html	2012-10-03 03:59:53 +0000
+++ src/maasserver/templates/maasserver/tag_view.html	2012-10-18 11:29:21 +0000
@@ -33,8 +33,9 @@
     {% endif %}
   </ul>
   <div id="nodes" class="block first pad-top">
-    <h2 class="block first line-top pad-top">Nodes</h2>
+    <h2 class="block first line-top pad-top">{{ paginator.count }} Node{{ paginator.count|pluralize }}</h2>
     {% include "maasserver/nodes_listing.html" %}
+    {% include "maasserver/pagination.html" %}
   </div>
 {% endblock %}
 

=== modified file 'src/maasserver/tests/test_views_tags.py'
--- src/maasserver/tests/test_views_tags.py	2012-10-10 11:29:26 +0000
+++ src/maasserver/tests/test_views_tags.py	2012-10-18 11:29:21 +0000
@@ -13,6 +13,7 @@
 __all__ = []
 
 from django.core.urlresolvers import reverse
+from lxml.etree import XPath
 from lxml.html import fromstring
 from maastesting.matchers import ContainsAll
 from maasserver.testing import (
@@ -22,6 +23,7 @@
 from maasserver.testing.testcase import (
     LoggedInTestCase,
     )
+from maasserver.views import tags as tags_views
 
 
 class TagViewsTest(LoggedInTestCase):
@@ -90,3 +92,47 @@
         content_text = doc.cssselect('#content')[0].text_content()
         self.assertIn(node.hostname, content_text)
         self.assertNotIn(node2.hostname, content_text)
+
+    def test_view_tag_paginates_nodes(self):
+        """Listing of nodes with tag is split across multiple pages
+
+        Copy-coded from NodeViewsTest.test_node_list_paginates evilly.
+        """
+        # Set a very small page size to save creating lots of nodes
+        page_size = 2
+        self.patch(tags_views.TagView, 'paginate_by', page_size)
+        tag = factory.make_tag()
+        nodes = [factory.make_node(created="2012-10-12 12:00:%02d" % i)
+            for i in range(page_size * 2 + 1)]
+        for node in nodes:
+            node.tags = [tag]
+        # Order node links with newest first as the view is expected to
+        node_links = [reverse('node-view', args=[node.system_id])
+            for node in reversed(nodes)]
+        expr_node_links = XPath("//div[@id='nodes']/table//a/@href")
+        expr_page_anchors = XPath("//div[@class='pagination']//a")
+        # Fetch first page, should link newest two nodes and page 2
+        response = self.client.get(reverse('tag-view', args=[tag.name]))
+        page1 = fromstring(response.content)
+        self.assertEqual(node_links[:page_size], expr_node_links(page1))
+        self.assertEqual([("next", "?page=2"), ("last", "?page=3")],
+            [(a.text.lower(), a.get("href"))
+                for a in expr_page_anchors(page1)])
+        # Fetch second page, should link next nodes and adjacent pages
+        response = self.client.get(reverse('tag-view', args=[tag.name]),
+            {"page": 2})
+        page2 = fromstring(response.content)
+        self.assertEqual(node_links[page_size:page_size * 2],
+            expr_node_links(page2))
+        self.assertEqual([("first", "."), ("previous", "."),
+                ("next", "?page=3"), ("last", "?page=3")],
+            [(a.text.lower(), a.get("href"))
+                for a in expr_page_anchors(page2)])
+        # Fetch third page, should link oldest node and node list page
+        response = self.client.get(reverse('tag-view', args=[tag.name]),
+            {"page": 3})
+        page3 = fromstring(response.content)
+        self.assertEqual(node_links[page_size * 2:], expr_node_links(page3))
+        self.assertEqual([("first", "."), ("previous", "?page=2")],
+            [(a.text.lower(), a.get("href"))
+                for a in expr_page_anchors(page3)])

=== modified file 'src/maasserver/views/tags.py'
--- src/maasserver/views/tags.py	2012-10-10 09:43:17 +0000
+++ src/maasserver/views/tags.py	2012-10-18 11:29:21 +0000
@@ -15,28 +15,29 @@
     ]
 
 from maasserver.models import Tag
-from django.views.generic import (
-    UpdateView,
-    )
-
-
-class TagView(UpdateView):
+from maasserver.views import PaginatedListView
+
+
+class TagView(PaginatedListView):
     """Basic view of a tag.
     """
 
     template_name = 'maasserver/tag_view.html'
-    context_object_name = 'tag'
+    context_object_name = 'node_list'
 
-    def get_object(self):
-        name = self.kwargs.get('name', None)
-        tag = Tag.objects.get_tag_or_404(
-            name=name, user=self.request.user,
+    def get(self, request, *args, **kwargs):
+        self.tag = Tag.objects.get_tag_or_404(
+            name=kwargs.get('name', None),
+            user=self.request.user,
             to_edit=False)
-        return tag
+        return super(TagView, self).get(request, *args, **kwargs)
+
+    def get_queryset(self):
+        return Tag.objects.get_nodes(
+            self.tag, user=self.request.user, prefetch_mac=True,
+            ).order_by('-created')
 
     def get_context_data(self, **kwargs):
         context = super(TagView, self).get_context_data(**kwargs)
-        nodes = Tag.objects.get_nodes(context['tag'], self.request.user,
-            prefetch_mac=True)
-        context['node_list'] = nodes
+        context['tag'] = self.tag
         return context