App registration: Clean up CMSAppConfig class

django-cms · czpython · Jun 26, 2018 · Jun 14, 2018 · Jun 14, 2018 · Jun 15, 2018
commit 5968fc99eb65d10cd73b57210fec8f9caf086d49
diff --git a/cms/app_base.py b/cms/app_base.py
@@ -93,3 +93,10 @@ def get_urls(self, page=None, language=None, **kwargs):
         :return: list of urlconfs strings
         """
         return self._urls
+
+
+class CMSAppConfig():
+    """Base class that all cms app configurations should inherit from"""
+
+    def configure_app(self, app):
+        raise NotImplementedError()
diff --git a/cms/app_registration.py b/cms/app_registration.py
@@ -1,10 +1,13 @@
+# -*- coding: utf-8 -*-
 import inspect
 from importlib import import_module
 
 from django.apps import apps
 from django.utils.module_loading import module_has_submodule
 from django.core.exceptions import ImproperlyConfigured
 
+from cms.app_base import CMSAppConfig
+
 
 CMS_CONFIG_NAME = 'cms_config'
 
@@ -49,28 +52,19 @@ def autodiscover_cms_configs():
                     "class which inherits from CMSAppConfig")
 
 
-def get_cms_apps_with_features():
+def get_cms_apps():
     """
-    Returns app configs of apps with cms features
+    Returns django app configs of apps with a cms config
     """
-    apps_with_features = []
+    cms_apps = []
 
     for app_config in apps.get_app_configs():
         # The cms_app attr is added by the autodiscover_cms_configs
         # function if a cms_config.py file with a suitable class is found.
-        is_cms_app = hasattr(app_config, 'cms_app')
-        if is_cms_app:
-            # The configure_app method is only present on the cms
-            # app class if the app provides a cms feature. For classes
-            # that only have config this method will not be present.
-            has_cms_extension = hasattr(
-                app_config.cms_app, 'configure_app')
-        else:
-            has_cms_extension = False
-        if is_cms_app and has_cms_extension:
-            apps_with_features.append(app_config)
+        if hasattr(app_config, 'cms_app'):
+            cms_apps.append(app_config)
 
-    return apps_with_features
+    return cms_apps
 
 
 def configure_cms_apps(apps_with_features):
@@ -79,8 +73,7 @@ def configure_cms_apps(apps_with_features):
     and run code to register them with their config
     """
     for app_with_feature in apps_with_features:
-        enabled_property = "{app_label}_enabled".format(
-            app_label=app_with_feature.label)
+        enabled_property = "{}_enabled".format(app_with_feature.label)
         configure_app = app_with_feature.cms_app.configure_app
 
         for app_config in apps.get_app_configs():
@@ -90,7 +83,3 @@ def configure_cms_apps(apps_with_features):
             if getattr(app_config.cms_app, enabled_property, False):
                 # Feature enabled for this app so configure
                 configure_app(app_config)
-
-
-class CMSAppConfig():
-    pass
diff --git a/cms/tests/test_app_registration.py b/cms/tests/test_app_registration.py
@@ -56,34 +56,30 @@ def test_raises_exception_when_more_than_one_cms_app_class_found_in_cms_file(sel
             app_registration.autodiscover_cms_configs()
 
 
-class GetCmsAppsWithFeaturesTestCase(CMSTestCase):
+class GetCmsAppsTestCase(CMSTestCase):
 
     @patch.object(apps, 'get_app_configs')
-    def test_returns_only_apps_with_features(self, mocked_apps):
+    def test_returns_only_cms_apps(self, mocked_apps):
         # apps with cms_app attr and a configure method
-        app_with_cms_ext1 = Mock(cms_app=Mock(spec=['configure_app']))
-        app_with_cms_ext2 = Mock(cms_app=Mock(spec=['configure_app']))
-        # app with cms_app attr without a configure method
-        # this throws an AttributeError if configure_app is accessed
-        app_with_cms_config = Mock(cms_app=Mock(spec=[]))
+        cms_app1 = Mock(cms_app=Mock(spec=['configure_app']))
+        cms_app2 = Mock(cms_app=Mock(spec=['configure_app']))
         # app without cms_app attr
         # this throws an AttributeError if cms_app is accessed
         non_cms_app = Mock(spec=[])
         # mock what apps have been installed
         mocked_apps.return_value = [
-            app_with_cms_ext1,
-            app_with_cms_config,
-            app_with_cms_ext2,
+            cms_app1,
+            cms_app2,
             non_cms_app,
         ]
 
-        apps_with_features = app_registration.get_cms_apps_with_features()
+        cms_apps = app_registration.get_cms_apps()
 
-        # Of the 4 installed apps only 2 have features (1 is a
-        # non-cms app and 1 is a cms app without a feature)
+        # Of the 3 installed apps only 2 have features (1 is a
+        # non-cms app)
         self.assertListEqual(
-            apps_with_features,
-            [app_with_cms_ext1, app_with_cms_ext2])
+            cms_apps,
+            [cms_app1, cms_app2])
 
 
 class ConfigureCmsAppsTestCase(CMSTestCase):
@@ -255,3 +251,11 @@ def test_cms_apps_setup_after_setup_function_run(self):
         # whole app registration code to run through.
         self.assertEqual(feature_app.cms_app.num_configured_apps, 1)
         self.assertTrue(config_app.cms_app.configured)
+
+    @override_settings(INSTALLED_APPS=[
+        'cms.tests.test_app_registry.app_with_cms_config',
+        'cms.tests.test_app_registry.app_using_non_feature'
+    ])
+    def test_raises_not_implemented_exception_when_feature_app_doesnt_implement_configure_method(self):
+        with self.assertRaises(NotImplementedError):
+            setup.setup_cms_apps()
diff --git a/cms/tests/test_app_registry/app_using_non_feature/__init__.py b/cms/tests/test_app_registry/app_using_non_feature/__init__.py
@@ -0,0 +1 @@
+default_app_config = 'cms.tests.test_app_registry.app_using_non_feature.apps.NonFeatureCMSConfig'
diff --git a/cms/tests/test_app_registry/app_using_non_feature/admin.py b/cms/tests/test_app_registry/app_using_non_feature/admin.py
diff --git a/cms/tests/test_app_registry/app_using_non_feature/apps.py b/cms/tests/test_app_registry/app_using_non_feature/apps.py
@@ -0,0 +1,6 @@
+from django.apps import AppConfig
+
+
+class NonFeatureCMSConfig(AppConfig):
+    name = 'cms.tests.test_app_registry.app_using_non_feature'
+    label = 'app_using_non_feature'
diff --git a/cms/tests/test_app_registry/app_using_non_feature/cms_config.py b/cms/tests/test_app_registry/app_using_non_feature/cms_config.py
@@ -0,0 +1,7 @@
+from cms.app_base import CMSAppConfig
+
+
+class NonFeatureCMSConfig(CMSAppConfig):
+    # Attempting to use features from a cms app that doesnt define any
+    # features
+    app_with_cms_config_enabled = True
diff --git a/cms/tests/test_app_registry/app_using_non_feature/models.py b/cms/tests/test_app_registry/app_using_non_feature/models.py
diff --git a/cms/tests/test_app_registry/app_using_non_feature/views.py b/cms/tests/test_app_registry/app_using_non_feature/views.py
diff --git a/cms/tests/test_app_registry/app_with_cms_config/cms_config.py b/cms/tests/test_app_registry/app_with_cms_config/cms_config.py
@@ -1,4 +1,4 @@
-from cms.app_registration import CMSAppConfig
+from cms.app_base import CMSAppConfig
 
 
 class CMSConfigConfig(CMSAppConfig):

diff --git a/cms/tests/test_app_registry/app_with_cms_feature/cms_config.py b/cms/tests/test_app_registry/app_with_cms_feature/cms_config.py
@@ -1,4 +1,4 @@
-from cms.app_registration import CMSAppConfig
+from cms.app_base import CMSAppConfig
 
 
 def some_function(*args, **kwargs):

diff --git a/cms/tests/test_app_registry/app_with_two_cms_app_classes/cms_config.py b/cms/tests/test_app_registry/app_with_two_cms_app_classes/cms_config.py
@@ -1,4 +1,4 @@
-from cms.app_registration import CMSAppConfig
+from cms.app_base import CMSAppConfig
 
 
 class CMSFeatureConfig(CMSAppConfig):

diff --git a/cms/utils/setup.py b/cms/utils/setup.py
@@ -3,7 +3,7 @@
 
 from cms.utils.compat.dj import is_installed as app_is_installed
 from cms.app_registration import (
-    autodiscover_cms_configs, get_cms_apps_with_features,
+    autodiscover_cms_configs, get_cms_apps,
     configure_cms_apps)
 
 
@@ -50,5 +50,5 @@ def setup_cms_apps():
     any of this functionality.
     """
     autodiscover_cms_configs()
-    apps_with_cms_features = get_cms_apps_with_features()
-    configure_cms_apps(apps_with_cms_features)
+    cms_apps = get_cms_apps()
+    configure_cms_apps(cms_apps)
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		default_app_config = 'cms.tests.test_app_registry.app_using_non_feature.apps.NonFeatureCMSConfig'