From a3fe8eb3b1141e50457d72516f9bd5d45b07cc6e Mon Sep 17 00:00:00 2001 From: Bertrand Bordage Date: Wed, 11 Jul 2018 20:17:48 +0200 Subject: [PATCH] Improves sitemap to avoid passing site now that request is used. --- CHANGELOG.txt | 2 +- docs/releases/2.2.rst | 2 +- wagtail/contrib/sitemaps/sitemap_generator.py | 14 +++-- wagtail/contrib/sitemaps/tests.py | 53 +++++++++---------- wagtail/contrib/sitemaps/views.py | 4 +- 5 files changed, 41 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index 2994198756..912ebdbe29 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -20,7 +20,7 @@ Changelog * Added faceted search using the `.facet()` method (Karl Hobley) * Admin modal views no longer rely on Javascript `eval()`, for better CSP compliance (Matt Westcott) * Update editor guide for embeds and documents in rich text (Kevin Howbrook) - * Improved performance of sitemap generation (Michael van Tellingen) + * Improved performance of sitemap generation (Michael van Tellingen, Bertrand Bordage) * Added an internal API for autocomplete (Karl Hobley) * Fix: Handle all exceptions from `Image.get_file_size` (Andrew Plummer) * Fix: Fix display of breadcrumbs in ModelAdmin (LB (Ben Johnston)) diff --git a/docs/releases/2.2.rst b/docs/releases/2.2.rst index 285c38d73c..bd11b7e16a 100644 --- a/docs/releases/2.2.rst +++ b/docs/releases/2.2.rst @@ -29,7 +29,7 @@ Other features * Added faceted search using the ``.facet()`` method (Karl Hobley) * Admin modal views no longer rely on Javascript ``eval()``, for better CSP compliance (Matt Westcott) * Update editor guide for embeds and documents in rich text (Kevin Howbrook) - * Improved performance of sitemap generation (Michael van Tellingen) + * Improved performance of sitemap generation (Michael van Tellingen, Bertrand Bordage) * Added an internal API for autocomplete (Karl Hobley) Bug fixes diff --git a/wagtail/contrib/sitemaps/sitemap_generator.py b/wagtail/contrib/sitemaps/sitemap_generator.py index 73b94773b1..ce8630b094 100644 --- a/wagtail/contrib/sitemaps/sitemap_generator.py +++ b/wagtail/contrib/sitemaps/sitemap_generator.py @@ -2,14 +2,14 @@ import warnings from django.contrib.sitemaps import Sitemap as DjangoSitemap +from wagtail.core.models import Site from wagtail.core.utils import accepts_kwarg from wagtail.utils.deprecation import RemovedInWagtail24Warning class Sitemap(DjangoSitemap): - def __init__(self, site=None, request=None): - self.site = site + def __init__(self, request=None): self.request = request def location(self, obj): @@ -20,9 +20,17 @@ class Sitemap(DjangoSitemap): # (for backwards compatibility from before last_published_at was added) return (obj.last_published_at or obj.latest_revision_created_at) + def get_wagtail_site(self): + site = getattr(self.request, 'site', None) + if site is None: + return Site.objects.select_related( + 'root_page' + ).get(is_default_site=True) + return site + def items(self): return ( - self.site + self.get_wagtail_site() .root_page .get_descendants(inclusive=True) .live() diff --git a/wagtail/contrib/sitemaps/tests.py b/wagtail/contrib/sitemaps/tests.py index e88a96f8d5..7dd0b7628a 100644 --- a/wagtail/contrib/sitemaps/tests.py +++ b/wagtail/contrib/sitemaps/tests.py @@ -52,10 +52,15 @@ class TestSitemapGenerator(TestCase): # Clear the cache to that runs are deterministic regarding the sql count ContentType.objects.clear_cache() - def test_items(self): - request = RequestFactory().get('/sitemap.xml') + def get_request_and_django_site(self, url): + request = RequestFactory().get(url) + request.site = self.site + return request, get_current_site(request) - sitemap = Sitemap(self.site, request) + def test_items(self): + request, django_site = self.get_request_and_django_site('/sitemap.xml') + + sitemap = Sitemap(request) pages = sitemap.items() self.assertIn(self.child_page.page_ptr.specific, pages) @@ -63,34 +68,31 @@ class TestSitemapGenerator(TestCase): self.assertNotIn(self.protected_child_page.page_ptr.specific, pages) def test_get_urls_without_request(self): - request = RequestFactory().get('/sitemap.xml') + request, django_site = self.get_request_and_django_site('/sitemap.xml') req_protocol = request.scheme - req_site = get_current_site(request) - sitemap = Sitemap(self.site) + sitemap = Sitemap() with self.assertNumQueries(18): - urls = [url['location'] for url in sitemap.get_urls(1, req_site, req_protocol)] + urls = [url['location'] for url in sitemap.get_urls(1, django_site, req_protocol)] self.assertIn('http://localhost/', urls) # Homepage self.assertIn('http://localhost/hello-world/', urls) # Child page def test_get_urls_with_request_site_cache(self): - request = RequestFactory().get('/sitemap.xml') + request, django_site = self.get_request_and_django_site('/sitemap.xml') req_protocol = request.scheme - req_site = get_current_site(request) - sitemap = Sitemap(self.site, request) + sitemap = Sitemap(request) with self.assertNumQueries(16): - urls = [url['location'] for url in sitemap.get_urls(1, req_site, req_protocol)] + urls = [url['location'] for url in sitemap.get_urls(1, django_site, req_protocol)] self.assertIn('http://localhost/', urls) # Homepage self.assertIn('http://localhost/hello-world/', urls) # Child page def test_get_urls_uses_specific(self): - request = RequestFactory().get('/sitemap.xml') + request, django_site = self.get_request_and_django_site('/sitemap.xml') req_protocol = request.scheme - req_site = get_current_site(request) # Add an event page which has an extra url in the sitemap self.home_page.add_child(instance=EventIndex( @@ -99,19 +101,18 @@ class TestSitemapGenerator(TestCase): live=True, )) - sitemap = Sitemap(self.site, request) - urls = [url['location'] for url in sitemap.get_urls(1, req_site, req_protocol)] + sitemap = Sitemap(request) + urls = [url['location'] for url in sitemap.get_urls(1, django_site, req_protocol)] self.assertIn('http://localhost/events/', urls) # Main view self.assertIn('http://localhost/events/past/', urls) # Sub view def test_lastmod_uses_last_published_date(self): - request = RequestFactory().get('/sitemap.xml') + request, django_site = self.get_request_and_django_site('/sitemap.xml') req_protocol = request.scheme - req_site = get_current_site(request) - sitemap = Sitemap(self.site, request) - urls = sitemap.get_urls(1, req_site, req_protocol) + sitemap = Sitemap(request) + urls = sitemap.get_urls(1, django_site, req_protocol) child_page_lastmod = [ url['lastmod'] for url in urls @@ -131,12 +132,11 @@ class TestSitemapGenerator(TestCase): self.home_page.last_published_at = datetime.datetime(2017, 3, 1, 12, 0, 0, tzinfo=pytz.utc) self.home_page.save() - request = RequestFactory().get('/sitemap.xml') + request, django_site = self.get_request_and_django_site('/sitemap.xml') req_protocol = request.scheme - req_site = get_current_site(request) - sitemap = Sitemap(self.site, request) - sitemap.get_urls(1, req_site, req_protocol) + sitemap = Sitemap(request) + sitemap.get_urls(1, django_site, req_protocol) self.assertEqual(sitemap.latest_lastmod, datetime.datetime(2017, 3, 1, 12, 0, 0, tzinfo=pytz.utc)) @@ -145,12 +145,11 @@ class TestSitemapGenerator(TestCase): self.home_page.last_published_at = None self.home_page.save() - request = RequestFactory().get('/sitemap.xml') + request, django_site = self.get_request_and_django_site('/sitemap.xml') req_protocol = request.scheme - req_site = get_current_site(request) - sitemap = Sitemap(self.site, request) - sitemap.get_urls(1, req_site, req_protocol) + sitemap = Sitemap(request) + sitemap.get_urls(1, django_site, req_protocol) self.assertFalse(hasattr(sitemap, 'latest_lastmod')) diff --git a/wagtail/contrib/sitemaps/views.py b/wagtail/contrib/sitemaps/views.py index 3d7aa2dd33..8267b2a9ba 100644 --- a/wagtail/contrib/sitemaps/views.py +++ b/wagtail/contrib/sitemaps/views.py @@ -12,7 +12,7 @@ def sitemap(request, sitemaps=None, **kwargs): if sitemaps: sitemaps = prepare_sitemaps(request, sitemaps) else: - sitemaps = {'wagtail': Sitemap(request.site, request)} + sitemaps = {'wagtail': Sitemap(request)} return sitemap_views.sitemap(request, sitemaps, **kwargs) @@ -21,7 +21,7 @@ def prepare_sitemaps(request, sitemaps): initialised_sitemaps = {} for name, sitemap_cls in sitemaps.items(): if issubclass(sitemap_cls, Sitemap): - initialised_sitemaps[name] = sitemap_cls(request.site, request) + initialised_sitemaps[name] = sitemap_cls(request) else: initialised_sitemaps[name] = sitemap_cls return initialised_sitemaps