#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 , 7 years ago
Cc: | added |
---|---|
Type: | Uncategorized → Bug |
comment:2 by , 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 , 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?
comment:4 by , 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 , 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 , 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 , 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 , 7 years ago
Component: | Forms → contrib.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 , 7 years ago
Summary: | merge method of class Media doesn't work properly → InlineAdminFormSet breaks order of media |
---|
comment:10 by , 7 years ago
Needs tests: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:11 by , 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 , 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 , 7 years ago
Easy pickings: | unset |
---|---|
Needs tests: | unset |
Summary: | InlineAdminFormSet breaks order of media → InlineAdminFormSet should include InlineModelAdmin's Media before its formset's Media |
I added a test to the PR.
comment:14 by , 7 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
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: