Add a Django system check... (#326)

* Add a Django system check that CONFIG_FIELDSETS accounts for all of CONFIG

* Fix test (don't actually modify values when submitting form) and restore python2 compatibility

constance/__init__.py

@@ -1,4 +1,5 @@
 from django.utils.functional import LazyObject
+from . import checks
 
 __version__ = '2.4.0'
 

constance/admin.py

@@ -24,6 +24,7 @@ from django.utils.module_loading import import_string
 from django.utils.translation import ugettext_lazy as _
 
 from . import LazyConfig, settings
+from .checks import get_inconsistent_fieldnames
 
 
 config = LazyConfig()
@@ -163,11 +164,7 @@ class ConstanceForm(forms.Form):
         if not settings.CONFIG_FIELDSETS:
             return cleaned_data
 
-        field_name_list = []
-        for fieldset_title, fields_list in settings.CONFIG_FIELDSETS.items():
-            for field_name in fields_list:
-                field_name_list.append(field_name)
-        if field_name_list and set(set(settings.CONFIG.keys()) - set(field_name_list)):
+        if get_inconsistent_fieldnames():
             raise forms.ValidationError(_('CONSTANCE_CONFIG_FIELDSETS is missing '
                                           'field(s) that exists in CONSTANCE_CONFIG.'))
 

constance/checks.py

@@ -0,0 +1,42 @@
+from django.core import checks
+from django.utils.translation import ugettext_lazy as _
+
+from . import settings
+
+
+@checks.register("constance")
+def check_fieldsets(*args, **kwargs):
+    """
+    A Django system check to make sure that, if defined, CONFIG_FIELDSETS accounts for
+    every entry in settings.CONFIG.
+    """
+    if hasattr(settings, "CONFIG_FIELDSETS") and settings.CONFIG_FIELDSETS:
+        inconsistent_fieldnames = get_inconsistent_fieldnames()
+        if inconsistent_fieldnames:
+            return [
+                checks.Warning(
+                    _(
+                        "CONSTANCE_CONFIG_FIELDSETS is missing "
+                        "field(s) that exists in CONSTANCE_CONFIG."
+                    ),
+                    hint=", ".join(sorted(inconsistent_fieldnames)),
+                    obj="settings.CONSTANCE_CONFIG",
+                    id="constance.E001",
+                )
+            ]
+    return []
+
+
+def get_inconsistent_fieldnames():
+    """
+    Returns a set of keys from settings.CONFIG that are not accounted for in
+    settings.CONFIG_FIELDSETS.
+    If there are no fieldnames in settings.CONFIG_FIELDSETS, returns an empty set.
+    """
+    field_name_list = []
+    for fieldset_title, fields_list in settings.CONFIG_FIELDSETS.items():
+        for field_name in fields_list:
+            field_name_list.append(field_name)
+    if not field_name_list:
+        return {}
+    return set(set(settings.CONFIG.keys()) - set(field_name_list))

tests/test_admin.py

@@ -3,6 +3,7 @@ from django.contrib import admin
 from django.contrib.auth.models import User, Permission
 from django.contrib.contenttypes.models import ContentType
 from django.core.exceptions import PermissionDenied
+from django.http import HttpResponseRedirect
 from django.template.defaultfilters import linebreaksbr
 from django.test import TestCase, RequestFactory
 from django.utils import six
@@ -74,6 +75,46 @@ class TestAdmin(TestCase):
         self.assertContains(response, '<h2>Numbers</h2>')
         self.assertContains(response, '<h2>Text</h2>')
 
+    @mock.patch('constance.settings.CONFIG_FIELDSETS', {
+        'FieldSetOne': ('INT_VALUE',)
+    })
+    @mock.patch('constance.settings.CONFIG', {
+        'INT_VALUE': (1, 'some int'),
+    })
+    @mock.patch('constance.settings.IGNORE_ADMIN_VERSION_CHECK', True)
+    def test_submit(self):
+        """
+        Test that submitting the admin page results in an http redirect when
+        everything is in order.
+        """
+        self.client.login(username='admin', password='nimda')
+        request = self.rf.post('/admin/constance/config/', data={
+            "INT_VALUE": settings.CONFIG['INT_VALUE'][0],
+            "version": "123",
+        })
+        request.user = self.superuser
+        request._dont_enforce_csrf_checks = True
+        with mock.patch("constance.admin.ConstanceForm.save"):
+            with mock.patch("django.contrib.messages.add_message"):
+                response = self.options.changelist_view(request, {})
+        self.assertIsInstance(response, HttpResponseRedirect)
+
+    @mock.patch('constance.settings.CONFIG_FIELDSETS', {
+        'Numbers': ('LONG_VALUE', 'INT_VALUE',),
+        'Text': ('STRING_VALUE', 'UNICODE_VALUE'),
+    })
+    def test_inconsistent_fieldset_submit(self):
+        """
+        Test that the admin page warns users if the CONFIG_FIELDSETS setting
+        doesn't account for every field in CONFIG.
+        """
+        self.client.login(username='admin', password='nimda')
+        request = self.rf.post('/admin/constance/config/', data=None)
+        request.user = self.superuser
+        request._dont_enforce_csrf_checks = True
+        response = self.options.changelist_view(request, {})
+        self.assertContains(response, 'is missing field(s)')
+
     @mock.patch('constance.settings.CONFIG_FIELDSETS', {
         'Numbers': ('LONG_VALUE', 'INT_VALUE',),
     })

tests/test_checks.py

@@ -0,0 +1,45 @@
+# -*- coding: utf-8 -*-
+
+import datetime
+from decimal import Decimal
+import mock
+
+from constance.admin import get_values
+from constance.checks import check_fieldsets, get_inconsistent_fieldnames
+from constance.management.commands.constance import _set_constance_value
+from django.core.exceptions import ValidationError
+from django.test import TestCase
+from constance import settings
+
+
+class ChecksTestCase(TestCase):
+    @mock.patch("constance.settings.CONFIG_FIELDSETS", {"Set1": settings.CONFIG.keys()})
+    def test_get_inconsistent_fieldnames_none(self):
+        """
+        Test that get_inconsistent_fieldnames returns an empty set and no checks fail
+        if CONFIG_FIELDSETS accounts for every key in settings.CONFIG.
+        """
+        self.assertFalse(get_inconsistent_fieldnames())
+        self.assertEqual(0, len(check_fieldsets()))
+
+    @mock.patch(
+        "constance.settings.CONFIG_FIELDSETS",
+        {"Set1": list(settings.CONFIG.keys())[:-1]},
+    )
+    def test_get_inconsistent_fieldnames_one(self):
+        """
+        Test that get_inconsistent_fieldnames returns a set and the check fails
+        if CONFIG_FIELDSETS does not account for every key in settings.CONFIG.
+        """
+        self.assertTrue(get_inconsistent_fieldnames())
+        self.assertEqual(1, len(check_fieldsets()))
+
+    @mock.patch(
+        "constance.settings.CONFIG_FIELDSETS", {}
+    )
+    def test_check_fieldsets(self):
+        """
+        check_fieldsets should not output warning if CONFIG_FIELDSETS is not defined.
+        """
+        del settings.CONFIG_FIELDSETS
+        self.assertEqual(0, len(check_fieldsets()))