Fixed django-cms#6035 -- Don't alter the AdvancedSettingsForm form data (django-cms#6262)

Author: czpython

CHANGELOG.txt

@@ -9,6 +9,7 @@
   deprecation warnings.
 * Fixed a bug where the sitemap would ignore the ``public`` setting of the site languages
   and thus display hidden languages.
+* Fixed an ``AttributeError`` raised when adding or removing apphooks in Django 1.11.
 
 
 === 3.4.5 (2017-10-12) ===

cms/admin/forms.py

@@ -233,8 +233,11 @@ class AdvancedSettingsForm(forms.ModelForm):
 
     # This is really a 'fake' field which does not correspond to any Page attribute
     # But creates a stub field to be populate by js
-    application_configs = forms.ChoiceField(label=_('Application configurations'),
-                                            choices=(), required=False, widget=ApplicationConfigSelect)
+    application_configs = forms.CharField(
+        label=_('Application configurations'),
+        required=False,
+        widget=ApplicationConfigSelect,
+    )
     fieldsets = (
         (None, {
             'fields': ('overwrite_url', 'redirect'),
@@ -282,32 +285,32 @@ def __init__(self, *args, **kwargs):
             if app_configs:
                 self.fields['application_configs'].widget = ApplicationConfigSelect(
                     attrs={'id': 'application_configs'},
-                    app_configs=app_configs)
+                    app_configs=app_configs,
+                )
 
                 if page_data.get('application_urls', False) and page_data['application_urls'] in app_configs:
-                    self.fields['application_configs'].choices = [(config.pk, force_text(config)) for config in app_configs[page_data['application_urls']].get_configs()]
+                    configs = app_configs[page_data['application_urls']].get_configs()
+                    self.fields['application_configs'].widget.choices = [(config.pk, force_text(config)) for config in configs]
 
-                    apphook = page_data.get('application_urls', False)
                     try:
-                        config = apphook_pool.get_apphook(apphook).get_configs().get(namespace=self.initial['application_namespace'])
+                        config = configs.get(namespace=self.initial['application_namespace'])
                         self.fields['application_configs'].initial = config.pk
                     except ObjectDoesNotExist:
                         # Provided apphook configuration doesn't exist (anymore),
                         # just skip it
                         # The user will choose another value anyway
                         pass
-                else:
-                    # If app_config apphook is not selected, drop any value
-                    # for application_configs to avoid the field data from
-                    # being validated by the field itself
-                    try:
-                        del self.data['application_configs']
-                    except KeyError:
-                        pass
 
         if 'redirect' in self.fields:
             self.fields['redirect'].widget.language = self.fields['language'].initial
 
+    def get_apphooks(self):
+        for hook in apphook_pool.get_apphooks():
+            yield (hook[0], apphook_pool.get_apphook(hook[0]))
+
+    def get_apphooks_with_config(self):
+        return {key: app for key, app in self.get_apphooks() if app.app_config}
+
     def get_navigation_extenders(self):
         return menu_pool.get_menus_by_attribute("cms_enabled", True)
 
@@ -364,12 +367,29 @@ def clean(self):
         instance_namespace = cleaned_data.get('application_namespace', None)
         application_config = cleaned_data.get('application_configs', None)
         if apphook:
+            apphooks_with_config = self.get_apphooks_with_config()
+
             # application_config wins over application_namespace
-            if application_config:
+            if apphook in apphooks_with_config and application_config:
                 # the value of the application config namespace is saved in
                 # the 'usual' namespace field to be backward compatible
                 # with existing apphooks
-                config = apphook_pool.get_apphook(apphook).get_configs().get(pk=int(application_config))
+                try:
+                    appconfig_pk = forms.IntegerField(required=True).to_python(application_config)
+                except ValidationError:
+                    self._errors['application_configs'] = ErrorList([
+                        _('Invalid application config value')
+                    ])
+                    return self.cleaned_data
+
+                try:
+                    config = apphooks_with_config[apphook].get_configs().get(pk=appconfig_pk)
+                except ObjectDoesNotExist:
+                    self._errors['application_configs'] = ErrorList([
+                        _('Invalid application config value')
+                    ])
+                    return self.cleaned_data
+
                 if self._check_unique_namespace_instance(config.namespace):
                     # Looks like there's already one with the default instance
                     # namespace defined.

cms/test_utils/project/sampleapp/admin.py

@@ -1,6 +1,7 @@
-from cms.admin.placeholderadmin import PlaceholderAdminMixin
 from django.contrib import admin
-from cms.test_utils.project.sampleapp.models import Picture, Category
+
+from cms.admin.placeholderadmin import PlaceholderAdminMixin
+from cms.test_utils.project.sampleapp.models import Picture, Category, SampleAppConfig
 
 
 class PictureInline(admin.StackedInline):
@@ -11,3 +12,4 @@ class CategoryAdmin(PlaceholderAdminMixin, admin.ModelAdmin):
     inlines = [PictureInline]
 
 admin.site.register(Category, CategoryAdmin)
+admin.site.register(SampleAppConfig)

cms/test_utils/project/sampleapp/cms_apps.py

@@ -1,10 +1,15 @@
-from cms.app_base import CMSApp
-from cms.test_utils.project.sampleapp.cms_menus import SampleAppMenu, StaticMenu3, StaticMenu4
-from cms.apphook_pool import apphook_pool
 from django.conf.urls import url
+from django.core.exceptions import ObjectDoesNotExist
+from django.core.urlresolvers import reverse
 from django.http import HttpResponse
 from django.utils.translation import ugettext_lazy as _
 
+from cms.app_base import CMSApp
+from cms.test_utils.project.sampleapp.cms_menus import SampleAppMenu, StaticMenu3, StaticMenu4
+from cms.apphook_pool import apphook_pool
+
+from .models import SampleAppConfig
+
 
 class SampleApp(CMSApp):
     name = _("Sample App")
@@ -15,6 +20,26 @@ class SampleApp(CMSApp):
 apphook_pool.register(SampleApp)
 
 
+class SampleAppWithConfig(CMSApp):
+    name = _("Sample App with config")
+    app_config = SampleAppConfig
+
+    def get_urls(self, page=None, language=None, **kwargs):
+        return ["cms.test_utils.project.sampleapp.urls_sample_config"]
+
+    def get_configs(self):
+        return self.app_config.objects.all()
+
+    def get_config(self, namespace):
+        try:
+            return self.app_config.objects.get(namespace=namespace)
+        except ObjectDoesNotExist:
+            return None
+
+    def get_config_add_url(self):
+        return reverse('admin:%s_%s_add' % (self.app_config._meta.app_label, self.app_config._meta.model_name))
+
+
 class SampleAppWithExcludedPermissions(CMSApp):
     name = _("Sample App with excluded permissions")
     urls = [

cms/test_utils/project/sampleapp/models.py

@@ -24,3 +24,11 @@ class Meta:
 class Picture(models.Model):
     image = models.ImageField(upload_to="pictures")
     category = models.ForeignKey(Category)
+
+
+class SampleAppConfig(models.Model):
+    namespace = models.CharField(
+        default=None,
+        max_length=100,
+        unique=True,
+    )

cms/test_utils/project/sampleapp/urls_sample_config.py

@@ -0,0 +1,8 @@
+from django.conf.urls import url
+
+from . import views
+
+
+urlpatterns = [
+    url(r'^$', views.sample_view, {'message': 'sample root page',}, name='sample-config-root'),
+]

cms/tests/test_page_admin.py

@@ -1,7 +1,9 @@
 # -*- coding: utf-8 -*-
 import datetime
+import sys
 
 from django.core.cache import cache
+from django.core.urlresolvers import clear_url_caches
 from django.contrib import admin
 from django.contrib.sites.models import Site
 from django.forms.models import model_to_dict
@@ -16,6 +18,7 @@
 from cms.admin.forms import AdvancedSettingsForm
 from cms.admin.pageadmin import PageAdmin
 from cms.api import create_page, add_plugin, create_title
+from cms.appresolver import clear_app_resolvers
 from cms.constants import (
     PUBLISHER_STATE_DEFAULT,
     PUBLISHER_STATE_DIRTY,
@@ -30,6 +33,7 @@
     CMSTestCase, URL_CMS_PAGE, URL_CMS_PAGE_MOVE,
     URL_CMS_PAGE_ADVANCED_CHANGE, URL_CMS_PAGE_CHANGE, URL_CMS_PAGE_ADD
 )
+from cms.test_utils.project.sampleapp.models import SampleAppConfig
 from cms.test_utils.util.context_managers import LanguageOverride, UserLoginContext
 from cms.utils import get_cms_setting
 from cms.utils.compat.dj import installed_apps
@@ -1046,6 +1050,114 @@ def test_advanced_settings_form(self):
         self.assertEqual([no_translation_error],
                          form.errors['__all__'])
 
+    @override_settings(CMS_APPHOOKS=[
+        'cms.test_utils.project.sampleapp.cms_apps.SampleApp',
+        'cms.test_utils.project.sampleapp.cms_apps.SampleAppWithConfig',
+    ])
+    def test_advanced_settings_form_apphook_config(self):
+        clear_app_resolvers()
+        clear_url_caches()
+
+        if 'cms.test_utils.project.sampleapp.cms_apps' in sys.modules:
+            del sys.modules['cms.test_utils.project.sampleapp.cms_apps']
+
+        self.apphook_clear()
+
+        superuser = self.get_superuser()
+        app_config = SampleAppConfig.objects.create(namespace='sample')
+        cms_page = create_page('app', 'nav_playground.html', 'en', published=True)
+        cms_pages = Page.objects.filter(pk__in=[cms_page.pk, cms_page.publisher_public_id])
+        redirect_to = self.get_admin_url(Page, 'changelist')
+        endpoint = self.get_admin_url(Page, 'advanced', cms_page.pk)
+        page_data = {
+            "redirect": "",
+            "language": "en",
+            "reverse_id": "",
+            "navigation_extenders": "",
+            "site": "1",
+            "xframe_options": "0",
+            "application_urls": "SampleAppWithConfig",
+            "application_configs": app_config.pk,
+            "application_namespace": "sampleapp",
+            "overwrite_url": "",
+            "template": "INHERIT",
+        }
+
+        with self.login_user_context(superuser):
+            # set the apphook config
+            response = self.client.post(endpoint, page_data)
+            self.assertRedirects(response, redirect_to)
+            self.assertEqual(
+                cms_pages.filter(
+                    application_urls='SampleAppWithConfig',
+                    application_namespace=app_config.namespace,
+                ).count(),
+                2
+            )
+
+        with self.login_user_context(superuser):
+            # change from apphook with config to normal apphook
+            page_data['application_urls'] = 'SampleApp'
+            page_data['application_namespace'] = 'sampleapp'
+            response = self.client.post(endpoint, page_data)
+            self.assertRedirects(response, redirect_to)
+            self.assertEqual(
+                cms_pages.filter(
+                    application_urls='SampleApp',
+                    application_namespace='sampleapp',
+                ).count(),
+                2
+            )
+
+        with self.login_user_context(superuser):
+            # set the apphook config again
+            page_data['application_urls'] = 'SampleAppWithConfig'
+            page_data['application_namespace'] = 'sampleapp'
+            response = self.client.post(endpoint, page_data)
+            self.assertRedirects(response, redirect_to)
+            self.assertEqual(
+                cms_pages.filter(
+                    application_urls='SampleAppWithConfig',
+                    application_namespace=app_config.namespace,
+                ).count(),
+                2
+            )
+
+        with self.login_user_context(superuser):
+            # change the apphook config to an invalid value
+            expected_error = '<ul class="errorlist"><li>Invalid application config value</li></ul>'
+            page_data['application_configs'] = '2'
+            response = self.client.post(endpoint, page_data)
+            self.assertEqual(response.status_code, 200)
+            self.assertContains(response, expected_error)
+            self.assertEqual(
+                cms_pages.filter(
+                    application_urls='SampleAppWithConfig',
+                    application_namespace=app_config.namespace,
+                ).count(),
+                2
+            )
+
+        with self.login_user_context(superuser):
+            # remove the apphook
+            page_data['application_urls'] = ''
+            page_data['application_namespace'] = ''
+            response = self.client.post(endpoint, page_data)
+            self.assertRedirects(response, redirect_to)
+            self.assertEqual(
+                cms_pages.filter(
+                    application_urls='',
+                    application_namespace=None,
+                ).count(),
+                2,
+            )
+        clear_app_resolvers()
+        clear_url_caches()
+
+        if 'cms.test_utils.project.sampleapp.cms_apps' in sys.modules:
+            del sys.modules['cms.test_utils.project.sampleapp.cms_apps']
+        self.apphook_clear()
+
     def test_form_url_page_change(self):
         superuser = self.get_superuser()
         with self.login_user_context(superuser):