Add panel configuration checks (#5093)

* checks for Page InlinePanel related model panels

* add panels check to modelAdmin models

* add and revise tests for panel model checks

* revise related model usage

* remove unused keys from format string

* fix up unique error collection

* rework check_panels_in_model - variable names and string formatting

* rework tests for check_panels_in_model to use new string formatting
plus linting of some unused imports

* add checks to snippet models

* use consistent naming for returning errors from checks in modeladmin

* add tests for snippets check_panels_in_model checks

* ignore vscode config files

* remove additional line added

.gitignore

@@ -23,3 +23,6 @@ npm-debug.log*
 *.iws
 coverage/
 client/node_modules
+
+### vscode
+.vscode

wagtail/admin/checks.py

@@ -70,3 +70,83 @@ def get_form_class_check(app_configs, **kwargs):
                 id='wagtailadmin.E002'))
 
     return errors
+
+
+@register('panels')
+def inline_panel_model_panels_check(app_configs, **kwargs):
+    from wagtail.core.models import get_page_models
+
+    errors = []
+    page_models = get_page_models()
+
+    for cls in page_models:
+        errors.extend(check_panels_in_model(cls))
+
+    # filter out duplicate errors found for the same model
+    unique_errors = []
+    for error in errors:
+        if error.msg not in [e.msg for e in unique_errors]:
+            unique_errors.append(error)
+    return unique_errors
+
+
+def check_panels_in_model(cls, context='model'):
+    """Check panels configuration uses `panels` when `edit_handler` not in use."""
+    from wagtail.core.models import Page
+    from wagtail.admin.edit_handlers import InlinePanel
+
+    errors = []
+
+    if hasattr(cls, 'get_edit_handler'):
+        # must check the InlinePanel related models
+        edit_handler = cls.get_edit_handler()
+        for tab in edit_handler.children:
+            inline_panel_children = [
+                panel for panel in tab.children if isinstance(panel, InlinePanel)]
+            for inline_panel_child in inline_panel_children:
+                errors.extend(check_panels_in_model(
+                    inline_panel_child.db_field.related_model,
+                    context='InlinePanel model',
+                ))
+
+    if issubclass(cls, Page) or hasattr(cls, 'edit_handler'):
+        # Pages do not need to be checked for standalone tabbed_panel usage
+        # if edit_handler is used on any model, assume config is correct
+        return errors
+
+    tabbed_panels = [
+        'content_panels',
+        'promote_panels',
+        'settings_panels',
+    ]
+
+    for panel_name in tabbed_panels:
+        class_name = cls.__name__
+        if not hasattr(cls, panel_name):
+            continue
+
+        panel_name_short = panel_name.replace('_panels', '').title()
+        error_title = "{}.{} will have no effect on {} editing".format(
+            class_name, panel_name, context)
+
+        if 'InlinePanel' in context:
+            error_hint = """Ensure that {} uses `panels` instead of `{}`.
+There are no tabs on non-Page model editing within InlinePanels.""".format(
+                class_name, panel_name)
+        else:
+            error_hint = """Ensure that {} uses `panels` instead of `{}`\
+or set up an `edit_handler` if you want a tabbed editing interface.
+There are no default tabs on non-Page models so there will be no \
+{} tab for the {} to render in.""".format(
+                class_name, panel_name, panel_name_short, panel_name)
+
+        error = Warning(
+            error_title,
+            hint=error_hint,
+            obj=cls,
+            id='wagtailadmin.W002'
+        )
+
+        errors.append(error)
+
+    return errors

wagtail/admin/tests/test_edit_handlers.py

@@ -8,8 +8,9 @@ from django.core.exceptions import FieldDoesNotExist, ImproperlyConfigured
 from django.test import RequestFactory, TestCase, override_settings
 
 from wagtail.admin.edit_handlers import (
-    FieldPanel, FieldRowPanel, InlinePanel, ObjectList, PageChooserPanel, RichTextFieldPanel,
-    TabbedInterface, extract_panel_definitions_from_model_class, get_form_for_model)
+    FieldPanel, FieldRowPanel, InlinePanel, ObjectList, PageChooserPanel,
+    RichTextFieldPanel, TabbedInterface, extract_panel_definitions_from_model_class,
+    get_form_for_model)
 from wagtail.admin.forms import WagtailAdminModelForm, WagtailAdminPageForm
 from wagtail.admin.rich_text import DraftailRichTextArea
 from wagtail.admin.widgets import AdminAutoHeightTextInput, AdminDateInput, AdminPageChooser
@@ -17,7 +18,8 @@ from wagtail.core.models import Page, Site
 from wagtail.images.edit_handlers import ImageChooserPanel
 from wagtail.tests.testapp.forms import ValidatedPageForm
 from wagtail.tests.testapp.models import (
-    EventPage, EventPageChooserModel, EventPageSpeaker, PageChooserModel, SimplePage, ValidatedPage)
+    EventPage, EventPageChooserModel, EventPageSpeaker, PageChooserModel,
+    SimplePage, ValidatedPage)
 from wagtail.tests.utils import WagtailTestUtils
 
 
@@ -890,3 +892,84 @@ class TestInlinePanel(TestCase, WagtailTestUtils):
         with self.ignore_deprecation_warnings():
             self.assertRaises(TypeError, lambda: InlinePanel(label="Speakers"))
             self.assertRaises(TypeError, lambda: InlinePanel(EventPage, 'speakers', label="Speakers", bacon="chunky"))
+
+
+class TestInlinePanelRelatedModelPanelConfigChecks(TestCase):
+
+    def setUp(self):
+        self.original_panels = EventPageSpeaker.panels
+        delattr(EventPageSpeaker, 'panels')
+
+        def get_checks_result():
+            # run checks only with the 'panels' tag
+            checks_result = checks.run_checks(tags=['panels'])
+            return [warning for warning in checks_result if warning.obj == EventPageSpeaker]
+
+        self.warning_id = 'wagtailadmin.W002'
+        self.get_checks_result = get_checks_result
+
+    def tearDown(self):
+        EventPageSpeaker.panels = self.original_panels
+
+    def test_page_with_inline_model_with_tabbed_panel_only(self):
+        """Test that checks will warn against setting single tabbed panel on InlinePanel model"""
+
+        EventPageSpeaker.settings_panels = [FieldPanel('first_name'), FieldPanel('last_name')]
+
+        warning = checks.Warning(
+            "EventPageSpeaker.settings_panels will have no effect on InlinePanel model editing",
+            hint="""Ensure that EventPageSpeaker uses `panels` instead of `settings_panels`.
+There are no tabs on non-Page model editing within InlinePanels.""",
+            obj=EventPageSpeaker,
+            id=self.warning_id,
+        )
+
+        checks_results = self.get_checks_result()
+
+        self.assertIn(warning, checks_results)
+
+        delattr(EventPageSpeaker, 'settings_panels')
+
+    def test_page_with_inline_model_with_two_tabbed_panels(self):
+        """Test that checks will warn against multiple tabbed panels on InlinePanel models"""
+
+        EventPageSpeaker.content_panels = [FieldPanel('first_name')]
+        EventPageSpeaker.promote_panels = [FieldPanel('last_name')]
+
+        warning_1 = checks.Warning(
+            "EventPageSpeaker.content_panels will have no effect on InlinePanel model editing",
+            hint="""Ensure that EventPageSpeaker uses `panels` instead of `content_panels`.
+There are no tabs on non-Page model editing within InlinePanels.""",
+            obj=EventPageSpeaker,
+            id=self.warning_id,
+        )
+        warning_2 = checks.Warning(
+            "EventPageSpeaker.promote_panels will have no effect on InlinePanel model editing",
+            hint="""Ensure that EventPageSpeaker uses `panels` instead of `promote_panels`.
+There are no tabs on non-Page model editing within InlinePanels.""",
+            obj=EventPageSpeaker,
+            id=self.warning_id,
+        )
+
+        checks_results = self.get_checks_result()
+
+        self.assertIn(warning_1, checks_results)
+        self.assertIn(warning_2, checks_results)
+
+        delattr(EventPageSpeaker, 'content_panels')
+        delattr(EventPageSpeaker, 'promote_panels')
+
+    def test_page_with_inline_model_with_edit_handler(self):
+        """Checks should NOT warn if InlinePanel models use tabbed panels AND edit_handler"""
+
+        EventPageSpeaker.content_panels = [FieldPanel('first_name')]
+        EventPageSpeaker.edit_handler = TabbedInterface(
+            ObjectList([FieldPanel('last_name')], heading='test')
+        )
+
+        # should not be any errors
+        self.assertEqual(self.get_checks_result(), [])
+
+        # clean up for future checks
+        delattr(EventPageSpeaker, 'edit_handler')
+        delattr(EventPageSpeaker, 'content_panels')

wagtail/contrib/modeladmin/options.py

@@ -1,9 +1,11 @@
 from django.conf.urls import url
 from django.contrib.auth.models import Permission
+from django.core import checks
 from django.core.exceptions import ImproperlyConfigured
 from django.db.models import Model
 from django.utils.safestring import mark_safe
 
+from wagtail.admin.checks import check_panels_in_model
 from wagtail.core import hooks
 from wagtail.core.models import Page
 
@@ -497,6 +499,14 @@ class ModelAdmin(WagtailRegisterable):
             queryset = queryset.not_type(self.model)
         return queryset
 
+    def register_with_wagtail(self):
+        super().register_with_wagtail()
+
+        @checks.register('panels')
+        def modeladmin_model_check(app_configs, **kwargs):
+            errors = check_panels_in_model(self.model, 'modeladmin')
+            return errors
+
 
 class ModelAdminGroup(WagtailRegisterable):
     """
@@ -584,6 +594,16 @@ class ModelAdminGroup(WagtailRegisterable):
                 parent_page, queryset, request)
         return queryset
 
+    def register_with_wagtail(self):
+        super().register_with_wagtail()
+
+        @checks.register('panels')
+        def modeladmin_model_check(app_configs, **kwargs):
+            errors = []
+            for modeladmin_class in self.items:
+                errors.extend(check_panels_in_model(modeladmin_class.model))
+            return errors
+
 
 def modeladmin_register(modeladmin_class):
     """

wagtail/contrib/modeladmin/tests/test_simple_modeladmin.py

@@ -2,8 +2,10 @@ from unittest import mock
 
 from django.contrib.auth import get_user_model
 from django.contrib.auth.models import Group
+from django.core import checks
 from django.test import TestCase
 
+from wagtail.admin.edit_handlers import FieldPanel, TabbedInterface
 from wagtail.images.models import Image
 from wagtail.images.tests.utils import get_test_image_file
 from wagtail.tests.modeladmintest.models import Author, Book, Publisher, Token
@@ -515,3 +517,89 @@ class TestHeaderBreadcrumbs(TestCase, WagtailTestUtils):
         position_of_header_close = content_str.index('</header>')
         position_of_breadcrumbs = content_str.index('<ul class="breadcrumb">')
         self.assertGreater(position_of_header_close, position_of_breadcrumbs)
+
+
+class TestPanelConfigurationChecks(TestCase, WagtailTestUtils):
+
+    def setUp(self):
+        self.warning_id = 'wagtailadmin.W002'
+
+        def get_checks_result():
+            # run checks only with the 'panels' tag
+            checks_result = checks.run_checks(tags=['panels'])
+            return [
+                warning for warning in
+                checks_result if warning.id == self.warning_id]
+
+        self.get_checks_result = get_checks_result
+
+
+    def test_model_with_single_tabbed_panel_only(self):
+
+        Publisher.content_panels = [FieldPanel('name'), FieldPanel('headquartered_in')]
+
+        warning = checks.Warning(
+            "Publisher.content_panels will have no effect on modeladmin editing",
+            hint="""Ensure that Publisher uses `panels` instead of `content_panels`\
+or set up an `edit_handler` if you want a tabbed editing interface.
+There are no default tabs on non-Page models so there will be no\
+ Content tab for the content_panels to render in.""",
+            obj=Publisher,
+            id='wagtailadmin.W002',
+        )
+
+        checks_results = self.get_checks_result()
+
+        self.assertIn(warning, checks_results)
+
+        # clean up for future checks
+        delattr(Publisher, 'content_panels')
+
+
+    def test_model_with_two_tabbed_panels_only(self):
+
+        Publisher.settings_panels = [FieldPanel('name')]
+        Publisher.promote_panels = [FieldPanel('headquartered_in')]
+
+
+        warning_1 = checks.Warning(
+            "Publisher.promote_panels will have no effect on modeladmin editing",
+            hint="""Ensure that Publisher uses `panels` instead of `promote_panels`\
+or set up an `edit_handler` if you want a tabbed editing interface.
+There are no default tabs on non-Page models so there will be no\
+ Promote tab for the promote_panels to render in.""",
+            obj=Publisher,
+            id='wagtailadmin.W002',
+        )
+
+        warning_2 = checks.Warning(
+            "Publisher.settings_panels will have no effect on modeladmin editing",
+            hint="""Ensure that Publisher uses `panels` instead of `settings_panels`\
+or set up an `edit_handler` if you want a tabbed editing interface.
+There are no default tabs on non-Page models so there will be no\
+ Settings tab for the settings_panels to render in.""",
+            obj=Publisher,
+            id='wagtailadmin.W002',
+        )
+
+        checks_results = self.get_checks_result()
+
+        self.assertIn(warning_1, checks_results)
+        self.assertIn(warning_2, checks_results)
+
+        # clean up for future checks
+        delattr(Publisher, 'settings_panels')
+        delattr(Publisher, 'promote_panels')
+
+
+    def test_model_with_single_tabbed_panel_and_edit_handler(self):
+
+        Publisher.content_panels = [FieldPanel('name'), FieldPanel('headquartered_in')]
+        Publisher.edit_handler = TabbedInterface(Publisher.content_panels)
+
+        # no errors should occur
+        self.assertEqual(self.get_checks_result(), [])
+
+        # clean up for future checks
+        delattr(Publisher, 'content_panels')
+        delattr(Publisher, 'edit_handler')

wagtail/snippets/models.py

@@ -1,6 +1,8 @@
 from django.contrib.admin.utils import quote
+from django.core import checks
 from django.urls import reverse
 
+from wagtail.admin.checks import check_panels_in_model
 from wagtail.admin.utils import get_object_usage
 
 SNIPPET_MODELS = []
@@ -16,6 +18,12 @@ def register_snippet(model):
         model.usage_url = get_snippet_usage_url
         SNIPPET_MODELS.append(model)
         SNIPPET_MODELS.sort(key=lambda x: x._meta.verbose_name)
+
+        @checks.register('panels')
+        def modeladmin_model_check(app_configs, **kwargs):
+            errors = check_panels_in_model(model, 'snippets')
+            return errors
+
     return model
 
 

wagtail/snippets/tests.py

@@ -3,6 +3,7 @@ import json
 from django.contrib.admin.utils import quote
 from django.contrib.auth import get_user_model
 from django.contrib.auth.models import AnonymousUser, Permission
+from django.core import checks
 from django.core.exceptions import ValidationError
 from django.core.files.base import ContentFile
 from django.core.files.uploadedfile import SimpleUploadedFile
@@ -11,6 +12,7 @@ from django.test.utils import override_settings
 from django.urls import reverse
 from taggit.models import Tag
 
+from wagtail.admin.edit_handlers import FieldPanel
 from wagtail.admin.forms import WagtailAdminModelForm
 from wagtail.core.models import Page
 from wagtail.snippets.blocks import SnippetChooserBlock
@@ -1142,3 +1144,39 @@ class TestSnippetChosenWithCustomUUIDPrimaryKey(TestCase, WagtailTestUtils):
         response = self.get(pk=AdvertWithCustomUUIDPrimaryKey.objects.all()[0].pk)
         response_json = json.loads(response.content.decode())
         self.assertEqual(response_json['step'], 'chosen')
+
+
+class TestPanelConfigurationChecks(TestCase, WagtailTestUtils):
+
+    def setUp(self):
+        self.warning_id = 'wagtailadmin.W002'
+
+        def get_checks_result():
+            # run checks only with the 'panels' tag
+            checks_result = checks.run_checks(tags=['panels'])
+            return [
+                warning for warning in
+                checks_result if warning.id == self.warning_id]
+
+        self.get_checks_result = get_checks_result
+
+    def test_model_with_single_tabbed_panel_only(self):
+
+        StandardSnippet.content_panels = [FieldPanel('text')]
+
+        warning = checks.Warning(
+            "StandardSnippet.content_panels will have no effect on snippets editing",
+            hint="""Ensure that StandardSnippet uses `panels` instead of `content_panels`\
+or set up an `edit_handler` if you want a tabbed editing interface.
+There are no default tabs on non-Page models so there will be no\
+ Content tab for the content_panels to render in.""",
+            obj=StandardSnippet,
+            id='wagtailadmin.W002',
+        )
+
+        checks_results = self.get_checks_result()
+
+        self.assertEqual([warning], checks_results)
+
+        # clean up for future checks
+        delattr(StandardSnippet, 'content_panels')