-
Notifications
You must be signed in to change notification settings - Fork 458
feat: initial PoC for new endpoint to update a flag, regardless of versioning #6221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| from rest_framework import status | ||
| from rest_framework.decorators import api_view | ||
| from rest_framework.request import Request | ||
| from rest_framework.response import Response | ||
|
|
||
| from environments.models import Environment | ||
| from features.models import Feature | ||
| from features.serializers import UpdateFlagSerializer | ||
|
|
||
|
|
||
| @api_view(http_method_names=["POST"]) | ||
| def update_flag(request: Request, environment_id: int, feature_name: str) -> Response: | ||
| environment = Environment.objects.get(id=environment_id) | ||
| feature = Feature.objects.get(name=feature_name, project_id=environment.project_id) | ||
|
|
||
| serializer = UpdateFlagSerializer( | ||
| data=request.data, context={"request": request, "view": update_flag} | ||
| ) | ||
| serializer.is_valid(raise_exception=True) | ||
| serializer.save(environment=environment, feature=feature) | ||
|
|
||
| return Response(serializer.data, status=status.HTTP_200_OK) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import typing | ||
| from datetime import datetime | ||
| from typing import Any | ||
|
|
||
|
|
@@ -18,6 +19,7 @@ | |
|
|
||
| from app_analytics.serializers import LabelsQuerySerializerMixin, LabelsSerializer | ||
| from environments.identities.models import Identity | ||
| from environments.models import Environment | ||
| from environments.sdk.serializers_mixins import ( | ||
| HideSensitiveFieldsSerializerMixin, | ||
| ) | ||
|
|
@@ -28,6 +30,7 @@ | |
| FeatureFlagCodeReferencesRepositoryCountSerializer, | ||
| ) | ||
| from projects.models import Project | ||
| from users.models import FFAdminUser | ||
| from users.serializers import ( | ||
| UserIdsSerializer, | ||
| UserListSerializer, | ||
|
|
@@ -47,6 +50,8 @@ | |
| ) | ||
| from .models import Feature, FeatureState | ||
| from .multivariate.serializers import NestedMultivariateFeatureOptionSerializer | ||
| from .versioning.dataclasses import FlagChangeSet | ||
| from .versioning.versioning_service import update_flag | ||
|
|
||
|
|
||
| class FeatureStateSerializerSmall(serializers.ModelSerializer): # type: ignore[type-arg] | ||
|
|
@@ -671,3 +676,45 @@ def create(self, validated_data: dict) -> FeatureState: # type: ignore[type-arg | |
| {"environment": SEGMENT_OVERRIDE_LIMIT_EXCEEDED_MESSAGE} | ||
| ) | ||
| return super().create(validated_data) # type: ignore[no-any-return,no-untyped-call] | ||
|
|
||
|
|
||
| class UpdateFlagSerializer(serializers.Serializer): # type: ignore[type-arg] | ||
| enabled = serializers.BooleanField(required=False) | ||
|
|
||
| # TODO: this is introducing _another_ way of handling typing, but it feels closer | ||
| # to what we should have done from the start. This might be out of scope for this | ||
| # work though. | ||
| feature_state_value = serializers.CharField(required=False) | ||
| type = serializers.ChoiceField( | ||
| choices=["int", "string", "bool", "float"], | ||
| required=False, | ||
| default="string", | ||
| ) | ||
|
|
||
| segment_id = serializers.IntegerField(required=False) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to provide priority here as well
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed we @gagantrivedi, we plan to focus on the environment default case for now, and ignore for segments for now. The logic behind this is it creates a much more streamlined endpoint here for users that don't care about segments. We could then either:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On further discussions, we have gone back to accommodating segment overrides in this endpoint. We should make priority optional (if the priority is missing, it just gets added to the end of the list). We see that there are 2 main use cases here:
The endpoint should handle creating or updating a segment override, and should return in the response payload whether the change was an update or not (the status code should still be 200). |
||
|
|
||
| # TODO: multivariate? | ||
|
|
||
| @property | ||
| def flag_change_set(self) -> FlagChangeSet: | ||
| validated_data = self.validated_data | ||
| change_set = FlagChangeSet( | ||
| enabled=validated_data.get("enabled"), | ||
| feature_state_value=validated_data.get("feature_state_value"), | ||
| type_=validated_data.get("type"), | ||
| segment_id=validated_data.get("segment_id"), | ||
| ) | ||
|
|
||
| request = self.context["request"] | ||
| if type(request.user) is FFAdminUser: | ||
| change_set.user = request.user | ||
| else: | ||
| change_set.api_key = request.user.key | ||
|
|
||
| return change_set | ||
|
|
||
| def save(self, **kwargs: typing.Any) -> FeatureState: | ||
| environment: Environment = kwargs["environment"] | ||
| feature: Feature = kwargs["feature"] | ||
|
|
||
| return update_flag(environment, feature, self.flag_change_set) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import json | ||
|
|
||
| import pytest | ||
| from common.environments.permissions import UPDATE_FEATURE_STATE | ||
| from django.urls import reverse | ||
| from pytest_lazyfixture import lazy_fixture # type: ignore[import-untyped] | ||
| from rest_framework import status | ||
| from rest_framework.test import APIClient | ||
|
|
||
| from environments.models import Environment | ||
| from features.models import Feature | ||
| from features.versioning.versioning_service import ( | ||
| get_environment_flags_list, | ||
| ) | ||
| from tests.types import WithEnvironmentPermissionsCallable | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "environment_", | ||
| (lazy_fixture("environment"), lazy_fixture("environment_v2_versioning")), | ||
| ) | ||
| def test_update_flag( | ||
| staff_client: APIClient, | ||
| feature: Feature, | ||
| environment_: Environment, | ||
| with_environment_permissions: WithEnvironmentPermissionsCallable, | ||
| ) -> None: | ||
| # Given | ||
| with_environment_permissions([UPDATE_FEATURE_STATE]) # type: ignore[call-arg] | ||
| url = reverse( | ||
| "api-v1:environments:update-flag", | ||
| kwargs={"environment_id": environment_.id, "feature_name": feature.name}, | ||
| ) | ||
|
|
||
| data = {"enabled": True, "feature_state_value": "42", "type": "int"} | ||
|
|
||
| # When | ||
| response = staff_client.post( | ||
| url, data=json.dumps(data), content_type="application/json" | ||
| ) | ||
|
|
||
| # Then | ||
| assert response.status_code == status.HTTP_200_OK | ||
|
|
||
| latest_flags = get_environment_flags_list( | ||
| environment=environment_, feature_name=feature.name | ||
| ) | ||
|
|
||
| assert latest_flags[0].enabled is True | ||
| assert latest_flags[0].get_feature_state_value() == 42 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, as discussed with @gagantrivedi, let's remove the feature name from the URL since there is nothing stopping it containing HTTP reserved characters. The endpoint should therefore be: