Code

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#12879 closed (fixed)

jquery gets included multiple times

Reported by: mariarchi Owned by: tobias
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 ubernostrum 4 years ago.
Patch + test case

Download all attachments as: .zip

Change History (12)

comment:1 Changed 4 years ago by russellm

  • milestone set to 1.2
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 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 4 years ago by tobias

  • Owner changed from nobody to tobias
  • Status changed from new to assigned

comment:4 Changed 4 years ago by tobias

  • Resolution set to worksforme
  • Status changed from assigned to closed

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 4 years ago by mariarchi

  • Resolution worksforme deleted
  • Status changed from closed to reopened

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 4 years ago by ubernostrum

Patch + test case

comment:6 Changed 4 years ago by ubernostrum

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 4 years ago by ubernostrum

  • Resolution set to fixed
  • Status changed from reopened to closed

(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 4 years ago by ubernostrum

(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 4 years ago by ubernostrum

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 4 years ago by ubernostrum

(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 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.