Repository: django-cms/django-cms
Pull request: Fixed #3734 -- Automated check that PlaceholderFields use Placeholder…
Fixed #3734 -- Automated check that PlaceholderFields use PlaceholderAdminMixin

### Summary
ModelAdmin instances that are using PlaceholderField fields should be also a subclass of PlaceholderAdminMixin


Problem statement:
Automated check that PlaceholderFields use PlaceholderAdminMixin
Haven't looked into the feasibility of this but would like for the checks framework to detect all models with PlaceholderField, if any of those models are also registered in the admin that the admin class inherits from PlaceholderAdminMixin. It's an error to ever do otherwise, so would like this to be loud instead of silent.

Hints from issue discussion:
That would be a nice addition.
It's definitely feasible by getting all the models with the Django API, checking if any field is a PlaceholderField and then getting the related model from the admin registry and checking the mro() method result

@jsma would you be able to open a pull request that addresses this issue?

Commit message: Fixed #3734 -- Automated check that PlaceholderFields use PlaceholderAdminMixin

Patch to review:
diff --git a/cms/tests/test_check.py b/cms/tests/test_check.py
index 4ef57b530c6..18cf890362c 100644
--- a/cms/tests/test_check.py
+++ b/cms/tests/test_check.py
@@ -4,6 +4,7 @@
 from django.conf import settings
 from django.core.exceptions import ImproperlyConfigured
 from django.test import TestCase
+from django.test.utils import isolate_apps
 
 from cms.api import add_plugin
 from cms.models.pluginmodel import CMSPlugin
@@ -118,6 +119,19 @@ def test_non_numeric_site_id(self):
         with self.settings(SITE_ID='broken'):
             self.assertCheck(False, warnings=0, errors=1)
 
+    @isolate_apps("test_app")
+    def test_placeholder_mixin(self):
+        from django.db import models
+        from django.contrib import admin
+        from cms.models.fields import PlaceholderField
+
+        class ModelTest(models.Model):
+            field_a = PlaceholderField(slotname="test")
+
+        admin.site.register(ModelTest)
+        self.assertCheck(False, warnings=0, errors=1)
+        admin.site.unregister(ModelTest)
+
 
 class CheckWithDatabaseTests(CheckAssertMixin, TestCase):
 
diff --git a/cms/utils/check.py b/cms/utils/check.py
index 24b2fb3cfea..b1c9b3e3e3d 100644
--- a/cms/utils/check.py
+++ b/cms/utils/check.py
@@ -343,6 +343,35 @@ def get_class(method_name, model):
                                    'https://django-cms.readthedocs.io/en/latest/extending_cms/extending_page_title.html#handling-relations.')  # noqa
 
 
+@define_check
+def check_placeholder_mixin(output):
+    """
+    ModelAdmin instances that are using PlaceholderField fields
+    should be also a subclass of PlaceholderAdminMixin
+    """
+    from django.contrib.admin import site
+    from cms.models.fields import PlaceholderField
+    from cms.admin.placeholderadmin import PlaceholderAdminMixin
+
+    with output.section("PlaceHolderAdminMixin") as section:
+        for model, model_admin in site._registry.items():
+            ph_fields = [field for field in model._meta.get_fields() if isinstance(field, PlaceholderField)]
+            if len(ph_fields) == 0:
+                continue
+
+            if isinstance(model_admin, PlaceholderAdminMixin):
+                section.success("%s is subclass of PlaceholderAdminMixin" % model_admin)
+            else:
+                section.error(output.colorize(
+                    "%s is subclass of PlaceholderAdminMixin" % model_admin,
+                    opts=['bold'],
+                    fg='red'
+                ))
+
+        if section.successful:
+            section.finish_success("PlaceHolderAdminMixin configuration okay")
+
+
 def check(output):
     """
     Checks the configuration/environment of this django CMS installation.


Code review comments:
diff_hunk: @@ -343,6 +343,35 @@ def get_class(method_name, model):
                                    'https://django-cms.readthedocs.io/en/latest/extending_cms/extending_page_title.html#handling-relations.')  # noqa
 
 
+@define_check
+def check_placeholder_mixin(output):
+    """
+    ModelAdmin instances that are using PlaceholderField fields
+    should be also a subclass of PlaceholderAdminMixin
+    """
+    from django.contrib.admin import site
+    from cms.models.fields import PlaceholderField
+    from cms.admin.placeholderadmin import PlaceholderAdminMixin
+
+    with output.section("PlaceHolderAdminMixin") as section:
path: cms/utils/check.py
text: @user1:
Change `PlaceHolderAdminMixin` to `PlaceholderField`

diff_hunk: @@ -118,6 +119,19 @@ def test_non_numeric_site_id(self):
         with self.settings(SITE_ID='broken'):
             self.assertCheck(False, warnings=0, errors=1)
 
+    @isolate_apps("test_app")
+    def test_placeholder_mixin(self):
path: cms/tests/test_check.py
text: @user1:
Rename to `test_placeholder_field`

diff_hunk: @@ -343,6 +343,35 @@ def get_class(method_name, model):
                                    'https://django-cms.readthedocs.io/en/latest/extending_cms/extending_page_title.html#handling-relations.')  # noqa
 
 
+@define_check
+def check_placeholder_mixin(output):
+    """
+    ModelAdmin instances that are using PlaceholderField fields
+    should be also a subclass of PlaceholderAdminMixin
+    """
+    from django.contrib.admin import site
+    from cms.models.fields import PlaceholderField
+    from cms.admin.placeholderadmin import PlaceholderAdminMixin
+
+    with output.section("PlaceHolderAdminMixin") as section:
+        for model, model_admin in site._registry.items():
+            ph_fields = [field for field in model._meta.get_fields() if isinstance(field, PlaceholderField)]
+            if len(ph_fields) == 0:
+                continue
+
+            if isinstance(model_admin, PlaceholderAdminMixin):
+                section.success("%s is subclass of PlaceholderAdminMixin" % model_admin)
+            else:
+                section.error(output.colorize(
path: cms/utils/check.py
text: @user1:
`section.error` already does colorization, can remove the call to `output.colorize`

diff_hunk: @@ -343,6 +343,35 @@ def get_class(method_name, model):
                                    'https://django-cms.readthedocs.io/en/latest/extending_cms/extending_page_title.html#handling-relations.')  # noqa
 
 
+@define_check
+def check_placeholder_mixin(output):
+    """
+    ModelAdmin instances that are using PlaceholderField fields
+    should be also a subclass of PlaceholderAdminMixin
+    """
+    from django.contrib.admin import site
+    from cms.models.fields import PlaceholderField
+    from cms.admin.placeholderadmin import PlaceholderAdminMixin
+
+    with output.section("PlaceHolderAdminMixin") as section:
+        for model, model_admin in site._registry.items():
+            ph_fields = [field for field in model._meta.get_fields() if isinstance(field, PlaceholderField)]
+            if len(ph_fields) == 0:
+                continue
+
+            if isinstance(model_admin, PlaceholderAdminMixin):
+                section.success("%s is subclass of PlaceholderAdminMixin" % model_admin)
+            else:
+                section.error(output.colorize(
+                    "%s is subclass of PlaceholderAdminMixin" % model_admin,
path: cms/utils/check.py
text: @user1:
```%s does not subclass PlaceholderAdminMixin```

diff_hunk: @@ -343,6 +343,35 @@ def get_class(method_name, model):
                                    'https://django-cms.readthedocs.io/en/latest/extending_cms/extending_page_title.html#handling-relations.')  # noqa
 
 
+@define_check
+def check_placeholder_mixin(output):
+    """
+    ModelAdmin instances that are using PlaceholderField fields
+    should be also a subclass of PlaceholderAdminMixin
+    """
+    from django.contrib.admin import site
+    from cms.models.fields import PlaceholderField
+    from cms.admin.placeholderadmin import PlaceholderAdminMixin
+
+    with output.section("PlaceHolderAdminMixin") as section:
+        for model, model_admin in site._registry.items():
+            ph_fields = [field for field in model._meta.get_fields() if isinstance(field, PlaceholderField)]
+            if len(ph_fields) == 0:
+                continue
+
+            if isinstance(model_admin, PlaceholderAdminMixin):
path: cms/utils/check.py
text: @user1:
I think is ok to remove this from output and instead only log if there's error.
so would result in:

```python
if not isinstance(model_admin, PlaceholderAdminMixin):
    ...
```

diff_hunk: @@ -343,6 +343,35 @@ def get_class(method_name, model):
                                    'https://django-cms.readthedocs.io/en/latest/extending_cms/extending_page_title.html#handling-relations.')  # noqa
 
 
+@define_check
+def check_placeholder_mixin(output):
path: cms/utils/check.py
text: @user1:
Change to `check_placeholder_fields`

diff_hunk: @@ -343,6 +343,35 @@ def get_class(method_name, model):
                                    'https://django-cms.readthedocs.io/en/latest/extending_cms/extending_page_title.html#handling-relations.')  # noqa
 
 
+@define_check
+def check_placeholder_mixin(output):
+    """
+    ModelAdmin instances that are using PlaceholderField fields
+    should be also a subclass of PlaceholderAdminMixin
+    """
+    from django.contrib.admin import site
+    from cms.models.fields import PlaceholderField
+    from cms.admin.placeholderadmin import PlaceholderAdminMixin
+
+    with output.section("PlaceHolderAdminMixin") as section:
+        for model, model_admin in site._registry.items():
+            ph_fields = [field for field in model._meta.get_fields() if isinstance(field, PlaceholderField)]
+            if len(ph_fields) == 0:
+                continue
+
+            if isinstance(model_admin, PlaceholderAdminMixin):
+                section.success("%s is subclass of PlaceholderAdminMixin" % model_admin)
+            else:
+                section.error(output.colorize(
+                    "%s is subclass of PlaceholderAdminMixin" % model_admin,
+                    opts=['bold'],
+                    fg='red'
+                ))
+
+        if section.successful:
+            section.finish_success("PlaceHolderAdminMixin configuration okay")
path: cms/utils/check.py
text: @user1:
Change `PlaceHolderAdminMixin`  to `PlaceholderField`