Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#13023 closed (fixed)

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

Reported by: rasca Owned by: gabrielhurley
Component: contrib.admin Version: master
Severity: Keywords:
Cc: rasca7@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

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.

Attachments (2)

13023_inlines_patch.diff (1.8 KB) - added by gabrielhurley 5 years ago.
Restores desired behavior without breaking anything else, corrects documentation.
13023_full_max_num_patch.diff (35.4 KB) - added by gabrielhurley 5 years ago.
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.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 5 years ago by jezdez

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

Changed 5 years ago by gabrielhurley

Restores desired behavior without breaking anything else, corrects documentation.

comment:2 Changed 5 years ago by gabrielhurley

  • Has patch set
  • Needs documentation unset
  • Owner changed from nobody to gabrielhurley
  • Status changed from new to assigned
  • Summary changed from Don't show "+ Add another " in admin inlines to Don't show "+ Add another" in admin inlines when extra = 0

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.

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

comment:4 in reply to: ↑ 3 Changed 5 years ago by gabrielhurley

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.

comment:5 Changed 5 years ago by DrMeers

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.

comment:6 Changed 5 years ago by gabrielhurley

  • Patch needs improvement set

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

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

comment:7 Changed 5 years ago by gabrielhurley

  • Patch needs improvement unset

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!

Changed 5 years ago by gabrielhurley

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.

comment:8 Changed 5 years ago by jezdez

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

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

comment:9 Changed 5 years ago by 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.

comment:10 Changed 5 years ago by gabrielhurley

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.

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

comment:12 Changed 5 years ago by gabrielhurley

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

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

comment:14 in reply to: ↑ 13 ; follow-up: Changed 5 years ago by 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.

comment:15 in reply to: ↑ 14 Changed 5 years ago by carsongee

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.

comment:16 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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