middleware, views: Reduce use of setup_helper

Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>
Reviewed-by: James Valleroy <jvalleroy@mailbox.org>

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]
+        app_id = resolver_match.namespaces[0]
-        module = plinth.module_loader.loaded_modules[module_name]
+        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):

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,
+    def test_module_is_up_to_date(_reverse, resolve, setup_helper_, app,
-                                  kwargs):
+                                  middleware, kwargs):
         """Test that none is returned when module is up-to-date."""
         resolve.return_value.namespaces = ['mockapp']
-        module = Mock()
+        setup_helper_.is_finished = None
-        module.setup_helper.is_finished = None
+        app.get_setup_state = lambda: app_module.App.SetupState.UP_TO_DATE
-        module.app.get_setup_state.return_value = \
-            app_module.App.SetupState.UP_TO_DATE
-        loaded_modules.__getitem__.return_value = module
 
         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,
+    def test_install_result_collection(reverse, resolve, messages_success,
-                                       messages_success, middleware, kwargs):
+                                       setup_helper_, app, middleware, kwargs):
         """Test that module installation result is collected properly."""
         resolve.return_value.namespaces = ['mockapp']
-        module = Mock()
+        setup_helper_.is_finished = True
-        module.setup_helper.is_finished = True
+        setup_helper_.collect_result.return_value = None
-        module.setup_helper.collect_result.return_value = None
+        app.get_setup_state = lambda: app_module.App.SetupState.UP_TO_DATE
-        module.app.get_setup_state.return_value = \
-            app_module.App.SetupState.UP_TO_DATE
-        loaded_modules.__getitem__.return_value = module
 
         # 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:

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)
+        context = super().get_context_data(**kwargs)
-        setup_helper = self.kwargs['setup_helper']
+        app_id = self.kwargs['app_id']
-        context['setup_helper'] = setup_helper
+        app = app_module.App.get(app_id)
-        context['app_info'] = setup_helper.module.app.info
+
+        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_):