Opened 14 years ago

Closed 4 years ago

Last modified 4 years ago

#12679 closed Bug (fixed)

In admin, inlines should be allowed to be properties

Reported by: mariarchi Owned by: nobody
Component: contrib.admin Version: dev
Severity: Normal Keywords: admin inlines duck typing
Cc: admin@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently, if admin class defines an attribute 'inlines' which is not an instance of tuple or a list, django will raise an error.
I believe that this is wrong and contradicts the duck typing philosophy - we shouldn't care that the attribute 'inlines' is an instance of list/tuple, but rather we should care that it is iterable.

The reason why I believe that behavior is wrong is the following:
one might want to define 'inlines' as a property. This might happen when admin class A inherits from admin class B, and class B adds some inlines (that can be useful when developing a pluggable application). If class A has some inlines of its own, it can be useful to declare inlines as a property and then call 'super' to retrieve the inlines of class B.

Change History (9)

comment:1 by mariarchi, 14 years ago

Anyway, that behavior is quite inconsistent with the fact that for most attributes in model/admin/url definition django does not check the instance of said attribute

comment:2 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Russell Keith-Magee, 14 years ago

Component: Contrib appsdjango.contrib.admin

comment:4 by Matt McClanahan, 13 years ago

Severity: Normal
Type: Bug

comment:5 by Julien Phalip, 13 years ago

Easy pickings: unset

Currently a lot of ModelAdmin's attributes (list_display, readonly_fields, list_filter, etc.) are expected to be lists or tuples. ModelAdmin.inlines shouldn't be treated as a special case. If we allow it to be merely an iterable, then the same should be done for all those other attributes. (Hint: check out django.contrib.admin.validation.check_isseq())

See also #16089 which is looking at adding flexibility with inlines registration.

comment:6 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:7 by Baptiste Mispelon, 4 years ago

Turns out this got fixed in 1d8eb0cae57731b481a88dca272b2cb0d645bd8e.

I wrote a pull request to add a test for the specific usecase of the original ticket but I'm not 100% convinced it's necessary.
To me, the test basically checks that python properties work which seems silly. Plus if we're testing ModelAdmin.inlines then why not check all other potential ModelAdmin options?

comment:8 by Mariusz Felisiak, 4 years ago

Resolution: fixed
Status: newclosed

Thanks for checking this. I don't think that we need more tests.

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 4161e35:

Refs #12679 -- Added test for using property as ModelAdmin.inlines.

Fixed in 1d8eb0cae57731b481a88dca272b2cb0d645bd8e.

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