[fix] Add revision_id filter to RevisionListView and renamed views and serializer for more clarity

Author: dee077

openwisp_controller/config/api/filters.py

@@ -146,10 +146,10 @@ class Meta(BaseConfigAPIFilter.Meta):
 
 
 class ReversionFilter(BaseConfigAPIFilter):
-    model = filters.CharFilter(field_name="content_type__model", lookup_expr="iexact")
+    model = filters.CharFilter(field_name='content_type__model', lookup_expr='iexact')
 
     def _set_valid_filterform_labels(self):
-        self.filters["model"].label = _("Model")
+        self.filters['model'].label = _('Model')
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)

openwisp_controller/config/api/serializers.py

@@ -379,13 +379,13 @@ def update(self, instance, validated_data):
         return instance
 
 
-class ReversionSerializer(BaseSerializer):
-    user_id = serializers.SerializerMethodField()
+class VersionSerializer(BaseSerializer):
+    user_id = serializers.CharField(source='revision.user_id', read_only=True)
     date_created = serializers.DateTimeField(
         source='revision.date_created', read_only=True
     )
     comment = serializers.CharField(source='revision.comment', read_only=True)
-    content_type = serializers.SerializerMethodField()
+    content_type = serializers.CharField(source='revision.content_type', read_only=True)
 
     class Meta:
         model = Version
@@ -402,9 +402,3 @@ class Meta:
             'date_created',
             'comment',
         ]
-
-    def get_user_id(self, obj):
-        return getattr(obj.revision, 'user_id', None)
-
-    def get_content_type(self, obj):
-        return getattr(obj.content_type, 'model', None)

openwisp_controller/config/api/urls.py

@@ -20,8 +20,8 @@ def get_api_urls(api_views):
             ),
             path(
                 'controller/<str:model>/revision/<str:pk>/',
-                api_views.revision_detail,
-                name='revision_detail',
+                api_views.version_detail,
+                name='version_detail',
             ),
             path(
                 'controller/<str:model>/revision/<str:pk>/restore/',

openwisp_controller/config/api/views.py

@@ -1,6 +1,7 @@
 import reversion
 from cache_memoize import cache_memoize
 from django.core.exceptions import ObjectDoesNotExist
+from django.db import transaction
 from django.db.models import F, Q
 from django.http import Http404
 from django.shortcuts import get_list_or_404
@@ -32,8 +33,8 @@
     DeviceDetailSerializer,
     DeviceGroupSerializer,
     DeviceListSerializer,
-    ReversionSerializer,
     TemplateSerializer,
+    VersionSerializer,
     VpnSerializer,
 )
 
@@ -302,53 +303,55 @@ def certificate_delete_invalidates_cache(cls, organization_id, common_name):
         cls.get_device_group.invalidate(cls, org_slug, common_name)
 
 
-class RevisionListView(ProtectedAPIMixin, AutoRevisionMixin, ListAPIView):
-    serializer_class = ReversionSerializer
+class RevisionListView(ProtectedAPIMixin, ListAPIView):
+    serializer_class = VersionSerializer
+    queryset = Version.objects.select_related('revision').order_by(
+        '-revision__date_created'
+    )
 
     def get_queryset(self):
-        model_slug = self.kwargs.get('model').lower()
-        return (
-            Version.objects.select_related('revision')
-            .filter(content_type__model=model_slug)
-            .order_by('-revision__date_created')
-        )
-
-
-class RevisionDetailView(ProtectedAPIMixin, RetrieveAPIView):
-    serializer_class = ReversionSerializer
+        model = self.kwargs.get('model').lower()
+        queryset = self.queryset.filter(content_type__model=model)
+        revision_id = self.request.query_params.get('revision_id')
+        if revision_id:
+            queryset = queryset.filter(revision_id=revision_id)
+        return self.queryset.filter(content_type__model=model)
+
+
+class VersionDetailView(ProtectedAPIMixin, RetrieveAPIView):
+    serializer_class = VersionSerializer
+    queryset = Version.objects.select_related('revision').order_by(
+        '-revision__date_created'
+    )
 
     def get_queryset(self):
-        model_slug = self.kwargs.get('model').lower()
-        return (
-            Version.objects.select_related('revision')
-            .filter(content_type__model=model_slug)
-            .order_by('-revision__date_created')
-        )
+        model = self.kwargs.get('model').lower()
+        return self.queryset.filter(content_type__model=model)
 
 
 class RevisionRestoreView(ProtectedAPIMixin, GenericAPIView):
     serializer_class = serializers.Serializer
+    queryset = Version.objects.select_related('revision').order_by(
+        '-revision__date_created'
+    )
 
     def get_queryset(self):
-        model_slug = self.kwargs.get('model').lower()
-        return (
-            Version.objects.select_related('revision')
-            .filter(content_type__model=model_slug)
-            .order_by('-revision__date_created')
-        )
+        model = self.kwargs.get('model').lower()
+        return self.queryset.filter(content_type__model=model)
 
     def post(self, request, *args, **kwargs):
         qs = self.get_queryset()
         versions = get_list_or_404(qs, revision_id=kwargs['pk'])
-        with reversion.create_revision():
-            for version in versions:
-                version.revert()
-            reversion.set_user(request.user)
-            reversion.set_comment(
-                f"Restored to previous revision: {self.kwargs.get('pk')}"
-            )
-
-        serializer = ReversionSerializer(
+        with transaction.atomic():
+            with reversion.create_revision():
+                for version in versions:
+                    version.revert()
+                reversion.set_user(request.user)
+                reversion.set_comment(
+                    f"Restored to previous revision: {self.kwargs.get('pk')}"
+                )
+
+        serializer = VersionSerializer(
             versions, many=True, context=self.get_serializer_context()
         )
         return Response(serializer.data, status=status.HTTP_200_OK)
@@ -366,5 +369,5 @@ def post(self, request, *args, **kwargs):
 devicegroup_detail = DeviceGroupDetailView.as_view()
 devicegroup_commonname = DeviceGroupCommonName.as_view()
 revision_list = RevisionListView.as_view()
-revision_detail = RevisionDetailView.as_view()
+version_detail = VersionDetailView.as_view()
 revision_restore = RevisionRestoreView.as_view()

openwisp_controller/config/tests/test_api.py

@@ -1580,15 +1580,24 @@ def test_revision_list_and_restore_api(self):
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response.data['name'], 'change-test-device')
 
-        path = reverse('config_api:revision_list', args=[model_slug])
-        response = self.client.get(path)
-        response_json = response.json()
-        version_id = response_json[1]["id"]
-        self.assertEqual(response.status_code, 200)
-        self.assertEqual(len(response_json), 2)
+        with self.subTest('Test revision list'):
+            path = reverse('config_api:revision_list', args=[model_slug])
+            response = self.client.get(path)
+            response_json = response.json()
+            version_id = response_json[1]["id"]
+            revision_id = response_json[1]['revision_id']
+            self.assertEqual(response.status_code, 200)
+            self.assertEqual(len(response_json), 2)
+
+        with self.subTest('Test revision list filter by revision id'):
+            path = reverse('config_api:revision_list', args=[model_slug])
+            response = self.client.get(f'{path}?revision_id={revision_id}')
+            response_json = response.json()
+            self.assertEqual(response.status_code, 200)
+            self.assertEqual(len(response_json), 2)
 
-        with self.subTest('Test revision detail'):
-            path = reverse('config_api:revision_detail', args=[model_slug, version_id])
+        with self.subTest('Test version detail'):
+            path = reverse('config_api:version_detail', args=[model_slug, version_id])
             response = self.client.get(path)
             self.assertEqual(response.status_code, 200)
             self.assertEqual(response.json()['id'], version_id)

openwisp_controller/connection/tests/test_api.py

@@ -458,7 +458,7 @@ def test_post_deviceconnection_list(self):
             'enabled': True,
             'failure_reason': '',
         }
-        with self.assertNumQueries(13):
+        with self.assertNumQueries(23):
             response = self.client.post(path, data, content_type='application/json')
         self.assertEqual(response.status_code, 201)
 
@@ -503,7 +503,7 @@ def test_put_devceconnection_detail(self):
             'enabled': False,
             'failure_reason': '',
         }
-        with self.assertNumQueries(14):
+        with self.assertNumQueries(23):
             response = self.client.put(path, data, content_type='application/json')
         self.assertEqual(response.status_code, 200)
         self.assertEqual(
@@ -517,7 +517,7 @@ def test_patch_deviceconnectoin_detail(self):
         path = reverse('connection_api:deviceconnection_detail', args=(d1, dc.pk))
         self.assertEqual(dc.update_strategy, app_settings.UPDATE_STRATEGIES[0][0])
         data = {'update_strategy': app_settings.UPDATE_STRATEGIES[1][0]}
-        with self.assertNumQueries(13):
+        with self.assertNumQueries(22):
             response = self.client.patch(path, data, content_type='application/json')
         self.assertEqual(response.status_code, 200)
         self.assertEqual(

openwisp_controller/geo/tests/test_api.py

@@ -3,6 +3,7 @@
 import uuid
 
 from django.contrib.auth import get_user_model
+from django.contrib.contenttypes.models import ContentType
 from django.contrib.gis.geos import Point
 from django.test import TestCase
 from django.test.client import BOUNDARY, MULTIPART_CONTENT, encode_multipart
@@ -300,6 +301,7 @@ class TestGeoApi(
     def setUp(self):
         admin = self._create_admin()
         self.client.force_login(admin)
+        ContentType.objects.clear_cache()
 
     def _create_device_location(self, **kwargs):
         options = dict()
@@ -494,7 +496,7 @@ def test_post_location_list(self):
             'address': 'Via del Corso, Roma, Italia',
             'geometry': coords,
         }
-        with self.assertNumQueries(9):
+        with self.assertNumQueries(13):
             response = self.client.post(path, data, content_type='application/json')
         self.assertEqual(response.status_code, 201)
 
@@ -525,7 +527,7 @@ def test_put_location_detail(self):
             'address': 'Via del Corso, Roma, Italia',
             'geometry': coords,
         }
-        with self.assertNumQueries(6):
+        with self.assertNumQueries(10):
             response = self.client.put(path, data, content_type='application/json')
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response.data['organization'], org1.pk)
@@ -536,7 +538,7 @@ def test_patch_location_detail(self):
         self.assertEqual(l1.name, 'test-location')
         path = reverse('geo_api:detail_location', args=[l1.pk])
         data = {'name': 'change-test-location'}
-        with self.assertNumQueries(5):
+        with self.assertNumQueries(9):
             response = self.client.patch(path, data, content_type='application/json')
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response.data['name'], 'change-test-location')
@@ -566,7 +568,7 @@ def test_patch_floorplan_detail_api(self):
         fl = self._create_floorplan(location=l1)
         path = reverse('geo_api:detail_location', args=[l1.pk])
         data = {'floorplan': {'floor': 13}}
-        with self.assertNumQueries(13):
+        with self.assertNumQueries(17):
             response = self.client.patch(path, data, content_type='application/json')
         self.assertEqual(response.status_code, 200)
         fl.refresh_from_db()
@@ -577,7 +579,7 @@ def test_change_location_type_to_outdoor_api(self):
         self._create_floorplan(location=l1)
         path = reverse('geo_api:detail_location', args=[l1.pk])
         data = {'type': 'outdoor'}
-        with self.assertNumQueries(9):
+        with self.assertNumQueries(13):
             response = self.client.patch(path, data, content_type='application/json')
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response.data['floorplan'], [])
@@ -603,7 +605,7 @@ def test_create_location_with_floorplan(self):
             'floorplan.floor': ['23'],
             'floorplan.image': [fl_image],
         }
-        with self.assertNumQueries(16):
+        with self.assertNumQueries(20):
             response = self.client.post(path, data, format='multipart')
         self.assertEqual(response.status_code, 201)
         self.assertEqual(Location.objects.count(), 1)
@@ -627,7 +629,7 @@ def test_create_new_floorplan_with_put_location_api(self):
             'floorplan.floor': '23',
             'floorplan.image': fl_image,
         }
-        with self.assertNumQueries(16):
+        with self.assertNumQueries(20):
             response = self.client.put(
                 path, encode_multipart(BOUNDARY, data), content_type=MULTIPART_CONTENT
             )
@@ -695,7 +697,7 @@ def test_create_devicelocation_using_related_ids(self):
         floorplan = self._create_floorplan()
         location = floorplan.location
         url = reverse('geo_api:device_location', args=[device.id])
-        with self.assertNumQueries(15):
+        with self.assertNumQueries(26):
             response = self.client.put(
                 url,
                 data={
@@ -733,7 +735,7 @@ def test_create_devicelocation_location_floorplan(self):
             'floorplan.image': self._get_simpleuploadedfile(),
             'indoor': ['12.342,23.541'],
         }
-        with self.assertNumQueries(29):
+        with self.assertNumQueries(40):
             response = self.client.put(
                 url, encode_multipart(BOUNDARY, data), content_type=MULTIPART_CONTENT
             )
@@ -800,7 +802,7 @@ def test_create_devicelocation_only_location(self):
                 'type': 'indoor',
             }
         }
-        with self.assertNumQueries(18):
+        with self.assertNumQueries(29):
             response = self.client.put(url, data=data, content_type='application/json')
         self.assertEqual(response.status_code, 201)
         self.assertEqual(self.location_model.objects.count(), 1)
@@ -840,7 +842,7 @@ def test_create_devicelocation_existing_location_new_floorplan(self):
             'floorplan.image': self._get_simpleuploadedfile(),
             'indoor': ['12.342,23.541'],
         }
-        with self.assertNumQueries(23):
+        with self.assertNumQueries(34):
             response = self.client.put(
                 url, encode_multipart(BOUNDARY, data), content_type=MULTIPART_CONTENT
             )
@@ -863,7 +865,7 @@ def test_update_devicelocation_change_location_outdoor_to_indoor(self):
         }
         self.assertEqual(device_location.location.type, 'outdoor')
         self.assertEqual(device_location.floorplan, None)
-        with self.assertNumQueries(21):
+        with self.assertNumQueries(31):
             response = self.client.put(
                 path, encode_multipart(BOUNDARY, data), content_type=MULTIPART_CONTENT
             )
@@ -882,7 +884,7 @@ def test_update_devicelocation_patch_indoor(self):
             'indoor': '0,0',
         }
         self.assertEqual(device_location.indoor, '-140.38620,40.369227')
-        with self.assertNumQueries(10):
+        with self.assertNumQueries(18):
             response = self.client.patch(path, data, content_type='application/json')
         self.assertEqual(response.status_code, 200)
         device_location.refresh_from_db()
@@ -899,7 +901,7 @@ def test_update_devicelocation_floorplan_related_id(self):
         data = {
             'floorplan': str(floor2.id),
         }
-        with self.assertNumQueries(12):
+        with self.assertNumQueries(20):
             response = self.client.patch(path, data, content_type='application/json')
         self.assertEqual(response.status_code, 200)
         device_location.refresh_from_db()
@@ -913,7 +915,7 @@ def test_update_devicelocation_location_related_id(self):
         data = {
             'location': str(location2.id),
         }
-        with self.assertNumQueries(9):
+        with self.assertNumQueries(19):
             response = self.client.patch(path, data, content_type='application/json')
         self.assertEqual(response.status_code, 200)
         device_location.refresh_from_db()