#15569 closed (fixed)
wrong index assigned when creating a new inline
| Reported by: | Simon Bächler | Owned by: | nobody |
|---|---|---|---|
| Component: | contrib.admin | Version: | 1.2 |
| Severity: | Keywords: | blocker | |
| Cc: | Matthias Kestenholz | Triage Stage: | Accepted |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
When I create a new inline in the admin interface and the inline contains a UL or span, the indexes of the nested elements are not consistent.
I.E.: <ul id="id_mediafilecontent_set-2-position"><li><input id="id_mediafilecontent_set-1-position_0">...
This is causing troubles when adding special functions to inlines in the admin interface.
The code that is causing the problem is in contrib/admin/media/js/inlines.js line 55 and 100.
When I add UL to the function in line 100, then the code works for UL.
Attachments (5)
Change History (23)
comment:1 by , 13 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
by , 13 years ago
| Attachment: | 15569.dump.patch added |
|---|
by , 13 years ago
| Attachment: | 15569.dumb.patch added |
|---|
comment:2 by , 13 years ago
| Has patch: | set |
|---|---|
| Patch needs improvement: | set |
Something like the attached patch fixes this bug. It is very dumb though, running row.find("*") several times should absolutely not be necessary. I'm not too familiar with this code and all its edge cases.
Not setting the "Needs tests" flag because we do not have javascript tests AFAIK.
comment:3 by , 13 years ago
I was also bitten by this (found in 1.3-rc-1, not in 1.2.1). We came up with a slightly different patch that does not change what is called exactly but just ensures that the values are set consistently.
comment:4 by , 13 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
Right. That is a better patch, as long as the logic isn't fixed definitively. Promoting to Ready for checkin because this is a bugfix which would be a pity to miss for 1.3.
comment:5 by , 13 years ago
| Needs tests: | set |
|---|---|
| Triage Stage: | Ready for checkin → Accepted |
Can't be RFC because has no tests.
follow-up: 7 comment:6 by , 13 years ago
Yes, we do not have any tests. However, we do not have any tests for javascript behavior at all currently. Not fixing a JS bug when we do not even have any automated tests for scripts (not even a blessed testing framework...) isn't helpful at all.
comment:7 by , 13 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
Replying to mk:
Yes, we do not have any tests. However, we do not have any tests for javascript behavior at all currently. Not fixing a JS bug when we do not even have any automated tests for scripts (not even a blessed testing framework...) isn't helpful at all.
Yes, I discovered that (we don't have tests for JS changes to the DOM) when trying to create the regression test, sorry for that, I will move this back to RFC although we need to also touch the minified version of inlines.js before committing.
FWIW this was introduced in r15422.
comment:8 by , 13 years ago
| Keywords: | blocker added |
|---|
Marking as a blocker since it's a regression introduced by a recent change
comment:10 by , 13 years ago
Two comments:
- Is it really too much to ask that when you report a bug, you provide a clear set of instructions for how to reproduce the bug? I've spent half an hour spelunking through admin source code trying to work out what magic incantations would allow me to get a UL in an inline. For future reference -- have a model field with choices, and add radio_fields to the InlineModelAdmin.
- Neither proposed patch fixes the problem without reverting the fix introduced by r15422. Moving the increment corrects the id number for the UL, but doesn't fix the numbering of LABEL elements associated with the associated with LIs on the UL. Using a glob to capture child elements gets the counter consistent for all elements, but gets the count wrong when you add three rows, delete one, add a new row (the last added new row will have an id 1 less than it should be).
comment:11 by , 13 years ago
Sorry to not provide clearer instructions. It took me a couple of hours to find this myself and I don't particularly like debugging Javascript ;)
In my case we were using a custom widget that also generates <div id="{{ id }}_related">.
I don't know why some things are updated using updateElementIndex() and some are updated by simple search and replace so I implemented the change that should have minimal consequences. Attached is a patch that cleans things up more also works for me (also works in the case described in 14303). The only remaining use of nextIndex is for some code I can't be bothered to check at this time (sorry).
comment:12 by , 13 years ago
Russell: Sorry for not being clear enough earlier. I had the impression that this would not happen in stock admin with stock widgets (I forgot about the radio_fields option)
I tested the second patch by Arthur in Chrome and Firefox and it fixes the problem we've seen while removing code (which is a good thing -- the same work isn't done twice anymore).
comment:13 by , 13 years ago
| Triage Stage: | Ready for checkin → Accepted |
|---|
@arthur - this latest patch looks closer, but it still has a problem -- after deletion, the enclosing <TR> for a row on a TabularInline doesn't have the same ID as the internal elements for every row after the deleted row. The internal elements get decremented; the TR's id doesn't.
comment:14 by , 13 years ago
| Needs tests: | unset |
|---|---|
| Patch needs improvement: | unset |
I *think* the patch I've just uploaded does the job; it's an amalgam of several bits from the patches provided, plus a little extra. If those affected can sign off that it fixes their problem, I'll commit.
comment:17 by , 13 years ago
For the record, I can confirm that this also works with our custom widget. Thanks for the quick response.
It seems the same work is done twice. Once directly below row.find("*") with the wrong index (correct index + 1), and a second time in row.find("input,select,textarea,label,a").each(function(){updateElementIndex.... , but this time for a reduced set of HTML tags only.
Stock django.contrib.admin does not use additional tags by default, it is easily possible to introduce those by using either custom widgets or other admin customizations (such as those FeinCMS uses)