10000 Integrate wizards with app registration by monikasulik · Pull Request #6436 · django-cms/django-cms · GitHub
[go: up one dir, main page]
Skip to content

Integrate wizards with app registration #6436

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Jul 11, 2018
Merged

Integrate wizards with app registration #6436

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
85edda6
WIP: Change wizards to use cms_config.py instead of pool
monikasulik Jul 2, 2018
4a59081
Fix tests
monikasulik Jul 2, 2018
156f07d
Modify get_entries to not use wizard_pool
monikasulik Jul 3, 2018
a54a4ee
Backwards compatibility of get_entry
monikasulik Jul 4, 2018
2d28a1b
cms_wizards setting validation
monikasulik Jul 4, 2018
4a47450
Backwards compatibility of wizard pool
monikasulik Jul 5, 2018
56931e8
Clean up
monikasulik Jul 5, 2018
5d4c924
Polishing up after code review
monikasulik Jul 6, 2018
f70ed48
WIP: Integration test for backwards compatibility of wizard pool auto…
monikasulik Jul 9, 2018
77d61d0
Display warning (instead of exception) when registering the same wiza…
monikasulik Jul 9, 2018
107ed5b
Integration test for backwards compatibility of registering wizards
monikasulik Jul 9, 2018
97c648a
Ensure backwards compatibility of cms_wizards.py autodiscover
monikasulik Jul 10, 2018
785153c
Restructure backwards compatibility code for pools
monikasulik Jul 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Display warning (instead of exception) when registering the same wiza… 
…rd twice
  • Loading branch information
@monikasulik
monikasulik committed Jul 9, 2018
commit 77d61d004964f2c06e11638c7e8d103f47ece868
15 changes: 8 additions & 7 deletions cms/cms_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from logging import getLogger
from collections import Iterable

from django.core.exceptions import ImproperlyConfigured
Expand All @@ -7,6 +8,9 @@
from cms.wizards.wizard_base import Wizard


logger = getLogger(__name__)


class CMSCoreConfig(CMSAppConfig):
cms_enabled = True
cms_wizards = [cms_page_wizard, cms_subpage_wizard]
Expand All @@ -31,13 +35,10 @@ def configure_wizards(self, cms_config):
"from cms.wizards.wizard_base.Wizard"
)
elif wizard.id in self.wizards:
pass
# TODO: This is currently breaking various tests
# raise ImproperlyConfigured(
# "Wizard for model {} has already been registered".format(
# wizard.get_model()
# )
#)
msg = "Wizard for model {} has already been registered".format(
wizard.get_model()
)
logger.warning(msg)
else:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. The test setup is loading cms and sampleapp as expected and their wizards are getting registered
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self.wizards[wizard.id] = wizard

Expand Down
1 change: 1 addition & 0 deletions cms/test_utils/project/backwards_wizards/cms_wizards.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from cms.test_utils.project.backwards_wizard.wizards import wizard
from cms.wizards.wizard_pool import wizard_pool


# NOTE: We keep this line separate from the actual wizard definition
Expand Down
1 change: 0 additions & 1 deletion cms/test_utils/project/backwards_wizards/wizards.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
from cms.wizards.wizard_base import Wizard
from cms.wizards.wizard_pool import wizard_pool

from cms.test_utils.project.sampleapp.forms import SampleWizardForm

Expand Down
29 changes: 17 additions & 12 deletions cms/tests/test_cms_config_wizards.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from mock import Mock
from mock import Mock, patch

from django.apps import apps
from django.core.exceptions import ImproperlyConfigured
Expand Down Expand Up @@ -63,17 +63,22 @@ def test_raises_exception_if_not_iterable(self):
with self.assertRaises(ImproperlyConfigured):
extensions.configure_wizards(cms_config)

# TODO: Currently adding this check breaks various tests elsewhere
#~ def test_cant_register_the_same_wizard_twice(self):
#~ """
#~ If a wizard is already added to the dict, raise an exception.
#~ """
#~ extensions = CMSCoreExtensions()
#~ wizard = Mock(id=81, spec=Wizard)
#~ cms_config = Mock(
#~ cms_enabled=True, cms_wizards=[wizard, wizard])
#~ with self.assertRaises(ImproperlyConfigured):
#~ extensions.configure_wizards(cms_config)
@patch('cms.cms_config.logger.warning')
def test_warning_if_registering_the_same_wizard_twice(self, mocked_logger):
"""
If a wizard is already added to the dict log a warning.
"""
extensions = CMSCoreExtensions()
wizard = Mock(id=81, spec=Wizard)
cms_config = Mock(
cms_enabled=True, cms_wizards=[wizard, wizard])
extensions.configure_wizards(cms_config)
warning_msg = "Wizard for model {} has already been registered".format(
wizard.get_model())
# Warning message displayed
mocked_logger.assert_called_once_with(warning_msg)
# wizards dict is still what we expect it to be
self.assertDictEqual(extensions.wizards, {81: wizard})


class ConfigureWizardsIntegrationTestCase(CMSTestCase):
Expand Down
0