Skip to content

Commit e7c1bf4

Browse files
MacLakefsbraun
authored and
Github Release Action
committed
fix: Sitemap: Return a QuerySet in CMSSitemap.items() (#8031)
* fix: Sitemap: Don’t convert QuerySet to list in CMSSitemap.items() According to the Django docs a sequence like a list is allowed for the return value of CMSSitemap.items() but djangocms-page-sitemap applies exclude, which only works with querysets. In django CMS 3.11 it was a QuerySet. In django CMS 4 the return value changed to a list, though, which breaks djangocms-page-sitemap. * style: Format cms/tests/test_sitemap.py with ruff So the next commit becomes more readable. * test: Add test for type of CMSSitemap.items() --------- Co-authored-by: Fabian Braun <fsbraun@gmx.de>
1 parent 9f5c58d commit e7c1bf4

File tree

2 files changed

+64
-52
lines changed

2 files changed

+64
-52
lines changed

‎cms/sitemaps/cms_sitemap.py

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
1-
21
from django.contrib.sitemaps import Sitemap
3-
from django.db.models import OuterRef, Q, Subquery
2+
from django.db.models import OuterRef, Q, QuerySet, Subquery
43

54
from cms.models import PageContent, PageUrl
65
from cms.utils import get_current_site
@@ -12,15 +11,14 @@ def from_iterable(iterables):
1211
Backport of itertools.chain.from_iterable
1312
"""
1413
for it in iterables:
15-
for element in it:
16-
yield element
14+
yield from it
1715

1816

1917
class CMSSitemap(Sitemap):
20-
changefreq = "monthly"
21-
priority = 0.5
18+
changefreq: str = "monthly"
19+
priority: float = 0.5
2220

23-
def items(self):
21+
def items(self) -> QuerySet:
2422
#
2523
# It is counter-productive to provide entries for:
2624
# > Pages which redirect:
@@ -44,22 +42,25 @@ def items(self):
4442
# If, for some reason, you require redirecting pages (PageContent) to be
4543
# included, simply create a new class inheriting from this one, and
4644
# supply a new items() method which doesn't filter out the redirects.
45+
#
46+
# Even though items() can also return a sequence, we should return a
47+
# QuerySet in this case in order to be compatible with
48+
# djangocms-page-sitemap.
4749
site = get_current_site()
4850
languages = get_public_languages(site_id=site.pk)
4951

50-
return list(
51-
PageUrl
52-
.objects
53-
.get_for_site(site)
52+
return (
53+
PageUrl.objects.get_for_site(site)
5454
.filter(language__in=languages, path__isnull=False, page__login_required=False)
5555
.order_by('page__node__path')
5656
.select_related("page")
57-
.annotate(content_pk=Subquery(
58-
PageContent.objects
59-
.filter(page=OuterRef("page"), language=OuterRef("language"))
60-
.filter(Q(redirect="") | Q(redirect=None))
61-
.values_list("pk")[:1]
62-
))
57+
.annotate(
58+
content_pk=Subquery(
59+
PageContent.objects.filter(page=OuterRef("page"), language=OuterRef("language"))
60+
.filter(Q(redirect="") | Q(redirect=None))
61+
.values_list("pk")[:1]
62+
)
63+
)
6364
.filter(content_pk__isnull=False) # Remove page content with redirects
6465
)
6566

‎cms/tests/test_sitemap.py

Lines changed: 46 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import copy
22

3+
from django.db.models import QuerySet
4+
35
from cms.api import create_page, create_page_content
46
from cms.models import PageUrl
57
from cms.sitemaps import CMSSitemap
@@ -9,6 +11,7 @@
911

1012
protocol = "http" if DJANGO_4_2 else "https"
1113

14+
1215
class SitemapTestCase(CMSTestCase):
1316
def setUp(self):
1417
"""
@@ -27,41 +30,41 @@ def setUp(self):
2730
+ P8 (de, en)
2831
"""
2932
defaults = {
30-
'template': 'nav_playground.html',
31-
'language': 'en',
33+
"template": "nav_playground.html",
34+
"language": "en",
3235
}
3336
with self.settings(CMS_PERMISSION=False):
34-
p1 = create_page('P1', in_navigation=True, **defaults)
35-
create_page_content(language='de', title="other title %s" % p1.get_title('en'), page=p1)
37+
p1 = create_page("P1", in_navigation=True, **defaults)
38+
create_page_content(language="de", title="other title %s" % p1.get_title("en"), page=p1)
3639

37-
p4 = create_page('P4', in_navigation=True, **defaults)
38-
create_page_content(language='de', title="other title %s" % p4.get_title('en'), page=p4)
40+
p4 = create_page("P4", in_navigation=True, **defaults)
41+
create_page_content(language="de", title="other title %s" % p4.get_title("en"), page=p4)
3942

40-
p6 = create_page('P6', in_navigation=False, **defaults)
41-
create_page_content(language='de', title="other title %s" % p6.get_title('en'), page=p6)
43+
p6 = create_page("P6", in_navigation=False, **defaults)
44+
create_page_content(language="de", title="other title %s" % p6.get_title("en"), page=p6)
4245

43-
p2 = create_page('P2', in_navigation=True, parent=p1, **defaults)
44-
create_page_content(language='de', title="other title %s" % p2.get_title('en'), page=p2)
46+
p2 = create_page("P2", in_navigation=True, parent=p1, **defaults)
47+
create_page_content(language="de", title="other title %s" % p2.get_title("en"), page=p2)
4548

46-
p3 = create_page('P3', in_navigation=True, parent=p2, **defaults)
47-
create_page_content(language='de', title="other title %s" % p3.get_title('en'), page=p3)
49+
p3 = create_page("P3", in_navigation=True, parent=p2, **defaults)
50+
create_page_content(language="de", title="other title %s" % p3.get_title("en"), page=p3)
4851

49-
p5 = create_page('P5', in_navigation=True, parent=p4, **defaults)
50-
create_page_content(language='de', title="other title %s" % p5.get_title('en'), page=p5)
52+
p5 = create_page("P5", in_navigation=True, parent=p4, **defaults)
53+
create_page_content(language="de", title="other title %s" % p5.get_title("en"), page=p5)
5154

52-
p7 = create_page('P7', in_navigation=True, parent=p6, **defaults)
53-
create_page_content(language='de', title="other title %s" % p7.get_title('en'), page=p7)
55+
p7 = create_page("P7", in_navigation=True, parent=p6, **defaults)
56+
create_page_content(language="de", title="other title %s" % p7.get_title("en"), page=p7)
5457

55-
p8 = create_page('P8', in_navigation=True, parent=p6, **defaults)
56-
create_page_content(language='de', title="other title %s" % p8.get_title('en'), page=p8)
58+
p8 = create_page("P8", in_navigation=True, parent=p6, **defaults)
59+
create_page_content(language="de", title="other title %s" % p8.get_title("en"), page=p8)
5760

58-
p9 = create_page('P9', in_navigation=True, parent=p1, **defaults)
59-
create_page_content(language='de', title="other title %s" % p9.get_title('en'), page=p9)
61+
p9 = create_page("P9", in_navigation=True, parent=p1, **defaults)
62+
create_page_content(language="de", title="other title %s" % p9.get_title("en"), page=p9)
6063

61-
p10 = create_page('P10', in_navigation=True, parent=p9, **defaults)
62-
create_page_content(language='de', title="other title %s" % p10.get_title('en'), page=p10)
64+
p10 = create_page("P10", in_navigation=True, parent=p9, **defaults)
65+
create_page_content(language="de", title="other title %s" % p10.get_title("en"), page=p10)
6366

64-
create_page('P11', in_navigation=True, parent=p9, **defaults)
67+
create_page("P11", in_navigation=True, parent=p9, **defaults)
6568

6669
def test_sitemap_count(self):
6770
"""
@@ -79,11 +82,11 @@ def test_sitemap_items_location(self):
7982
sitemap = CMSSitemap()
8083
urlset = sitemap.get_urls()
8184
for item in urlset:
82-
if item['item'].path:
85+
if item["item"].path:
8386
url = f'{protocol}://example.com/{item["item"].language}/{item["item"].path}/'
8487
else:
8588
url = f'{protocol}://example.com/{item["item"].language}/'
86-
self.assertEqual(item['location'], url)
89+
self.assertEqual(item["location"], url)
8790

8891
def test_sitemap_urls(self):
8992
"""
@@ -93,28 +96,36 @@ def test_sitemap_urls(self):
9396
locations = []
9497
urlset = sitemap.get_urls()
9598
for item in urlset:
96-
locations.append(item['location'])
99+
locations.append(item["location"])
97100
for page_url in PageUrl.objects.all():
98101
if page_url.path:
99-
url = f'{protocol}://example.com/{page_url.language}/{page_url.path}/'
102+
url = f"{protocol}://example.com/{page_url.language}/{page_url.path}/"
100103
else:
101-
url = f'{protocol}://example.com/{page_url.language}/'
104+
url = f"{protocol}://example.com/{page_url.language}/"
102105
self.assertTrue(url in locations)
103106

104107
def test_sitemap_uses_public_languages_only(self):
105108
"""
106109
Pages on the sitemap should only show public languages.
107110
"""
108-
lang_settings = copy.deepcopy(get_cms_setting('LANGUAGES'))
111+
lang_settings = copy.deepcopy(get_cms_setting("LANGUAGES"))
109112
# sanity check
110-
assert lang_settings[1][1]['code'] == 'de'
113+
assert lang_settings[1][1]["code"] == "de"
111114
# set german as private
112-
lang_settings[1][1]['public'] = False
115+
lang_settings[1][1]["public"] = False
113116

114117
with self.settings(CMS_LANGUAGES=lang_settings):
115118
for item in CMSSitemap().get_urls():
116-
url = f'{protocol}://example.com/en/'
119+
url = f"{protocol}://example.com/en/"
120+
121+
if item["item"].path:
122+
url += item["item"].path + "/"
123+
self.assertEqual(item["location"], url)
117124

118-
if item['item'].path:
119-
url += item['item'].path + '/'
120-
self.assertEqual(item['location'], url)
125+
def test_sitemap_items_type(self):
126+
"""
127+
Ensure that CMSSitemap.items() returns a QuerySet, not a list.
128+
"""
129+
sitemap = CMSSitemap()
130+
items = sitemap.items()
131+
self.assertIsInstance(items, QuerySet, "CMSSitemap.items() must return a QuerySet.")

0 commit comments

Comments
 (0)