#2281 add tests, and refactor redirects, to handle duplicates with query strings

wagtail/wagtailredirects/middleware.py

@@ -4,6 +4,16 @@ from django.utils.six.moves.urllib.parse import urlparse
 from wagtail.wagtailredirects import models
 
 
+def get_redirect(request, path):
+    try:
+        return models.Redirect.get_for_site(request.site).get(old_path=path)
+    except models.Redirect.MultipleObjectsReturned:
+        # We have a site-specific and a site-ambivalent redirect; prefer the specific one
+        return models.Redirect.objects.get(site=request.site, old_path=path)
+    except models.Redirect.DoesNotExist:
+        return None
+
+
 # Originally pinched from: https://github.com/django/django/blob/master/django/contrib/redirects/middleware.py
 class RedirectMiddleware(object):
     def process_response(self, request, response):
@@ -19,23 +29,18 @@ class RedirectMiddleware(object):
         # Get the path
         path = models.Redirect.normalise_path(request.get_full_path())
 
-        # Get the path without the query string or params
-        path_without_query = urlparse(path).path
-
         # Find redirect
-        try:
-            redirect = models.Redirect.get_for_site(request.site).get(old_path=path)
-        except models.Redirect.MultipleObjectsReturned:
-            # We have a site-specific and a site-ambivalent redirect; prefer the specific one
-            redirect = models.Redirect.objects.get(site=request.site, old_path=path)
-        except models.Redirect.DoesNotExist:
+        redirect = get_redirect(request, path)
+        if redirect is None:
+            # Get the path without the query string or params
+            path_without_query = urlparse(path).path
+
             if path == path_without_query:
                 # don't try again if we know we will get the same response
                 return response
 
-            try:
-                redirect = models.Redirect.get_for_site(request.site).get(old_path=path_without_query)
-            except models.Redirect.DoesNotExist:
+            redirect = get_redirect(request, path_without_query)
+            if redirect is None:
                 return response
 
         if redirect.is_permanent:

wagtail/wagtailredirects/tests.py

@@ -159,17 +159,70 @@ class TestRedirects(TestCase):
     def test_duplicate_redirects_when_match_is_for_generic(self):
         contact_page = Page.objects.get(url_path='/home/contact-us/')
         site = Site.objects.create(hostname='other.example.com', port=80, root_page=contact_page)
-        christmas_page = Page.objects.get(url_path='/home/events/christmas/')
 
         # two redirects, one for any site, one for specific
-        models.Redirect.objects.create(old_path='/xmas', redirect_page=contact_page)
-        models.Redirect.objects.create(site=site, old_path='/xmas', redirect_page=christmas_page)
+        models.Redirect.objects.create(old_path='/xmas', redirect_link='/generic')
+        models.Redirect.objects.create(site=site, old_path='/xmas', redirect_link='/site-specific')
 
-        response = self.client.get('/xmas/', HTTP_HOST='localhost')
-        # the redirect which matched was the generic one, pointing to contact_page
-        self.assertRedirects(response, 'http://other.example.com/', status_code=301, fetch_redirect_response=False)
+        response = self.client.get('/xmas/')
+        # the redirect which matched was /generic
+        self.assertRedirects(response, '/generic', status_code=301, fetch_redirect_response=False)
 
-    def test_duplicate_redirects_across_sites_when_match_is_for_specific(self):
+    def test_duplicate_redirects_with_query_string_when_match_is_for_generic(self):
+        contact_page = Page.objects.get(url_path='/home/contact-us/')
+        site = Site.objects.create(hostname='other.example.com', port=80, root_page=contact_page)
+
+        # two redirects, one for any site, one for specific, both with query string
+        models.Redirect.objects.create(old_path='/xmas?foo=Bar', redirect_link='/generic-with-query-string')
+        models.Redirect.objects.create(site=site, old_path='/xmas?foo=Bar', redirect_link='/site-specific-with-query-string')
+
+        # and two redirects, one for any site, one for specific, without query strings
+        models.Redirect.objects.create(old_path='/xmas', redirect_link='/generic')
+        models.Redirect.objects.create(site=site, old_path='/xmas', redirect_link='/site-specific')
+
+        response = self.client.get('/xmas/?foo=Bar')
+        # the redirect which matched was /generic-with-query-string
+        self.assertRedirects(response, '/generic-with-query-string', status_code=301, fetch_redirect_response=False)
+
+        # now use a non-matching query string
+        response = self.client.get('/xmas/?foo=Baz')
+        # the redirect which matched was /generic
+        self.assertRedirects(response, '/generic', status_code=301, fetch_redirect_response=False)
+
+    def test_duplicate_redirects_when_match_is_for_specific(self):
+        contact_page = Page.objects.get(url_path='/home/contact-us/')
+        site = Site.objects.create(hostname='other.example.com', port=80, root_page=contact_page)
+
+        # two redirects, one for any site, one for specific
+        models.Redirect.objects.create(old_path='/xmas', redirect_link='/generic')
+        models.Redirect.objects.create(site=site, old_path='/xmas', redirect_link='/site-specific')
+
+        response = self.client.get('/xmas/', HTTP_HOST='other.example.com')
+        # the redirect which matched was /site-specific
+        self.assertRedirects(response, 'http://other.example.com/site-specific', status_code=301, fetch_redirect_response=False)
+
+    def test_duplicate_redirects_with_query_string_when_match_is_for_specific_with_qs(self):
+        contact_page = Page.objects.get(url_path='/home/contact-us/')
+        site = Site.objects.create(hostname='other.example.com', port=80, root_page=contact_page)
+
+        # two redirects, one for any site, one for specific, both with query string
+        models.Redirect.objects.create(old_path='/xmas?foo=Bar', redirect_link='/generic-with-query-string')
+        models.Redirect.objects.create(site=site, old_path='/xmas?foo=Bar', redirect_link='/site-specific-with-query-string')
+
+        # and two redirects, one for any site, one for specific, without query strings
+        models.Redirect.objects.create(old_path='/xmas', redirect_link='/generic')
+        models.Redirect.objects.create(site=site, old_path='/xmas', redirect_link='/site-specific')
+
+        response = self.client.get('/xmas/?foo=Bar', HTTP_HOST='other.example.com')
+        # the redirect which matched was /site-specific-with-query-string
+        self.assertRedirects(response, 'http://other.example.com/site-specific-with-query-string', status_code=301, fetch_redirect_response=False)
+
+        # now use a non-matching query string
+        response = self.client.get('/xmas/?foo=Baz', HTTP_HOST='other.example.com')
+        # the redirect which matched was /site-specific
+        self.assertRedirects(response, 'http://other.example.com/site-specific', status_code=301, fetch_redirect_response=False)
+
+    def test_duplicate_page_redirects_when_match_is_for_specific(self):
         contact_page = Page.objects.get(url_path='/home/contact-us/')
         site = Site.objects.create(hostname='other.example.com', port=80, root_page=contact_page)
         christmas_page = Page.objects.get(url_path='/home/events/christmas/')
@@ -182,20 +235,6 @@ class TestRedirects(TestCase):
         response = self.client.get('/xmas/', HTTP_HOST='other.example.com')
         self.assertRedirects(response, 'http://localhost/events/christmas/', status_code=301, fetch_redirect_response=False)
 
-    def test_duplicate_redirects_within_a_site_when_match_is_for_specific(self):
-        businessy_page = Page.objects.get(url_path='/home/events/businessy-events/')
-        site = Site.objects.create(hostname='other.example.com', port=80, root_page=businessy_page)
-        board_meetings_page = Page.objects.get(url_path='/home/events/businessy-events/board-meetings/')
-        christmas_page = Page.objects.get(url_path='/home/events/christmas/')
-
-        # two redirects, one for any site, one for specific
-        models.Redirect.objects.create(old_path='/xmas', redirect_page=christmas_page)
-        models.Redirect.objects.create(site=site, old_path='/xmas', redirect_page=board_meetings_page)
-
-        # request for specific site gets the meetings_page redirect, which is accessible from that site
-        response = self.client.get('/xmas/', HTTP_HOST='other.example.com')
-        self.assertRedirects(response, 'http://other.example.com/board-meetings/', status_code=301, fetch_redirect_response=False)
-
 
 class TestRedirectsIndexView(TestCase, WagtailTestUtils):
     def setUp(self):