pagination: Fix quirks

There are a couple of pages where the clickable list of pages
would include missing or duplicate pages.

Write a test that ensures:
 - you always have a link to the next/prev numbered page
 - there are no duplicate page numbers

Fiddle with the pagination algorithm to get it to pass - required
tweaking a display parameter and a couple of comparison operators,
so all pretty minor.

Now, if there are 10 pages, the displayed page numbers for a given
page are as follows:

Page # | Displayed page #s
---------------------------
1      | [] [1, 2, 3, 4] [9, 10]
2      | [] [1, 2, 3, 4] [9, 10]
3      | [] [1, 2, 3, 4] [9, 10]
4      | [1, 2] [3, 4, 5] [9, 10]
5      | [1, 2] [4, 5, 6] [9, 10]
6      | [1, 2] [5, 6, 7] [9, 10]
7      | [1, 2] [6, 7, 8] [9, 10]
8      | [1, 2] [7, 8, 9, 10] []
9      | [1, 2] [7, 8, 9, 10] []
10     | [1, 2] [7, 8, 9, 10] []

Closes: #102
Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Stephen Finucane <stephen@that.guru>

diff --git a/patchwork/paginator.py b/patchwork/paginator.py
index e4cf556ebd96ca602dccbb242c567e2fb09b9cb5..359ec86793b7f0bfa43845987e0c143e0b8a49b1 100644 (file)
--- a/patchwork/paginator.py
+++ b/patchwork/paginator.py
@@ -28,7 +28,7 @@ from patchwork.compat import is_authenticated
 DEFAULT_ITEMS_PER_PAGE = 100
 LONG_PAGE_THRESHOLD = 30
 LEADING_PAGE_RANGE_DISPLAYED = 4
-TRAILING_PAGE_RANGE_DISPLAYED = 2
+TRAILING_PAGE_RANGE_DISPLAYED = 4
 LEADING_PAGE_RANGE = 4
 TRAILING_PAGE_RANGE = 2
 NUM_PAGES_OUTSIDE_RANGE = 2
@@ -69,12 +69,12 @@ class Paginator(paginator.Paginator):
         if pages <= LEADING_PAGE_RANGE_DISPLAYED:
             adjacent_start = 1
             adjacent_end = pages + 1
-        elif page_no <= LEADING_PAGE_RANGE:
+        elif page_no < LEADING_PAGE_RANGE:
             adjacent_start = 1
             adjacent_end = LEADING_PAGE_RANGE_DISPLAYED + 1
             self.leading_set = [n + pages for n in
                                 range(0, -NUM_PAGES_OUTSIDE_RANGE, -1)]
-        elif page_no > pages - TRAILING_PAGE_RANGE:
+        elif page_no >= pages - TRAILING_PAGE_RANGE:
             adjacent_start = pages - TRAILING_PAGE_RANGE_DISPLAYED + 1
             adjacent_end = pages + 1
             self.trailing_set = [n + 1 for n in

diff --git a/patchwork/tests/test_paginator.py b/patchwork/tests/test_paginator.py
index 2291eb058fe092a60ee69edf2a6a964e54b5592a..b2191bbb92d11fe38c92132060b89374f86396b5 100644 (file)
--- a/patchwork/tests/test_paginator.py
+++ b/patchwork/tests/test_paginator.py
@@ -66,6 +66,44 @@ class PaginatorTest(TestCase):
             self.assertEqual(response.context['page'].object_list[0].id,
                              self.patches[-page].id)
 
+    def test_navigation(self):
+        self.client.login(username=self.user.username,
+                          password=self.user.username)
+        for page_num in range(1, 11):
+            response = self._get_patches({'page': page_num})
+
+            page = response.context['page']
+            leading = page.paginator.leading_set
+            adjacent = page.paginator.adjacent_set
+            trailing = page.paginator.trailing_set
+
+            # if there is a prev page, it should be:
+            if page.has_previous():
+                self.assertEqual(page.previous_page_number(),
+                                 page_num - 1)
+                # ... either in the adjacent set or in the trailing set
+                if adjacent is not None:
+                    self.assertIn(page_num - 1, adjacent)
+                else:
+                    self.assertIn(page_num - 1, trailing)
+
+            # if there is a next page, it should be:
+            if page.has_next():
+                self.assertEqual(page.next_page_number(),
+                                 page_num + 1)
+                # ... either in the adjacent set or in the leading set
+                if adjacent is not None:
+                    self.assertIn(page_num + 1, adjacent)
+                else:
+                    self.assertIn(page_num + 1, leading)
+
+            # no page number should appear more than once
+            for x in adjacent:
+                self.assertNotIn(x, leading)
+                self.assertNotIn(x, trailing)
+            for x in leading:
+                self.assertNotIn(x, trailing)
+
     def test_page_invalid(self):
         self.client.login(username=self.user.username,
                           password=self.user.username)