fix: Unlocalize page and node ids when rendering the page tree in the…

… admin (#7188) * #7175: unlocalize page and node ids when rendering the page tree in the admin * Fix flake8 issue * Update the test so as not to have to generate one thousand pages. The downside is that it only works in sqlite * Make isort happy * #7155: extend unlocalisation of pks to a few more templates * #7175: cleanup, dont unlocalize in `if` tags * Update CHANGELOG about #7175 Co-authored-by: Fabian Braun <fsbraun@gmx.de> Co-authored-by: Mark Walker <mark@django-cms.org>
django-cms · Oct 9, 2022 · 9e3c579 · 9e3c579
1 parent 6d83a9f
commit 9e3c579
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 23 deletions.
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -7,6 +7,7 @@ unreleased
 
 * Make javascript dark mode functions available to popups as CMS.API.getColorScheme
   and CMS.API.setColorScheme
+* Unlocalize page and node ids when rendering the page tree in the admin (#7175)
 * Add support for tel: and mailto: URIs in Advanced Page Settings redirect field.
 * Allow to partially override ``CMS_CACHE_DURATIONS``
 

diff --git a/cms/admin/pageadmin.py b/cms/admin/pageadmin.py
@@ -1,5 +1,6 @@
 import copy
 import json
+import re
 import sys
 import uuid
 from collections import namedtuple
@@ -1401,8 +1402,12 @@ def get_tree(self, request):
         """
         site = self.get_site(request)
         pages = self.get_queryset(request)
-        node_id = request.GET.get('nodeId')
-        open_nodes = list(map(int, request.GET.getlist('openNodes[]')))
+        node_id = re.sub(r'[^\d]', '', request.GET.get('nodeId', '')) or None
+        open_nodes = list(map(
+            int,
+            [re.sub(r'[^\d]', '', node) for node in
+                request.GET.getlist('openNodes[]')]
+            ))
 
         if node_id:
             page = get_object_or_404(pages, node_id=int(node_id))

diff --git a/cms/templates/admin/cms/page/tree/actions_dropdown.html b/cms/templates/admin/cms/page/tree/actions_dropdown.html
@@ -1,12 +1,12 @@
-{% load i18n admin_urls %}
+{% load i18n l10n admin_urls %}
 {% spaceless %}
 <ul class="cms-pagetree-dropdown-menu-inner">
     {% block actions %}
         <li
             {% if has_copy_page_permission %}
                 class="js-cms-tree-item-copy"
-                data-id="{{ page.pk }}"
-                data-node-id="{{ node.pk }}"
+                data-id="{{ page.pk|unlocalize }}"
+                data-node-id="{{ node.pk|unlocalize }}"
                 data-apphook="{{ page.application_urls|default_if_none:"" }}"
             {% endif %}
         >
@@ -18,8 +18,8 @@
         <li
             {% if has_move_page_permission %}
                 class="js-cms-tree-item-cut"
-                data-id="{{ page.pk }}"
-                data-node-id="{{ node.pk }}"
+                data-id="{{ page.pk|unlocalize }}"
+                data-node-id="{{ node.pk|unlocalize }}"
             {% endif %}
         >
             <a{% if not has_move_page_permission %} class="cms-pagetree-dropdown-item-disabled"{% endif %}
@@ -29,8 +29,8 @@
             </a>
         </li>
         <li>
-            <a href="#" data-id="{{ page.pk }}"
-                data-node-id="{{ node.pk }}"
+            <a href="#" data-id="{{ page.pk|unlocalize }}"
+                data-node-id="{{ node.pk|unlocalize }}"
                 class="{% if has_add_permission %}js-cms-tree-item-paste{% endif %}
                 {% if not has_add_permission or not paste_enabled %} cms-pagetree-dropdown-item-disabled{% endif %}">
                 <span class="cms-icon cms-icon-alias"></span>

diff --git a/cms/templates/admin/cms/page/tree/base.html b/cms/templates/admin/cms/page/tree/base.html
@@ -1,5 +1,5 @@
 {% extends "admin/change_list.html" %}
-{% load i18n admin_list static admin_urls cms_admin cms_js_tags cms_static cms_tags %}
+{% load i18n l10n admin_list static admin_urls cms_admin cms_js_tags cms_static cms_tags %}
 
 {# TODO might not need that #}
 {% block title %}{% trans "List of pages" %}{% endblock %}
@@ -108,7 +108,7 @@ <h1>
                                 </li>
                                 {% for site in tree.sites %}
                                     <li{% if site.pk == tree.site.pk %} class="active"{% endif %}>
-                                        <a href="#{{ site.pk }}" class="js-cms-pagetree-site-trigger" data-id="{{ site.pk }}">{{ site.name }}</a>
+                                        <a href="#{{ site.pk|unlocalize }}" class="js-cms-pagetree-site-trigger" data-id="{{ site.pk|unlocalize }}">{{ site.name }}</a>
                                     </li>
                                 {% endfor %}
                                 {% if has_recover_permission %}
@@ -127,7 +127,7 @@ <h1>
                         <form method="post" class="js-cms-pagetree-site-form cms-hidden">
                             <select name="site">
                                 {% for site in tree.sites %}
-                                    <option value="{{ site.pk }}">{{ site.name }}</option>
+                                    <option value="{{ site.pk|unlocalize }}">{{ site.name }}</option>
                                 {% endfor %}
                             </select>
                             {% csrf_token %}
@@ -186,7 +186,7 @@ <h2>{% trans "Main Navigation" %}</h2>
                             "permission": {{ CMS_PERMISSION|bool }},
                             "debug": {{ DEBUG|bool }},
                             "filtered": {% if tree.is_filtered %}true{% else %}false{% endif %},
-                            "site": {{ tree.site.pk }},
+                            "site": {{ tree.site.pk|unlocalize }},
                             "hasAddRootPermission": {{ has_add_permission|yesno:"true,false" }},
                             "lang": {
                                 "code": "{{ preview_language|lower }}",
@@ -250,7 +250,7 @@ <h2>{% trans "Main Navigation" %}</h2>
                         "permission": {{ CMS_PERMISSION|bool }},
                         "debug": {{ DEBUG|bool }},
                         "filtered": {% if tree.is_filtered %}true{% else %}false{% endif %},
-                        "site": {{ tree.site.pk }},
+                        "site": {{ tree.site.pk|unlocalize }},
                         "hasAddRootPermission": {{ has_add_permission|yesno:"true,false" }},
                         "lang": {
                             "code": "{{ preview_language|lower }}",

diff --git a/cms/templates/admin/cms/page/tree/menu.html b/cms/templates/admin/cms/page/tree/menu.html
@@ -1,4 +1,4 @@
-{% load i18n cms_admin admin_urls %}
+{% load i18n cms_admin admin_urls l10n %}
 
 {# INFO: columns are defined in base.html options #}
 {% spaceless %}
@@ -16,9 +16,9 @@
         {% endif %}
     {% endblock %}
     "
-    {% if is_popup %}onclick="opener.dismissRelatedLookupPopup(window, {{ page.id }}); return false;"{% endif %}
-    data-id="{{ page.pk }}"
-    data-node-id="{{ node.pk }}"
+    {% if is_popup %}onclick="opener.dismissRelatedLookupPopup(window, {{ page.id|unlocalize }}); return false;"{% endif %}
+    data-id="{{ page.pk|unlocalize }}"
+    data-node-id="{{ node.pk|unlocalize }}"
     data-slug="{{ page.get_slug }}"
     data-is-home="{{ page.is_home|yesno:"true,false" }}"
     data-move-permission="{{ has_move_page_permission|yesno:"true,false" }}"
@@ -152,7 +152,7 @@
             <div class="cms-tree-item cms-tree-item-menu{% if not has_change_permission %} cms-tree-item-disabled{% endif %} js-cms-tree-item-menu">
                 <div class="cms-tree-item-inner">
                     {% if has_change_permission %}
-                        <a href="{% url opts|admin_urlname:'change_innavigation' page.id %}?language={{ preview_language }}&amp;{{ page.id }}={{ page.in_navigation|yesno:"1,0" }}">
+                        <a href="{% url opts|admin_urlname:'change_innavigation' page.id %}?language={{ preview_language }}&amp;{{ page.id|unlocalize }}={{ page.in_navigation|yesno:"1,0" }}">
                     {% endif %}
                         {% if page.in_navigation %}
                             <div class="cms-hover-tooltip cms-hover-tooltip-left cms-hover-tooltip-delay"
@@ -198,7 +198,7 @@
                     <div{% if has_add_page_permission %} class="cms-hover-tooltip cms-hover-tooltip-left cms-hover-tooltip-delay"
                         data-cms-tooltip="{% autoescape on %}{% trans 'New sub page' %}{% endautoescape %}"{% endif %}>
                         {% if has_add_page_permission %}
-                            <a href="{% url opts|admin_urlname:'add' %}?parent_node={{ node.pk }}"
+                            <a href="{% url opts|admin_urlname:'add' %}?parent_node={{ node.pk|unlocalize }}"
                                 class="js-cms-pagetree-add-page cms-btn cms-btn-default cms-icon cms-icon-plus">
                         {% else %}
                             <span class="cms-btn cms-btn-default cms-btn-disabled cms-icon cms-icon-plus">
@@ -212,7 +212,7 @@
                     </div>
                 </div>
                 <div class="js-cms-pagetree-actions-dropdown cms-tree-item cms-tree-item-button cms-pagetree-dropdown js-cms-pagetree-dropdown" data-lazy-url="{% url opts|admin_urlname:'actions_menu' page.id %}">
-                    <a data-node-id="{{ node.pk }}" data-id="{{ page.pk }}" href="#" class="js-cms-pagetree-dropdown-trigger js-cms-pagetree-options cms-pagetree-dropdown-trigger cms-btn cms-btn-default cms-btn-no-border cms-icon cms-icon-menu">
+                    <a data-node-id="{{ node.pk|unlocalize }}" data-id="{{ page.pk|unlocalize }}" href="#" class="js-cms-pagetree-dropdown-trigger js-cms-pagetree-options cms-pagetree-dropdown-trigger cms-btn cms-btn-default cms-btn-no-border cms-icon cms-icon-menu">
                         <span class="sr-only">{% autoescape on %}{% trans "Options" %}{% endautoescape %}</span>
                     </a>
 
@@ -234,7 +234,7 @@
                                 </a>
                             </li>
                             <li>
-                                <a href="#" data-node-id="{{ node.pk }}" data-id="{{ page.pk }}" class="cms-pagetree-dropdown-item-disabled">
+                                <a href="#" data-node-id="{{ node.pk|unlocalize }}" data-id="{{ page.pk|unlocalize }}" class="cms-pagetree-dropdown-item-disabled">
                                     <span class="cms-icon cms-icon-alias"></span>
                                     <span>{% autoescape on %}{% trans "Paste" %}{% endautoescape %}</span>
                                 </a>

diff --git a/cms/templates/cms/toolbar/dragitem.html b/cms/templates/cms/toolbar/dragitem.html
@@ -47,7 +47,7 @@
             </div>
         {% endif %}
 
-        <span class="cms-dragitem-text" title="{{ plugin.plugin_type }} ID: {{ plugin.pk }}"><strong>{{ plugin.get_plugin_name }}</strong> {{ plugin.get_short_description }}</span>
+        <span class="cms-dragitem-text" title="{{ plugin.plugin_type }} ID: {{ plugin.pk|unlocalize }}"><strong>{{ plugin.get_plugin_name }}</strong> {{ plugin.get_short_description }}</span>
     </div>
 
     <div class="cms-collapsable-container cms-hidden

diff --git a/cms/tests/test_page_admin.py b/cms/tests/test_page_admin.py
@@ -1,7 +1,9 @@
 import datetime
 import json
 import sys
+from unittest import skipUnless
 
+from django.conf import settings
 from django.contrib import admin
 from django.contrib.sites.models import Site
 from django.core.cache import cache
@@ -1912,6 +1914,75 @@ def test_clear_placeholder_marks_page_as_dirty(self):
             self.assertEqual(page.reload().get_publisher_state("en"), PUBLISHER_STATE_DIRTY)
 
 
+    @skipUnless(
+        'sqlite' in settings.DATABASES.get('default').get('ENGINE').lower(),
+        'This test only works in SQLITE',
+    )
+    @override_settings(USE_THOUSAND_SEPARATOR=True, USE_L10N=True)
+    def test_page_tree_render_localized_page_ids(self):
+        from django.db import connection
+
+        # Artificially increment the sequence number on cms_page and cms_treenode (below)
+        # to be > 1000, to trigger a THOUSAND_SEPARATOR localization in the
+        # rendered template
+
+        admin_user = self.get_superuser()
+        root = create_page(
+            "home", "nav_playground.html", "fr", created_by=admin_user, published=True
+        )
+        with connection.cursor() as c:
+            c.execute('UPDATE SQLITE_SEQUENCE SET seq = 1001 WHERE name="cms_page"')
+            c.execute('UPDATE SQLITE_SEQUENCE SET seq = 1001 WHERE name="cms_treenode"')
+
+        page = create_page(
+            "child-page",
+            "nav_playground.html",
+            "fr",
+            created_by=admin_user,
+            published=True,
+            parent=root,
+            slug="child-page",
+        )
+
+        sub_page = create_page(
+            "grand-child-page",
+            "nav_playground.html",
+            "fr",
+            created_by=admin_user,
+            published=True,
+            parent=page,
+            slug="grand-child-page",
+        )
+        self.assertTrue(page.id > 1000)
+        self.assertTrue(sub_page.id > 1000)
+
+        self.assertTrue(page.node.id > 1000)
+        self.assertTrue(sub_page.node.id > 1000)
+
+        # make sure the rendered page tree doesn't
+        # localize page or node ids
+        with self.login_user_context(admin_user):
+            data = {'openNodes[]': [root.node.pk, page.node.pk], 'language': 'fr'}
+
+            endpoint = self.get_admin_url(Page, 'get_tree')
+            response = self.client.get(endpoint, data=data)
+
+            self.assertEqual(response.status_code, 200)
+            content = force_str(response.content)
+            self.assertFalse(f'parent_node={page.node.pk:,}"' in content)
+            self.assertTrue(f'parent_node={page.node.pk}"' in content)
+
+        # if per chance we have localized node ids in our localstorage,
+        # make sure DjangoCMS doesn't choke on them when they are passed
+        # into the view
+        with self.login_user_context(admin_user):
+            data = {'openNodes[]': [root.node.pk, f'{page.node.pk:,}'], 'language': 'fr'}
+            endpoint = self.get_admin_url(Page, 'get_tree')
+            response = self.client.get(endpoint, data=data)
+            self.assertEqual(response.status_code, 200)
+
+
+
 class PermissionsTestCase(PageTestBase):
 
     def _add_translation_to_page(self, page):