diff --git a/api/core/workflows_services.py b/api/core/workflows_services.py index c68429ee2408..e4e2699db5b7 100644 --- a/api/core/workflows_services.py +++ b/api/core/workflows_services.py @@ -24,14 +24,18 @@ class ChangeRequestCommitService: def __init__(self, change_request: "ChangeRequest") -> None: self.change_request = change_request + @transaction.atomic def commit(self, committed_by: "FFAdminUser") -> None: + from features.dependencies.services import ( + validate_change_request_flag_dependencies, + ) + if not self.change_request.is_approved(): raise ChangeRequestNotApprovedError( "Change request has not been approved by all required approvers." ) - # Runs before anything publishes: commit is not atomic as a whole, so - # raising any later would leave the change request half-applied. self._validate_segments_are_not_cohort_managed() + validate_change_request_flag_dependencies(self.change_request) self._publish_feature_states() self._publish_environment_feature_versions(committed_by) @@ -122,7 +126,6 @@ def _validate_segments_are_not_cohort_managed(self) -> None: "via a change request." ) - @transaction.atomic def _publish_segments(self) -> None: for draft_segment in self.change_request.segments.all(): live_segment = draft_segment.version_of diff --git a/api/environments/urls.py b/api/environments/urls.py index 4921177c7e1f..200cbf40c0d5 100644 --- a/api/environments/urls.py +++ b/api/environments/urls.py @@ -10,8 +10,8 @@ ) from features.dependencies.views import ( FeatureDependenciesAPIView, - FeatureDependencyAPIView, FeatureDependentsAPIView, + FeaturePrerequisiteAPIView, ) from features.views import ( EnvironmentFeatureStateViewSet, @@ -171,7 +171,7 @@ path( "/features/" "/dependencies//", - FeatureDependencyAPIView.as_view(), + FeaturePrerequisiteAPIView.as_view(), name="feature-dependency", ), path( diff --git a/api/features/dependencies/exceptions.py b/api/features/dependencies/exceptions.py index b20ac3d6d19d..fa24910e3a5c 100644 --- a/api/features/dependencies/exceptions.py +++ b/api/features/dependencies/exceptions.py @@ -1,6 +1,9 @@ from abc import ABC, abstractmethod -from common.environments.permissions import MANAGE_SEGMENT_OVERRIDES +from common.environments.permissions import ( + CREATE_CHANGE_REQUEST, + MANAGE_SEGMENT_OVERRIDES, +) from rest_framework import status from rest_framework.exceptions import APIException, NotFound, PermissionDenied @@ -180,3 +183,65 @@ def __init__( "segments": segments, } self.detail = detail # type: ignore[assignment] + + +class ChangeRequestNotFoundError(APIError, NotFound): + """Raised when a referenced change request does not exist.""" + + default_code = "change_request_not_found" + default_detail = "Change request not found." + + +class ChangeRequestAlreadyCommittedError(APIError): + """Raised when a referenced change request has already been committed.""" + + status_code = status.HTTP_400_BAD_REQUEST + default_code = "change_request_committed" + default_detail = "The change request is already committed." + + +class ChangeRequestPermissionDeniedError(APIError, PermissionDenied): + """Raised when a referenced change request is not accessible to the caller.""" + + def __init__(self) -> None: + permission_name = CREATE_CHANGE_REQUEST.capitalize().replace("_", " ") + super().__init__( + f'The permission "{permission_name}" is necessary' + " to change dependencies in change requests." + ) + + +class ChangeRequestApprovedError(APIError): + """Raised when a referenced change request has already been approved.""" + + status_code = status.HTTP_400_BAD_REQUEST + default_code = "change_request_approved" + default_detail = "Cannot change an approved change request." + + +class ChangeRequestDisabledError(APIError): + """Raised when a change request is referenced where change requests are disabled.""" + + status_code = status.HTTP_409_CONFLICT + default_code = "change_requests_disabled" + default_detail = ( + "Enable Feature Change Requests to change dependencies in change requests." + ) + + +class FeatureVersioningDisabledError(APIError): + """Raised when a change request is referenced where feature versioning is disabled.""" + + status_code = status.HTTP_409_CONFLICT + default_code = "feature_versioning_disabled" + default_detail = ( + "Enable Feature Versioning to change dependencies in change requests." + ) + + +class ChangeRequestRequiredError(APIError): + """Raised when a dependency is changed directly where change requests are enabled.""" + + status_code = status.HTTP_409_CONFLICT + default_code = "change_requests_enabled" + default_detail = "Cannot make this change where change requests are enabled." diff --git a/api/features/dependencies/permissions.py b/api/features/dependencies/permissions.py index 5364e09030af..0d632ea3e756 100644 --- a/api/features/dependencies/permissions.py +++ b/api/features/dependencies/permissions.py @@ -1,7 +1,13 @@ -from common.environments.permissions import MANAGE_SEGMENT_OVERRIDES +from common.environments.permissions import ( + CREATE_CHANGE_REQUEST, + MANAGE_SEGMENT_OVERRIDES, +) from environments.models import Environment -from features.dependencies.exceptions import FeatureDependencyPermissionDeniedError +from features.dependencies.exceptions import ( + ChangeRequestPermissionDeniedError, + FeatureDependencyPermissionDeniedError, +) from features.future.permissions import check_read_permissions from users.abc import UserABC @@ -11,3 +17,9 @@ def check_manage_permissions(user: UserABC, environment: Environment) -> None: check_read_permissions(user, environment) if not user.has_environment_permission(MANAGE_SEGMENT_OVERRIDES, environment): raise FeatureDependencyPermissionDeniedError() + + +def check_change_request_permissions(user: UserABC, environment: Environment) -> None: + """Authorise a caller to change the environment's feature dependencies in change requests.""" + if not user.has_environment_permission(CREATE_CHANGE_REQUEST, environment): + raise ChangeRequestPermissionDeniedError() diff --git a/api/features/dependencies/serializers.py b/api/features/dependencies/serializers.py new file mode 100644 index 000000000000..a93d74a8ef84 --- /dev/null +++ b/api/features/dependencies/serializers.py @@ -0,0 +1,5 @@ +from rest_framework import serializers + + +class ChangeRequestReferenceSerializer(serializers.Serializer[None]): + change_request = serializers.IntegerField() diff --git a/api/features/dependencies/services.py b/api/features/dependencies/services.py index 1de6ca7bf8bf..504b181ef186 100644 --- a/api/features/dependencies/services.py +++ b/api/features/dependencies/services.py @@ -5,6 +5,7 @@ import structlog from django.db import transaction from django.db.models import QuerySet +from django.utils import timezone from flag_engine.segments import constants from api_keys.user import APIKeyUser @@ -17,6 +18,7 @@ from core.dataclasses import AuthorData from environments.models import Environment from features.dependencies.exceptions import ( + ChangeRequestRequiredError, CircularDependencyError, DependencyExistsError, DependencyNotFoundError, @@ -41,6 +43,8 @@ ReferencingEnvironment, ) from features.models import Feature, FeatureSegment +from features.versioning.models import VersionChangeSet +from features.workflows.core.models import ChangeRequest from projects.models import Project from segments.models import Segment, SegmentManagedBy from segments.services import get_live_overrides, write_segment_rules @@ -134,6 +138,43 @@ def report_flag_dependencies( log.info("dependencies.created", prerequisite_feature__name=feature_name) +def report_flag_dependency_changes( + *, + environment: Environment, + feature: Feature, + created_segment_ids: Collection[int], + deleted_segment_ids: Collection[int], + author: AuthorData, +) -> None: + """Report and audit the prerequisites a feature gains and loses as its + segment overrides go live.""" + created: set[FeatureName] = set() + deleted: set[FeatureName] = set() + for reference in SegmentFlagReference.objects.filter( + segment_id__in=[*created_segment_ids, *deleted_segment_ids] + ).select_related("segment", "prerequisite_feature"): + is_created = reference.segment_id in created_segment_ids + (created if is_created else deleted).add(reference.prerequisite_feature.name) + if reference.segment.managed_by == SegmentManagedBy.DEPENDENCY: + message = ( + FEATURE_DEPENDENCY_CREATED_MESSAGE + if is_created + else FEATURE_DEPENDENCY_DELETED_MESSAGE + ) + AuditLog.objects.create( + environment=environment, + project=environment.project, + related_object_type=RelatedObjectType.FEATURE.name, + related_object_id=feature.id, + author=author.user, + master_api_key=author.api_key, + log=message % (reference.prerequisite_feature.name, feature.name), + ) + report_flag_dependencies( + environment=environment, feature=feature, created=created, deleted=deleted + ) + + def validate_segment_flag_dependencies(segment: "Segment") -> None: """Raise if any feature the segment overrides ends up depending on itself. @@ -268,16 +309,20 @@ def _get_dependency_edges( segment_rules, condition_json_path, is_system_segment, - ) in overrides.filter(segment__flag_references__isnull=False).values_list( - "feature__id", - "feature__name", - "segment__flag_references__prerequisite_feature__id", - "segment__flag_references__prerequisite_feature__name", - "segment__id", - "segment__name", - "segment__rules_data", - "segment__flag_references__condition_json_path", - "segment__is_system_segment", + ) in ( + overrides.filter(segment__flag_references__isnull=False) + .order_by("id") + .values_list( + "feature__id", + "feature__name", + "segment__flag_references__prerequisite_feature__id", + "segment__flag_references__prerequisite_feature__name", + "segment__id", + "segment__name", + "segment__rules_data", + "segment__flag_references__condition_json_path", + "segment__is_system_segment", + ) ): assert segment_rules is not None edges[feature_name].append( @@ -299,13 +344,89 @@ def _get_dependency_edges( return edges +def _get_scheduled_change_sets( + environment: Environment, +) -> "QuerySet[VersionChangeSet]": + change_sets: "QuerySet[VersionChangeSet]" = VersionChangeSet.objects.filter( + environment=environment, + change_request__committed_at__isnull=False, + published_at__isnull=True, + live_from__gt=timezone.now(), + ) + return change_sets + + +def _get_dependency_edges_to_validate( + environment: Environment, + change_request: ChangeRequest | None, +) -> dict[FeatureName, list[DependencyEdge]]: + edges = _get_dependency_edges( + get_live_overrides(include_scheduled=True).filter(environment=environment) + ) + _apply_change_sets( # Scheduled deletions leave edges live until they publish. + edges, _get_scheduled_change_sets(environment), include_deletions=False + ) + if change_request is not None: + _apply_change_sets(edges, change_request.change_sets.all()) + return edges + + +def _get_live_dependency_edges( + environment: Environment, + change_request: ChangeRequest | None, +) -> dict[FeatureName, list[DependencyEdge]]: + edges = _get_dependency_edges(get_live_overrides().filter(environment=environment)) + if change_request is not None: + _apply_change_sets(edges, change_request.change_sets.all()) + return edges + + +def _apply_change_sets( + edges: dict[FeatureName, list[DependencyEdge]], + change_sets: "QuerySet[VersionChangeSet]", + *, + include_deletions: bool = True, +) -> None: + for change_set in change_sets.select_related("feature").order_by("live_from", "id"): + feature = change_set.feature + deleted_segment_ids = ( + set(change_set.get_parsed_segment_ids_to_delete_overrides()) + if include_deletions + else set() + ) + feature_edges = [ + edge + for edge in edges[feature.name] + if edge["segment"]["id"] not in deleted_segment_ids + ] + created_references = ( + SegmentFlagReference.objects.filter( + segment_id__in=change_set.get_parsed_segment_ids_to_create_overrides() + ) + .exclude(segment_id__in=[edge["segment"]["id"] for edge in feature_edges]) + .select_related("segment", "prerequisite_feature") + .order_by("id") + ) + edges[feature.name] = [ + *feature_edges, + *( + map_reference_to_dependency_edge(feature=feature, reference=reference) + for reference in created_references + ), + ] + + def list_flag_dependencies( *, environment: Environment, feature: Feature, + change_request: ChangeRequest | None = None, ) -> DependencyList: - """List the features the feature depends on in the environment.""" - edges = _get_dependency_edges(get_live_overrides().filter(environment=environment)) + """List the features the feature depends on in the environment. + + Given an uncommitted change request, list them as if it were live. + """ + edges = _get_live_dependency_edges(environment, change_request) return {"results": edges[feature.name]} @@ -313,9 +434,13 @@ def list_flag_dependents( *, environment: Environment, feature: Feature, + change_request: ChangeRequest | None = None, ) -> DependencyList: - """List the features depending on the feature in the environment.""" - edges = _get_dependency_edges(get_live_overrides().filter(environment=environment)) + """List the features depending on the feature in the environment. + + Given an uncommitted change request, list them as if it were live. + """ + edges = _get_live_dependency_edges(environment, change_request) return { "results": [ edge @@ -347,17 +472,91 @@ def get_prerequisite_feature_names_by_environment( } +def _check_dependency_is_allowed( + *, + environment: Environment, + edges: dict[FeatureName, list[DependencyEdge]], + feature: Feature, + prerequisite_feature: Feature, +) -> None: + log = logger.bind( + organisation__id=environment.project.organisation_id, + project__id=environment.project_id, + environment__key=environment.api_key, + feature__name=feature.name, + prerequisite_feature__name=prerequisite_feature.name, + ) + referencing_environment: ReferencingEnvironment = { + "key": environment.api_key, + "name": environment.name, + } + if existing_edges := [ + edge + for edge in edges[feature.name] + if edge["prerequisite"]["id"] == prerequisite_feature.id + ]: + log.info("dependencies.create_failed") + raise DependencyExistsError( + environment=referencing_environment, path=existing_edges + ) + if prerequisite_edges := edges[prerequisite_feature.name]: + log.info("dependencies.create_failed") + raise PrerequisiteHasPrerequisiteError( + environment=referencing_environment, path=prerequisite_edges + ) + if dependent_edges := [ + edge + for feature_edges in edges.values() + for edge in feature_edges + if edge["prerequisite"]["id"] == feature.id + ]: + log.info("dependencies.create_failed") + raise FeatureIsPrerequisiteError( + environment=referencing_environment, path=dependent_edges + ) + + +def validate_change_request_flag_dependencies(change_request: ChangeRequest) -> None: + """Raise if any dependency the change request stages is no longer allowed. + + Must run before committing the change request, as other changes may have + gone live or been scheduled since the dependency was staged. + """ + if (environment := change_request.environment) is None: + return + with transaction.atomic(): + _lock_project_flag_dependencies(environment.project_id) + edges = _get_dependency_edges_to_validate(environment, change_request) + for change_set in change_request.change_sets.select_related("feature"): + feature = change_set.feature + for reference in SegmentFlagReference.objects.filter( + segment_id__in=change_set.get_parsed_segment_ids_to_create_overrides(), + segment__managed_by=SegmentManagedBy.DEPENDENCY, + ).select_related("prerequisite_feature"): + edges_without_staged = edges.copy() + edges_without_staged[feature.name] = [ + edge + for edge in edges[feature.name] + if edge["segment"]["id"] != reference.segment_id + ] + _check_dependency_is_allowed( + environment=environment, + edges=edges_without_staged, + feature=feature, + prerequisite_feature=reference.prerequisite_feature, + ) + + def create_flag_dependency( *, environment: Environment, feature: Feature, prerequisite_feature: Feature, author: FFAdminUser | APIKeyUser, + change_request: ChangeRequest | None = None, ) -> DependencyEdge: """Disable the feature in the environment unless the prerequisite is enabled.""" - from features.feature_segments.services import ( - get_next_segment_override_priority, - ) + from features.feature_segments.services import get_next_segment_override_priority from features.future.services import update_flag log = logger.bind( @@ -367,42 +566,20 @@ def create_flag_dependency( feature__name=feature.name, prerequisite_feature__name=prerequisite_feature.name, ) + if change_request is None and environment.is_workflow_enabled: + log.info("dependencies.create_failed") + raise ChangeRequestRequiredError() if feature.id == prerequisite_feature.id: log.info("dependencies.create_failed") raise PrerequisiteIsSelfError() - referencing_environment: ReferencingEnvironment = { - "key": environment.api_key, - "name": environment.name, - } with transaction.atomic(): _lock_project_flag_dependencies(environment.project_id) - edges = _get_dependency_edges( - get_live_overrides(include_scheduled=True).filter(environment=environment) + _check_dependency_is_allowed( + environment=environment, + edges=_get_dependency_edges_to_validate(environment, change_request), + feature=feature, + prerequisite_feature=prerequisite_feature, ) - if existing_edges := [ - edge - for edge in edges[feature.name] - if edge["prerequisite"]["id"] == prerequisite_feature.id - ]: - log.info("dependencies.create_failed") - raise DependencyExistsError( - environment=referencing_environment, path=existing_edges - ) - if prerequisite_edges := edges[prerequisite_feature.name]: - log.info("dependencies.create_failed") - raise PrerequisiteHasPrerequisiteError( - environment=referencing_environment, path=prerequisite_edges - ) - if dependent_edges := [ - edge - for feature_edges in edges.values() - for edge in feature_edges - if edge["prerequisite"]["id"] == feature.id - ]: - log.info("dependencies.create_failed") - raise FeatureIsPrerequisiteError( - environment=referencing_environment, path=dependent_edges - ) segment = _get_or_create_dependency_segment( feature=feature, prerequisite_feature=prerequisite_feature ) @@ -417,7 +594,9 @@ def create_flag_dependency( { "segment": {"id": segment.id}, "enabled": False, - "priority": get_next_segment_override_priority( + "priority": 0 + if change_request + else get_next_segment_override_priority( environment=environment, feature=feature ), } @@ -425,16 +604,11 @@ def create_flag_dependency( }, replace=False, author=AuthorData.from_user(author), + change_request=change_request, overwrite_system_segment_overrides=True, ) - overrides.get(segment=segment).to(0) - _create_dependency_audit_log( - environment=environment, - feature=feature, - prerequisite_feature=prerequisite_feature, - author=author, - message=FEATURE_DEPENDENCY_CREATED_MESSAGE, - ) + if change_request is None: + overrides.get(segment=segment).to(0) return map_reference_to_dependency_edge( feature=feature, reference=SegmentFlagReference.objects.select_related( @@ -449,6 +623,7 @@ def delete_flag_dependency( feature: Feature, prerequisite_feature: Feature, author: FFAdminUser | APIKeyUser, + change_request: ChangeRequest | None = None, ) -> None: """Stop the feature depending on the prerequisite in the environment. @@ -466,11 +641,13 @@ def delete_flag_dependency( feature__name=feature.name, prerequisite_feature__name=prerequisite_feature.name, ) + if change_request is None and environment.is_workflow_enabled: + log.info("dependencies.delete_failed") + raise ChangeRequestRequiredError() with transaction.atomic(): _lock_project_flag_dependencies(environment.project_id) - feature_edges = _get_dependency_edges( - get_live_overrides().filter(environment=environment, feature=feature) - )[feature.name] + all_edges = _get_live_dependency_edges(environment, change_request) + feature_edges = all_edges[feature.name] if not ( edges := [ edge @@ -490,15 +667,9 @@ def delete_flag_dependency( feature=feature, segment_id=edge["segment"]["id"], author=AuthorData.from_user(author), + change_request=change_request, overwrite_system_segment_overrides=True, ) - _create_dependency_audit_log( - environment=environment, - feature=feature, - prerequisite_feature=prerequisite_feature, - author=author, - message=FEATURE_DEPENDENCY_DELETED_MESSAGE, - ) def _get_or_create_dependency_segment( @@ -534,23 +705,3 @@ def _get_or_create_dependency_segment( write_segment_rules(segment, rules) index_segment_flag_references(segment) return segment - - -def _create_dependency_audit_log( - *, - environment: Environment, - feature: Feature, - prerequisite_feature: Feature, - author: FFAdminUser | APIKeyUser, - message: str, -) -> None: - user = author if isinstance(author, FFAdminUser) else None - AuditLog.objects.create( - environment=environment, - project=environment.project, - related_object_type=RelatedObjectType.FEATURE.name, - related_object_id=feature.id, - author=user, - master_api_key=None if user else author.key, - log=message % (prerequisite_feature.name, feature.name), - ) diff --git a/api/features/dependencies/views.py b/api/features/dependencies/views.py index 52996b9147e6..8d872b110051 100644 --- a/api/features/dependencies/views.py +++ b/api/features/dependencies/views.py @@ -1,3 +1,4 @@ +from django.conf import settings from drf_spectacular.utils import PolymorphicProxySerializer, extend_schema from rest_framework import status from rest_framework.permissions import IsAuthenticated @@ -6,8 +7,19 @@ from core.types import APIErrorDetail, AuthenticatedRequest from environments.services import get_environment -from features.dependencies.exceptions import DependencyConflictDetail -from features.dependencies.permissions import check_manage_permissions +from features.dependencies.exceptions import ( + ChangeRequestAlreadyCommittedError, + ChangeRequestApprovedError, + ChangeRequestDisabledError, + ChangeRequestNotFoundError, + DependencyConflictDetail, + FeatureVersioningDisabledError, +) +from features.dependencies.permissions import ( + check_change_request_permissions, + check_manage_permissions, +) +from features.dependencies.serializers import ChangeRequestReferenceSerializer from features.dependencies.services import ( create_flag_dependency, delete_flag_dependency, @@ -17,9 +29,42 @@ from features.dependencies.types import DependencyEdge, DependencyList from features.future.permissions import check_read_permissions from features.services import get_feature +from features.workflows.core.models import ChangeRequest + + +def _ensure_change_request_is_open(change_request: ChangeRequest) -> None: + if change_request.has_approvals(): + raise ChangeRequestApprovedError() + + +def _get_change_request( + request: AuthenticatedRequest, environment_api_key: str +) -> ChangeRequest | None: + if not settings.WORKFLOWS_LOGIC_INSTALLED: + return None + if "change_request" not in request.query_params: + return None + serializer = ChangeRequestReferenceSerializer(data=request.query_params) + serializer.is_valid(raise_exception=True) + environment = get_environment(environment_api_key) + check_read_permissions(request.user, environment) + if not environment.is_workflow_enabled: + raise ChangeRequestDisabledError() + if not environment.use_v2_feature_versioning: + raise FeatureVersioningDisabledError() + try: + change_request: ChangeRequest = ChangeRequest.objects.get( + environment=environment, + id=serializer.validated_data["change_request"], + ) + except ChangeRequest.DoesNotExist: + raise ChangeRequestNotFoundError() + if change_request.is_committed: + raise ChangeRequestAlreadyCommittedError() + return change_request -class FeatureDependencyAPIView(APIView): +class FeaturePrerequisiteAPIView(APIView): """Manage a feature's dependency on another feature in an environment.""" permission_classes = [IsAuthenticated] @@ -44,7 +89,11 @@ def post( prerequisite_feature_id: int, ) -> Response: environment = get_environment(environment_api_key) - check_manage_permissions(request.user, environment) + if change_request := _get_change_request(request, environment_api_key): + check_change_request_permissions(request.user, environment) + _ensure_change_request_is_open(change_request) + else: + check_manage_permissions(request.user, environment) feature = get_feature(environment, feature_id) prerequisite_feature = get_feature(environment, prerequisite_feature_id) return Response( @@ -53,6 +102,7 @@ def post( feature=feature, prerequisite_feature=prerequisite_feature, author=request.user, + change_request=change_request, ), status=status.HTTP_201_CREATED, ) @@ -73,7 +123,11 @@ def delete( prerequisite_feature_id: int, ) -> Response: environment = get_environment(environment_api_key) - check_manage_permissions(request.user, environment) + if change_request := _get_change_request(request, environment_api_key): + check_change_request_permissions(request.user, environment) + _ensure_change_request_is_open(change_request) + else: + check_manage_permissions(request.user, environment) feature = get_feature(environment, feature_id) prerequisite_feature = get_feature(environment, prerequisite_feature_id) delete_flag_dependency( @@ -81,6 +135,7 @@ def delete( feature=feature, prerequisite_feature=prerequisite_feature, author=request.user, + change_request=change_request, ) return Response(status=status.HTTP_204_NO_CONTENT) @@ -104,7 +159,11 @@ def get( check_read_permissions(request.user, environment) feature = get_feature(environment, feature_id) return Response( - list_flag_dependencies(environment=environment, feature=feature) + list_flag_dependencies( + environment=environment, + feature=feature, + change_request=_get_change_request(request, environment_api_key), + ) ) @@ -126,4 +185,10 @@ def get( environment = get_environment(environment_api_key) check_read_permissions(request.user, environment) feature = get_feature(environment, feature_id) - return Response(list_flag_dependents(environment=environment, feature=feature)) + return Response( + list_flag_dependents( + environment=environment, + feature=feature, + change_request=_get_change_request(request, environment_api_key), + ) + ) diff --git a/api/features/future/services.py b/api/features/future/services.py index 2a4968ac921f..c58cc0b2b4f4 100644 --- a/api/features/future/services.py +++ b/api/features/future/services.py @@ -1,22 +1,22 @@ """https://docs.flagsmith.com/managing-flags/updating-flags""" +import json from collections.abc import Collection, Iterable, Sequence from itertools import count, islice from typing import NamedTuple import structlog +from common.features.serializers import FeatureStateValueSerializer from django.db import transaction from django.db.models import Count, Q from rest_framework.exceptions import ValidationError from core.dataclasses import AuthorData from environments.models import Environment -from features.dependencies.models import SegmentFlagReference from features.dependencies.services import ( - report_flag_dependencies, + report_flag_dependency_changes, validate_segment_flag_dependencies, ) -from features.dependencies.types import FeatureName from features.feature_segments.limits import ( SEGMENT_OVERRIDE_LIMIT_EXCEEDED_MESSAGE, exceeds_segment_override_limit, @@ -41,8 +41,9 @@ ) from features.models import Feature, FeatureSegment, FeatureState, FeatureStateValue from features.multivariate.models import MultivariateFeatureStateValue -from features.versioning.models import EnvironmentFeatureVersion +from features.versioning.models import EnvironmentFeatureVersion, VersionChangeSet from features.versioning.versioning_service import get_environment_flags_list +from features.workflows.core.models import ChangeRequest from segments.models import Segment from segments.services import check_segment_is_not_system from util.db import with_delete_hooks @@ -397,18 +398,78 @@ def _write_segment_overrides( return segments -def _get_prerequisite_feature_names( - segments: _OverriddenSegments, -) -> tuple[set[FeatureName], set[FeatureName]]: - """Return the prerequisite feature names the flag gains and loses.""" - created: set[FeatureName] = set() - deleted: set[FeatureName] = set() - for segment_id, feature_name in SegmentFlagReference.objects.filter( - segment_id__in=[*segments.created, *segments.deleted] - ).values_list("segment_id", "prerequisite_feature__name"): - names = created if segment_id in segments.created else deleted - names.add(feature_name) - return created, deleted +def _stage_segment_overrides( + *, + environment: Environment, + feature: Feature, + change_request: ChangeRequest, + created: Sequence[SegmentOverrideRequest], + deleted_segment_ids: Collection[int], +) -> None: + created_segment_ids = [override["segment"]["id"] for override in created] + environment_default = next( + feature_state + for feature_state in _get_feature_states(environment, feature) + if feature_state.feature_segment_id is None + ) + + with transaction.atomic(): + change_set, _ = VersionChangeSet.objects.select_for_update().get_or_create( + change_request=change_request, + feature=feature, + defaults={"live_from": change_request.live_from}, + ) + feature_states_to_create = change_set.get_parsed_feature_states_to_create() + segment_ids_to_delete = change_set.get_parsed_segment_ids_to_delete_overrides() + + for override in created: + segment_id = override["segment"]["id"] + if segment_id in segment_ids_to_delete: + segment_ids_to_delete.remove(segment_id) + continue + feature_states_to_create.append( + { + "feature_segment": { + "segment": segment_id, + "priority": override["priority"], + }, + "enabled": override["enabled"], + "feature_state_value": FeatureStateValueSerializer( + environment_default.feature_state_value + ).data, + } + ) + + for segment_id in deleted_segment_ids: + staged = [ + feature_state + for feature_state in feature_states_to_create + if feature_state["feature_segment"]["segment"] == segment_id + ] + if not staged: + segment_ids_to_delete.append(segment_id) + for feature_state in staged: + feature_states_to_create.remove(feature_state) + + change_set.feature_states_to_create = json.dumps(feature_states_to_create) + change_set.segment_ids_to_delete_overrides = json.dumps(segment_ids_to_delete) + change_set.save( + update_fields=[ + "feature_states_to_create", + "segment_ids_to_delete_overrides", + ] + ) + + logger.info( + "flag.staged", + organisation__id=environment.project.organisation_id, + project__id=environment.project_id, + environment__id=environment.id, + feature__id=feature.id, + change_request__id=change_request.id, + segment_overrides__created__segment__ids=created_segment_ids, + segment_overrides__deleted__segment__ids=list(deleted_segment_ids), + ) def update_flag( @@ -418,6 +479,7 @@ def update_flag( changes: UpdateFlagRequest, replace: bool, author: AuthorData, + change_request: ChangeRequest | None = None, overwrite_system_segment_overrides: bool, ) -> UpdateFlagResponse: """Write the given parts of a flag, whichever versioning the environment uses. @@ -440,6 +502,17 @@ def update_flag( ], ) + # TODO: Support all changes as per https://github.com/Flagsmith/flagsmith/issues/8738 + if change_request is not None: + _stage_segment_overrides( + environment=environment, + feature=feature, + change_request=change_request, + created=changes.get("segment_overrides", []), + deleted_segment_ids=[], + ) + return get_flag(environment=environment, feature=feature) + segments = _OverriddenSegments([], [], []) with transaction.atomic(): @@ -475,12 +548,12 @@ def update_flag( for segment in Segment.objects.filter(id__in=segments.created): validate_segment_flag_dependencies(segment) - created, deleted = _get_prerequisite_feature_names(segments) - report_flag_dependencies( + report_flag_dependency_changes( environment=environment, feature=feature, - created=created, - deleted=deleted, + created_segment_ids=segments.created, + deleted_segment_ids=segments.deleted, + author=author, ) logger.info( @@ -503,6 +576,7 @@ def delete_segment_override( feature: Feature, segment_id: int, author: AuthorData, + change_request: ChangeRequest | None = None, overwrite_system_segment_overrides: bool, ) -> UpdateFlagResponse: """Remove a flag's override for one segment, leaving the rest of the flag alone. @@ -510,6 +584,16 @@ def delete_segment_override( Overrides of system segments can only be removed by passing `overwrite_system_segment_overrides=True`. """ + if change_request is not None: + _stage_segment_overrides( + environment=environment, + feature=feature, + change_request=change_request, + created=[], + deleted_segment_ids=[segment_id], + ) + return get_flag(environment=environment, feature=feature) + with transaction.atomic(): version = _create_draft_version(environment, feature) feature_states = _get_feature_states_to_write(environment, feature, version) @@ -533,14 +617,12 @@ def delete_segment_override( published_by=author.user, published_by_api_key=author.api_key ) - _, deleted = _get_prerequisite_feature_names( - _OverriddenSegments([], [], [segment_id]) - ) - report_flag_dependencies( + report_flag_dependency_changes( environment=environment, feature=feature, - created=[], - deleted=deleted, + created_segment_ids=[], + deleted_segment_ids=[segment_id], + author=author, ) logger.info( diff --git a/api/features/versioning/models.py b/api/features/versioning/models.py index 93a85254bf69..fa428266109a 100644 --- a/api/features/versioning/models.py +++ b/api/features/versioning/models.py @@ -274,6 +274,13 @@ def get_parsed_feature_states_to_update(self) -> list[dict[str, typing.Any]]: return json.loads(self.feature_states_to_update) # type: ignore[no-any-return] return [] + def get_parsed_segment_ids_to_create_overrides(self) -> list[int]: + return [ + feature_state["feature_segment"]["segment"] + for feature_state in self.get_parsed_feature_states_to_create() + if feature_state.get("feature_segment") + ] + def get_parsed_segment_ids_to_delete_overrides(self) -> list[int]: if self.segment_ids_to_delete_overrides: return json.loads(self.segment_ids_to_delete_overrides) # type: ignore[no-any-return] diff --git a/api/features/versioning/tasks.py b/api/features/versioning/tasks.py index abb885a88ad7..3991389f7123 100644 --- a/api/features/versioning/tasks.py +++ b/api/features/versioning/tasks.py @@ -12,6 +12,7 @@ from audit.constants import ENVIRONMENT_FEATURE_VERSION_PUBLISHED_MESSAGE from audit.models import AuditLog from audit.related_object_type import RelatedObjectType +from core.dataclasses import AuthorData from features.models import FeatureState from features.versioning.exceptions import FeatureVersioningError from features.versioning.models import ( @@ -304,6 +305,7 @@ def publish_version_change_set( # djoser user serializer (which imports settings), we have to use # a local import, since the tasks module gets loaded on app start, # to avoid AppRegistryNotReady error. + from features.dependencies.services import report_flag_dependency_changes from features.versioning.serializers import ( EnvironmentFeatureVersionCreateSerializer, ) @@ -346,6 +348,13 @@ def publish_version_change_set( # in the past since the task processor won't have picked it up and handled # it immediately, but we always care about the _actual_ time it's published. version.publish(published_by=user, live_from=now) + report_flag_dependency_changes( + environment=version_change_set.environment, + feature=version_change_set.feature, + created_segment_ids=serializer.segment_ids_to_create_overrides, + deleted_segment_ids=version_change_set.get_parsed_segment_ids_to_delete_overrides(), + author=AuthorData(user=user), + ) # if live_from was set on the version_change set, then leave it alone for # auditing purposes. diff --git a/api/pyproject.toml b/api/pyproject.toml index 95002097033c..7093e1d9323c 100644 --- a/api/pyproject.toml +++ b/api/pyproject.toml @@ -271,6 +271,10 @@ ignore_missing_imports = true module = ["scim.*"] ignore_missing_imports = true +[[tool.mypy.overrides]] +module = ["workflows_logic.*"] +ignore_missing_imports = true + [[tool.mypy.overrides]] # Suppresses `no-any-return` when the `scim` package isn't installed (which # happens on the public PR workflow that runs mypy without the `--extra private` diff --git a/api/tests/integration/conftest.py b/api/tests/integration/conftest.py index d12f30cfe2f8..1d65cb76e4f4 100644 --- a/api/tests/integration/conftest.py +++ b/api/tests/integration/conftest.py @@ -1,11 +1,14 @@ import json import uuid +from contextlib import contextmanager +from typing import Generator, cast import pytest import requests import responses from django.core.cache import BaseCache from django.core.cache.backends.locmem import LocMemCache +from django.db.models import QuerySet from django.test import Client as DjangoClient from django.urls import reverse from influxdb_client import InfluxDBClient @@ -16,11 +19,18 @@ from app.utils import create_hash from app_analytics.influxdb_wrapper import InfluxDBWrapper +from audit.models import AuditLog from environments.enums import EnvironmentDocumentCacheMode -from features.future.types import SegmentOverrideRequest, UpdateFlagRequest +from features.future.types import ( + SegmentOverrideRequest, + UpdateFlagRequest, + UpdateFlagResponse, +) from organisations.models import Organisation from tests.integration.helpers import create_mv_option_with_api +from tests.integration.types import GetFeatureFixture from tests.types import ( + CaptureAuditLogsFixture, CreateSegmentOverrideFixture, SetMultivariateAllocationsFixture, ) @@ -420,6 +430,33 @@ def feature_segment(admin_client, segment, feature, environment): # type: ignor return response.json()["id"] +@pytest.fixture +def capture_audit_logs() -> CaptureAuditLogsFixture: + @contextmanager + def _capture_audit_logs() -> Generator[QuerySet[AuditLog], None, None]: + last_log_before = AuditLog.objects.only("id").order_by("-id").first() + yield (queryset := AuditLog.objects.none()) + last_log_after = AuditLog.objects.only("id").order_by("-id").first() + queryset.query = AuditLog.objects.filter( # Force context window + id__gt=last_log_before.id if last_log_before else 0, + id__lte=last_log_after.id if last_log_after else 0, + ).query + + return _capture_audit_logs + + +@pytest.fixture +def get_feature(admin_client: APIClient) -> GetFeatureFixture: + def _get_feature(environment_api_key: str, feature_id: int) -> UpdateFlagResponse: + response = admin_client.get( + f"/api/__future__/environments/{environment_api_key}/features/{feature_id}/", + ) + assert response.status_code == 200 + return cast(UpdateFlagResponse, response.json()) + + return _get_feature + + @pytest.fixture() def create_segment_override(admin_client: APIClient) -> CreateSegmentOverrideFixture: """Return a callable putting a segment override live, whichever versioning is in use.""" diff --git a/api/tests/integration/features/dependencies/conftest.py b/api/tests/integration/features/dependencies/conftest.py new file mode 100644 index 000000000000..ff6329f2e2b8 --- /dev/null +++ b/api/tests/integration/features/dependencies/conftest.py @@ -0,0 +1,61 @@ +from datetime import datetime +from typing import cast + +import pytest +from django.utils import timezone +from rest_framework.test import APIClient + +from features.dependencies.types import DependencyEdge +from features.workflows.core.models import ChangeRequest +from tests.integration.types import ( + AddFeaturePrerequisiteFixture, + CreateChangeRequestFixture, +) + + +@pytest.fixture +def add_feature_prerequisite(admin_client: APIClient) -> AddFeaturePrerequisiteFixture: + def _add_feature_prerequisite( + environment_api_key: str, + feature_id: int, + prerequisite_feature_id: int, + ) -> DependencyEdge: + response = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature_id}/dependencies/{prerequisite_feature_id}/", + ) + assert response.status_code == 201 + return cast(DependencyEdge, response.json()) + + return _add_feature_prerequisite + + +@pytest.fixture +def create_change_request( + admin_client: APIClient, + environment_api_key: str, +) -> CreateChangeRequestFixture: + def _create_change_request( + feature_id: int, + title: str, + live_from: datetime | None = None, + ) -> ChangeRequest: + response = admin_client.post( + f"/api/v1/environments/{environment_api_key}/create-change-request/", + { + "title": title, + "feature_states": [], + "change_sets": [ + { + "feature": feature_id, + "live_from": (live_from or timezone.now()).isoformat(), + "feature_states_to_create": [], + "feature_states_to_update": [], + "segment_ids_to_delete_overrides": [], + } + ], + }, + format="json", + ) + return ChangeRequest.objects.get(id=response.json()["id"]) # type: ignore[no-any-return] + + return _create_change_request diff --git a/api/tests/integration/features/dependencies/test_change_requests.py b/api/tests/integration/features/dependencies/test_change_requests.py new file mode 100644 index 000000000000..dd42fad6b28d --- /dev/null +++ b/api/tests/integration/features/dependencies/test_change_requests.py @@ -0,0 +1,1546 @@ +from datetime import timedelta + +import pytest +from common.environments.permissions import ( + MANAGE_SEGMENT_OVERRIDES, + VIEW_ENVIRONMENT, +) +from django.conf import settings +from django.utils import timezone +from freezegun.api import FrozenDateTimeFactory +from pytest_structlog import StructuredLogCapture +from rest_framework.test import APIClient + +from environments.models import Environment +from features.dependencies.exceptions import PrerequisiteHasPrerequisiteError +from features.dependencies.types import DependencyEdge +from features.models import Feature +from features.versioning.tasks import publish_version_change_set +from features.workflows.core.models import ChangeRequest, ChangeRequestApproval +from organisations.models import Organisation +from segments.models import Segment +from tests.integration.types import ( + AddFeaturePrerequisiteFixture, + CreateChangeRequestFixture, + GetFeatureFixture, +) +from tests.types import CaptureAuditLogsFixture, WithEnvironmentPermissionsCallable +from users.models import FFAdminUser + +pytestmark = [ + pytest.mark.skipif( + not settings.WORKFLOWS_LOGIC_INSTALLED, + reason="workflows_logic module not installed (private package extra)", + ), + pytest.mark.usefixtures("environment_v2_versioning"), +] + + +def _enable_change_requests(environment: int) -> None: + Environment.objects.filter(id=environment).update( + minimum_change_request_approvals=1, + ) + + +def _approve_change_request( + change_request: ChangeRequest, approver: FFAdminUser +) -> None: + ChangeRequestApproval.objects.create( + change_request=change_request, + user=approver, + approved_at=timezone.now(), + ) + + +# Direct dependency changes but change requests are enabled + + +def test_add_feature_dependency__bare_but_change_requests_enabled__responds_409( + admin_client: APIClient, + capture_audit_logs: CaptureAuditLogsFixture, + environment: int, + environment_api_key: str, + get_feature: GetFeatureFixture, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + _enable_change_requests(environment) + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/", + ) + + # Then + assert response.status_code == 409 + assert response.json() == { + "code": "change_requests_enabled", + "message": "Cannot make this change where change requests are enabled.", + } + assert not Segment.objects.filter(feature=feature).exists() + assert get_feature(environment_api_key, feature.id)["segment_overrides"] == [] + assert list(audit_logs) == [] + assert not log.has("dependencies.created") + + +def test_delete_feature_dependency__bare_but_change_requests_enabled__responds_409( + add_feature_prerequisite: AddFeaturePrerequisiteFixture, + admin_client: APIClient, + capture_audit_logs: CaptureAuditLogsFixture, + environment: int, + environment_api_key: str, + get_feature: GetFeatureFixture, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + add_feature_prerequisite(environment_api_key, feature.id, prerequisite.id) + _enable_change_requests(environment) + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.delete( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/", + ) + + # Then + assert response.status_code == 409 + assert response.json() == { + "code": "change_requests_enabled", + "message": "Cannot make this change where change requests are enabled.", + } + assert len(get_feature(environment_api_key, feature.id)["segment_overrides"]) == 1 + assert list(audit_logs) == [] + assert not log.has("dependencies.deleted") + + +# Open change requests + + +def test_add_feature_dependency__point_to_open_change_request__responds_201( + admin_client: APIClient, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + _enable_change_requests(environment) + change_request = create_change_request(feature.id, title="Pending") + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 201 + assert response.json() == ( + dependency := DependencyEdge( + { + "feature": {"id": feature.id, "name": "checkout"}, + "prerequisite": {"id": prerequisite.id, "name": "payments"}, + "segment": { + "id": response.json()["segment"]["id"], + "name": "checkout-depends-on-payments", + "rules": [ + { + "type": "ANY", + "conditions": [ + { + "property": '$.flags["payments"].enabled', + "operator": "NOT_EQUAL", + "value": "true", + "description": None, + } + ], + "rules": [], + } + ], + "condition_json_path": "$[0].conditions[0]", + "is_system": True, + }, + } + ) + ) + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/?change_request={change_request.id}", + ).json() == {"results": [dependency]} + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": []} + assert list(audit_logs) == [] + assert not log.has("dependencies.created") + + +def test_delete_feature_dependency__point_to_open_change_request__responds_204( + add_feature_prerequisite: AddFeaturePrerequisiteFixture, + admin_client: APIClient, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + live_dependency = add_feature_prerequisite( + environment_api_key, feature.id, prerequisite.id + ) + _enable_change_requests(environment) + change_request = create_change_request(feature.id, title="Pending") + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.delete( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 204 + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/?change_request={change_request.id}", + ).json() == {"results": []} + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": [live_dependency]} + assert list(audit_logs) == [] + assert not log.has("dependencies.deleted") + + +def test_list_feature_dependencies__point_to_open_change_request__responds_200( + add_feature_prerequisite: AddFeaturePrerequisiteFixture, + admin_client: APIClient, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + organisation: int, + project: int, + staff_client: APIClient, + staff_user: FFAdminUser, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + payments = Feature.objects.create(name="payments", project_id=project) + inventory = Feature.objects.create(name="inventory", project_id=project) + shipping = Feature.objects.create(name="shipping", project_id=project) + staff_user.add_organisation(Organisation.objects.get(id=organisation)) + with_environment_permissions([VIEW_ENVIRONMENT], environment, False) + live_payments_dependency = add_feature_prerequisite( + environment_api_key, feature.id, payments.id + ) + live_inventory_dependency = add_feature_prerequisite( + environment_api_key, feature.id, inventory.id + ) + _enable_change_requests(environment) + change_request = create_change_request(feature.id, title="Pending") + staged_shipping_dependency = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{shipping.id}/?change_request={change_request.id}", + ).json() + admin_client.delete( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{inventory.id}/?change_request={change_request.id}", + ) + + # When + with capture_audit_logs() as audit_logs: + response = staff_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 200 + assert response.json() == { + "results": [live_payments_dependency, staged_shipping_dependency] + } + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": [live_payments_dependency, live_inventory_dependency]} + assert list(audit_logs) == [] + + +# Approved change requests + + +def test_add_feature_dependency__point_to_approved_change_request__responds_400( + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + _enable_change_requests(environment) + change_request = create_change_request(feature.id, title="Approved") + _approve_change_request(change_request, admin_user) + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 400 + assert response.json() == { + "code": "change_request_approved", + "message": "Cannot change an approved change request.", + } + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/?change_request={change_request.id}", + ).json() == {"results": []} + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": []} + assert list(audit_logs) == [] + assert not log.has("dependencies.created") + + +def test_delete_feature_dependency__point_to_approved_change_request__responds_400( + add_feature_prerequisite: AddFeaturePrerequisiteFixture, + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + live_dependency = add_feature_prerequisite( + environment_api_key, feature.id, prerequisite.id + ) + _enable_change_requests(environment) + change_request = create_change_request(feature.id, title="Approved") + _approve_change_request(change_request, admin_user) + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.delete( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 400 + assert response.json() == { + "code": "change_request_approved", + "message": "Cannot change an approved change request.", + } + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/?change_request={change_request.id}", + ).json() == {"results": [live_dependency]} + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": [live_dependency]} + assert list(audit_logs) == [] + assert not log.has("dependencies.deleted") + + +def test_list_feature_dependencies__point_to_approved_change_request__responds_200( + add_feature_prerequisite: AddFeaturePrerequisiteFixture, + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + payments = Feature.objects.create(name="payments", project_id=project) + inventory = Feature.objects.create(name="inventory", project_id=project) + shipping = Feature.objects.create(name="shipping", project_id=project) + live_payments_dependency = add_feature_prerequisite( + environment_api_key, feature.id, payments.id + ) + live_inventory_dependency = add_feature_prerequisite( + environment_api_key, feature.id, inventory.id + ) + _enable_change_requests(environment) + change_request = create_change_request(feature.id, title="Approved") + staged_shipping_dependency = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{shipping.id}/?change_request={change_request.id}", + ).json() + admin_client.delete( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{inventory.id}/?change_request={change_request.id}", + ) + _approve_change_request(change_request, admin_user) + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 200 + assert response.json() == { + "results": [live_payments_dependency, staged_shipping_dependency] + } + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": [live_payments_dependency, live_inventory_dependency]} + assert list(audit_logs) == [] + + +# Committed change requests + + +def test_add_feature_dependency__point_to_committed_change_request__responds_400( + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + _enable_change_requests(environment) + change_request = create_change_request(feature.id, title="Committed") + _approve_change_request(change_request, admin_user) + change_request.commit(committed_by=admin_user) + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 400 + assert response.json() == { + "code": "change_request_committed", + "message": "The change request is already committed.", + } + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": []} + assert list(audit_logs) == [] + assert not log.has("dependencies.created") + + +def test_delete_feature_dependency__point_to_committed_change_request__responds_400( + add_feature_prerequisite: AddFeaturePrerequisiteFixture, + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + live_dependency = add_feature_prerequisite( + environment_api_key, feature.id, prerequisite.id + ) + _enable_change_requests(environment) + change_request = create_change_request(feature.id, title="Committed") + _approve_change_request(change_request, admin_user) + change_request.commit(committed_by=admin_user) + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.delete( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 400 + assert response.json() == { + "code": "change_request_committed", + "message": "The change request is already committed.", + } + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": [live_dependency]} + assert list(audit_logs) == [] + assert not log.has("dependencies.deleted") + + +def test_list_feature_dependencies__point_to_committed_change_request__responds_400( + add_feature_prerequisite: AddFeaturePrerequisiteFixture, + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + payments = Feature.objects.create(name="payments", project_id=project) + inventory = Feature.objects.create(name="inventory", project_id=project) + shipping = Feature.objects.create(name="shipping", project_id=project) + live_payments_dependency = add_feature_prerequisite( + environment_api_key, feature.id, payments.id + ) + add_feature_prerequisite(environment_api_key, feature.id, inventory.id) + _enable_change_requests(environment) + change_request = create_change_request(feature.id, title="Committed") + staged_shipping_dependency = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{shipping.id}/?change_request={change_request.id}", + ).json() + admin_client.delete( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{inventory.id}/?change_request={change_request.id}", + ) + _approve_change_request(change_request, admin_user) + change_request.commit(committed_by=admin_user) + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 400 + assert response.json() == { + "code": "change_request_committed", + "message": "The change request is already committed.", + } + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": [live_payments_dependency, staged_shipping_dependency]} + assert list(audit_logs) == [] + + +# Unusable change requests + + +def test_list_feature_dependencies__point_to_unknown_change_request__responds_404( + admin_client: APIClient, + environment: int, + environment_api_key: str, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + _enable_change_requests(environment) + + # When + response = admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/?change_request=999999", + ) + + # Then + assert response.status_code == 404 + assert response.json() == { + "code": "change_request_not_found", + "message": "Change request not found.", + } + + +def test_add_feature_dependency__change_requests_disabled__responds_409( + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + change_request = ChangeRequest.objects.create( + environment_id=environment, title="Pending", user=admin_user + ) + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 409 + assert response.json() == { + "code": "change_requests_disabled", + "message": "Enable Feature Change Requests to change dependencies in change requests.", + } + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": []} + assert list(audit_logs) == [] + assert not log.has("dependencies.created") + + +# Feature versioning v1 + + +@pytest.mark.parametrize("environment_v2_versioning", [None]) +def test_add_feature_dependency__feature_versioning_v1__responds_409( + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + _enable_change_requests(environment) + change_request = ChangeRequest.objects.create( + environment_id=environment, title="Pending", user=admin_user + ) + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 409 + assert response.json() == { + "code": "feature_versioning_disabled", + "message": "Enable Feature Versioning to change dependencies in change requests.", + } + assert not Segment.objects.filter(feature=feature).exists() + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": []} + assert list(audit_logs) == [] + assert not log.has("dependencies.created") + + +@pytest.mark.parametrize("environment_v2_versioning", [None]) +def test_delete_feature_dependency__feature_versioning_v1__responds_409( + add_feature_prerequisite: AddFeaturePrerequisiteFixture, + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + live_dependency = add_feature_prerequisite( + environment_api_key, feature.id, prerequisite.id + ) + _enable_change_requests(environment) + change_request = ChangeRequest.objects.create( + environment_id=environment, title="Pending", user=admin_user + ) + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.delete( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 409 + assert response.json() == { + "code": "feature_versioning_disabled", + "message": "Enable Feature Versioning to change dependencies in change requests.", + } + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": [live_dependency]} + assert list(audit_logs) == [] + assert not log.has("dependencies.deleted") + + +# Lacking permission + + +def test_add_feature_dependency__lacking_permission__responds_403( + admin_client: APIClient, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + organisation: int, + project: int, + staff_client: APIClient, + staff_user: FFAdminUser, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + _enable_change_requests(environment) + change_request = create_change_request(feature.id, title="Pending") + staff_user.add_organisation(Organisation.objects.get(id=organisation)) + with_environment_permissions([MANAGE_SEGMENT_OVERRIDES], environment, False) + + # When + with capture_audit_logs() as audit_logs: + response = staff_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 403 + assert response.json() == { + "code": "permission_denied", + "message": 'The permission "Create change request" is necessary to change dependencies in change requests.', + } + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/?change_request={change_request.id}", + ).json() == {"results": []} + assert list(audit_logs) == [] + assert not log.has("dependencies.created") + + +def test_delete_feature_dependency__lacking_permission__responds_403( + add_feature_prerequisite: AddFeaturePrerequisiteFixture, + admin_client: APIClient, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + organisation: int, + project: int, + staff_client: APIClient, + staff_user: FFAdminUser, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + live_dependency = add_feature_prerequisite( + environment_api_key, feature.id, prerequisite.id + ) + _enable_change_requests(environment) + change_request = create_change_request(feature.id, title="Pending") + staff_user.add_organisation(Organisation.objects.get(id=organisation)) + with_environment_permissions([MANAGE_SEGMENT_OVERRIDES], environment, False) + + # When + with capture_audit_logs() as audit_logs: + response = staff_client.delete( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 403 + assert response.json() == { + "code": "permission_denied", + "message": 'The permission "Create change request" is necessary to change dependencies in change requests.', + } + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/?change_request={change_request.id}", + ).json() == {"results": [live_dependency]} + assert list(audit_logs) == [] + assert not log.has("dependencies.deleted") + + +def test_add_feature_dependency__unrelated_to_environment__responds_404( + admin_client: APIClient, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + project: int, + staff_client: APIClient, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + _enable_change_requests(environment) + change_request = create_change_request(feature.id, title="Pending") + + # When + with capture_audit_logs() as audit_logs: + response = staff_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 404 + assert response.json() == { + "code": "environment_not_found", + "message": f"Environment key '{environment_api_key}' does not exist.", + } + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/?change_request={change_request.id}", + ).json() == {"results": []} + assert list(audit_logs) == [] + assert not log.has("dependencies.created") + + +# Stage hygiene + + +def test_delete_feature_dependency__previously_staged__responds_204_undoing_staged( + admin_client: APIClient, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + _enable_change_requests(environment) + change_request = create_change_request(feature.id, title="Pending") + admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.delete( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 204 + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/?change_request={change_request.id}", + ).json() == {"results": []} + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": []} + assert list(audit_logs) == [] + assert not log.has("dependencies.created") + assert not log.has("dependencies.deleted") + + +def test_add_feature_dependency__previously_staged_for_deletion__responds_201_keeping_live( + add_feature_prerequisite: AddFeaturePrerequisiteFixture, + admin_client: APIClient, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + live_dependency = add_feature_prerequisite( + environment_api_key, feature.id, prerequisite.id + ) + _enable_change_requests(environment) + change_request = create_change_request(feature.id, title="Pending") + admin_client.delete( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 201 + assert response.json() == live_dependency + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/?change_request={change_request.id}", + ).json() == {"results": [live_dependency]} + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": [live_dependency]} + assert list(audit_logs) == [] + assert not log.has("dependencies.deleted") + + +def test_add_feature_dependency__previously_staged__responds_400_leaving_one_staged( + admin_client: APIClient, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + environment_name: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + _enable_change_requests(environment) + change_request = create_change_request(feature.id, title="Pending") + staged_dependency = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ).json() + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 400 + assert response.json() == { + "code": "dependency_exists", + "message": 'The feature "checkout" already depends on the feature "payments".', + "environment": {"key": environment_api_key, "name": environment_name}, + "path": [staged_dependency], + } + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/?change_request={change_request.id}", + ).json() == {"results": [staged_dependency]} + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": []} + assert list(audit_logs) == [] + assert not log.has("dependencies.created") + + +# Prerequisite depth + + +def test_add_feature_dependency__prerequisite_has_live_prerequisite__responds_400( + add_feature_prerequisite: AddFeaturePrerequisiteFixture, + admin_client: APIClient, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + environment_name: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + checkout = Feature.objects.create(name="checkout", project_id=project) + payments = Feature.objects.create(name="payments", project_id=project) + inventory = Feature.objects.create(name="inventory", project_id=project) + live_inventory_dependency = add_feature_prerequisite( + environment_api_key, payments.id, inventory.id + ) + _enable_change_requests(environment) + change_request = create_change_request(checkout.id, title="Pending") + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{checkout.id}/dependencies/{payments.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 400 + assert response.json() == { + "code": "prerequisite_has_prerequisite", + "message": 'The prerequisite "payments" already has a prerequisite "inventory".', + "environment": {"key": environment_api_key, "name": environment_name}, + "path": [live_inventory_dependency], + } + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{checkout.id}/dependencies/?change_request={change_request.id}", + ).json() == {"results": []} + assert list(audit_logs) == [] + assert not log.has("dependencies.created", feature__name="checkout") + + +def test_add_feature_dependency__prerequisite_has_scheduled_prerequisite__responds_400( + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + environment_name: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + checkout = Feature.objects.create(name="checkout", project_id=project) + payments = Feature.objects.create(name="payments", project_id=project) + inventory = Feature.objects.create(name="inventory", project_id=project) + _enable_change_requests(environment) + scheduled_change_request = create_change_request( + payments.id, title="Scheduled", live_from=timezone.now() + timedelta(days=1) + ) + scheduled_inventory_dependency = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{payments.id}/dependencies/{inventory.id}/?change_request={scheduled_change_request.id}", + ).json() + _approve_change_request(scheduled_change_request, admin_user) + scheduled_change_request.commit(committed_by=admin_user) + change_request = create_change_request(checkout.id, title="Pending") + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{checkout.id}/dependencies/{payments.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 400 + assert response.json() == { + "code": "prerequisite_has_prerequisite", + "message": 'The prerequisite "payments" already has a prerequisite "inventory".', + "environment": {"key": environment_api_key, "name": environment_name}, + "path": [scheduled_inventory_dependency], + } + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{checkout.id}/dependencies/?change_request={change_request.id}", + ).json() == {"results": []} + assert list(audit_logs) == [] + assert not log.has("dependencies.created") + + +def test_add_feature_dependency__prerequisite_has_prerequisite_scheduled_for_deletion__responds_400( + add_feature_prerequisite: AddFeaturePrerequisiteFixture, + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + environment_name: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + checkout = Feature.objects.create(name="checkout", project_id=project) + payments = Feature.objects.create(name="payments", project_id=project) + inventory = Feature.objects.create(name="inventory", project_id=project) + live_inventory_dependency = add_feature_prerequisite( + environment_api_key, payments.id, inventory.id + ) + _enable_change_requests(environment) + scheduled_change_request = create_change_request( + payments.id, title="Scheduled", live_from=timezone.now() + timedelta(days=1) + ) + admin_client.delete( + f"/api/v1/environments/{environment_api_key}/features/{payments.id}/dependencies/{inventory.id}/?change_request={scheduled_change_request.id}", + ) + _approve_change_request(scheduled_change_request, admin_user) + scheduled_change_request.commit(committed_by=admin_user) + change_request = create_change_request(checkout.id, title="Pending") + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{checkout.id}/dependencies/{payments.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 400 + assert response.json() == { + "code": "prerequisite_has_prerequisite", + "message": 'The prerequisite "payments" already has a prerequisite "inventory".', + "environment": {"key": environment_api_key, "name": environment_name}, + "path": [live_inventory_dependency], + } + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{checkout.id}/dependencies/?change_request={change_request.id}", + ).json() == {"results": []} + assert list(audit_logs) == [] + assert not log.has("dependencies.created", feature__name="checkout") + + +def test_add_feature_dependency__feature_is_prerequisite_scheduled_for_deletion__responds_400( + add_feature_prerequisite: AddFeaturePrerequisiteFixture, + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + environment_name: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + checkout = Feature.objects.create(name="checkout", project_id=project) + payments = Feature.objects.create(name="payments", project_id=project) + inventory = Feature.objects.create(name="inventory", project_id=project) + live_payments_dependency = add_feature_prerequisite( + environment_api_key, checkout.id, payments.id + ) + _enable_change_requests(environment) + scheduled_change_request = create_change_request( + checkout.id, title="Scheduled", live_from=timezone.now() + timedelta(days=1) + ) + admin_client.delete( + f"/api/v1/environments/{environment_api_key}/features/{checkout.id}/dependencies/{payments.id}/?change_request={scheduled_change_request.id}", + ) + _approve_change_request(scheduled_change_request, admin_user) + scheduled_change_request.commit(committed_by=admin_user) + change_request = create_change_request(payments.id, title="Pending") + + # When + with capture_audit_logs() as audit_logs: + response = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{payments.id}/dependencies/{inventory.id}/?change_request={change_request.id}", + ) + + # Then + assert response.status_code == 400 + assert response.json() == { + "code": "feature_is_prerequisite", + "message": 'The feature "payments" is already a prerequisite for the feature "checkout".', + "environment": {"key": environment_api_key, "name": environment_name}, + "path": [live_payments_dependency], + } + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{payments.id}/dependencies/?change_request={change_request.id}", + ).json() == {"results": []} + assert list(audit_logs) == [] + assert not log.has("dependencies.created", feature__name="payments") + + +def test_commit_change_request__prerequisite_gained_prerequisite_since_staged__raises_and_publishes_nothing( + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + organisation: int, + project: int, +) -> None: + # Given + checkout = Feature.objects.create(name="checkout", project_id=project) + payments = Feature.objects.create(name="payments", project_id=project) + inventory = Feature.objects.create(name="inventory", project_id=project) + _enable_change_requests(environment) + payments_change_request = create_change_request(payments.id, title="Payments") + admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{payments.id}/dependencies/{inventory.id}/?change_request={payments_change_request.id}", + ) + checkout_change_request = create_change_request(checkout.id, title="Checkout") + admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{checkout.id}/dependencies/{payments.id}/?change_request={checkout_change_request.id}", + ) + _approve_change_request(payments_change_request, admin_user) + _approve_change_request(checkout_change_request, admin_user) + payments_change_request.commit(committed_by=admin_user) + + # When + with capture_audit_logs() as audit_logs: + with pytest.raises(PrerequisiteHasPrerequisiteError): + checkout_change_request.commit(committed_by=admin_user) + + # Then + checkout_change_request.refresh_from_db() + assert checkout_change_request.committed_at is None + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{checkout.id}/dependencies/", + ).json() == {"results": []} + assert list(audit_logs) == [] + assert log.has( + "dependencies.create_failed", + level="info", + organisation__id=organisation, + project__id=project, + environment__key=environment_api_key, + feature__name="checkout", + prerequisite_feature__name="payments", + ) + + +def test_commit_change_request__prerequisite_has_prerequisite_scheduled_for_deletion__raises_and_publishes_nothing( + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + organisation: int, + project: int, +) -> None: + # Given + checkout = Feature.objects.create(name="checkout", project_id=project) + payments = Feature.objects.create(name="payments", project_id=project) + inventory = Feature.objects.create(name="inventory", project_id=project) + _enable_change_requests(environment) + checkout_change_request = create_change_request(checkout.id, title="Checkout") + admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{checkout.id}/dependencies/{payments.id}/?change_request={checkout_change_request.id}", + ) + payments_change_request = create_change_request(payments.id, title="Payments") + admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{payments.id}/dependencies/{inventory.id}/?change_request={payments_change_request.id}", + ) + _approve_change_request(payments_change_request, admin_user) + payments_change_request.commit(committed_by=admin_user) + scheduled_change_request = create_change_request( + payments.id, title="Scheduled", live_from=timezone.now() + timedelta(days=1) + ) + admin_client.delete( + f"/api/v1/environments/{environment_api_key}/features/{payments.id}/dependencies/{inventory.id}/?change_request={scheduled_change_request.id}", + ) + _approve_change_request(scheduled_change_request, admin_user) + scheduled_change_request.commit(committed_by=admin_user) + _approve_change_request(checkout_change_request, admin_user) + + # When + with capture_audit_logs() as audit_logs: + with pytest.raises(PrerequisiteHasPrerequisiteError): + checkout_change_request.commit(committed_by=admin_user) + + # Then + checkout_change_request.refresh_from_db() + assert checkout_change_request.committed_at is None + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{checkout.id}/dependencies/", + ).json() == {"results": []} + assert list(audit_logs) == [] + assert log.has( + "dependencies.create_failed", + level="info", + organisation__id=organisation, + project__id=project, + environment__key=environment_api_key, + feature__name="checkout", + prerequisite_feature__name="payments", + ) + + +# Audit logs + + +def test_commit_change_request__scheduled_dependency_addition__produces_no_audit_log( + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + _enable_change_requests(environment) + change_request = create_change_request( + feature.id, title="Scheduled", live_from=timezone.now() + timedelta(days=1) + ) + admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + _approve_change_request(change_request, admin_user) + + # When + with capture_audit_logs() as audit_logs: + change_request.commit(committed_by=admin_user) + + # Then + assert list(audit_logs.values("related_object_type", "log")) == [ + { + "related_object_type": "CHANGE_REQUEST", + "log": "Change Request: Scheduled committed", + }, + ] + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": []} + assert not log.has("dependencies.created") + + +def test_commit_change_request__scheduled_dependency_deletion__produces_no_audit_log( + add_feature_prerequisite: AddFeaturePrerequisiteFixture, + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + live_dependency = add_feature_prerequisite( + environment_api_key, feature.id, prerequisite.id + ) + _enable_change_requests(environment) + change_request = create_change_request( + feature.id, title="Scheduled", live_from=timezone.now() + timedelta(days=1) + ) + admin_client.delete( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + _approve_change_request(change_request, admin_user) + + # When + with capture_audit_logs() as audit_logs: + change_request.commit(committed_by=admin_user) + + # Then + assert list(audit_logs.values("related_object_type", "log")) == [ + { + "related_object_type": "CHANGE_REQUEST", + "log": "Change Request: Scheduled committed", + }, + ] + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": [live_dependency]} + assert not log.has("dependencies.deleted") + + +def test_commit_change_request__dependency_addition_live_now__produces_audit_log( + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + organisation: int, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + _enable_change_requests(environment) + change_request = create_change_request(feature.id, title="Live now") + staged_dependency = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ).json() + _approve_change_request(change_request, admin_user) + + # When + with capture_audit_logs() as audit_logs: + change_request.commit(committed_by=admin_user) + + # Then + assert list(audit_logs.order_by("id").values("related_object_type", "log")) == [ + { + "related_object_type": "EF_VERSION", + "log": "New version published for feature: checkout", + }, + { + "related_object_type": "FEATURE", + "log": "Feature 'payments' added as a dependency for feature 'checkout'.", + }, + { + "related_object_type": "CHANGE_REQUEST", + "log": "Change Request: Live now committed", + }, + ] + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": [staged_dependency]} + assert log.has( + "dependencies.created", + level="info", + organisation__id=organisation, + project__id=project, + environment__key=environment_api_key, + feature__name="checkout", + prerequisite_feature__name="payments", + ) + + +def test_commit_change_request__dependency_deletion_live_now__produces_audit_log( + add_feature_prerequisite: AddFeaturePrerequisiteFixture, + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + log: StructuredLogCapture, + organisation: int, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + add_feature_prerequisite(environment_api_key, feature.id, prerequisite.id) + _enable_change_requests(environment) + change_request = create_change_request(feature.id, title="Live now") + admin_client.delete( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + _approve_change_request(change_request, admin_user) + + # When + with capture_audit_logs() as audit_logs: + change_request.commit(committed_by=admin_user) + + # Then + assert list(audit_logs.order_by("id").values("related_object_type", "log")) == [ + { + "related_object_type": "EF_VERSION", + "log": "New version published for feature: checkout", + }, + { + "related_object_type": "FEATURE", + "log": "Feature 'payments' removed as a dependency for feature 'checkout'.", + }, + { + "related_object_type": "CHANGE_REQUEST", + "log": "Change Request: Live now committed", + }, + ] + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": []} + assert log.has( + "dependencies.deleted", + level="info", + organisation__id=organisation, + project__id=project, + environment__key=environment_api_key, + feature__name="checkout", + prerequisite_feature__name="payments", + ) + + +def test_publish_change_request__dependency_addition_going_live__produces_audit_log( + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + freezer: FrozenDateTimeFactory, + log: StructuredLogCapture, + organisation: int, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + _enable_change_requests(environment) + live_from = timezone.now() + timedelta(days=1) + change_request = create_change_request( + feature.id, title="Scheduled", live_from=live_from + ) + staged_dependency = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ).json() + _approve_change_request(change_request, admin_user) + change_request.commit(committed_by=admin_user) + freezer.move_to(live_from) + + # When + with capture_audit_logs() as audit_logs: + publish_version_change_set( + version_change_set_id=change_request.change_sets.get().id, + user_id=admin_user.id, + is_scheduled=True, + ) + + # Then + assert list(audit_logs.order_by("id").values("related_object_type", "log")) == [ + { + "related_object_type": "EF_VERSION", + "log": "New version published for feature: checkout", + }, + { + "related_object_type": "FEATURE", + "log": "Feature 'payments' added as a dependency for feature 'checkout'.", + }, + ] + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": [staged_dependency]} + assert log.has( + "dependencies.created", + level="info", + organisation__id=organisation, + project__id=project, + environment__key=environment_api_key, + feature__name="checkout", + prerequisite_feature__name="payments", + ) + + +def test_publish_change_request__dependency_deletion_going_live__produces_audit_log( + add_feature_prerequisite: AddFeaturePrerequisiteFixture, + admin_client: APIClient, + admin_user: FFAdminUser, + capture_audit_logs: CaptureAuditLogsFixture, + create_change_request: CreateChangeRequestFixture, + environment: int, + environment_api_key: str, + freezer: FrozenDateTimeFactory, + log: StructuredLogCapture, + organisation: int, + project: int, +) -> None: + # Given + feature = Feature.objects.create(name="checkout", project_id=project) + prerequisite = Feature.objects.create(name="payments", project_id=project) + freezer.tick() + add_feature_prerequisite(environment_api_key, feature.id, prerequisite.id) + _enable_change_requests(environment) + live_from = timezone.now() + timedelta(days=1) + change_request = create_change_request( + feature.id, title="Scheduled", live_from=live_from + ) + admin_client.delete( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/{prerequisite.id}/?change_request={change_request.id}", + ) + _approve_change_request(change_request, admin_user) + change_request.commit(committed_by=admin_user) + freezer.move_to(live_from) + + # When + with capture_audit_logs() as audit_logs: + publish_version_change_set( + version_change_set_id=change_request.change_sets.get().id, + user_id=admin_user.id, + is_scheduled=True, + ) + + # Then + assert list(audit_logs.order_by("id").values("related_object_type", "log")) == [ + { + "related_object_type": "EF_VERSION", + "log": "New version published for feature: checkout", + }, + { + "related_object_type": "FEATURE", + "log": "Feature 'payments' removed as a dependency for feature 'checkout'.", + }, + ] + assert admin_client.get( + f"/api/v1/environments/{environment_api_key}/features/{feature.id}/dependencies/", + ).json() == {"results": []} + assert log.has( + "dependencies.deleted", + level="info", + organisation__id=organisation, + project__id=project, + environment__key=environment_api_key, + feature__name="checkout", + prerequisite_feature__name="payments", + ) diff --git a/api/tests/integration/types.py b/api/tests/integration/types.py new file mode 100644 index 000000000000..e5383fb5787c --- /dev/null +++ b/api/tests/integration/types.py @@ -0,0 +1,32 @@ +from datetime import datetime +from typing import Protocol + +from features.dependencies.types import DependencyEdge +from features.future.types import UpdateFlagResponse +from features.workflows.core.models import ChangeRequest + + +class GetFeatureFixture(Protocol): + def __call__( + self, + environment_api_key: str, + feature_id: int, + ) -> UpdateFlagResponse: ... + + +class AddFeaturePrerequisiteFixture(Protocol): + def __call__( + self, + environment_api_key: str, + feature_id: int, + prerequisite_feature_id: int, + ) -> DependencyEdge: ... + + +class CreateChangeRequestFixture(Protocol): + def __call__( + self, + feature_id: int, + title: str, + live_from: datetime | None = None, + ) -> ChangeRequest: ... diff --git a/api/tests/types.py b/api/tests/types.py index 1aa83f95be08..e1e752192a93 100644 --- a/api/tests/types.py +++ b/api/tests/types.py @@ -1,8 +1,11 @@ +from contextlib import AbstractContextManager from datetime import datetime from typing import Callable, Literal, Optional, Protocol +from django.db.models import QuerySet from django_test_migrations.migrator import Migrator +from audit.models import AuditLog from environments.identities.models import Identity from environments.models import Environment from environments.permissions.models import UserEnvironmentPermission @@ -86,3 +89,7 @@ def __call__( committed: bool = False, live_from: datetime | None = None, ) -> None: ... + + +class CaptureAuditLogsFixture(Protocol): + def __call__(self) -> AbstractContextManager[QuerySet[AuditLog]]: ... diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index 4b99ccce2362..77e2f179ff58 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -305,11 +305,12 @@ Attributes: ### `features.dependencies.create_failed` Logged at `info` from: - - `api/features/dependencies/services.py:171` - - `api/features/dependencies/services.py:371` - - `api/features/dependencies/services.py:387` - - `api/features/dependencies/services.py:392` - - `api/features/dependencies/services.py:402` + - `api/features/dependencies/services.py:212` + - `api/features/dependencies/services.py:498` + - `api/features/dependencies/services.py:503` + - `api/features/dependencies/services.py:513` + - `api/features/dependencies/services.py:570` + - `api/features/dependencies/services.py:573` Attributes: - `environment.key` @@ -321,7 +322,7 @@ Attributes: ### `features.dependencies.created` Logged at `info` from: - - `api/features/dependencies/services.py:134` + - `api/features/dependencies/services.py:138` Attributes: - `environment.key` @@ -333,7 +334,8 @@ Attributes: ### `features.dependencies.delete_failed` Logged at `info` from: - - `api/features/dependencies/services.py:482` + - `api/features/dependencies/services.py:645` + - `api/features/dependencies/services.py:659` Attributes: - `environment.key` @@ -345,8 +347,8 @@ Attributes: ### `features.dependencies.deleted` Logged at `info` from: - - `api/features/dependencies/services.py:106` - - `api/features/dependencies/services.py:132` + - `api/features/dependencies/services.py:110` + - `api/features/dependencies/services.py:136` Attributes: - `environment.key` @@ -355,6 +357,20 @@ Attributes: - `prerequisite_feature.name` - `project.id` +### `features.flag.staged` + +Logged at `info` from: + - `api/features/future/services.py:463` + +Attributes: + - `change_request.id` + - `environment.id` + - `feature.id` + - `organisation.id` + - `project.id` + - `segment_overrides.created.segment.ids` + - `segment_overrides.deleted.segment.ids` + ### `features.flag.update_rejected` Logged at `warning` from: @@ -370,8 +386,8 @@ Attributes: ### `features.flag.updated` Logged at `info` from: - - `api/features/future/services.py:486` - - `api/features/future/services.py:546` + - `api/features/future/services.py:559` + - `api/features/future/services.py:628` Attributes: - `environment.id` @@ -983,7 +999,7 @@ Attributes: ### `workflows.change_request.committed` Logged at `info` from: - - `api/core/workflows_services.py:45` + - `api/core/workflows_services.py:49` Attributes: - `environment.id` @@ -993,7 +1009,7 @@ Attributes: ### `workflows.missing_live_segment` Logged at `warning` from: - - `api/core/workflows_services.py:130` + - `api/core/workflows_services.py:133` Attributes: - `draft_segment` @@ -1001,7 +1017,7 @@ Attributes: ### `workflows.segment_revision_created` Logged at `info` from: - - `api/core/workflows_services.py:135` + - `api/core/workflows_services.py:138` Attributes: - `revision_id`