Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#28866 closed Bug (fixed)

InlineAdminFormSet should include InlineModelAdmin's Media before its formset's Media

Reported by: clincher Owned by: nobody
Component: contrib.admin Version: 2.0
Severity: Release blocker Keywords: formset, media, admin
Cc: Johannes Maron Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I was trying to use django-map-widgets with django2.0rc1 and it didn't show the widget. The problem is that widget js-files were included before django-admin jquery's files which it relies on.
I went deeper and found that Media.merge method are pretty much implicit which breaks Python zen guidelines. The implicity caused by MediaOrderConflictWarning check, without that check code might be way more simplier. More than that, the realization of that check is not working. I got few ideas how to solve it, but honestly I would prefer just remove it and make the method really simple.

Example of lists with which the method doesn't work properly:
list_1 = ['admin/js/vendor/jquery/jquery.js', 'admin/js/jquery.init.js', 'admin/js/core.js', 'admin/js/admin/RelatedObjectLookups.js', 'admin/js/actions.js', 'admin/js/urlify.js', 'admin/js/prepopulate.js', 'admin/js/vendor/xregexp/xregexp.js']
list_2 = ['https://maps.googleapis.com/maps/api/js?libraries=places&key=', 'mapwidgets/js/jquery_class.js', 'mapwidgets/js/django_mw_base.js', 'mapwidgets/js/mw_google_point_field.js', 'admin/js/vendor/jquery/jquery.js', 'admin/js/jquery.init.js', 'admin/js/inlines.js']

I can easily make a patch without the warning check. Also can check the order, but it's gonna be very different way from what's there now.

I set serverity to blocker since it's totally breaks possibility to use Django with many other libraries

Change History (16)

comment:1 by Tim Graham, 7 years ago

Cc: Johannes Maron added
Type: UncategorizedBug

How do you propose to solve the problem described in #28377 if the merging logic is removed?

Is it impossible to modifying the ordering of your lists so the warning doesn't happen? This is documented as a backwards incompatible change in the Django 2.0 release notes:

In older versions, forms and formsets combine their Media with widget Media by concatenating the two. The combining now tries to preserve the relative order of elements in each list. MediaOrderConflictWarning is issued if the order can’t be preserved.

comment:2 by Johannes Maron, 7 years ago

Hello there!

I was the guy who wrote the implicit and none zen conform code ;) Do you happen to have a trace that warning or an example that I could reproduce it myself?

That aside, Tim is correct, the changes are required in #28377. I would be curious myself how to improve it. I currently don't see how not throwing a warning would improve verbosity of the code and its behavior. Could you elaborate more on how you patch would would work?

I the mean time I will have a look at the package you talked about to get a better understanding of the problem.

Cheers
-Johannes

comment:3 by clincher, 7 years ago

Hi guys, thanks for fast response!

I know that static assets must not be duplicated as well as follow predefined order. The easiest way to do it will be like that:

for item in list_2:
    if item not in list_1:
        list_1.append(item)

it will follow all the orders and not duplicate anything.

Personally I find it overcaring to throw the warning about unmatching order in different lists (Media). However if you are sure it is super necessary, than I don't find the current logic proper. I barely experience problem with reading other's people code, but the current method blowed me. However I think the current algorithm is not correct (as it shown with data I provided in the ticket) and probably it's not possible way to solve it.

For throwing the warning (if you really badly need it, guys), we can do that way:

order_list = []
for item in list_2:
    try:
        order_list.append(list_1.index(item))
    except ValueError:
        pass
if order_list is not sort(order_list):
    OrderWarning(...)

So, the idea will be that we can associate order of first and second list and if it is not strictly incremental then throw the warning.

However, seriously, guys, I find it too much and still not perfect, because there can be cases when in list_2 will be additional paths before duplicated paths, how shall we manage them? We must make the method work as expected and clear: merge lists of paths in the proper way, not as it is now. In other words better we will have working explicit solution without warning rather than buggy solution with warning. What do you think?

P.S. What if there are 2 lists like that:

list_1= [path1, path2, path3]
list_2 = [path4, path2, path5]

how shall we merge it?

Version 3, edited 7 years ago by clincher (previous) (next) (diff)

comment:4 by Tim Graham, 7 years ago

Have you read the documentation about how merging is supposed to work?

If you want jQuery included before the Google Maps stuff, then you need to reorder list_2 so that jQuery is before it. Something like ['admin/js/vendor/jquery/jquery.js', 'admin/js/jquery.init.js', 'admin/js/inlines.js', '​https://maps.googleapis.com/maps/api/js?libraries=places&key=', 'mapwidgets/js/jquery_class.js', 'mapwidgets/js/django_mw_base.js', 'mapwidgets/js/mw_google_point_field.js'].

Is there a reason you can't do that?

In older versions of Django, the resulting Media only happened to work because jQuery was used in list_1. I think the new ordering logic that tries to preserve relative ordering in the two media lists is more intuitive.

comment:5 by Johannes Maron, 7 years ago

Hi @clincher,

let me try to go though you points one by one:

This

for item in list_2:
    if item not in list_1:
        list_1.append(item)

does not work. It avoids duplicates but does not preserve the order. This was the whole point of #28377

This I don't really understand, but it does not preserve the order ether. You will have to go though it reversed.

order_list = []
for item in list_2:
    try:
        order_list.append(list_1.index(item))
    except ValueError:
        pass
if order_list is not sort(order_list):
    OrderWarning(...)

Your example

list_1= [path1, path2, path3]
list_2 = [path4, path2, path5]

does work, as such:

>>> list_1= ['path1', 'path2', 'path3']
>>> list_2 = ['path4', 'path2', 'path5']
>>> from django.forms.widgets import Media
>>> Media.merge(list_1, list_2)
['path1', 'path4', 'path2', 'path3', 'path5']

It does preserve the order of both initial lists.

I know this is a difficult topic and the algorithm isn't easy to understand, but to my knowledge it's the only way to achieve both uniqueness and order.
The warning is just so that you are alerted that your widget setup might now work, which it doesn't in your case. I would consider this a good thing, because it alerted you to the issue of sorting.

comment:6 by clincher, 7 years ago

Thanks for the pointing to my fault, @Tim
Thanks a lot for explanaition, @ Johannes

Then everything works fine in the merge method and the actual issue is not in the forms app, but django.admin.helpers in the method media of InlineAdminFormSet, line 307 shall be just changed from to

media = self.formset.media + self.opts.media

to

media = self.opts.media + self.formset.media

Because this is a place where it put opts.media (jquery.js, jquery.init.js, inlines.js) before the formset.media where it actually utilized.

Shall I create a patch or?

Cheers, Vasiliy

comment:7 by Johannes Maron, 7 years ago

Hi Vasiliy,

sure please create a patch. I would be happy to review it.
A little side note, I remember that the admin already has some form media defined that includes jQuery. I don't believe that is needed anymore. Actually jQuery should only be imported if actually used by a widget. So the only place the jQuery media should be included is there.
This duplication could be the reason for your troubles or make your patch more difficult. Fingers crossed! Anyways, I am here to help should you need me :)

-Joe

comment:8 by clincher, 7 years ago

Component: Formscontrib.admin
Easy pickings: set
Has patch: set
Keywords: formset admin added; forms removed

I made the pull request https://github.com/django/django/pull/9404

Not sure if I have to write about it here

Cheers, Vasiliy

comment:9 by clincher, 7 years ago

Summary: merge method of class Media doesn't work properlyInlineAdminFormSet breaks order of media

comment:10 by Claude Paroz, 7 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

comment:11 by Tim Graham, 7 years ago

Did you test the fix? In my testing it does not resolve the issue (as I would expect). The order in which the two lists are combined should make no difference. The relative ordering of the elements in the two lists is preserved. I believe django-map-widgets must fix this issue as I outlined in comment:4. I'll take a closer look and perhaps propose a fix there.

comment:12 by Tim Graham, 7 years ago

I take that last comment back. I made a mistake in testing and I see that it's working now. I'll work on a test.

comment:13 by Tim Graham, 7 years ago

Easy pickings: unset
Needs tests: unset
Summary: InlineAdminFormSet breaks order of mediaInlineAdminFormSet should include InlineModelAdmin's Media before its formset's Media

I added a test to the PR.

comment:14 by Claude Paroz, 7 years ago

Triage Stage: AcceptedReady for checkin

comment:15 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: newclosed

In 03974d81:

Fixed #28866 -- Made InlineAdminFormSet include InlineModelAdmin's Media before its formset's Media.

This provides better backwards compatibility following refs #28377.

comment:16 by Tim Graham <timograham@…>, 7 years ago

In 6ece69a7:

[2.0.x] Fixed #28866 -- Made InlineAdminFormSet include InlineModelAdmin's Media before its formset's Media.

This provides better backwards compatibility following refs #28377.

Backport of 03974d81220ffd237754a82c77913799dd5909a4 from master

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