Merge pull request #427 from uktrade/CMS-1397-prefetch-routing-settings

[CMS-1397] Optimise get_tree_based_url() by fetching routing settings and site simultaneously

CHANGELOG.md

@@ -11,6 +11,7 @@
 - [CMS-1417](https://uktrade.atlassian.net/browse/CMS-1417) Integrate staff SSO
 - [CMS-1462](https://uktrade.atlassian.net/browse/CMS-1462) Remove char limit from fields in `InternationalArticlePage`
 - [CMS-1449](https://uktrade.atlassian.net/browse/CMS-1449) Upgrade to wagtailmedia 0.3.0, for future Wagtail 2.4+ compat
+- [CMS-1396](https://uktrade.atlassian.net/browse/CMS-1396) Improve `BasePage.get_tree_based_routing()` efficiency by fetching site and routing settings in the same query
 - [CI-99](https://uktrade.atlassian.net/browse/CI-99) Removed unused fields for the capital invest banner on the Invest landing page
 - [CI-101](https://uktrade.atlassian.net/browse/CI-101) Added Investor Support Directory section to `InvestHomePage`
 - [CMS-1397](https://uktrade.atlassian.net/browse/CMS-1397) Updated custom url methods on `BasePage` to support tree-based routing when enabled

core/models.py

@@ -9,7 +9,7 @@
 from modeltranslation.utils import build_localized_fieldname
 from wagtail.admin.edit_handlers import FieldPanel, MultiFieldPanel
 from wagtail.contrib.settings.models import BaseSetting, register_setting
-from wagtail.core.models import Page, PageBase
+from wagtail.core.models import Page, PageBase, Site
 
 from django.core import signing
 from django.conf import settings
@@ -182,21 +182,41 @@ def get_non_prefixed_url(self, site=None):
         site = site or self.get_site()
         return self.url_path[len(site.root_page.url_path):]
 
+    def get_site(self):
+        """
+        Overrides Page.get_site() in order to fetch ``RoutingSettings``
+        in the same query (for tree-based-routing). Will also create a
+        ``RoutingSettings`` for the site if they haven't been created yet.
+        """
+        url_parts = self.get_url_parts()
+
+        if url_parts is None:
+            # page is not routable
+            return
+
+        site_id, root_url, page_path = url_parts
+
+        site = Site.objects.select_related('routingsettings').get(id=site_id)
+
+        # Ensure the site has routingsettings before returning
+        try:
+            # if select_related() above was successful, great!
+            site.routingsettings
+        except RoutingSettings.DoesNotExist:
+            # RoutingSettings need creating
+            site.routingsettings = RoutingSettings.objects.create(site=site)
+        return site
+
     def get_tree_based_url(self, include_site_url=False):
         """
         Returns the URL for this page based on it's position in the tree,
         and the RoutingSettings options for the `Site` the page belongs to.
         Wagtail multisite must be set up in order for this to work.
         """
         site = self.get_site()
+        routing_settings = site.routingsettings
         page_path = self.get_non_prefixed_url(site)
 
-        # get routing settings for the site
-        try:
-            routing_settings = site.routingsettings
-        except RoutingSettings.DoesNotExist:
-            routing_settings = RoutingSettings.objects.create(site=site)
-
         # prefix path with prefix from routing settings
         prefix = routing_settings.root_path_prefix.rstrip('/')
         if prefix:

core/tests/test_models.py

@@ -275,30 +275,60 @@ def test_get_tree_based_url(root_page):
 
 
 @pytest.mark.django_db
-def test_get_tree_based_url_without_routing_settings(root_page):
-    # This time, call without RoutingSettings configured for the site
-    # and check that an instance is created
-    domestic_app = ExportReadinessAppFactory(parent=root_page)
-    domestic_page_one = TopicLandingPageFactory(
-        parent=domestic_app, slug='topic')
-    domestic_page_two = ArticleListingPageFactory(
-        parent=domestic_page_one, slug='list')
+def test_get_site_returns_none_when_page_not_routable(
+    root_page, django_assert_num_queries
+):
+    Site.objects.all().delete()  # ensures pages are not routable
+    page = IndustryPageFactory(parent=root_page, slug='industry')
+    result = page.get_site()
+    assert result is None
 
-    Site.objects.all().delete()
-    site = Site.objects.create(
-        site_name='Great Domestic',
-        hostname='domestic.trade.great',
-        port=8007,
-        root_page=domestic_app,
-    )
 
-    domestic_page_two.get_tree_based_url()
+@pytest.mark.django_db
+def test_get_site_fetches_routing_settings_if_they_exist(
+    root_page, django_assert_num_queries
+):
+    page = IndustryPageFactory(parent=root_page, slug='industry')
+    site = Site.objects.create(hostname='example.org', root_page=page)
+    RoutingSettings.objects.create(site=site)
+
+    # running this first so that the query doesn't count toward
+    # the total query count (the value is usually cached)
+    page._get_site_root_paths()
+
+    with django_assert_num_queries(1):
+        # site and routing settings should be fetched in one query
+        result_site = page.get_site()
 
-    # Check routing settings were created
-    routing_settings = RoutingSettings.objects.get()
-    assert routing_settings.site == site
-    assert routing_settings.root_path_prefix == ''
-    assert routing_settings.include_port_in_urls is True
+        # Check the correct site was returned
+        assert result_site == site
+
+        # This attribute is set to reference the newly created object,
+        # so this shouldn't result in another query
+        result_site.routingsettings
+
+
+@pytest.mark.django_db
+def test_get_site_creates_routing_settings_if_none_exist(
+    root_page, django_assert_num_queries
+):
+    page = IndustryPageFactory(parent=root_page, slug='industry')
+    site = Site.objects.create(hostname='example.gov', root_page=page)
+
+    # running this first so that the query doesn't count toward
+    # the total query count (the value is usually cached)
+    page._get_site_root_paths()
+
+    with django_assert_num_queries(2):
+        # 1 query to get the site, 1 to create routing settings
+        result_site = page.get_site()
+
+        # Check the correct site was returned
+        assert result_site == site
+
+        # This attribute is set to reference the newly created object,
+        # so this shouldn't result in another query
+        result_site.routingsettings
 
 
 @pytest.mark.django_db