#6191: Admin delete view doesn't show all items in some circumstances
-------------------------------------------+--------------------------------
Reporter: nicklane | Owner: carljm
Status: new | Milestone: 1.2
Component: django.contrib.admin | Version: SVN
Resolution: | Keywords:
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
-------------------------------------------+--------------------------------
Comment (by carljm):
Thanks for the review, very helpful.
Replying to [comment:23 russellm]:
> * I got a test failure running the
admin_util.NestedObjectsTests.test_siblings test (got [0, [2, 1]],
expected [0, [1, 2]]). The problem appears to be that NestedObjects uses a
set to collect objects, which therefore doesn't give a guaranteed result
order. One fix is to use a list instead if a set; the other fix is to
modify the test so it isn't order dependent. I'm fairly certain just using
a list is all that is required - I couldn't see anything obvious that was
relying on the fact that sets were unique or easy to look up.
Right, I'll just use a list. Somehow I thought the set was to prevent
duplicates, but I check for already-seen first, so that's not an issue.
> * I'm not wild about the inner _format_callback function in
get_deleted_objects(). admin_site, perms_needed, and levels_to_root are
all variables from a scope outside the function itself, which is a bit of
a code smell. I'd be much happier if _format_callback was a standalone
function in its own right that took these extra values as kwargs that were
passed in by nested (i.e., nested takes any extra kwargs and passes them
down the the callback)
One man's closure is another's code smell, I guess ;-) Just don't tell the
Lispers & Javascripters. I think the closure style is simpler and more
elegant here than adding **kwargs all over the place, but I'm not tied to
it; I'll use your suggestion instead.
> * The one test case that I can see that is obviously missing is
inheritance. On paper, this is probably caught by deleting OneToOneFields,
but there is enough special handling for inheritance that it's worth
having a specific test case for it (suggestion: SuperVillain subclassing
Villain, create a SuperVillain; what happens if you delete the villain? if
you delete the supervillain?)
I was trying to strike a balance of thoroughly testing the admin
functionality without redundantly testing Model._collect_sub_objects,
which is tested elsewhere; since my code has no special handling for
inheritance I thought this fell into the "redundant" category. But I guess
that same argument could apply to the multiple-fkey tests; I'll add
inheritance tests.
> * The template change makes me mildly nervous. Ideally, I'd prefer that
this change wasn't necessary, as it will break any existing templates in
the field that are constructed against the context that has historically
existed - even if that just means introducing a dummy top-level list entry
so the old template can iterate over a single element.
Ok. I'll use a dummy top-level list wrapper to keep the template context
backwards-compatible, but I'll at least fix the template misspelling of
"deletable," since that's internal to the short loop ;)
> * The comments on the add() method for NestedObjects makes a point of
highlighting that the model, pk and parent_model arguments aren't actually
used. I accept that they are completely redundant as they can be derived
from the obj and parent_obj arguments. However, I'm a little nervous
Was there more here that got cut off regarding the nature of your
nervousness? If I do merge CollectedObjects and NestedObjects, I would at
least be tempted to remove redundancy from the .add() API, but this would
require giving CollectedObjects knowledge about model internals
(specifically model.pk), which currently it avoids. Do you have a clear
preference between the simpler API (object, parent), which requires the
collection class to know it's collecting model instances, vs. the current
redundant API (obj_class, obj_pk, obj, parent_class, parent_pk), where the
redundancy is needed because NestedObjects ultimately needs access to the
full instance?
> * Is there any reason that NestedObjects can't be merged into
CollectedObjects? It seems to me like there is a lot of duplication
between the two classes, and we would be better served by extending
CollectedObjects to provide the nested() functionality rather than have a
second collection class.
These last two issues are a result of me trying to touch django.db.* as
little as possible. I certainly thought of merging CollectedObjects and
NestedObjects; if you like the idea, I'll give it a shot. I'm a little
concerned that the internals look more similar than they actually are, and
the combined code may be more complex and harder to read than the
separated versions; but I'll see how it works out.
--
Ticket URL: <http://code.djangoproject.com/ticket/6191#comment:24>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To post to this group, send email to django-upda...@googlegroups.com.
To unsubscribe from this group, send email to
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-updates?hl=en.
