Django Admin - ModelAdmin exclude

[Peter Farrell]

We are writing a custom admin for CleanerVersion that subclasses ModelAdmin. Just about every attribute has a hook method which makes extension easier. For example, list_display has get_list_display(). However, exclude doesn't have one implemented and it seems like an oversight. I'm proposing we add one.

Our current work seeking is to write a property descriptor for exclude but then the admin check fails with it not being a tuple or list. Then you have to create a custom admin checks class to suppress the exclude check error because it's not a tuple or list (but really a descriptor).

If this is ok, I'Ill write a PR.

[Marc Tamlyn]

I don't think we should add this. Exclude is passed to the form, and we already have a way of changing the form based on the request. I'd rather see changes made to reduce some of the many many ways to select fields in the admin. By my count at present we have:

ModelAdmin.fields

ModelAdmin.get_fields()

ModelAdmin.exclude

ModelAdmin.fieldsets

ModelAdmin.get_fieldsets()

ModelAdmin.readonly_fields

ModelAdmin.get_readonly_fields()

ModelAdmin.form concrete fields

ModelAdmin.form.Meta.fields

ModelAdmin.form.Meta.exclude

ModelAdmin.get_form()

There's an argument the only one missing here is get_exclude(), but I think the current API is silly. Personally, I'd like to see us moving towards us encouraging doing more work in the form (and defining the form) rather than doing so much in the ModelAdmin class. This may well require better support for fieldsets in django.forms.

Marc

--
You received this message because you are subscribed to the Google Groups "Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/c1e8762d-1c9f-47e9-8fe7-e9761c27e057%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Peter J. Farrell]

In our case, we need to dynamically include additional "exclude" fields in our VersionableAdmin and not override the selections the developer has set in their subclass.  There are two options to accomplished this:

1) Override get_form() which is messy because you can't just call super here and modify things.  This is a lot of code duplication to just get at the exclude

def get_form(self, request, obj=None, **kwargs):
    """
    Returns a Form class for use in the admin add view. This is used by
    add_view and change_view.
    """
    if 'fields' in kwargs:
        fields = kwargs.pop('fields')
    else:
        fields = flatten_fieldsets(self.get_fieldsets(request, obj))
    if self.exclude is None:
        exclude = []
    else:
        exclude = list(self.exclude)
    exclude.extend(self.get_readonly_fields(request, obj))
    if self.exclude is None and hasattr(self.form, '_meta') and self.form._meta.exclude:
        # Take the custom ModelForm's Meta.exclude into account only if the
        # ModelAdmin doesn't define its own.
        exclude.extend(self.form._meta.exclude)
    # if exclude is an empty list we pass None to be consistent with the
    # default on modelform_factory
    exclude = exclude or None
    defaults = {
        "form": self.form,
        "fields": fields,
        "exclude": exclude,
        "formfield_callback": partial(self.formfield_for_dbfield, request=request),
    }
    defaults.update(kwargs)

    if defaults['fields'] is None and not modelform_defines_fields(defaults['form']):
        defaults['fields'] = forms.ALL_FIELDS

    try:
        return modelform_factory(self.model, **defaults)
    except FieldError as e:
        raise FieldError('%s. Check fields/fieldsets/exclude attributes of class %s.'
                         % (e, self.__class__.__name__))

2) Build your own descriptor for exclude:

@property
def exclude(self):
    """
    Custom descriptor for exclude since there is no get_exclude method to be overridden
    """
    exclude = self.VERSIONED_EXCLUDE

    if super(VersionedAdmin, self).exclude is not None:
        # Force cast to list as super exclude could return a tuple
        exclude = list(super(VersionedAdmin, self).exclude) + exclude

    return exclude

But then this trips the _check_exclude check in ModelAdminChecks for not being a list or tuple. So you have to subclass that:

class VersionedAdminChecks(ModelAdminChecks):
    def _check_exclude(self, cls, model):
        """
        Required to suppress error about exclude not being a tuple since we are using @property to dynamically change it
        """
        return []

All of this is a lot of effort just to get at dynamic exclude where get_exclude() fits the current pattern used in the admin in the current code base.

[Tim Graham]

I agree with Marc that so many ways to do things isn't ideal, but I don't see a different simple way to accommodate Peter's use case, so +0 to adding get_exclude().

[Peter Farrell]

I agree there are many ways to accomplishing things in the admin, but cleaning that up and continuing BC is a completely other ticket / implementation (it looks like a good Sumer of Code project similar to the Meta API recently completed).  Since the ticket was marked as accepted, I'll double check my patch and make a PR this week.

[Luke Plant]

I agree with Marc on this one  - I remember doing the work to change "ModelForm.fields" so that you needed "__all__" to indicate everything, instead of defaulting to all. This was a nightmare precisely because of the different number of ways of defining the fields. This is partly the result of retrofitting ModelAdmin to use ModelForms, which it didn't originally.

Just implementing ModelAdmin.get_exclude() might prove to be extremely difficult to do, especially when you need to document how it interacts with all the other ones.

I think the problems with the false negatives for the admin checks may need to be solved another way - namely by removing them. That's unfortunate, but I found them tricky in the past - they may prevent common errors, but they also prevent more advanced ways that you might want to use the admin.

Luke

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAMwjO1HKDr6T9BKf-%2B4rNm6775S7akyHfC%2BNEk4sqSDw8cOb%2Bg%40mail.gmail.com.

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

-- 
"He knows the way I take: when he has tried me, I shall come forth 
as gold" (Job 23:10).

Luke Plant || http://lukeplant.me.uk/

[Ramez Ashraf]

I agree that there are a lot of of hooks in the admin and we might need to better document them or remove unneeded or maybe make a revolution :)
But, with the current api and scheme, having get_exclude is more of the expected behavior of Django and i think it should be implemented.

What we should certainly do -i believe- is more documenting the admin flow & how it's constructed, best practices to achieve certain repeated goals etc..

Regards,
Ramez

[Tim Graham]

A pull request has been offered: https://github.com/django/django/pull/5214

It seems simple enough to me. Marc or Luke, would you withdraw your objection after seeing the implementation?

[Luke Plant]

My initial reaction:

The docs, with Tim's correction, will look like:

The ``get_exclude`` method is given the ``HttpRequest`` and the ``obj`` being edited and is expected to return a list of fields, as described above in the :attr:`ModelAdmin.exclude` section.

However, the default implementation of ``get_exclude`` does not return a list at all, it returns None. And, it has to do this, otherwise it would actually break things - currently None is used as a sentinel value and it is different from an empty list (https://github.com/django/django/pull/5214/files#diff-3c42de3e53aba87b32c494f995a728dfL1801)

So, an overridden `get_exclude` method that calls `super` will get surprises, and will have to deal with them.

I would expect a method like `get_exclude` to hide these kind of warts, but for historic reasons it can't.  By adding this method, we're adding another wart - a method that is documented as returning a list but doesn't. Of course, we could document all the gory details, but the longer the documentation, the more I think "this just smells". Django has enough warts and smells without adding more.

I realise that `get_fields` already behaves in the same way as the proposed `get_exclude`, but it's not very nice. The behaviour of `get_fieldsets` is much better.

However, having said all this, I realise there is a reasonable use case for this (like Peter Farrell's). Also, the current asymmetry between 'fields' and 'exclude' is also a wart. There must be a lesson for the future in all of this, I'm not sure what exactly...


Luke

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/ee4e1491-78cb-4e3a-8419-5a054ca079f1%40googlegroups.com.

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

-- 
Clothes make the man. Naked people have little or no influence on 
society.
           -- Mark Twain

Luke Plant || http://lukeplant.me.uk/