diff --git a/api/edge_api/identities/edge_identity_service.py b/api/edge_api/identities/edge_identity_service.py index e269747a57b6..ce5cef2d067a 100644 --- a/api/edge_api/identities/edge_identity_service.py +++ b/api/edge_api/identities/edge_identity_service.py @@ -1,3 +1,4 @@ +from edge_api.identities.models import EdgeIdentity from environments.dynamodb import DynamoEnvironmentV2Wrapper from environments.dynamodb.types import ( IdentityOverridesV2List, @@ -53,3 +54,11 @@ def get_edge_identity_overrides_for_feature_ids( ) return results + + +def get_overridden_feature_ids_for_edge_identity(identity_uuid: str) -> set[int]: + identity_document = EdgeIdentity.dynamo_wrapper.get_item_from_uuid_or_404( + identity_uuid + ) + identity = EdgeIdentity.from_identity_document(identity_document) + return {fs.feature.id for fs in identity.feature_overrides} diff --git a/api/features/serializers.py b/api/features/serializers.py index 134181afe3a0..41a97ffee7e2 100644 --- a/api/features/serializers.py +++ b/api/features/serializers.py @@ -95,6 +95,10 @@ class FeatureQuerySerializer(serializers.Serializer): # type: ignore[type-arg] required=False, help_text="Integer ID of the segment to retrieve segment overrides for.", ) + identity = serializers.CharField( + required=False, + help_text="ID of the identity to sort features with identity overrides first.", + ) is_enabled = serializers.BooleanField( allow_null=True, required=False, diff --git a/api/features/views.py b/api/features/views.py index de01c33a700d..4d8e6027ba05 100644 --- a/api/features/views.py +++ b/api/features/views.py @@ -7,7 +7,17 @@ from common.projects.permissions import VIEW_PROJECT from django.conf import settings from django.core.cache import caches -from django.db.models import Max, Q, QuerySet +from django.db.models import ( + BooleanField, + Case, + Exists, + Max, + OuterRef, + Q, + QuerySet, + Value, + When, +) from django.utils import timezone from django.utils.decorators import method_decorator from django.views.decorators.cache import cache_page @@ -35,6 +45,9 @@ from app_analytics.throttles import InfluxQueryThrottle from core.constants import FLAGSMITH_UPDATED_AT_HEADER, SDK_ENVIRONMENT_KEY_HEADER from core.request_origin import RequestOrigin +from edge_api.identities.edge_identity_service import ( + get_overridden_feature_ids_for_edge_identity, +) from environments.authentication import EnvironmentKeyAuthentication from environments.identities.models import Identity from environments.identities.serializers import ( @@ -56,7 +69,7 @@ from .constants import INTERSECTION, UNION from .features_service import get_overrides_data -from .models import Feature, FeatureState +from .models import Feature, FeatureSegment, FeatureState from .multivariate.serializers import ( FeatureMVOptionsValuesResponseSerializer, ) @@ -224,7 +237,42 @@ def get_queryset(self): # type: ignore[no-untyped-def] "-" if query_data["sort_direction"] == "DESC" else "", query_data["sort_field"], ) - queryset = queryset.order_by(sort) + override_ordering: list[str] = [] + if environment_id and (segment_id := query_data.get("segment")): + queryset = queryset.annotate( + has_segment_override=Exists( + FeatureSegment.objects.filter( + feature=OuterRef("pk"), + segment_id=segment_id, + environment_id=environment_id, + ) + ), + ) + override_ordering.append("-has_segment_override") + if identity := query_data.get("identity"): + if project.enable_dynamo_db: + # Bounded by Project.max_features_allowed + override_feature_ids = get_overridden_feature_ids_for_edge_identity( + identity + ) + queryset = queryset.annotate( + has_identity_override=Case( + When(pk__in=override_feature_ids, then=Value(True)), + default=Value(False), + output_field=BooleanField(), + ), + ) + else: + queryset = queryset.annotate( + has_identity_override=Exists( + FeatureState.objects.filter( + feature=OuterRef("pk"), + identity_id=identity, + ) + ), + ) + override_ordering.append("-has_identity_override") + queryset = queryset.order_by(*override_ordering, sort) if environment_id: page = self.paginate_queryset(queryset) diff --git a/api/tests/unit/edge_api/identities/test_edge_identity_service.py b/api/tests/unit/edge_api/identities/test_edge_identity_service.py new file mode 100644 index 000000000000..f2dc3f398f37 --- /dev/null +++ b/api/tests/unit/edge_api/identities/test_edge_identity_service.py @@ -0,0 +1,104 @@ +from unittest.mock import MagicMock + +import pytest +from pytest_mock import MockerFixture + +from edge_api.identities.edge_identity_service import ( + get_overridden_feature_ids_for_edge_identity, +) + + +@pytest.fixture() +def _mock_ddb_identity_wrapper(mocker: MockerFixture) -> MagicMock: + return mocker.patch( + "edge_api.identities.models.EdgeIdentity.dynamo_wrapper", + ) + + +def _make_identity_document( + *, + environment_api_key: str, + identity_features: list[dict], # type: ignore[type-arg] +) -> dict: # type: ignore[type-arg] + return { + "composite_key": f"{environment_api_key}_test_user", + "identity_traits": [], + "identity_features": identity_features, + "identifier": "test_user", + "created_date": "2021-09-21T10:12:42.230257+00:00", + "environment_api_key": environment_api_key, + "identity_uuid": "59efa2a7-6a45-46d6-b953-a7073a90eacf", + "django_id": None, + } + + +def test_get_overridden_feature_ids_for_edge_identity__identity_with_overrides__returns_feature_ids( + _mock_ddb_identity_wrapper: MagicMock, +) -> None: + # Given + feature_a_id = 101 + feature_b_id = 202 + identity_document = _make_identity_document( + environment_api_key="test_key", + identity_features=[ + { + "feature": { + "id": feature_a_id, + "name": "feature_a", + "type": "STANDARD", + }, + "enabled": True, + "featurestate_uuid": "a7495917-ee57-41d1-a64e-e0697dbc57fb", + "feature_state_value": None, + "feature_segment": None, + "multivariate_feature_state_values": [], + }, + { + "feature": { + "id": feature_b_id, + "name": "feature_b", + "type": "STANDARD", + }, + "enabled": False, + "featurestate_uuid": "b8506028-ff68-42e2-b75f-f1708ecd68fc", + "feature_state_value": None, + "feature_segment": None, + "multivariate_feature_state_values": [], + }, + ], + ) + _mock_ddb_identity_wrapper.get_item_from_uuid_or_404.return_value = ( + identity_document + ) + + # When + result = get_overridden_feature_ids_for_edge_identity( + "59efa2a7-6a45-46d6-b953-a7073a90eacf" + ) + + # Then + assert result == {feature_a_id, feature_b_id} + _mock_ddb_identity_wrapper.get_item_from_uuid_or_404.assert_called_once_with( + "59efa2a7-6a45-46d6-b953-a7073a90eacf" + ) + + +def test_get_overridden_feature_ids_for_edge_identity__identity_without_overrides__returns_empty_set( + _mock_ddb_identity_wrapper: MagicMock, +) -> None: + # Given + identity_document = _make_identity_document( + environment_api_key="test_key", + identity_features=[], + ) + _mock_ddb_identity_wrapper.get_item_from_uuid_or_404.return_value = ( + identity_document + ) + + # When + result = get_overridden_feature_ids_for_edge_identity( + "59efa2a7-6a45-46d6-b953-a7073a90eacf" + ) + + # Then + assert result == set() diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index 05abbecf7562..983f4b94a74a 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/api/tests/unit/features/test_unit_features_views.py @@ -4150,6 +4150,115 @@ def test_list_features_segment_query_param_with_invalid_segment( assert feature_data["segment_feature_state"] is None +@pytest.mark.parametrize("sort_field", ["name", "created_date"]) +def test_list_features__segment_query__sorts_by_field_with_overrides_first( + admin_client_new: APIClient, + project: Project, + environment: Environment, + segment: Segment, + sort_field: str, +) -> None: + # Given + Feature.objects.create(project=project, name="feature_a") + feature_b = Feature.objects.create(project=project, name="feature_b") + feature_c = Feature.objects.create(project=project, name="feature_c") + other_environment = Environment.objects.create( + project=project, name="other_environment" + ) + + # feature_c has a segment override in the requested environment + FeatureSegment.objects.create( + feature=feature_c, segment=segment, environment=environment + ) + # feature_b has a segment override in a different environment + FeatureSegment.objects.create( + feature=feature_b, segment=segment, environment=other_environment + ) + + # When + response = admin_client_new.get( + f"/api/v1/projects/{project.id}/features/" + f"?environment={environment.id}" + f"&segment={segment.id}" + f"&sort_field={sort_field}&sort_direction=ASC" + ) + + # Then + assert response.status_code == status.HTTP_200_OK + assert ["feature_c", "feature_a", "feature_b"] == [ + f["name"] for f in response.json()["results"] + ] + + +@pytest.mark.parametrize("sort_field", ["name", "created_date"]) +def test_list_features__identity_query__sorts_by_field_with_overrides_first( + admin_client_new: APIClient, + project: Project, + environment: Environment, + identity: Identity, + sort_field: str, +) -> None: + # Given + Feature.objects.create(project=project, name="feature_a") + Feature.objects.create(project=project, name="feature_b") + feature_c = Feature.objects.create(project=project, name="feature_c") + + # feature_c has an identity override + FeatureState.objects.create( + feature=feature_c, environment=environment, identity=identity + ) + + # When + response = admin_client_new.get( + f"/api/v1/projects/{project.id}/features/" + f"?environment={environment.id}" + f"&identity={identity.id}" + f"&sort_field={sort_field}&sort_direction=ASC" + ) + + # Then + assert response.status_code == status.HTTP_200_OK + assert ["feature_c", "feature_a", "feature_b"] == [ + f["name"] for f in response.json()["results"] + ] + + +@pytest.mark.parametrize("sort_field", ["name", "created_date"]) +def test_list_features__edge_identity_query__sorts_by_field_with_overrides_first( + admin_client_new: APIClient, + project: Project, + environment: Environment, + sort_field: str, + mocker: MockerFixture, +) -> None: + # Given + project.enable_dynamo_db = True + project.save() + + Feature.objects.create(project=project, name="feature_a") + feature_b = Feature.objects.create(project=project, name="feature_b") + Feature.objects.create(project=project, name="feature_c") + mocker.patch.object( + views, + "get_overridden_feature_ids_for_edge_identity", + return_value={feature_b.id}, + ) + + # When + response = admin_client_new.get( + f"/api/v1/projects/{project.id}/features/" + f"?environment={environment.id}" + f"&identity=59efa2a7-6a45-46d6-b953-a7073a90eacf" + f"&sort_field={sort_field}&sort_direction=ASC" + ) + + # Then + assert response.status_code == status.HTTP_200_OK + assert ["feature_b", "feature_a", "feature_c"] == [ + f["name"] for f in response.json()["results"] + ] + + def test_create_multiple_features_with_metadata_keeps_metadata_isolated( admin_client_new: APIClient, project: Project, diff --git a/frontend/e2e/tests/segment-test.ts b/frontend/e2e/tests/segment-test.ts index 916cb257af27..15e2916afc79 100644 --- a/frontend/e2e/tests/segment-test.ts +++ b/frontend/e2e/tests/segment-test.ts @@ -258,7 +258,8 @@ export const testSegment3 = async () => { await click(byId('user-feature-switch-1-on')) await click('#confirm-toggle-feature-btn') await waitAndRefresh() // wait and refresh to avoid issues with data sync from UK -> US in github workflows - await waitForElementVisible(byId('user-feature-switch-1-off')) + // After toggling, 'flag' has an identity override and sorts first (index 0). + await waitForElementVisible(byId('user-feature-switch-0-off')) log('Edit flag for user') await click(byId('user-feature-0')) @@ -268,8 +269,8 @@ export const testSegment3 = async () => { await assertTextContent(byId('user-feature-value-0'), '"small"') log('Toggle flag for user again') - await click(byId('user-feature-switch-1-off')) + await click(byId('user-feature-switch-0-off')) await click('#confirm-toggle-feature-btn') await waitAndRefresh() // wait and refresh to avoid issues with data sync from UK -> US in github workflows - await waitForElementVisible(byId('user-feature-switch-1-on')) + await waitForElementVisible(byId('user-feature-switch-0-on')) } diff --git a/frontend/web/components/pages/UserPage.tsx b/frontend/web/components/pages/UserPage.tsx index cbb1a27315cb..7775d68e747a 100644 --- a/frontend/web/components/pages/UserPage.tsx +++ b/frontend/web/components/pages/UserPage.tsx @@ -88,9 +88,9 @@ const UserPage: FC = () => { true, search, sort, - getServerFilter(filter), + { ...getServerFilter(filter), identity: id }, ) - }, [filter, environmentId, projectId]) + }, [filter, environmentId, projectId, id]) useEffect(() => { AppActions.getIdentity(environmentId, id) @@ -141,10 +141,10 @@ const UserPage: FC = () => { search, sort, pageNumber, - getServerFilter(filter), + { ...getServerFilter(filter), identity: id }, ) }, - [environmentId, projectId, filter], + [environmentId, projectId, filter, id], ) return (