Ticket #17198: 17198.changelist-order.3.diff

django/contrib/admin/views/main.py

@@ -66 +66 @@
             self.list_editable = ()
         else:
             self.list_editable = list_editable
-        self.ordering = self.get_ordering(request)
         self.query = request.GET.get(SEARCH_VAR, '')
         self.query_set = self.get_query_set(request)
         self.get_results(request)
@@ -218 +217 @@
                 attr = getattr(self.model, field_name)
             return getattr(attr, 'admin_order_field', None)
 
-    def get_ordering(self, request):
+    def get_ordering(self, request, queryset):
         params = self.params
-        # For ordering, first check the if exists the "get_ordering" method
-        # in model admin, then check "ordering" parameter in the admin
-        # options, then check the object's default ordering. Finally, a
+        # For ordering, first check the "get_ordering" method in model admin,
+        # then check the object's default ordering. Finally, a
         # manually-specified ordering from the query string overrides anything.
-        ordering = self.model_admin.get_ordering(request) or self._get_default_ordering()
+        ordering = list(self.model_admin.get_ordering(request)
+                        or self._get_default_ordering())
         if ORDER_VAR in params:
             # Clear ordering and used params
             ordering = []
@@ -239 +238 @@
                     ordering.append(pfx + order_field)
                 except (IndexError, ValueError):
                     continue # Invalid ordering specified, skip it.
+
+        # Add the given query's ordering fields, if any.
+        ordering.extend(queryset.query.order_by)
+
+        # Ensure that the primary key is systematically present in the list of
+        # ordering fields so we can guarantee a deterministic order.
+        pk_name = self.lookup_opts.pk.name
+        if not (set(ordering) & set(['pk', '-pk', pk_name, '-' + pk_name])):
+            # The two sets do not intersect, meaning the pk isn't present. So
+            # we add it.
+            ordering.append('pk')
+
         return ordering
 
     def get_ordering_field_columns(self):
@@ -322 +333 @@
                             break
 
         # Set ordering.
-        if self.ordering:
-            qs = qs.order_by(*self.ordering)
+        ordering = self.get_ordering(request, qs)
+        qs = qs.order_by(*ordering)
 
         # Apply keyword searches.
         def construct_search(field_name):

tests/regressiontests/admin_changelist/models.py

@@ -57 +57 @@
 
     class Meta:
         ordering = ('speed', 'load')
+
+
+class UnorderedObject(models.Model):
+    """
+    Model without any defined `Meta.ordering`.
+    Refs #17198.
+    """
+    bool = models.BooleanField(default=True)
+
+
+class OrderedObjectManager(models.Manager):
+    def get_query_set(self):
+        return super(OrderedObjectManager, self).get_query_set().order_by('-number')
+
+class OrderedObject(models.Model):
+    """
+    Model with Manager that defines a default order.
+    Refs #17198.
+    """
+    name = models.CharField(max_length=255)
+    bool = models.BooleanField(default=True)
+    number = models.IntegerField(default=0)
+
+    objects = OrderedObjectManager()
+ No newline at end of file

tests/regressiontests/admin_changelist/tests.py

@@ -14 +14 @@
     FilteredChildAdmin, CustomPaginator, site as custom_site,
     SwallowAdmin)
 from .models import (Child, Parent, Genre, Band, Musician, Group, Quartet,
-    Membership, ChordsMusician, ChordsBand, Invitation, Swallow)
+    Membership, ChordsMusician, ChordsBand, Invitation, Swallow,
+    UnorderedObject, OrderedObject)
 
 
 class ChangeListTests(TestCase):
@@ -430 +431 @@
         self.assertContains(response, unicode(swallow.load))
         self.assertContains(response, unicode(swallow.speed))
 
+    def test_deterministic_order(self):
+        """
+        Ensure that the primary key is systematically used in the ordering of
+        the changelist's results to guarantee a deterministic order.
+        Refs #17198.
+        """
+        superuser = self._create_superuser('superuser')
+
+        # ---------------------------------------------------------------------
+        # Firstly, when the Model doesn't have any default ordering defined
+        for counter in range(1, 51):
+            UnorderedObject.objects.create(id=counter, bool=True)
+
+        class UnorderedObjectAdmin(admin.ModelAdmin):
+            list_per_page = 10
+
+        def check_results_order(reverse=False):
+            admin.site.register(UnorderedObject, UnorderedObjectAdmin)
+            model_admin = UnorderedObjectAdmin(UnorderedObject, admin.site)
+            counter = 51 if reverse else 0
+            for page in range (0, 5):
+                request = self._mocked_authenticated_request('/unorderedobject/?p=%s' % page, superuser)
+                response = model_admin.changelist_view(request)
+                for result in response.context_data['cl'].result_list:
+                    counter += -1 if reverse else 1
+                    self.assertEqual(result.id, counter)
+            admin.site.unregister(UnorderedObject)
+
+        # When no order is defined at all, everything is ordered by 'pk'.
+        check_results_order()
+
+        # When an order field is defined but multiple records have the same
+        # value for that field, make sure everything gets ordered by pk as well.
+        UnorderedObjectAdmin.ordering = ['bool']
+        check_results_order()
+
+        # When order fields are defined, including the pk itself, use them.
+        UnorderedObjectAdmin.ordering = ['bool', '-pk']
+        check_results_order(reverse=True)
+        UnorderedObjectAdmin.ordering = ['bool', 'pk']
+        check_results_order()
+        UnorderedObjectAdmin.ordering = ['-id', 'bool']
+        check_results_order(reverse=True)
+        UnorderedObjectAdmin.ordering = ['id', 'bool']
+        check_results_order()
+
+        # ---------------------------------------------------------------------
+        # Secondly, when the Model has a manager that defines a default ordering
+        for counter in range(1, 51):
+            OrderedObject.objects.create(id=counter, bool=True, number=counter)
+
+        class OrderedObjectAdmin(admin.ModelAdmin):
+            list_per_page = 10
+
+        def check_results_order(reverse=False):
+            admin.site.register(OrderedObject, OrderedObjectAdmin)
+            model_admin = OrderedObjectAdmin(OrderedObject, admin.site)
+            counter = 51 if reverse else 0
+            for page in range (0, 5):
+                request = self._mocked_authenticated_request('/orderedobject/?p=%s' % page, superuser)
+                response = model_admin.changelist_view(request)
+                for result in response.context_data['cl'].result_list:
+                    counter += -1 if reverse else 1
+                    self.assertEqual(result.id, counter)
+            admin.site.unregister(OrderedObject)
+
+        # When no order is defined at all, use the model's default ordering (i.e. '-number')
+        check_results_order(reverse=True)
+
+        # When an order field is defined but multiple records have the same
+        # value for that field, make sure everything gets ordered by pk as well.
+        OrderedObjectAdmin.ordering = ['bool']
+        check_results_order()
+
+        # When order fields are defined, including the pk itself, use them.
+        OrderedObjectAdmin.ordering = ['bool', '-pk']
+        check_results_order(reverse=True)
+        OrderedObjectAdmin.ordering = ['bool', 'pk']
+        check_results_order()
+        OrderedObjectAdmin.ordering = ['-id', 'bool']
+        check_results_order(reverse=True)
+        OrderedObjectAdmin.ordering = ['id', 'bool']
+        check_results_order()
+ No newline at end of file

tests/regressiontests/admin_views/admin.py

@@ -26 +26 @@
     CoverLetter, Story, OtherStory, Book, Promo, ChapterXtra1, Pizza, Topping,
     Album, Question, Answer, ComplexSortedPerson, PrePopulatedPostLargeSlug,
     AdminOrderedField, AdminOrderedModelMethod, AdminOrderedAdminMethod,
-    AdminOrderedCallable, Report, Color2, MainPrepopulated, RelatedPrepopulated)
+    AdminOrderedCallable, Report, Color2, UnorderedObject, MainPrepopulated,
+    RelatedPrepopulated)
 
 
 def callable_year(dt_value):
@@ -562 +563 @@
                            'slug2': ['status', 'name']}
 
 
+class UnorderedObjectAdmin(admin.ModelAdmin):
+    list_display = ['name']
+    list_editable = ['name']
+    list_per_page = 2
+
 
 
 site = admin.AdminSite(name="admin")
@@ -609 +615 @@
 site.register(OtherStory, OtherStoryAdmin)
 site.register(Report, ReportAdmin)
 site.register(MainPrepopulated, MainPrepopulatedAdmin)
+site.register(UnorderedObject, UnorderedObjectAdmin)
 
 # We intentionally register Promo and ChapterXtra1 but not Chapter nor ChapterXtra2.
 # That way we cover all four cases:

tests/regressiontests/admin_views/models.py

@@ -596 +596 @@
         choices=(('option one', 'Option One'),
                  ('option two', 'Option Two')))
     slug1 = models.SlugField(max_length=50)
-    slug2 = models.SlugField(max_length=60)
- No newline at end of file
+    slug2 = models.SlugField(max_length=60)
+
+
+class UnorderedObject(models.Model):
+    """
+    Model without any defined `Meta.ordering`.
+    Refs #16819.
+    """
+    name = models.CharField(max_length=255)
+    bool = models.BooleanField(default=True)

tests/regressiontests/admin_views/tests.py

@@ -12 +12 @@
 from django.core.files import temp as tempfile
 from django.core.urlresolvers import reverse
 # Register auth models with the admin.
+from django.contrib import admin
 from django.contrib.admin.helpers import ACTION_CHECKBOX_NAME
 from django.contrib.admin.models import LogEntry, DELETION
 from django.contrib.admin.sites import LOGIN_FORM_KEY
@@ -41 +42 @@
     FoodDelivery, RowLevelChangePermissionModel, Paper, CoverLetter, Story,
     OtherStory, ComplexSortedPerson, Parent, Child, AdminOrderedField,
     AdminOrderedModelMethod, AdminOrderedAdminMethod, AdminOrderedCallable,
-    Report, MainPrepopulated, RelatedPrepopulated)
+    Report, MainPrepopulated, RelatedPrepopulated, UnorderedObject)
 
 
 ERROR_MESSAGE = "Please enter the correct username and password \
@@ -272 +273 @@
         )
 
     def testChangeListSortingPreserveQuerySetOrdering(self):
-        # If no ordering on ModelAdmin, or query string, the underlying order of
-        # the queryset should not be changed.
-
+        """
+        If no ordering is defined in `ModelAdmin.ordering` or in the query
+        string, then the underlying order of the queryset should not be
+        changed, even if it is defined in `Modeladmin.queryset()`.
+        Refs #11868, #7309.
+        """
         p1 = Person.objects.create(name="Amy", gender=1, alive=True, age=80)
         p2 = Person.objects.create(name="Bob", gender=1, alive=True, age=70)
         p3 = Person.objects.create(name="Chris", gender=2, alive=False, age=60)
@@ -1881 +1885 @@
         self.assertEqual(Category.objects.get(id=3).order, 1)
         self.assertEqual(Category.objects.get(id=4).order, 0)
 
+    def test_list_editable_ordering_default(self):
+        """
+        Ensure that the list_editable items on the changelist are ordered
+        in a deterministic order and without raising exceptions, even when
+        no default ordering is explicitly defined.
+        Refs #16819.
+        """
+        UnorderedObject.objects.create(id=1, name='Unordered object #1')
+        UnorderedObject.objects.create(id=2, name='Unordered object #2')
+        UnorderedObject.objects.create(id=3, name='Unordered object #3')
+        response = self.client.get('/test_admin/admin/admin_views/unorderedobject/')
+        self.assertContains(response, 'Unordered object #1')
+        self.assertContains(response, 'Unordered object #2')
+        self.assertNotContains(response, 'Unordered object #3')
+        response = self.client.get('/test_admin/admin/admin_views/unorderedobject/?p=1')
+        self.assertNotContains(response, 'Unordered object #1')
+        self.assertNotContains(response, 'Unordered object #2')
+        self.assertContains(response, 'Unordered object #3')
+
     def test_list_editable_action_submit(self):
         # List editable changes should not be executed if the action "Go" button is
         # used to submit the form.