[Django] #29917: admin.E130 (__name__ uniqueness) regression
Django
-----------------------------------------------+------------------------
Reporter: Matthias Kestenholz | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------------+------------------------
**Short version:** The introduction of admin.E130 (<class
'cabinet.admin.FileAdmin'>: (admin.E130) __name__ attributes of actions
defined in <class 'cabinet.admin.FileAdmin'> must be unique.) makes it
impossible to define `actions` on a base model admin class.
**Longer explanation:**
I have an admin base class which defines an `actions` attribute:
https://github.com/matthiask/django-
cabinet/blob/38d8d4333057d1bb848db0bad88304b05ca3df02/cabinet/base_admin.py#L372
The `actions` attribute isn't overridden in the model admin class which is
actually used:
https://github.com/matthiask/django-
cabinet/blob/38d8d4333057d1bb848db0bad88304b05ca3df02/cabinet/admin.py#L11
So when collecting `actions` from the MRO in ModelAdmin._get_base_actions
https://github.com/django/django/blob/5a2dd5ec5330938e9afb77faf6ca89abf39c018c/django/contrib/admin/options.py#L854
the same `move_to_folder` string is added twice which leads to the
following failure:
https://travis-ci.org/matthiask/django-cabinet/jobs/450169887
The actions dropdown in the administration interface only shows the
`move_to_folder` action once (only tested with Django 2.1 and lower), so
I'd assume that the uniqueness of actions is enforced later somewhere
somehow and the check should probably be reworked to either only check the
final result or to take the MRO into account.
--
Ticket URL: <https://code.djangoproject.com/ticket/29917>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Matthias Kestenholz | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
OK, I need to have a little think about this one. #29711 was reasonable
enough.
Initial thought it that you can silence `admin.E130`, so it’s not that
your use-case is made impossible.
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:1>
Django
Reporter: Matthias Kestenholz | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthias Kestenholz):
Of course I can workaround the issue (a even better workaround in this
case would be to move the `actions = ["move_to_folder"]` line from
`FileAdminBase` to `FileAdmin`).
But if defining `actions` on base classes does not work why would
`_get_base_actions` even traverse the MRO?
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:2>
Django
Reporter: Matthias Kestenholz | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Yes. But it seems to me the problem is the same action getting collected
twice, not the warning about the name...
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:3>
Django
Reporter: Matthias Kestenholz | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthias Kestenholz):
Yes. That was what I was trying to write, sorry for being unclear!
Maybe it would be best to deprecate and remove the `_get_base_actions`
method respectively its collection of `actions` from the MRO? The behavior
is there for a long time already, but it does not seem to be documented,
and it is different in a surprising way from all (almost all?) other
`ModelAdmin` attributes and/or callables. The proposed behavior would be
easier to explain, easier to implement and test and the system check
(which I agree with, at least with its intention!) wouldn't have to be
changed at all.
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:4>
Django
Reporter: Matthias Kestenholz | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
`_get_base_actions()` was added for #29419 (to allow permissions on
actions, as part of v2.1 adding the "view" permission for the admin).
The
[https://github.com/django/django/blob/5a2dd5ec5330938e9afb77faf6ca89abf39c018c/django/contrib/admin/options.py#L863-L867
underlying block] here is long-standing:
{{{
# Then gather them from the model admin and all parent classes,
# starting with self and working back up.
for klass in self.__class__.mro()[::-1]:
class_actions = getattr(klass, 'actions', []) or []
actions.extend(self.get_action(action) for action in
class_actions)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:5>
Django
Reporter: Matthias Kestenholz | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthias Kestenholz):
It's quite interesting. Django does not have any tests verifying the MRO
behavior. The complete test suite still passes after applying the
following patch:
{{{
diff --git a/django/contrib/admin/options.py
b/django/contrib/admin/options.py
index 241d22e82a..07521e3df3 100644
--- a/django/contrib/admin/options.py
+++ b/django/contrib/admin/options.py
@@ -862,9 +862,11 @@ class ModelAdmin(BaseModelAdmin):
- # Then gather them from the model admin and all parent classes,
- # starting with self and working back up.
- for klass in self.__class__.mro()[::-1]:
- class_actions = getattr(klass, 'actions', []) or []
- actions.extend(self.get_action(action) for action in
class_actions)
+ actions.extend(self.get_action(action) for action in
(getattr(self, 'actions', None) or []))
# get_action might have returned None, so filter any of those
out.
return filter(None, actions)
}}}
People might rely on it though, so silently introducing this change is out
of the question. I propose the following changes to Django:
- Apply the patch above.
- Add a new system check which warns users when the `actions` attribute of
registered model admin classes does not contain a superset of the
`actions` attribute of all it's bases (recursively). This cannot be made
an error because actions might be excluded on purpose.
- I'm not sure whether documentation changes are necessary -- this
proposal does not change documented behavior (AFAIK)
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:6>
Django
Reporter: Matthias Kestenholz | Owner: nobody
Component: contrib.admin | Version: master
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => needsinfo
Comment:
Hmmm. I'm not at all sure why we'd apply your patch.
I'm not seeing the issue here.
I don't see a failure adding this test to
`tests/modeladmin/test_actions.py`:
{{{
def test_actions_are_correctly_inherited(self):
class MockRequest:
pass
class AdminWithoutActions(admin.ModelAdmin):
# No actions attribute defined
pass
ma = AdminWithoutActions(Band, admin.AdminSite())
mock_request = MockRequest()
mock_request.GET = {}
mock_request.user = self.superuser
action_names = [a[1] for a in ma._get_base_actions()]
self.assertEqual(action_names, ['delete_selected'])
}}}
If it were picking up the action from the subclass as well as the
superclass we'd expect `delete_selected` to appear twice. This isn't
happening. Thus there must be more going on in your example.
If you can add a test case of this form that reproduces the issue, I'm
happy to re-open and accept the issue if Django is at fault.
(I'm not yet convinced this is really a regression from #29711: it seems
at best it'll have highlighted a latent bug, and since the warning can be
silenced I'm not sure it'd justify blocking a release.)
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:7>
Django
Reporter: Matthias Kestenholz | Owner: nobody
Component: contrib.admin | Version: master
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthias Kestenholz):
Here's a failing test:
{{{
diff --git a/tests/modeladmin/test_actions.py
b/tests/modeladmin/test_actions.py
index 17b3c324d2..41687329c4 100644
--- a/tests/modeladmin/test_actions.py
+++ b/tests/modeladmin/test_actions.py
@@ -55,3 +55,23 @@ class AdminActionsTests(TestCase):
mock_request.user = user
actions = ma.get_actions(mock_request)
self.assertEqual(list(actions.keys()), expected)
+
+ def test_actions_mro(self):
+ class MockRequest:
+ pass
+
+ class AdminBase(admin.ModelAdmin):
+ actions = ['custom_action']
+
+ def custom_action(modeladmin, request, queryset):
+ pass
+
+ class BandAdmin(AdminBase):
+ pass
+
+ ma = BandAdmin(Band, admin.AdminSite())
+ mock_request = MockRequest()
+ mock_request.GET = {}
+ mock_request.user = self.superuser
+ action_names = [a[1] for a in ma._get_base_actions()]
+ self.assertEqual(action_names, ['delete_selected',
'custom_action'])
}}}
Here's the failure:
{{{
FAIL: test_actions_mro (modeladmin.test_actions.AdminActionsTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/matthias/Projects/django/tests/modeladmin/test_actions.py",
line 77, in test_actions_mro
self.assertEqual(action_names, ['delete_selected', 'custom_action'])
AssertionError: Lists differ: ['delete_selected', 'custom_action',
'custom_action'] != ['delete_selected', 'custom_action']
}}}
See how the `custom_action` is added twice, once by `AdminBase.actions`
directly, and once by its inheritance in `BandAdmin`
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:8>
Django
admin.E130 __name__ uniqueness error)
-------------------------------------+-------------------------------------
Reporter: Matthias Kestenholz | Owner: nobody
Component: contrib.admin | Version: master
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthias Kestenholz):
I'm sorry for the bad ticket title -- it was misleading. I agree that the
E130 check didn't introduce the problem, it just made a problem visible
which already existed before.
I hope that we can move forward with this, because to me it's quite clear
that the MRO traversal to collect actions has unintended consequences.
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:9>
Django
-------------------------------------+------------------------------------
Reporter: Matthias Kestenholz | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: closed => new
* resolution: needsinfo =>
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:10>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Yep. Perfect. Thanks Matthias.
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:11>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
So, following the discussion of the breaking change in the PRs, digging
into the history, it seems the MRO based collection of actions has been in
place for the entire history of the feature..
It was introduced in #10505 as part of the original feature for v1.1. (The
[https://github.com/django/django/commit/44f3080226#diff-
3c42de3e53aba87b32c494f995a728dfR423 line in the diff].)
`get_actions()` was
[https://code.djangoproject.com/attachment/ticket/10505/admin-
actions.9.diff introduced explicitly] to allow collection of actions from
superclasses.
Given the contributors to #10505, and that it was a conscious decision on
their part, I'm don't feel we can just change the behaviour as part of a
bug fix.
Maybe there's a case for a change, but that should be made separately.
(Currently the behaviour is not tested, but with
[https://github.com/django/django/pull/10607 PR10607] it would be, and not
documented, but a long-overdue paragraph could be added.)
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:12>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthias Kestenholz):
Well, I'm not of the opinion that the certainly very impressive group of
people involved in the original work makes it impossible to judge the
feature on its own merits.
Tim also wrote in
https://github.com/django/django/pull/10603#issuecomment-435624170 that
following Python's usual inheritance is usually better than this type of
"magic".
But my vote certainly counts less here since I'm "just" a contributor and
not a maintainer. So I still don't agree with keeping this behavior
around, but as I already wrote on your pull request, it's your call.
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:13>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
> ...makes it impossible to judge the feature on its own merits.
That’s not what I’m saying. What I’m saying is that we can’t just change a
design decision in a bug fix. Not when there’s a one line change that just
fixes it.
We can change the feature if decided, but that would be done separately:
the policy is that to change a design there needs to be a discussion and
agreement on django-developers and so on.
The impressive list of people is relevant because they obviously discussed
this at length and spent a lot of time putting the feature together. It’s
been unchanged for 10 years. They must have got something right.
I’m not sure that I’m at all confident that we’d make a better decision
now, having given it not much thought or time or effort at all.
(I appreciate our time/thought/effort is not zero.)
> ...my vote certainly counts less...
No, your vote counts too. You’ll note I didn’t close your PR, or merge
mine. I just dug up the history. I don’t mean to upset you. I’m just
trying to get it right.
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:14>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
I [https://groups.google.com/d/topic/django-
developers/-OWoYL_zryM/discussion made the proposal] to remove action
collection from superclasses on django-developers.
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:15>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthias Kestenholz):
Thanks, to both of you.
Carlton, I misunderstood the process you were proposing (regarding bug
fixes vs. change of behavior) and got annoyed without a good reason. Sorry
for that.
I see that nobody disagrees on django-developers. Yay!
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:16>
Django
Reporter: Matthias Kestenholz | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
Comment:
I'll give it a more days for negative mailing list feedback, otherwise I
think the [https://github.com/django/django/pull/10603 PR] is good to go.
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:17>
Django
-------------------------------------+-------------------------------------
Component: contrib.admin | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"f9ff1df1daac8ae1fc22b27f48735148cb5488dd" f9ff1df]:
{{{
#!CommitTicketReference repository=""
revision="f9ff1df1daac8ae1fc22b27f48735148cb5488dd"
Fixed #29917 -- Stopped collecting ModelAdmin.actions from base
ModelAdmins.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29917#comment:18>