diff --git a/api/core/models.py b/api/core/models.py index b561833b5e51..b5791a153eed 100644 --- a/api/core/models.py +++ b/api/core/models.py @@ -42,6 +42,16 @@ def natural_key(self): return (str(self.uuid),) +class SoftDeleteAwareHistoricalRecords(HistoricalRecords): + def create_historical_record( + self, instance: models.Model, history_type: str, using: str = None + ) -> None: + if getattr(instance, "deleted_at", None) is not None and history_type == "~": + # Don't create `update` historical record for soft-deleted objects + return + super().create_historical_record(instance, history_type, using) + + class SoftDeleteExportableManager(UUIDNaturalKeyManagerMixin, SoftDeleteManager): pass @@ -198,7 +208,7 @@ def abstract_base_auditable_model_factory( show_change_details_for_create: bool = False, ) -> typing.Type[AbstractBaseAuditableModel]: class Base(AbstractBaseAuditableModel): - history = HistoricalRecords( + history = SoftDeleteAwareHistoricalRecords( bases=[ base_historical_model_factory( change_details_excluded_fields or [], diff --git a/api/core/signals.py b/api/core/signals.py index db3d92bdc09e..a5efe0040ddb 100644 --- a/api/core/signals.py +++ b/api/core/signals.py @@ -34,6 +34,8 @@ def create_audit_log_from_historical_record( if settings.TASK_RUN_METHOD == TaskRunMethod.TASK_PROCESSOR else None ) + if instance.get_skip_create_audit_log(): + return try: environment, project = instance.get_environment_and_project() diff --git a/api/features/signals.py b/api/features/signals.py index 1fa2d864bdbf..1d9a5ea1926d 100644 --- a/api/features/signals.py +++ b/api/features/signals.py @@ -12,6 +12,6 @@ @receiver(post_save, sender=FeatureState) def trigger_feature_state_change_webhooks_signal(instance, **kwargs): - if instance.environment_feature_version_id: + if instance.environment_feature_version_id or instance.deleted_at: return trigger_feature_state_change_webhooks(instance) diff --git a/api/poetry.lock b/api/poetry.lock index cf05d38d5831..cfefc879b7b9 100644 --- a/api/poetry.lock +++ b/api/poetry.lock @@ -918,19 +918,18 @@ files = [ [[package]] name = "django-lifecycle" -version = "1.0.0" +version = "1.2.4" description = "Declarative model lifecycle hooks." optional = false python-versions = "*" groups = ["main"] files = [ - {file = "django-lifecycle-1.0.0.tar.gz", hash = "sha256:136c48e65d439cc34a6cb9a10d6d17611a9a5223f8249ee9c190ca5c7a67e9c1"}, + {file = "django_lifecycle-1.2.4-py3-none-any.whl", hash = "sha256:b54aea17b50de45adb5c90a06eea0171afa0d547682f51990dffb578b82fc658"}, + {file = "django_lifecycle-1.2.4.tar.gz", hash = "sha256:b37add8a95d0e85f9f97e652fac989cd5914cddb2380d933b6568f80238ab61e"}, ] [package.dependencies] -Django = ">=2.0" -packaging = ">=21.0" -urlman = ">=1.2.0" +Django = ">=3.2" [[package]] name = "django-multiselectfield" @@ -3552,6 +3551,7 @@ files = [ {file = "PyYAML-6.0.1-cp311-cp311-win_amd64.whl", hash = "sha256:bf07ee2fef7014951eeb99f56f39c9bb4af143d8aa3c21b1677805985307da34"}, {file = "PyYAML-6.0.1-cp312-cp312-macosx_10_9_x86_64.whl", hash = "sha256:855fb52b0dc35af121542a76b9a84f8d1cd886ea97c84703eaa6d88e37a2ad28"}, {file = "PyYAML-6.0.1-cp312-cp312-macosx_11_0_arm64.whl", hash = "sha256:40df9b996c2b73138957fe23a16a4f0ba614f4c0efce1e9406a184b6d07fa3a9"}, + {file = "PyYAML-6.0.1-cp312-cp312-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:a08c6f0fe150303c1c6b71ebcd7213c2858041a7e01975da3a99aed1e7a378ef"}, {file = "PyYAML-6.0.1-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:6c22bec3fbe2524cde73d7ada88f6566758a8f7227bfbf93a408a9d86bcc12a0"}, {file = "PyYAML-6.0.1-cp312-cp312-musllinux_1_1_x86_64.whl", hash = "sha256:8d4e9c88387b0f5c7d5f281e55304de64cf7f9c0021a3525bd3b1c542da3b0e4"}, {file = "PyYAML-6.0.1-cp312-cp312-win32.whl", hash = "sha256:d483d2cdf104e7c9fa60c544d92981f12ad66a457afae824d146093b8c294c54"}, @@ -4216,18 +4216,6 @@ brotli = ["brotli (==1.0.9)", "brotli (>=1.0.9)", "brotlicffi (>=0.8.0)", "brotl secure = ["certifi", "cryptography (>=1.3.4)", "idna (>=2.0.0)", "ipaddress", "pyOpenSSL (>=0.14)", "urllib3-secure-extra"] socks = ["PySocks (>=1.5.6,!=1.5.7,<2.0)"] -[[package]] -name = "urlman" -version = "2.0.1" -description = "Django URL pattern helpers" -optional = false -python-versions = "*" -groups = ["main"] -files = [ - {file = "urlman-2.0.1-py2.py3-none-any.whl", hash = "sha256:909177485ddbbeada15a21f80155762faf70fc53be31890ff7288e21803f06c7"}, - {file = "urlman-2.0.1.tar.gz", hash = "sha256:3b9c5ac4e447b1e29fa259dc76953d46d711c84b296a0c66c34870e248eb1205"}, -] - [[package]] name = "virtualenv" version = "20.28.0" @@ -4457,4 +4445,4 @@ files = [ [metadata] lock-version = "2.1" python-versions = ">=3.11, <3.13" -content-hash = "1d3c289404e691bbe2c044d0794e81a46fc982104960f1edcfd8a07c2100c49e" +content-hash = "3709b760a58385704708366fb1ce9ee65309c01ff4939e8e2ddeca66dcd43e3b" diff --git a/api/pyproject.toml b/api/pyproject.toml index 38657f2aa7ce..e4e4abbb668e 100644 --- a/api/pyproject.toml +++ b/api/pyproject.toml @@ -140,7 +140,7 @@ drf-yasg = "~1.21.6" django-debug-toolbar = "~3.2.1" sentry-sdk = "~2.8.0" environs = "~9.2.0" -django-lifecycle = "~1.0.0" +django-lifecycle = "~1.2.4" drf-writable-nested = "~0.6.2" django-filter = "~2.4.0" flagsmith-flag-engine = "^5.3.0" diff --git a/api/segments/helpers.py b/api/segments/helpers.py deleted file mode 100644 index 9d5797f512dd..000000000000 --- a/api/segments/helpers.py +++ /dev/null @@ -1,16 +0,0 @@ -class SegmentAuditLogHelper: - def __init__(self) -> None: - self.skip_audit_log = {} - - def should_skip_audit_log(self, segment_id: int) -> None | bool: - return self.skip_audit_log.get(segment_id) - - def set_skip_audit_log(self, segment_id: int) -> None: - self.skip_audit_log[segment_id] = True - - def unset_skip_audit_log(self, segment_id: int) -> None: - if segment_id in self.skip_audit_log: - del self.skip_audit_log[segment_id] - - -segment_audit_log_helper = SegmentAuditLogHelper() diff --git a/api/segments/migrations/0027_historicalsegmentrule.py b/api/segments/migrations/0027_historicalsegmentrule.py new file mode 100644 index 000000000000..50ba876ad8bf --- /dev/null +++ b/api/segments/migrations/0027_historicalsegmentrule.py @@ -0,0 +1,42 @@ +# Generated by Django 3.2.23 on 2025-01-23 12:19 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import simple_history.models + + +class Migration(migrations.Migration): + + dependencies = [ + ('api_keys', '0003_masterapikey_is_admin'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('segments', '0026_add_change_request_to_segments'), + ] + + operations = [ + migrations.CreateModel( + name='HistoricalSegmentRule', + fields=[ + ('id', models.IntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID')), + ('deleted_at', models.DateTimeField(blank=True, db_index=True, default=None, editable=False, null=True)), + ('type', models.CharField(choices=[('ALL', 'all'), ('ANY', 'any'), ('NONE', 'none')], max_length=50)), + ('created_at', models.DateTimeField(blank=True, editable=False, null=True)), + ('updated_at', models.DateTimeField(blank=True, editable=False, null=True)), + ('history_id', models.AutoField(primary_key=True, serialize=False)), + ('history_date', models.DateTimeField()), + ('history_change_reason', models.CharField(max_length=100, null=True)), + ('history_type', models.CharField(choices=[('+', 'Created'), ('~', 'Changed'), ('-', 'Deleted')], max_length=1)), + ('history_user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL)), + ('master_api_key', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.DO_NOTHING, to='api_keys.masterapikey')), + ('rule', models.ForeignKey(blank=True, db_constraint=False, null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='segments.segmentrule')), + ('segment', models.ForeignKey(blank=True, db_constraint=False, null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='segments.segment')), + ], + options={ + 'verbose_name': 'historical segment rule', + 'ordering': ('-history_date', '-history_id'), + 'get_latest_by': 'history_date', + }, + bases=(simple_history.models.HistoricalChanges, models.Model), + ), + ] diff --git a/api/segments/models.py b/api/segments/models.py index 22b8c30d19d1..f904f1a4d2a4 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -29,7 +29,6 @@ from metadata.models import Metadata from projects.models import Project -from .helpers import segment_audit_log_helper from .managers import LiveSegmentManager, SegmentManager logger = logging.getLogger(__name__) @@ -92,10 +91,6 @@ def __str__(self): return "Segment - %s" % self.name def get_skip_create_audit_log(self) -> bool: - skip = segment_audit_log_helper.should_skip_audit_log(self.id) - if skip is not None: - return skip - try: if self.version_of_id and self.version_of_id != self.id: return True @@ -146,10 +141,8 @@ def set_version_of_to_self_if_none(self): This allows the segment model to reference all versions of itself including itself. """ - segment_audit_log_helper.set_skip_audit_log(self.id) self.version_of = self - self.save() - segment_audit_log_helper.unset_skip_audit_log(self.id) + self.save_without_historical_record() def shallow_clone( self, @@ -178,10 +171,8 @@ def deep_clone(self) -> "Segment": cloned_segment.version_of = self cloned_segment.save() - segment_audit_log_helper.set_skip_audit_log(self.id) self.version += 1 - self.save() - segment_audit_log_helper.unset_skip_audit_log(self.id) + self.save_without_historical_record() cloned_rules = [] for rule in self.rules.all(): @@ -211,7 +202,10 @@ def _get_project(self): return self.project -class SegmentRule(SoftDeleteExportableModel): +class SegmentRule( + SoftDeleteExportableModel, + abstract_base_auditable_model_factory(["uuid"]), +): ALL_RULE = "ALL" ANY_RULE = "ANY" NONE_RULE = "NONE" @@ -230,6 +224,8 @@ class SegmentRule(SoftDeleteExportableModel): created_at = models.DateTimeField(null=True, auto_now_add=True) updated_at = models.DateTimeField(null=True, auto_now=True) + history_record_class_path = "segments.models.HistoricalSegmentRule" + def clean(self): super().clean() parents = [self.segment, self.rule] @@ -246,8 +242,17 @@ def __str__(self): ) def get_skip_create_audit_log(self) -> bool: - segment = self.get_segment() - return segment.version_of_id != segment.id + try: + segment = self.get_segment() + if segment.deleted_at: + return True + return segment.version_of_id != segment.id + except (Segment.DoesNotExist, SegmentRule.DoesNotExist): + # handle hard delete + return True + + def _get_project(self) -> typing.Optional[Project]: + return self.get_segment().project def get_segment(self): """ @@ -361,8 +366,19 @@ def __str__(self): ) def get_skip_create_audit_log(self) -> bool: - segment = self.rule.get_segment() - return segment.version_of_id != segment.id + try: + + if self.rule.deleted_at: + return True + + segment = self.rule.get_segment() + if segment.deleted_at: + return True + + return segment.version_of_id != segment.id + except (Segment.DoesNotExist, SegmentRule.DoesNotExist): + # handle hard delete + return True def get_update_log_message(self, history_instance) -> typing.Optional[str]: return f"Condition updated on segment '{self._get_segment().name}'." diff --git a/api/tests/unit/features/test_unit_features_models.py b/api/tests/unit/features/test_unit_features_models.py index 6f440e8917dc..5188a199beab 100644 --- a/api/tests/unit/features/test_unit_features_models.py +++ b/api/tests/unit/features/test_unit_features_models.py @@ -470,6 +470,23 @@ def test_feature_state_save_calls_trigger_webhooks( mock_trigger_webhooks.assert_called_with(feature_state) +def test_delete_feature_should_not_trigger_fs_change_webhooks( + mocker: MockerFixture, + feature: Feature, + environment: Environment, +) -> None: + # Given + mock_trigger_webhooks = mocker.patch( + "features.signals.trigger_feature_state_change_webhooks" + ) + + # When + feature.delete() + + # Then + mock_trigger_webhooks.assert_not_called() + + def test_feature_state_type_environment( feature: Feature, environment: Environment, diff --git a/api/tests/unit/segments/test_unit_segments_models.py b/api/tests/unit/segments/test_unit_segments_models.py index 60ae2750e110..f2d8e61a1016 100644 --- a/api/tests/unit/segments/test_unit_segments_models.py +++ b/api/tests/unit/segments/test_unit_segments_models.py @@ -90,6 +90,60 @@ def test_condition_get_delete_log_message_for_valid_segment( assert msg == f"Condition removed from segment '{segment.name}'." +def test__condition_get_skip_create_audit_log_on_rule_delete( + segment_rule: SegmentRule, +) -> None: + # Given + condition = Condition.objects.create( + rule=segment_rule, + property="foo", + operator=EQUAL, + value="bar", + created_with_segment=False, + ) + # When + segment_rule.delete() + + # Then + assert condition.get_skip_create_audit_log() is True + + +def test__condition_get_skip_create_audit_log_on_segment_delete( + segment_rule: SegmentRule, segment: Segment +) -> None: + # Given + condition = Condition.objects.create( + rule=segment_rule, + property="foo", + operator=EQUAL, + value="bar", + created_with_segment=False, + ) + # When + segment.delete() + + # Then + assert condition.get_skip_create_audit_log() is True + + +def test__condition_get_skip_create_audit_log_on_segment_hard_delete( + segment_rule: SegmentRule, segment: Segment +) -> None: + # Given + condition = Condition.objects.create( + rule=segment_rule, + property="foo", + operator=EQUAL, + value="bar", + created_with_segment=False, + ) + # When + segment.delete() + + # Then + assert condition.get_skip_create_audit_log() is True + + def test_condition_get_delete_log_message_for_deleted_segment( segment, segment_rule, mocker ): @@ -404,6 +458,36 @@ def test_deep_clone_of_segment_with_grandchild_rule( ) +def test_segment_rule_get_skip_create_on_segment_hard_delete( + segment: Segment, +) -> None: + # Given + segment_rule = SegmentRule.objects.create( + segment=segment, type=SegmentRule.ALL_RULE + ) + + # When + segment.hard_delete() + + # Then + assert segment_rule.get_skip_create_audit_log() is True + + +def test_segment_rule_get_skip_create_on_segment_delete( + segment: Segment, +) -> None: + # Given + segment_rule = SegmentRule.objects.create( + segment=segment, type=SegmentRule.ALL_RULE + ) + + # When + segment.delete() + + # Then + assert segment_rule.get_skip_create_audit_log() is True + + def test_segment_rule_get_skip_create_audit_log_when_doesnt_skip( segment: Segment, ) -> None: @@ -451,3 +535,28 @@ def test_segment_get_skip_create_audit_log_when_exception( # Then assert result is True + + +def test_delete_segment_only_schedules_one_task_for_audit_log_creation( + mocker: MockerFixture, segment: Segment +) -> None: + # Given + for _ in range(5): + segment_rule = SegmentRule.objects.create( + segment=segment, type=SegmentRule.ALL_RULE + ) + for _ in range(5): + Condition.objects.create( + rule=segment_rule, + property="foo", + operator=EQUAL, + value="bar", + created_with_segment=False, + ) + + # When + mocked_tasks = mocker.patch("core.signals.tasks") + segment.delete() + + # Then + assert len(mocked_tasks.mock_calls) == 1 diff --git a/api/tests/unit/users/test_unit_users_views.py b/api/tests/unit/users/test_unit_users_views.py index 00ce5fff89fa..f93a184f6475 100644 --- a/api/tests/unit/users/test_unit_users_views.py +++ b/api/tests/unit/users/test_unit_users_views.py @@ -760,7 +760,7 @@ def test_delete_user_social_auth_with_no_password(password): @pytest.mark.django_db def test_change_email_address_api(mocker): # Given - mocked_task = mocker.patch("users.signals.send_email_changed_notification_email") + mocked_task = mocker.patch("users.tasks.send_email_changed_notification_email") # create an user old_email = "test_user@test.com" first_name = "firstname" diff --git a/api/users/models.py b/api/users/models.py index 2f0487a4592d..e5565cb2d7f9 100644 --- a/api/users/models.py +++ b/api/users/models.py @@ -10,7 +10,8 @@ from django.db import models from django.db.models import Count, QuerySet from django.utils import timezone -from django_lifecycle import AFTER_CREATE, LifecycleModel, hook +from django_lifecycle import AFTER_CREATE, AFTER_SAVE, LifecycleModel, hook +from django_lifecycle.conditions import WhenFieldHasChanged from integrations.lead_tracking.hubspot.tasks import ( track_hubspot_lead_without_organisation, @@ -134,6 +135,18 @@ def schedule_hubspot_tracking(self) -> None: ), ) + @hook(AFTER_SAVE, condition=(WhenFieldHasChanged("email", has_changed=True))) + def send_warning_email(self): + from users.tasks import send_email_changed_notification_email + + send_email_changed_notification_email.delay( + args=( + self.first_name, + settings.DEFAULT_FROM_EMAIL, + self.initial_value("email"), + ) + ) + def delete_orphan_organisations(self): Organisation.objects.filter( id__in=self.organisations.values_list("id", flat=True) diff --git a/api/users/signals.py b/api/users/signals.py index 8dd4bc9603ff..a6544e301b00 100644 --- a/api/users/signals.py +++ b/api/users/signals.py @@ -1,6 +1,5 @@ import warnings -from django.conf import settings from django.db.models.signals import post_migrate, post_save from django.dispatch import receiver from django.urls import reverse @@ -9,10 +8,7 @@ PipedriveLeadTracker, ) from users.models import FFAdminUser -from users.tasks import ( - create_pipedrive_lead, - send_email_changed_notification_email, -) +from users.tasks import create_pipedrive_lead @receiver(post_migrate, sender=FFAdminUser) @@ -37,15 +33,3 @@ def create_pipedrive_lead_signal(sender, instance, created, **kwargs): return create_pipedrive_lead.delay(args=(user.id,)) - - -@receiver(post_save, sender=FFAdminUser) -def send_warning_email(sender, instance, created, **kwargs): - if instance._initial_state and (instance._initial_state["email"] != instance.email): - send_email_changed_notification_email.delay( - args=( - instance.first_name, - settings.DEFAULT_FROM_EMAIL, - instance._initial_state["email"], - ) - )