From 0284a26af9d9adc58647df1a684b76969cf258e9 Mon Sep 17 00:00:00 2001 From: Hasan Ramezani Date: Fri, 15 Nov 2019 21:59:22 +0100 Subject: [PATCH] Fixed #30981 -- Fixed admin changelist crash when using F() or OrderBy() expressions in admin_order_field. --- django/contrib/admin/views/main.py | 7 +++- tests/admin_views/admin.py | 13 ++++++- tests/admin_views/tests.py | 61 +++++++++++++++++++----------- 3 files changed, 56 insertions(+), 25 deletions(-) diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py index 2f0e915d5e..e587d4ffca 100644 --- a/django/contrib/admin/views/main.py +++ b/django/contrib/admin/views/main.py @@ -308,7 +308,12 @@ class ChangeList: order_field = self.get_ordering_field(field_name) if not order_field: continue # No 'admin_order_field', skip it - if hasattr(order_field, 'as_sql'): + if isinstance(order_field, OrderBy): + if pfx == '-': + order_field = order_field.copy() + order_field.reverse_ordering() + ordering.append(order_field) + elif hasattr(order_field, 'resolve_expression'): # order_field is an expression. ordering.append(order_field.desc() if pfx == '-' else order_field.asc()) # reverse order if order_field has already "-" as prefix diff --git a/tests/admin_views/admin.py b/tests/admin_views/admin.py index 6a11d5b9bb..bf10151356 100644 --- a/tests/admin_views/admin.py +++ b/tests/admin_views/admin.py @@ -105,6 +105,7 @@ class ArticleAdmin(admin.ModelAdmin): 'content', 'date', callable_year, 'model_year', 'modeladmin_year', 'model_year_reversed', 'section', lambda obj: obj.title, 'order_by_expression', 'model_property_year', 'model_month', + 'order_by_f_expression', 'order_by_orderby_expression', ) list_editable = ('section',) list_filter = ('date', 'section') @@ -122,12 +123,20 @@ class ArticleAdmin(admin.ModelAdmin): }) ) + # These orderings aren't particularly useful but show that expressions can + # be used for admin_order_field. def order_by_expression(self, obj): return obj.model_year - # This ordering isn't particularly useful but shows that expressions can - # be used for admin_order_field. order_by_expression.admin_order_field = models.F('date') + datetime.timedelta(days=3) + def order_by_f_expression(self, obj): + return obj.model_year + order_by_f_expression.admin_order_field = models.F('date') + + def order_by_orderby_expression(self, obj): + return obj.model_year + order_by_orderby_expression.admin_order_field = models.F('date').asc(nulls_last=True) + def changelist_view(self, request): return super().changelist_view(request, extra_context={'extra_var': 'Hello!'}) diff --git a/tests/admin_views/tests.py b/tests/admin_views/tests.py index e833c44f95..c9ca64097f 100644 --- a/tests/admin_views/tests.py +++ b/tests/admin_views/tests.py @@ -364,30 +364,47 @@ class AdminViewBasicTest(AdminViewBasicTestCase): ) def test_change_list_sorting_callable_query_expression(self): - """ - Query expressions may be used for admin_order_field. (column 9 is - order_by_expression in ArticleAdmin). - """ - response = self.client.get(reverse('admin:admin_views_article_changelist'), {'o': '9'}) - self.assertContentBefore( - response, 'Oldest content', 'Middle content', - 'Results of sorting on callable are out of order.' - ) - self.assertContentBefore( - response, 'Middle content', 'Newest content', - 'Results of sorting on callable are out of order.' - ) + """Query expressions may be used for admin_order_field.""" + tests = [ + ('order_by_expression', 9), + ('order_by_f_expression', 12), + ('order_by_orderby_expression', 13), + ] + for admin_order_field, index in tests: + with self.subTest(admin_order_field): + response = self.client.get( + reverse('admin:admin_views_article_changelist'), + {'o': index}, + ) + self.assertContentBefore( + response, 'Oldest content', 'Middle content', + 'Results of sorting on callable are out of order.' + ) + self.assertContentBefore( + response, 'Middle content', 'Newest content', + 'Results of sorting on callable are out of order.' + ) def test_change_list_sorting_callable_query_expression_reverse(self): - response = self.client.get(reverse('admin:admin_views_article_changelist'), {'o': '-9'}) - self.assertContentBefore( - response, 'Middle content', 'Oldest content', - 'Results of sorting on callable are out of order.' - ) - self.assertContentBefore( - response, 'Newest content', 'Middle content', - 'Results of sorting on callable are out of order.' - ) + tests = [ + ('order_by_expression', -9), + ('order_by_f_expression', -12), + ('order_by_orderby_expression', -13), + ] + for admin_order_field, index in tests: + with self.subTest(admin_order_field): + response = self.client.get( + reverse('admin:admin_views_article_changelist'), + {'o': index}, + ) + self.assertContentBefore( + response, 'Middle content', 'Oldest content', + 'Results of sorting on callable are out of order.' + ) + self.assertContentBefore( + response, 'Newest content', 'Middle content', + 'Results of sorting on callable are out of order.' + ) def test_change_list_sorting_model(self): """