Admin actions are duplicated when using model admins with inheritance

[Matthias Kestenholz]

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.

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

[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 ìsn't allowed why would _get_base_actions even traverse the MRO?

(Changed "does not work" to "isn't allowed")

[Carlton Gibson]

Yes. But it seems to me the problem is the same action getting collected twice, not the warning about the name...

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

[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 ​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)

[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)

[Carlton Gibson]

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

Update: this doesn't fail because delete_selected is defined on AdminSite, not ModelAdmin. Test below is true case.

[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 AdminBase(admin.ModelAdmin):
+            actions = ['custom_action']
+
+            def custom_action(modeladmin, request, queryset):
+                pass
+
+        class BandAdmin(AdminBase):
+            pass
+
+        ma = BandAdmin(Band, admin.AdminSite())
+        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']                                                                                                            

Observe how the custom_action is added twice, once by AdminBase.actions directly, and once by its inheritance in BandAdmin

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

[Carlton Gibson]

Yep. Perfect. Thanks Matthias.

[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 ​line in the diff.)

get_actions() was 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 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 ​PR10607 it would be, and not documented, but a long-overdue paragraph could be added.)

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

[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 use-case that we’re proposing to break is kind of useful: declare an action on a base class and not have to redeclare it later: it’ll just be there. That was one of the original considerations.)

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.

[Tim Graham]

I ​made the proposal to remove action collection from superclasses on django-developers.

[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!

[Tim Graham]

I'll give it a more days for negative mailing list feedback, otherwise I think the ​PR is good to go.

[Tim Graham <timograham@…>]

In f9ff1df:

Fixed #29917 -- Stopped collecting ModelAdmin.actions from base ModelAdmins.