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

[clincher]

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

[Tim Graham]

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.

[Johannes Maron]

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

[clincher]

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? Also we have to consider not only theoretical things but practical. What are duplicated paths? Most likely it's some common libraries. It doesn't matter if jquery and, let's say, d3.js will be included as ['jquery.js', 'd3.js'] or ['d3.js', 'jquery.js'], what is more important is the order of libraries after these duplicates, isn't it?

[Tim Graham]

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.

[Johannes Maron]

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.

[clincher]

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

[Johannes Maron]

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

[clincher]

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

Not sure if I have to write about it here

Cheers, Vasiliy

[Tim Graham]

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.

[Tim Graham]

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.

[Tim Graham]

I added a test to the PR.

[Tim Graham <timograham@…>]

In 03974d81:

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

This provides better backwards compatibility following refs #28377.

[Tim Graham <timograham@…>]

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