diff --git a/bread/__init__.py b/bread/__init__.py index 52f089c..143b639 100644 --- a/bread/__init__.py +++ b/bread/__init__.py @@ -1 +1 @@ -VERSION = '0.4.1' +VERSION = '0.5.0' diff --git a/bread/bread.py b/bread/bread.py index 67c734f..c36142e 100644 --- a/bread/bread.py +++ b/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: diff --git a/docs/changes.rst b/docs/changes.rst index ea0d27f..cad777e 100644 --- a/docs/changes.rst +++ b/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 -------------------- diff --git a/docs/configuration.rst b/docs/configuration.rst index 3a96ad3..3557ef9 100644 --- a/docs/configuration.rst +++ b/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 diff --git a/tests/factories.py b/tests/factories.py index f6c0f33..924163b 100644 --- a/tests/factories.py +++ b/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) diff --git a/tests/models.py b/tests/models.py index 3f16aa8..b8c3047 100644 --- a/tests/models.py +++ b/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'), diff --git a/tests/test_add.py b/tests/test_add.py index 0c3b591..8a9d494 100644 --- a/tests/test_add.py +++ b/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() diff --git a/tests/test_browse.py b/tests/test_browse.py index d4042a7..5d77bea 100644 --- a/tests/test_browse.py +++ b/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,6 +73,9 @@ 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) @@ -78,9 +83,11 @@ def test_sort_all_ascending(self, mock_logger): @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) diff --git a/tests/test_edit.py b/tests/test_edit.py index 37e3265..1b98ca1 100644 --- a/tests/test_edit.py +++ b/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() diff --git a/tests/test_forms.py b/tests/test_forms.py index 133ead3..edad624 100644 --- a/tests/test_forms.py +++ b/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() diff --git a/tests/test_utils.py b/tests/test_utils.py index 4dba331..6d59993 100644 --- a/tests/test_utils.py +++ b/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'))