Adding dynamic methods to ModelAdmin fails in 1.8

[Malcolm Box]

Hi,

I'm trying to add a dynamic method to a ModelAdmin so that I can have automatically generated thumbnail fields for any image by simply adding '<fieldname>_thumbnail' to the field definitions - which saves a lot of code when there's a load of admin classes with image fields that need thumbnails.

e.g.

class ThumbnailMixin(object):

    def getattr(self, name):

         if name.endswith('_thumbnail'):

              # ... generate appropriate thumbnail method and return

              return thumbnail_function

         else:

             raise AttributeError

class ExampleAdmin(models.ModelAdmin, ThumbnailMixin):

     fields = ['image_thumbnail', 'image', ...]

This worked fine in Django 1.6/1.7, but it's now failing in 1.8 with a SystemCheckError (admin.E035 / admin.E108). If I silence the error, the admin actually works fine, but that feels icky.

The check fails because it checks for the field being on the ExampleAdmin class, not on an instance - whereas the admin always uses an instance, so it works.

What's the "right" way to do this in the 1.8 world - I've considered a custom metaclass, but that feels like overkill for a simple task like this. Should this even work, and if so is it a bug in the check framework?

Cheers,

Malcolm

[Simon Charette]

Hi Malcom!

I would suggest you have a look at the ModelAdmin.get_fields() method to append your thumbnail read-only field names.

As documented, just make sure you also override ModelAdmin.get_readonly_fields() to return them as well.

Cheers,
Simon

[Malcolm Box]

Hi Simon,

Thanks for the pointer, but I don't think that helps.

The fields are already declared using the existing fields / readonly_fields attributes on the ExampleAdmin class - and this is what get_fields / get_readonly_fields return. The system check fails because the fields declared don't exist on the ExampleAdmin class nor on the model. Here's the relevant lines from contrib/admin/checks.py:

    def _check_readonly_fields_item(self, cls, model, field_name, label):

        if callable(field_name):

            return []

        elif hasattr(cls, field_name):

            return []

        elif hasattr(model, field_name):

            return []

        else:

            try:

                model._meta.get_field(field_name)

            except FieldDoesNotExist:

                return [

                    checks.Error(

                        "The value of '%s' is not a callable, an attribute of '%s', or an attribute of '%s.%s'." % (

                            label, cls.__name__, model._meta.app_label, model._meta.object_name

                        ),

                        hint=None,

                        obj=cls,

                        id='admin.E035',

                    )

                ]

            else:

                return []

If the thumbnail fields were defined as methods on the ExampleAdmin, all would be fine e.g.:

class ExampleAdmin(models.ModelAdmin):

   fields = ['image', 'image_thumbnail']

   

   def image_thumbnail(self, obj):

       return "<img src="%s"/>" % obj.image.url 

That's fine, but if there's lots of image fields (with a variety of names) spread over several ModelAdmin classes, then you end up with a lot of duplicated code. The normal solution then is to refactor the code. And that's where I get stuck - I can't see (short of metaclass programming!) how to inject the methods into the class such that the system check succeeds.

I therefore suspect that the check is actually borked, and it should be checking hasattr(instance, field_name) rather than hasattr(cls, field_name)

So I see three possibilities, in order of probability:

1. I'm being dumb, and there's an easy way to dynamically create attributes on a ModelAdmin that passes system checks
2. The system check is incorrect, and should allow dynamically created attributes on ModelAdmin when validating fields
3. Metaclass programming is really the right way to do this

Malcolm

--
You received this message because you are subscribed to a topic in the Google Groups "Django users" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/django-users/lsDP5oUWOsw/unsubscribe.
To unsubscribe from this group and all its topics, send an email to django-users...@googlegroups.com.
To post to this group, send email to django...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-users.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-users/05249551-899b-4a64-b416-216c9f81d950%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Malcolm Box

CTO & Co-Founder, Tellybug  http://tellybug.com

mal...@tellybug.com   +44 7766 990123    @malcolmbox

[Simon Charette]

Hi Malcom,

What I meant to suggest is to remove the fields from `fields`/`readonly_fields` and dynamically return them in the `get_(fields|readonly_fields)` fields method.

e.g.

class ThumbnailFieldsAdmin(models.ModelAdmin):
    fields = []
    readonly_fields = []
    thumbnail_fields = []

    def __getattr__(self, name):
        if name.endswith('_thumbnail'):
            return thumbnail_function
        raise AttributeError

    def get_fields(request, obj=None):
        fields = super(ThumbnailFieldsAdmin, self).get_fields(request, obj=obj)
        return self.thumbnail_fields + fields

    def get_readonly_fields(request, obj=None):
        readonly_fields = super(ThumbnailFieldsAdmin, self).get_fields(request, obj=obj)
        return readonly_fields + thumbnail_fields

But I'm afraid that you'll have to rely on metaclass programming if you want the order of fields to be maintained somehow.

> I therefore suspect that the check is actually borked, and it should be checking hasattr(instance, field_name) rather than hasattr(cls, field_name)

The thing is checks are run against ModelAdmin classes and not the instances bound to the site they were registered to. You could submit a feature request to actually run the test against the instances but since this is really and edge case you'd have to provide a patch/PR if you don't want the ticket to rot in Trac make it to Django 1.9 which should enter feature freeze soon enough. I'd be glad to review your proposed changes if you're interested.

Simon

[Malcolm Box]

Hi Simon,

I've tried that, and it still fails the same system check.

The system check checks that all the values returned from get_readonly_fields exist as class attributes on the ModelAdmin (or are callable/fields on model, neither of which helps here). With them being created via __getattr__, they don't.

I'm coming to the conclusion that the right behaviour is to run the system check against an instance, not the class, since that's what the core admin code uses.

Thanks for the offer to review changes - I'll try to put a patch together this week.

Cheers,

Malcolm

To view this discussion on the web visit https://groups.google.com/d/msgid/django-users/5c3d0da9-30bd-484e-9f39-6e011e0f5abc%40googlegroups.com.

[Malcolm Box]

Ticket filed: https://code.djangoproject.com/ticket/25374

On Thursday, 10 September 2015 11:27:13 UTC+1, Malcolm Box wrote:

Hi Simon,

I've tried that, and it still fails the same system check.

The system check checks that all the values returned from get_readonly_fields exist as class attributes on the ModelAdmin (or are callable/fields on model, neither of which helps here). With them being created via __getattr__, they don't.

I'm coming to the conclusion that the right behaviour is to run the system check against an instance, not the class, since that's what the core admin code uses.

Thanks for the offer to review changes - I'll try to put a patch together this week.

Cheers,

Malcolm

To unsubscribe from this group and all its topics, send an email to django-users+unsubscribe@googlegroups.com.

To post to this group, send email to django...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-users.

[Simon Charette]

Hi Malcolm,

> The system check checks that all the values returned from get_readonly_fields exist as class attributes on the ModelAdmin (or are callable/fields on model, neither of which helps here). With them being created via __getattr__, they don't.

There might be something else going on here but I highly doubt checks are ran against the get_readonly_fields() return value since it's a method bound to a ModelAdmin instance and requires a `request` argument to be called in the first place.

Simon

[Malcolm Box]

Hi Simon,

On Thursday, 10 September 2015 16:57:51 UTC+1, Simon Charette wrote:

Hi Malcolm,

> The system check checks that all the values returned from get_readonly_fields exist as class attributes on the ModelAdmin (or are callable/fields on model, neither of which helps here). With them being created via __getattr__, they don't.

There might be something else going on here but I highly doubt checks are ran against the get_readonly_fields() return value since it's a method bound to a ModelAdmin instance and requires a `request` argument to be called in the first place.

My mistake, you're entirely correct. The SystemChecks check the readonly_fields property, not the result of calling get_readonly_fields().

With the patch to check an instance rather than a class, perhaps I should extend the checks to check the result of get_readonly_fields() rather than just obj.readonly_fields. What do you think?

Malcolm

[Simon Charette]

Hi Malcolm,

> With the patch to check an instance rather than a class, perhaps I should extend the checks to check the result of get_readonly_fields() rather than just obj.readonly_fields. What do you think?

I think the checks should be run statically against ModelAdmin instances, that is only the properties of the instances should be checked and not the return value of methods that could depend on request properties such as the user.

Simon