Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#25396 closed New feature (duplicate)

ModelAdmin.get_queryset doesn't accept ValueQuerySet

Reported by: Piotr Roszatycki Owned by: nobody
Component: contrib.admin Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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:

Change History (9)

comment:1 by Simon Charette, 9 years ago

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?

comment:2 by Piotr Roszatycki, 9 years ago

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).

comment:3 by Piotr Roszatycki, 9 years ago

I tried to override Admin's internals so it is possible to use this feature 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 :(

Last edited 9 years ago by Piotr Roszatycki (previous) (diff)

comment:4 by Simon Charette, 9 years ago

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.

comment:5 by Tim Graham, 9 years ago

Resolution: duplicate
Status: newclosed

Yes, I think when you find yourself overriding large portions of the admin, it's time to write your own views.

comment:6 by Piotr Roszatycki, 9 years ago

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...

comment:7 by Tim Graham, 9 years ago

I believe a different API to construct the desired query might be the proper solution.

comment:8 by Piotr Roszatycki, 9 years ago

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

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'


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)


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

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.

Last edited 9 years ago by Piotr Roszatycki (previous) (diff)

comment:9 by Tim Graham, 9 years ago

I mean a different way to construct the GROUP BY query without using values().

Note: See TracTickets for help on using tickets.
Back to Top