From 62ce7028af8ba3ebab2ecf347ca54fab2e6641b4 Mon Sep 17 00:00:00 2001 From: Filip Weidemann Date: Tue, 29 Oct 2024 08:44:24 +0100 Subject: [PATCH 01/11] fix: setting partial admin_content_cache was responsible for rendering create forms inside the admin language tabs --- cms/admin/pageadmin.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/cms/admin/pageadmin.py b/cms/admin/pageadmin.py index ab590247cee..80b0a630923 100644 --- a/cms/admin/pageadmin.py +++ b/cms/admin/pageadmin.py @@ -740,11 +740,7 @@ def get_object(self, request, object_id, from_field=None): key is used if no field is provided. Return ``None`` if no match is found or the object_id fails validation. """ - obj = super().get_object(request, object_id, from_field) - - if obj: - obj.page.admin_content_cache[obj.language] = obj - return obj + return super().get_object(request, object_id, from_field) def get_admin_url(self, action, *args): url_name = f"{self.opts.app_label}_{self.opts.model_name}_{action}" From 62e65809b997effb058282215492acf48f273369 Mon Sep 17 00:00:00 2001 From: Fabian Braun Date: Tue, 29 Oct 2024 09:43:42 +0100 Subject: [PATCH 02/11] Update pagemodel.py to also include latest versions in language tabs if they are unpublished or archived --- cms/models/pagemodel.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/models/pagemodel.py b/cms/models/pagemodel.py index 12428b7d93e..d63337e818e 100644 --- a/cms/models/pagemodel.py +++ b/cms/models/pagemodel.py @@ -722,7 +722,7 @@ def set_translations_cache(self): self.page_content_cache.setdefault(translation.language, translation) def set_admin_content_cache(self): - for translation in self.pagecontent_set(manager="admin_manager").current_content().all(): + for translation in self.pagecontent_set(manager="admin_manager").latest_content().all(): self.admin_content_cache.setdefault(translation.language, translation) def get_admin_content(self, language, fallback=False): From 1ec8b216348ee3e7dfe98da141fdf80b6dc54393 Mon Sep 17 00:00:00 2001 From: Filip Weidemann Date: Tue, 29 Oct 2024 10:10:59 +0100 Subject: [PATCH 03/11] fix: remove get_object on PageContentAdmin --- cms/admin/pageadmin.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/cms/admin/pageadmin.py b/cms/admin/pageadmin.py index 80b0a630923..d7460bf5526 100644 --- a/cms/admin/pageadmin.py +++ b/cms/admin/pageadmin.py @@ -734,14 +734,6 @@ def log_change(self, request, object, message): # Block the admin log for change. A signal takes care of this! return - def get_object(self, request, object_id, from_field=None): - """ - Return an instance matching the field and value provided, the primary - key is used if no field is provided. Return ``None`` if no match is - found or the object_id fails validation. - """ - return super().get_object(request, object_id, from_field) - def get_admin_url(self, action, *args): url_name = f"{self.opts.app_label}_{self.opts.model_name}_{action}" return admin_reverse(url_name, args=args) From 0ed79dd0adae0c1ed6a91a0ebbd5865b938261f5 Mon Sep 17 00:00:00 2001 From: Fabian Braun Date: Tue, 29 Oct 2024 17:06:11 +0100 Subject: [PATCH 04/11] chore: Introduce AdminCacheDict --- cms/models/pagemodel.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/cms/models/pagemodel.py b/cms/models/pagemodel.py index d63337e818e..c0a0316a7ca 100644 --- a/cms/models/pagemodel.py +++ b/cms/models/pagemodel.py @@ -28,6 +28,12 @@ logger = getLogger(__name__) +class AdminCacheDict(dict): + """Dictionary that disallows setting individual items to prevent accidental cache corruption.""" + def __setitem__(self, key, value): + raise ValueError("Do not set individual items in the admin cache dict. Use the clear_cache method instead.") + + class Page(MP_Node): """ A ``Page`` is the basic unit of site structure in django CMS. The CMS uses a hierarchical page model: each page @@ -139,7 +145,7 @@ def __init__(self, *args, **kwargs): self.urls_cache = {} self.page_content_cache = {} #: Internal cache for page content objects visible publicly - self.admin_content_cache = {} + self.admin_content_cache = AdminCacheDict() #: Internal cache for page content objects visible in the admin (i.e. to staff users.) #: Might be larger than the page_content_cache @@ -203,7 +209,7 @@ def get_cached_descendants(self): def _clear_internal_cache(self): self.urls_cache = {} self.page_content_cache = {} - self.admin_content_cache = {} + self.admin_content_cache = AdminCacheDict() if hasattr(self, '_prefetched_objects_cache'): del self._prefetched_objects_cache @@ -722,6 +728,7 @@ def set_translations_cache(self): self.page_content_cache.setdefault(translation.language, translation) def set_admin_content_cache(self): + self.admin_conent_cache = AdminCacheDict() for translation in self.pagecontent_set(manager="admin_manager").latest_content().all(): self.admin_content_cache.setdefault(translation.language, translation) From aefad054717e3a5edb28e7eb4a6a5217a4f7be3e Mon Sep 17 00:00:00 2001 From: Filip Weidemann Date: Tue, 29 Oct 2024 21:40:35 +0100 Subject: [PATCH 05/11] chore: add tests for the GetAdminUrlForLanguage template tag --- cms/tests/test_templatetags.py | 44 +++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/cms/tests/test_templatetags.py b/cms/tests/test_templatetags.py index f2f2806b102..b3f61a30630 100644 --- a/cms/tests/test_templatetags.py +++ b/cms/tests/test_templatetags.py @@ -27,7 +27,7 @@ PageUrl, Placeholder, ) -from cms.templatetags.cms_admin import GetPreviewUrl, get_page_display_name +from cms.templatetags.cms_admin import GetAdminUrlForLanguage, GetPreviewUrl, get_page_display_name from cms.templatetags.cms_js_tags import json_filter from cms.templatetags.cms_tags import ( _get_page_by_untyped_arg, @@ -43,6 +43,48 @@ class TemplatetagTests(CMSTestCase): + def test_get_admin_url_for_language(self): + # This creates a new page and the corresponding PageContent object for lang 'en', both with pk=1 + page = create_page("AdminURLTestPage English Content", "nav_playground.html", "en") + + # German PageContent will have pk=2 + german_content = create_page_content("de", "AdminURLTestPage German Content", page) + + + request = RequestFactory().get('/') + request.current_page = page + with force_language('de'): + url = GetAdminUrlForLanguage.get_value(None, context={'request': request}, page=page, language="de") + # We expect that getting the German URL will return the German page content (change link, not a create link) + self.assertEqual(url, '/de/admin/cms/pagecontent/2/change/') + + with force_language('en'): + url = GetAdminUrlForLanguage.get_value(None, context={'request': request}, page=page, language="en") + # Same goes for the English URL + self.assertEqual(url, '/en/admin/cms/pagecontent/1/change/') + + with force_language('fr'): + url = GetAdminUrlForLanguage.get_value(None, context={'request': request}, page=page, language="fr") + # PageContent for the French languag does not exist, we expect a link to create a new PageContent instance + self.assertEqual(url, '/fr/admin/cms/pagecontent/add/?cms_page=1&language=fr') + + def test_admin_pagecontent_language_tab_urls(self): + # Same setup as in the templatetag test above + page = create_page('Test', 'nav_playground.html', 'en') + german_content = create_page_content("de", "AdminURLTestPage German Content", page) + + request = RequestFactory().get('/') + request.current_page = page + template = """ + {% load cms_tags cms_admin %} + {% get_admin_url_for_language page_obj 'en' %} + {% get_admin_url_for_language page_obj 'de' %} + {% get_admin_url_for_language page_obj 'fr' %} + """ + output = self.render_template_obj(template, {'page_obj': page}, request) + self.assertIn('/en/admin/cms/pagecontent/1/change/', output) + self.assertIn('/en/admin/cms/pagecontent/2/change/', output) + self.assertIn('/en/admin/cms/pagecontent/add/?cms_page=1&language=fr', output) def test_get_preview_url(self): """The get_preview_url template tag returns the content preview url for its language: From 456cb2bea847b57d8c7c824f6c6799fc58058989 Mon Sep 17 00:00:00 2001 From: Filip Weidemann Date: Tue, 29 Oct 2024 22:02:46 +0100 Subject: [PATCH 06/11] chore: don't assume primary keys in templatetag tests --- cms/tests/test_templatetags.py | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/cms/tests/test_templatetags.py b/cms/tests/test_templatetags.py index b3f61a30630..a2d00c631a9 100644 --- a/cms/tests/test_templatetags.py +++ b/cms/tests/test_templatetags.py @@ -44,33 +44,35 @@ class TemplatetagTests(CMSTestCase): def test_get_admin_url_for_language(self): - # This creates a new page and the corresponding PageContent object for lang 'en', both with pk=1 + # This creates a new page and the corresponding PageContent object for lang 'en' + # After that we create a German PageContent object page = create_page("AdminURLTestPage English Content", "nav_playground.html", "en") - - # German PageContent will have pk=2 + english_content = page.pagecontent_set(manager="admin_manager").first() german_content = create_page_content("de", "AdminURLTestPage German Content", page) request = RequestFactory().get('/') request.current_page = page + with force_language('de'): url = GetAdminUrlForLanguage.get_value(None, context={'request': request}, page=page, language="de") # We expect that getting the German URL will return the German page content (change link, not a create link) - self.assertEqual(url, '/de/admin/cms/pagecontent/2/change/') + self.assertEqual(url, f'/de/admin/cms/pagecontent/{german_content.pk}/change/') with force_language('en'): url = GetAdminUrlForLanguage.get_value(None, context={'request': request}, page=page, language="en") # Same goes for the English URL - self.assertEqual(url, '/en/admin/cms/pagecontent/1/change/') + self.assertEqual(url, f'/en/admin/cms/pagecontent/{english_content.pk}/change/') with force_language('fr'): url = GetAdminUrlForLanguage.get_value(None, context={'request': request}, page=page, language="fr") # PageContent for the French languag does not exist, we expect a link to create a new PageContent instance - self.assertEqual(url, '/fr/admin/cms/pagecontent/add/?cms_page=1&language=fr') + self.assertEqual(url, f'/fr/admin/cms/pagecontent/add/?cms_page={page.pk}&language=fr') def test_admin_pagecontent_language_tab_urls(self): # Same setup as in the templatetag test above - page = create_page('Test', 'nav_playground.html', 'en') + page = create_page('AdminURLTestPage English Content', 'nav_playground.html', 'en') + english_content = page.pagecontent_set(manager="admin_manager").first() german_content = create_page_content("de", "AdminURLTestPage German Content", page) request = RequestFactory().get('/') @@ -82,9 +84,9 @@ def test_admin_pagecontent_language_tab_urls(self): {% get_admin_url_for_language page_obj 'fr' %} """ output = self.render_template_obj(template, {'page_obj': page}, request) - self.assertIn('/en/admin/cms/pagecontent/1/change/', output) - self.assertIn('/en/admin/cms/pagecontent/2/change/', output) - self.assertIn('/en/admin/cms/pagecontent/add/?cms_page=1&language=fr', output) + self.assertIn(f'/en/admin/cms/pagecontent/{english_content.pk}/change/', output) + self.assertIn(f'/en/admin/cms/pagecontent/{german_content.pk}/change/', output) + self.assertIn(f'/en/admin/cms/pagecontent/add/?cms_page={page.pk}&language=fr', output) def test_get_preview_url(self): """The get_preview_url template tag returns the content preview url for its language: From 91b90e2c475b707f5920473b15d61ae568a127c5 Mon Sep 17 00:00:00 2001 From: Filip Weidemann Date: Tue, 29 Oct 2024 22:06:55 +0100 Subject: [PATCH 07/11] chore: fix spelling problem --- cms/tests/test_templatetags.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/tests/test_templatetags.py b/cms/tests/test_templatetags.py index a2d00c631a9..c11a9815af2 100644 --- a/cms/tests/test_templatetags.py +++ b/cms/tests/test_templatetags.py @@ -66,7 +66,7 @@ def test_get_admin_url_for_language(self): with force_language('fr'): url = GetAdminUrlForLanguage.get_value(None, context={'request': request}, page=page, language="fr") - # PageContent for the French languag does not exist, we expect a link to create a new PageContent instance + # PageContent for the French language does not exist, we expect a link to create a new PageContent instance self.assertEqual(url, f'/fr/admin/cms/pagecontent/add/?cms_page={page.pk}&language=fr') def test_admin_pagecontent_language_tab_urls(self): From 08fb99e7304457aec336da5c223e5241acd42d08 Mon Sep 17 00:00:00 2001 From: Filip Weidemann Date: Tue, 29 Oct 2024 22:27:08 +0100 Subject: [PATCH 08/11] chore: restructure GetAdminUrlForLanguage tests, add one that tests partial cache presence --- cms/tests/test_templatetags.py | 40 +++++++++++++++------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/cms/tests/test_templatetags.py b/cms/tests/test_templatetags.py index c11a9815af2..36ac9e2815f 100644 --- a/cms/tests/test_templatetags.py +++ b/cms/tests/test_templatetags.py @@ -43,38 +43,34 @@ class TemplatetagTests(CMSTestCase): - def test_get_admin_url_for_language(self): - # This creates a new page and the corresponding PageContent object for lang 'en' - # After that we create a German PageContent object - page = create_page("AdminURLTestPage English Content", "nav_playground.html", "en") + def test_admin_pagecontent_language_tab_urls(self): + # Same setup as in the templatetag test above + page = create_page('AdminURLTestPage English Content', 'nav_playground.html', 'en') english_content = page.pagecontent_set(manager="admin_manager").first() german_content = create_page_content("de", "AdminURLTestPage German Content", page) - request = RequestFactory().get('/') request.current_page = page + template = """ + {% load cms_tags cms_admin %} + {% get_admin_url_for_language page_obj 'en' %} + {% get_admin_url_for_language page_obj 'de' %} + {% get_admin_url_for_language page_obj 'fr' %} + """ + output = self.render_template_obj(template, {'page_obj': page}, request) + self.assertIn(f'/en/admin/cms/pagecontent/{english_content.pk}/change/', output) + self.assertIn(f'/en/admin/cms/pagecontent/{german_content.pk}/change/', output) + self.assertIn(f'/en/admin/cms/pagecontent/add/?cms_page={page.pk}&language=fr', output) - with force_language('de'): - url = GetAdminUrlForLanguage.get_value(None, context={'request': request}, page=page, language="de") - # We expect that getting the German URL will return the German page content (change link, not a create link) - self.assertEqual(url, f'/de/admin/cms/pagecontent/{german_content.pk}/change/') - - with force_language('en'): - url = GetAdminUrlForLanguage.get_value(None, context={'request': request}, page=page, language="en") - # Same goes for the English URL - self.assertEqual(url, f'/en/admin/cms/pagecontent/{english_content.pk}/change/') - - with force_language('fr'): - url = GetAdminUrlForLanguage.get_value(None, context={'request': request}, page=page, language="fr") - # PageContent for the French language does not exist, we expect a link to create a new PageContent instance - self.assertEqual(url, f'/fr/admin/cms/pagecontent/add/?cms_page={page.pk}&language=fr') - - def test_admin_pagecontent_language_tab_urls(self): - # Same setup as in the templatetag test above + def test_admin_pagecontent_language_tab_urls_accessed_page(self): page = create_page('AdminURLTestPage English Content', 'nav_playground.html', 'en') english_content = page.pagecontent_set(manager="admin_manager").first() german_content = create_page_content("de", "AdminURLTestPage German Content", page) + # Try to fill the cache with partial data (this should not be possible) + page.get_content_obj(language='en') + page.get_admin_content(language='en') + request = RequestFactory().get('/') request.current_page = page template = """ From dec8efc57573f13aa7e4d44a04160d11f0f5a8a4 Mon Sep 17 00:00:00 2001 From: Filip Weidemann Date: Tue, 29 Oct 2024 22:30:21 +0100 Subject: [PATCH 09/11] chore: merge GetAdminUrlForLanguage template tag tests into one --- cms/tests/test_templatetags.py | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/cms/tests/test_templatetags.py b/cms/tests/test_templatetags.py index 36ac9e2815f..966558f677a 100644 --- a/cms/tests/test_templatetags.py +++ b/cms/tests/test_templatetags.py @@ -49,24 +49,6 @@ def test_admin_pagecontent_language_tab_urls(self): english_content = page.pagecontent_set(manager="admin_manager").first() german_content = create_page_content("de", "AdminURLTestPage German Content", page) - request = RequestFactory().get('/') - request.current_page = page - template = """ - {% load cms_tags cms_admin %} - {% get_admin_url_for_language page_obj 'en' %} - {% get_admin_url_for_language page_obj 'de' %} - {% get_admin_url_for_language page_obj 'fr' %} - """ - output = self.render_template_obj(template, {'page_obj': page}, request) - self.assertIn(f'/en/admin/cms/pagecontent/{english_content.pk}/change/', output) - self.assertIn(f'/en/admin/cms/pagecontent/{german_content.pk}/change/', output) - self.assertIn(f'/en/admin/cms/pagecontent/add/?cms_page={page.pk}&language=fr', output) - - def test_admin_pagecontent_language_tab_urls_accessed_page(self): - page = create_page('AdminURLTestPage English Content', 'nav_playground.html', 'en') - english_content = page.pagecontent_set(manager="admin_manager").first() - german_content = create_page_content("de", "AdminURLTestPage German Content", page) - # Try to fill the cache with partial data (this should not be possible) page.get_content_obj(language='en') page.get_admin_content(language='en') From 319a94eb49f5c64137c84907e5fe47e77aa17f1c Mon Sep 17 00:00:00 2001 From: Filip Weidemann Date: Tue, 29 Oct 2024 22:36:14 +0100 Subject: [PATCH 10/11] Update cms/tests/test_templatetags.py Co-authored-by: Fabian Braun --- cms/tests/test_templatetags.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/tests/test_templatetags.py b/cms/tests/test_templatetags.py index 966558f677a..c23e226c062 100644 --- a/cms/tests/test_templatetags.py +++ b/cms/tests/test_templatetags.py @@ -50,7 +50,7 @@ def test_admin_pagecontent_language_tab_urls(self): german_content = create_page_content("de", "AdminURLTestPage German Content", page) # Try to fill the cache with partial data (this should not be possible) - page.get_content_obj(language='en') + page.get_content_obj(language='en') # should not affect admin template tag page.get_admin_content(language='en') request = RequestFactory().get('/') From b95e4708edcbc64597c783a4564beecc30cd1166 Mon Sep 17 00:00:00 2001 From: Filip Weidemann Date: Tue, 29 Oct 2024 22:36:24 +0100 Subject: [PATCH 11/11] Update cms/tests/test_templatetags.py Co-authored-by: Fabian Braun --- cms/tests/test_templatetags.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/tests/test_templatetags.py b/cms/tests/test_templatetags.py index c23e226c062..60e4bd7fe8a 100644 --- a/cms/tests/test_templatetags.py +++ b/cms/tests/test_templatetags.py @@ -51,7 +51,7 @@ def test_admin_pagecontent_language_tab_urls(self): # Try to fill the cache with partial data (this should not be possible) page.get_content_obj(language='en') # should not affect admin template tag - page.get_admin_content(language='en') + page.get_admin_content(language='en') # Should fill the whole cache request = RequestFactory().get('/') request.current_page = page