From b0f59bb55b57f2838f68f0d061570af56f13e0e6 Mon Sep 17 00:00:00 2001 From: Fabian Braun Date: Thu, 29 Feb 2024 08:12:56 +0100 Subject: [PATCH] fix: Efficient build menu for versioned and unversioned pages (#7807) * Update cms menus to allow use cms 4.1 features * Fix: Menus are only cached for preview and live urls (not edit) * Update tests * fix tests * Fix fallback * Fix get live url test * Fix surprising lint error in cms.models.pagemodel * Small refactor * Wording improvement * Fix: Remove redundant calculation of absolute URL --- cms/cms_menus.py | 116 ++++++++++++++++++++-------------------- cms/models/pagemodel.py | 2 +- cms/tests/test_menu.py | 2 +- cms/toolbar/utils.py | 10 ++-- menus/menu_pool.py | 4 +- 5 files changed, 68 insertions(+), 66 deletions(-) diff --git a/cms/cms_menus.py b/cms/cms_menus.py index fbe08d1f66..5d9b3b22b4 100644 --- a/cms/cms_menus.py +++ b/cms/cms_menus.py @@ -8,6 +8,7 @@ from cms import constants from cms.apphook_pool import apphook_pool from cms.models import EmptyPageContent, PageContent, PageUrl +from cms.toolbar.utils import get_object_preview_url, get_toolbar_from_request from cms.utils.conf import get_cms_setting from cms.utils.i18n import ( get_fallback_languages, @@ -28,8 +29,8 @@ def get_visible_nodes(request, pages, site): `pages` contains all published pages. """ user = request.user - public_for = get_cms_setting('PUBLIC_FOR') - can_see_unrestricted = public_for == 'all' or (public_for == 'staff' and user.is_staff) + public_for = get_cms_setting("PUBLIC_FOR") + can_see_unrestricted = public_for == "all" or (public_for == "staff" and user.is_staff) if not user.is_authenticated and not can_see_unrestricted: # User is not authenticated and can't see unrestricted pages, @@ -48,7 +49,7 @@ def get_visible_nodes(request, pages, site): return list(pages) if can_see_unrestricted else [] user_id = user.pk - user_groups = SimpleLazyObject(lambda: frozenset(user.groups.values_list('pk', flat=True))) + user_groups = SimpleLazyObject(lambda: frozenset(user.groups.values_list("pk", flat=True))) is_auth_user = user.is_authenticated def user_can_see_page(page): @@ -69,7 +70,7 @@ def user_can_see_page(page): return [page for page in pages if user_can_see_page(page)] -def get_menu_node_for_page(renderer, page, language, fallbacks=None): +def get_menu_node_for_page(renderer, page, language, fallbacks=None, endpoint=False): """ Transform a CMS page into a navigation node. @@ -78,7 +79,7 @@ def get_menu_node_for_page(renderer, page, language, fallbacks=None): page: The page to transform. language: The current language used to render the menu. fallbacks: List of fallback languages (optional). - + url: The URL to use for the node (optional) instead of page_content.get_absolute_url(). Returns: A CMSNavigationNode instance. """ @@ -88,21 +89,21 @@ def get_menu_node_for_page(renderer, page, language, fallbacks=None): # These are simple to port over, since they are not calculated. # Other attributes will be added conditionally later. attr = { - 'is_page': True, - 'soft_root': page.get_soft_root(language), - 'auth_required': page.login_required, - 'reverse_id': page.reverse_id, + "is_page": True, + "soft_root": page.get_soft_root(language), + "auth_required": page.login_required, + "reverse_id": page.reverse_id, } limit_visibility_in_menu = page.get_limit_visibility_in_menu(language) if limit_visibility_in_menu is constants.VISIBILITY_ALL: - attr['visible_for_authenticated'] = True - attr['visible_for_anonymous'] = True + attr["visible_for_authenticated"] = True + attr["visible_for_anonymous"] = True else: - attr['visible_for_authenticated'] = limit_visibility_in_menu == constants.VISIBILITY_USERS - attr['visible_for_anonymous'] = limit_visibility_in_menu == constants.VISIBILITY_ANONYMOUS - attr['is_home'] = page.is_home + attr["visible_for_authenticated"] = limit_visibility_in_menu == constants.VISIBILITY_USERS + attr["visible_for_anonymous"] = limit_visibility_in_menu == constants.VISIBILITY_ANONYMOUS + attr["is_home"] = page.is_home # Extenders can be either navigation extenders or from apphooks. extenders = [] if page.navigation_extenders: @@ -126,26 +127,30 @@ def get_menu_node_for_page(renderer, page, language, fallbacks=None): # CMSAttachMenus are treated a bit differently to allow them to be # able to be attached to multiple points in the navigation. exts.append(f"{ext.__name__}:{page.pk}") - elif hasattr(ext, '__name__'): + elif hasattr(ext, "__name__"): exts.append(ext.__name__) else: exts.append(ext) if exts: - attr['navigation_extenders'] = exts + attr["navigation_extenders"] = exts for lang in [language] + fallbacks: - translation = page.page_content_cache[lang] + translation = page.page_content_cache.get(lang) if translation: page_url = page.urls_cache[lang] # Do we have a redirectURL? - attr['redirect_url'] = translation.redirect # save redirect URL if any + attr["redirect_url"] = translation.redirect # save redirect URL if any # Now finally, build the NavigationNode object and return it. # The parent_id is manually set by the menu get_nodes method. + if endpoint: + url = get_object_preview_url(translation) + else: + url = translation.get_absolute_url() ret_node = CMSNavigationNode( title=translation.menu_title or translation.title, - url='', + url=url, id=page.pk, attr=attr, visible=page.get_in_navigation(translation.language), @@ -153,8 +158,7 @@ def get_menu_node_for_page(renderer, page, language, fallbacks=None): language=(translation.language if translation.language != language else None), ) return ret_node - else: - raise RuntimeError('Unable to render cms menu. There is a language misconfiguration.') + return None class CMSNavigationNode(NavigationNode): @@ -188,25 +192,17 @@ def is_selected(self, request): return False return page_id == self.id - def _get_absolute_url(self): - if self.attr['is_home']: - return reverse('pages-root') - return reverse('pages-details-by-slug', kwargs={"slug": self.path}) - - def get_absolute_url(self): - if self.language: - with force_language(self.language): - return self._get_absolute_url() - return self._get_absolute_url() - class CMSMenu(Menu): """Subclass of :class:`menus.base.Menu`. Its :meth:`~menus.base.Menu.get_nodes()` creates a list of NavigationNodes based on a site's :class:`cms.models.pagemodel.Page` objects. """ + def get_nodes(self, request): site = self.renderer.site lang = self.renderer.request_language + toolbar = get_toolbar_from_request(request) + pages = get_page_queryset(site) if is_valid_site_language(lang, site_id=site.pk): @@ -231,10 +227,9 @@ def get_nodes(self, request): fallbacks = languages pages = ( - pages - .filter(pagecontent_set__language__in=languages) - .select_related('node') - .order_by('node__path') + pages.filter(pagecontent_set__language__in=languages) + .select_related("node") + .order_by("node__path") .distinct() ) pages = get_visible_nodes(request, pages, site) @@ -248,22 +243,25 @@ def get_nodes(self, request): homepage = None urls_lookup = Prefetch( - 'urls', - to_attr='filtered_urls', + "urls", + to_attr="filtered_urls", queryset=PageUrl.objects.filter(language__in=languages), ) + if toolbar.edit_mode_active or toolbar.preview_mode_active: + # Get all translations visible in the admin for the current page + translations_qs = PageContent.admin_manager.current_content(language__in=languages) + else: + # Only get public translations + translations_qs = PageContent.objects.filter(language__in=languages) translations_lookup = Prefetch( - 'pagecontent_set', - to_attr='filtered_translations', - queryset=PageContent.objects.filter(language__in=languages), + "pagecontent_set", + to_attr="filtered_translations", + queryset=translations_qs, ) prefetch_related_objects(pages, urls_lookup, translations_lookup) # Build the blank title instances only once blank_page_content_cache = {language: EmptyPageContent(language=language) for language in languages} - if lang not in blank_page_content_cache: - blank_page_content_cache[lang] = EmptyPageContent(language=lang) - # Maps a node id to its page id node_id_to_page = {} @@ -283,6 +281,7 @@ def _page_to_node(page): page, language=lang, fallbacks=fallbacks, + endpoint=toolbar.preview_mode_active or toolbar.edit_mode_active, ) return menu_node @@ -298,16 +297,18 @@ def _page_to_node(page): continue menu_node = _page_to_node(page) - cut_homepage = homepage and not homepage.get_in_navigation(lang) - - if cut_homepage and parent_id == homepage.pk: - # When the homepage is hidden from navigation, - # we need to cut all its direct children from it. - menu_node.parent_id = None - else: - menu_node.parent_id = parent_id - node_id_to_page[node.pk] = page.pk - menu_nodes.append(menu_node) + if menu_node: + # Only add pages with at least one page content + cut_homepage = homepage and not homepage.get_in_navigation(lang) + + if cut_homepage and parent_id == homepage.pk: + # When the homepage is hidden from navigation, + # we need to cut all its direct children from it. + menu_node.parent_id = None + else: + menu_node.parent_id = parent_id + node_id_to_page[node.pk] = page.pk + menu_nodes.append(menu_node) return menu_nodes @@ -315,7 +316,6 @@ def _page_to_node(page): class NavExtender(Modifier): - def modify(self, request, nodes, namespace, root_id, post_cut, breadcrumb): if post_cut: return nodes @@ -347,8 +347,7 @@ def modify(self, request, nodes, namespace, root_id, post_cut, breadcrumb): # find all not assigned nodes for menu in self.renderer.menus.items(): - if (hasattr(menu[1], 'cms_enabled') - and menu[1].cms_enabled and menu[0] not in exts): + if hasattr(menu[1], "cms_enabled") and menu[1].cms_enabled and menu[0] not in exts: for node in nodes: if node.namespace == menu[0]: removed.append(node) @@ -484,8 +483,7 @@ def find_ancestors_and_remove_children(self, node, nodes): node.parent.parent = None nodes = [node.parent] + nodes else: - nodes = self.find_ancestors_and_remove_children( - node.parent, nodes) + nodes = self.find_ancestors_and_remove_children(node.parent, nodes) else: for newnode in nodes: if newnode != node and not newnode.parent: diff --git a/cms/models/pagemodel.py b/cms/models/pagemodel.py index 54f0a944bd..e0c5592090 100644 --- a/cms/models/pagemodel.py +++ b/cms/models/pagemodel.py @@ -363,7 +363,7 @@ def get_absolute_url(self, language=None, fallback=True): if self.is_home: return reverse('pages-root') path = self.get_path(language, fallback) or self.get_slug(language, fallback) # TODO: Disallow get_slug - return reverse('pages-details-by-slug', kwargs={"slug": path}) + return reverse('pages-details-by-slug', kwargs={"slug": path}) if path else None def set_tree_node(self, site, target=None, position='first-child'): assert position in ('last-child', 'first-child', 'left', 'right') diff --git a/cms/tests/test_menu.py b/cms/tests/test_menu.py index c038236f05..410f58857a 100644 --- a/cms/tests/test_menu.py +++ b/cms/tests/test_menu.py @@ -425,7 +425,7 @@ def test_show_menu_num_queries(self): The queries should be: get all page nodes get all page permissions - get all titles + get all page contents get all page urls get the menu cache key set the menu cache key diff --git a/cms/toolbar/utils.py b/cms/toolbar/utils.py index 94baea34fb..b1be7f77e8 100644 --- a/cms/toolbar/utils.py +++ b/cms/toolbar/utils.py @@ -5,6 +5,7 @@ from django.apps import apps from django.contrib.contenttypes.models import ContentType from django.db import models +from django.urls import NoReverseMatch from django.utils.encoding import force_str from django.utils.translation import ( get_language, @@ -126,7 +127,7 @@ def get_toolbar_from_request(request): def add_live_url_querystring_param(obj, url, language=None): """ - Append a live url to a given Page url using a supplied url parameter configured + Append a live url to a given object url using a supplied url parameter configured by the setting: CMS_ENDPOINT_LIVE_URL_QUERYSTRING_PARAM :param obj: Placeholder source object @@ -135,9 +136,12 @@ def add_live_url_querystring_param(obj, url, language=None): :returns: A url string """ url_param = get_cms_setting('ENDPOINT_LIVE_URL_QUERYSTRING_PARAM') - if not isinstance(obj, PageContent): + if not hasattr(obj, "get_absolute_url"): + return url + try: + live_url = obj.get_absolute_url() + except NoReverseMatch: return url - live_url = obj.page.get_absolute_url(language=language) url_fragments = url.split('?') if len(url_fragments) > 1: url += f'&{url_param}={live_url}' diff --git a/menus/menu_pool.py b/menus/menu_pool.py index 8021ff6b77..417fe1b0f8 100644 --- a/menus/menu_pool.py +++ b/menus/menu_pool.py @@ -310,9 +310,9 @@ def get_registered_modifiers(self): return self.modifiers def clear(self, site_id=None, language=None, all=False): - ''' + """ This invalidates the cache for a given menu (site_id and language) - ''' + """ if all: cache_keys = CacheKey.objects.get_keys() else: