Add a check for ModelAdmin.actions functions not taking three arguments

[Riccardo Magliocchetti]

Recently I've introduced a runtime exception because after a refactoring a function implementing an action got renamed but the old name stayed as an helper with 1 parameter. Setting in ModelAdmin.actions a function that does not take 3 arguments will raise an exception in BaseModelAdmin.response_action.
A check that will throw an error in that case could have saved me some embarassament in production :)

A rough untested implementation could look something like that:

diff --git a/django/contrib/admin/checks.py b/django/contrib/admin/checks.py
index 1665023434..9c4f7f125a 100644
--- a/django/contrib/admin/checks.py
+++ b/django/contrib/admin/checks.py
@@ -1,4 +1,5 @@
 import collections
+import inspect
 from itertools import chain
 
 from django.apps import apps
@@ -1244,6 +1245,31 @@ class ModelAdminChecks(BaseModelAdminChecks):
                 )
         return errors
 
+    def _check_actions_parameters(self, obj):
+        """Check that every action takes the correct number of parameters."""
+        errors = []
+        actions = obj._get_base_actions()
+        expected_func_parameters = 3
+        for func, name, _ in actions:
+            params = inspect.signature(func).parameters.values()
+            # TODO: does people define actions functions with more parameters with a default?
+            if len(params) != expected_func_parameters:
+                errors.append(
+                    checks.Error(
+                        "action %r used in %s has an invalid number of parameters. Expected %d got %d."
+                        % (
+                            name,
+                            obj.__class__.__name__,
+                            expected_func_parameters,
+                            len(params),
+                        ),
+                        obj=obj.__class__,
+                        id="admin.E131",
+                    )
+                )
+        return errors
+
+
 
 class InlineModelAdminChecks(BaseModelAdminChecks):
     def check(self, inline_obj, **kwargs):

[Natalia Bidart]

Hello Riccardo, thank you for your ticket and interest to make Django better.

I understand your report, but Django can not possibly add checks for every function or method signature that Django users create when using the framework. In my experience, these errors are better caught by unit tests, there are plenty of examples in the Django source code and in other open projects that would provide examples about how to test Admin actions.

Did you have unit tests that did not catch this? If so, perhaps you could seek further help in how to improve/fix those by using any of the user support channels from ​this link. I'll be closing this ticket as wontfix following the ​ticket triaging process.

Thanks again!

[Riccardo Magliocchetti]

Hello Natalia, we are already unit testing the admin actions but not through the testing client, i.e. we are testing the function and not posting data to the model admin view. I have to say I'm not really thrilled to start doing so for every action. I would have thought that the check would have been in the same spirit as the other checks on the fields though. Thanks.

[Natalia Bidart]

Replying to Riccardo Magliocchetti:

Hello Natalia, we are already unit testing the admin actions but not through the testing client, i.e. we are testing the function and not posting data to the model admin view. I have to say I'm not really thrilled to start doing so for every action. I thought that the check would have been in the same spirit as the other checks on the fields though. Thanks.

Hi Riccardo,

Regarding testing the actions exercising the whole request stack (ie using the test client, for example): I agree is not trivial, but I still think is necessary, if anything as a way of integration/functional testing.

Regarding whether the proposed check is in the same spirit of existing checks, I beg to differ. IMHO, existing checks are about ensuring *semantic* correctness of various parts and pieces of the framework as a tool ("does the max_length parameter makes sense for an IntegerField"), not so much about ensuring syntactic/static checking ("is this method being called with the right amount of params"). The former is a manageable sized-set, while the latter could grow tremendously making the checks non performant, hard to manage and very hard to maintain.

As documented in the ​System check framework, the checks are extensible, so your project could define as many custom checks as you consider necessary. Besides that, and following my professional experience, there are common practices like having a dev and/or staging environments where code is deployed before reaching production, allowing for these type of errors to be caught earlier.

Suffices to say that, ultimately, is up to you and your company what kind of tests and testing environments are used, but from the point of view of Django, the framework can not possible provide checks for every potentially incorrect method or function call.

[Riccardo Magliocchetti]

Hello Natalia, thanks for the answer, I'll implement something locally, no big deal. BTW I was referring to the ModelAdmin checks ​https://docs.djangoproject.com/en/4.2/ref/checks/#modeladmin where stuff is checked by its type.