Opened 6 years ago

Closed 5 years ago

#29711 closed Cleanup/optimization (fixed)

Add system check to ensure uniqueness of admin actions' __name__.

Reported by: Przemysław Buczkowski Owned by: Przemysław Buczkowski
Component: contrib.admin Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

It might be that I don't understand something, but I wrote this piece of code yesterday:

# admin.py
def action(fun):
    def immediately(modeladmin, request, queryset):
        result = fun(queryset)
        if result:
            modeladmin.message_user(request, result)
    immediately.short_description = fun.short_description + " (now)"
    def queue(modeladmin, request, queryset):
        fun.delay(queryset)
        modeladmin.message_user(request, "Added to the queue.")
    queue.short_description = fun.short_description + " (add to the queue)"
    return immediately, queue

class CompanyAdmin(admin.ModelAdmin):
    actions = (*action(telephone), *action(mail))
# (...)

And only functions generated by the first call to action() get added. (In the case above two actions from "telephone" only; when reversed, from "mail" only).

I suspect that Django might detect them as duplicates.

Change History (11)

comment:1 by Carlton Gibson, 6 years ago

Resolution: invalid
Status: newclosed

The issue occurs here in the `get_actions()` call:

        # Convert the actions into an OrderedDict keyed by name.
        return OrderedDict(
            (name, (func, name, desc))
            for func, name, desc in actions
        )

Here you've got two pairs of (func, name, desc) tuples keyed by the immediately and queue names. Thus when they're passed to the OrderedDict constructor, the keys conflict.

And only functions generated by the first call to action() get added...

This doesn't seem right. We should expect the opposite:

If a key occurs more than once, the last value for that key becomes the corresponding value in the new dictionary.
Python Docs

The `name` comes from the `__name__` attribute of the action function.

You should be able to adjust this to ensure unique names.

Perhaps something like:

immediately.__name__ = 'immediately_{}'.format(fun.__name__)

... and so on.

comment:2 by Przemysław Buczkowski, 6 years ago

Yes, you're right, the opposite happens - I got a bit confused :)

Do you think that a warning should be added to Django docs? I can provide a patch to the docs since I don't think that this behaviour is obvious, even if it's intended - but maybe that's just me.

comment:3 by Adam Johnson, 6 years ago

A system check could be added to ensure that every action in actions has a unique __name__ - since the __name__ collapsing into an OrderedDict isn't done until runtime.

comment:4 by Przemysław Buczkowski, 6 years ago

Resolution: invalid
Status: closednew

@Adam That would be great :)

comment:5 by Carlton Gibson, 6 years ago

Summary: One can use as actions functions generated only by the first call to another functionAdd system check to ensure uniqueness of admin actions' __name__.
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization
Version: 2.1master

Yes OK. Przemysław, if you'd like to code that up, that would be great.

comment:6 by Przemysław Buczkowski, 6 years ago

Owner: changed from nobody to Przemysław Buczkowski
Status: newassigned

Gladly.

comment:7 by Przemysław Buczkowski, 6 years ago

Just a quick heads-up that I'll be getting down to it, I'm just reading on this and friends in before :)

comment:8 by Przemysław Buczkowski, 6 years ago

Has patch: set

comment:9 by Carlton Gibson, 6 years ago

Patch needs improvement: set

comment:10 by Przemysław Buczkowski, 6 years ago

The patch was modified.

comment:11 by Tim Graham <timograham@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 70d0a1c:

Fixed #29711 -- Added a system check for uniquness of admin actions' name.

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