[feature] Added configuration error reason openwisp#628

Closes openwisp#628

openwisp_controller/config/admin.py

@@ -420,6 +420,20 @@ def get_queryset(self, request):
         qs = super().get_queryset(request)
         return qs.select_related(*self.change_select_related)
 
+    def _error_reason_field_conditional(self, obj, fields):
+        if obj and obj.status == 'error' and 'error_reason' not in fields:
+            fields = fields.copy()
+            fields.insert(fields.index('status') + 1, 'error_reason')
+        return fields
+
+    def get_readonly_fields(self, request, obj):
+        fields = super().get_readonly_fields(request, obj)
+        return self._error_reason_field_conditional(obj, fields)
+
+    def get_fields(self, request, obj):
+        fields = super().get_fields(request, obj)
+        return self._error_reason_field_conditional(obj, fields)
+
 
 class ChangeDeviceGroupForm(forms.Form):
     device_group = forms.ModelChoiceField(

openwisp_controller/config/api/serializers.py

@@ -108,8 +108,11 @@ def get_queryset(self):
 class BaseConfigSerializer(serializers.ModelSerializer):
     class Meta:
         model = Config
-        fields = ['status', 'backend', 'templates', 'context', 'config']
-        extra_kwargs = {'status': {'read_only': True}}
+        fields = ['status', 'error_reason', 'backend', 'templates', 'context', 'config']
+        extra_kwargs = {
+            'status': {'read_only': True},
+            'error_reason': {'read_only': True},
+        }
 
 
 class DeviceListConfigSerializer(BaseConfigSerializer):

openwisp_controller/config/base/config.py

@@ -71,6 +71,12 @@ class AbstractConfig(BaseConfig):
             '"error" means the configuration caused issues and it was rolled back;'
         ),
     )
+    error_reason = models.CharField(
+        _('error reason'),
+        max_length=1024,
+        help_text=_('Error reason reported by the device'),
+        blank=True,
+    )
     context = JSONField(
         blank=True,
         default=dict,
@@ -419,6 +425,16 @@ def get_backend_instance(self, template_instances=None, context=None, **kwargs):
             kwargs['dsa'] = dsa_enabled
         return super().get_backend_instance(template_instances, context, **kwargs)
 
+    def clean_error_reason(self):
+        if len(self.error_reason) > 1024:
+            self.error_reason = f'{self.error_reason[:1012]}\n[truncated]'
+
+    def full_clean(self, exclude=None, validate_unique=True):
+        # Modify the "error_reason" before the field validation
+        # is executed by self.full_clean
+        self.clean_error_reason()
+        return super().full_clean(exclude, validate_unique)
+
     def clean(self):
         """
         * validates context field
@@ -515,11 +531,18 @@ def _send_config_status_changed_signal(self):
         """
         config_status_changed.send(sender=self.__class__, instance=self)
 
-    def _set_status(self, status, save=True):
-        self.status = status
+    def _set_status(self, status, save=True, reason=None):
         self._send_config_status_changed = True
+        update_fields = ['status']
+        # The error reason should be updated when
+        # 1. the configuration is in "error" status
+        # 2. the configuration has changed from error status
+        if reason or (self.status == 'error' and self.status != status):
+            self.error_reason = reason or ''
+            update_fields.append('error_reason')
+        self.status = status
         if save:
-            self.save(update_fields=['status'])
+            self.save(update_fields=update_fields)
 
     def set_status_modified(self, save=True, send_config_modified_signal=True):
         if send_config_modified_signal:
@@ -529,8 +552,8 @@ def set_status_modified(self, save=True, send_config_modified_signal=True):
     def set_status_applied(self, save=True):
         self._set_status('applied', save)
 
-    def set_status_error(self, save=True):
-        self._set_status('error', save)
+    def set_status_error(self, save=True, reason=None):
+        self._set_status('error', save, reason)
 
     def _has_device(self):
         return hasattr(self, 'device')

openwisp_controller/config/controller/views.py

@@ -9,6 +9,7 @@
 from django.db import transaction
 from django.db.models import Q
 from django.utils.decorators import method_decorator
+from django.utils.translation import gettext_lazy as _
 from django.views.decorators.csrf import csrf_exempt
 from django.views.generic.base import View
 from django.views.generic.detail import SingleObjectMixin
@@ -247,7 +248,13 @@ def post(self, request, *args, **kwargs):
         status = status if status != 'running' else 'applied'
         # call set_status_{status} method on Config model
         method_name = f'set_status_{status}'
-        getattr(config, method_name)()
+        if status == 'error':
+            error_reason = request.POST.get(
+                'error_reason', _('Reason not reported by the device.')
+            )
+            getattr(config, method_name)(reason=error_reason)
+        else:
+            getattr(config, method_name)()
         return ControllerResponse(
             f'report-result: success\ncurrent-status: {config.status}\n',
             content_type='text/plain',

openwisp_controller/config/migrations/0044_config_error_reason.py

@@ -0,0 +1,23 @@
+# Generated by Django 4.0.3 on 2022-07-07 16:14
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('config', '0043_devicegroup_templates'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='config',
+            name='error_reason',
+            field=models.CharField(
+                blank=True,
+                help_text='Error reason reported by the device',
+                max_length=1024,
+                verbose_name='error reason',
+            ),
+        ),
+    ]

openwisp_controller/config/static/config/css/admin.css

@@ -313,6 +313,15 @@ button.show-sc{
   margin-left: 7px;
   opacity: 0.8;
 }
+.form-row.field-error_reason .readonly {
+  font-family: monospace;
+  line-height: 1.5em;
+  white-space: pre;
+  max-height: 10em;
+  padding: 0.5em;
+  overflow: auto;
+  background-color: #ffe5e582 ;
+}
 
 @media (max-width: 767px) {
   ul.tabs li {

openwisp_controller/config/tests/test_admin.py

@@ -975,6 +975,35 @@ def test_preview_device_405(self):
         response = self.client.get(path, {})
         self.assertEqual(response.status_code, 405)
 
+    def test_config_error_reason(self):
+        device = self._create_device(name='download')
+        config = self._create_config(device=device)
+        url = reverse(f'admin:{self.app_label}_device_change', args=[device.pk])
+
+        with self.subTest('Test config status "modified" or "applied"'):
+            self.assertEqual(config.status, 'modified')
+            response = self.client.get(url)
+            self.assertNotContains(response, '<label>Error reason:</label>', html=True)
+            config.set_status_applied()
+            response = self.client.get(url)
+            self.assertNotContains(response, '<label>Error reason:</label>', html=True)
+
+        with self.subTest('Test config status "error"'):
+            config.set_status_error(reason='Reason not reported by the device.')
+            response = self.client.get(url)
+            self.assertContains(response, '<label>Error reason:</label>', html=True)
+            self.assertContains(
+                response,
+                '<div class="readonly">Reason not reported by the device.</div>',
+                html=True,
+            )
+
+        with self.subTest('Test regression status "applied" after "error"'):
+            config.set_status_applied()
+            self.assertEqual(config.status, 'applied')
+            response = self.client.get(url)
+            self.assertNotContains(response, '<label>Error reason:</label>', html=True)
+
     def test_download_template_config(self):
         t = Template.objects.first()
         path = reverse(f'admin:{self.app_label}_template_download', args=[t.pk])

openwisp_controller/config/tests/test_config.py

@@ -59,6 +59,35 @@ def test_backend_instance(self):
         c = Config(backend='netjsonconfig.OpenWrt', config=config)
         self.assertIsInstance(c.backend_instance, OpenWrt)
 
+    def test_error_reason_clean(self):
+        config = self._create_config(organization=self._get_org())
+        config.error_reason = 'e' * 1030
+        config.full_clean()
+        self.assertEqual(len(config.error_reason), 1024)
+        self.assertEqual(config.error_reason[1013:], '[truncated]')
+
+    def test_error_reason_status_error_modified(self):
+        error_reason = 'Configuration cannot be applied.'
+        config = self._create_config(organization=self._get_org())
+        self.assertEqual(config.status, 'modified')
+        self.assertEqual(config.error_reason, '')
+
+        with self.subTest('Test configuration status changes to modified'):
+            config.set_status_error(reason=error_reason)
+            self.assertEqual(config.status, 'error')
+            self.assertEqual(config.error_reason, error_reason)
+            config.set_status_modified()
+            self.assertEqual(config.status, 'modified')
+            self.assertEqual(config.error_reason, '')
+
+        with self.subTest('Test configuration status changes to applied'):
+            config.set_status_error(reason=error_reason)
+            self.assertEqual(config.status, 'error')
+            self.assertEqual(config.error_reason, error_reason)
+            config.set_status_applied()
+            self.assertEqual(config.status, 'applied')
+            self.assertEqual(config.error_reason, '')
+
     @patch.object(app_settings, 'DSA_DEFAULT_FALLBACK', False)
     @patch.object(
         app_settings,

openwisp_controller/config/tests/test_controller.py

@@ -648,18 +648,41 @@ def test_device_report_status_applied(self):
 
     def test_device_report_status_error(self):
         d = self._create_device_config()
-        with catch_signal(config_status_changed) as handler:
-            response = self.client.post(
-                reverse('controller:device_report_status', args=[d.pk]),
-                {'key': d.key, 'status': 'error'},
-            )
+        url = reverse('controller:device_report_status', args=[d.pk])
+        with self.subTest('Test without error reason'):
+            with catch_signal(config_status_changed) as handler:
+                response = self.client.post(
+                    url,
+                    {'key': d.key, 'status': 'error'},
+                )
+                d.config.refresh_from_db()
+                handler.assert_called_once_with(
+                    sender=Config, signal=config_status_changed, instance=d.config
+                )
+            self._check_header(response)
             d.config.refresh_from_db()
-            handler.assert_called_once_with(
-                sender=Config, signal=config_status_changed, instance=d.config
+            self.assertEqual(d.config.status, 'error')
+
+        with self.subTest('Test with error reason'):
+            error_reason = (
+                'daemon.crit openwisp: Could not apply configuration,'
+                ' openwisp-update-config exit code was 1\n'
+                'daemon.info openwisp: The most recent configuration'
+                ' backup was restored'
             )
-        self._check_header(response)
-        d.config.refresh_from_db()
-        self.assertEqual(d.config.status, 'error')
+            with catch_signal(config_status_changed) as handler:
+                response = self.client.post(
+                    url,
+                    {'key': d.key, 'status': 'error', 'error_reason': error_reason},
+                )
+                d.config.refresh_from_db()
+                handler.assert_called_once_with(
+                    sender=Config, signal=config_status_changed, instance=d.config
+                )
+            self._check_header(response)
+            d.config.refresh_from_db()
+            self.assertEqual(d.config.status, 'error')
+            self.assertEqual(d.config.error_reason, error_reason)
 
     def test_device_report_status_bad_uuid(self):
         d = self._create_device_config()

tests/openwisp2/sample_config/migrations/0001_initial.py

@@ -109,6 +109,15 @@ class Migration(migrations.Migration):
                         verbose_name='configuration status',
                     ),
                 ),
+                (
+                    'error_reason',
+                    models.CharField(
+                        blank=True,
+                        help_text='Error reason reported by the device',
+                        max_length=1024,
+                        verbose_name='error reason',
+                    ),
+                ),
                 (
                     'context',
                     jsonfield.fields.JSONField(