Skip to content

fix: Sitemap: Return a QuerySet in CMSSitemap.items() #8031

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 4 commits into from
Oct 16, 2024
Merged

fix: Sitemap: Return a QuerySet in CMSSitemap.items() #8031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
339aa82
fix: Sitemap: Don’t convert QuerySet to list in CMSSitemap.items()
MacLake Oct 11, 2024
4ab2684
style: Format cms/tests/test_sitemap.py with ruff
MacLake Oct 14, 2024
764b6eb
test: Add test for type of CMSSitemap.items()
MacLake Oct 14, 2024
d41c8ab
Merge branch 'develop-4' into sitemap
fsbraun Oct 16, 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
33 changes: 18 additions & 15 deletions cms/sitemaps/cms_sitemap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from django.contrib.sitemaps import Sitemap
from django.db.models import OuterRef, Q, Subquery
from django.db.models import OuterRef, Q, QuerySet, Subquery

from cms.models import PageContent, PageUrl
from cms.utils import get_current_site
Expand All @@ -15,10 +15,10 @@ def from_iterable(iterables):


class CMSSitemap(Sitemap):
changefreq = "monthly"
priority = 0.5
changefreq: str = "monthly"
priority: float = 0.5

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

return list(
PageUrl
.objects
.get_for_site(site)
return (
PageUrl.objects.get_for_site(site)
.filter(language__in=languages, path__isnull=False, page__login_required=False)
.order_by('page__path')
.order_by("page__path")
.select_related("page")
.annotate(content_pk=Subquery(
PageContent.objects
.filter(page=OuterRef("page"), language=OuterRef("language"))
.filter(Q(redirect="") | Q(redirect=None))
.values_list("pk")[:1]
))
.annotate(
content_pk=Subquery(
PageContent.objects.filter(page=OuterRef("page"), language=OuterRef("language"))
.filter(Q(redirect="") | Q(redirect=None))
.values_list("pk")[:1]
)
)
.filter(content_pk__isnull=False) # Remove page content with redirects
)

Expand Down
81 changes: 46 additions & 35 deletions cms/tests/test_sitemap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import copy

from django.db.models import QuerySet

from cms.api import create_page, create_page_content
from cms.models import PageUrl
from cms.sitemaps import CMSSitemap
Expand All @@ -9,6 +11,7 @@

protocol = "http" if DJANGO_4_2 else "https"


class SitemapTestCase(CMSTestCase):
def setUp(self):
"""
Expand All @@ -27,41 +30,41 @@ def setUp(self):
+ P8 (de, en)
"""
defaults = {
'template': 'nav_playground.html',
'language': 'en',
"template": "nav_playground.html",
"language": "en",
}
with self.settings(CMS_PERMISSION=False):
p1 = create_page('P1', in_navigation=True, **defaults)
create_page_content(language='de', title="other title %s" % p1.get_title('en'), page=p1)
p1 = create_page("P1", in_navigation=True, **defaults)
create_page_content(language="de", title="other title %s" % p1.get_title("en"), page=p1)

p4 = create_page('P4', in_navigation=True, **defaults)
create_page_content(language='de', title="other title %s" % p4.get_title('en'), page=p4)
p4 = create_page("P4", in_navigation=True, **defaults)
create_page_content(language="de", title="other title %s" % p4.get_title("en"), page=p4)

p6 = create_page('P6', in_navigation=False, **defaults)
create_page_content(language='de', title="other title %s" % p6.get_title('en'), page=p6)
p6 = create_page("P6", in_navigation=False, **defaults)
create_page_content(language="de", title="other title %s" % p6.get_title("en"), page=p6)

p2 = create_page('P2', in_navigation=True, parent=p1, **defaults)
create_page_content(language='de', title="other title %s" % p2.get_title('en'), page=p2)
p2 = create_page("P2", in_navigation=True, parent=p1, **defaults)
create_page_content(language="de", title="other title %s" % p2.get_title("en"), page=p2)

p3 = create_page('P3', in_navigation=True, parent=p2, **defaults)
create_page_content(language='de', title="other title %s" % p3.get_title('en'), page=p3)
p3 = create_page("P3", in_navigation=True, parent=p2, **defaults)
create_page_content(language="de", title="other title %s" % p3.get_title("en"), page=p3)

p5 = create_page('P5', in_navigation=True, parent=p4, **defaults)
create_page_content(language='de', title="other title %s" % p5.get_title('en'), page=p5)
p5 = create_page("P5", in_navigation=True, parent=p4, **defaults)
create_page_content(language="de", title="other title %s" % p5.get_title("en"), page=p5)

p7 = create_page('P7', in_navigation=True, parent=p6, **defaults)
create_page_content(language='de', title="other title %s" % p7.get_title('en'), page=p7)
p7 = create_page("P7", in_navigation=True, parent=p6, **defaults)
create_page_content(language="de", title="other title %s" % p7.get_title("en"), page=p7)

p8 = create_page('P8', in_navigation=True, parent=p6, **defaults)
create_page_content(language='de', title="other title %s" % p8.get_title('en'), page=p8)
p8 = create_page("P8", in_navigation=True, parent=p6, **defaults)
create_page_content(language="de", title="other title %s" % p8.get_title("en"), page=p8)

p9 = create_page('P9', in_navigation=True, parent=p1, **defaults)
create_page_content(language='de', title="other title %s" % p9.get_title('en'), page=p9)
p9 = create_page("P9", in_navigation=True, parent=p1, **defaults)
create_page_content(language="de", title="other title %s" % p9.get_title("en"), page=p9)

p10 = create_page('P10', in_navigation=True, parent=p9, **defaults)
create_page_content(language='de', title="other title %s" % p10.get_title('en'), page=p10)
p10 = create_page("P10", in_navigation=True, parent=p9, **defaults)
create_page_content(language="de", title="other title %s" % p10.get_title("en"), page=p10)

create_page('P11', in_navigation=True, parent=p9, **defaults)
create_page("P11", in_navigation=True, parent=p9, **defaults)

def test_sitemap_count(self):
"""
Expand All @@ -79,11 +82,11 @@ def test_sitemap_items_location(self):
sitemap = CMSSitemap()
urlset = sitemap.get_urls()
for item in urlset:
if item['item'].path:
if item["item"].path:
url = f'{protocol}://example.com/{item["item"].language}/{item["item"].path}/'
else:
url = f'{protocol}://example.com/{item["item"].language}/'
self.assertEqual(item['location'], url)
self.assertEqual(item["location"], url)

def test_sitemap_urls(self):
"""
Expand All @@ -93,28 +96,36 @@ def test_sitemap_urls(self):
locations = []
urlset = sitemap.get_urls()
for item in urlset:
locations.append(item['location'])
locations.append(item["location"])
for page_url in PageUrl.objects.all():
if page_url.path:
url = f'{protocol}://example.com/{page_url.language}/{page_url.path}/'
url = f"{protocol}://example.com/{page_url.language}/{page_url.path}/"
else:
url = f'{protocol}://example.com/{page_url.language}/'
url = f"{protocol}://example.com/{page_url.language}/"
self.assertTrue(url in locations)

def test_sitemap_uses_public_languages_only(self):
"""
Pages on the sitemap should only show public languages.
"""
lang_settings = copy.deepcopy(get_cms_setting('LANGUAGES'))
lang_settings = copy.deepcopy(get_cms_setting("LANGUAGES"))
# sanity check
assert lang_settings[1][1]['code'] == 'de'
assert lang_settings[1][1]["code"] == "de"
# set german as private
lang_settings[1][1]['public'] = False
lang_settings[1][1]["public"] = False

with self.settings(CMS_LANGUAGES=lang_settings):
for item in CMSSitemap().get_urls():
url = f'{protocol}://example.com/en/'
url = f"{protocol}://example.com/en/"

if item["item"].path:
url += item["item"].path + "/"
self.assertEqual(item["location"], url)

if item['item'].path:
url += item['item'].path + '/'
self.assertEqual(item['location'], url)
def test_sitemap_items_type(self):
"""
Ensure that CMSSitemap.items() returns a QuerySet, not a list.
"""
sitemap = CMSSitemap()
items = sitemap.items()
self.assertIsInstance(items, QuerySet, "CMSSitemap.items() must return a QuerySet.")
Loading