ui: Handle and show most page load errors as alerts

- In addition to the OperationalError, also handle all generic exceptions during
page submit and page load. Redirect to the same page or parent using
breadcrumbs.

- Log exceptions handled by common error middleware so that they are also part
of the system logs.

- Update kiwix test as needed.

- Refactor some test code that is setting up the menu items.

Tests:

- When an error occurs during form POST, the same page is show but with an error
message.

- When an error occurs in an app page during GET, the browser is redirected to
the parent section.

- When an error occurs in apps page during GET, the browser is redirected to the
home page.

- When an error occurs in home page during GET, the error is not handled and
default 500 handle is triggered.

Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>
Reviewed-by: Veiko Aasa <veiko17@disroot.org>

plinth/conftest.py

@@ -266,3 +266,17 @@ def fixture_host_sudo(host):
     """Pytest fixture to run commands with sudo."""
     with host.sudo():
         yield host
+
+
+@pytest.fixture(name='test_menu')
+def fixture_test_menu():
+    """Initialized menu module."""
+    from plinth import menu as menu_module
+
+    menu_module.Menu._all_menus = set()
+    menu_module.init()
+    menu_module.Menu('home-id', name='Home', url_name='index')
+    menu_module.Menu('apps-id', name='Apps', url_name='apps',
+                     parent_url_name='index')
+    menu_module.Menu('testapp-id', name='Test App', url_name='testapp:index',
+                     parent_url_name='apps')

plinth/middleware.py

@@ -11,7 +11,7 @@ from django.contrib import messages
 from django.contrib.auth.decorators import login_required
 from django.core.exceptions import PermissionDenied
 from django.db.utils import OperationalError
-from django.shortcuts import render
+from django.shortcuts import redirect, render
 from django.template.response import SimpleTemplateResponse
 from django.utils.deprecation import MiddlewareMixin
 from django.utils.translation import gettext as _
@@ -126,6 +126,8 @@ class CommonErrorMiddleware(MiddlewareMixin):
     @staticmethod
     def process_exception(request, exception):
         """Show a custom error page when OperationalError is raised."""
+        logger.exception('Error processing page. %s %s, exception: %s',
+                         request.method, request.path, exception)
         if isinstance(exception, OperationalError):
             message = _(
                 'System is possibly under heavy load. Please retry later.')
@@ -133,4 +135,34 @@ class CommonErrorMiddleware(MiddlewareMixin):
                                           context={'message': message},
                                           status=503)
 
+        if isinstance(exception, Exception):
+            match = request.resolver_match
+            if not match.app_name and match.url_name == 'index':
+                # Don't try to handle errors on the home page as it will lead
+                # to infinite redirects.
+                return None
+
+            if request.method == 'POST':
+                message = _('Error running operation.')
+            else:
+                message = _('Error loading page.')
+
+            views.messages_error(request, message, exception)
+            redirect_url = CommonErrorMiddleware._get_redirect_url_on_error(
+                request)
+            return redirect(redirect_url)
+
         return None
+
+    @staticmethod
+    def _get_redirect_url_on_error(request):
+        """Return the URL to redirect to after an error."""
+        if request.method != 'GET':
+            return request.path
+
+        # If the original request was a GET, trying to redirect to same URL
+        # with same request method might result in an recursive loop. Instead
+        # redirect to a parent URL.
+        breadcrumbs = views.get_breadcrumbs(request)
+        parent_index = 1 if len(breadcrumbs) > 1 else 0
+        return list(breadcrumbs.keys())[parent_index]

plinth/modules/help/tests/test_views.py

@@ -20,9 +20,7 @@ from django.conf import settings
 from django.http import Http404
 from django.urls import re_path
 
-from plinth import cfg
+from plinth import cfg, module_loader
-from plinth import menu as menu_module
-from plinth import module_loader
 from plinth.modules import help as help_module
 from plinth.modules.help import views
 
@@ -57,9 +55,8 @@ def fixture_developer_configuration():
 
 
 @pytest.fixture(name='menu', autouse=True)
-def fixture_menu():
+def fixture_menu(test_menu):
     """Initialized menu module."""
-    menu_module.init()
     help_module.HelpApp()  # Create all menu components
 
 

plinth/modules/kiwix/tests/test_views.py

@@ -3,12 +3,12 @@
 Test module for Kiwix views.
 """
 
-from unittest.mock import call, patch, MagicMock
+from unittest.mock import MagicMock, call, patch
 
 import pytest
 from django import urls
-from django.core.files.uploadedfile import SimpleUploadedFile
 from django.contrib.messages.storage.fallback import FallbackStorage
+from django.core.files.uploadedfile import SimpleUploadedFile
 from django.http.response import Http404
 
 from plinth import module_loader
@@ -94,7 +94,8 @@ def test_add_package_failed(add_package, add_package_request):
                                       views.AddPackageView.as_view())
     assert response.status_code == 302
     assert response.url == urls.reverse('kiwix:index')
-    assert list(messages)[0].message == 'Failed to add content package.'
+    assert list(messages)[0].message.startswith(
+        'Failed to add content package.')
 
 
 @patch('plinth.app.App.get')

plinth/tests/test_context_processors.py

@@ -5,27 +5,13 @@ Test module for custom context processors.
 
 from unittest.mock import MagicMock, Mock, patch
 
-import pytest
 from django.urls import resolve
 
 from plinth import context_processors as cp
-from plinth import menu as menu_module
-
-
-@pytest.fixture(name='menu', autouse=True)
-def fixture_menu():
-    """Initialized menu module."""
-    menu_module.Menu._all_menus = set()
-    menu_module.init()
-    menu_module.Menu('home-id', name='Home', url_name='index')
-    menu_module.Menu('apps-id', name='Apps', url_name='apps',
-                     parent_url_name='index')
-    menu_module.Menu('testapp-id', name='Test App', url_name='testapp:index',
-                     parent_url_name='apps')
 
 
 @patch('plinth.notification.Notification')
-def test_common(Notification, load_cfg, rf):
+def test_common(Notification, load_cfg, rf, test_menu):
     """Verify that the common() function returns the correct values."""
     url = '/apps/testapp/create/'
     request = rf.get(url)

plinth/tests/test_middleware.py

@@ -9,8 +9,9 @@ import pytest
 from django.contrib.auth.models import AnonymousUser, Group, User
 from django.core.exceptions import PermissionDenied
 from django.db.utils import OperationalError
-from django.http import HttpResponse
+from django.http import HttpResponse, HttpResponseRedirect
 from django.test.client import RequestFactory
+from django.urls import resolve
 from stronghold.decorators import public
 
 from plinth import app as app_module
@@ -144,12 +145,14 @@ class TestSetupMiddleware:
         resolve.return_value.namespaces = ['mockapp']
         operation_manager.collect_results.return_value = [
             Mock(translated_message='message1', exception=None),
-            Mock(translated_message='message2', exception='x-exception')
+            Mock(translated_message='message2',
+                 exception=RuntimeError('x-exception'))
         ]
         app.get_setup_state = lambda: app_module.App.SetupState.UP_TO_DATE
 
         # Admin user can collect result
         request = RequestFactory().get('/plinth/mockapp')
+        request.resolver_match = Mock()
         user = User(username='adminuser')
         user.save()
         group = Group(name='admin')
@@ -162,7 +165,9 @@ class TestSetupMiddleware:
 
         assert response is None
         messages_success.assert_has_calls([call(request, 'message1')])
-        messages_error.assert_has_calls([call(request, 'message2')])
+        messages_error.assert_called_once()
+        assert messages_error.call_args.args[0] == request
+        assert messages_error.call_args.args[1].startswith('message2')
         operation_manager.collect_results.assert_has_calls([call('mockapp')])
 
         # Non-admin user can't collect result
@@ -265,7 +270,8 @@ class TestCommonErrorMiddleware:
     @pytest.fixture(name='web_request')
     def fixture_web_request():
         """Fixture for returning web request."""
-        web_request = RequestFactory().get('/plinth/mockapp')
+        web_request = RequestFactory().get('/apps/testapp/')
+        web_request.resolver_match = resolve('/apps/testapp/')
         web_request.user = Mock()
         return web_request
 
@@ -288,6 +294,36 @@ class TestCommonErrorMiddleware:
         assert 'message' in response.context_data
 
     @staticmethod
-    def test_other_error(middleware, web_request, other_error):
+    @patch('django.contrib.messages.error')
+    def test_other_error_get(messages_error, middleware, web_request,
+                             other_error, test_menu):
         response = middleware.process_exception(web_request, other_error)
-        assert response is None
+        assert isinstance(response, HttpResponseRedirect)
+        assert response.url == '/apps/'
+        messages_error.assert_called_once()
+        assert messages_error.call_args.args[0] == web_request
+        assert messages_error.call_args.args[1].startswith(
+            'Error loading page.')
+
+    @staticmethod
+    @patch('django.contrib.messages.error')
+    def test_other_error_post(messages_error, middleware, web_request,
+                              other_error, test_menu):
+        web_request.method = 'POST'
+        response = middleware.process_exception(web_request, other_error)
+        assert isinstance(response, HttpResponseRedirect)
+        assert response.url == '/apps/testapp/'
+        messages_error.assert_called_once()
+        assert messages_error.call_args.args[0] == web_request
+        assert messages_error.call_args.args[1].startswith(
+            'Error running operation.')
+
+    @staticmethod
+    @patch('django.contrib.messages.error')
+    def test_other_error_index(messages_error, middleware, web_request,
+                               other_error, test_menu):
+        web_request.path = '/'
+        web_request.resolver_match = resolve('/')
+        response = middleware.process_exception(web_request, other_error)
+        assert not response
+        messages_error.assert_not_called()

plinth/tests/test_views.py

@@ -6,23 +6,10 @@ Tests for common FreedomBox views.
 import pytest
 from django.urls import resolve
 
-from plinth import menu as menu_module
 from plinth.views import get_breadcrumbs, is_safe_url
 
 
-@pytest.fixture(name='menu')
+def test_get_breadcrumbs(rf, test_menu):
-def fixture_menu():
-    """Initialized menu module."""
-    menu_module.Menu._all_menus = set()
-    menu_module.init()
-    menu_module.Menu('home-id', name='Home', url_name='index')
-    menu_module.Menu('apps-id', name='Apps', url_name='apps',
-                     parent_url_name='index')
-    menu_module.Menu('testapp-id', name='Test App', url_name='testapp:index',
-                     parent_url_name='apps')
-
-
-def test_get_breadcrumbs(rf, menu):
     """Test that computing breadcrumbs works."""
 
     def _crumb(name: str, is_active: bool = False, url_name: str | None = None,