Author: julien
Date: 2012-03-02 18:13:35 -0800 (Fri, 02 Mar 2012)
New Revision: 17635
Modified:
django/trunk/django/contrib/admin/views/main.py
django/trunk/tests/regressiontests/admin_changelist/models.py
django/trunk/tests/regressiontests/admin_changelist/tests.py
django/trunk/tests/regressiontests/admin_views/admin.py
django/trunk/tests/regressiontests/admin_views/models.py
django/trunk/tests/regressiontests/admin_views/tests.py
Log:
Fixed #17198 -- Ensured that a deterministic order is used across all database
backends for displaying the admin change list's results. Many thanks to Luke
Plant for the report and general approach, to everyone involved in the design
discussions, and to Carl Meyer for the patch review. Refs #16819.
Modified: django/trunk/django/contrib/admin/views/main.py
===================================================================
--- django/trunk/django/contrib/admin/views/main.py 2012-03-03 01:06:37 UTC
(rev 17634)
+++ django/trunk/django/contrib/admin/views/main.py 2012-03-03 02:13:35 UTC
(rev 17635)
@@ -66,7 +66,6 @@
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,13 +217,18 @@
attr = getattr(self.model, field_name)
return getattr(attr, 'admin_order_field', None)
- def get_ordering(self, request):
+ def get_ordering(self, request, queryset):
+ """
+ Returns the list of ordering fields for the change list.
+ First we check the get_ordering() method in model admin, then we check
+ the object's default ordering. Then, any manually-specified ordering
+ from the query string overrides anything. Finally, a deterministic
+ order is guaranteed by ensuring the primary key is used as the last
+ ordering field.
+ """
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
- # 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,6 +243,19 @@
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 across all
+ # database backends.
+ 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,8 +339,8 @@
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):
Modified: django/trunk/tests/regressiontests/admin_changelist/models.py
===================================================================
--- django/trunk/tests/regressiontests/admin_changelist/models.py
2012-03-03 01:06:37 UTC (rev 17634)
+++ django/trunk/tests/regressiontests/admin_changelist/models.py
2012-03-03 02:13:35 UTC (rev 17635)
@@ -57,3 +57,27 @@
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
Modified: django/trunk/tests/regressiontests/admin_changelist/tests.py
===================================================================
--- django/trunk/tests/regressiontests/admin_changelist/tests.py
2012-03-03 01:06:37 UTC (rev 17634)
+++ django/trunk/tests/regressiontests/admin_changelist/tests.py
2012-03-03 02:13:35 UTC (rev 17635)
@@ -14,7 +14,8 @@
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,3 +431,92 @@
self.assertContains(response, unicode(swallow.load))
self.assertContains(response, unicode(swallow.speed))
+ def test_deterministic_order_for_unordered_model(self):
+ """
+ Ensure that the primary key is systematically used in the ordering of
+ the changelist's results to guarantee a deterministic order, even
+ when the Model doesn't have any default ordering defined.
+ Refs #17198.
+ """
+ superuser = self._create_superuser('superuser')
+
+ 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()
+
+ def test_deterministic_order_for_model_ordered_by_its_manager(self):
+ """
+ Ensure that the primary key is systematically used in the ordering of
+ the changelist's results to guarantee a deterministic order, even
+ when the Model has a manager that defines a default ordering.
+ Refs #17198.
+ """
+ superuser = self._create_superuser('superuser')
+
+ 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
Modified: django/trunk/tests/regressiontests/admin_views/admin.py
===================================================================
--- django/trunk/tests/regressiontests/admin_views/admin.py 2012-03-03
01:06:37 UTC (rev 17634)
+++ django/trunk/tests/regressiontests/admin_views/admin.py 2012-03-03
02:13:35 UTC (rev 17635)
@@ -26,7 +26,8 @@
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,8 +563,13 @@
'slug2': ['status', 'name']}
+class UnorderedObjectAdmin(admin.ModelAdmin):
+ list_display = ['name']
+ list_editable = ['name']
+ list_per_page = 2
+
site = admin.AdminSite(name="admin")
site.register(Article, ArticleAdmin)
site.register(CustomArticle, CustomArticleAdmin)
@@ -609,6 +615,7 @@
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:
Modified: django/trunk/tests/regressiontests/admin_views/models.py
===================================================================
--- django/trunk/tests/regressiontests/admin_views/models.py 2012-03-03
01:06:37 UTC (rev 17634)
+++ django/trunk/tests/regressiontests/admin_views/models.py 2012-03-03
02:13:35 UTC (rev 17635)
@@ -596,4 +596,13 @@
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)
Modified: django/trunk/tests/regressiontests/admin_views/tests.py
===================================================================
--- django/trunk/tests/regressiontests/admin_views/tests.py 2012-03-03
01:06:37 UTC (rev 17634)
+++ django/trunk/tests/regressiontests/admin_views/tests.py 2012-03-03
02:13:35 UTC (rev 17635)
@@ -12,6 +12,7 @@
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,7 +42,7 @@
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,9 +273,12 @@
)
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,6 +1885,23 @@
self.assertEqual(Category.objects.get(id=3).order, 1)
self.assertEqual(Category.objects.get(id=4).order, 0)
+ def test_list_editable_pagination(self):
+ """
+ Ensure that pagination works for list_editable items.
+ 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.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-updates?hl=en.
