Repository: django-oscar/django-oscar

Issue:
InvalidPage exceptions in CatalogueView are not properly handled
Found a bug? Please fill out the sections below.

### Issue Summary

Oscar 2.0 (sorry if already fixed..)

If the customer is on page 2 of the Shop product list, and some products are deleted such that there is now only one page, then the customer clicks 'add to cart', they are greeted with a 404 on `
/catalogue/?page=2`

### Steps to Reproduce

1. Create 24 products, and set products to page to 20
2. Navigate to page 2 of the shop
3. Separate tab, delete 20 products but not the one about to be added to cart
4. Back on the shop, click 'add to cart.'

### Technical details

* Python version: 3.8
* Django version: 3.0
* Oscar version: 2.0


Discussion:
I've retitled the issue because it's not related to adding to cart - the issue is more simply that if you pass an invalid page number to the catalogue view it will always return a 404. We're supposed to be catching the `InvalidPage` exception [here](https://github.com/django-oscar/django-oscar/blob/master/src/oscar/apps/catalogue/views.py#L133) and redirecting to the first page, but the exception is being raised too late, in [`get_context_data()`](https://github.com/django-oscar/django-oscar/blob/master/src/oscar/apps/catalogue/views.py#L145).

That is: https://latest.oscarcommerce.com/en-gb/catalogue/?page=50 should redirect to https://latest.oscarcommerce.com/en-gb/catalogue/ instead of throwing a 404.
Basically, `SimpleProductSearchHandler` inherits `MultipleObjectMixin`, which catches `InvalidPage` and would not allow to propagate it further, but return 404 - https://github.com/django/django/blob/335c9c94acf263901fb023404408880245b0c4b4/django/views/generic/list.py#L71-L75.

Don't have better idea, other than rewrite this method ^.



Fix patch:
diff --git a/src/oscar/apps/catalogue/search_handlers.py b/src/oscar/apps/catalogue/search_handlers.py
--- a/src/oscar/apps/catalogue/search_handlers.py
+++ b/src/oscar/apps/catalogue/search_handlers.py
@@ -2,10 +2,11 @@
 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 @@ def get_search_queryset(self):
         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()
diff --git a/src/oscar/apps/catalogue/views.py b/src/oscar/apps/catalogue/views.py
--- a/src/oscar/apps/catalogue/views.py
+++ b/src/oscar/apps/catalogue/views.py
@@ -130,11 +130,12 @@ def get(self, request, *args, **kwargs):
         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 @@ def get(self, request, *args, **kwargs):
         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
diff --git a/src/oscar/apps/search/search_handlers.py b/src/oscar/apps/search/search_handlers.py
--- a/src/oscar/apps/search/search_handlers.py
+++ b/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 @@
 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 @@ def __init__(self, request_data, full_path):
         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 @@ def get_search_queryset(self):
             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):


Test patch:
diff --git a/tests/functional/catalogue/test_catalogue.py b/tests/functional/catalogue/test_catalogue.py
--- a/tests/functional/catalogue/test_catalogue.py
+++ b/tests/functional/catalogue/test_catalogue.py
@@ -103,6 +103,13 @@ def test_is_public_off(self):
         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):