diff --git a/api/features/serializers.py b/api/features/serializers.py index 26b6ea771c7c..70a55bda4f28 100644 --- a/api/features/serializers.py +++ b/api/features/serializers.py @@ -1,4 +1,5 @@ import typing +from datetime import datetime import django.core.exceptions from drf_writable_nested import WritableNestedModelSerializer @@ -121,6 +122,18 @@ class ListCreateFeatureSerializer(DeleteBeforeUpdateWritableNestedModelSerialize "Note: will return null for Edge enabled projects." ) + last_modified_in_any_environment = serializers.SerializerMethodField( + help_text="Datetime representing the last time that the feature was modified " + "in any environment in the given project. Note: requires feature " + "versioning v2 enabled on the environment." + ) + last_modified_in_current_environment = serializers.SerializerMethodField( + help_text="Datetime representing the last time that the feature was modified " + "in any environment in the current environment. Note: requires that " + "the environment query parameter is passed and feature versioning v2 " + "enabled on the environment." + ) + class Meta: model = Feature fields = ( @@ -141,6 +154,8 @@ class Meta: "num_segment_overrides", "num_identity_overrides", "is_server_key_only", + "last_modified_in_any_environment", + "last_modified_in_current_environment", ) read_only_fields = ("feature_segments", "created_date", "uuid", "project") @@ -244,6 +259,16 @@ def get_num_identity_overrides(self, instance) -> typing.Optional[int]: except (KeyError, AttributeError): return None + def get_last_modified_in_any_environment( + self, instance: Feature + ) -> datetime | None: + return getattr(instance, "last_modified_in_any_environment", None) + + def get_last_modified_in_current_environment( + self, instance: Feature + ) -> datetime | None: + return getattr(instance, "last_modified_in_current_environment", None) + class UpdateFeatureSerializer(ListCreateFeatureSerializer): """prevent users from changing certain values after creation""" diff --git a/api/features/views.py b/api/features/views.py index bad7082e6934..62cb0b610def 100644 --- a/api/features/views.py +++ b/api/features/views.py @@ -8,7 +8,7 @@ from core.request_origin import RequestOrigin from django.conf import settings from django.core.cache import caches -from django.db.models import Q, QuerySet +from django.db.models import Max, Q, QuerySet from django.utils.decorators import method_decorator from django.views.decorators.cache import cache_page from drf_yasg import openapi @@ -117,8 +117,18 @@ def get_queryset(self): accessible_projects = self.request.user.get_permitted_projects(VIEW_PROJECT) project = get_object_or_404(accessible_projects, pk=self.kwargs["project_pk"]) - queryset = project.features.all().prefetch_related( - "multivariate_options", "owners", "tags", "group_owners" + + queryset = ( + project.features.all() + .annotate( + last_modified_in_any_environment=Max( + "feature_states__environment_feature_version__created_at", + filter=Q( + feature_states__environment_feature_version__published_at__isnull=False + ), + ), + ) + .prefetch_related("multivariate_options", "owners", "tags", "group_owners") ) query_serializer = FeatureQuerySerializer(data=self.request.query_params) @@ -127,6 +137,17 @@ def get_queryset(self): queryset = self._filter_queryset(queryset) + if environment_id := query_data.get("environment"): + queryset = queryset.annotate( + last_modified_in_current_environment=Max( + "feature_states__environment_feature_version__created_at", + filter=Q( + feature_states__environment=environment_id, + feature_states__environment_feature_version__published_at__isnull=False, + ), + ) + ) + sort = "%s%s" % ( "-" if query_data["sort_direction"] == "DESC" else "", query_data["sort_field"], diff --git a/api/projects/migrations/0022_add_stale_flags_threshold_to_project.py b/api/projects/migrations/0022_add_stale_flags_threshold_to_project.py new file mode 100644 index 000000000000..f50a2cb881f5 --- /dev/null +++ b/api/projects/migrations/0022_add_stale_flags_threshold_to_project.py @@ -0,0 +1,19 @@ +# Generated by Django 3.2.23 on 2023-12-06 14:35 +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("projects", "0021_add_identity_overrides_migration_status"), + ] + + operations = [ + migrations.AddField( + model_name="project", + name="stale_flags_limit_days", + field=models.IntegerField( + default=30, + help_text="Number of days without modification in any environment before a flag is considered stale.", + ), + ), + ] diff --git a/api/projects/models.py b/api/projects/models.py index f87cc4d930a2..329c0e3668ee 100644 --- a/api/projects/models.py +++ b/api/projects/models.py @@ -88,6 +88,10 @@ class Project(LifecycleModelMixin, SoftDeleteExportableModel): choices=IdentityOverridesV2MigrationStatus.choices, default=IdentityOverridesV2MigrationStatus.NOT_STARTED, ) + stale_flags_limit_days = models.IntegerField( + default=30, + help_text="Number of days without modification in any environment before a flag is considered stale.", + ) objects = ProjectManager() diff --git a/api/projects/tags/migrations/0005_add_tag_fields_for_stale_flags_logic.py b/api/projects/tags/migrations/0005_add_tag_fields_for_stale_flags_logic.py new file mode 100644 index 000000000000..dde44ac75389 --- /dev/null +++ b/api/projects/tags/migrations/0005_add_tag_fields_for_stale_flags_logic.py @@ -0,0 +1,96 @@ +# Generated by Django 3.2.23 on 2023-12-06 15:11 +import re + +from django.apps.registry import Apps +from django.db import migrations, models +from django.db.backends.base.schema import BaseDatabaseSchemaEditor + +from projects.tags.models import TagType + +PROTECTED_LABELS = {"protected", "donotdelete", "permanent"} +LABEL_REGEX = re.compile(r"[ _]") + + +def get_sanitised_label(label: str) -> str: + return LABEL_REGEX.sub("", label.lower()) + + +def mark_existing_tags_as_permanent( + apps: Apps, schema_editor: BaseDatabaseSchemaEditor +) -> None: + """ + Update all tags in the database which the FE treats as 'permanent' through regex matching + to have the new is_permanent label. The FE will subsequently be updated to use the + is_permanent attribute instead of regex matching. + """ + tag_class = apps.get_model("tags", "tag") + + to_update = [] + + for tag in filter( + lambda t: get_sanitised_label(t.label) in PROTECTED_LABELS, + tag_class.objects.all(), + ): + tag.is_permanent = True + to_update.append(tag) + + tag_class.objects.bulk_update(to_update, fields=["is_permanent"]) + + +def populate_default_tag_type( + apps: Apps, schema_editor: BaseDatabaseSchemaEditor +) -> None: + apps.get_model("tags", "tag").objects.filter(type__isnull=True).update( + type=TagType.NONE.value + ) + + +class Migration(migrations.Migration): + dependencies = [ + ("tags", "0004_add_uuid_field"), + ] + + operations = [ + migrations.AddField( + model_name="tag", + name="is_permanent", + field=models.BooleanField( + default=False, + help_text="When applied to a feature, it means this feature should be excluded from stale flags logic.", + ), + ), + migrations.AddField( + model_name="tag", + name="is_system_tag", + field=models.BooleanField( + default=False, + help_text="Indicates that a tag was created by the system, not the user.", + ), + ), + migrations.RunPython( + mark_existing_tags_as_permanent, reverse_code=migrations.RunPython.noop + ), + migrations.AddField( + model_name="tag", + name="type", + field=models.CharField( + choices=[("NONE", "None"), ("STALE", "Stale")], + null=True, + help_text="Field used to provide a consistent identifier for the FE and API to use for business logic.", + max_length=100, + ), + ), + migrations.RunPython( + populate_default_tag_type, reverse_code=migrations.RunPython.noop + ), + migrations.AlterField( + model_name="tag", + name="type", + field=models.CharField( + choices=[("NONE", "None"), ("STALE", "Stale")], + default="NONE", + help_text="Field used to provide a consistent identifier for the FE and API to use for business logic.", + max_length=100, + ), + ), + ] diff --git a/api/projects/tags/models.py b/api/projects/tags/models.py index 46f430227fb1..597bebdd0d9a 100644 --- a/api/projects/tags/models.py +++ b/api/projects/tags/models.py @@ -4,6 +4,11 @@ from projects.models import Project +class TagType(models.Choices): + NONE = "NONE" + STALE = "STALE" + + class Tag(AbstractBaseExportableModel): label = models.CharField(max_length=100) color = models.CharField( @@ -12,6 +17,21 @@ class Tag(AbstractBaseExportableModel): description = models.CharField(max_length=512, blank=True, null=True) project = models.ForeignKey(Project, on_delete=models.CASCADE, related_name="tags") + is_system_tag = models.BooleanField( + default=False, + help_text="Indicates that a tag was created by the system, not the user.", + ) + is_permanent = models.BooleanField( + default=False, + help_text="When applied to a feature, it means this feature should be excluded from stale flags logic.", + ) + type = models.CharField( + default=TagType.NONE.value, + choices=TagType.choices, + help_text="Field used to provide a consistent identifier for the FE and API to use for business logic.", + max_length=100, + ) + class Meta: ordering = ("id",) # explicit ordering to prevent pagination warnings diff --git a/api/projects/tags/serializers.py b/api/projects/tags/serializers.py index 4026623b7276..cdebfd1f735c 100644 --- a/api/projects/tags/serializers.py +++ b/api/projects/tags/serializers.py @@ -6,5 +6,15 @@ class TagSerializer(serializers.ModelSerializer): class Meta: model = Tag - fields = ("id", "label", "color", "description", "project", "uuid") - read_only_fields = ("project",) + fields = ( + "id", + "label", + "color", + "description", + "project", + "uuid", + "is_permanent", + "is_system_tag", + "type", + ) + read_only_fields = ("project", "uuid", "is_system_tag", "type") diff --git a/api/tests/integration/features/featurestate/test_environment_featurestate_viewset.py b/api/tests/integration/features/featurestate/test_environment_featurestate_viewset.py index 138049033eaa..2e5b6408fbdb 100644 --- a/api/tests/integration/features/featurestate/test_environment_featurestate_viewset.py +++ b/api/tests/integration/features/featurestate/test_environment_featurestate_viewset.py @@ -34,4 +34,4 @@ def test_update_feature_state_value_updates_feature_state_value( # Then assert response.status_code == status.HTTP_200_OK - response.json()["feature_state_value"] == new_value + assert response.json()["feature_state_value"] == new_value diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index 751ffa8eeb5d..276a1f586012 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/api/tests/unit/features/test_unit_features_views.py @@ -10,6 +10,7 @@ from django.forms import model_to_dict from django.urls import reverse from django.utils import timezone +from freezegun import freeze_time from pytest_django import DjangoAssertNumQueries from pytest_lazyfixture import lazy_fixture from rest_framework import status @@ -55,6 +56,10 @@ # patch this function as it's triggering extra threads and causing errors mock.patch("features.signals.trigger_feature_state_change_webhooks").start() +now = timezone.now() +two_hours_ago = now - timedelta(hours=2) +one_hour_ago = now - timedelta(hours=1) + @pytest.mark.django_db class ProjectFeatureTestCase(TestCase): @@ -2554,3 +2559,66 @@ def test_simple_feature_state_returns_only_latest_versions( response_json = response.json() assert response_json["count"] == 2 + + +@pytest.mark.freeze_time(two_hours_ago) +def test_feature_list_last_modified_values( + staff_client: APIClient, + staff_user: FFAdminUser, + environment_v2_versioning: Environment, + project: Project, + feature: Feature, + with_project_permissions: WithProjectPermissionsCallable, + django_assert_num_queries: DjangoAssertNumQueries, +) -> None: + # Given + # another v2 versioning environment + environment_v2_versioning_2 = Environment.objects.create( + name="environment 2", project=project, use_v2_feature_versioning=True + ) + + url = "{base_url}?environment={environment_id}".format( + base_url=reverse("api-v1:projects:project-features-list", args=[project.id]), + environment_id=environment_v2_versioning.id, + ) + + with_project_permissions([VIEW_PROJECT]) + + with freeze_time(one_hour_ago): + # create a new published version in another environment, simulated to be one hour ago + environment_v2_versioning_2_version_2 = ( + EnvironmentFeatureVersion.objects.create( + environment=environment_v2_versioning_2, feature=feature + ) + ) + environment_v2_versioning_2_version_2.publish(staff_user) + + with freeze_time(now): + # and create a new unpublished version in the current environment, simulated to be now + # this shouldn't affect the values returned + EnvironmentFeatureVersion.objects.create( + environment=environment_v2_versioning, feature=feature + ) + + # let's add a few more features to ensure we aren't adding N+1 issues + for i in range(2): + Feature.objects.create(name=f"feature_{i}", project=project) + + # When + with django_assert_num_queries(14): # TODO: reduce this number of queries! + response = staff_client.get(url) + + # Then + assert response.status_code == status.HTTP_200_OK + + response_json = response.json() + assert response_json["count"] == 3 + + feature_data = next( + filter(lambda r: r["id"] == feature.id, response_json["results"]) + ) + assert feature_data["last_modified_in_any_environment"] == one_hour_ago.isoformat() + assert ( + feature_data["last_modified_in_current_environment"] + == two_hours_ago.isoformat() + )