#8060: Admin Inlines do not respect user permissions
-------------------------------------+-------------------------------------
Reporter: | Owner: sjaensch
p.patruno@… | Status: assigned
Type: Bug | Component: contrib.admin
Milestone: | Severity: Normal
Version: SVN | Keywords: inlines User
Resolution: | authentication
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 1 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by carljm):
* needs_better_patch: 0 => 1
Comment:
This is looking good! Thanks for all your work on it. A few comments:
1. The tests should be broken up into many test methods, one for each case
you're testing (here, I think each block of three related assertContains
or assertNotContains constitutes a "case"). Generally it's ok to have a
few closely-related asserts in a single test method, but as few as
possible. This'll result in some duplication of setup code; that's ok. If
the duplication is really bad, you can factor out some of the setup code
into a helper method on the testcase (that's better than putting it in
setUp() as you still explicitly call the helper method in each test that
needs it, so there's less implicit dependency between tests). In tests
it's better to duplicate a bit of setup code here and there than it is to
have super-long test methods where everything is intertwined and its hard
to change the conditions for one test without affecting others later on,
and a single failure early on can obscure later failures. (Django does
have some super-long test methods in its suite, mostly as a result of
conversion from doctests, but that doesn't mean we want more!). I haven't
reviewed in depth yet whether the test coverage is adequate, that'll be
easier to do once separate tests are broken out.
2. You can get an empty version of any queryset with the `.none()` method,
rather than using `pk__isnull=True`.
3. The other bit I'm not sure is right is the special handling for m2m
auto-created through models. In the m2m case, each inlined instance
represents a relationship, or an instance of the through model, not an
instance of the related model. There are no permissions for an auto-
created through model, but I think the most sensible thing is to consider
any change, addition, or deletion of a through-model to be a *change* to
the related model. In other words, it doesn't make sense to prevent
deleting a through-model instance because the user doesn't have delete
permissions on the related model; no related-model-instance is being
deleted. So in the auto-created case I think all three has_X_permission
methods should return has_change_permission on the related model. Also,
this reasoning should be encapsulated in a comment.
--
Ticket URL: <https://code.djangoproject.com/ticket/8060#comment:20>
Django <https://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-updates@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.
