From eb8ccb555ed1bc18af246f152533ff89682316fc Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Mon, 3 Jun 2024 12:01:51 +0100 Subject: [PATCH] Remove `get_related_feature_id` logic and add a new endpoint to retrieve an environment feature version without the path args --- api/audit/audit_helpers.py | 31 --- api/audit/serializers.py | 6 - api/features/versioning/permissions.py | 7 + api/features/versioning/urls.py | 6 + api/features/versioning/views.py | 20 +- .../unit/audit/test_unit_audit_helpers.py | 103 -------- api/tests/unit/audit/test_unit_audit_views.py | 1 - .../versioning/test_unit_versioning_views.py | 240 ++++++++++-------- 8 files changed, 165 insertions(+), 249 deletions(-) delete mode 100644 api/audit/audit_helpers.py delete mode 100644 api/tests/unit/audit/test_unit_audit_helpers.py diff --git a/api/audit/audit_helpers.py b/api/audit/audit_helpers.py deleted file mode 100644 index 20b218a75f49..000000000000 --- a/api/audit/audit_helpers.py +++ /dev/null @@ -1,31 +0,0 @@ -from contextlib import suppress - -from audit.models import AuditLog -from audit.related_object_type import RelatedObjectType -from features.models import FeatureState -from features.versioning.models import EnvironmentFeatureVersion - - -def get_related_feature_id(audit_log_record: AuditLog) -> int | None: - """ - Given an AuditLog record, return the related feature id if applicable. - """ - - feature_id = None - - with suppress(EnvironmentFeatureVersion.DoesNotExist, FeatureState.DoesNotExist): - match audit_log_record.related_object_type: - case RelatedObjectType.FEATURE.name: - feature_id = audit_log_record.related_object_id - case RelatedObjectType.EF_VERSION.name: - ef_version = EnvironmentFeatureVersion.objects.get( - uuid=audit_log_record.related_object_uuid - ) - feature_id = ef_version.feature_id - case RelatedObjectType.FEATURE_STATE.name: - feature_state = FeatureState.objects.get( - id=audit_log_record.related_object_id - ) - feature_id = feature_state.feature_id - - return feature_id diff --git a/api/audit/serializers.py b/api/audit/serializers.py index 10800af57baa..70e975cfaa2e 100644 --- a/api/audit/serializers.py +++ b/api/audit/serializers.py @@ -3,7 +3,6 @@ from drf_yasg.utils import swagger_serializer_method from rest_framework import serializers -from audit.audit_helpers import get_related_feature_id from audit.models import AuditLog from environments.serializers import EnvironmentSerializerLight from projects.serializers import ProjectListSerializer @@ -43,7 +42,6 @@ class AuditLogRetrieveSerializer(serializers.ModelSerializer): project = ProjectListSerializer() change_details = serializers.SerializerMethodField() change_type = serializers.SerializerMethodField() - related_feature_id = serializers.SerializerMethodField() class Meta: model = AuditLog @@ -60,7 +58,6 @@ class Meta: "is_system_event", "change_details", "change_type", - "related_feature_id", ) @swagger_serializer_method( @@ -86,9 +83,6 @@ def get_change_type(self, instance: AuditLog) -> str: "~": "UPDATE", }.get(history_record.history_type) - def get_related_feature_id(self, instance: AuditLog) -> int | None: - return get_related_feature_id(instance) - class AuditLogsQueryParamSerializer(serializers.Serializer): project = serializers.IntegerField(required=False) diff --git a/api/features/versioning/permissions.py b/api/features/versioning/permissions.py index f6add7839318..538029704259 100644 --- a/api/features/versioning/permissions.py +++ b/api/features/versioning/permissions.py @@ -35,6 +35,13 @@ def has_object_permission( ) +class EnvironmentFeatureVersionRetrievePermissions(BasePermission): + def has_object_permission(self, request, view, obj): + return request.user.has_environment_permission( + VIEW_ENVIRONMENT, obj.environment + ) + + class EnvironmentFeatureVersionFeatureStatePermissions(BasePermission): def has_permission(self, request: Request, view: GenericViewSet) -> bool: environment_pk = view.kwargs["environment_pk"] diff --git a/api/features/versioning/urls.py b/api/features/versioning/urls.py index a9b97b358fe0..fc7bc9322966 100644 --- a/api/features/versioning/urls.py +++ b/api/features/versioning/urls.py @@ -4,6 +4,7 @@ from features.versioning.views import ( EnvironmentFeatureVersionFeatureStatesViewSet, + EnvironmentFeatureVersionRetrieveAPIView, EnvironmentFeatureVersionViewSet, ) @@ -34,4 +35,9 @@ "environments//features//", include(ef_version_fs_router.urls), ), + path( + "environment-feature-versions//", + EnvironmentFeatureVersionRetrieveAPIView.as_view(), + name="get-efv-by-uuid", + ), ] diff --git a/api/features/versioning/views.py b/api/features/versioning/views.py index c21032f52db5..14fa4929bbcc 100644 --- a/api/features/versioning/views.py +++ b/api/features/versioning/views.py @@ -4,11 +4,11 @@ from django.utils.decorators import method_decorator from drf_yasg.utils import swagger_auto_schema from rest_framework.decorators import action +from rest_framework.generics import RetrieveAPIView from rest_framework.mixins import ( CreateModelMixin, DestroyModelMixin, ListModelMixin, - RetrieveModelMixin, UpdateModelMixin, ) from rest_framework.permissions import IsAuthenticated @@ -26,6 +26,7 @@ from features.versioning.permissions import ( EnvironmentFeatureVersionFeatureStatePermissions, EnvironmentFeatureVersionPermissions, + EnvironmentFeatureVersionRetrievePermissions, ) from features.versioning.serializers import ( EnvironmentFeatureVersionFeatureStateSerializer, @@ -46,7 +47,6 @@ ) class EnvironmentFeatureVersionViewSet( GenericViewSet, - RetrieveModelMixin, ListModelMixin, CreateModelMixin, DestroyModelMixin, @@ -135,6 +135,22 @@ def publish(self, request: Request, **kwargs) -> Response: return Response(serializer.data) +class EnvironmentFeatureVersionRetrieveAPIView(RetrieveAPIView): + """ + This is an additional endpoint to retrieve a specific version without needing + to provide the environment or feature as part of the URL. + """ + + permission_classes = [ + IsAuthenticated, + EnvironmentFeatureVersionRetrievePermissions, + ] + serializer_class = EnvironmentFeatureVersionRetrieveSerializer + + def get_queryset(self): + return EnvironmentFeatureVersion.objects.all() + + class EnvironmentFeatureVersionFeatureStatesViewSet( GenericViewSet, ListModelMixin, diff --git a/api/tests/unit/audit/test_unit_audit_helpers.py b/api/tests/unit/audit/test_unit_audit_helpers.py deleted file mode 100644 index 8c3ffcc365da..000000000000 --- a/api/tests/unit/audit/test_unit_audit_helpers.py +++ /dev/null @@ -1,103 +0,0 @@ -from audit.audit_helpers import get_related_feature_id -from audit.models import AuditLog -from audit.related_object_type import RelatedObjectType -from environments.models import Environment -from features.models import Feature, FeatureState -from features.versioning.models import EnvironmentFeatureVersion -from segments.models import Segment - - -def test_get_related_feature_id_for_feature_audit_log( - feature: Feature, environment: Environment -) -> None: - # Given - audit_log_record = AuditLog.objects.create( - environment=environment, - related_object_id=feature.id, - related_object_type=RelatedObjectType.FEATURE.name, - log="Test", - ) - - # When - related_feature_id = get_related_feature_id(audit_log_record) - - # Then - assert related_feature_id == feature.id - - -def test_get_related_feature_id_for_environment_feature_version_audit_log( - environment_v2_versioning: Environment, feature: Feature -) -> None: - # Given - ef_version = EnvironmentFeatureVersion.objects.get( - environment=environment_v2_versioning, feature=feature - ) - - audit_log_record = AuditLog.objects.create( - environment=environment_v2_versioning, - related_object_uuid=ef_version.uuid, - related_object_type=RelatedObjectType.EF_VERSION.name, - log="Test", - ) - - # When - related_feature_id = get_related_feature_id(audit_log_record) - - # Then - assert related_feature_id == feature.id - - -def test_get_related_feature_id_for_feature_state_audit_log( - environment: Environment, feature: Feature -) -> None: - # Given - feature_state = FeatureState.objects.get(environment=environment, feature=feature) - - audit_log_record = AuditLog.objects.create( - environment=environment, - related_object_id=feature_state.id, - related_object_type=RelatedObjectType.FEATURE_STATE.name, - log="Test", - ) - - # When - related_feature_id = get_related_feature_id(audit_log_record) - - # Then - assert related_feature_id == feature.id - - -def test_get_related_feature_id_returns_none_for_entities_with_no_related_feature( - segment: Segment, feature: Feature -) -> None: - # Given - audit_log_record = AuditLog.objects.create( - project=segment.project, - related_object_id=segment.id, - related_object_type=RelatedObjectType.SEGMENT.name, - log="Test", - ) - - # When - related_feature_id = get_related_feature_id(audit_log_record) - - # Then - assert related_feature_id is None - - -def test_get_related_feature_id_returns_none_when_related_object_not_found( - environment: Environment, -) -> None: - # Given - audit_log_record = AuditLog.objects.create( - environment=environment, - related_object_id=1, - related_object_type=RelatedObjectType.FEATURE_STATE.name, - log="Test", - ) - - # When - related_feature_id = get_related_feature_id(audit_log_record) - - # Then - assert related_feature_id is None diff --git a/api/tests/unit/audit/test_unit_audit_views.py b/api/tests/unit/audit/test_unit_audit_views.py index 7f853605f70b..9c10d46dd4f1 100644 --- a/api/tests/unit/audit/test_unit_audit_views.py +++ b/api/tests/unit/audit/test_unit_audit_views.py @@ -182,7 +182,6 @@ def test_retrieve_environment_feature_version_published_audit_log_record_include response_json = response.json() assert response_json["related_object_uuid"] == str(new_version.uuid) assert response_json["related_object_type"] == RelatedObjectType.EF_VERSION.name - assert response_json["related_feature_id"] == new_version.feature_id assert ( response_json["log"] == ENVIRONMENT_FEATURE_VERSION_PUBLISHED_MESSAGE % feature.name diff --git a/api/tests/unit/features/versioning/test_unit_versioning_views.py b/api/tests/unit/features/versioning/test_unit_versioning_views.py index e89fda4c50d3..ab472e1f507a 100644 --- a/api/tests/unit/features/versioning/test_unit_versioning_views.py +++ b/api/tests/unit/features/versioning/test_unit_versioning_views.py @@ -126,112 +126,6 @@ def test_delete_feature_version( assert environment_feature_version.deleted is True -def test_retrieve_feature_version_with_no_previous_version( - feature: Feature, - environment_v2_versioning: Environment, - staff_client: APIClient, - with_environment_permissions: WithEnvironmentPermissionsCallable, - with_project_permissions: WithProjectPermissionsCallable, -) -> None: - # Given - environment_feature_version = EnvironmentFeatureVersion.objects.get( - feature=feature, environment=environment_v2_versioning - ) - - url = reverse( - "api-v1:versioning:environment-feature-versions-detail", - args=[ - environment_v2_versioning.id, - feature.id, - environment_feature_version.uuid, - ], - ) - - with_environment_permissions([VIEW_ENVIRONMENT]) - with_project_permissions([VIEW_PROJECT]) - - # When - response = staff_client.get(url) - - # Then - assert response.status_code == status.HTTP_200_OK - - response_json = response.json() - assert response_json["uuid"] == str(environment_feature_version.uuid) - assert response_json["previous_version_uuid"] is None - - -def test_retrieve_feature_version_with_previous_version( - feature: Feature, - environment_v2_versioning: Environment, - staff_user: FFAdminUser, - staff_client: APIClient, - with_environment_permissions: WithEnvironmentPermissionsCallable, - with_project_permissions: WithProjectPermissionsCallable, -) -> None: - # Given - with_environment_permissions([VIEW_ENVIRONMENT]) - with_project_permissions([VIEW_PROJECT]) - - version_1 = EnvironmentFeatureVersion.objects.filter( - feature=feature, environment=environment_v2_versioning - ).get() - version_2 = EnvironmentFeatureVersion.objects.create( - feature=feature, environment=environment_v2_versioning - ) - version_2.publish(published_by=staff_user) - - url = reverse( - "api-v1:versioning:environment-feature-versions-detail", - args=[environment_v2_versioning.id, feature.id, version_2.uuid], - ) - - # When - response = staff_client.get(url) - - # Then - assert response.status_code == status.HTTP_200_OK - - response_json = response.json() - assert response_json["uuid"] == str(version_2.uuid) - assert response_json["previous_version_uuid"] == str(version_1.uuid) - - -def test_retrieve_feature_version_for_unpublished_version( - feature: Feature, - environment_v2_versioning: Environment, - staff_user: FFAdminUser, - staff_client: APIClient, - with_environment_permissions: WithEnvironmentPermissionsCallable, - with_project_permissions: WithProjectPermissionsCallable, -) -> None: - # Given - with_environment_permissions([VIEW_ENVIRONMENT]) - with_project_permissions([VIEW_PROJECT]) - - version_1 = EnvironmentFeatureVersion.objects.filter( - feature=feature, environment=environment_v2_versioning - ).get() - version_2 = EnvironmentFeatureVersion.objects.create( - feature=feature, environment=environment_v2_versioning - ) - - url = reverse( - "api-v1:versioning:environment-feature-versions-detail", - args=[environment_v2_versioning.id, feature.id, version_2.uuid], - ) - - # When - response = staff_client.get(url) - - # Then - assert response.status_code == status.HTTP_200_OK - - response_json = response.json() - assert response_json["uuid"] == str(version_2.uuid) - assert response_json["previous_version_uuid"] == str(version_1.uuid) - - def test_cannot_delete_live_feature_version( admin_client: APIClient, environment_v2_versioning: Environment, @@ -758,3 +652,137 @@ def test_disable_v2_versioning_returns_bad_request_if_not_using_v2_versioning( # Then assert response.status_code == status.HTTP_400_BAD_REQUEST + + +def test_retrieve_environment_feature_version_without_path_args( + feature: Feature, + environment_v2_versioning: Environment, + staff_client: APIClient, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + efv = EnvironmentFeatureVersion.objects.first() + + url = reverse("api-v1:versioning:get-efv-by-uuid", args=[efv.uuid]) + with_environment_permissions([VIEW_ENVIRONMENT], efv.environment_id) + + # When + response = staff_client.get(url) + + # Then + assert response.status_code == status.HTTP_200_OK + + response_json = response.json() + assert response_json["uuid"] == str(efv.uuid) + + +def test_retrieve_environment_feature_version_without_path_args_permission_denied( + feature: Feature, + environment_v2_versioning: Environment, + staff_client: APIClient, +) -> None: + # Given + efv = EnvironmentFeatureVersion.objects.first() + + url = reverse("api-v1:versioning:get-efv-by-uuid", args=[str(efv.uuid)]) + + # When + response = staff_client.get(url) + + # Then + assert response.status_code == status.HTTP_403_FORBIDDEN + + +def test_retrieve_feature_version_with_no_previous_version( + feature: Feature, + environment_v2_versioning: Environment, + staff_client: APIClient, + with_environment_permissions: WithEnvironmentPermissionsCallable, + with_project_permissions: WithProjectPermissionsCallable, +) -> None: + # Given + environment_feature_version = EnvironmentFeatureVersion.objects.get( + feature=feature, environment=environment_v2_versioning + ) + + url = reverse( + "api-v1:versioning:get-efv-by-uuid", args=[environment_feature_version.uuid] + ) + + with_environment_permissions([VIEW_ENVIRONMENT]) + with_project_permissions([VIEW_PROJECT]) + + # When + response = staff_client.get(url) + + # Then + assert response.status_code == status.HTTP_200_OK + + response_json = response.json() + assert response_json["uuid"] == str(environment_feature_version.uuid) + assert response_json["previous_version_uuid"] is None + + +def test_retrieve_feature_version_with_previous_version( + feature: Feature, + environment_v2_versioning: Environment, + staff_user: FFAdminUser, + staff_client: APIClient, + with_environment_permissions: WithEnvironmentPermissionsCallable, + with_project_permissions: WithProjectPermissionsCallable, +) -> None: + # Given + with_environment_permissions([VIEW_ENVIRONMENT]) + with_project_permissions([VIEW_PROJECT]) + + version_1 = EnvironmentFeatureVersion.objects.filter( + feature=feature, environment=environment_v2_versioning + ).get() + version_2 = EnvironmentFeatureVersion.objects.create( + feature=feature, environment=environment_v2_versioning + ) + version_2.publish(published_by=staff_user) + + url = reverse("api-v1:versioning:get-efv-by-uuid", args=[version_2.uuid]) + + # When + response = staff_client.get(url) + + # Then + assert response.status_code == status.HTTP_200_OK + + response_json = response.json() + assert response_json["uuid"] == str(version_2.uuid) + assert response_json["previous_version_uuid"] == str(version_1.uuid) + + +def test_retrieve_feature_version_for_unpublished_version( + feature: Feature, + environment_v2_versioning: Environment, + staff_user: FFAdminUser, + staff_client: APIClient, + with_environment_permissions: WithEnvironmentPermissionsCallable, + with_project_permissions: WithProjectPermissionsCallable, +) -> None: + # Given + with_environment_permissions([VIEW_ENVIRONMENT]) + with_project_permissions([VIEW_PROJECT]) + + version_1 = EnvironmentFeatureVersion.objects.filter( + feature=feature, environment=environment_v2_versioning + ).get() + version_2 = EnvironmentFeatureVersion.objects.create( + feature=feature, environment=environment_v2_versioning + ) + + url = reverse("api-v1:versioning:get-efv-by-uuid", args=[version_2.uuid]) + + # When + response = staff_client.get(url) + + # Then + assert response.status_code == status.HTTP_200_OK + + response_json = response.json() + assert response_json["uuid"] == str(version_2.uuid) + assert response_json["previous_version_uuid"] == str(version_1.uuid)