From 4afe5e15f45c7aa3df17cdeef2cf84b5c29eed6a Mon Sep 17 00:00:00 2001 From: James Rutherford Date: Fri, 14 Sep 2012 10:25:42 +0100 Subject: [PATCH 1/8] CMS_PERMISSION: optionally use raw id user lookups The "view restrictions" and "page permissions" inlines on the "page" admin change forms can cause performance problems where there are many thousands of users being put into simple select boxes. Added a new setting (CMS_RAW_ID_USERS) that, if set, forces the inlines on that page to use standard Django admin "raw ID" lookups rather than select boxes. The restriction on which users may be selected should still be honoured by the new widget because we set the 'limit_choices_to' attribute to be the same as the queryset on the original widget. Using raw_id_fields in combination with limit_choices_to blows up if you have many thousands of users. For this reason, we only apply this limit if the number of users is relatively small (< 500, though that figure is somewhat arbitrary). If the number of users we need to limit to is greater than that, we use the usual input field instead unless the user is a CMS superuser, in which case we bypass the limit. Unfortunately, this means that non-superusers won't see any benefit from this change. --- cms/__init__.py | 2 +- cms/admin/forms.py | 41 +++++++++++++++++++++++++++++++----- cms/admin/permissionadmin.py | 9 ++++++++ 3 files changed, 46 insertions(+), 6 deletions(-) diff --git a/cms/__init__.py b/cms/__init__.py index 593b25ed9bd..b3f80659f7d 100644 --- a/cms/__init__.py +++ b/cms/__init__.py @@ -1,5 +1,5 @@ # -*- coding: utf-8 -*- -__version__ = '2.2' +__version__ = '2.2.1' # patch settings try: diff --git a/cms/admin/forms.py b/cms/admin/forms.py index da3d53e3757..8e27ef37e96 100644 --- a/cms/admin/forms.py +++ b/cms/admin/forms.py @@ -2,12 +2,12 @@ from cms.apphook_pool import apphook_pool from cms.forms.widgets import UserSelectAdminWidget from cms.models import (Page, PagePermission, PageUser, ACCESS_PAGE, - PageUserGroup) + PageUserGroup, GlobalPagePermission) from cms.utils.mail import mail_page_user_change from cms.utils.page import is_valid_page_slug from cms.utils.page_resolver import get_page_from_path from cms.utils.permissions import (get_current_user, get_subordinate_users, - get_subordinate_groups) + get_subordinate_groups, get_user_permission_level) from cms.utils.urlutils import any_path_re from django import forms from django.conf import settings @@ -188,14 +188,45 @@ class PagePermissionInlineAdminForm(forms.ModelForm): level or under him in choosen page tree, and users which were created by him, but aren't assigned to higher page level than current user. """ - user = forms.ModelChoiceField('user', label=_('user'), widget=UserSelectAdminWidget, required=False) page = forms.ModelChoiceField(Page, label=_('user'), widget=HiddenInput(), required=True) def __init__(self, *args, **kwargs): super(PagePermissionInlineAdminForm, self).__init__(*args, **kwargs) user = get_current_user() # current user from threadlocals - self.fields['user'].queryset = get_subordinate_users(user) - self.fields['user'].widget.user = user # assign current user + sub_users = get_subordinate_users(user) + + limit_choices = True + use_raw_id = False + if hasattr(settings, 'CMS_RAW_ID_USERS') and settings.CMS_RAW_ID_USERS: + if len(sub_users) < 500: + # If there aren't too many users, proceed as normal and use a raw + # id field with limit_choices_to + limit_choices = True + use_raw_id = True + elif get_user_permission_level(user) == 0: + # If there are enough choices to possibly cause a 414 request + # URI too large error, we only proceed with the raw id field if + # the user is a superuser & thus can legitimately circumvent + # the limit_choices_to condition. + limit_choices = False + use_raw_id = True + + # We can't use the fancy custom widget if the admin form wants to use a + # raw id field for the user + if use_raw_id: + # We can't set a queryset on a raw id lookup, but we can use the + # fact that it respects the limit_choices_to parameter. + from django.contrib.admin.widgets import ForeignKeyRawIdWidget + if isinstance(self.fields['user'].widget, ForeignKeyRawIdWidget): + if limit_choices: + self.fields['user'].widget.rel.limit_choices_to = dict( + id__in=list(sub_users.values_list('pk', flat=True)) + ) + else: + self.fields['user'].widget = UserSelectAdminWidget() + self.fields['user'].queryset = sub_users + self.fields['user'].widget.user = user # assign current user + self.fields['group'].queryset = get_subordinate_groups(user) def clean(self): diff --git a/cms/admin/permissionadmin.py b/cms/admin/permissionadmin.py index 81f2ecc24f7..b581737ef0a 100644 --- a/cms/admin/permissionadmin.py +++ b/cms/admin/permissionadmin.py @@ -22,6 +22,15 @@ class PagePermissionInlineAdmin(admin.TabularInline): classes = ['collapse', 'collapsed'] exclude = ['can_view'] + def __getattribute__(self, name): + # Dynamically set raw_id_fields based on settings + if name == 'raw_id_fields': + if hasattr(settings, 'CMS_RAW_ID_USERS') and settings.CMS_RAW_ID_USERS: + return ['user'] + return [] + else: + return super(PagePermissionInlineAdmin, self).__getattribute__(name) + def queryset(self, request): """ Queryset change, so user with global change permissions can see From 1ea6e555f9f93adef00ace7bdb3b1ada6ce28e1f Mon Sep 17 00:00:00 2001 From: James Rutherford Date: Fri, 14 Sep 2012 11:04:19 +0100 Subject: [PATCH 2/8] Removed unused import --- cms/admin/forms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/admin/forms.py b/cms/admin/forms.py index 8e27ef37e96..15e7f2a91da 100644 --- a/cms/admin/forms.py +++ b/cms/admin/forms.py @@ -2,7 +2,7 @@ from cms.apphook_pool import apphook_pool from cms.forms.widgets import UserSelectAdminWidget from cms.models import (Page, PagePermission, PageUser, ACCESS_PAGE, - PageUserGroup, GlobalPagePermission) + PageUserGroup) from cms.utils.mail import mail_page_user_change from cms.utils.page import is_valid_page_slug from cms.utils.page_resolver import get_page_from_path From 2b1b614f8ecb46251f135ccd1444aae8b1c16630 Mon Sep 17 00:00:00 2001 From: James Rutherford Date: Tue, 16 Oct 2012 14:19:24 +0100 Subject: [PATCH 3/8] .count() rather than len() for performance boost --- cms/__init__.py | 2 +- cms/admin/forms.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cms/__init__.py b/cms/__init__.py index b3f80659f7d..5f69184b65c 100644 --- a/cms/__init__.py +++ b/cms/__init__.py @@ -1,5 +1,5 @@ # -*- coding: utf-8 -*- -__version__ = '2.2.1' +__version__ = '2.2.2' # patch settings try: diff --git a/cms/admin/forms.py b/cms/admin/forms.py index 15e7f2a91da..205ca5fcdf5 100644 --- a/cms/admin/forms.py +++ b/cms/admin/forms.py @@ -198,7 +198,7 @@ def __init__(self, *args, **kwargs): limit_choices = True use_raw_id = False if hasattr(settings, 'CMS_RAW_ID_USERS') and settings.CMS_RAW_ID_USERS: - if len(sub_users) < 500: + if sub_users.count() < 500: # If there aren't too many users, proceed as normal and use a raw # id field with limit_choices_to limit_choices = True From b3183f261c2dda1fb7bd9d4e18051acfeda5a0b4 Mon Sep 17 00:00:00 2001 From: James Rutherford Date: Thu, 18 Oct 2012 10:15:07 +0100 Subject: [PATCH 4/8] CMS_RAW_ID_USERS: added default setting + docs Also removed the previous version bump so it can merge cleanly upstream. --- cms/__init__.py | 2 +- cms/conf/global_settings.py | 3 +++ docs/getting_started/configuration.rst | 26 ++++++++++++++++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/cms/__init__.py b/cms/__init__.py index 5f69184b65c..593b25ed9bd 100644 --- a/cms/__init__.py +++ b/cms/__init__.py @@ -1,5 +1,5 @@ # -*- coding: utf-8 -*- -__version__ = '2.2.2' +__version__ = '2.2' # patch settings try: diff --git a/cms/conf/global_settings.py b/cms/conf/global_settings.py index f2be9437c4e..e89b6727005 100644 --- a/cms/conf/global_settings.py +++ b/cms/conf/global_settings.py @@ -26,6 +26,9 @@ # Whether to enable permissions. CMS_PERMISSION = False +# Whether to use raw ID lookups for users when CMS_PERMISSION is set to True +CMS_RAW_ID_USERS = False + # Decides if pages without any view restrictions are public by default, or staff only CMS_PUBLIC_FOR = 'all' # or 'staff' diff --git a/docs/getting_started/configuration.rst b/docs/getting_started/configuration.rst index 263871868c0..636c93e55d5 100644 --- a/docs/getting_started/configuration.rst +++ b/docs/getting_started/configuration.rst @@ -438,6 +438,32 @@ a certain page all users he creates can, in turn, only edit this page. Naturally he can limit the rights of the users he creates even further, allowing them to see only a subset of the pages he's allowed access to, for example. +.. setting:: CMS_RAW_ID_USERS + +CMS_RAW_ID_USERS +================ + +Default: ``False`` + +This setting only applies if :setting:`CMS_PERMISSION` is ``True`` + +The "view restrictions" and "page permissions" inlines on the +:class:`cms.models.Page` admin change forms can cause performance problems +where there are many thousands of users being put into simple select boxes. If +``True``, this setting forces the inlines on that page to use standard Django +admin raw ID widgets rather than select boxes, dramatically improving +performance. + +.. note:: Using raw ID fields in combination with ``limit_choices_to`` causes + errors due to excessively long URLs if you have many thousands of + users (the PKs are all included in the URL of the popup window). For + this reason, we only apply this limit if the number of users is + relatively small (fewer than 500). If the number of users we need to + limit to is greater than that, we use the usual input field instead + unless the user is a CMS superuser, in which case we bypass the + limit. Unfortunately, this means that non-superusers won't see any + benefit from this setting. + .. setting:: CMS_PUBLIC_FOR CMS_PUBLIC_FOR From 5e823d5ef37e95c4716918517f2a257cb979fe8b Mon Sep 17 00:00:00 2001 From: James Rutherford Date: Mon, 12 Nov 2012 17:08:44 +0000 Subject: [PATCH 5/8] clearer conditional check --- cms/admin/forms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/admin/forms.py b/cms/admin/forms.py index 205ca5fcdf5..4c75e686ce9 100644 --- a/cms/admin/forms.py +++ b/cms/admin/forms.py @@ -197,7 +197,7 @@ def __init__(self, *args, **kwargs): limit_choices = True use_raw_id = False - if hasattr(settings, 'CMS_RAW_ID_USERS') and settings.CMS_RAW_ID_USERS: + if getattr(settings, 'CMS_RAW_ID_USERS', False): if sub_users.count() < 500: # If there aren't too many users, proceed as normal and use a raw # id field with limit_choices_to From ec9f8d8a1376081cae6f0df11b1376c9b8e94a49 Mon Sep 17 00:00:00 2001 From: James Rutherford Date: Fri, 5 Apr 2013 14:18:08 +0100 Subject: [PATCH 6/8] Only use raw ID above a certain number of users So the RAW_ID_USERS setting should now be either left as False or set to a positive integer which will be the threshold for the number of users above which the raw ID widget will be used. --- cms/admin/forms.py | 2 ++ cms/admin/permissionadmin.py | 4 +++- docs/getting_started/configuration.rst | 5 +++-- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/cms/admin/forms.py b/cms/admin/forms.py index 0438f74b47e..2c0946eb728 100644 --- a/cms/admin/forms.py +++ b/cms/admin/forms.py @@ -229,6 +229,8 @@ def __init__(self, *args, **kwargs): # raw id field for the user if use_raw_id: from django.contrib.admin.widgets import ForeignKeyRawIdWidget + # This check will be False if the number of users in the system + # is less than the threshold set by the RAW_ID_USERS setting. if isinstance(self.fields['user'].widget, ForeignKeyRawIdWidget): # We can't set a queryset on a raw id lookup, but we can use # the fact that it respects the limit_choices_to parameter. diff --git a/cms/admin/permissionadmin.py b/cms/admin/permissionadmin.py index cc7b8bcca5d..e4cfef33b2e 100644 --- a/cms/admin/permissionadmin.py +++ b/cms/admin/permissionadmin.py @@ -7,6 +7,7 @@ from cms.utils.permissions import get_user_permission_level from copy import deepcopy from django.contrib import admin +from django.contrub.auth.models import User from django.template.defaultfilters import title from django.utils.translation import ugettext as _ @@ -29,7 +30,8 @@ class PagePermissionInlineAdmin(TabularInline): def __getattribute__(self, name): # Dynamically set raw_id_fields based on settings if name == 'raw_id_fields': - if get_cms_setting('RAW_ID_USERS'): + threshold = get_cms_setting('RAW_ID_USERS') + if threshold and User.objects.count() > threshold: return ['user'] return [] else: diff --git a/docs/getting_started/configuration.rst b/docs/getting_started/configuration.rst index 8ea59a1c39a..553c546a9b2 100644 --- a/docs/getting_started/configuration.rst +++ b/docs/getting_started/configuration.rst @@ -542,8 +542,9 @@ This setting only applies if :setting:`CMS_PERMISSION` is ``True`` The "view restrictions" and "page permissions" inlines on the :class:`cms.models.Page` admin change forms can cause performance problems where there are many thousands of users being put into simple select boxes. If -``True``, this setting forces the inlines on that page to use standard Django -admin raw ID widgets rather than select boxes, dramatically improving +set to a positive integer, this setting forces the inlines on that page to use +standard Django admin raw ID widgets rather than select boxes if the number of +users in the system is greater than that number, dramatically improving performance. .. note:: Using raw ID fields in combination with ``limit_choices_to`` causes From 61055b90ea6e020152776a4cedc40e5d2cf7f6c6 Mon Sep 17 00:00:00 2001 From: James Rutherford Date: Fri, 5 Apr 2013 14:34:37 +0100 Subject: [PATCH 7/8] import typo --- cms/admin/permissionadmin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/admin/permissionadmin.py b/cms/admin/permissionadmin.py index e4cfef33b2e..bb78bbd4e89 100644 --- a/cms/admin/permissionadmin.py +++ b/cms/admin/permissionadmin.py @@ -7,7 +7,7 @@ from cms.utils.permissions import get_user_permission_level from copy import deepcopy from django.contrib import admin -from django.contrub.auth.models import User +from django.contrib.auth.models import User from django.template.defaultfilters import title from django.utils.translation import ugettext as _ From 9a778f5663d596646f93ed125a35a42af63b2a9d Mon Sep 17 00:00:00 2001 From: James Rutherford Date: Mon, 8 Apr 2013 10:46:35 +0100 Subject: [PATCH 8/8] Switch from __getattribute__ to classproperty Added the new classproperty helper in cms.utils.helpers. --- cms/admin/permissionadmin.py | 17 ++++++++--------- cms/utils/helpers.py | 33 ++++++++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 10 deletions(-) diff --git a/cms/admin/permissionadmin.py b/cms/admin/permissionadmin.py index bb78bbd4e89..5081b5a1ee2 100644 --- a/cms/admin/permissionadmin.py +++ b/cms/admin/permissionadmin.py @@ -4,6 +4,7 @@ from cms.exceptions import NoPermissionsException from cms.models import Page, PagePermission, GlobalPagePermission, PageUser from cms.utils.conf import get_cms_setting +from cms.utils.helpers import classproperty from cms.utils.permissions import get_user_permission_level from copy import deepcopy from django.contrib import admin @@ -26,16 +27,14 @@ class PagePermissionInlineAdmin(TabularInline): classes = ['collapse', 'collapsed'] exclude = ['can_view'] extra = 0 # edit page load time boost - - def __getattribute__(self, name): + + @classproperty + def raw_id_fields(cls): # Dynamically set raw_id_fields based on settings - if name == 'raw_id_fields': - threshold = get_cms_setting('RAW_ID_USERS') - if threshold and User.objects.count() > threshold: - return ['user'] - return [] - else: - return super(PagePermissionInlineAdmin, self).__getattribute__(name) + threshold = get_cms_setting('RAW_ID_USERS') + if threshold and User.objects.count() > threshold: + return ['user'] + return [] def queryset(self, request): """ diff --git a/cms/utils/helpers.py b/cms/utils/helpers.py index f70d77bf4f3..615f17631ad 100644 --- a/cms/utils/helpers.py +++ b/cms/utils/helpers.py @@ -66,4 +66,35 @@ def make_revision_with_plugins(obj, user=None, message=None): revision_context.add_to_context(revision_manager, plugin, bpadapter.get_version_data(plugin, VERSION_CHANGE)) def find_placeholder_relation(obj): - return 'page' \ No newline at end of file + return 'page' + + +class classproperty(object): + """Like @property, but for classes, not just instances. + + Example usage: + + >>> from cms.utils.helpers import classproperty + >>> class A(object): + ... @classproperty + ... def x(cls): + ... return 'x' + ... @property + ... def y(self): + ... return 'y' + ... + >>> A.x + 'x' + >>> A().x + 'x' + >>> A.y + + >>> A().y + 'y' + + """ + def __init__(self, fget): + self.fget = fget + + def __get__(self, owner_self, owner_cls): + return self.fget(owner_cls)