[Django] #25396: ModelAdmin.get_queryset doesn't accept ValueQuerySet
Django
-------------------------------+--------------------
Reporter: dex4er | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
Django Admin crashes for following code:
models.py
{{{
from django.db import models
from django.contrib.auth.models import User
class Category(models.Model):
name = models.CharField(max_length=100)
class Meta:
verbose_name_plural = 'categories'
def __unicode__(self):
return self.name
class Note(models.Model):
user = models.ForeignKey(User)
category = models.ForeignKey(Category)
keywords = models.CharField(max_length=400)
title = models.CharField(max_length=200)
date = models.DateTimeField()
content = models.TextField(max_length=50000)
def __unicode__(self):
return self.title
class UserNotesReport(models.Model):
user = models.CharField(max_length=30, primary_key=True)
category = models.CharField(max_length=100, primary_key=True)
n = models.IntegerField()
}}}
admin.py
{{{
from django.contrib import admin
import hello_blog.models as models
from django.db.models import Count
class CategoryAdmin(admin.ModelAdmin):
pass
admin.site.register(models.Category, CategoryAdmin)
class NoteAdmin(admin.ModelAdmin):
pass
admin.site.register(models.Note, NoteAdmin)
class UserNotesReportAdmin(admin.ModelAdmin):
actions = None
list_display = ['user__username', 'category__name', 'n']
list_display_links = None
readonly_fields = ['user', 'category', 'n']
def user__username(self):
pass
def category__name(self):
pass
def get_queryset(self, request):
qs = models.Note.objects.order_by('user',
'category').values('user__username',
'category__name').annotate(n=Count('*'))
print(qs.query)
return qs
def has_add_permission(self, request):
return False
admin.site.register(models.UserNotesReport, UserNotesReportAdmin)
}}}
The fix is trivial:
{{{
diff --git a/django/contrib/admin/templatetags/admin_list.py
b/django/contrib/admin/templatetags/admin_list.py
index f4033e2..9e70f8c 100644
--- a/django/contrib/admin/templatetags/admin_list.py
+++ b/django/contrib/admin/templatetags/admin_list.py
@@ -199,7 +199,10 @@ def items_for_result(cl, result, form):
empty_value_display = cl.model_admin.get_empty_value_display()
row_classes = ['field-%s' % field_name]
try:
- f, attr, value = lookup_field(field_name, result,
cl.model_admin)
+ if isinstance(result, dict):
+ f, attr, value = None, field_name, result.get(field_name)
+ else:
+ f, attr, value = lookup_field(field_name, result,
cl.model_admin)
except ObjectDoesNotExist:
result_repr = empty_value_display
else:
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25396>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: dex4er | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
This looks like a duplicate of #24387 to me.
From what I understand you are adding this explicit `values()` call to
GROUP BY related username and category?
--
Ticket URL: <https://code.djangoproject.com/ticket/25396#comment:1>
Django
Reporter: dex4er | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by dex4er):
Oh, it is probably the duplicate of this bug.
Yes, this is for GROUP BY statement and not only for related model (it
might be a simple field, too).
Django Admin view might not be the best option for showing aggregated
queryset, but as far as the fix is very simple, I think it might be
supported. And other options are still worse: creating view in raw SQL,
using dedicated view (with pagination, filters, actions, oh my... it would
be a huge rewrite of admin stuff).
--
Ticket URL: <https://code.djangoproject.com/ticket/25396#comment:2>
Django
Reporter: dex4er | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by dex4er):
I tried to override Admin's internals so it is possible to use it with
official Django 1.8.4 (with or without Django Suit template). The result
is https://gist.github.com/dex4er/c658c38c3a12df7b92fc
As you can see there is a lot of duplicated code: 2 meaning lines of code
and 153 lines of copy-and-paste :(
--
Ticket URL: <https://code.djangoproject.com/ticket/25396#comment:3>
Django
Reporter: dex4er | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by charettes):
I understand your use case but I'm not sure simply allowing
`get_queryset()` to return a `ValueQuerySet` will do here.
From what I understand this will break many things since your results omit
the `pk` from the results. Your provided examples are good example of
customization required to make this work (e.g `list_display` mapping to
methods, `has_add_permission` returning `False`) and silence strange
errors the admin code might be raising.
I'm still not convinced we should accept this new feature, even with a
defined use case.
--
Ticket URL: <https://code.djangoproject.com/ticket/25396#comment:4>
Django
Reporter: dex4er | Owner: nobody
Type: New feature | Status: closed
Component: contrib.admin | Version: master
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* status: new => closed
* resolution: => duplicate
Comment:
Yes, I think when you find yourself overriding large portions of the
admin, it's time to write your own views.
--
Ticket URL: <https://code.djangoproject.com/ticket/25396#comment:5>
Django
Reporter: dex4er | Owner: nobody
Type: New feature | Status: closed
Component: contrib.admin | Version: master
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by dex4er):
Writing own view which supports paging, filtering, actions, ordering and
import/export (via additional packages) means that I need to copy-and-
paste large portions of the admin view or need to reinvent the wheel.
I think adding a few methods and settings to ModelAdmin is still better
option.
This patch doesn't break anything else in Django and just allows to make
some new use cases for admin interface...
--
Ticket URL: <https://code.djangoproject.com/ticket/25396#comment:6>
Django
Reporter: dex4er | Owner: nobody
Type: New feature | Status: closed
Component: contrib.admin | Version: master
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by timgraham):
I believe a different API to construct the desired query might be the
proper solution.
--
Ticket URL: <https://code.djangoproject.com/ticket/25396#comment:7>
Django
Reporter: dex4er | Owner: nobody
Type: New feature | Status: closed
Component: contrib.admin | Version: master
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by dex4er):
I'm not sure what do you mean "different API".
I've refactored this example use case and created "database view" with
`model.Manager`. The fields are aliased with
`annotate(aliased_field=F('original_field')).
This solution works also with django-import-export, so I can generate CSV
or XLS file based on this aggregated `QuerySet`!
models.py
{{{#!python
from django.db import models
from django.db.models import Count, F
from django.contrib.auth.models import User
class Category(models.Model):
name = models.CharField(max_length=100)
class Meta:
verbose_name_plural = 'categories'
def __unicode__(self):
return self.name
class Note(models.Model):
user = models.ForeignKey(User)
category = models.ForeignKey(Category)
keywords = models.CharField(max_length=400)
title = models.CharField(max_length=200)
date = models.DateTimeField()
content = models.TextField(max_length=50000)
def __unicode__(self):
return self.title
class UserNotesReportManager(models.Manager):
def get_queryset(self):
return Note.objects \
.annotate(user_username=F('user__username'),
category_name=F('category__name')) \
.values('user_username', 'category_name') \
.order_by('user_username', 'category_name') \
.annotate(n=Count('*'))
class UserNotesReport(models.Model):
user_username = models.CharField(max_length=30, primary_key=True)
category_name = models.CharField(max_length=100, primary_key=True)
n = models.IntegerField()
objects = UserNotesReportManager()
}}}
admin.py
{{{#!python
from django.contrib import admin
import hello_blog.models as models
class CategoryAdmin(admin.ModelAdmin):
pass
admin.site.register(models.Category, CategoryAdmin)
class NoteAdmin(admin.ModelAdmin):
pass
admin.site.register(models.Note, NoteAdmin)
class UserNotesReportAdmin(admin.ModelAdmin):
actions = None
list_display = ['user_username', 'category_name', 'n']
list_display_links = None
readonly_fields = list_display
def has_add_permission(self, request):
return False
admin.site.register(models.UserNotesReport, UserNotesReportAdmin)
}}}
This example is available at https://github.com/dex4er/django-hello-
world/tree/blog_reports so you can check if the modification works
correctly.
--
Ticket URL: <https://code.djangoproject.com/ticket/25396#comment:8>
Django
Reporter: dex4er | Owner: nobody
Type: New feature | Status: closed
Component: contrib.admin | Version: master
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by timgraham):
I mean a different way to construct the GROUP BY query without using
`values()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/25396#comment:9>