Integrate wizards with app registration #6436

monikasulik · 2018-07-05T16:19:16Z

Summary

Implement wizards via the new app registration system. Ensure backwards compatibility

Proposed changes in this pull request

This changes (although ensures backwards compatibility with) how wizards will be registered with the cms going forwards. From now on to register wizards one needs to do the following:

Create a cms_config.py file in the app (if one doesn't exist already)
Implement a class that inherits from app_base.CMSAppConfig (if it doesn't exist already) and make sure it has the following attributes:

class Config(CMSAppConfig):
    cms_enabled = True
    cms_wizards = [wizard1, wizard2]

where wizard1 and wizard2 are wizards as they are currently defined in cms_wizards.py

Documentation checklist

monikasulik · 2018-07-05T16:23:38Z

cms/wizards/wizard_pool.py

-
-    # PRIVATE METHODS -----------------
-
-    def _discover(self):


After thinking about this later, I'm getting the impression I may have messed up backwards compatibility with this. Because although the method wizard_pool.register still works, there is now no code that actually looks for cms_wizards.py
@czpython Where would you like the code that searches for cms_wizards.py files to live now?

I think the __init__ in CMSCoreExtensions might be a good place.

Ok, this is bizarre. Just wrote a test with an app that doesn't have cms_config.py but only cms_wizards.py and it seems to have registered the wizard from cms_wizards.py without me adding this into the init! Will check what's up there Monday morning, but possibly all that is needed is that test?

Likely there is an import somewhere of cms_wizards.py which should be removed

So, I can't use the __init__ in CMSCoreExtensions due to the order in which things happen (it means attempting to add wizards to a CMSCoreExtensions instance that doesn't exist yet). However, I have added this elsewhere. Hope this will work for you.

coveralls · 2018-07-05T16:41:47Z

Coverage remained the same at 78.189% when pulling 785153c on FidelityInternational:FIL-256-wizard-app-reg into 0394153 on divio:release/4.0.x.

czpython

It's looking good!

czpython · 2018-07-05T21:26:59Z

cms/cms_config.py

+        Adds all registered wizards from apps that define them to the
+        wizards dictionary on this class
+        """
+        if not hasattr(cms_config, 'cms_wizards'):


I think this would be better in the configure_app method.
So this method only runs if wizards is configured.

czpython · 2018-07-05T21:27:55Z

cms/cms_config.py

+            # The cms_wizards settings is optional. If it's not here
+            # just move on.
+            return
+        if type(cms_config.cms_wizards) != list:


don't think we need to do a type check here
maybe just check if cms_wizards is iterable

czpython · 2018-07-05T21:28:30Z

cms/cms_config.py

+            if not isinstance(wizard, Wizard):
+                raise ImproperlyConfigured(
+                    "all wizards defined in cms_wizards must inherit "
+                    "from cms.wizards.wizard_base.Wizard")


match closing bracket with raise

czpython · 2018-07-05T21:29:38Z

cms/cms_config.py

+                raise ImproperlyConfigured(
+                    "all wizards defined in cms_wizards must inherit "
+                    "from cms.wizards.wizard_base.Wizard")
+            else:


should we do an elif checking that the wizard has not been registered already?

So I did this locally and am now struggling with random tests failing. What seems to be happening is:

The test setup is loading cms and sampleapp as expected and their wizards are getting registered

The individual tests use the override_settings decorator with INSTALLED_APPS and include the cms app on the revised list, which in turn triggers another autodiscover thing because the cms's ready method is getting called twice. And therefore raises an error.

I can think of some ways of fixing this, but all these ways seem really hacky and will be really annoying and unobvious for future test writers. Do you have an idea for a nice way of doing this?
Alternatively, because we're using a dict, nothing bad is actually going to happen if a wizard is added to the dict twice (it'll just override the previous wizard), so we could just change this to a logger.warning? Really our code can cope with double registration of wizards, it's just that the developer has done something fishy if that's what's happening, so we should warn them.

Is the app config instance setup on the first load, persisting through to the second load on the tests that use override_settings?

@czpython I believe so, yes. I seem to be struggling on a similar issue with the integration test on backwards compatibility (needed for th 8000 e work on retaining the autodiscover of cms_wizards) as well. I've just pushed that.

czpython · 2018-07-05T21:30:48Z

cms/tests/test_cms_config.py

@@ -0,0 +1,97 @@
+from mock import Mock


I would suggest to make this test_cms_config_wizards
Basically each feature in the config should have its own file.

czpython · 2018-07-05T22:10:47Z

cms/tests/test_wizards.py

+        Test for backwards compatibility of the unregister method.
+        Removes a wizard from the wizards dict.
+        """
+        was_unregistered = wizard_pool.unregister(cms_page_wizard)



remove newline

czpython · 2018-07-05T22:12:27Z

cms/tests/test_wizards.py

+        """
+        user = self.get_superuser()
+        page = create_page('home', 'nav_playground.html', 'en', published=True)
+


remove newline

czpython · 2018-07-05T22:12:30Z

cms/tests/test_wizards.py

+        page = create_page('home', 'nav_playground.html', 'en', published=True)
+
+        wizard_choices = [option for option in entry_choices(user, page)]
+


remove newline

czpython · 2018-07-05T22:12:33Z

cms/tests/test_wizards.py

+        page = create_page('home', 'nav_playground.html', 'en', published=True)
+
+        wizard_choices = [option for option in entry_choices(user, page)]
+


remove newline

czpython · 2018-07-05T22:12:37Z

cms/tests/test_wizards.py

+        """
+        user = self.get_superuser()
+        page = create_page('home', 'nav_playground.html', 'en', published=True)
+


remove newline

…discover

…rd twice

monikasulik added 7 commits July 2, 2018 09:39

WIP: Change wizards to use cms_config.py instead of pool

85edda6

Fix tests

4a59081

Modify get_entries to not use wizard_pool

156f07d

Backwards compatibility of get_entry

a54a4ee

cms_wizards setting validation

2d28a1b

Backwards compatibility of wizard pool

4a47450

Clean up

56931e8

monikasulik commented Jul 5, 2018

View reviewed changes

czpython suggested changes Jul 5, 2018

View reviewed changes

czpython changed the title ~~WIP: Register wizards with the new app registration~~ Integrate wizards with app registration Jul 5, 2018

czpython self-assigned this Jul 5, 2018

czpython added status: work in progress kind: enhancement status: accepted labels Jul 5, 2018

czpython added this to the 4.0.0 milestone Jul 5, 2018

monikasulik added 6 commits July 6, 2018 15:20

Polishing up after code review

5d4c924

WIP: Integration test for backwards compatibility of wizard pool auto…

f70ed48

…discover

Display warning (instead of exception) when registering the same wiza…

77d61d0

…rd twice

Integration test for backwards compatibility of registering wizards

107ed5b

Ensure backwards compatibility of cms_wizards.py autodiscover

97c648a

Restructure backwards compatibility code for pools

785153c

czpython approved these changes Jul 11, 2018

View reviewed changes

czpython merged commit c8f56a9 into django-cms:release/4.0.x Jul 11, 2018

czpython removed the status: work in progress label Jul 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Integrate wizards with app registration #6436

Integrate wizards with app registration #6436

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		page = create_page('home', 'nav_playground.html', 'en', published=True)

		wizard_choices = [option for option in entry_choices(user, page)]

Uh oh!

Integrate wizards with app registration #6436

Integrate wizards with app registration #6436

Uh oh!

Conversation

Summary

Proposed changes in this pull request

Documentation checklist

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!

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!