diff --git a/api/segments/managers.py b/api/segments/managers.py deleted file mode 100644 index d1adddb94771..000000000000 --- a/api/segments/managers.py +++ /dev/null @@ -1,16 +0,0 @@ -from django.db.models import F - -from core.models import SoftDeleteExportableManager - - -class SegmentManager(SoftDeleteExportableManager): - pass - - -class LiveSegmentManager(SoftDeleteExportableManager): - def get_queryset(self): # type: ignore[no-untyped-def] - """ - Returns only the canonical segments, which will always be - the highest version. - """ - return super().get_queryset().filter(id=F("version_of")) diff --git a/api/segments/migrations/0030_add_default_to_segment_version.py b/api/segments/migrations/0030_add_default_to_segment_version.py new file mode 100644 index 000000000000..4813ecd210a5 --- /dev/null +++ b/api/segments/migrations/0030_add_default_to_segment_version.py @@ -0,0 +1,23 @@ +# Generated by Django 4.2.22 on 2025-11-11 00:08 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("segments", "0029_add_is_system_segment"), + ] + + operations = [ + migrations.AlterField( + model_name="historicalsegment", + name="version", + field=models.IntegerField(default=1, null=True), + ), + migrations.AlterField( + model_name="segment", + name="version", + field=models.IntegerField(default=1, null=True), + ), + ] diff --git a/api/segments/models.py b/api/segments/models.py index c1892383eb55..89966e97f818 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -9,7 +9,6 @@ from django.db import models, transaction from django_lifecycle import ( # type: ignore[import-untyped] AFTER_CREATE, - BEFORE_CREATE, LifecycleModelMixin, hook, ) @@ -30,13 +29,24 @@ from metadata.models import Metadata from projects.models import Project -from .managers import LiveSegmentManager, SegmentManager - ModelT = typing.TypeVar("ModelT", bound=models.Model) logger = logging.getLogger(__name__) +class SegmentManager(SoftDeleteExportableManager): + pass + + +class LiveSegmentManager(SoftDeleteExportableManager): + def get_queryset(self): # type: ignore[no-untyped-def] + """ + Returns only the canonical segments, which will always be + the highest version. + """ + return super().get_queryset().filter(id=models.F("version_of")) + + class ConfiguredOrderManager(SoftDeleteExportableManager, models.Manager[ModelT]): setting_name: str @@ -87,8 +97,7 @@ class Segment( Feature, on_delete=models.CASCADE, related_name="segments", null=True ) - # This defaults to 1 for newly created segments. - version = models.IntegerField(null=True) + version = models.IntegerField(default=1, null=True) version_of = models.ForeignKey( "self", @@ -126,18 +135,8 @@ def __str__(self): # type: ignore[no-untyped-def] def get_skip_create_audit_log(self) -> bool: if self.is_system_segment: return True - try: - if self.version_of_id and self.version_of_id != self.id: - return True - except Segment.DoesNotExist: - return True - - return False - - @hook(BEFORE_CREATE, when="version_of", is_now=None) - def set_default_version_to_one_if_new_segment(self): # type: ignore[no-untyped-def] - if self.version is None: - self.version = 1 + is_revision = bool(self.version_of_id and self.version_of_id != self.id) + return is_revision @hook(AFTER_CREATE, when="version_of", is_now=None) def set_version_of_to_self_if_none(self): # type: ignore[no-untyped-def] diff --git a/api/tests/unit/segments/test_unit_segments_models.py b/api/tests/unit/segments/test_unit_segments_models.py index bb689d0239ff..ae9b392bdeb1 100644 --- a/api/tests/unit/segments/test_unit_segments_models.py +++ b/api/tests/unit/segments/test_unit_segments_models.py @@ -1,6 +1,5 @@ from collections.abc import Callable from typing import Any -from unittest.mock import PropertyMock import pytest from django.core.exceptions import ValidationError @@ -29,7 +28,15 @@ def test_Condition_str__returns_readable_representation_of_condition( assert result == "Condition for ALL rule for Segment - segment: foo EQUAL bar" -def test_Condition_get_skip_create_audit_log__returns_true( +@pytest.mark.parametrize( + "delete", + [ + lambda rule: rule.delete(), + lambda rule: rule.hard_delete(), + ], +) +def test_Condition_get_skip_create_audit_log__rule_deleted__returns_true( + delete: Callable[[SegmentRule], None], segment_rule: SegmentRule, ) -> None: # Given @@ -38,16 +45,45 @@ def test_Condition_get_skip_create_audit_log__returns_true( property="foo", operator=EQUAL, value="bar", + created_with_segment=False, ) # When - result = condition.get_skip_create_audit_log() + delete(segment_rule) # Then - assert result is True + assert condition.get_skip_create_audit_log() is True -def test_manager_returns_only_highest_version_of_segments( +@pytest.mark.parametrize( + "delete", + [ + lambda segment: segment.delete(), + lambda segment: segment.hard_delete(), + ], +) +def test_Condition_get_skip_create_audit_log__segment_deleted__returns_true( + delete: Callable[[Segment], None], + segment: Segment, + segment_rule: SegmentRule, +) -> None: + # Given + condition = Condition.objects.create( + rule=segment_rule, + property="foo", + operator=EQUAL, + value="bar", + created_with_segment=False, + ) + + # When + delete(segment) + + # Then + assert condition.get_skip_create_audit_log() is True + + +def test_LiveSegmentManager__returns_only_highest_version_of_segments( segment: Segment, ) -> None: # Given @@ -108,24 +144,7 @@ def test_SegmentRule_get_skip_create_audit_log__returns_true( assert result is True -def test_segment_get_skip_create_audit_log_when_exception( - mocker: MockerFixture, - segment: Segment, -) -> None: - # Given - patched_segment = mocker.patch.object( - Segment, "version_of_id", new_callable=PropertyMock - ) - patched_segment.side_effect = Segment.DoesNotExist("Segment missing") - - # When - result = segment.get_skip_create_audit_log() - - # Then - assert result is True - - -def test_delete_segment_only_schedules_one_task_for_audit_log_creation( +def test_Segment_delete__multiple_rules_conditions__schedules_audit_log_task_once( mocker: MockerFixture, segment: Segment ) -> None: # Given @@ -143,11 +162,11 @@ def test_delete_segment_only_schedules_one_task_for_audit_log_creation( ) # When - mocked_tasks = mocker.patch("core.signals.tasks") + task = mocker.patch("core.signals.tasks.create_audit_log_from_historical_record") segment.delete() # Then - assert len(mocked_tasks.mock_calls) == 1 + assert task.delay.call_count == 1 def test_Segment_clone__can_create_standalone_segment_clone( @@ -264,7 +283,9 @@ def test_Segment_clone__segment_with_rules__returns_new_segment_with_copied_rule ] -def test_system_segment_get_skip_create_audit_log(system_segment: Segment) -> None: +def test_Segment_get_skip_create_audit_log__system_segment__returns_true( + system_segment: Segment, +) -> None: # When result = system_segment.get_skip_create_audit_log()