Fix handling of InvalidPage exceptions in CatalogueView (#3505)

src/oscar/apps/catalogue/search_handlers.py

@@ -2,10 +2,11 @@ from django.conf import settings
 from django.utils.module_loading import import_string
 from django.views.generic.list import MultipleObjectMixin
 
-from oscar.core.loading import get_class, get_model
+from oscar.core.loading import get_class, get_classes, get_model
 
 BrowseCategoryForm = get_class('search.forms', 'BrowseCategoryForm')
-SearchHandler = get_class('search.search_handlers', 'SearchHandler')
+SearchResultsPaginationMixin, SearchHandler = get_classes(
+    'search.search_handlers', ('SearchHandler', 'SearchResultsPaginationMixin'))
 is_solr_supported = get_class('search.features', 'is_solr_supported')
 is_elasticsearch_supported = get_class('search.features', 'is_elasticsearch_supported')
 Product = get_model('catalogue', 'Product')
@@ -77,7 +78,7 @@ class ESProductSearchHandler(SearchHandler):
         return sqs
 
 
-class SimpleProductSearchHandler(MultipleObjectMixin):
+class SimpleProductSearchHandler(SearchResultsPaginationMixin, MultipleObjectMixin):
     """
     A basic implementation of the full-featured SearchHandler that has no
     faceting support, but doesn't require a Haystack backend. It only
@@ -89,6 +90,7 @@ class SimpleProductSearchHandler(MultipleObjectMixin):
     paginate_by = settings.OSCAR_PRODUCTS_PER_PAGE
 
     def __init__(self, request_data, full_path, categories=None):
+        self.request_data = request_data
         self.categories = categories
         self.kwargs = {'page': request_data.get('page') or 1}
         self.object_list = self.get_queryset()

src/oscar/apps/catalogue/views.py

@@ -130,11 +130,12 @@ class CatalogueView(TemplateView):
         try:
             self.search_handler = self.get_search_handler(
                 self.request.GET, request.get_full_path(), [])
+            response = super().get(request, *args, **kwargs)
         except InvalidPage:
             # Redirect to page one.
             messages.error(request, _('The given page number was invalid.'))
             return redirect('catalogue:index')
-        return super().get(request, *args, **kwargs)
+        return response
 
     def get_search_handler(self, *args, **kwargs):
         return get_product_search_handler_class()(*args, **kwargs)
@@ -172,11 +173,12 @@ class ProductCategoryView(TemplateView):
         try:
             self.search_handler = self.get_search_handler(
                 request.GET, request.get_full_path(), self.get_categories())
+            response = super().get(request, *args, **kwargs)
         except InvalidPage:
             messages.error(request, _('The given page number was invalid.'))
             return redirect(self.category.get_absolute_url())
 
-        return super().get(request, *args, **kwargs)
+        return response
 
     def is_viewable(self, category, request):
         return category.is_public or request.user.is_staff

src/oscar/apps/search/search_handlers.py

@@ -1,5 +1,4 @@
 from django.core.paginator import InvalidPage, Paginator
-from django.utils.translation import gettext_lazy as _
 from haystack import connections
 
 from oscar.core.loading import get_class
@@ -9,7 +8,39 @@ from . import facets
 FacetMunger = get_class('search.facets', 'FacetMunger')
 
 
-class SearchHandler(object):
+class SearchResultsPaginationMixin:
+    paginate_by = None
+    paginator_class = Paginator
+    page_kwarg = 'page'
+
+    def paginate_queryset(self, queryset, page_size):
+        """
+        Paginate the search results. This is a simplified version of
+        Django's MultipleObjectMixin.paginate_queryset
+        """
+        paginator = self.get_paginator(queryset, page_size)
+        page_kwarg = self.page_kwarg
+        page_number = self.request_data.get(page_kwarg, 1)
+        try:
+            page_number = int(page_number)
+        except ValueError:
+            if page_number == 'last':
+                page_number = paginator.num_pages
+            else:
+                raise InvalidPage
+        # This can also raise an InvalidPage exception.
+        page = paginator.page(page_number)
+        return paginator, page, page.object_list, page.has_other_pages()
+
+    def get_paginator(self, queryset, per_page=None):
+        """
+        Return a paginator. Override this to set settings like orphans,
+        allow_empty, etc.
+        """
+        return self.paginator_class(queryset, per_page)
+
+
+class SearchHandler(SearchResultsPaginationMixin):
     """
     A class that is concerned with performing a search and paginating the
     results. The search is triggered upon initialisation (mainly to have a
@@ -36,9 +67,6 @@ class SearchHandler(object):
 
     form_class = None
     model_whitelist = None
-    paginate_by = None
-    paginator_class = Paginator
-    page_kwarg = 'page'
 
     def __init__(self, request_data, full_path):
         self.full_path = full_path
@@ -51,8 +79,7 @@ class SearchHandler(object):
         self.results = self.get_search_results(self.search_form)
         # If below raises an UnicodeDecodeError, you're running pysolr < 3.2
         # with Solr 4.
-        self.paginator, self.page = self.paginate_queryset(
-            self.results, request_data)
+        self.paginator, self.page = self.paginate_queryset(self.results, self.paginate_by)[0:2]
 
     # Search related methods
 
@@ -85,34 +112,6 @@ class SearchHandler(object):
             sqs = sqs.models(*self.model_whitelist)
         return sqs
 
-    # Pagination related methods
-
-    def paginate_queryset(self, queryset, request_data):
-        """
-        Paginate the search results. This is a simplified version of
-        Django's MultipleObjectMixin.paginate_queryset
-        """
-        paginator = self.get_paginator(queryset)
-        page_kwarg = self.page_kwarg
-        page = request_data.get(page_kwarg, 1)
-        try:
-            page_number = int(page)
-        except ValueError:
-            if page == 'last':
-                page_number = paginator.num_pages
-            else:
-                raise InvalidPage(_(
-                    "Page is not 'last', nor can it be converted to an int."))
-        # This can also raise an InvalidPage exception.
-        return paginator, paginator.page(page_number)
-
-    def get_paginator(self, queryset):
-        """
-        Return a paginator. Override this to set settings like orphans,
-        allow_empty, etc.
-        """
-        return self.paginator_class(queryset, self.paginate_by)
-
     # Accessing the search results and meta data
 
     def bulk_fetch_results(self, paginated_results):

tests/functional/catalogue/test_catalogue.py

@@ -103,6 +103,13 @@ class TestProductListView(WebTestCase):
         products_on_page = list(page.context['products'].all())
         self.assertEqual(products_on_page, [])
 
+    def test_invalid_page_redirects_to_index(self):
+        create_product()
+        products_list_url = reverse('catalogue:index')
+        response = self.app.get('%s?page=200' % products_list_url)
+        self.assertEqual(response.status_code, 302)
+        self.assertRedirectsTo(response, 'catalogue:index')
+
 
 class TestProductCategoryView(WebTestCase):