Opened 14 years ago

Closed 4 years ago

#13068 closed Bug (fixed)

The "Add another [inline object]" javascript doesn't respect prepopulated_fields settings

Reported by: hejsan Owned by: Sean Brant
Component: contrib.admin Version: 1.7
Severity: Normal Keywords: prepopulated_field inline add
Cc: hr.bjarni+django@…, brant.sean@…, Gabriel Hurley, rafael.capucho@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: yes

Description

If I have an inline form with a prepopulated field, i.e:

class SomethingInline(admin.TabularInline):
    model = Something
    prepopulated_fields = {'title_slug': ('title',)}
    extra = 2

Then the extra inlines added by the new javascript dont automatically populate the title_slug field when entering a title.

BTW: The alternating row colors also dissappear for me in firefox when I click "Add another.."

Attachments (14)

prepopulate.diff (6.0 KB ) - added by Sean Brant 14 years ago.
prepopulate-with-9983-fix.diff (6.2 KB ) - added by Sean Brant 14 years ago.
Also fixes #9983
models.py (1.2 KB ) - added by Carl Meyer 14 years ago.
models.py for easily testing this bug and associated ones
prepopulate-with-9983-and-9784-fix.diff (7.3 KB ) - added by Sean Brant 14 years ago.
prepopulate-with-9983-and-9784-fix.2.diff (7.0 KB ) - added by Sean Brant 14 years ago.
Removed some code that was not being used
prepopulate-with-9983-and-9784-fix.3.diff (7.2 KB ) - added by Sean Brant 14 years ago.
Fixed adding the list as part of the css class hack
prepopulate.2.diff (12.3 KB ) - added by Sean Brant 14 years ago.
Fixes this ticket as well as the other tickets mentioned.
prepopulate.3.diff (12.4 KB ) - added by Sean Brant 14 years ago.
prepopulate.4.diff (12.4 KB ) - added by Sean Brant 14 years ago.
Works with r12873
prepopulate.5.diff (12.5 KB ) - added by Sean Brant 14 years ago.
Fixes luke's comments
prepopulate.6-1.3.diff (1.3 KB ) - added by stan <stanislas.guerra__at__gmail.com> 13 years ago.
Patch for the StackedInline formsets (django-1.3).
models.2.py (1.4 KB ) - added by stan <stanislas.guerra__at__gmail.com> 13 years ago.
Simple test models/admin with fieldset set in StackedInline.
screenshot-stackedInline.png (108.8 KB ) - added by stan <stanislas.guerra__at__gmail.com> 13 years ago.
Screenshot of the stackedInline
13068.inline-prepopulated-fields.diff (2.3 KB ) - added by Julien Phalip 12 years ago.

Download all attachments as: .zip

Change History (51)

comment:1 by Carl Meyer, 14 years ago

milestone: 1.2
Triage Stage: UnreviewedAccepted
Version: 1.2-betaSVN

Confirmed the prepopulated_fields issue.

On Firefox 3.5.8 I don't see any problem with the alternating row colors; in any case, if that actually is a problem in some browsers, it should get its own separate Trac issue.

comment:2 by Rob Hudson, 14 years ago

I have a similar case that seems related. I suspect any widget in an inline that requires events to fire will fail here since the inline addition happens dynamically and a new DOM element is inserted but the events aren't added to the new DOM element.

comment:3 by Rob Hudson, 14 years ago

An update. I added a slug field to an existing project to test this myself and also noticed that the HTML output also contains inline Javascript that looks like the following:

    document.getElementById("id_special_set-__prefix__-header_slug").onchange = function() { this._changed = true; };
    
    document.getElementById("id_special_set-__prefix__-header").onkeyup = function() {
        var e = document.getElementById("id_special_set-__prefix__-header_slug");
        if (!e._changed) { e.value = URLify(document.getElementById("id_special_set-__prefix__-header").value, 200); }
    }

comment:4 by Sean Brant, 14 years ago

Has patch: set
Owner: changed from nobody to Sean Brant

I added a new jQuery plugin called prepopulate, usage: $('input').prepopulate(jquery_object_containing_dependent_inputs, maxLength);. I reworked the code in prepopulate_fields.js.html to use this new plugin. I also fixed the bug this ticket is for, by hooking up the plugin to newly added rows. I have tested this, but with all js fixes it could use some more testing in whatever browser you are using.

comment:5 by Sean Brant, 14 years ago

Cc: brant.sean@… added

by Sean Brant, 14 years ago

Attachment: prepopulate.diff added

comment:6 by Carl Meyer, 14 years ago

Confirmed that this patch does fixe the issue for tabular inlines. Stacked inlines are still broken; the easy solution is to copy-paste the new JS from tabular.html to stacked.html. That seems to have been done before with other JS, but it's nasty; we should find a way to avoid duplicating all that identical JS.

The current patch here also fixes #9264. It does not yet fix #9983 or #9784; I'm looking into how difficult that would be to add.

by Sean Brant, 14 years ago

Also fixes #9983

comment:7 by Sean Brant, 14 years ago

Just attached a patch that fixes 9983. I will post a link to this ticket over on that ticket to keep those interested in the loop.

by Carl Meyer, 14 years ago

Attachment: models.py added

models.py for easily testing this bug and associated ones

comment:8 by Carl Meyer, 14 years ago

Since Django doesn't yet have an automated testing framework for Javascript, I'm attaching a models.py with the necessary fields and admin configuration to manually test this bug as well as #9264, #9983, and #9784. Just create an app with this models.py, add it to INSTALLED_APPS, syncdb, run the admin and add a Question. If all the referenced bugs were fixed, you'd see:

  • The slug1 field populated from both "name" and "pubdate" (including if pubdate is populated via the calendar picker)
  • The slug2 field populated from both "status" dropdown and "name"
  • The same behavior for both tabular and stacked inlines, including if you "add another"

All of my testing so far has been on FF 3.5.8 on Linux. Once the patch seems ready otherwise I can do cross-browser testing on Chrome Windows, Chrome Linux, FF 3.6 Windows, Opera 10 Windows, Safari 4.0 Windows, and IE 8 Windows (incl 7 compat mode).

by Sean Brant, 14 years ago

by Sean Brant, 14 years ago

Removed some code that was not being used

by Sean Brant, 14 years ago

Fixed adding the list as part of the css class hack

comment:9 by Sean Brant, 14 years ago

The only thing i'm not sure of is L100 of tabular.html. The problem is we need to get the target fields list of dependencies but the dynamic fields are added by cloning a hidden set of fields. This works but the current version of jQuery does not support cloning "data" attached to the DOM element. I choose to figure out the data by matching the template field biased on the target fields class names, then get the data from that. This still seems a little nasty, so I open for suggestions. I think one way to fix it would be to dig into inlines.js and add our own clone "data" feature. I held off for now not wanting this patch to get to large.

Also I have not messed with the stacked inline code yet, but that needs to be fixed up once we tighten down the current patch.

comment:10 by Carl Meyer, 14 years ago

Patch needs improvement: set

by Sean Brant, 14 years ago

Attachment: prepopulate.2.diff added

Fixes this ticket as well as the other tickets mentioned.

comment:11 by Sean Brant, 14 years ago

Patch needs improvement: unset

With this latest patch I modified inlines.js so that copying a DOM elements data attribute works.

by Sean Brant, 14 years ago

Attachment: prepopulate.3.diff added

by Sean Brant, 14 years ago

Attachment: prepopulate.4.diff added

Works with r12873

comment:12 by Luke Plant, 14 years ago

This looks pretty good. Some comments:

  • 'console.log()' call stops it from working at all in Firefox.
  • For inlines that have been added via javascript, pre-populating from the drop down boxes is broken (whether I use keyboard or mouse). This is the same for Firefox (3.5.8 for Linux) and Chrome (5.0.342.7 beta for Linux).

If you could fix these, and then if someone can do the cross browser testing on Windows/Mac (like carljm offered), I will commit this. Many thanks.

comment:13 by Sean Brant, 14 years ago

Thanks luke I can look at those issues tonight.

comment:14 by Gabriel Hurley, 14 years ago

Cc: Gabriel Hurley added

I'm happy to help with cross-browser/cross-platform testing. I've got both Mac and Windows machines running most of the major browsers. I'll cc myself here so I know when the new patch goes up.

by Sean Brant, 14 years ago

Attachment: prepopulate.5.diff added

Fixes luke's comments

comment:15 by Luke Plant, 14 years ago

OK, I've been able to confirm the latest patch fixes #13068, #9264, #9983 and #9784 on Chrome 5.0.342.7 beta (Linux), Firefox 3.5.8 (Linux), Opera 10.10 (Linux), Internet Explorer 7, Safari 4.0.4 (Windows). On that basis I doubt there will be problems for IE 8 or Mac browsers, so I will go ahead and commit, but testing on those platforms is welcome.

Many thanks Sean, Carl and Rob for the work on this ticket, I know that javascript can take ages to debug! Thanks also for making it easy to test out.

comment:16 by Luke Plant, 14 years ago

Resolution: fixed
Status: newclosed

(In [12937]) Fixed #13068, #9264, #9983, #9784 - regression with pre-populated fields and javascript inlines, and related bugs.

Thanks to hejsan for the report, and to Sean Brant and Carl Meyer for the patch.

#13068 is a regression caused by the new javascript inline forms in the
admin. The others were existing javascript bugs with prepopulated fields.
Since the solution depends on jQuery and would likely be very hard to
backport without it, it will not be backported to 1.1.X.

comment:17 by Gabriel Hurley, 14 years ago

I was writing this up when the patch got merged, but here's my testing report:

Patch tested successfully on the following platforms & browsers:

Mac OS X:

  • Safari 4
  • Firefox 3.6
  • Chrome 4 (stable channel)

Windows XP:

  • Internet Explorer 8
  • Chrome 4 (developer channel)
  • Firefox 3.6
  • Safari 4

Windows Vista:

  • Internet Explorer 7
  • Chrome 4 (stable)

by stan <stanislas.guerra__at__gmail.com>, 13 years ago

Attachment: prepopulate.6-1.3.diff added

Patch for the StackedInline formsets (django-1.3).

comment:18 by stan <stanislas.guerra__at__gmail.com>, 13 years ago

Easy pickings: unset
Keywords: prepopulated_field inline add added
Resolution: fixed
Severity: Normal
Status: closedreopened
Type: Uncategorized
Version: SVN1.3

I reopen the ticket because It still does not works with the StackedInline.

The problem is in initPrepopulatedFields javascript function in the django/contrib/admin/edit_inline/{stacked,tabular}.html files and in the way the selector are set in the html.

Having the following in my admin.py:

class CahierInline(admin.StackedInline):
    model = models.Cahier
    extra = 1
    fieldsets = (
        (None, {
            'fields': (('ordre',), ('nom', 'euid', 'valide',), 'nom_court',)
        }),
        ('périodicité', {
            'fields': ('periodicite', 'jours_de_parution_semaine', 'jours_de_parution_mois',)
        }),
    )
    prepopulated_fields = {'euid': ('nom',)}

For a TabularInline, the html produced looks like that :

<td class="euid prepopulated_field">
<input id="id_cahiers-1-euid" class="vTextField" type="text" maxlength="50" name="cahiers-1-euid">
</td>

And with StackedInline :

<div class="form-row nom euid valide prepopulated_field">
  <div class="field-box">
    <label class="required" for="id_cahiers-1-nom">Nom:</label>
    <input id="id_cahiers-1-nom" class="vTextField" type="text" maxlength="100" name="cahiers-1-nom">
  </div>
  <div class="field-box">
    <label class="required inline" for="id_cahiers-1-euid">Clé:</label>
    <input id="id_cahiers-1-euid" class="vTextField" type="text" maxlength="50" name="cahiers-1-euid">
  </div>
  <div class="field-box">
    <input id="id_cahiers-1-valide" type="checkbox" name="cahiers-1-valide" checked="checked">
    <label class="vCheckboxLabel inline" for="id_cahiers-1-valide">Valide</label>
  </div>
</div>

The lookup in initPrepopulatedFields for the pre-populated input cannot works in the last case (and is not possible unless with a crap) because the class euid (via field.field.name) is too high in the DOM.

There is at last two way to correct that :

  1. put the field.field.name class on the div.field-box ;
  2. change the lookup in initPrepopulatedFields for django/contrib/admin/edit_inline/{stacked,tabular}.html.

I am a bit surprised that we have two different files admin/edit_inline/{stacked,tabular}.html with one using an include to render the fieldset and the other doing it in-situ but having the similar bits of javascript.
Those 2 files being factorizables (at least for the js part), adding some differences in the behavior is not the best way so my patch implements the first solution.
I didn't run the test suite so it may break some stuff because the render is now like that :

<div class="form-row">
  <div class="field-box nom">
    <label class="required" for="id_cahiers-1-nom">Nom:</label>
    <input id="id_cahiers-1-nom" class="vTextField" type="text" maxlength="100" name="cahiers-1-nom">
  </div>
  <div class="field-box euid prepopulated_field">
    <label class="required inline" for="id_cahiers-1-euid">Clé:</label>
    <input id="id_cahiers-1-euid" class="vTextField" type="text" maxlength="50" name="cahiers-1-euid">
  </div>
  <div class="field-box valide">...
</div>

comment:19 by Sean Brant, 13 years ago

Im unable to confirm that this is not working for StackedInlines. Can you confirm that it does not work when using the attached test models.py/admin.py [1]? Also if you can attach a sample models.py/admin.py that fail that would be helpful in debugging this.

Any other details like what exactly is not working would also be helpful. Thanks!

[1] https://code.djangoproject.com/attachment/ticket/13068/models.py

by stan <stanislas.guerra__at__gmail.com>, 13 years ago

Attachment: models.2.py added

Simple test models/admin with fieldset set in StackedInline.

comment:20 by anonymous, 13 years ago

It works in your example because you don't use fieldsets.

I have modified it to wrap the name and the slugs fields in the same group and then it fails to populate.

http://code.djangoproject.com/attachment/ticket/13068/models.2.py

Cordialement,

Stanislas.

comment:21 by Sean Brant, 13 years ago

I just tried using your attached admin with the fieldset addition and it is still working for me.

by stan <stanislas.guerra__at__gmail.com>, 13 years ago

Screenshot of the stackedInline

comment:22 by stan <stanislas.guerra__at__gmail.com>, 13 years ago

We are talking about the StackedInlines added with the Add Another button, are we ?

http://code.djangoproject.com/attachment/ticket/13068/screenshot-stackedInline.png

I'am running Django-1.3, not the trunk and I have tested with the latest Firefox and Safari.

comment:23 by Sean Brant, 13 years ago

Im sorry I did not test the "Add another" you are correct I see the error as well.

I just tried out your patch and it corrects the problem. Thanks!

comment:24 by Julien Phalip, 13 years ago

Type: UncategorizedBug

comment:25 by Julien Phalip, 13 years ago

UI/UX: set

comment:26 by Jacob, 12 years ago

milestone: 1.2

Milestone 1.2 deleted

by Julien Phalip, 12 years ago

comment:27 by Julien Phalip, 12 years ago

Severity: NormalRelease blocker

[16953] actually introduced a regression as now the prepopulated fields don't work for new inlines of any kind (stacked or tabular). The attached patch fixes it. I'll now work on adding some Selenium tests.

comment:28 by Julien Phalip, 12 years ago

Needs tests: set

comment:29 by Julien Phalip, 12 years ago

Resolution: fixed
Status: reopenedclosed

In [17562]:

Fixed #13068 (again) -- Corrected the admin stacked inline template to allow prepopulated fields to work (Thanks Stanislas Guerra for the report). Also fixed a regression introduced in [16953] where prepopulated fields wouldn't be recognized any more due to the additional "field-" CSS class name prefix.

comment:30 by Frank Wiles, 11 years ago

Resolution: fixed
Status: closedreopened

I'm still seeing this in 1.4.2. It works for any Inlines that are created because of 'extra', but adding additional ones the slug prepopulation doesn't work.

comment:31 by Aymeric Augustin, 11 years ago

Severity: Release blockerNormal

Since the regression was fixed, this isn't a release blocker for 1.5.

in reply to:  30 comment:32 by Julien Phalip, 11 years ago

Replying to frankwiles:

I'm still seeing this in 1.4.2. It works for any Inlines that are created because of 'extra', but adding additional ones the slug prepopulation doesn't work.

I'm afraid I can't reproduce the problem. Could you describe the issue in more detail? In particular, what do you exactly mean by "adding additional ones"? Is that via clicking the "+" green button?

comment:33 by Aymeric Augustin, 11 years ago

Status: reopenednew

comment:34 by bruno@…, 10 years ago

[17562] DoesntWork(tm) on Django 1.5.1 / (Mozilla Firefox 25.0.1, Chrome 31.0.1650.57) / Ubuntu 12.04

The problem comes from jQuery class selectors:

templates/admin/prepopulated_fields_js.html line 22:

$('.empty-form .form-row .field-{{ field.field.name }})

should be

$('.empty-form .form-row.field-{{ field.field.name }})

(no whitespace between ".form-row" and ".field-name")

Same thing in static/admin/js/inline.js line 246, replacing

dependencies.push('#' + row.find('.form-row .field-' + field_name).find('input, select, textarea').attr('id'));

with

dependencies.push('#' + row.find('.form-row.field-' + field_name).find('input, select, textarea').attr('id'));

fixes the problem.

comment:35 by Rafael Capucho, 9 years ago

Cc: rafael.capucho@… added
Has patch: unset
Patch needs improvement: set
Severity: NormalRelease blocker
Version: 1.31.7

The problem in prepopulated_fields when using inline are incredibly still present in Django 1.7.5.

The solution proposed by bruno 16 months ago works well for Django 1.7.5.

comment:36 by Baptiste Mispelon, 9 years ago

Severity: Release blockerNormal

Hi,

I don't think the release blocker status is warranted here since the regression has been fixed.

Having a testcase for this issue would certainly help moving the ticket forward if you want to take a crack at it.

Thanks.

comment:37 by Mariusz Felisiak, 4 years ago

Has patch: set
Needs tests: unset
Patch needs improvement: unset
Resolution: fixed
Status: newclosed

Closing as fixed, because a remaining issue for prepopulated_fields and StackedInline is handled in #28357.

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