Don't show "+ Add another" in admin inlines when extra = 0

[rasca]

Up to 1.1 when dealing with inlines in the admin one could set extra=0 to prevent a user to add a new related model.
With the new dynamic inlines, this can't be prevented.

I'm not sure if this is a Bug or not, but certainly we've lost a functionality we had in 1.1, and the code works differently

The 'extra' doc says:
"This controls the number of extra forms the formset will display in addition to the initial forms."
and
"Extra forms for inlines will be hidden and replaced with a link to dynamically add any number of new inlines for users with Javascript enabled."
It isn't clear if extra refers to the amount of extra inlines in the initial display or the extra amount of dynamic additions permitted.

I can think of 2 alternatives, changing the behavior of "+ Add another" when extra = 0 (will allow the user the same functionality as when javascript disabled), or allowing a extra = None.
I support the first alternative, so we don't to have to change our code, and it maintains the same usability with or without js.

Use case:
I have models A and B. Model B has a fk to model A, and it's shown as inline in the A's change_form.
Model B is too complex to be created in an inline (or has inline's itself).
For usability's sake I often show B as inlines in A, so the user can see the list of B while editing A and allow them to delete a certain B from A (I only show a few readonly fields of B).
Another use is to allow easy drag and drop ordering for a set of B.

[Gabriel Hurley]

Restores desired behavior without breaking anything else, corrects documentation.

[Gabriel Hurley]

I added a patch which restores the desired behavior by automatically setting max_num to the length of the queryset if both max_num and extra are zero. The result seems to be completely in line with the expected behavior from 1.1 and doesn't introduce any new effects that I can see. Overall I love the new "add another" button.

I also added/updated docs for this, and since this is my fifth or sixth patch to Django, I added my name to the contributors list... I hope that's okay :-)

In the larger scope of things, the default for max_num being zero seems counter-intuitive. Rather than a value of zero for max_num actually meaning that there can be no forms/modelforms/inlines, the value of zero is actually just ignored because it's the default. This patch wouldn't be necessary if that weren't the case. However, for backwards compatibility I understand that changing the default value of max_num is a complete non-starter.

[Greg Brown]

Another use case which is affected here - I have a flatpages-esque app with content stored in a Block model, which has a ForeignKey to Page. When a Page is saved, it introspects the selected template to generate the required blocks, which can then be edited inline. A page template can include any number of blocks, so max_num isn't an option here, but the number and type of blocks are also completely dependent on the template, so it makes no sense to have "Add another block" at the bottom. Pre-1.2 I just set extra=0 and it was fine; now I can't do that.

[Gabriel Hurley]

Replying to gregplaysguitar:

Another use case which is affected here - I have a flatpages-esque app with content stored in a Block model, which has a ForeignKey to Page. When a Page is saved, it introspects the selected template to generate the required blocks, which can then be edited inline. A page template can include any number of blocks, so max_num isn't an option here, but the number and type of blocks are also completely dependent on the template, so it makes no sense to have "Add another block" at the bottom. Pre-1.2 I just set extra=0 and it was fine; now I can't do that.

It's not entirely clear to me what your code is actually doing, but if this patch does not fix your issue, you could try taking the change I applied to BaseInlineFormSet and moving it to BaseModelFormSet instead. The bug report was about Admin Inlines, though, and I can't guarantee that applying this same fix to BaseModelFormSet won't have unintended side-effects.

This patch fixes this ticket. You might want to open a new ticket with more detailed information on how to reproduce your bug so we can look at it in its own right.

[Simon Meers]

We should be careful not to throw away something useful here; in some cases an "add" link is very useful even if extra is set to 0. For example, an inline which is not used in 90% of cases so should not clutter the interface, but can be added when needed. Perhaps a hybrid solution is possible, but may need to utilise an additional flag.

[Gabriel Hurley]

New patch in progress. See discussion and design decision here:

​http://groups.google.com/group/django-developers/browse_frm/thread/95c801018e13185e

[Gabriel Hurley]

I've added an all-new patch that makes max_num default to None instead of 0. This makes the meaning of max_num clearer as per the discussion on django-dev, and makes the behavior of the dynamic "Add Another" admin inline link fall into line with the no-script behavior. Explicitly setting both max_num and extra to 0 achieves the functionality desired by this ticket.

The patch passes the complete test suite. A handful of new tests were added to prevent future regression. Only three existing tests needed real updating, though many others had a hard-coded value of 0 for the max_num default. The tests all passed even with that hard-coded value, but just for completeness I changed them all to None (the new default).

Lastly, the patch also updates the docs for all mentions of max_num.

Please feel free to test and review, but at this point the patch should be complete!

[Gabriel Hurley]

Updated: Noticed another place in inlines.js that needed correction. Complete patch to make max_num default to None instead of 0, including tests and docs.

[Jannis Leidel]

(In [12872]) Fixed #13023 - Removed ambiguity with regard to the max_num option of formsets and as a result of admin inlines. Thanks to Gabriel Hurley for the patch.

[sehmaschine@…]

strange, but this doesn´t work for me:
replacing line 81 in inline.js
if ((maxForms.val() != ) && (maxForms.val() <= totalForms.val())) {
to
if ((maxForms.val() != "") && (maxForms.val()-totalForms.val()) < 0) {
does the trick.

[Gabriel Hurley]

sehmaschine, can you describe what *exactly* isn't working? steps to reproduce would be extremely helpful as well.

I suspect something else is at fault here because you say changing the single quotes to double quotes fixes it for you, but if you look at the minified code (which is actually being used by the admin) the minifier *already* converted the single quotes to double quotes.

There are regression tests for the backend code and I've tested the js in FF3.6, Chrome 4, Safari 4, IE7 and IE8... I'm not sure what could be misbehaving for you.

[anonymous]

sorry, I´ve been posting the wrong code ...
this is the fix

if ((maxForms.val() != 0) && (maxForms.val()-totalForms.val()) < 0) {

if max_num is not defined, I´m having a maxForm value of "0" and not "".
line 36 of inline.js has the following note

// note that max_num = None translates to a blank string.

that´s not true for me ...
tested with firefox 3.6.2 on osx, django rev 12852.

[Gabriel Hurley]

Did you mean to say you're running on 12852? I'm guessing that's a typo and you meant 12872, 'cuz that's where this patch was merged. If you're actually running 12852 then there's your problem...

[Carson Gee]

Can we get this listed in the release notes for 1.2 as a backwards incompatible change? I have an application that lets the user choose how many forms to have in a formset by storing a value in the database, and I had to go through source code for a quite a while when testing 1.2 rc1 for why all of the sudden I didn't have any forms in my formsets. I may be an anomaly, but it could save some people time if it was in the release notes.

[Ramiro Morales]

Replying to carsongee:

Can we get this listed in the release notes for 1.2 as a backwards incompatible change? I have an application that lets the user choose how many forms to have in a formset by storing a value in the database, and I had to go through source code for a quite a while when testing 1.2 rc1 for why all of the sudden I didn't have any forms in my formsets. I may be an anomaly, but it could save some people time if it was in the release notes.

Please review the patch attached to #13524 and suggest any changes you deem appropiate.

[Carson Gee]

Replying to ramiro:

Replying to carsongee:

Can we get this listed in the release notes for 1.2 as a backwards incompatible change? I have an application that lets the user choose how many forms to have in a formset by storing a value in the database, and I had to go through source code for a quite a while when testing 1.2 rc1 for why all of the sudden I didn't have any forms in my formsets. I may be an anomaly, but it could save some people time if it was in the release notes.

Please review the patch attached to #13524 and suggest any changes you deem appropiate.

The patch looks good to me. Thanks for that.

[Jacob]

Milestone 1.2 deleted