launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #13360
[Merge] lp:~gz/maas/paginate_nodes_page_1064672 into lp:maas
Martin Packman has proposed merging lp:~gz/maas/paginate_nodes_page_1064672 into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~gz/maas/paginate_nodes_page_1064672/+merge/129409
Adds pagination to the node list page, that works for both unfiltered and query views.
Uses the existing template added by Huw and adds logic to it, including a helper ListView subclass to make getting the page links correct simpler. At the moment only one set of page links is shown, at the bottom, adding one line to template would be enough to duplicate at the top.
Includes drive by fix to page title to include "matching" for query results, as a change was needed anyway to give the full count rather than just the number of results on the page.
The magic number 50 is what I went for on nodes per page. If anyone has strong feelings about a different value, it's easy enough to change, but exposing it to user customisation does not strike me as necessary.
The tests are a little horrible, and I'm not sure how much I can escape django when trying to tests just a view class, but not using a QuerySet does seem to work.
This branch does not include changes to the tag page which can also try listing a huge number of nodes.
== Relative url cheat sheet ==
href="" current link including query and fragment if present
href="." removes the last path component if present
href="x" replaces the last path component
href="?x=y" keeps the current path but replaces the current query
Play around with urlparse.urljoin for more.
--
https://code.launchpad.net/~gz/maas/paginate_nodes_page_1064672/+merge/129409
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gz/maas/paginate_nodes_page_1064672 into lp:maas.
=== modified file 'src/maasserver/templates/maasserver/node_list.html'
--- src/maasserver/templates/maasserver/node_list.html 2012-10-05 17:17:02 +0000
+++ src/maasserver/templates/maasserver/node_list.html 2012-10-12 12:38:20 +0000
@@ -2,7 +2,7 @@
{% block nav-active-node-list %}active{% endblock %}
{% block title %}Nodes{% endblock %}
-{% block page-title %}{{ node_list|length }} node{{ node_list|length|pluralize }} in {% include "maasserver/site_title.html" %}{% endblock %}
+{% block page-title %}{{ page_obj.paginator.count }}{% if input_query %} matching{% endif %} node{{ page_obj.paginator.count|pluralize }} in {% include "maasserver/site_title.html" %}{% endblock %}
{% block site-switcher %}{% endblock %}
{% block header-search %}{% endblock %}
@@ -39,6 +39,7 @@
<p class="form-errors">{{input_query_error}}</p>
{% endif %}
{% include "maasserver/nodes_listing.html" %}
+ {% include "maasserver/pagination.html" %}
<a id="addnode" href="#" class="button right space-top">+ Add node</a>
<div class="clear"></div>
<a class="right space-top" href="{% url "enlist-preseed-view" %}">View enlistment preseed</a>
=== modified file 'src/maasserver/templates/maasserver/pagination.html'
--- src/maasserver/templates/maasserver/pagination.html 2012-02-22 04:16:32 +0000
+++ src/maasserver/templates/maasserver/pagination.html 2012-10-12 12:38:20 +0000
@@ -1,7 +1,19 @@
-<div class="pagination">
- <span class="inactive">FIRST</span>
- <span class="inactive">PREVIOUS</span>
- <span>1-50 of 237</span>
- <a href="">NEXT</a>
- <a href="">LAST</a>
-</div>
+{% if is_paginated %}
+ <div class="pagination">
+ {% if page_obj.has_previous %}
+ <a href="{{first_page_link}}">FIRST</a>
+ <a href="{{previous_page_link}}">PREVIOUS</a>
+ {% else %}
+ <span class="inactive">FIRST</span>
+ <span class="inactive">PREVIOUS</span>
+ {% endif %}
+ <span>{{page_obj.start_index}}-{{page_obj.end_index}} of {{paginator.count}}</span>
+ {% if page_obj.has_next %}
+ <a href="{{next_page_link}}">NEXT</a>
+ <a href="{{last_page_link}}">LAST</a>
+ {% else %}
+ <span class="inactive">NEXT</span>
+ <span class="inactive">LAST</span>
+ {% endif %}
+ </div>
+{% endif %}
=== modified file 'src/maasserver/tests/test_views.py'
--- src/maasserver/tests/test_views.py 2012-09-28 15:32:06 +0000
+++ src/maasserver/tests/test_views.py 2012-10-12 12:38:20 +0000
@@ -31,7 +31,10 @@
LoggedInTestCase,
TestCase,
)
-from maasserver.views import HelpfulDeleteView
+from maasserver.views import (
+ HelpfulDeleteView,
+ PaginatedListView,
+ )
from maasserver.views.nodes import NodeEdit
from maastesting.matchers import ContainsAll
@@ -242,6 +245,106 @@
view.compose_feedback_deleted(view.obj))
+class SimpleFakeModel:
+ """Pretend model object for testing"""
+
+ def __init__(self, counter):
+ self.id = counter
+
+
+class SimpleListView(PaginatedListView):
+ """Simple paginated view for testing"""
+
+ paginate_by = 2
+
+ def __init__(self, query_results):
+ self._query_results = list(query_results)
+
+ def get_queryset(self):
+ """Return precanned list of objects
+
+ Really this should return a QuerySet object, but for basic usage a
+ list is close enough.
+ """
+ return self._query_results
+
+
+class PaginatedListViewTests(TestCase):
+ """Check PaginatedListView page links inserted into context are correct"""
+
+ def test_single_page(self):
+ view = SimpleListView([SimpleFakeModel(1)])
+ request = RequestFactory().get('/index')
+ response = view.dispatch(request)
+ context = response.context_data
+ self.assertEqual("", context["first_page_link"])
+ self.assertEqual("", context["previous_page_link"])
+ self.assertEqual("", context["next_page_link"])
+ self.assertEqual("", context["last_page_link"])
+
+ def test_on_first_page(self):
+ view = SimpleListView([SimpleFakeModel(i) for i in range(5)])
+ request = RequestFactory().get('/index')
+ response = view.dispatch(request)
+ context = response.context_data
+ self.assertEqual("", context["first_page_link"])
+ self.assertEqual("", context["previous_page_link"])
+ self.assertEqual("?page=2", context["next_page_link"])
+ self.assertEqual("?page=3", context["last_page_link"])
+
+ def test_on_second_page(self):
+ view = SimpleListView([SimpleFakeModel(i) for i in range(7)])
+ request = RequestFactory().get('/index?page=2')
+ response = view.dispatch(request)
+ context = response.context_data
+ self.assertEqual("index", context["first_page_link"])
+ self.assertEqual("index", context["previous_page_link"])
+ self.assertEqual("?page=3", context["next_page_link"])
+ self.assertEqual("?page=4", context["last_page_link"])
+
+ def test_on_final_page(self):
+ view = SimpleListView([SimpleFakeModel(i) for i in range(5)])
+ request = RequestFactory().get('/index?page=3')
+ response = view.dispatch(request)
+ context = response.context_data
+ self.assertEqual("index", context["first_page_link"])
+ self.assertEqual("?page=2", context["previous_page_link"])
+ self.assertEqual("", context["next_page_link"])
+ self.assertEqual("", context["last_page_link"])
+
+ def test_relative_to_directory(self):
+ view = SimpleListView([SimpleFakeModel(i) for i in range(6)])
+ request = RequestFactory().get('/index/?page=2')
+ response = view.dispatch(request)
+ context = response.context_data
+ self.assertEqual(".", context["first_page_link"])
+ self.assertEqual(".", context["previous_page_link"])
+ self.assertEqual("?page=3", context["next_page_link"])
+ self.assertEqual("?page=3", context["last_page_link"])
+
+ def test_preserves_query_string(self):
+ view = SimpleListView([SimpleFakeModel(i) for i in range(6)])
+ request = RequestFactory().get('/index?lookup=value')
+ response = view.dispatch(request)
+ context = response.context_data
+ self.assertEqual("", context["first_page_link"])
+ self.assertEqual("", context["previous_page_link"])
+ # Does this depend on dict hash values for order or does django sort?
+ self.assertEqual("?lookup=value&page=2", context["next_page_link"])
+ self.assertEqual("?lookup=value&page=3", context["last_page_link"])
+
+ def test_preserves_query_string_with_page(self):
+ view = SimpleListView([SimpleFakeModel(i) for i in range(8)])
+ request = RequestFactory().get('/index?page=3&lookup=value')
+ response = view.dispatch(request)
+ context = response.context_data
+ self.assertEqual("?lookup=value", context["first_page_link"])
+ # Does this depend on dict hash values for order or does django sort?
+ self.assertEqual("?lookup=value&page=2", context["previous_page_link"])
+ self.assertEqual("?lookup=value&page=4", context["next_page_link"])
+ self.assertEqual("?lookup=value&page=4", context["last_page_link"])
+
+
class MAASExceptionHandledInView(LoggedInTestCase):
def test_raised_MAASException_redirects(self):
=== modified file 'src/maasserver/tests/test_views_nodes.py'
--- src/maasserver/tests/test_views_nodes.py 2012-10-11 12:03:35 +0000
+++ src/maasserver/tests/test_views_nodes.py 2012-10-12 12:38:20 +0000
@@ -17,6 +17,7 @@
from django.conf import settings
from django.core.urlresolvers import reverse
+from lxml.etree import XPath
from lxml.html import fromstring
from maasserver import messages
import maasserver.api
@@ -423,6 +424,63 @@
node_links = document.xpath("//div[@id='nodes']/table//a/@href")
self.assertEqual([node2_link], node_links)
+ def test_node_list_paginates(self):
+ """Node listing is split across multiple pages with links"""
+ # Set a very small page size to save creating lots of nodes
+ page_size = 2
+ self.patch(nodes_views.NodeListView, 'paginate_by', page_size)
+ nodes = [factory.make_node(created="2012-10-12 12:00:%02d" % i)
+ for i in range(page_size * 2 + 1)]
+ # 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('node-list'))
+ 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('node-list'), {"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('node-list'), {"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)])
+
+ def test_node_list_query_paginates(self):
+ """Node list query subset is split across multiple pages with links"""
+ # 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": "maas-tags=odd", "page": 3})
+ document = fromstring(response.content)
+ self.assertIn("5 matching nodes", document.xpath("string(//h1)"))
+ self.assertEqual([last_node_link],
+ document.xpath("//div[@id='nodes']/table//a/@href"))
+ self.assertEqual([("first", "?query=maas-tags%3Dodd"),
+ ("previous", "?query=maas-tags%3Dodd&page=2")],
+ [(a.text.lower(), a.get("href"))
+ for a in document.xpath("//div[@class='pagination']//a")])
+
class NodePreseedViewTest(LoggedInTestCase):
=== modified file 'src/maasserver/views/__init__.py'
--- src/maasserver/views/__init__.py 2012-04-23 14:02:51 +0000
+++ src/maasserver/views/__init__.py 2012-10-12 12:38:20 +0000
@@ -12,6 +12,7 @@
__metaclass__ = type
__all__ = [
"HelpfulDeleteView",
+ "PaginatedListView",
"process_form",
"AccountsEdit",
"AccountsView",
@@ -29,7 +30,10 @@
Http404,
HttpResponseRedirect,
)
-from django.views.generic import DeleteView
+from django.views.generic import (
+ DeleteView,
+ ListView,
+ )
class HelpfulDeleteView(DeleteView):
@@ -110,6 +114,46 @@
return HttpResponseRedirect(self.get_next_url())
+class PaginatedListView(ListView):
+ """Paginating extension to :class:`django.views.generic.ListView`
+
+ Adds to the normal list view pagination support by including context
+ variables for relative links to other pages, correctly preserving the
+ existing query string and path.
+ """
+
+ paginate_by = 50
+
+ def _make_page_link(self, page_number):
+ new_query = self.request.GET.copy()
+ if page_number == 1:
+ if "page" in new_query:
+ del new_query["page"]
+ else:
+ new_query["page"] = str(page_number)
+ if not new_query:
+ return self.request.path.rsplit("/", 1)[-1] or "."
+ return "?" + new_query.urlencode()
+
+ def get_context_data(self, **kwargs):
+ context = super(PaginatedListView, self).get_context_data(**kwargs)
+ page_obj = context["page_obj"]
+ if page_obj.has_previous():
+ context["first_page_link"] = self._make_page_link(1)
+ context["previous_page_link"] = self._make_page_link(
+ page_obj.previous_page_number())
+ else:
+ context["first_page_link"] = context["previous_page_link"] = ""
+ if page_obj.has_next():
+ context["next_page_link"] = self._make_page_link(
+ page_obj.next_page_number())
+ context["last_page_link"] = self._make_page_link(
+ page_obj.paginator.num_pages)
+ else:
+ context["next_page_link"] = context["last_page_link"] = ""
+ return context
+
+
def process_form(request, form_class, redirect_url, prefix,
success_message=None, form_kwargs=None):
"""Utility method to process subforms (i.e. forms with a prefix).
=== modified file 'src/maasserver/views/nodes.py'
--- src/maasserver/views/nodes.py 2012-10-10 09:43:17 +0000
+++ src/maasserver/views/nodes.py 2012-10-12 12:38:20 +0000
@@ -35,7 +35,6 @@
from django.views.generic import (
CreateView,
DetailView,
- ListView,
UpdateView,
)
from maasserver.enum import (
@@ -64,7 +63,10 @@
get_enlist_preseed,
get_preseed,
)
-from maasserver.views import HelpfulDeleteView
+from maasserver.views import (
+ HelpfulDeleteView,
+ PaginatedListView,
+ )
def get_longpoll_context():
@@ -101,7 +103,7 @@
return constraints
-class NodeListView(ListView):
+class NodeListView(PaginatedListView):
context_object_name = "node_list"
