Request to reconsider #30311 -- allow overriding site-wide admin actions

[Shai Berger]

Hello fellow Djangonauts,

While working on upgrading a project to 2.2, I ran into multiple
instances of

<class '...Admin'>: (admin.E130) __name__ attributes of actions
defined in <class '...Admin'> must be unique.

caused by the fact that, out of a large set of Admin classes, about a
dozen have defined their own version of the 'delete_selected' action.

When looking into it, I came across ticket #30311[1] which basically
complained about the very same thing; but was closed as invalid,
under the claim that the user is in full control of both the site-wide
actions and the admin-specific actions, and so can do whatever they
like.

While that claim is certainly correct, I would like to suggest that it
creates an odd situation for the app which wants to define its own
version of delete_selected (or any other site-wide-availabe action) for
one of its models. In fact, it now cannot be done at the app level --
it requires that the default action be disabled at the site level.

If (as is probably the case with delete_selected) it is still desired
as a site-wide-available action, then the default needs to be replaced
with a custom action which first looks for an override on the specific
admin, and if not found invokes the original. And that seems to me like
a hoop we shouldn't force people to jump through.

Opinions?

[1] https://code.djangoproject.com/ticket/30311

[Carlton Gibson]

Hi Shai. 

I triaged that, and was involved in the change in #29917 that led to your issue. 

https://code.djangoproject.com/ticket/29917

The proposal to change it was made on the list here:

https://groups.google.com/d/topic/django-developers/-OWoYL_zryM/discussion

I’ll have another review of it, but is there an **exact** change you have in mind that satisfies all the competing Wants? (It was all quite interconnected IIRC)

Thanks! 

Carlton 

--
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 view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/20200318180034.15cff442.shai%40platonix.com.

[charettes]

Given the common need to override delete_selected I wonder if we could define it as ModelAdmin method and use a .actions string reference to it in the AdminSite instead[0].

That would allow admin classes to simply override the delete_selected method on their admin classes without having to resort to redefining .actions which seems to trigger the issue here.

Cheers,

Simon

[0] https://github.com/django/django/blob/d4df5e1b0b1c643fe0fc521add0236764ec8e92a/django/contrib/admin/sites.py#L66

[charettes]

Just to make the above clear, here's what I had in mind

https://gist.github.com/charettes/a0cb94242ac9c198625b23f4f55fab45

[Shai Berger]

Hi Carlton,

On Wed, 18 Mar 2020 17:11:49 +0100
Carlton Gibson <carlton...@gmail.com> wrote:

> I triaged that, and was involved in the change in #29917 that led to
> your issue.
>
> https://code.djangoproject.com/ticket/29917

> https://groups.google.com/d/topic/django-developers/-OWoYL_zryM/discussion
>

Yes, I've seen these too.

>
> I’ll have another review of it, but is there an **exact** change you
> have in mind that satisfies all the competing Wants? (It was all quite
> interconnected IIRC)
>

I think they're not all that interconnected. The change in #29917 was
about duplication caused by inheritance; #30311 is about duplication
between an admin's specific actions and the site-wide actions. I fully
endorse and support the inheritance change.

The change I'd like to see is a rewrite of _get_base_actions(), so that
instead of blindly adding the site-wide actions, it only adds those not
defined by the specific admin. There's lines there[1] that say:

for (name, func) in self.admin_site.actions:
description = getattr(func, 'short_description', name.replace('_', ' '))
actions.append((func, name, description))

[1] https://github.com/django/django/blob/master/django/contrib/admin/options.py#L855

and I'd like to see added something along the lines of:

for (name, func) in self.admin_site.actions:
if name in (self.actions or []):
continue
description = getattr(func, 'short_description', name.replace('_', ' '))
actions.append((func, name, description))

(that's too simplistic, actions can given by more than name, but
that's the general idea)

This way, if an admin wants to override, they just do.

HTH,
Shai.

[Shai Berger]

[Shai Berger]

(sorry about the previous empty mail, UI glitch)

On Wed, 18 Mar 2020 09:29:17 -0700 (PDT)
charettes <chare...@gmail.com> wrote:

> Just to make the above clear, here's what I had in mind
>
> https://gist.github.com/charettes/a0cb94242ac9c198625b23f4f55fab45
>

Yes, that would do what I want and seems better than my suggestion.

[Carlton Gibson]

OK, thanks both. Seems reasonable. Let me have a look at it in the morning.

One question is whether we handle this for just delete_selected or any action which an AdminSite declares.

The example from #30311 was `expect_inquisition` (Nice) Do we want to handle that too? 🤔

Cheers.
C.

[charettes]

I think deleted_selected is *special* since it's the only default action provided.

I guess we could document that a method name string reference should be passed to AdminSite.add_action if it's meant to be overridden.

Simon

[Shai Berger]

On Wed, 18 Mar 2020 10:34:30 -0700 (PDT)
charettes <chare...@gmail.com> wrote:

> I think deleted_selected is *special* since it's the only default
> action provided.
>

I agree, but...

> I guess we could document that a method name string reference should
> be passed to AdminSite.add_action if it's meant to be overridden.
>

For that to be viable, the project need to have an admin class which
all others inherit, and that's not a trivial requirement -- unless we
can pass a method-name-string-reference *and* a default function.

My 2 cents,
Shai.

[Carlton Gibson]

OK, reopened and accepted.

Thanks for bringing it to the list Shai!

C.