Skip to content
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

fix: Fine tune feature import export #3163

Merged
merged 7 commits into from
Dec 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/features/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,7 @@
# Feature state statuses
COMMITTED = "COMMITTED"
DRAFT = "DRAFT"

# Tag filtering strategy
UNION = "UNION"
INTERSECTION = "INTERSECTION"
9 changes: 7 additions & 2 deletions api/features/import_export/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,13 @@ def has_permission(
if not super().has_permission(request, view):
return False

environment = Environment.objects.get(id=view.kwargs["environment_id"])
return request.user.is_environment_admin(environment)
environment = Environment.objects.select_related(
"project__organisation",
).get(id=view.kwargs["environment_id"])
organisation = environment.project.organisation

# Since feature imports can be destructive, use org admin.
return request.user.is_organisation_admin(organisation)


class CreateFeatureExportPermissions(IsAuthenticated):
Expand Down
3 changes: 1 addition & 2 deletions api/features/import_export/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ def get_queryset(self) -> QuerySet[FeatureExport]:
if user.is_environment_admin(environment):
environment_ids.append(environment.id)

# Order by environment name to match environment list order.
return FeatureExport.objects.filter(environment__in=environment_ids).order_by(
"environment__name"
"-created_at"
)
11 changes: 10 additions & 1 deletion api/features/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
DeleteBeforeUpdateWritableNestedModelSerializer,
)

from .constants import INTERSECTION, UNION
from .feature_segments.serializers import (
CreateSegmentOverrideFeatureSegmentSerializer,
)
Expand Down Expand Up @@ -81,8 +82,16 @@ class FeatureQuerySerializer(serializers.Serializer):
sort_direction = serializers.ChoiceField(choices=("ASC", "DESC"), default="ASC")

tags = serializers.CharField(
required=False, help_text="Comma separated list of tag ids to filter on (AND)"
required=False,
help_text=(
"Comma separated list of tag ids to filter on (AND with "
"INTERSECTION, and OR with UNION via tag_strategy)"
),
)
tag_strategy = serializers.ChoiceField(
choices=(UNION, INTERSECTION), default=INTERSECTION
)

is_archived = serializers.BooleanField(required=False)
environment = serializers.IntegerField(
required=False,
Expand Down
4 changes: 4 additions & 0 deletions api/features/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
from users.models import FFAdminUser, UserPermissionGroup
from webhooks.webhooks import WebhookEventType

from .constants import INTERSECTION, UNION
from .features_service import get_overrides_data
from .models import Feature, FeatureState
from .permissions import (
Expand Down Expand Up @@ -290,7 +291,10 @@ def _filter_queryset(self, queryset: QuerySet) -> QuerySet:
if "tags" in query_serializer.initial_data:
if query_data.get("tags", "") == "":
queryset = queryset.filter(tags__isnull=True)
elif query_data["tag_strategy"] == UNION:
queryset = queryset.filter(tags__in=query_data["tags"])
else:
assert query_data["tag_strategy"] == INTERSECTION
queryset = reduce(
lambda qs, tag_id: qs.filter(tags=tag_id),
query_data["tags"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,13 @@ def test_list_feature_exports(

# Then
assert response.status_code == 200
assert response.data["results"][0]["environment_id"] == environment.id
assert response.data["results"][0]["id"] == feature_export1.id
assert response.data["results"][0]["name"].startswith(f"{environment.name} | ")
assert response.data["results"][0]["environment_id"] == environment2.id
assert response.data["results"][0]["id"] == feature_export2.id
assert response.data["results"][0]["name"].startswith(f"{environment2.name} | ")

assert response.data["results"][1]["environment_id"] == environment2.id
assert response.data["results"][1]["id"] == feature_export2.id
assert response.data["results"][1]["name"].startswith(f"{environment2.name} | ")
assert response.data["results"][1]["environment_id"] == environment.id
assert response.data["results"][1]["id"] == feature_export1.id
assert response.data["results"][1]["name"].startswith(f"{environment.name} | ")


def test_list_feature_export_with_filtered_environments(
Expand Down
102 changes: 102 additions & 0 deletions api/tests/unit/features/test_unit_features_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -2404,3 +2404,105 @@ def test_list_features_n_plus_1(

# Then
assert response.status_code == status.HTTP_200_OK


def test_list_features_with_union_tag(
staff_client: APIClient,
project: Project,
feature: Feature,
with_project_permissions: Callable,
django_assert_num_queries: DjangoAssertNumQueries,
environment: Environment,
) -> None:
# Given
with_project_permissions([VIEW_PROJECT])

tag1 = Tag.objects.create(
label="Test Tag",
color="#fffff",
description="Test Tag description",
project=project,
)
tag2 = Tag.objects.create(
label="Test Tag2",
color="#fffff",
description="Test Tag2 description",
project=project,
)
feature2 = Feature.objects.create(
name="another_feature", project=project, initial_value="initial_value"
)
feature3 = Feature.objects.create(
name="missing_feature", project=project, initial_value="gone"
)
feature2.tags.add(tag1)
feature3.tags.add(tag2)

base_url = reverse("api-v1:projects:project-features-list", args=[project.id])
url = (
f"{base_url}?environment={environment.id}&tags={tag1.id}"
f",{tag2.id}&tag_strategy=UNION"
)
# When
response = staff_client.get(url)

# Then
assert response.status_code == status.HTTP_200_OK

# The first feature has been filtered out of the results.
assert len(response.data["results"]) == 2

assert response.data["results"][0]["id"] == feature2.id
assert response.data["results"][0]["tags"] == [tag1.id]
assert response.data["results"][1]["id"] == feature3.id
assert response.data["results"][1]["tags"] == [tag2.id]


def test_list_features_with_intersection_tag(
staff_client: APIClient,
project: Project,
feature: Feature,
with_project_permissions: Callable,
django_assert_num_queries: DjangoAssertNumQueries,
environment: Environment,
) -> None:
# Given
with_project_permissions([VIEW_PROJECT])

tag1 = Tag.objects.create(
label="Test Tag",
color="#fffff",
description="Test Tag description",
project=project,
)
tag2 = Tag.objects.create(
label="Test Tag2",
color="#fffff",
description="Test Tag2 description",
project=project,
)
feature2 = Feature.objects.create(
name="another_feature", project=project, initial_value="initial_value"
)
feature3 = Feature.objects.create(
name="missing_feature", project=project, initial_value="gone"
)
feature2.tags.add(tag1, tag2)
feature3.tags.add(tag2)

base_url = reverse("api-v1:projects:project-features-list", args=[project.id])
url = (
f"{base_url}?environment={environment.id}&tags={tag1.id}"
f",{tag2.id}&tag_strategy=INTERSECTION"
)
# When
response = staff_client.get(url)

# Then
assert response.status_code == status.HTTP_200_OK

# Only feature2 has both tags, so it is the only result.
assert len(response.data["results"]) == 1

assert response.data["results"][0]["id"] == feature2.id
assert response.data["results"][0]["tags"] == [tag1.id, tag2.id]