From 528fd08245279b532b0861ebfe2e1cd5c6993b5c Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Sun, 21 Nov 2021 10:47:49 -0800 Subject: [PATCH] middleware, views: Reduce use of setup_helper Signed-off-by: Sunil Mohan Adapa Reviewed-by: James Valleroy --- plinth/middleware.py | 21 ++++++----- plinth/tests/test_middleware.py | 66 ++++++++++++++++++++------------- plinth/views.py | 30 +++++++++------ 3 files changed, 69 insertions(+), 48 deletions(-) diff --git a/plinth/middleware.py b/plinth/middleware.py index fad795eda..b68db7a89 100644 --- a/plinth/middleware.py +++ b/plinth/middleware.py @@ -4,6 +4,7 @@ Common Django middleware. """ import logging +import sys from django import urls from django.conf import settings @@ -15,7 +16,6 @@ from django.utils.deprecation import MiddlewareMixin from django.utils.translation import gettext_lazy as _ from stronghold.utils import is_view_func_public -import plinth from plinth import app as app_module from plinth import setup from plinth.package import PackageException @@ -26,14 +26,15 @@ from . import views logger = logging.getLogger(__name__) -def _collect_setup_result(request, module): +def _collect_setup_result(request, app): """Show success/fail message from previous install operation.""" - if not module.setup_helper.is_finished: + setup_helper = sys.modules[app.__module__].setup_helper + if not setup_helper.is_finished: return - exception = module.setup_helper.collect_result() + exception = setup_helper.collect_result() if not exception: - if not module.setup_helper.app.info.is_essential: + if not app.info.is_essential: messages.success(request, _('Application installed.')) else: if isinstance(exception, PackageException): @@ -72,16 +73,16 @@ class SetupMiddleware(MiddlewareMixin): # Requested URL does not belong to any application return - module_name = resolver_match.namespaces[0] - module = plinth.module_loader.loaded_modules[module_name] + app_id = resolver_match.namespaces[0] + app = app_module.App.get(app_id) is_admin = is_user_admin(request) # Collect and show setup operation result to admins if is_admin: - _collect_setup_result(request, module) + _collect_setup_result(request, app) # Check if application is up-to-date - if module.app.get_setup_state() == \ + if app.get_setup_state() == \ app_module.App.SetupState.UP_TO_DATE: return @@ -90,7 +91,7 @@ class SetupMiddleware(MiddlewareMixin): # Only allow logged-in users to access any setup page view = login_required(views.SetupView.as_view()) - return view(request, setup_helper=module.setup_helper) + return view(request, app_id=app_id) class AdminRequiredMiddleware(MiddlewareMixin): diff --git a/plinth/tests/test_middleware.py b/plinth/tests/test_middleware.py index ff1e80fb8..26f9dfb6b 100644 --- a/plinth/tests/test_middleware.py +++ b/plinth/tests/test_middleware.py @@ -15,6 +15,8 @@ from stronghold.decorators import public from plinth import app as app_module from plinth.middleware import AdminRequiredMiddleware, SetupMiddleware +setup_helper = None + @pytest.fixture(name='kwargs') def fixture_kwargs(): @@ -26,6 +28,26 @@ def fixture_kwargs(): } +@pytest.fixture(name='app') +def fixture_app(): + """Fixture for returning a test app.""" + + class AppTest(app_module.App): + """Test app.""" + app_id = 'mockapp' + + def __init__(self): + """Add info component.""" + super().__init__() + self.add(app_module.Info('mockapp', version=1)) + + def get_setup_state(self): + return app_module.App.SetupState.NEEDS_SETUP + + app_module.App._all_apps = {} + return AppTest() + + class TestSetupMiddleware: """Test cases for setup middleware.""" @@ -52,18 +74,15 @@ class TestSetupMiddleware: assert response is None @staticmethod - @patch('plinth.module_loader.loaded_modules') + @patch('plinth.tests.test_middleware.setup_helper') @patch('django.urls.resolve') @patch('django.urls.reverse', return_value='users:login') - def test_module_is_up_to_date(reverse, resolve, loaded_modules, middleware, - kwargs): + def test_module_is_up_to_date(_reverse, resolve, setup_helper_, app, + middleware, kwargs): """Test that none is returned when module is up-to-date.""" resolve.return_value.namespaces = ['mockapp'] - module = Mock() - module.setup_helper.is_finished = None - module.app.get_setup_state.return_value = \ - app_module.App.SetupState.UP_TO_DATE - loaded_modules.__getitem__.return_value = module + setup_helper_.is_finished = None + app.get_setup_state = lambda: app_module.App.SetupState.UP_TO_DATE request = RequestFactory().get('/plinth/mockapp') request.user = AnonymousUser() @@ -71,22 +90,20 @@ class TestSetupMiddleware: assert response is None @staticmethod + @patch('plinth.tests.test_middleware.setup_helper') @patch('plinth.views.SetupView') - @patch('plinth.module_loader.loaded_modules') @patch('django.urls.resolve') @patch('django.urls.reverse', return_value='users:login') @pytest.mark.django_db - def test_module_view(reverse, resolve, loaded_modules, setup_view, + def test_module_view(_reverse, resolve, setup_view, setup_helper, app, middleware, kwargs): """Test that only registered users can access the setup view.""" resolve.return_value.namespaces = ['mockapp'] - module = Mock() - module.setup_helper.is_finished = None - loaded_modules.__getitem__.return_value = module view = Mock() setup_view.as_view.return_value = view request = RequestFactory().get('/plinth/mockapp') request.session = MagicMock() + setup_helper.is_finished = None # Verify that anonymous users cannot access the setup page request.user = AnonymousUser() @@ -116,24 +133,21 @@ class TestSetupMiddleware: request.user = user middleware.process_view(request, **kwargs) setup_view.as_view.assert_called_once_with() - view.assert_called_once_with(request, setup_helper=module.setup_helper) + view.assert_called_once_with(request, app_id='mockapp') @staticmethod + @patch('plinth.tests.test_middleware.setup_helper') @patch('django.contrib.messages.success') - @patch('plinth.module_loader.loaded_modules') @patch('django.urls.resolve') @patch('django.urls.reverse', return_value='users:login') @pytest.mark.django_db - def test_install_result_collection(reverse, resolve, loaded_modules, - messages_success, middleware, kwargs): + def test_install_result_collection(reverse, resolve, messages_success, + setup_helper_, app, middleware, kwargs): """Test that module installation result is collected properly.""" resolve.return_value.namespaces = ['mockapp'] - module = Mock() - module.setup_helper.is_finished = True - module.setup_helper.collect_result.return_value = None - module.app.get_setup_state.return_value = \ - app_module.App.SetupState.UP_TO_DATE - loaded_modules.__getitem__.return_value = module + setup_helper_.is_finished = True + setup_helper_.collect_result.return_value = None + app.get_setup_state = lambda: app_module.App.SetupState.UP_TO_DATE # Admin user can't collect result request = RequestFactory().get('/plinth/mockapp') @@ -149,11 +163,11 @@ class TestSetupMiddleware: assert response is None assert messages_success.called - module.setup_helper.collect_result.assert_called_once_with() + setup_helper_.collect_result.assert_called_once_with() # Non-admin user can't collect result messages_success.reset_mock() - module.setup_helper.collect_result.reset_mock() + setup_helper_.collect_result.reset_mock() request = RequestFactory().get('/plinth/mockapp') user = User(username='johndoe') user.save() @@ -163,7 +177,7 @@ class TestSetupMiddleware: assert response is None assert not messages_success.called - module.setup_helper.collect_result.assert_not_called() + setup_helper_.collect_result.assert_not_called() class TestAdminMiddleware: diff --git a/plinth/views.py b/plinth/views.py index 5a381c96c..42b86b06a 100644 --- a/plinth/views.py +++ b/plinth/views.py @@ -3,6 +3,7 @@ Main FreedomBox views. """ +import sys import time import urllib.parse @@ -16,7 +17,8 @@ from django.views.generic import TemplateView from django.views.generic.edit import FormView from stronghold.decorators import public -from plinth import app, package +from plinth import app as app_module +from plinth import package from plinth.daemon import app_is_running from plinth.modules.config import get_advanced_mode from plinth.modules.firewall.components import get_port_forwarding_info @@ -182,7 +184,7 @@ class AppView(FormView): if not self.app_id: raise ImproperlyConfigured('Missing attribute: app_id') - return app.App.get(self.app_id) + return app_module.App.get(self.app_id) def get_form(self, *args, **kwargs): """Return an instance of this view's form. @@ -269,19 +271,21 @@ class SetupView(TemplateView): def get_context_data(self, **kwargs): """Return the context data rendering the template.""" - context = super(SetupView, self).get_context_data(**kwargs) - setup_helper = self.kwargs['setup_helper'] - context['setup_helper'] = setup_helper - context['app_info'] = setup_helper.module.app.info + context = super().get_context_data(**kwargs) + app_id = self.kwargs['app_id'] + app = app_module.App.get(app_id) + + context['app_info'] = app.info # Report any installed conflicting packages that will be removed. package_conflicts, package_conflicts_action = \ - self._get_app_package_conflicts(setup_helper.module.app) + self._get_app_package_conflicts(app) context['package_conflicts'] = package_conflicts context['package_conflicts_action'] = package_conflicts_action # Reuse the value of setup_state throughout the view for consistency. - context['setup_state'] = setup_helper.module.app.get_setup_state() + context['setup_state'] = app.get_setup_state() + setup_helper = sys.modules[app.__module__].setup_helper context['setup_current_operation'] = setup_helper.current_operation # Perform expensive operation only if needed. @@ -290,10 +294,10 @@ class SetupView(TemplateView): 'package_manager_is_busy'] = package.is_package_manager_busy() context[ 'has_unavailable_packages'] = self._has_unavailable_packages( - setup_helper.module.app) + app) context['refresh_page_sec'] = None - if context['setup_state'] == app.App.SetupState.UP_TO_DATE: + if context['setup_state'] == app_module.App.SetupState.UP_TO_DATE: context['refresh_page_sec'] = 0 elif context['setup_current_operation']: context['refresh_page_sec'] = 3 @@ -306,7 +310,9 @@ class SetupView(TemplateView): # Handle installing/upgrading applications. # Start the application setup, and refresh the page every few # seconds to keep displaying the status. - self.kwargs['setup_helper'].run_in_thread() + app = app_module.App.get(self.kwargs['app_id']) + setup_helper = sys.modules[app.__module__].setup_helper + setup_helper.run_in_thread() # Give a moment for the setup process to start and show # meaningful status. @@ -321,7 +327,7 @@ class SetupView(TemplateView): package.refresh_package_lists() return self.render_to_response(self.get_context_data()) - return super(SetupView, self).dispatch(request, *args, **kwargs) + return super().dispatch(request, *args, **kwargs) @staticmethod def _get_app_package_conflicts(app_):