wrong index assigned when creating a new inline

[Simon Bächler]

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.

[Matthias Kestenholz]

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)

[Matthias Kestenholz]

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.

[Arthur de Jong <arthur@…>]

alternate fix

[Arthur de Jong <arthur@…>]

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.

[Matthias Kestenholz]

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.

[Ramiro Morales]

Can't be RFC because has no tests.

[Matthias Kestenholz]

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.

[Ramiro Morales]

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.

[Russell Keith-Magee]

Marking as a blocker since it's a regression introduced by a recent change

[Matthias Kestenholz]

Thanks ramiro & russelm -- really appreciating your hard work.

[Russell Keith-Magee]

Two comments:

1. 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.

2. 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).

[Arthur de Jong <arthur@…>]

second try

[Arthur de Jong <arthur@…>]

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).

[Matthias Kestenholz]

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).

[Russell Keith-Magee]

@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.

[Russell Keith-Magee]

Another attempt at a patch for this issue

[Russell Keith-Magee]

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.

[Russell Keith-Magee]

In [15862]:

Fixed #15569 -- Corrected the numbering updates to inlines when rows are added and deleted. Thanks to sbaechler for the report, to Arthur de Jong and mk for the work on the patch, and to Karen Tracey for the last minute testing help.

[Russell Keith-Magee]

In [15863]:

[1.2.X] Fixed #15569 -- Corrected the numbering updates to inlines when rows are added and deleted. Thanks to sbaechler for the report, to Arthur de Jong and mk for the work on the patch, and to Karen Tracey for the last minute testing help.

Backport of r15862 from trunk.

[Arthur de Jong <arthur@…>]

For the record, I can confirm that this also works with our custom widget. Thanks for the quick response.

[Jacob]

Milestone 1.3 deleted