diagnostics: Improve exception handling in app diagnostics

Fixes the problem when the app throws an error when running diagnostics,
the app's diagnostics page shows an html 500 error and on the diagnostics
app page, only an excpetion message is shown without other apps diagnostics.
(I encountered this when the network app diagnostics threw an exception
because of a particular network configuration.)

Tests performed:
- Checked that an app diagnostics page and the diagnostics app shows
all info correctly.
- Modified the networks app diagnostics code so that it throws an exception.
Checked that the app diagnostics page shows an exception message and
the diagnostics module shows all the apps and the networks app section
shows an exception message.

Signed-off-by: Veiko Aasa <veiko17@disroot.org>
[sunil: Re-add removed relevant comment]
[sunil: Remove ability to dismiss the exception message, not much gain for user]
[sunil: Remove unnecessary redundant if condition in template]
Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>
Reviewed-by: Sunil Mohan Adapa <sunil@medhas.org>

plinth/modules/diagnostics/__init__.py

@@ -78,22 +78,10 @@ def start_task():
     if running_task:
         raise Exception('Task already running')
 
-    running_task = threading.Thread(target=_run_on_all_enabled_modules_wrapper)
+    running_task = threading.Thread(target=run_on_all_enabled_modules)
     running_task.start()
 
 
-def _run_on_all_enabled_modules_wrapper():
-    """Wrapper over actual task to catch exceptions."""
-    try:
-        run_on_all_enabled_modules()
-    except Exception as exception:
-        logger.exception('Error running diagnostics - %s', exception)
-        current_results['error'] = str(exception)
-
-    global running_task
-    running_task = None
-
-
 def run_on_all_enabled_modules():
     """Run diagnostics on all the enabled modules and store the result."""
     global current_results
@@ -111,7 +99,6 @@ def run_on_all_enabled_modules():
 
     apps = []
     for app in app_module.App.list():
-        # XXX: Implement more cleanly.
         # Don't run diagnostics on apps have not been setup yet.
         # However, run on apps that need an upgrade.
         module = importlib.import_module(app.__class__.__module__)
@@ -125,18 +112,30 @@ def run_on_all_enabled_modules():
             continue
 
         apps.append((app.app_id, app))
-        current_results['results'][app.app_id] = None
+        app_name = app.info.name or _('None')
+        current_results['results'][app.app_id] = {'name': app_name}
 
     current_results['apps'] = apps
     for current_index, (app_id, app) in enumerate(apps):
-        app_name = app.info.name or _('None')
-        current_results['results'][app_id] = {
-            'name': app_name,
-            'results': app.diagnose()
+        app_results = {
+            'diagnosis': None,
+            'exception': None,
         }
+
+        try:
+            app_results['diagnosis'] = app.diagnose()
+        except Exception as exception:
+            logger.exception('Error running %s diagnostics - %s', app_id,
+                             exception)
+            app_results['exception'] = str(exception)
+
+        current_results['results'][app_id].update(app_results)
         current_results['progress_percentage'] = \
             int((current_index + 1) * 100 / len(apps))
 
+    global running_task
+    running_task = None
+
 
 def _get_memory_info_from_cgroups():
     """Return information about RAM usage from cgroups."""

plinth/modules/diagnostics/templates/diagnostics.html

@@ -31,26 +31,23 @@
 
   {% if results %}
     <h3>{% trans "Results" %}</h3>
-    {% if results.error %}
-      <div class="alert alert-danger alert-dismissable">
-        <a class="close" data-dismiss="alert">&times;</a>
-        {{ results.error }}
-      </div>
-    {% else %}
-      {% for app_id, app_data in results.results.items %}
-        <h4>
-          {% blocktrans with app_name=app_data.name %}
-            App: {{app_name}}
-          {% endblocktrans %}
-        </h4>
+    {% for app_id, app_data in results.results.items %}
+      <h4>
+        {% blocktrans with app_name=app_data.name %}
+          App: {{app_name}}
+        {% endblocktrans %}
+      </h4>
 
-        {% if app_data.results %}
-          {% include "diagnostics_results.html" with results=app_data.results %}
-        {% else %}
-          <p><span class="fa fa-hourglass-o"></span></p>
-        {% endif %}
-      {% endfor %}
-    {% endif %}
+      {% if app_data.diagnosis %}
+        {% include "diagnostics_results.html" with results=app_data.diagnosis %}
+      {% elif app_data.exception %}
+        <div class="alert alert-danger" role="alert">
+          {{ app_data.exception }}
+        </div>
+      {% else %}
+        <p><span class="fa fa-hourglass-o"></span></p>
+      {% endif %}
+    {% endfor %}
   {% endif %}
 
 {% endblock %}

plinth/modules/diagnostics/templates/diagnostics_app.html

@@ -13,6 +13,10 @@
 
   {% if results %}
     {% include "diagnostics_results.html" with results=results %}
+  {% elif exception %}
+    <div class="alert alert-danger" role="alert">
+      {{ exception }}
+    </div>
   {% else %}
     <p>{% trans "This app does not support diagnostics" %}</p>
   {% endif %}

plinth/modules/diagnostics/views.py

@@ -3,6 +3,8 @@
 FreedomBox app for running diagnostics.
 """
 
+import logging
+
 from django.http import Http404
 from django.template.response import TemplateResponse
 from django.utils.translation import ugettext_lazy as _
@@ -11,6 +13,8 @@ from django.views.decorators.http import require_POST
 from plinth.app import App
 from plinth.modules import diagnostics
 
+logger = logging.getLogger(__name__)
+
 
 def index(request):
     """Serve the index page"""
@@ -35,8 +39,19 @@ def diagnose_app(request, app_id):
     except KeyError:
         raise Http404('App does not exist')
 
-    return TemplateResponse(request, 'diagnostics_app.html', {
-        'title': _('Diagnostic Test'),
-        'app_id': app_id,
-        'results': app.diagnose()
-    })
+    diagnosis = None
+    diagnosis_exception = None
+    try:
+        diagnosis = app.diagnose()
+    except Exception as exception:
+        logger.exception('Error running %s diagnostics - %s', app_id,
+                         exception)
+        diagnosis_exception = str(exception)
+
+    return TemplateResponse(
+        request, 'diagnostics_app.html', {
+            'title': _('Diagnostic Test'),
+            'app_id': app_id,
+            'results': diagnosis,
+            'exception': diagnosis_exception,
+        })