From 407fccba2f7edd9a7acd0dcade20117d0cd91ae9 Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Mon, 23 Dec 2024 20:32:12 -0800 Subject: [PATCH] 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 Reviewed-by: Veiko Aasa --- plinth/conftest.py | 14 +++++++ plinth/middleware.py | 34 ++++++++++++++++- plinth/modules/help/tests/test_views.py | 7 +--- plinth/modules/kiwix/tests/test_views.py | 7 ++-- plinth/tests/test_context_processors.py | 16 +------- plinth/tests/test_middleware.py | 48 +++++++++++++++++++++--- plinth/tests/test_views.py | 15 +------- 7 files changed, 97 insertions(+), 44 deletions(-) diff --git a/plinth/conftest.py b/plinth/conftest.py index df8527692..d6f14017e 100644 --- a/plinth/conftest.py +++ b/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') diff --git a/plinth/middleware.py b/plinth/middleware.py index 22c26c132..a22533016 100644 --- a/plinth/middleware.py +++ b/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] diff --git a/plinth/modules/help/tests/test_views.py b/plinth/modules/help/tests/test_views.py index 56e73602a..573415816 100644 --- a/plinth/modules/help/tests/test_views.py +++ b/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 menu as menu_module -from plinth import module_loader +from plinth import cfg, 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 diff --git a/plinth/modules/kiwix/tests/test_views.py b/plinth/modules/kiwix/tests/test_views.py index 66ab72118..4d58372a1 100644 --- a/plinth/modules/kiwix/tests/test_views.py +++ b/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') diff --git a/plinth/tests/test_context_processors.py b/plinth/tests/test_context_processors.py index 33dbe642c..32a0face7 100644 --- a/plinth/tests/test_context_processors.py +++ b/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) diff --git a/plinth/tests/test_middleware.py b/plinth/tests/test_middleware.py index 1ba500205..0b489a22f 100644 --- a/plinth/tests/test_middleware.py +++ b/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() diff --git a/plinth/tests/test_views.py b/plinth/tests/test_views.py index e6d7e4bea..618a35e5a 100644 --- a/plinth/tests/test_views.py +++ b/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 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): +def test_get_breadcrumbs(rf, test_menu): """Test that computing breadcrumbs works.""" def _crumb(name: str, is_active: bool = False, url_name: str | None = None,