Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#15569 closed (fixed)

wrong index assigned when creating a new inline

Reported by: sbaechler Owned by: nobody
Component: contrib.admin Version: 1.2
Severity: Keywords: blocker
Cc: mk Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

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)

15569.dump.patch (1015 bytes) - added by mk 4 years ago.
15569.dumb.patch (1015 bytes) - added by mk 4 years ago.
fix-nextindex-update.patch (823 bytes) - added by Arthur de Jong <arthur@…> 4 years ago.
alternate fix
fix-nextindex-update2.patch (2.6 KB) - added by Arthur de Jong <arthur@…> 4 years ago.
second try
t15569-rc1.diff (8.8 KB) - added by russellm 4 years ago.
Another attempt at a patch for this issue

Download all attachments as: .zip

Change History (23)

comment:1 Changed 4 years ago by mk

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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)

Changed 4 years ago by mk

Changed 4 years ago by mk

comment:2 Changed 4 years ago by mk

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

Changed 4 years ago by Arthur de Jong <arthur@…>

alternate fix

comment:3 Changed 4 years ago by 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.

comment:4 Changed 4 years ago by mk

  • Triage Stage changed from Accepted to 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 Changed 4 years ago by ramiro

  • Needs tests set
  • Triage Stage changed from Ready for checkin to Accepted

Can't be RFC because has no tests.

comment:6 follow-up: Changed 4 years ago by 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.

comment:7 in reply to: ↑ 6 Changed 4 years ago by ramiro

  • Triage Stage changed from Accepted to 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 Changed 4 years ago by russellm

  • Keywords blocker added

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

comment:9 Changed 4 years ago by mk

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

comment:10 Changed 4 years ago by russellm

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

Changed 4 years ago by Arthur de Jong <arthur@…>

second try

comment:11 Changed 4 years ago by 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).

comment:12 Changed 4 years ago by mk

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 Changed 4 years ago by russellm

  • Triage Stage changed from Ready for checkin to 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.

Changed 4 years ago by russellm

Another attempt at a patch for this issue

comment:14 Changed 4 years ago by russellm

  • 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:15 Changed 4 years ago by russellm

  • Resolution set to fixed
  • Status changed from new to closed

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.

comment:16 Changed 4 years ago by russellm

In [15863]:

(The changeset message doesn't reference this ticket)

comment:17 Changed 4 years ago by Arthur de Jong <arthur@…>

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

comment:18 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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