diff --git a/api/audit/signals.py b/api/audit/signals.py index 65712d164f3f..20c2ddcf3e0f 100644 --- a/api/audit/signals.py +++ b/api/audit/signals.py @@ -217,11 +217,20 @@ def send_feature_flag_went_live_signal(sender, instance, **kwargs): # type: ign # Handle FeatureState, FeatureStateValue, and MultivariateFeatureStateValue audit logs # All these types have related_object_type=FEATURE_STATE - if isinstance(audited_instance, (FeatureStateValue, MultivariateFeatureStateValue)): - feature_state = audited_instance.feature_state - elif isinstance(audited_instance, FeatureState): - feature_state = audited_instance - else: + try: + if isinstance( + audited_instance, (FeatureStateValue, MultivariateFeatureStateValue) + ): + feature_state = audited_instance.feature_state + elif isinstance(audited_instance, FeatureState): + feature_state = audited_instance + else: + return + except FeatureState.DoesNotExist: + logger.debug( + "FeatureState no longer exists for audit log %d, skipping.", + instance.pk, + ) return if feature_state.is_scheduled: diff --git a/api/features/tasks.py b/api/features/tasks.py index 7ae434d9b60d..5c5f127b97a6 100644 --- a/api/features/tasks.py +++ b/api/features/tasks.py @@ -1,6 +1,7 @@ import logging from typing import Any +from django.core.exceptions import ObjectDoesNotExist from task_processor.decorators import ( register_task_handler, ) @@ -131,6 +132,12 @@ def _get_feature_state_webhook_data( ) assert feature_state.environment is not None + + try: + feature_segment = feature_state.feature_segment + except ObjectDoesNotExist: + feature_segment = None + return Webhook.generate_webhook_feature_state_data( feature_state.feature, environment=feature_state.environment, @@ -138,7 +145,7 @@ def _get_feature_state_webhook_data( value=value, identity_id=feature_state.identity_id, identity_identifier=getattr(feature_state.identity, "identifier", None), - feature_segment=feature_state.feature_segment, + feature_segment=feature_segment, multivariate_feature_state_values=mv_values, ) diff --git a/api/integrations/grafana/mappers.py b/api/integrations/grafana/mappers.py index 74c3abaee5f2..13c4118b9884 100644 --- a/api/integrations/grafana/mappers.py +++ b/api/integrations/grafana/mappers.py @@ -1,3 +1,5 @@ +from django.core.exceptions import ObjectDoesNotExist + from audit.models import AuditLog from audit.services import get_audited_instance_from_audit_log_record from features.models import ( @@ -20,41 +22,44 @@ def _get_feature_tags( def _get_instance_tags_from_audit_log_record( audit_log_record: AuditLog, ) -> list[str]: - if instance := get_audited_instance_from_audit_log_record(audit_log_record): - if isinstance(instance, Feature): - return [ - f"feature:{instance.name}", - *_get_feature_tags(instance), - ] + try: + if instance := get_audited_instance_from_audit_log_record(audit_log_record): + if isinstance(instance, Feature): + return [ + f"feature:{instance.name}", + *_get_feature_tags(instance), + ] - if isinstance(instance, FeatureState): - return [ - f"feature:{(feature := instance.feature).name}", - f"flag:{'enabled' if instance.enabled else 'disabled'}", - *_get_feature_tags(feature), # type: ignore[has-type] - ] + if isinstance(instance, FeatureState): + return [ + f"feature:{(feature := instance.feature).name}", + f"flag:{'enabled' if instance.enabled else 'disabled'}", + *_get_feature_tags(feature), # type: ignore[has-type] + ] - if isinstance(instance, FeatureStateValue): - return [ - f"feature:{(feature := instance.feature_state.feature).name}", - *_get_feature_tags(feature), - ] + if isinstance(instance, FeatureStateValue): + return [ + f"feature:{(feature := instance.feature_state.feature).name}", + *_get_feature_tags(feature), + ] - if isinstance(instance, Segment): - return [f"segment:{instance.name}"] + if isinstance(instance, Segment): + return [f"segment:{instance.name}"] - if isinstance(instance, FeatureSegment): - return [ - f"feature:{(feature := instance.feature).name}", - f"segment:{instance.segment.name}", - *_get_feature_tags(feature), - ] + if isinstance(instance, FeatureSegment): + return [ + f"feature:{(feature := instance.feature).name}", + f"segment:{instance.segment.name}", + *_get_feature_tags(feature), + ] - if isinstance(instance, EnvironmentFeatureVersion): - return [ - f"feature:{instance.feature.name}", - *_get_feature_tags(instance.feature), - ] + if isinstance(instance, EnvironmentFeatureVersion): + return [ + f"feature:{instance.feature.name}", + *_get_feature_tags(instance.feature), + ] + except ObjectDoesNotExist: + return [] return [] diff --git a/api/tests/unit/audit/test_unit_audit_signals.py b/api/tests/unit/audit/test_unit_audit_signals.py index 2ab3eea2e0f1..349af1693fe0 100644 --- a/api/tests/unit/audit/test_unit_audit_signals.py +++ b/api/tests/unit/audit/test_unit_audit_signals.py @@ -16,7 +16,7 @@ trigger_feature_state_change_webhooks, ) from environments.models import Environment -from features.models import Feature, FeatureState +from features.models import Feature, FeatureState, FeatureStateValue from features.versioning.models import EnvironmentFeatureVersion from integrations.dynatrace.dynatrace import EVENTS_API_URI from integrations.dynatrace.models import DynatraceConfiguration @@ -596,3 +596,40 @@ def test_send_feature_flag_went_live_signal__non_feature_state_instance__skips_s # Then mock_signal_send.assert_not_called() + + +def test_send_feature_flag_went_live_signal__deleted_feature_state__skips_signal( + environment: Environment, + feature: Feature, + mocker: MockerFixture, +) -> None: + # Given + feature_state = FeatureState.objects.get( + environment=environment, feature=feature, feature_segment__isnull=True + ) + feature_state_value = feature_state.feature_state_value + + audit_log = AuditLog.objects.create( + environment=environment, + related_object_id=feature_state.id, + related_object_type=RelatedObjectType.FEATURE_STATE.name, + history_record_id=feature_state_value.history.first().history_id, + history_record_class_path=feature_state_value.history_record_class_path, + ) + + # Simulate the FeatureState being deleted before the signal runs. + mock_audited = mocker.MagicMock(spec=FeatureStateValue) + type(mock_audited).feature_state = mocker.PropertyMock( + side_effect=FeatureState.DoesNotExist + ) + mocker.patch( + "audit.signals.get_audited_instance_from_audit_log_record", + return_value=mock_audited, + ) + mock_signal_send = mocker.patch("audit.signals.feature_state_change_went_live.send") + + # When + send_feature_flag_went_live_signal(sender=AuditLog, instance=audit_log) + + # Then + mock_signal_send.assert_not_called() diff --git a/api/tests/unit/features/test_unit_features_tasks.py b/api/tests/unit/features/test_unit_features_tasks.py index b5bfa79c2863..d2867e2e755f 100644 --- a/api/tests/unit/features/test_unit_features_tasks.py +++ b/api/tests/unit/features/test_unit_features_tasks.py @@ -1,11 +1,15 @@ import pytest +from django.core.exceptions import ObjectDoesNotExist from pytest_lazyfixture import lazy_fixture # type: ignore[import-untyped] from pytest_mock import MockerFixture from api_keys.models import MasterAPIKey from environments.models import Environment from features.models import Feature, FeatureState -from features.tasks import trigger_feature_state_change_webhooks +from features.tasks import ( + _get_feature_state_webhook_data, + trigger_feature_state_change_webhooks, +) from organisations.models import Organisation from projects.models import Project from users.models import FFAdminUser @@ -162,3 +166,26 @@ def test_trigger_feature_state_change_webhooks_for_deleted_flag_uses_fs_instance assert data["previous_state"]["feature"]["id"] == feature_state.feature.id assert event_type == WebhookEventType.FLAG_DELETED.value + + +def test_get_feature_state_webhook_data__deleted_feature_segment__returns_data( + mocker: MockerFixture, + environment: Environment, + feature: Feature, +) -> None: + # Given + feature_state = FeatureState.objects.get(feature=feature, environment=environment) + + # Simulate the FeatureSegment being deleted after the FeatureState was loaded. + mocker.patch.object( + type(feature_state), + "feature_segment", + new_callable=mocker.PropertyMock, + side_effect=ObjectDoesNotExist, + ) + + # When + data = _get_feature_state_webhook_data(feature_state) + + # Then - should not raise; feature_segment should be None in the result. + assert data["feature_segment"] is None diff --git a/api/tests/unit/integrations/grafana/test_mappers.py b/api/tests/unit/integrations/grafana/test_mappers.py index 13546335dee2..3d89f6d3568b 100644 --- a/api/tests/unit/integrations/grafana/test_mappers.py +++ b/api/tests/unit/integrations/grafana/test_mappers.py @@ -5,7 +5,7 @@ from audit.models import AuditLog from environments.models import Environment -from features.models import Feature, FeatureSegment, FeatureState +from features.models import Feature, FeatureSegment, FeatureState, FeatureStateValue from integrations.grafana.mappers import ( map_audit_log_record_to_grafana_annotation, ) @@ -199,6 +199,37 @@ def test_map_audit_log_record_to_grafana_annotation__feature_segment__return_exp } +def test_map_audit_log_record_to_grafana_annotation__deleted_feature_state__returns_no_instance_tags( + mocker: MockerFixture, + audit_log_record: AuditLog, +) -> None: + # Given + mock_instance = mocker.MagicMock(spec=FeatureStateValue) + type(mock_instance).feature_state = mocker.PropertyMock( + side_effect=FeatureState.DoesNotExist + ) + mocker.patch( + "integrations.grafana.mappers.get_audited_instance_from_audit_log_record", + return_value=mock_instance, + ) + + # When + annotation = map_audit_log_record_to_grafana_annotation(audit_log_record) + + # Then + assert annotation == { + "tags": [ + "flagsmith", + "project:Test Project", + "environment:unknown", + "by:superuser@example.com", + ], + "text": "Test event", + "time": 1719220187325, + "timeEnd": 1719220187325, + } + + @pytest.mark.django_db def test_map_audit_log_record_to_grafana_annotation__generic__return_expected( mocker: MockerFixture,