Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#12879 closed (fixed)

jquery gets included multiple times

Reported by: mariarchi Owned by: Tobias McNulty
Component: contrib.admin Version: 1.2-beta
Severity: Keywords: jquery
Cc: admin@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

ModelAdmin adds jquery.min.js to the self.media if self.actions is not False.
However, InlineModelAdmin by default includes jquery.min.js as well.

So given a model with N inlines by default we end up with N+1 jqueries.

It wouldn't be that bad, however, what makes it really bad is the fact that if the main form specifies a jquery plugin in the media definition it will be overwritten by vanilla redundant jquery.

The simplest fix would be to implement internal storage in Media object as a set which would remove redundant entries.

Attachments (1)

12879.diff (1.4 KB) - added by James Bennett 7 years ago.
Patch + test case

Download all attachments as: .zip

Change History (12)

comment:1 Changed 7 years ago by Russell Keith-Magee

milestone: 1.2
Triage Stage: UnreviewedAccepted

comment:2 Changed 7 years ago by sebleier@…

Correct me if I'm wrong, but I thought there was already a mechanism to limit dupes in the admin media?

http://code.djangoproject.com/browser/django/trunk/django/forms/widgets.py#L76

comment:3 Changed 7 years ago by Tobias McNulty

Owner: changed from nobody to Tobias McNulty
Status: newassigned

comment:4 Changed 7 years ago by Tobias McNulty

Resolution: worksforme
Status: assignedclosed

Unable to reproduce. If this is still an issue for you, please provide your forms, models, and admin file or, better yet, a unit test demonstrating the issue.

comment:5 Changed 7 years ago by mariarchi

Resolution: worksforme
Status: closedreopened

Ooops, I was wrong, sorry.

However, if one will do that -

class MovieAdmin(admin.ModelAdmin):
    class Media:
        js = ("/media/js/jquery.min.js",
              "media/js/jquery.min.js",
              "media/js/jquery.min.js")

jquery will get included three times. Furthermore, if one codes up a class like

class MovieAdmin(admin.ModelAdmin):
    class Media:
        js = ("/admin_media/js/jquery.min.js",
              "/admin_media/js/jquery.min.js",
              "/admin_media/js/jquery.min.js")

none of this declarations will make it through, if jquery already got in the media object. Or all three will appear, if it wasn't there yet.

I think that this behavior is both inconsistent and wrong.

The main use case is that AFAIK the current way of creating pluggable admin/form features is to provide mixins. So,
assuming we have mixins MixinA, MixinB and MixinC then our admin class will probably look like

class MyAdmin(admin.ModelAdmin, MixinA, MixinB, MixinC):
    class Media:
        js = MixinA.Media.js + MixinB.Media.js + MixinC.Media.js

and guess what happens if all three use jquery.

Changed 7 years ago by James Bennett

Attachment: 12879.diff added

Patch + test case

comment:6 Changed 7 years ago by James Bennett

The root issue is that JS is added in a list comprehension, which generates the list of JS file paths to add before adding any of them; thus the same file path can be added multiple times by a single declaration. Patch attached above resolves this by using a loop which checks after adding each path, and also includes a regression test case which demonstrates the problem and passes after the fix is applied.

comment:7 Changed 7 years ago by James Bennett

Resolution: fixed
Status: reopenedclosed

(In [12663]) Fixed #12879: Declaring the same JS file multiple times in a single Media instance now only includes that file once.

comment:8 Changed 7 years ago by James Bennett

(In [12664]) [1.1.X] Fixed #12879: Declaring the same JS file multiple times in a single Media instance now only includes that file once. Backport of [12663] from trunk.

comment:9 Changed 7 years ago by James Bennett

Note that the fix applied does not deal with the issue of specifying the same file once with a relative path and once with an absolute path; as far as I'm concerned, that's expected behavior for the moment, but maybe down the road we'll come up with some clever normalization trick to keep it from happening.

comment:10 Changed 7 years ago by James Bennett

(In [12666]) [1.1.X] Expanded the fix in [12663] to cover CSS declarations, which were also affected by the bug. Refs #12879. Backport of [12665] from trunk.

comment:11 Changed 5 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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