From 875682812c61e81a2bd2d5180f52a915ed0d9842 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Fri, 30 Aug 2024 10:07:25 +0530 Subject: [PATCH 1/3] fix(github-4555): use api_key name for changed_by --- api/edge_api/identities/serializers.py | 2 +- api/edge_api/identities/tasks.py | 11 +++-- ...est_edge_identity_featurestates_viewset.py | 21 +++++---- .../test_unit_edge_api_identities_tasks.py | 46 +++++++++++++++++++ 4 files changed, 66 insertions(+), 14 deletions(-) diff --git a/api/edge_api/identities/serializers.py b/api/edge_api/identities/serializers.py index 5d0c0907d6e5..3ed79ab10c24 100644 --- a/api/edge_api/identities/serializers.py +++ b/api/edge_api/identities/serializers.py @@ -189,7 +189,7 @@ def save(self, **kwargs): "environment_api_key": identity.environment_api_key, "identity_id": identity.id, "identity_identifier": identity.identifier, - "changed_by_user_id": request.user.id, + "changed_by_user_id": request.user.pk, "new_enabled_state": self.instance.enabled, "new_value": new_value, "previous_enabled_state": getattr(previous_state, "enabled", None), diff --git a/api/edge_api/identities/tasks.py b/api/edge_api/identities/tasks.py index e97e7208f8a0..2dbe37b8a128 100644 --- a/api/edge_api/identities/tasks.py +++ b/api/edge_api/identities/tasks.py @@ -5,6 +5,7 @@ from task_processor.decorators import register_task_handler from task_processor.models import TaskPriority +from api_keys.models import MasterAPIKey from audit.models import AuditLog from audit.related_object_type import RelatedObjectType from edge_api.identities.types import IdentityChangeset @@ -24,7 +25,7 @@ def call_environment_webhook_for_feature_state_change( environment_api_key: str, identity_id: typing.Union[id, str], identity_identifier: str, - changed_by_user_id: int, + changed_by_user_id: int | str, timestamp: str, new_enabled_state: bool = None, new_value: typing.Union[bool, int, str] = None, @@ -40,10 +41,14 @@ def call_environment_webhook_for_feature_state_change( return feature = Feature.objects.get(id=feature_id) - changed_by = FFAdminUser.objects.get(id=changed_by_user_id) + match changed_by_user_id: + case str(): + changed_by = MasterAPIKey.objects.get(id=changed_by_user_id).name + case _: + changed_by = FFAdminUser.objects.get(id=changed_by_user_id).email data = { - "changed_by": changed_by.email, + "changed_by": changed_by, "timestamp": timestamp, "new_state": None, } diff --git a/api/tests/integration/edge_api/identities/test_edge_identity_featurestates_viewset.py b/api/tests/integration/edge_api/identities/test_edge_identity_featurestates_viewset.py index 19cdf8b07dab..101d7e22ded8 100644 --- a/api/tests/integration/edge_api/identities/test_edge_identity_featurestates_viewset.py +++ b/api/tests/integration/edge_api/identities/test_edge_identity_featurestates_viewset.py @@ -1,6 +1,7 @@ import json import typing import uuid +from unittest.mock import MagicMock import pytest from core.constants import BOOLEAN, INTEGER, STRING @@ -351,15 +352,15 @@ def test_edge_identities_create_featurestate_returns_400_if_feature_state_alread def test_edge_identities_create_featurestate( - dynamodb_wrapper_v2, - admin_client, - environment, - environment_api_key, - identity_document_without_fs, - edge_identity_dynamo_wrapper_mock, - feature, - feature_name, - webhook_mock, + dynamodb_wrapper_v2: DynamoEnvironmentV2Wrapper, + admin_client_new: APIClient, + environment: int, + environment_api_key: str, + identity_document_without_fs: dict, + edge_identity_dynamo_wrapper_mock: MagicMock, + feature: int, + feature_name: str, + webhook_mock: MagicMock, ): # Given edge_identity_dynamo_wrapper_mock.get_item_from_uuid_or_404.return_value = ( @@ -381,7 +382,7 @@ def test_edge_identities_create_featurestate( } # When - response = admin_client.post( + response = admin_client_new.post( url, data=json.dumps(data), content_type="application/json" ) diff --git a/api/tests/unit/edge_api/identities/test_unit_edge_api_identities_tasks.py b/api/tests/unit/edge_api/identities/test_unit_edge_api_identities_tasks.py index d5b6d29cea51..0c96ab715809 100644 --- a/api/tests/unit/edge_api/identities/test_unit_edge_api_identities_tasks.py +++ b/api/tests/unit/edge_api/identities/test_unit_edge_api_identities_tasks.py @@ -4,6 +4,7 @@ from django.utils import timezone from pytest_mock import MockerFixture +from api_keys.models import MasterAPIKey from audit.models import AuditLog from audit.related_object_type import RelatedObjectType from edge_api.identities.tasks import ( @@ -16,7 +17,9 @@ IdentityOverridesV2Changeset, IdentityOverrideV2, ) +from environments.identities.models import Identity from environments.models import Environment, Webhook +from features.models import Feature from webhooks.webhooks import WebhookEventType @@ -128,6 +131,49 @@ def test_call_environment_webhook_for_feature_state_change_with_previous_state_o assert data["timestamp"] == now_isoformat +def test_call_environment_webhook_for_feature_state_change_with_master_api_key_id( + mocker: MockerFixture, + environment: Environment, + feature: Feature, + identity: Identity, + master_api_key_object: MasterAPIKey, +) -> None: + # Given + mock_call_environment_webhooks = mocker.patch( + "edge_api.identities.tasks.call_environment_webhooks" + ) + Webhook.objects.create(environment=environment, url="https://foo.com/webhook") + + mock_feature_state_data = mocker.MagicMock() + mocker.patch.object( + Webhook, + "generate_webhook_feature_state_data", + return_value=mock_feature_state_data, + ) + + now_isoformat = timezone.now().isoformat() + previous_enabled_state = True + previous_value = "foo" + + # When + call_environment_webhook_for_feature_state_change( + feature_id=feature.id, + environment_api_key=environment.api_key, + identity_id=identity.id, + identity_identifier=identity.identifier, + changed_by_user_id=master_api_key_object.id, + timestamp=now_isoformat, + previous_enabled_state=previous_enabled_state, + previous_value=previous_value, + ) + + # Then + call_args = mock_call_environment_webhooks.call_args + + data = call_args[0][1] + assert data["changed_by"] == master_api_key_object.name + + @pytest.mark.parametrize( "previous_enabled_state, previous_value, new_enabled_state, new_value", ( From 3b5561c6b72abac003fa984146ff82ec994d465f Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Tue, 3 Sep 2024 14:28:08 +0530 Subject: [PATCH 2/3] misc --- api/api_keys/user.py | 3 ++ api/conftest.py | 16 ++++++ api/edge_api/identities/models.py | 36 ++++++------- api/edge_api/identities/serializers.py | 7 +-- api/edge_api/identities/tasks.py | 9 +--- api/edge_api/identities/views.py | 20 ++----- api/tests/unit/api_keys/test_user.py | 11 ++++ .../test_edge_api_identities_serializers.py | 23 ++++++-- .../identities/test_edge_identity_models.py | 41 ++++++++++---- .../test_unit_edge_api_identities_tasks.py | 54 ++----------------- 10 files changed, 106 insertions(+), 114 deletions(-) diff --git a/api/api_keys/user.py b/api/api_keys/user.py index 6af597360d17..102c4ebc0836 100644 --- a/api/api_keys/user.py +++ b/api/api_keys/user.py @@ -23,6 +23,9 @@ class APIKeyUser(UserABC): def __init__(self, key: MasterAPIKey): self.key = key + def __str__(self) -> str: + return self.key.name + @property def is_authenticated(self) -> bool: return True diff --git a/api/conftest.py b/api/conftest.py index 4476ea624855..9aaa0e840a97 100644 --- a/api/conftest.py +++ b/api/conftest.py @@ -21,6 +21,7 @@ from xdist import get_xdist_worker_id from api_keys.models import MasterAPIKey +from api_keys.user import APIKeyUser from environments.identities.models import Identity from environments.identities.traits.models import Trait from environments.models import Environment, EnvironmentAPIKey @@ -658,6 +659,16 @@ def master_api_key_object( return master_api_key[0] +@pytest.fixture +def master_api_key_id(master_api_key_object: MasterAPIKey) -> str: + return master_api_key_object.id + + +@pytest.fixture +def admin_user_id(admin_user: FFAdminUser) -> str: + return admin_user.id + + @pytest.fixture def admin_master_api_key_object( admin_master_api_key: typing.Tuple[MasterAPIKey, str] @@ -665,6 +676,11 @@ def admin_master_api_key_object( return admin_master_api_key[0] +@pytest.fixture +def api_key_user(master_api_key_object: MasterAPIKey) -> APIKeyUser: + return APIKeyUser(master_api_key_object) + + @pytest.fixture() def admin_master_api_key_client( admin_master_api_key: typing.Tuple[MasterAPIKey, str] diff --git a/api/edge_api/identities/models.py b/api/edge_api/identities/models.py index 1e062ecd005e..aad70198f88d 100644 --- a/api/edge_api/identities/models.py +++ b/api/edge_api/identities/models.py @@ -6,7 +6,7 @@ from flag_engine.features.models import FeatureStateModel from flag_engine.identities.models import IdentityFeaturesList, IdentityModel -from api_keys.models import MasterAPIKey +from api_keys.user import APIKeyUser from edge_api.identities.tasks import ( generate_audit_log_records, sync_identity_document_features, @@ -165,26 +165,22 @@ def remove_feature_override(self, feature_state: FeatureStateModel) -> None: with suppress(ValueError): # ignore if feature state didn't exist self._engine_identity_model.identity_features.remove(feature_state) - def save(self, user: FFAdminUser = None, master_api_key: MasterAPIKey = None): + def save(self, user: FFAdminUser | APIKeyUser = None): self.dynamo_wrapper.put_item(self.to_document()) changeset = self._get_changes() self._update_feature_overrides( changeset=changeset, user=user, - master_api_key=master_api_key, ) self._reset_initial_state() - def delete( - self, user: FFAdminUser = None, master_api_key: MasterAPIKey = None - ) -> None: + def delete(self, user: FFAdminUser | APIKeyUser = None) -> None: self.dynamo_wrapper.delete_item(self._engine_identity_model.composite_key) self._engine_identity_model.identity_features.clear() changeset = self._get_changes() self._update_feature_overrides( changeset=changeset, user=user, - master_api_key=master_api_key, ) self._reset_initial_state() @@ -200,23 +196,21 @@ def to_document(self) -> dict: return map_engine_identity_to_identity_document(self._engine_identity_model) def _update_feature_overrides( - self, - changeset: IdentityChangeset, - user: FFAdminUser = None, - master_api_key: MasterAPIKey = None, + self, changeset: IdentityChangeset, user: FFAdminUser | APIKeyUser ) -> None: if changeset["feature_overrides"]: # TODO: would this be simpler if we put a wrapper around FeatureStateModel instead? - generate_audit_log_records.delay( - kwargs={ - "environment_api_key": self.environment_api_key, - "identifier": self.identifier, - "user_id": getattr(user, "id", None), - "changes": changeset, - "identity_uuid": str(self.identity_uuid), - "master_api_key_id": getattr(master_api_key, "id", None), - } - ) + kwargs = { + "environment_api_key": self.environment_api_key, + "identifier": self.identifier, + "user_id": getattr(user, "id", None), + "changes": changeset, + "identity_uuid": str(self.identity_uuid), + "master_api_key_id": ( + user.pk if getattr(user, "is_master_api_key_user", False) else None + ), + } + generate_audit_log_records.delay(kwargs=kwargs) update_flagsmith_environments_v2_identity_overrides.delay( kwargs={ "environment_api_key": self.environment_api_key, diff --git a/api/edge_api/identities/serializers.py b/api/edge_api/identities/serializers.py index 3ed79ab10c24..00e937520b48 100644 --- a/api/edge_api/identities/serializers.py +++ b/api/edge_api/identities/serializers.py @@ -171,10 +171,7 @@ def save(self, **kwargs): self.instance.multivariate_feature_state_values, ) - identity.save( - user=request.user, - master_api_key=getattr(request, "master_api_key", None), - ) + identity.save(user=request.user) new_value = self.instance.get_value(identity.id) previous_value = ( @@ -189,7 +186,7 @@ def save(self, **kwargs): "environment_api_key": identity.environment_api_key, "identity_id": identity.id, "identity_identifier": identity.identifier, - "changed_by_user_id": request.user.pk, + "changed_by": str(request.user), "new_enabled_state": self.instance.enabled, "new_value": new_value, "previous_enabled_state": getattr(previous_state, "enabled", None), diff --git a/api/edge_api/identities/tasks.py b/api/edge_api/identities/tasks.py index 2dbe37b8a128..9bd8cd510adf 100644 --- a/api/edge_api/identities/tasks.py +++ b/api/edge_api/identities/tasks.py @@ -5,14 +5,12 @@ from task_processor.decorators import register_task_handler from task_processor.models import TaskPriority -from api_keys.models import MasterAPIKey from audit.models import AuditLog from audit.related_object_type import RelatedObjectType from edge_api.identities.types import IdentityChangeset from environments.dynamodb import DynamoEnvironmentV2Wrapper from environments.models import Environment, Webhook from features.models import Feature, FeatureState -from users.models import FFAdminUser from util.mappers import map_identity_changeset_to_identity_override_changeset from webhooks.webhooks import WebhookEventType, call_environment_webhooks @@ -25,7 +23,7 @@ def call_environment_webhook_for_feature_state_change( environment_api_key: str, identity_id: typing.Union[id, str], identity_identifier: str, - changed_by_user_id: int | str, + changed_by: str, timestamp: str, new_enabled_state: bool = None, new_value: typing.Union[bool, int, str] = None, @@ -41,11 +39,6 @@ def call_environment_webhook_for_feature_state_change( return feature = Feature.objects.get(id=feature_id) - match changed_by_user_id: - case str(): - changed_by = MasterAPIKey.objects.get(id=changed_by_user_id).name - case _: - changed_by = FFAdminUser.objects.get(id=changed_by_user_id).email data = { "changed_by": changed_by, diff --git a/api/edge_api/identities/views.py b/api/edge_api/identities/views.py index 9065324457c5..76d2eef94b6f 100644 --- a/api/edge_api/identities/views.py +++ b/api/edge_api/identities/views.py @@ -161,10 +161,7 @@ def get_environment_from_request(self): def perform_destroy(self, instance): edge_identity = EdgeIdentity.from_identity_document(instance) - edge_identity.delete( - user=self.request.user, - master_api_key=getattr(self.request, "master_api_key", None), - ) + edge_identity.delete(user=self.request.user) @swagger_auto_schema( responses={200: EdgeIdentityTraitsSerializer(many=True)}, @@ -284,10 +281,7 @@ def list(self, request, *args, **kwargs): def perform_destroy(self, instance): self.identity.remove_feature_override(instance) - self.identity.save( - user=self.request.user, - master_api_key=getattr(self.request, "master_api_key", None), - ) + self.identity.save(user=self.request.user) @swagger_auto_schema(responses={200: IdentityAllFeatureStatesSerializer(many=True)}) @action(detail=False, methods=["GET"]) @@ -332,10 +326,7 @@ def clone_from_given_identity(self, request, *args, **kwargs) -> Response: ) self.identity.clone_flag_states_from(source_identity) - self.identity.save( - user=request.user.id, - master_api_key=getattr(request, "master_api_key", None), - ) + self.identity.save(user=request.user) return self.all(request, *args, **kwargs) @@ -391,10 +382,7 @@ def delete(self, request, *args, **kwargs): feature_state = self.identity.get_feature_state_by_feature_name_or_id(feature) if feature_state: self.identity.remove_feature_override(feature_state) - self.identity.save( - user=request.user.id, - master_api_key=getattr(request, "master_api_key", None), - ) + self.identity.save(user=request.user) return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/api/tests/unit/api_keys/test_user.py b/api/tests/unit/api_keys/test_user.py index d1cff9b6d646..8a8242fc9c08 100644 --- a/api/tests/unit/api_keys/test_user.py +++ b/api/tests/unit/api_keys/test_user.py @@ -17,6 +17,17 @@ def test_is_authenticated(master_api_key_object): assert user.is_authenticated is True +def test__str__returns_name(master_api_key_object): + # Given + user = APIKeyUser(master_api_key_object) + + # When + result = str(user) + + # Then + assert result == master_api_key_object.name + + @pytest.mark.parametrize( "for_organisation, expected_result", [ diff --git a/api/tests/unit/edge_api/identities/test_edge_api_identities_serializers.py b/api/tests/unit/edge_api/identities/test_edge_api_identities_serializers.py index c3cdfc79f5b4..cd704ce628ee 100644 --- a/api/tests/unit/edge_api/identities/test_edge_api_identities_serializers.py +++ b/api/tests/unit/edge_api/identities/test_edge_api_identities_serializers.py @@ -1,5 +1,8 @@ +import pytest +from django.test import RequestFactory from django.utils import timezone from flag_engine.features.models import FeatureModel, FeatureStateModel +from pytest_lazyfixture import lazy_fixture from edge_api.identities.models import EdgeIdentity from edge_api.identities.serializers import EdgeIdentityFeatureStateSerializer @@ -51,15 +54,27 @@ def test_edge_identity_feature_state_serializer_save_allows_missing_mvfsvs( assert saved_identity_feature_state["feature"]["id"] == feature.id +@pytest.mark.parametrize( + "user", + [ + lazy_fixture("api_key_user"), + lazy_fixture("admin_user"), + ], +) def test_edge_identity_feature_state_serializer_save_calls_webhook_for_new_override( - mocker, identity, feature, admin_user + mocker, + identity, + feature, + user, + rf: RequestFactory, ): # Given identity_model = EdgeIdentity.from_identity_document( map_identity_to_identity_document(identity) ) view = mocker.MagicMock(identity=identity_model) - request = mocker.MagicMock(user=admin_user, master_api_key=None) + request = rf.post("/") + request.user = user new_enabled_state = True new_value = "foo" @@ -93,7 +108,7 @@ def test_edge_identity_feature_state_serializer_save_calls_webhook_for_new_overr "environment_api_key": identity.environment.api_key, "identity_id": identity.id, "identity_identifier": identity.identifier, - "changed_by_user_id": admin_user.id, + "changed_by": str(user), "new_enabled_state": new_enabled_state, "new_value": new_value, "previous_enabled_state": None, @@ -154,7 +169,7 @@ def test_edge_identity_feature_state_serializer_save_calls_webhook_for_update( "environment_api_key": identity.environment.api_key, "identity_id": identity.id, "identity_identifier": identity.identifier, - "changed_by_user_id": admin_user.id, + "changed_by": str(admin_user), "new_enabled_state": new_enabled_state, "new_value": new_value, "previous_enabled_state": previous_enabled_state, diff --git a/api/tests/unit/edge_api/identities/test_edge_identity_models.py b/api/tests/unit/edge_api/identities/test_edge_identity_models.py index 92b1f89320bf..2dbe6c216db1 100644 --- a/api/tests/unit/edge_api/identities/test_edge_identity_models.py +++ b/api/tests/unit/edge_api/identities/test_edge_identity_models.py @@ -7,14 +7,17 @@ from flag_engine.features.models import FeatureModel, FeatureStateModel from freezegun import freeze_time from pytest_django import DjangoAssertNumQueries +from pytest_lazyfixture import lazy_fixture from pytest_mock import MockerFixture +from api_keys.user import APIKeyUser from edge_api.identities.models import EdgeIdentity from environments.models import Environment from features.models import Feature, FeatureSegment, FeatureState from features.versioning.tasks import enable_v2_versioning from features.workflows.core.models import ChangeRequest from segments.models import Segment +from users.models import FFAdminUser def test_get_all_feature_states_for_edge_identity_uses_segment_priorities( @@ -261,10 +264,20 @@ def test_edge_identity_save_does_not_generate_audit_records_if_no_changes( mocked_generate_audit_log_records.delay.assert_not_called() +@pytest.mark.parametrize( + "user, user_id, api_key_id", + [ + (lazy_fixture("api_key_user"), None, lazy_fixture("master_api_key_id")), + (lazy_fixture("admin_user"), lazy_fixture("admin_user_id"), None), + ], +) def test_edge_identity_save_called__feature_override_added__expected_tasks_called( mocker: MockerFixture, edge_identity_model: EdgeIdentity, edge_identity_dynamo_wrapper_mock: MagicMock, + user: FFAdminUser | APIKeyUser, + user_id: int | None, + api_key_id: int | None, ) -> None: # Given mocked_generate_audit_log_records = mocker.patch( @@ -280,8 +293,6 @@ def test_edge_identity_save_called__feature_override_added__expected_tasks_calle ) edge_identity_model.add_feature_override(feature_state_model) - user = mocker.MagicMock() - expected_changes = { "feature_overrides": { "test_feature": { @@ -301,14 +312,15 @@ def test_edge_identity_save_called__feature_override_added__expected_tasks_calle # Then edge_identity_dynamo_wrapper_mock.put_item.assert_called_once() + mocked_generate_audit_log_records.delay.assert_called_once_with( kwargs={ "environment_api_key": edge_identity_model.environment_api_key, "identifier": edge_identity_model.identifier, - "user_id": user.id, + "user_id": user_id, "changes": expected_changes, "identity_uuid": expected_identity_uuid, - "master_api_key_id": None, + "master_api_key_id": api_key_id, } ) mocked_update_flagsmith_environments_v2_identity_overrides.delay.assert_called_once_with( @@ -321,10 +333,20 @@ def test_edge_identity_save_called__feature_override_added__expected_tasks_calle ) +@pytest.mark.parametrize( + "user, user_id, api_key_id", + [ + (lazy_fixture("api_key_user"), None, lazy_fixture("master_api_key_id")), + (lazy_fixture("admin_user"), lazy_fixture("admin_user_id"), None), + ], +) def test_edge_identity_save_called__feature_override_removed__expected_tasks_called( mocker: MockerFixture, edge_identity_model: EdgeIdentity, edge_identity_dynamo_wrapper_mock: MagicMock, + user: FFAdminUser, + user_id: int | None, + api_key_id: int | None, ) -> None: # Given mocked_generate_audit_log_records = mocker.patch( @@ -340,8 +362,6 @@ def test_edge_identity_save_called__feature_override_removed__expected_tasks_cal ) edge_identity_model.add_feature_override(feature_state_model) - user = mocker.MagicMock() - expected_changes = { "feature_overrides": { "test_feature": { @@ -372,10 +392,10 @@ def test_edge_identity_save_called__feature_override_removed__expected_tasks_cal kwargs={ "environment_api_key": edge_identity_model.environment_api_key, "identifier": edge_identity_model.identifier, - "user_id": user.id, + "user_id": user_id, "changes": expected_changes, "identity_uuid": expected_identity_uuid, - "master_api_key_id": None, + "master_api_key_id": api_key_id, } ) mocked_update_flagsmith_environments_v2_identity_overrides.delay.assert_called_once_with( @@ -404,6 +424,7 @@ def test_edge_identity_save_called_generate_audit_records_if_feature_override_up mocker: MockerFixture, edge_identity_model: EdgeIdentity, edge_identity_dynamo_wrapper_mock: MagicMock, + admin_user: FFAdminUser, ) -> None: # Given mocked_generate_audit_log_records = mocker.patch( @@ -453,7 +474,7 @@ def test_edge_identity_save_called_generate_audit_records_if_feature_override_up feature_override.set_value(new_value) # When - edge_identity_model.save(user=user) + edge_identity_model.save(user=admin_user) # Then edge_identity_dynamo_wrapper_mock.put_item.assert_called_once() @@ -461,7 +482,7 @@ def test_edge_identity_save_called_generate_audit_records_if_feature_override_up kwargs={ "environment_api_key": edge_identity_model.environment_api_key, "identifier": edge_identity_model.identifier, - "user_id": user.id, + "user_id": admin_user.id, "changes": expected_changes, "identity_uuid": expected_identity_uuid, "master_api_key_id": None, diff --git a/api/tests/unit/edge_api/identities/test_unit_edge_api_identities_tasks.py b/api/tests/unit/edge_api/identities/test_unit_edge_api_identities_tasks.py index 0c96ab715809..bc6f9cbdcc49 100644 --- a/api/tests/unit/edge_api/identities/test_unit_edge_api_identities_tasks.py +++ b/api/tests/unit/edge_api/identities/test_unit_edge_api_identities_tasks.py @@ -4,7 +4,6 @@ from django.utils import timezone from pytest_mock import MockerFixture -from api_keys.models import MasterAPIKey from audit.models import AuditLog from audit.related_object_type import RelatedObjectType from edge_api.identities.tasks import ( @@ -17,9 +16,7 @@ IdentityOverridesV2Changeset, IdentityOverrideV2, ) -from environments.identities.models import Identity from environments.models import Environment, Webhook -from features.models import Feature from webhooks.webhooks import WebhookEventType @@ -51,7 +48,7 @@ def test_call_environment_webhook_for_feature_state_change_with_new_state_only( environment_api_key=environment.api_key, identity_id=identity.id, identity_identifier=identity.identifier, - changed_by_user_id=admin_user.id, + changed_by=str(admin_user), timestamp=now_isoformat, new_enabled_state=new_enabled_state, new_value=new_value, @@ -104,7 +101,7 @@ def test_call_environment_webhook_for_feature_state_change_with_previous_state_o environment_api_key=environment.api_key, identity_id=identity.id, identity_identifier=identity.identifier, - changed_by_user_id=admin_user.id, + changed_by=str(admin_user), timestamp=now_isoformat, previous_enabled_state=previous_enabled_state, previous_value=previous_value, @@ -131,49 +128,6 @@ def test_call_environment_webhook_for_feature_state_change_with_previous_state_o assert data["timestamp"] == now_isoformat -def test_call_environment_webhook_for_feature_state_change_with_master_api_key_id( - mocker: MockerFixture, - environment: Environment, - feature: Feature, - identity: Identity, - master_api_key_object: MasterAPIKey, -) -> None: - # Given - mock_call_environment_webhooks = mocker.patch( - "edge_api.identities.tasks.call_environment_webhooks" - ) - Webhook.objects.create(environment=environment, url="https://foo.com/webhook") - - mock_feature_state_data = mocker.MagicMock() - mocker.patch.object( - Webhook, - "generate_webhook_feature_state_data", - return_value=mock_feature_state_data, - ) - - now_isoformat = timezone.now().isoformat() - previous_enabled_state = True - previous_value = "foo" - - # When - call_environment_webhook_for_feature_state_change( - feature_id=feature.id, - environment_api_key=environment.api_key, - identity_id=identity.id, - identity_identifier=identity.identifier, - changed_by_user_id=master_api_key_object.id, - timestamp=now_isoformat, - previous_enabled_state=previous_enabled_state, - previous_value=previous_value, - ) - - # Then - call_args = mock_call_environment_webhooks.call_args - - data = call_args[0][1] - assert data["changed_by"] == master_api_key_object.name - - @pytest.mark.parametrize( "previous_enabled_state, previous_value, new_enabled_state, new_value", ( @@ -216,7 +170,7 @@ def test_call_environment_webhook_for_feature_state_change_with_both_states( environment_api_key=environment.api_key, identity_id=identity.id, identity_identifier=identity.identifier, - changed_by_user_id=admin_user.id, + changed_by=str(admin_user), timestamp=now_isoformat, previous_enabled_state=previous_enabled_state, previous_value=previous_value, @@ -277,7 +231,7 @@ def test_call_environment_webhook_for_feature_state_change_does_nothing_if_no_we environment_api_key=environment.api_key, identity_id=identity.id, identity_identifier=identity.identifier, - changed_by_user_id=admin_user.id, + changed_by=str(admin_user), timestamp=now_isoformat, new_enabled_state=True, new_value="foo", From f8ca4df6e65a55faa7c941ac8009b584d08ff48b Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Wed, 4 Sep 2024 14:39:49 +0530 Subject: [PATCH 3/3] Add typing --- .../test_edge_api_identities_serializers.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/api/tests/unit/edge_api/identities/test_edge_api_identities_serializers.py b/api/tests/unit/edge_api/identities/test_edge_api_identities_serializers.py index cd704ce628ee..faa053a8fffa 100644 --- a/api/tests/unit/edge_api/identities/test_edge_api_identities_serializers.py +++ b/api/tests/unit/edge_api/identities/test_edge_api_identities_serializers.py @@ -3,14 +3,18 @@ from django.utils import timezone from flag_engine.features.models import FeatureModel, FeatureStateModel from pytest_lazyfixture import lazy_fixture +from pytest_mock import MockerFixture +from api_keys.user import APIKeyUser from edge_api.identities.models import EdgeIdentity from edge_api.identities.serializers import EdgeIdentityFeatureStateSerializer +from environments.identities.models import Identity from environments.identities.serializers import ( IdentityAllFeatureStatesSerializer, ) from features.feature_types import STANDARD -from features.models import FeatureState +from features.models import Feature, FeatureState +from users.models import FFAdminUser from util.mappers import map_identity_to_identity_document from webhooks.constants import WEBHOOK_DATETIME_FORMAT @@ -62,10 +66,10 @@ def test_edge_identity_feature_state_serializer_save_allows_missing_mvfsvs( ], ) def test_edge_identity_feature_state_serializer_save_calls_webhook_for_new_override( - mocker, - identity, - feature, - user, + mocker: MockerFixture, + identity: Identity, + feature: Feature, + user: FFAdminUser | APIKeyUser, rf: RequestFactory, ): # Given