8000 refactor: Replace PageAdmin.delete_view by two smaller methods by jrief · Pull Request #7995 · django-cms/django-cms · GitHub
[go: up one dir, main page]
Skip to content

refactor: Replace PageAdmin.delete_view by two smaller methods #7995

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 4 commits into from
Sep 14, 2024
Merged

refactor: Replace PageAdmin.delete_view by two smaller methods #7995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f3e7d3f
Refactor PageAdmin.delete_view to prevent code duplication
jrief Sep 11, 2024
79561a3
Let `pageadmin.has_change_permission(obj=None)` return `page_permissi…
Sep 11, 2024
1d8cc36
Fix: Clear lru_cache of user object after deleting a page
Sep 12, 2024
80aef49
Fix typos
Sep 12, 2024
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
123 changes: 21 additions & 102 deletions cms/admin/pageadmin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@
get_language_tuple,
get_site_language_from_request,
)
from cms.utils.permissions import clear_permission_lru_caches
from cms.utils.plugins import copy_plugins_to_placeholder
from cms.utils.urlutils import admin_reverse

Expand All @@ -97,7 +98,6 @@ def get_site(request):

@admin.register(Page)
class PageAdmin(admin.ModelAdmin):
change_list_template = "admin/cms/page/tree/base.html"
actions_menu_template = 'admin/cms/page/tree/actions_dropdown.html'

form = AdvancedSettingsForm
Expand All @@ -113,10 +113,13 @@ def has_change_permission(self, request, obj=None):
Return true if the current user has permission on the page.
Return the string 'All' if the user has all rights.
"""
if obj is None:
return

site = get_site(request)
if obj is None:
# Checks if user can change at least one page
return page_permissions.user_can_change_at_least_one_page(
user=request.user,
site=site,
)
return page_permissions.user_can_change_page(request.user, page=obj, site=site)

def has_change_advanced_settings_permission(self, request, obj=None):
Expand Down Expand Up @@ -334,105 +337,22 @@ def get_list(self, *args, **kwargs):
return HttpResponseForbidden()

def changelist_view(self, request, extra_context=None):
can_change_any_page = page_permissions.user_can_change_at_least_one_page(
user=request.user,
site=get_site(request),
use_cache=False,
)

if not can_change_any_page:
raise Http404
return HttpResponseRedirect(admin_reverse('cms_pagecontent_changelist'))

@transaction.atomic
def delete_view(self, request, object_id, extra_context=None):
# This is an unfortunate copy/paste from django's delete view.
# The reason is to add the descendant pages to the deleted objects list.
opts = self.model._meta
app_label = opts.app_label

obj = self.get_object(request, object_id=object_id)

if not self.has_delete_permission(request, obj):
raise PermissionDenied

if obj is None:
raise self._get_404_exception(object_id)

# Populate deleted_objects, a data structure of all related objects that
# will also be deleted.
objs = [obj] + list(obj.get_descendant_pages())
parameter = "?" + request.GET.urlencode() if request.GET else ""
return HttpResponseRedirect(admin_reverse('cms_pagecontent_changelist') + parameter)

get_deleted_objects_additional_kwargs = {'request': request}
(deleted_objects, model_count, perms_needed, protected) = get_deleted_objects(
objs, admin_site=self.admin_site,
**get_deleted_objects_additional_kwargs
)

if request.POST and not protected: # The user has confirmed the deletion.
if perms_needed:
raise PermissionDenied
obj_display = force_str(obj)
obj_id = obj.serializable_value(opts.pk.attname)
self.log_deletion(request, obj, obj_display)
self.delete_model(request, obj)

if IS_POPUP_VAR in request.POST:
popup_response_data = json.dumps({
'action': 'delete',
'value': str(obj_id),
})
return TemplateResponse(request, self.popup_response_template or [
f'admin/{opts.app_label}/{opts.model_name}/popup_response.html',
'admin/%s/popup_response.html' % opts.app_label,
'admin/popup_response.html',
], {'popup_response_data': popup_response_data})

self.message_user(
request,
_('The %(name)s "%(obj)s" was deleted successfully.') % {
'name': force_str(opts.verbose_name),
'obj': force_str(obj_display),
},
messages.SUCCESS,
)

can_change_any_page = page_permissions.user_can_change_at_least_one_page(
user=request.user,
site=get_site(request),
use_cache=False,
)

if can_change_any_page:
query = self.get_preserved_filters(request)
post_url = admin_reverse('cms_pagecontent_changelist') + '?' + query
else:
post_url = admin_reverse('index')
return HttpResponseRedirect(post_url)

object_name = force_str(opts.verbose_name)
def response_delete(self, request, obj_display, obj_id):
"""
Determine the HttpResponse for the delete_view stage. Clear the user's permission
lru cache
"""
clear_permission_lru_caches(request.user)
return super().response_delete(request, obj_display, obj_id)

if perms_needed or protected:
title = _("Cannot delete %(name)s") % {"name": object_name}
else:
title = _("Are you sure?")

context = dict(
self.admin_site.each_context(request),
title=title,
object_name=object_name,
object=obj,
deleted_objects=deleted_objects,
model_count=dict(model_count).items(),
perms_lacking=perms_needed,
protected=protected,
opts=opts,
app_label=app_label,
is_popup=(IS_POPUP_VAR in request.POST or IS_POPUP_VAR in request.GET),
to_field=None,
)
context.update(extra_context or {})
return self.render_delete_form(request, context)
def get_deleted_objects(self, objs, request):
deleted_objs = list(objs)
for obj in objs:
deleted_objs.extend(obj.get_descendant_pages())
return super().get_deleted_objects(deleted_objs, request)

def delete_model(self, request, obj):
operation_token = send_pre_page_operation(
Expand Down Expand Up @@ -1061,7 +981,6 @@ def has_change_permission(self, request, obj=None):
can_change_page = page_permissions.user_can_change_at_least_one_page(
user=request.user,
site=site,
use_cache=False,
)
return can_change_page

Expand Down
5 changes: 3 additions & 2 deletions cms/tests/test_admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ def test_delete(self):
add_plugin(body, 'TextPlugin', 'en', body='text')
with self.login_user_context(admin_user):
data = {'post': 'yes'}
response = self.client.post(URL_CMS_PAGE_DELETE % page.pk, data)
response = self.client.post(URL_CMS_PAGE_DELETE % page.pk, data, follow=True)
self.assertRedirects(response, self.get_pages_admin_list_uri('en'))

def test_delete_diff_language(self):
Expand All @@ -148,7 +148,8 @@ def test_delete_diff_language(self):
add_plugin(body, 'TextPlugin', 'en', body='text')
with self.login_user_context(admin_user):
data = {'post': 'yes'}
response = self.client.post(URL_CMS_PAGE_DELETE % page.pk, data)
response = self.client.post(URL_CMS_PAGE_DELETE % page.pk, data, follow=True)
# follow=True, since page changelist redirects to page content changelist
self.assertRedirects(response, self.get_pages_admin_list_uri('en'))

def test_search_fields(self):
Expand Down
11 changes: 6 additions & 5 deletions cms/tests/test_page_admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1876,7 +1876,8 @@ def test_user_can_delete_empty_page(self):

with self.login_user_context(staff_user):
data = {'post': 'yes'}
response = self.client.post(endpoint, data)
response = self.client.post(endpoint, data, follow=True)
# follow=True, since page changelist redirects to page content changelist

self.assertRedirects(response, redirect_to)
self.assertFalse(self._page_exists())
Expand Down Expand Up @@ -1932,7 +1933,8 @@ def test_user_can_delete_non_empty_page(self):

with self.login_user_context(staff_user):
data = {'post': 'yes'}
response = self.client.post(endpoint, data)
response = self.client.post(endpoint, data, follow=True)
# follow=True, since page changelist redirects to page content changelist

self.assertRedirects(response, redirect_to)
self.assertFalse(self._page_exists())
Expand Down Expand Up @@ -3359,7 +3361,7 @@ def test_user_can_delete_empty_page(self):

with self.login_user_context(staff_user):
data = {'post': 'yes'}
response = self.client.post(endpoint, data)
response = self.client.post(endpoint, data, follow=True)

self.assertRedirects(response, redirect_to)
self.assertFalse(self._page_exists())
Expand Down Expand Up @@ -3423,10 +3425,9 @@ def test_user_can_delete_non_empty_page(self):
can_change=True,
can_delete=True,
)

with self.login_user_context(staff_user):
data = {'post': 'yes'}
response = self.client.post(endpoint, data)
response = self.client.post(endpoint, data, follow=True)

self.assertRedirects(response, redirect_to)
self.assertFalse(self._page_exists())
Expand Down
14 changes: 14 additions & 0 deletions cms/utils/permissions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,20 @@ def cached_func(user, *args, **kwargs):
return cached_func


def clear_func_cache(user, func):
func_cache_name = '_djangocms_cached_func_%s' % func.__name__
if hasattr(user, func_cache_name):
delattr(user, func_cache_name)


def clear_permission_lru_caches(user):
"""
Clear all python lru caches used by the permission system
"""
clear_func_cache(user, get_global_actions_for_user)
clear_func_cache(user, get_page_actions_for_user)


@cached_func
def get_global_actions_for_user(user, site):
actions = set()
Expand Down
Loading
0