Some speedups (permissions) and a cms.api fix #958

Merged
merged 2 commits into from Aug 25, 2011

Projects

None yet

3 participants

@stephrdev
Contributor

Some changes to speed up the page change_list and to fix the add_plugin api.

This pull request is to review the changes and is no "ready for checkin" request. Tests are missing, but maybe someone would finish this work.

@chrisglass chrisglass commented on the diff Aug 19, 2011
cms/templatetags/cms_admin.py
- icon = boolean_icon(all_perms.exists())
- return mark_safe(
- ugettext('<span title="Restrictions: %(title)s">%(icon)s</span>') % {
- 'title': u', '.join((perm.get_grant_on_display() for perm in all_perms)) or None,
- 'icon': icon,
- })
+ if settings.CMS_PERMISSION:
+ all_perms = get_any_page_view_permissions(request, page)
+ icon = boolean_icon(all_perms.exists())
+ return mark_safe(
+ ugettext('<span title="Restrictions: %(title)s">%(icon)s</span>') % {
+ 'title': u', '.join((perm.get_grant_on_display() for perm in all_perms)) or None,
+ 'icon': icon,
+ })
+ else:
+ icon = boolean_icon(None)
@chrisglass
chrisglass Aug 19, 2011 Collaborator

This case is basically the only part that needs testing - the other case is currently covered by tests already (obviously making sure the other one is correct, too)

@chrisglass chrisglass commented on the diff Aug 19, 2011
cms/utils/admin.py
@@ -41,9 +41,10 @@ def get_admin_menu_item_context(request, page, filtered=False):
moderator_state = moderator.page_moderator_state(request, page)
has_add_on_same_level_permission = False
opts = Page._meta
- if (request.user.has_perm(opts.app_label + '.' + opts.get_add_permission()) and
- GlobalPagePermission.objects.with_user(request.user).filter(can_add=True, sites__in=[page.site_id])):
- has_add_on_same_level_permission = True
+ if settings.CMS_PERMISSION:
@chrisglass
chrisglass Aug 19, 2011 Collaborator

This is not covered for CMS_PERMISSION = False.

The rest of the file could benefit from a little permission testing love while we're at it :)

@chrisglass
Collaborator

This makes a lot of sense - generally the permissions-checking code is simply not executed when the permissions setting is False. Sane defaults are set when not using the permissions a all.
Why it isn't already the case is a mystery.

@ojii ojii commented on the diff Aug 19, 2011
cms/api.py
@@ -266,10 +267,14 @@ def add_plugin(placeholder, plugin_type, language, position='last-child',
# validate and normalize plugin type
plugin_model, plugin_type = _verify_plugin_type(plugin_type)
+
+ max_pos = CMSPlugin.objects.filter(language=language,
+ placeholder=placeholder).aggregate(Max('position')).values()[0] or 0
@ojii
ojii Aug 19, 2011 Collaborator

why .values()[0] instead of ['position_max']?

@ojii ojii commented on the diff Aug 19, 2011
cms/templatetags/cms_admin.py
@@ -80,13 +80,21 @@ def boolean_icon(value):
@register.filter
def is_restricted(page, request):
- all_perms = get_any_page_view_permissions(request, page)
- icon = boolean_icon(all_perms.exists())
- return mark_safe(
- ugettext('<span title="Restrictions: %(title)s">%(icon)s</span>') % {
- 'title': u', '.join((perm.get_grant_on_display() for perm in all_perms)) or None,
- 'icon': icon,
- })
+ if settings.CMS_PERMISSION:
+ all_perms = get_any_page_view_permissions(request, page)
+ icon = boolean_icon(all_perms.exists())
@ojii
ojii Aug 19, 2011 Collaborator

actually while we're optimizing: Since we iterate over all_perms later (line 88), this actually does an extra query. using list(all_perms) and checking the list is not empty would cause less queries.

@chrisglass
chrisglass Aug 19, 2011 Collaborator

Good point :)

@ojii ojii merged commit c8312f7 into divio:develop Aug 25, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment