Introduced app registration system #6421

monikasulik · 2018-06-21T12:18:12Z

Summary

This is the new app registration system. It allows django apps that extend the cms to register their functionality with the cms and introduces a way to set configuration for other apps that use this additional functionality.

Proposed changes in this pull request

To understand how to use the app registration system, lets use an example. Lets say our INSTALLED_APPS are defined like this:

INSTALLED_APPS = [
    ...
    'pink_cms_admin',
    'pony_cms_icons',
    'blog_posts',
]

The pink_cms_admin is an app that extends the cms by making apps, that are so configured, to have a pink admin. To do that, it would define a pink_cms_admin/cms_config.py file, which would look like this:

from cms.app_base import CMSAppExtension

from pink_cms_admin import make_admin_pink


class PinkAdminCMSExtension(CMSAppExtension):

    def configure_app(self, cms_config):
         # Do anything you need to do to each app that wants to be pink
         make_admin_pink(cms_config)

The blog_posts app wants to be pink and wants to have pony icons everywhere. So it would define blog_posts/cms_config.py like this:

from cms.app_base import CMSAppConfig


class BlogPostsCMSConfig(CMSAppConfig):
    # To enable functionality define an attribute like <app_label>_enabled and set it to True
    pink_cms_admin_enabled = True
    pony_cms_icons_enabled = True
    # pony_cms_icons also has additional settings. These are defined here.
    pony_cms_icons_pony_colours = ['purple', 'pink']
    pony_cms_icons_ponies_with_wings = True

The pony_cms_icons app lets other apps have pony icons everywhere, but also wants to have a pink admin. So it would define pony_cms_icons/cms_config.py like this:

from django.core.exceptions import ImproperlyConfigured

from cms.app_base import CMSAppConfig, CMSAppExtension

from pony_cms_icons import add_pony_icons


class PonyIconsCMSConfig(CMSAppConfig):
    pink_cms_admin_enabled = True


class PonyIconsCMSExtension(CMSAppExtension):

    def configure_app(self, cms_config):
         # Do anything you need to do to each app that wants to have pony icons here

         # As pony icons defines additional settings, you will also need to check
         # for any required settings here
         pony_colours = getattr(cms_config, 'pony_cms_icons_pony_colours', None)
         if not pony_colours:
             raise ImproperlyConfigured(
                 "Apps that use pony_cms_icons, must define pony_cms_icons_pony_colours")
         ponies_with_wings = getattr(cms_config, 'pony_cms_icons_ponies_with_wings', False)

         add_pony_icons(cms_config.django_app, pony_colours, ponies_with_wings)

The configure_app method, as is already apparent, takes one param - cms_config. cms_config is an instance of an app's CMSAppConfig class. In addition to that you can also access the django app object (as defined in the app's apps.py) by using cms_config.app_config. In this way you can access attributes that django provides (such as label, verbose_name etc.).
The configure_app method is run once for every django cms app that declares a feature as enabled.

Documentation checklist

App registration - configuring apps

App registration start up

coveralls · 2018-06-21T13:40:17Z

Coverage increased (+0.008%) to 78.189% when pulling 741a803 on monikasulik:app_registration into 98380b5 on divio:release/4.0.x.

…se cms_config.py filenames instead of cms_apps.py

App registration: improvements after Paulo's comments

monikasulik · 2018-06-22T13:05:09Z

@evildmp From what I understand you will be creating documentation for this. Please let me know if you need more than what I've put in the PR description to create this. Also, on the topic of docs there's a comment from Noel - it might be better to use a differently named app for the example so that nobody gets confused that these docs describe how to actually integrate with the actual versioning app.

monikasulik · 2018-06-22T18:01:18Z

@czpython I ran the description in this PR by Matus and Dami who are planning on designing the generic (i.e. configurable) part of moderation imminently and gave me detailed feedback with that very much in mind. There's some stuff I probably need to add to the description of this PR to make a few things clearer based on that discussion (@evildmp I'll try to do that on Monday), but also there are two suggestions for changing how this would work:

Instead of versioning_enabled = True, moderation_enabled = True etc. just have supported_apps = ['moderation', 'versioning'] in the config. Interestingly, even though Matus and Dami read through the docs separately and didn't consult with each other before speaking to me, they were both confused by the "enabled" config options and found the idea of having a supported apps config much clearer and more intuitive. Although I wasn't confused by the enabled options when I first saw them, I do agree that the supported_apps option would be a clearer interface.
Matus seemed to have the same confusion as I did when I first saw Paulo's prototype. To me initially it was very confusing that we didn't have two base classes - something like CMSAppConfig and CMSAppExtension. cms_config.py files could then define either one or two classes. Classes that inherit from CMSAppExtension would have to implement the configure_app method. Classes which inherit from CMSAppConfig would have to set supported_apps and any other app-specific settings. Dami was not as confused, but agreed that to him this would also be clearer.

IMO it would be good to implement both of those :) But obviously want to consult with @czpython

Some other questions also arose:

What about namespacing the settings? Should we do that so as to avoid setting name clash between different apps?
What about if two apps conflict with each other? For example lets say we have an app called 'pink_admin' which makes the templates in the admin turn pink and an app called 'purple_admin' which makes the admin turn purple. If both are enabled then surely one will win over the other or something else odd will happen. Should we worry about that now?

czpython

Did a quick scan through the pr.
Will do a full review after addressing the open questions.

czpython · 2018-06-22T20:23:30Z

CHANGELOG.txt

@@ -1,3 +1,8 @@
+=== 4.0.0 (unreleased) ===


I know the PR guidelines suggest to do this but for 4.0.x, let's leave the changelog as is.
Once we're closer to release, then we can use the pr descriptions to compose the changelog.
The reason for this is that otherwise there will be conflicts all the time with changelog file and will need constant rebasing, etc..

czpython · 2018-06-22T20:25:22Z

cms/app_base.py

+
+        :param app: django app that has defined the feature as enabled
+        """
+        raise NotImplementedError()


when not passing arguments, raise NotImplementedError, vs NotImplementedError()

czpython · 2018-06-22T20:27:14Z

cms/utils/setup.py

@@ -2,6 +2,9 @@
 from django.core.exceptions import ImproperlyConfigured

 from cms.utils.compat.dj import is_installed as app_is_installed
+from cms.app_registration import (
+    autodiscover_cms_configs, get_cms_apps,
+    configure_cms_apps)


please change to:

from cms.app_registration import ( autodiscover_cms_configs, configure_cms_apps, get_cms_apps, )

czpython · 2018-06-22T20:29:04Z

cms/test_utils/project/app_with_bad_cms_file/cms_config.py

@@ -0,0 +1 @@
+raise KeyError


Is there a reason for the KeyError? maybe use a RuntimeError instead.
Also some comments here would be good.

czpython · 2018-06-22T20:30:39Z

cms/app_registration.py

+        # function if a cms_config.py file with a suitable class is found.
+        if hasattr(app_config, 'cms_app'):
+            cms_apps.append(app_config)
+


remove newline

czpython · 2018-06-22T20:30:49Z

cms/app_registration.py

+    """
+    Returns django app configs of apps with a cms config
+    """
+    cms_apps = []


I think this can be a list comprehension

czpython · 2018-06-22T20:31:09Z

cms/app_registration.py

+                    "class which inherits from CMSAppConfig")
+
+
+def get_cms_apps():


decorate with lru_cache(max_size=None)

czpython · 2018-06-22T20:45:35Z

@monikasulik
My answers below:

I'm not a big fan of list based config, mostly because it makes it a bit more difficult to turn off one feature (moderation) in an installation. One of my ideas for the boolean approach was to add a setting which defaults to True for each app supported. Example:

djangocms_moderation_enabled = settings.ENABLE_MODERATION_FOR_BLOG

We can achieve the same with list/tuple approach but IMHO is less readable.

After seeing the app in full (via this pr) and based on your initial feedback, I'm now convinced that separate classes will be cleaner. So 👍 from me.
The way I saw settings was using the app name as prefix, so something like djangocms_moderation_, but I'm not sure is something we need to do automatically.
I'm seeing more as convention because its up to the app to use/validate its own setting.
This is already an existing issue with django apps, so I don't think is something we need to address. The general rule is, last loaded app wins.

mmoravcik · 2018-06-25T09:01:00Z

@czpython, re point 1 in your answer.

I thought that the purpose of foo_enabled is to determine if the app supports foo integration, not that it is enabled for this integration? E.g. if I look at some package, and I don't see foo_enabled, I am not sure if the app supports foo or not. That was the idea behind supported_apps = ['foo', 'bar'] and then we can even have the extra one foo_enabled=settings.FOO_ENABLED.

App registration: Paulo's comments

monikasulik · 2018-06-25T15:25:51Z

@czpython I think I've acted on all comments now.

monikasulik · 2018-06-25T16:15:38Z

@evildmp I have now improved the PR description. Let me know if you need any extra info to write the actual docs.

czpython · 2018-06-26T12:14:43Z

cms/utils/setup.py

@@ -2,6 +2,11 @@
 from django.core.exceptions import ImproperlyConfigured

 from cms.utils.compat.dj import is_installed as app_is_installed
+from cms.app_registration import (


import order.
move this one before utils import

czpython · 2018-06-26T12:22:50Z

cms/app_base.py

+    # by checking for classes which are subclasses of CMSAppConfig.
+
+
+class CMSAppExtension(object):


because we've now split the config from extension, we should use ABC class here and mark the configure_app method as abstract.

czpython · 2018-06-26T12:32:00Z

cms/test_utils/project/app_with_cms_config/cms_config.py

+    app_with_cms_feature_enabled = True
+    # a test attr that should get changed to True by
+    # app_with_cms_feature on successful configuration
+    configured = False


I know this is for testing purposes but we should avoid apps that alter another app config in any way.
In this case I suggest to use something like app_with_cms_feature_strings = ['something'] then have the configure_app method on app_with_cms_feature hold all strings for all apps that extend it.

czpython · 2018-06-26T12:37:42Z

cms/test_utils/project/app_with_cms_feature/cms_config.py

+
+class CMSSomeFeatureConfig(CMSAppExtension):
+
+    num_configured_apps = 0


I suggest declaring this in the __init__

czpython · 2018-06-26T12:38:07Z

cms/test_utils/project/app_with_cms_feature/cms_config.py

+    raise Exception('some_function called')
+
+
+class SomeClass():


because of python2 support, subclass object

czpython · 2018-06-26T12:52:54Z

cms/tests/test_app_registration.py

+        'cms.test_utils.project.app_with_feature_not_implemented',
+        'cms.test_utils.project.app_using_non_feature'
+    ])
+    def test_raises_not_implemented_exception_when_feature_app_doesnt_implement_configure_method(self):


can be shortened to test_raises_not_implemented_exception and the rest explained in comment

czpython · 2018-06-26T12:57:21Z

cms/app_registration.py

+        enabled_property = "{}_enabled".format(app_with_feature.label)
+        configure_app = app_with_feature.cms_extension.configure_app
+
+        for app_config in apps.get_app_configs():


there should be a function that returns all apps with cms configs.
this can be wrapped with lru_cache for performance

In the spirit of YAGNI, wouldn't it be better to add that when we actually need it rather than before? It's a simple thing to add later if it is needed.

We would use it here, and will be good because then we can compute it once at runtime

You want one with all cms apps (both extensions and configs) or one with just the cms apps that define a config? Cause actually I think in this case it makes more sense if it's just the apps that have a cms_config attribute.

Correct, just the ones with cms config

czpython · 2018-06-26T12:59:57Z

cms/app_registration.py

+                continue
+            if getattr(app_config.cms_config, enabled_property, False):
+                # Feature enabled for this app so configure
+                configure_app(app_config)


We should pass the cms_config directly here.
It will cover most cases, otherwise everyone would have to do app.cms_config all the time
in their configure_app method and I consider cms_config an internal that we shouldn't expose.

Ok, that will mean that I'll also need to override the __init__ to take the django app config as in your original design. People might need things like app label, verbose_name etc. for things.

Now that I've just done this and looked at how the code is now, it has occurred to me that it would probably be neater to do def configure_app(cms_config, app_config) rather than override init and do cms_config.app_config 😉 Thoughts?

Don't like it because it implies that they're different while they're not, most cases will not need to use app_config.

czpython · 2018-06-26T13:02:10Z

cms/tests/test_app_registration.py

+    @override_settings(INSTALLED_APPS=[
+        'cms.test_utils.project.app_with_bad_cms_file',
+    ])
+    def test_raises_exception_raised_in_cms_file(self):


rename to test_exception_propagates_from_cms_file

czpython · 2018-06-26T13:07:36Z

cms/app_registration.py

+    """
+    cms_config_classes = []
+    # Find all classes that inherit from CMSAppConfig
+    for name, obj in inspect.getmembers(cms_module):


this logic should be a function because is used by the _find_extension above.

monikasulik · 2018-06-26T16:29:58Z

@czpython I think all your comments have been addressed now.
Still need to update the PR description though @evildmp because the changes have changed how people would interact with app registration.

czpython · 2018-06-26T19:19:06Z

cms/app_base.py

+        cms config classes, the method app A has defined will run twice,
+        once for app B and once for app C.
+
+        :param app: django app that has defined the feature as enabled


param is now cms_config

czpython · 2018-06-26T19:20:33Z

cms/app_base.py

@@ -1,4 +1,5 @@
 # -*- coding: utf-8 -*-
+from abc import ABC, abstractmethod


Have a look at https://stackoverflow.com/a/35673504/389453 for python2 and 3 compat

czpython · 2018-06-26T19:23:16Z

cms/test_utils/project/app_with_cms_feature/cms_config.py

+from cms.app_base import CMSAppExtension
+
+
+def some_function(*args, **kwargs):


this is a bit odd. what does this test?

As the comment in the next line says - that autodiscover doesn't pick this up :) Just wanted to add some random functions and classes in there, so that we know that autodiscover only ever picks up classes that inherit from CMSAppConfig and CMSAppExtension. Possibly now a bit redundant because the separation into two classes means that the code would have to check the parent class anyway, which wasn't true before.

Yeah I think is fine to remove this

czpython · 2018-06-26T19:26:47Z

cms/app_registration.py

+from cms.app_base import CMSAppConfig, CMSAppExtension
+
+
+CMS_CONFIG_NAME = 'cms_config'


I think this can go into the constants.py

czpython · 2018-06-26T19:27:40Z

cms/app_registration.py

+    classes = []
+    # Find all classes that inherit from klass
+    for name, obj in inspect.getmembers(cms_module):
+        is_cms_config = (


rename to is_subclass

czpython · 2018-06-26T19:30:16Z

cms/app_registration.py

+            if config:
+                app_config.cms_config = config(app_config)
+            if extension:
+                app_config.cms_extension = extension()


should we also pass app_config here for consistency?

IMO no because nobody should really be accessing app_config.cms_extension anyway, so there's no need for app_config.cms_extension.django_app. They are two different classes so don't think they need the init to be the same.

czpython · 2018-06-26T19:34:05Z

cms/tests/test_app_registration.py

+        app_registration.get_cms_config_apps.cache_clear()
+
+    @patch.object(apps, 'get_app_configs')
+    def test_runs_configure_app_method_for_app_with_enabled_config(


one line is fine here

czpython · 2018-06-26T19:34:12Z

cms/tests/test_app_registration.py

+            config_app.cms_config)
+
+    @patch.object(apps, 'get_app_configs')
+    def test_doesnt_run_configure_app_method_for_disabled_app(


czpython · 2018-06-26T19:34:17Z

cms/tests/test_app_registration.py

+        self.assertFalse(feature_app.cms_extension.configure_app.called)
+
+    @patch.object(apps, 'get_app_configs')
+    def test_doesnt_raise_exception_if_not_cms_app(


czpython · 2018-06-26T19:34:51Z

cms/tests/test_app_registration.py

+            self.fail("Exception raised when cms app config not defined")
+
+    @patch.object(apps, 'get_app_configs')
+    def test_runs_configure_app_method_for_correct_apps_when_multiple_apps(


monikasulik added 12 commits June 14, 2018 10:55

App registry: autodiscover

4f4139e

App registration: Add cms_app attr to django app config

7db2fc9

App registration: run extension methods

0394871

App registration: Some clean up

0ee728e

App registration: Edge cases

f79f575

App registration: Configure CMS apps

f329f22

App registration: More clean up

b5f1825

Merge pull request #3 from monikasulik/app_registration_config

5b88aa1

App registration - configuring apps

App registration: Test comments

c9f064d

App registration: Integrate into start up (WIP)

e53eac3

App registration: Clean up

f2299db

Merge pull request #4 from monikasulik/app_registration_start_up

2416687

App registration start up

monikasulik added 5 commits June 21, 2018 15:23

App registration: Remove import test (inconsistent across envs) and u…

ec35c49

…se cms_config.py filenames instead of cms_apps.py

App registration: Clean up CMSAppConfig class

5968fc9

Move app registration test apps to test_utils

c1b606e

App registration: CHANGELOG and AUTHORS changes

367e49f

Merge pull request #5 from monikasulik/app_registration_improvements

31d767d

App registration: improvements after Paulo's comments

monikasulik changed the title ~~WIP: App registration~~ App registration Jun 22, 2018

czpython suggested changes Jun 22, 2018

View reviewed changes

czpython self-assigned this Jun 22, 2018

czpython added kind: enhancement status: accepted labels Jun 22, 2018

czpython added this to the 4.0.0 milestone Jun 22, 2018

czpython changed the title ~~App registration~~ Introduced app registration system Jun 22, 2018

czpython added the status: work in progress label Jun 22, 2018

monikasulik added 4 commits June 25, 2018 10:49

App registration: Various minor changes after code review

d908a66

App registration: Split app config into 2 classes

093408c

App registration: Polish up after separating config base classes

263c1c5

Merge pull request #6 from monikasulik/app_registration_improvements

741a803

App registration: Paulo's comments

Fix python 2.7 compatibility issue

bcc02f4

czpython suggested changes Jun 26, 2018

View reviewed changes

monikasulik added 3 commits June 26, 2018 17:20

App registration: Improvements after code review

e64a652

Clean up

6a55666

Clean up

9f3f6f2

czpython suggested changes Jun 26, 2018

View reviewed changes

monikasulik and others added 3 commits June 26, 2018 20:35

Fix abc import for python 2.7

6e683d9

App registration: Clean up

0850fd1

Minor cleanup

2055642

czpython approved these changes Jun 26, 2018

View reviewed changes

czpython merged commit 97515c8 into django-cms:release/4.0.x Jun 26, 2018

Aiky30 mentioned this pull request Dec 8, 2020

Consider to replace cms_app.py against apps.py #4600

Closed

		"class which inherits from CMSAppConfig")


		def get_cms_apps():

		# by checking for classes which are subclasses of CMSAppConfig.


		class CMSAppExtension(object):


		class CMSSomeFeatureConfig(CMSAppExtension):

		num_configured_apps = 0

		@@ -1,4 +1,5 @@
		# -- coding: utf-8 --
		from abc import ABC, abstractmethod

		from cms.app_base import CMSAppExtension


		def some_function(args, *kwargs):

		from cms.app_base import CMSAppConfig, CMSAppExtension


		CMS_CONFIG_NAME = 'cms_config'

Uh oh!

Introduced app registration system #6421

Introduced app registration system #6421

Uh oh!

Conversation

Uh oh!

Summary

Proposed changes in this pull request

Documentation checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!