-
-
Notifications
You must be signed in to change notification settings - Fork 218
[feature] Add support to reversion to the REST API #994
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
80288b6
675f162
dd5c232
73d5145
4bb281f
0aa5c4d
7580cdc
3afb759
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,21 @@ def get_api_urls(api_views): | |
""" | ||
if getattr(settings, "OPENWISP_CONTROLLER_API", True): | ||
return [ | ||
path( | ||
"controller/<str:model>/revision/", | ||
api_views.revision_list, | ||
name="revision_list", | ||
), | ||
path( | ||
"controller/<str:model>/revision/<str:pk>/", | ||
api_views.version_detail, | ||
name="version_detail", | ||
), | ||
path( | ||
"controller/<str:model>/revision/<str:pk>/restore/", | ||
api_views.revision_restore, | ||
name="revision_restore", | ||
), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if we shorten this to just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have another doubt on this, how are we enforcing that we are enabling this only on the models that support reversions in this app? Eg: Device, Template, Vpn, etc. Would this work for any model? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently, empty revisions are being stored with only user and comment entries, which does not offer anything. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
path( | ||
"controller/template/", | ||
api_views.template_list, | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,22 +1,27 @@ | ||||||||||||||||||||||||||||||
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 | ||||||||||||||||||||||||||||||
from django.urls.base import reverse | ||||||||||||||||||||||||||||||
from django_filters.rest_framework import DjangoFilterBackend | ||||||||||||||||||||||||||||||
from rest_framework import pagination, serializers, status | ||||||||||||||||||||||||||||||
from rest_framework.generics import ( | ||||||||||||||||||||||||||||||
GenericAPIView, | ||||||||||||||||||||||||||||||
ListAPIView, | ||||||||||||||||||||||||||||||
ListCreateAPIView, | ||||||||||||||||||||||||||||||
RetrieveAPIView, | ||||||||||||||||||||||||||||||
RetrieveUpdateDestroyAPIView, | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
from rest_framework.response import Response | ||||||||||||||||||||||||||||||
from reversion.models import Version | ||||||||||||||||||||||||||||||
from swapper import load_model | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
from openwisp_users.api.permissions import DjangoModelPermissions | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
from ...mixins import ProtectedAPIMixin | ||||||||||||||||||||||||||||||
from ...mixins import AutoRevisionMixin, ProtectedAPIMixin | ||||||||||||||||||||||||||||||
from .filters import ( | ||||||||||||||||||||||||||||||
DeviceGroupListFilter, | ||||||||||||||||||||||||||||||
DeviceListFilter, | ||||||||||||||||||||||||||||||
|
@@ -29,6 +34,7 @@ | |||||||||||||||||||||||||||||
DeviceGroupSerializer, | ||||||||||||||||||||||||||||||
DeviceListSerializer, | ||||||||||||||||||||||||||||||
TemplateSerializer, | ||||||||||||||||||||||||||||||
VersionSerializer, | ||||||||||||||||||||||||||||||
VpnSerializer, | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
@@ -48,28 +54,30 @@ class ListViewPagination(pagination.PageNumberPagination): | |||||||||||||||||||||||||||||
max_page_size = 100 | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
class TemplateListCreateView(ProtectedAPIMixin, ListCreateAPIView): | ||||||||||||||||||||||||||||||
class TemplateListCreateView(ProtectedAPIMixin, AutoRevisionMixin, ListCreateAPIView): | ||||||||||||||||||||||||||||||
serializer_class = TemplateSerializer | ||||||||||||||||||||||||||||||
queryset = Template.objects.prefetch_related("tags").order_by("-created") | ||||||||||||||||||||||||||||||
pagination_class = ListViewPagination | ||||||||||||||||||||||||||||||
filter_backends = [DjangoFilterBackend] | ||||||||||||||||||||||||||||||
filterset_class = TemplateListFilter | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
class TemplateDetailView(ProtectedAPIMixin, RetrieveUpdateDestroyAPIView): | ||||||||||||||||||||||||||||||
class TemplateDetailView( | ||||||||||||||||||||||||||||||
ProtectedAPIMixin, AutoRevisionMixin, RetrieveUpdateDestroyAPIView | ||||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||||
serializer_class = TemplateSerializer | ||||||||||||||||||||||||||||||
queryset = Template.objects.all() | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
class VpnListCreateView(ProtectedAPIMixin, ListCreateAPIView): | ||||||||||||||||||||||||||||||
class VpnListCreateView(ProtectedAPIMixin, AutoRevisionMixin, ListCreateAPIView): | ||||||||||||||||||||||||||||||
serializer_class = VpnSerializer | ||||||||||||||||||||||||||||||
queryset = Vpn.objects.select_related("subnet").order_by("-created") | ||||||||||||||||||||||||||||||
pagination_class = ListViewPagination | ||||||||||||||||||||||||||||||
filter_backends = [DjangoFilterBackend] | ||||||||||||||||||||||||||||||
filterset_class = VPNListFilter | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
class VpnDetailView(ProtectedAPIMixin, RetrieveUpdateDestroyAPIView): | ||||||||||||||||||||||||||||||
class VpnDetailView(ProtectedAPIMixin, AutoRevisionMixin, RetrieveUpdateDestroyAPIView): | ||||||||||||||||||||||||||||||
serializer_class = VpnSerializer | ||||||||||||||||||||||||||||||
queryset = Vpn.objects.all() | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
@@ -82,7 +90,7 @@ def has_object_permission(self, request, view, obj): | |||||||||||||||||||||||||||||
return perm and not obj.is_deactivated() | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
class DeviceListCreateView(ProtectedAPIMixin, ListCreateAPIView): | ||||||||||||||||||||||||||||||
class DeviceListCreateView(ProtectedAPIMixin, AutoRevisionMixin, ListCreateAPIView): | ||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||
Templates: Templates flagged as required will be added automatically | ||||||||||||||||||||||||||||||
to the `config` of a device and cannot be unassigned. | ||||||||||||||||||||||||||||||
|
@@ -97,7 +105,9 @@ class DeviceListCreateView(ProtectedAPIMixin, ListCreateAPIView): | |||||||||||||||||||||||||||||
filterset_class = DeviceListFilter | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
class DeviceDetailView(ProtectedAPIMixin, RetrieveUpdateDestroyAPIView): | ||||||||||||||||||||||||||||||
class DeviceDetailView( | ||||||||||||||||||||||||||||||
ProtectedAPIMixin, AutoRevisionMixin, RetrieveUpdateDestroyAPIView | ||||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||
Templates: Templates flagged as _required_ will be added automatically | ||||||||||||||||||||||||||||||
to the `config` of a device and cannot be unassigned. | ||||||||||||||||||||||||||||||
|
@@ -124,7 +134,7 @@ def get_serializer_context(self): | |||||||||||||||||||||||||||||
return context | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
class DeviceActivateView(ProtectedAPIMixin, GenericAPIView): | ||||||||||||||||||||||||||||||
class DeviceActivateView(ProtectedAPIMixin, AutoRevisionMixin, GenericAPIView): | ||||||||||||||||||||||||||||||
serializer_class = serializers.Serializer | ||||||||||||||||||||||||||||||
queryset = Device.objects.filter(_is_deactivated=True) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
@@ -137,7 +147,7 @@ def post(self, request, *args, **kwargs): | |||||||||||||||||||||||||||||
return Response(serializer.data, status=status.HTTP_200_OK) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
class DeviceDeactivateView(ProtectedAPIMixin, GenericAPIView): | ||||||||||||||||||||||||||||||
class DeviceDeactivateView(ProtectedAPIMixin, AutoRevisionMixin, GenericAPIView): | ||||||||||||||||||||||||||||||
serializer_class = serializers.Serializer | ||||||||||||||||||||||||||||||
queryset = Device.objects.filter(_is_deactivated=False) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
@@ -150,15 +160,19 @@ def post(self, request, *args, **kwargs): | |||||||||||||||||||||||||||||
return Response(serializer.data, status=status.HTTP_200_OK) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
class DeviceGroupListCreateView(ProtectedAPIMixin, ListCreateAPIView): | ||||||||||||||||||||||||||||||
class DeviceGroupListCreateView( | ||||||||||||||||||||||||||||||
ProtectedAPIMixin, AutoRevisionMixin, ListCreateAPIView | ||||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||||
serializer_class = DeviceGroupSerializer | ||||||||||||||||||||||||||||||
queryset = DeviceGroup.objects.prefetch_related("templates").order_by("-created") | ||||||||||||||||||||||||||||||
pagination_class = ListViewPagination | ||||||||||||||||||||||||||||||
filter_backends = [DjangoFilterBackend] | ||||||||||||||||||||||||||||||
filterset_class = DeviceGroupListFilter | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
class DeviceGroupDetailView(ProtectedAPIMixin, RetrieveUpdateDestroyAPIView): | ||||||||||||||||||||||||||||||
class DeviceGroupDetailView( | ||||||||||||||||||||||||||||||
ProtectedAPIMixin, AutoRevisionMixin, RetrieveUpdateDestroyAPIView | ||||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||||
serializer_class = DeviceGroupSerializer | ||||||||||||||||||||||||||||||
queryset = DeviceGroup.objects.select_related("organization").order_by("-created") | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
@@ -172,7 +186,7 @@ def get_cached_devicegroup_args_rewrite(cls, org_slugs, common_name): | |||||||||||||||||||||||||||||
return url | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
class DeviceGroupCommonName(ProtectedAPIMixin, RetrieveAPIView): | ||||||||||||||||||||||||||||||
class DeviceGroupCommonName(ProtectedAPIMixin, AutoRevisionMixin, RetrieveAPIView): | ||||||||||||||||||||||||||||||
serializer_class = DeviceGroupSerializer | ||||||||||||||||||||||||||||||
queryset = DeviceGroup.objects.select_related("organization").order_by("-created") | ||||||||||||||||||||||||||||||
# Not setting lookup_field makes DRF raise error. but it is not used | ||||||||||||||||||||||||||||||
|
@@ -289,6 +303,60 @@ def certificate_delete_invalidates_cache(cls, organization_id, common_name): | |||||||||||||||||||||||||||||
cls.get_device_group.invalidate(cls, org_slug, common_name) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
class RevisionListView(ProtectedAPIMixin, ListAPIView): | ||||||||||||||||||||||||||||||
serializer_class = VersionSerializer | ||||||||||||||||||||||||||||||
queryset = Version.objects.select_related("revision").order_by( | ||||||||||||||||||||||||||||||
"-revision__date_created" | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
def get_queryset(self): | ||||||||||||||||||||||||||||||
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) | ||||||||||||||||||||||||||||||
Comment on lines
+312
to
+318
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way this method is written breaks the convention of Django/DRF. It is also inconsistent with OpenWISP.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now, it is not filtering the queryset with the And, there's no test which verifies the filtering. This is bad. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filtering using https://django-filter.readthedocs.io/en/stable/guide/rest_framework.html#quickstart You can check out other API views for details. |
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
class VersionDetailView(ProtectedAPIMixin, RetrieveAPIView): | ||||||||||||||||||||||||||||||
serializer_class = VersionSerializer | ||||||||||||||||||||||||||||||
queryset = Version.objects.select_related("revision").order_by( | ||||||||||||||||||||||||||||||
"-revision__date_created" | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
def get_queryset(self): | ||||||||||||||||||||||||||||||
model = self.kwargs.get("model").lower() | ||||||||||||||||||||||||||||||
return self.queryset.filter(content_type__model=model) | ||||||||||||||||||||||||||||||
Comment on lines
+327
to
+329
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Maybe, we can create a Mixin class which filters the queryset like this. |
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
class RevisionRestoreView(ProtectedAPIMixin, GenericAPIView): | ||||||||||||||||||||||||||||||
serializer_class = serializers.Serializer | ||||||||||||||||||||||||||||||
queryset = Version.objects.select_related("revision").order_by( | ||||||||||||||||||||||||||||||
"-revision__date_created" | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
def get_queryset(self): | ||||||||||||||||||||||||||||||
model = self.kwargs.get("model").lower() | ||||||||||||||||||||||||||||||
return self.queryset.filter(content_type__model=model) | ||||||||||||||||||||||||||||||
Comment on lines
+338
to
+340
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here! |
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
def post(self, request, *args, **kwargs): | ||||||||||||||||||||||||||||||
qs = self.get_queryset() | ||||||||||||||||||||||||||||||
versions = get_list_or_404(qs, revision_id=kwargs["pk"]) | ||||||||||||||||||||||||||||||
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')}" | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
Comment on lines
+345
to
+352
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't we use Revision.revert() instead of reverting individual versions? |
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
serializer = VersionSerializer( | ||||||||||||||||||||||||||||||
versions, many=True, context=self.get_serializer_context() | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
return Response(serializer.data, status=status.HTTP_200_OK) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
template_list = TemplateListCreateView.as_view() | ||||||||||||||||||||||||||||||
template_detail = TemplateDetailView.as_view() | ||||||||||||||||||||||||||||||
vpn_list = VpnListCreateView.as_view() | ||||||||||||||||||||||||||||||
|
@@ -300,3 +368,6 @@ def certificate_delete_invalidates_cache(cls, organization_id, common_name): | |||||||||||||||||||||||||||||
devicegroup_list = DeviceGroupListCreateView.as_view() | ||||||||||||||||||||||||||||||
devicegroup_detail = DeviceGroupDetailView.as_view() | ||||||||||||||||||||||||||||||
devicegroup_commonname = DeviceGroupCommonName.as_view() | ||||||||||||||||||||||||||||||
revision_list = RevisionListView.as_view() | ||||||||||||||||||||||||||||||
version_detail = VersionDetailView.as_view() | ||||||||||||||||||||||||||||||
revision_restore = RevisionRestoreView.as_view() |
Uh oh!
There was an error while loading. Please reload this page.