Ticket 13023: Admin inlines, max_num, and extra
Gabriel Hurley
status update as a ticket that "will require some significant design
work". Since I've accepted that ticket I'd love to get some feedback
from folks.
The ticket: http://code.djangoproject.com/ticket/13023
The ticket was opened because the new javascript "Add Another"
functionality for admin inlines caused a useful (though possibly
unintended) feature to be removed: when setting extra to 0 you could
display existing inlines without allowing any new ones to be created.
The new javascript made this impossible because the "Add Another"
button was controlled by max_num, and ignored a value of 0.
When I tracked back through the code, I realized there was a more
fundamental problem: the javascript ignored a value of 0 because
max_num has a default value of 0, and all the code using it had taken
to equating max_num = 0 with being "off". So you can't actually have a
maximum of 0. It's not possible.
My patch for the ticket restored the desired behavior for admin
inlines, but I'll admit it was a bit of a hack and didn't address the
larger problem. I wasn't sure the larger problem was appropriate to
fix in the 1.2 timeframe.
So what is the appropriate course here? Should the default value and
behavior of max_num be changed? Does the entire feature need to be re-
evaluated? Are there other issues involved that I'm missing entirely?
I'm happy to work on the ticket (and have a reasonable grasp on that
area of the codebase), but I need some direction.
Thanks so much!
- Gabriel
Iván Raskovsky
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to django-develop...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Gabriel Hurley
Ideally a complete solution would have the following characteristics:
1. Makes behavior consistent with or without Javascript enabled. I
personally would argue against any solution that caused extra and
max_num to have different effects with and without JS.
2. Preserves existing behaviors from 1.1 for both extra and max_num.
If those behaviors become "features" instead of "unintended side-
effects" that's cool, but IMHO removing those behaviors doesn't
benefit Django overall. Even if it means codifying the "feature" into
something more like readonly that is no longer tied to max_num and
extra.
3. Clarifies the meaning of max_num = 0. Currently max_num being 0 has
a very odd behavior, and while it would be somewhat silly to create an
Admin that had inlines but said the maximum number was 0, it seems
equally strange that if I deliberately set max_num to 0 that it is, in
fact, ignored.
I guess in a sense, I really see three separate but related issues
here. Anyhow, that's where I'm at with it. I'd love to hear from some
of the core devs, since they've already given it some thought, and
will make the ultimate decision about any changes that take place.
- Gabriel
> > django-develop...@googlegroups.com<django-developers%2Bunsubscr i...@googlegroups.com>
Gabriel Hurley
1. Change the default value of max_num from 0 to -1. That's a pretty
standard solution to indicating an "off" value for something with a
range of [0, ∞]. That way when max_num and extra are *both* explicitly
set to 0, the "Add Another" link could be removed. For the vast
majority of people who *don't* explicitly set max_num to 0, it would
be functionally identical to the current status of things in 1.2-beta.
It would also bring the javascript-enabled and noscript behaviors
closer into line with each other. Even better it means that the
behavior of extra is completely untouched. This solution would also
mean that max_num = 0 (which should be a valid value even if it's not
a common usage) is no longer ignored. Obviously the docs and tests
would be updated in conjunction with all that.
2. Create a readonly attribute for inlines as a completely new
feature. This would default to False, and setting it to True would
cause the desired behavior of preventing new inlines from being
created. It would override both extra and max_num. This solution would
be more explicit (as compared to the setting both max_num and extra to
0), but I feel it doesn't address the deeper issue around max_num.
I guess it ultimately comes down to wether you view this as a bug or a
feature. It has aspects of both. Calling it a "new" feature isn't
quite right because it already exists in 1.1, it just wasn't
intentional. If this is determined to be a feature, I'll open a new
bug ticket about max_num because I do think that should be corrected.
But it doesn't absolutely have to be corrected in connection with this
ticket.
Feedback?
- Gabriel
Russell Keith-Magee
Sorry for taking so long to get back to you on this. I certainly
appreciate the effort you've put into the analysis.
On Fri, Mar 19, 2010 at 6:03 AM, Gabriel Hurley <gab...@gmail.com> wrote:
> There are two possible solutions, as I see it:
>
> 1. Change the default value of max_num from 0 to -1. That's a pretty
> standard solution to indicating an "off" value for something with a
> range of [0, ∞]. That way when max_num and extra are *both* explicitly
> set to 0, the "Add Another" link could be removed. For the vast
> majority of people who *don't* explicitly set max_num to 0, it would
> be functionally identical to the current status of things in 1.2-beta.
> It would also bring the javascript-enabled and noscript behaviors
> closer into line with each other. Even better it means that the
> behavior of extra is completely untouched. This solution would also
> mean that max_num = 0 (which should be a valid value even if it's not
> a common usage) is no longer ignored. Obviously the docs and tests
> would be updated in conjunction with all that.
This seems like the better approach, although I would use None rather
than -1 as a more Pythonic alternative i.e., "max_num = None" says
"there is no max number"; max_num=0 says "there is a maximum of 0".
However, I can see two downsides.
Firstly, the original use case that raised this problem (i.e., someone
that wants to show exactly 2 inlines, with no extras allowed) will
require code changes. I'm not convinced this is a show-stopper, but it
is something that we should mention in the release notes as an edge
case backwards incompatibility if you were relying on one particular
interpretation of the docs.
Secondly -- and more concerning -- what happens if you set max_num = X
(where X is any non-null value), and you already have > X inlines? For
example, if you have 4 inline related objects, and you set max_num =
2, what happens to the extra 2 objects?
This is actually already a problem with 1.1, but if we're going to
start getting esoteric on the interpretation of max_num, it's doubly
important we get this right.
So, is the interpretation of max_num:
1) Only display max_num inlines
2) Always display all inlines, but only allow max_num inlines to be
saved (and raise a validation error for the remainder)
3) Don't allow extra inlines to be added past the limit of max_num;
if a form already has more than max_num inlines, allow them to remain,
but prevent any extra inlines being added.
The current implementation (1) is clearly wrong, if only because it's
possible to display an object and *not* see all the inlines.
Whether (2) or (3) is the right response depends on whether we
consider the ModelAdmin to be a validation condition or a display
guideline. (2) enforces ModelAdmin as a validation condition; (3)
treats it as display guidelines. My initial response is that (3) is
correct (since you could easily add other related objects
programatically, or using a different form), but I'm willing to be
convinced otherwise.
The interpretation here also reflects back on the original problem. In
the original problem, B had a FK on A, so A allowed inline B's in the
admin. B was a complex object, so not all the fields of B were
displayed in the inline. The inlines on A were made available as a
convenience to edit the most notable properties of B. If you wanted to
add a new B, you used the full admin interface for B, and set the FK
to point to a specific instance of A. As a consequence of this, you
can't add new B's via the inline -- but you also don't know a prioiri
how many B's will actually be there. The constraint required here is
"Don't allow any more B's to be added", not "don't allow more than N
related B's". Again, (3) is the useful interpretation here, since
ModelAdmin isn't a validation condition, it's display guidelines.
Yours,
Russ Magee %-)
Gabriel Hurley
being the most useful and beneficial.
The first concern about the original use case is something I'd
considered, but I don't think it can be helped without creating an
ugly hack (like my original patch on the ticket). The change should be
noted in the docs, but thankfully it's an incredibly easy fix for
anyone who wants to keep using that "feature".
I think we both agree about the second concern. All existing inlines
should be displayed. Since max_num and extra have no actual DB
constraints backing them up, I've always seen them as display
guidelines.
With that, I'll get to work on a proper solution over the weekend
(code changes, docs, and tests) and will have it done with time to
spare for 1.2.
Thanks so much!
- Gabriel
On Mar 18, 7:45 pm, Russell Keith-Magee <freakboy3...@gmail.com>
wrote:
Gabriel Hurley
discussed in this thread.
It makes max_num default to None instead of 0. This makes the meaning
of max_num clearer as per Russ above, 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 the OP of the 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 test and review, but at this point the patch should be
complete!
Rolando Espinoza La Fuente
I've just opened a new ticket http://code.djangoproject.com/ticket/13521
tl;dr:
if ((maxForms.val() != '') && (maxForms.val() <= totalForms.val())) {
val() function returns a string, resulting "10" <= "2" which is True
in javascript.
~Rolando
> You received this message because you are subscribed to the Google Groups "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com.
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.