8000 fix: some Django antipatterns by jrief · Pull Request #7867 · django-cms/django-cms · GitHub
[go: up one dir, main page]
Skip to content

fix: some Django antipatterns #7867

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 7 commits into from
Apr 20, 2024
Merged

fix: some Django antipatterns #7867

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a01fc41
replace antipattern force_str(_(…)) -> gettext(…)
jrief Apr 9, 2024
2e40d24
fix antipattern: use JsonResponse as intended
jrief Apr 9, 2024
b994119
Update pageadmin.py
jrief Apr 10, 2024
3f0172d
Merge branch 'develop-4' into fix-antipatterns
fsbraun Apr 20, 2024
bdcb676
Pageadmin: gettext only
fsbraun Apr 20, 2024
72ae356
placeholderadmin.py: gettext only
fsbraun Apr 20, 2024
3b6ea83
Merge branch 'develop-4' into fix-antipatterns
fsbraun Apr 20, 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
34 changes: 18 additions & 16 deletions cms/admin/pageadmin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
from django.urls import re_path
from django.utils.decorators import method_decorator
from django.utils.encoding import force_str
from django.utils.translation import gettext_lazy as _
from django.utils.translation import gettext as _
from django.views.decorators.http import require_POST

from cms import operations
Expand Down Expand Up @@ -279,7 +279,7 @@ def set_home(self, request, object_id):
raise self._get_404_exception(object_id)

if not page.is_potential_home():
return HttpResponseBadRequest(force_str(_("The page is not eligible to be home.")))
return HttpResponseBadRequest(_("The page is not eligible to be home."))

new_home_tree, old_home_tree = page.set_as_homepage(request.user)

Expand Down Expand Up @@ -597,8 +597,8 @@ def move_page(self, request, page_id, extra_context=None):

# Does the user have permissions to do this...?
if not can_move_page or (target and not target.has_add_permission(user)):
message = force_str(
_("Error! You don't have permissions to move this page. Please reload the page")
message = _(
"Error! You don't have permissions to move this page. Please reload the page"
)
return jsonify_request(HttpResponseForbidden(message))

Expand Down Expand Up @@ -706,15 +706,17 @@ def copy_page(self, request, page_id):
can_copy_page = page_permissions.user_can_add_page(user, site)

if not can_copy_page:
message = force_str(_("Error! You don't have permissions to copy this page."))
message = _("Error! You don't have permissions to copy this page.")
return jsonify_request(HttpResponseForbidden(message))

page_languages = page.get_languages()
site_languages = get_language_list(site_id=site.pk)

if not any(lang in page_languages for lang in site_languages):
message = force_str(_("Error! The page you're pasting is not "
"translated in any of the languages configured by the target site."))
message = _(
"Error! "
"The page you're pasting is not translated in any of the languages configured by the target site."
)
return jsonify_request(HttpResponseBadRequest(message))

new_page = form.copy_page(user)
Expand All @@ -725,7 +727,7 @@ def edit_title_fields(self, request, page_id, language):
translation = page.get_content_obj(language, fallback=False)

if not self.has_change_permission(request, obj=page):
return HttpResponseForbidden(force_str(_("You do not have permission to edit this page")))
return HttpResponseForbidden(_("You do not have permission to edit this page"))

if page is None:
raise self._get_404_exception(page_id)
Expand Down Expand Up @@ -1180,12 +1182,12 @@ def change_template(self, request, object_id):
to_template = request.POST.get("template", None)

if to_template not in dict(get_cms_setting('TEMPLATES')):
return HttpResponseBadRequest(force_str(_("Template not valid")))
return HttpResponseBadRequest(_("Template not valid"))

page_content.template = to_template
page_content.save()

return HttpResponse(force_str(_("The template was successfully changed")))
return HttpResponse(_("The template was successfully changed"))

@require_POST
@transaction.atomic
Expand All @@ -1202,7 +1204,7 @@ def copy_language(self, request, object_id):
page = source_page_content.page

if not target_language or target_language not in get_language_list(site_id=page.node.site_id):
return HttpResponseBadRequest(force_str(_("Language must be set to a supported language!")))
return HttpResponseBadRequest(_("Language must be set to a supported language!"))

target_page_content = page.get_content_obj(target_language, fallback=False)

Expand All @@ -1212,7 +1214,7 @@ def copy_language(self, request, object_id):
plugins = placeholder.get_plugins_list(source_page_content.language)

if not target.has_add_plugins_permission(request.user, plugins):
return HttpResponseForbidden(force_str(_('You do not have permission to copy these plugins.')))
return HttpResponseForbidden(_("You do not have permission to copy these plugins."))
copy_plugins_to_placeholder(plugins, target, language=target_language)
return HttpResponse("ok")

Expand All @@ -1225,7 +1227,7 @@ def delete_view(self, request, object_id, extra_context=None):
request_language = get_site_language_from_request(request, site_id=page.node.site_id)

if not self.has_delete_translation_permission(request, language, page):
return HttpResponseForbidden(force_str(_("You do not have permission to delete this page")))
return HttpResponseForbidden(_("You do not have permission to delete this page"))

if page is None:
raise self._get_404_exception(object_id)
Expand Down Expand Up @@ -1336,11 +1338,11 @@ def change_innavigation(self, request, object_id):
if not self.has_change_permission(request, obj=page_content):
if self.has_change_permission(request):
# General (permission) problem
message = _("You do not have permission to change a page's navigation status")
message = "You do not have permission to change a page's navigation status"
else:
# Only this page? Can be permissions or versioning, or ...
message = _("You cannot change this page's navigation status")
return HttpResponseForbidden(force_str(message))
message = "You cannot change this page's navigation status"
return HttpResponseForbidden(_(message))

if page_content is None:
raise self._get_404_exception(object_id)
Expand Down
60 changes: 31 additions & 29 deletions cms/admin/placeholderadmin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,13 @@ def edit_field(self, request, object_id, language):
if not fields:
context = {
'opts': opts,
'message': force_str(_("Field %s not found")) % raw_fields
'message': _("Field %s not found") % raw_fields
}
return render(request, 'admin/cms/page/plugin/error_form.html', context)
if not request.user.has_perm(f"{self.model._meta.app_label}.change_{self.model._meta.model_name}"):
context = {
'opts': opts,
'message': force_str(_("You do not have permission to edit this item"))
'message': _("You do not have permission to edit this item")
}
return render(request, 'admin/cms/page/plugin/error_form.html', context)
# Dynamically creates the form class with only `field_name` field
Expand Down Expand Up @@ -361,11 +361,11 @@ def add_plugin(self, request):
plugin_type = plugin_data['plugin_type']

if not self.has_add_plugin_permission(request, placeholder, plugin_type):
message = force_str(_('You do not have permission to add a plugin'))
message = _('You do not have permission to add a plugin')
return HttpResponseForbidden(message)

if not placeholder.check_source(request.user):
message = force_str(_('You do not have permission to add a plugin'))
message = _('You do not have permission to add a plugin')
return HttpResponseForbidden(message)

plugin_class = plugin_pool.get_plugin(plugin_type)
Expand Down Expand Up @@ -417,7 +417,7 @@ def copy_plugins(self, request):
target_placeholder = get_object_or_404(Placeholder, pk=target_placeholder_id)

if not target_language or target_language not in get_language_list():
return HttpResponseBadRequest(force_str(_("Language must be set to a supported language!")))
return HttpResponseBadRequest(_("Language must be set to a supported language!"))

copy_to_clipboard = target_placeholder.pk == request.toolbar.clipboard.pk
source_plugin_id = request.POST.get('source_plugin_id', None)
Expand Down Expand Up @@ -458,7 +458,7 @@ def _copy_plugin_to_clipboard(self, request, target_placeholder):

if not self.has_copy_plugins_permission(request, old_plugins):
message = _('You do not have permission to copy these plugins.')
raise PermissionDenied(force_str(message))
raise PermissionDenied(message)

if not target_placeholder.check_source(request.user):
message = _('You do not have permission to copy these plugins.')
Expand All @@ -483,11 +483,11 @@ def _copy_placeholder_to_clipboard(self, request, source_placeholder, target_pla

if not self.has_copy_plugins_permission(request, old_plugins):
message = _('You do not have permission to copy this placeholder.')
raise PermissionDenied(force_str(message))
raise PermissionDenied(message)

if not target_placeholder.check_source(request.user):
message = _('You do not have permission to copy this placeholder.')
raise PermissionDenied(force_str(message))
raise PermissionDenied(message)

# Empty the clipboard
target_placeholder.clear()
Expand Down Expand Up @@ -529,11 +529,11 @@ def _add_plugins_from_placeholder(self, request, source_placeholder, target_plac

if not has_permissions:
message = _('You do not have permission to copy these plugins.')
raise PermissionDenied(force_str(message))
raise PermissionDenied(message)

if not target_placeholder.check_source(request.user):
message = _('You do not have permission to copy these plugins.')
raise PermissionDenied(force_str(message))
raise PermissionDenied(message)

target_tree_order = target_placeholder.get_plugin_tree_order(
language=target_language,
Expand Down Expand Up @@ -577,18 +577,18 @@ def edit_plugin(self, request, plugin_id):
try:
plugin_id = int(plugin_id)
except ValueError:
return HttpResponseNotFound(force_str(_("Plugin not found")))
return HttpResponseNotFound(_("Plugin not found"))

obj = self._get_plugin_from_id(plugin_id)

# CMSPluginBase subclass instance
plugin_instance = obj.get_plugin_class_instance(admin=self.admin_site)

if not self.has_change_plugin_permission(request, obj):
return HttpResponseForbidden(force_str(_("You do not have permission to edit this plugin")))
return HttpResponseForbidden(_("You do not have permission to edit this plugin"))

if not obj.placeholder.check_source(request.user):
message = force_str(_("You do not have permission to edit this plugin"))
message = _("You do not have permission to edit this plugin")
return HttpResponseForbidden(message)

response = plugin_instance.change_view(request, str(plugin_id))
Expand Down Expand Up @@ -732,11 +732,11 @@ def _paste_plugin(self, request, plugin, target_language,
plugins = [plugin] + list(plugin.get_descendants())

if not self.has_copy_from_clipboard_permission(request, target_placeholder, plugins):
message = force_str(_("You have no permission to paste this plugin"))
message = _("You have no permission to paste this plugin")
raise PermissionDenied(message)

if not target_placeholder.check_source(request.user):
message = force_str(_("You have no permission to paste this plugin"))
message = _("You have no permission to paste this plugin")
raise PermissionDenied(message)

if target_parent:
Expand Down Expand Up @@ -792,11 +792,11 @@ def _paste_placeholder(self, request, plugin, target_language,
plugins = plugin.placeholder_ref.get_plugins_list()

if not self.has_copy_from_clipboard_permission(request, target_placeholder, plugins):
message = force_str(_("You have no permission to paste this placeholder"))
message = _("You have no permission to paste this placeholder")
raise PermissionDenied(message)

if not target_placeholder.check_source(request.user):
message = force_str(_("You have no permission to paste this placeholder"))
message = _("You have no permission to paste this placeholder")
raise PermissionDenied(message)

action_token = self._send_pre_placeholder_operation(
Expand Down Expand Up @@ -849,19 +849,19 @@ def _move_plugin(self, request, plugin, target_position, target_placeholder=None
source_placeholder = plugin.placeholder

if not self.has_move_plugin_permission(request, plugin, source_placeholder):
message = force_str(_("You have no permission to move this plugin"))
message = _("You have no permission to move this plugin")
raise PermissionDenied(message)

if target_placeholder and not self.has_move_plugin_permission(request, plugin, target_placeholder):
message = force_str(_("You have no permission to move this plugin"))
message = _("You have no permission to move this plugin")
raise PermissionDenied(message)

if not source_placeholder.check_source(request.user):
message = force_str(_("You have no permission to move this plugin"))
message = _("You have no permission to move this plugin")
raise PermissionDenied(message)

if target_placeholder and not target_placeholder.check_source(request.user):
message = force_str(_("You have no permission to move this plugin"))
message = _("You have no permission to move this plugin")
raise PermissionDenied(message)

if target_parent:
Expand Down Expand Up @@ -913,11 +913,11 @@ def _cut_plugin(self, request, plugin, target_language, target_placeholder):
source_placeholder = plugin.placeholder

if not self.has_move_plugin_permission(request, plugin, source_placeholder):
message = force_str(_("You have no permission to cut this plugin"))
message = _("You have no permission to cut this plugin")
raise PermissionDenied(message)

if not source_placeholder.check_source(request.user):
message = force_str(_("You have no permission to cut this plugin"))
message = _("You have no permission to cut this plugin")
raise PermissionDenied(message)

action_token = self._send_pre_placeholder_operation(
Expand Down Expand Up @@ -962,11 +962,11 @@ def delete_plugin(self, request, plugin_id):
plugin = self._get_plugin_from_id(plugin_id)

if not self.has_delete_plugin_permission(request, plugin):
return HttpResponseForbidden(force_str(
_("You do not have permission to delete this plugin")))
message = _("You do not have permission to delete this plugin")
return HttpResponseForbidden(message)

if not plugin.placeholder.check_source(request.user):
message = force_str(_("You do not have permission to delete this plugin"))
message = _("You do not have permission to delete this plugin")
raise PermissionDenied(message)

opts = plugin._meta
Expand Down Expand Up @@ -1044,10 +1044,11 @@ def clear_placeholder(self, request, placeholder_id):
return HttpResponseRedirect(admin_reverse('index', current_app=self.admin_site.name))

if not self.has_clear_placeholder_permission(request, placeholder, language):
return HttpResponseForbidden(force_str(_("You do not have permission to clear this placeholder")))
message = _("You do not have permission to clear this placeholder")
return HttpResponseForbidden(message)

if not placeholder.check_source(request.user):
message = force_str(_("You do not have permission to clear this placeholder"))
message = _("You do not have permission to clear this placeholder")
raise PermissionDenied(message)

opts = Placeholder._meta
Expand All @@ -1064,7 +1065,8 @@ def clear_placeholder(self, request, placeholder_id):
if request.POST:
# The user has already confirmed the deletion.
if perms_needed:
return HttpResponseForbidden(force_str(_("You do not have permission to clear this placeholder")))
message = _("You do not have permission to clear this placeholder")
return HttpResponseForbidden(message)

operation_token = self._send_pre_placeholder_operation(
request,
Expand Down
6 changes: 2 additions & 4 deletions cms/utils/admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
import json

from django.http import HttpResponse
from django.http import JsonResponse
from django.utils.encoding import smart_str

NOT_FOUND_RESPONSE = "NotFound"
Expand All @@ -13,4 +11,4 @@ def jsonify_request(response):
* content: original response content
"""
content = {'status': response.status_code, 'content': smart_str(response.content, response.charset)}
return HttpResponse(json.dumps(content), content_type="application/json")
return JsonResponse(content)
0