Skip to content
Open
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
1 change: 1 addition & 0 deletions api/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ features/workflows/logic/

# Unit test coverage
.coverage
.coverage.*
4 changes: 3 additions & 1 deletion api/app_analytics/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,9 @@ def get_fields(self): # type: ignore[no-untyped-def]

def save(self, **kwargs: typing.Any) -> None:
request = self.context["request"]
for feature_name, evaluation_count in self.validated_data.items():
# validated_data splits out request body with '.' in feature name (e.g a.b.c).
# Instead, it's safe to use self.data as keys are not altered.
Comment on lines +91 to +92
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment doesn't make sense to me. Surely we want the "." in the feature name. Can we be more explicit here about what the difference is between validated_data and data in this scenario?

Also, is there a neater way to solve this in the serializer rather than the view? I wonder if skipping validated_data here might lead to other problems down the line.

for feature_name, evaluation_count in self.data.items():
feature_evaluation_cache.track_feature_evaluation(
environment_id=request.environment.id,
feature_name=feature_name,
Expand Down
39 changes: 39 additions & 0 deletions api/tests/unit/app_analytics/test_unit_app_analytics_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,15 @@
Organisation,
OrganisationSubscriptionInformationCache,
)
from projects.models import Project
from tests.types import EnableFeaturesFixture


@pytest.fixture()
def feature_with_dots(project: Project) -> Feature:
return Feature.objects.create(name="feature.with.dots", project=project) # type: ignore[no-any-return]
Comment on lines +30 to +32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be a fixture since it's only used in a single test. I'd rather that we just instantiate the feature directly in the test.



def test_sdk_analytics_ignores_bad_data(
mocker: MockerFixture,
environment: Environment,
Expand Down Expand Up @@ -59,6 +65,39 @@ def test_sdk_analytics_ignores_bad_data(
)


def test_sdk_analytics_ignores_feature_data_with_dots(
mocker: MockerFixture,
environment: Environment,
feature_with_dots: Feature,
api_client: APIClient,
) -> None:
# Given
api_client.credentials(HTTP_X_ENVIRONMENT_KEY=environment.api_key)

data = {feature_with_dots.name: 20}
mocked_feature_eval_cache = mocker.patch(
"app_analytics.views.feature_evaluation_cache"
)

url = reverse("api-v1:analytics-flags")

# When
response = api_client.post(
url, data=json.dumps(data), content_type="application/json"
)

# Then
assert response.status_code == status.HTTP_200_OK
assert mocked_feature_eval_cache.track_feature_evaluation.call_count == 1

mocked_feature_eval_cache.track_feature_evaluation.assert_called_once_with(
environment_id=environment.id,
feature_name=feature_with_dots.name,
evaluation_count=data[feature_with_dots.name],
labels={},
)


def test_get_usage_data(mocker, admin_client, organisation): # type: ignore[no-untyped-def]
# Given
url = reverse("api-v1:organisations:usage-data", args=[organisation.id])
Expand Down