Merge branch 'master' into feature/894-rest-api-revisions

Author: nemesifier

.github/workflows/ci.yml

@@ -87,9 +87,10 @@ jobs:
     - name: Tests
       if: ${{ !cancelled() && steps.deps.conclusion == 'success' }}
       run: |
-        coverage run runtests.py --parallel
+        coverage run runtests.py --parallel || coverage run ./runtests.py
         # tests the extension capability
-        SAMPLE_APP=1 coverage run ./runtests.py --parallel --keepdb --exclude-tag=selenium_tests
+        SAMPLE_APP=1 coverage run ./runtests.py --parallel --keepdb --exclude-tag=selenium_tests \
+          || SAMPLE_APP=1 coverage run ./runtests.py --keepdb --exclude-tag=selenium_tests
         coverage combine
         coverage xml
       env:

openwisp_controller/config/admin.py

@@ -26,6 +26,7 @@
 from django.utils.translation import ngettext_lazy
 from flat_json_widget.widgets import FlatJsonWidget
 from import_export.admin import ImportExportMixin
+from import_export.forms import ExportForm
 from openwisp_ipam.filters import SubnetFilter
 from swapper import load_model
 
@@ -971,6 +972,7 @@ def add_reversion_following(cls, follow):
 
 
 class DeviceAdminExportable(ImportExportMixin, DeviceAdmin):
+    export_form_class = ExportForm
     resource_class = DeviceResource
     # needed to support both reversion and import-export
     change_list_template = 'admin/config/change_list_device.html'

openwisp_controller/config/api/serializers.py

@@ -1,5 +1,3 @@
-from copy import deepcopy
-
 from django.core.exceptions import ValidationError
 from django.db import transaction
 from django.db.models import Q
@@ -8,9 +6,9 @@
 from reversion.models import Version
 from swapper import load_model
 
-from openwisp_users.api.mixins import FilterSerializerByOrgManaged
 from openwisp_utils.api.serializers import ValidatedModelSerializer
 
+from ...serializers import BaseSerializer
 from .. import settings as app_settings
 
 Template = load_model('config', 'Template')
@@ -25,10 +23,6 @@ class BaseMeta:
     read_only_fields = ['created', 'modified']
 
 
-class BaseSerializer(FilterSerializerByOrgManaged, ValidatedModelSerializer):
-    pass
-
-
 class TemplateSerializer(BaseSerializer):
     config = serializers.JSONField(initial={}, required=False)
     tags = serializers.StringRelatedField(many=True, read_only=True)
@@ -111,7 +105,12 @@ def get_queryset(self):
         return queryset
 
 
-class BaseConfigSerializer(serializers.ModelSerializer):
+class BaseConfigSerializer(ValidatedModelSerializer):
+    # The device object is excluded from validation
+    # because this serializer focuses on validating
+    # config objects.
+    exclude_validation = ['device']
+
     class Meta:
         model = Config
         fields = ['status', 'error_reason', 'backend', 'templates', 'context', 'config']
@@ -120,8 +119,26 @@ class Meta:
             'error_reason': {'read_only': True},
         }
 
+    def validate(self, data):
+        """
+        The validation here is a bit tricky:
+
+        For existing devices, we have to perform the
+        model validation of the existing config object,
+        because if we simulate the validation on a new
+        config object pointing to an existing device,
+        the validation will fail because a config object
+        for this device already exists (due to one-to-one relationship).
+        """
+        device = self.context.get('device')
+        if not self.instance and device:
+            # Existing device with existing config
+            if device._has_config():
+                self.instance = device.config
+        return super().validate(data)
+
 
-class DeviceConfigMixin(object):
+class DeviceConfigSerializer(BaseSerializer):
     def _get_config_templates(self, config_data):
         return [template.pk for template in config_data.pop('templates', [])]
 
@@ -132,11 +149,29 @@ def _prepare_config(self, device, config_data):
         config.full_clean()
         return config
 
+    def _is_config_data_relevant(self, config_data):
+        """
+        Returns True if ``config_data`` does not equal
+        the default values and hence the config is useful.
+        """
+        return not (
+            config_data.get('backend') == app_settings.DEFAULT_BACKEND
+            and not config_data.get('templates')
+            and not config_data.get('context')
+            and not config_data.get('config')
+        )
+
     @transaction.atomic
     def _create_config(self, device, config_data):
         config_templates = self._get_config_templates(config_data)
         try:
             if not device._has_config():
+                # if the user hasn't set any useful config data, skip
+                if (
+                    not self._is_config_data_relevant(config_data)
+                    and not config_templates
+                ):
+                    return
                 config = Config(device=device, **config_data)
                 config.full_clean()
                 config.save()
@@ -152,15 +187,10 @@ def _create_config(self, device, config_data):
             raise serializers.ValidationError({'config': error.messages})
 
     def _update_config(self, device, config_data):
-        if (
-            config_data.get('backend') == app_settings.DEFAULT_BACKEND
-            and not config_data.get('templates')
-            and not config_data.get('context')
-            and not config_data.get('config')
-        ):
-            # Do not create Config object if config_data only
-            # contains the default value.
-            # See https://github.com/openwisp/openwisp-controller/issues/699
+        # Do not create Config object if config_data only
+        # contains the default values.
+        # See https://github.com/openwisp/openwisp-controller/issues/699
+        if not self._is_config_data_relevant(config_data):
             return
         if not device._has_config():
             return self._create_config(device, config_data)
@@ -191,9 +221,7 @@ class DeviceListConfigSerializer(BaseConfigSerializer):
     templates = FilterTemplatesByOrganization(many=True, write_only=True)
 
 
-class DeviceListSerializer(
-    DeviceConfigMixin, FilterSerializerByOrgManaged, serializers.ModelSerializer
-):
+class DeviceListSerializer(DeviceConfigSerializer):
     config = DeviceListConfigSerializer(required=False)
 
     class Meta(BaseMeta):
@@ -220,14 +248,13 @@ class Meta(BaseMeta):
             'management_ip': {'allow_blank': True},
         }
 
-    def validate(self, attrs):
-        device_data = deepcopy(attrs)
+    def validate(self, data):
         # Validation of "config" is performed after
         # device object is created in the "create" method.
-        device_data.pop('config', None)
-        device = self.instance or self.Meta.model(**device_data)
-        device.full_clean()
-        return attrs
+        config_data = data.pop('config', None)
+        data = super().validate(data)
+        data['config'] = config_data
+        return data
 
     def create(self, validated_data):
         config_data = validated_data.pop('config', None)
@@ -248,7 +275,7 @@ class DeviceDetailConfigSerializer(BaseConfigSerializer):
     templates = FilterTemplatesByOrganization(many=True)
 
 
-class DeviceDetailSerializer(DeviceConfigMixin, BaseSerializer):
+class DeviceDetailSerializer(DeviceConfigSerializer):
     config = DeviceDetailConfigSerializer(allow_null=True)
     is_deactivated = serializers.BooleanField(read_only=True)
 
@@ -281,7 +308,7 @@ def update(self, instance, validated_data):
         if config_data:
             self._update_config(instance, config_data)
 
-        elif hasattr(instance, 'config') and validated_data.get('organization'):
+        elif instance._has_config() and validated_data.get('organization'):
             if instance.organization != validated_data.get('organization'):
                 # config.device.organization is used for validating
                 # the organization of templates. It is also used for adding

openwisp_controller/config/api/urls.py

@@ -34,12 +34,12 @@ def get_api_urls(api_views):
                 name='template_list',
             ),
             path(
-                'controller/template/<str:pk>/',
+                'controller/template/<uuid:pk>/',
                 api_views.template_detail,
                 name='template_detail',
             ),
             path(
-                'controller/template/<str:pk>/configuration/',
+                'controller/template/<uuid:pk>/configuration/',
                 api_download_views.download_template_config,
                 name='download_template_config',
             ),
@@ -49,12 +49,12 @@ def get_api_urls(api_views):
                 name='vpn_list',
             ),
             path(
-                'controller/vpn/<str:pk>/',
+                'controller/vpn/<uuid:pk>/',
                 api_views.vpn_detail,
                 name='vpn_detail',
             ),
             path(
-                'controller/vpn/<str:pk>/configuration/',
+                'controller/vpn/<uuid:pk>/configuration/',
                 api_download_views.download_vpn_config,
                 name='download_vpn_config',
             ),
@@ -64,17 +64,17 @@ def get_api_urls(api_views):
                 name='device_list',
             ),
             path(
-                'controller/device/<str:pk>/',
+                'controller/device/<uuid:pk>/',
                 api_views.device_detail,
                 name='device_detail',
             ),
             path(
-                'controller/device/<str:pk>/activate/',
+                'controller/device/<uuid:pk>/activate/',
                 api_views.device_activate,
                 name='device_activate',
             ),
             path(
-                'controller/device/<str:pk>/deactivate/',
+                'controller/device/<uuid:pk>/deactivate/',
                 api_views.device_deactivate,
                 name='device_deactivate',
             ),
@@ -84,7 +84,7 @@ def get_api_urls(api_views):
                 name='devicegroup_list',
             ),
             path(
-                'controller/group/<str:pk>/',
+                'controller/group/<uuid:pk>/',
                 api_views.devicegroup_detail,
                 name='devicegroup_detail',
             ),
@@ -94,7 +94,7 @@ def get_api_urls(api_views):
                 name='devicegroup_x509_commonname',
             ),
             path(
-                'controller/device/<str:pk>/configuration/',
+                'controller/device/<uuid:pk>/configuration/',
                 api_download_views.download_device_config,
                 name='download_device_config',
             ),

openwisp_controller/config/api/views.py

@@ -120,6 +120,18 @@ def perform_destroy(self, instance):
         force_deletion = self.request.query_params.get('force', None) == 'true'
         instance.delete(check_deactivated=(not force_deletion))
 
+    def get_object(self):
+        """Set device property for serializer context."""
+        obj = super().get_object()
+        self.device = obj
+        return obj
+
+    def get_serializer_context(self):
+        """Add device to serializer context for validation purposes."""
+        context = super().get_serializer_context()
+        context['device'] = self.device
+        return context
+
 
 class DeviceActivateView(ProtectedAPIMixin, AutoRevisionMixin, GenericAPIView):
     serializer_class = serializers.Serializer

openwisp_controller/config/base/config.py

@@ -469,13 +469,13 @@ def clean(self):
         * modifies status if key attributes of the configuration
           have changed (queries the database)
         """
-        super().clean()
         if not self.context:
             self.context = {}
         if not isinstance(self.context, dict):
             raise ValidationError(
                 {'context': _('the supplied value is not a JSON object')}
             )
+        super().clean()
 
     def save(self, *args, **kwargs):
         created = self._state.adding

openwisp_controller/config/exportable.py

@@ -127,10 +127,10 @@ def validate_instance(
         if config.device_id != instance.id:
             config.device_id = instance.id
 
-    def after_save_instance(self, instance, using_transactions, dry_run):
-        super().after_save_instance(instance, using_transactions, dry_run)
-        if not dry_run:
-            # save config afte device has been imported
+    def after_save_instance(self, instance, row, **kwargs):
+        super().after_save_instance(instance, row, **kwargs)
+        if not self._is_dry_run(kwargs):
+            # save config after device has been imported
             if instance._has_config():
                 instance.config.save()
 

openwisp_controller/config/static/config/js/unsaved_changes.js

@@ -11,6 +11,7 @@
             "config-0-config",
             "config-0-context",
             "devicelocation-0-geometry",
+            "geometry",
           ];
         // ignore fields that have no name attribute, begin with "_" or "initial-"
         if (

openwisp_controller/config/tests/test_admin.py

@@ -90,7 +90,7 @@ def test_device_import_export_buttons(self):
 
     def test_device_export(self):
         response = self.client.post(
-            reverse(f'admin:{self.app_label}_device_export'), {'file_format': '0'}
+            reverse(f'admin:{self.app_label}_device_export'), {'format': '0'}
         )
         self.assertNotContains(response, 'error')
         self.assertIsNotNone(response.get('Content-Disposition'))
@@ -108,7 +108,7 @@ def test_device_import(self):
         csv = ContentFile(contents)
         response = self.client.post(
             reverse(f'admin:{self.app_label}_device_import'),
-            {'input_format': '0', 'import_file': csv},
+            {'format': '0', 'import_file': csv},
         )
         self.assertNotContains(response, 'errorlist')
         self.assertNotContains(response, 'Errors')
@@ -132,7 +132,7 @@ def test_device_import_empty_config(self):
         csv = ContentFile(contents)
         response = self.client.post(
             reverse(f'admin:{self.app_label}_device_import'),
-            {'input_format': '0', 'import_file': csv, 'file_name': 'test.csv'},
+            {'format': '0', 'import_file': csv, 'file_name': 'test.csv'},
         )
         self.assertFalse(response.context['result'].has_errors())
         self.assertIn('confirm_form', response.context)
@@ -173,7 +173,7 @@ def test_device_import_missing_config(self):
         csv = ContentFile(contents)
         response = self.client.post(
             reverse(f'admin:{self.app_label}_device_import'),
-            {'input_format': '0', 'import_file': csv, 'file_name': 'test.csv'},
+            {'format': '0', 'import_file': csv, 'file_name': 'test.csv'},
         )
         self.assertFalse(response.context['result'].has_errors())
         self.assertIn('confirm_form', response.context)
@@ -212,7 +212,7 @@ def test_device_import_config_not_templates(self):
         csv = ContentFile(contents)
         response = self.client.post(
             reverse(f'admin:{self.app_label}_device_import'),
-            {'input_format': '0', 'import_file': csv, 'file_name': 'test.csv'},
+            {'format': '0', 'import_file': csv, 'file_name': 'test.csv'},
         )
         self.assertFalse(response.context['result'].has_errors())
         self.assertIn('confirm_form', response.context)
@@ -636,7 +636,7 @@ def test_device_import_with_group_apply_templates(self):
         csv = ContentFile(contents)
         response = self.client.post(
             reverse(f'admin:{self.app_label}_device_import'),
-            {'input_format': '0', 'import_file': csv, 'file_name': 'test.csv'},
+            {'format': '0', 'import_file': csv, 'file_name': 'test.csv'},
         )
         self.assertFalse(response.context['result'].has_errors())
         self.assertIn('confirm_form', response.context)
@@ -682,7 +682,7 @@ def test_device_import_templates_and_config(self):
         csv = ContentFile(contents)
         response = self.client.post(
             reverse(f'admin:{self.app_label}_device_import'),
-            {'input_format': '0', 'import_file': csv, 'file_name': 'test.csv'},
+            {'format': '0', 'import_file': csv, 'file_name': 'test.csv'},
         )
         self.assertFalse(response.context['result'].has_errors())
         self.assertIn('confirm_form', response.context)