Merge pull request #70 from caktus/stabilize_pagination

Stabilize pagination

bread/__init__.py

@@ -1 +1 @@
-VERSION = '0.4.1'
+VERSION = '0.5.0'

bread/bread.py

@@ -263,6 +263,12 @@ def get_queryset(self):
                         % (column_number, self._valid_sorting_columns))
                 order_by.append('%s%s' %
                                 (prefix, self.get_sort_field_name_for_column(column_number)))
+            # Add any ordering from the model's Meta data that isn't already included.
+            # That will make the rest of the sort stable, if the model has some default sort order.
+            order_by_without_leading_dashes = [x.lstrip('-') for x in order_by]
+            for order_spec in qset.model._meta.ordering:
+                if order_spec.lstrip('-') not in order_by_without_leading_dashes:
+                    order_by.append(order_spec)
             qset = qset.order_by(*order_by)
             # Validate those parms
             try:

docs/changes.rst

@@ -3,6 +3,12 @@
 Change Log
 ==========
 
+0.5.0 - Nov 30, 2017
+--------------------
+
+* Use models' default ordering to make pagination more stable
+  when user is sorting.
+
 0.4.0 - Oct 31, 2017
 --------------------
 

docs/configuration.rst

@@ -187,6 +187,13 @@ sorting
     The default browse template will include sort controls on the column headers
     for columns that are sortable.
 
+    It's a good idea to define a default ordering in the model's ``Meta`` class.
+    After applying any sort columns specified by the user, Bread will add on any
+    default orderings not already mentioned. That will result in the overall sort
+    being stable, which is important if you want pagination to be sensible.
+    (Otherwise, every time we show a new page, we could be working off a different
+    sorting of the results!)  If nothing else, include a sort on the primary key.
+
     Configuring the browse view:
 
     If the second item in the ``columns`` entry for a column is not a valid specification

tests/factories.py

@@ -14,6 +14,7 @@ class BreadTestModelFactory(factory.DjangoModelFactory):
     FACTORY_FOR = BreadTestModel
 
     name = factory.fuzzy.FuzzyText(length=10)
+    age = factory.fuzzy.FuzzyInteger(low=1, high=99)
     other = factory.SubFactory(BreadTestModel2Factory)
 
 

tests/models.py

@@ -41,13 +41,17 @@ def get_text(self):
 @python_2_unicode_compatible
 class BreadTestModel(models.Model):
     name = models.CharField(max_length=10)
+    age = models.IntegerField()
     other = models.ForeignKey(
         BreadTestModel2, blank=True, null=True,
         on_delete=models.CASCADE,
     )
 
     class Meta:
-        ordering = ['name']
+        ordering = [
+            'name',
+            '-age',  # If same name, sort oldest first
+        ]
         permissions = [
             ('read_breadtestmodel', 'can read BreadTestModel'),
             ('browse_breadtestmodel', 'can browse BreadTestModel'),

tests/test_add.py

@@ -24,7 +24,7 @@ def setUp(self):
     def test_new_item(self):
         self.model.objects.all().delete()
         url = reverse(self.bread.get_url_name('add'))
-        request = self.request_factory.post(url, data={'name': 'Fred Jones'})
+        request = self.request_factory.post(url, data={'name': 'Fred Jones', 'age': '19'})
         request.user = self.user
         self.give_permission('add')
         view = self.bread.get_add_view()
@@ -37,7 +37,8 @@ def test_new_item(self):
     def test_fail_validation(self):
         self.model.objects.all().delete()
         url = reverse(self.bread.get_url_name('add'))
-        request = self.request_factory.post(url, data={'name': 'this name is too much long yeah'})
+        request = self.request_factory.post(
+            url, data={'name': 'this name is too much long yeah', 'age': '19'})
         request.user = self.user
         self.give_permission('add')
         view = self.bread.get_add_view()

tests/test_browse.py

@@ -55,9 +55,11 @@ def test_post(self):
     @patch('bread.templatetags.bread_tags.logger')
     def test_sort_all_ascending(self, mock_logger):
         self.set_urls(self.bread)
-        BreadTestModelFactory(name='999', other__text='012')
-        BreadTestModelFactory(name='555', other__text='333')
-        BreadTestModelFactory(name='111', other__text='555')
+        BreadTestModelFactory(name='999', other__text='012', age=50)
+        BreadTestModelFactory(name='555', other__text='333', age=60)
+        BreadTestModelFactory(name='111', other__text='555', age=10)
+        BreadTestModelFactory(name='111', other__text='555', age=20)
+        BreadTestModelFactory(name='111', other__text='555', age=5)
         self.give_permission('browse')
         url = reverse(self.bread.get_url_name('browse')) + '?o=0,1'
         request = self.request_factory.get(url)
@@ -71,16 +73,21 @@ def test_sort_all_ascending(self, mock_logger):
             sortA = (results[i].name, results[i].other.text)
             sortB = (results[i+1].name, results[i+1].other.text)
             self.assertLessEqual(sortA, sortB)
+            if sortA == sortB:
+                # default sort is '-age'
+                self.assertGreaterEqual(results[i].age, results[i+1].age)
             i += 1
         # No exceptions logged
         self.assertFalse(mock_logger.exception.called)
 
     @patch('bread.templatetags.bread_tags.logger')
     def test_sort_all_descending(self, mock_logger):
         self.set_urls(self.bread)
-        BreadTestModelFactory(name='999', other__text='012')
-        BreadTestModelFactory(name='555', other__text='333')
-        BreadTestModelFactory(name='111', other__text='555')
+        BreadTestModelFactory(name='999', other__text='012', age=50)
+        BreadTestModelFactory(name='555', other__text='333', age=60)
+        BreadTestModelFactory(name='111', other__text='555', age=10)
+        BreadTestModelFactory(name='111', other__text='555', age=20)
+        BreadTestModelFactory(name='111', other__text='555', age=5)
         self.give_permission('browse')
         url = reverse(self.bread.get_url_name('browse')) + '?o=-0,-1'
         request = self.request_factory.get(url)
@@ -94,6 +101,9 @@ def test_sort_all_descending(self, mock_logger):
             sortA = (results[i].name, results[i].other.text)
             sortB = (results[i+1].name, results[i+1].other.text)
             self.assertGreaterEqual(sortA, sortB)
+            if sortA == sortB:
+                # default sort is '-age'
+                self.assertGreaterEqual(results[i].age, results[i+1].age)
             i += 1
         # No exceptions logged
         self.assertFalse(mock_logger.exception.called)

tests/test_edit.py

@@ -16,7 +16,7 @@ def setUp(self):
     def test_edit_item(self):
         item = self.model_factory()
         url = reverse(self.bread.get_url_name('edit'), kwargs={'pk': item.pk})
-        request = self.request_factory.post(url, data={'name': 'Fred Jones'})
+        request = self.request_factory.post(url, data={'name': 'Fred Jones', 'age': '19'})
         request.user = self.user
         self.give_permission('change')
         view = self.bread.get_edit_view()
@@ -29,7 +29,8 @@ def test_edit_item(self):
     def test_fail_validation(self):
         item = self.model_factory()
         url = reverse(self.bread.get_url_name('edit'), kwargs={'pk': item.pk})
-        request = self.request_factory.post(url, data={'name': 'this name is too much long yeah'})
+        request = self.request_factory.post(
+            url, data={'name': 'this name is too much long yeah', 'age': '19'})
         request.user = self.user
         self.give_permission('change')
         view = self.bread.get_edit_view()

tests/test_forms.py

@@ -13,7 +13,7 @@ class TestForm(forms.ModelForm):
     # It only allows names that start with 'Dan'
     class Meta:
         model = BreadTestModel
-        fields = ['name']
+        fields = ['name', 'age']
 
     def clean_name(self):
         name = self.cleaned_data['name']
@@ -34,7 +34,7 @@ def setUp(self):
     def test_new_item(self):
         self.model.objects.all().delete()
         url = reverse(self.bread.get_url_name('add'))
-        request = self.request_factory.post(url, data={'name': 'Dan Jones'})
+        request = self.request_factory.post(url, data={'name': 'Dan Jones', 'age': '19'})
         request.user = self.user
         self.give_permission('add')
         view = self.bread.get_add_view()
@@ -47,7 +47,7 @@ def test_new_item(self):
     def test_fail_validation(self):
         self.model.objects.all().delete()
         url = reverse(self.bread.get_url_name('add'))
-        request = self.request_factory.post(url, data={'name': 'Fred'})
+        request = self.request_factory.post(url, data={'name': 'Fred', 'age': '19'})
         request.user = self.user
         self.give_permission('add')
         view = self.bread.get_add_view()
@@ -86,7 +86,7 @@ def setUp(self):
     def test_edit_item(self):
         item = self.model_factory()
         url = reverse(self.bread.get_url_name('edit'), kwargs={'pk': item.pk})
-        request = self.request_factory.post(url, data={'name': 'Dan Jones'})
+        request = self.request_factory.post(url, data={'name': 'Dan Jones', 'age': '19'})
         request.user = self.user
         self.give_permission('change')
         view = self.bread.get_edit_view()
@@ -99,7 +99,7 @@ def test_edit_item(self):
     def test_fail_validation(self):
         item = self.model_factory()
         url = reverse(self.bread.get_url_name('edit'), kwargs={'pk': item.pk})
-        request = self.request_factory.post(url, data={'name': 'Fred'})
+        request = self.request_factory.post(url, data={'name': 'Fred', 'age': '19'})
         request.user = self.user
         self.give_permission('change')
         view = self.bread.get_edit_view()

tests/test_utils.py

@@ -80,7 +80,7 @@ def test_it(self):
             text="Rhinocerous"
         )
         obj1 = BreadTestModel.objects.create(
-            name="Rudy Vallee", other=obj2
+            name="Rudy Vallee", other=obj2, age=72
         )
         self.assertEqual(obj1.name, get_model_field(obj1, 'name'))
         self.assertEqual(obj1.name, get_model_field(obj1, 'get_name'))