Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add fields necessary for stale flags #3263

Merged
merged 15 commits into from
Feb 14, 2024
Merged
25 changes: 25 additions & 0 deletions api/features/serializers.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import typing
from datetime import datetime

import django.core.exceptions
from drf_writable_nested import WritableNestedModelSerializer
Expand Down Expand Up @@ -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 = (
Expand All @@ -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")

Expand Down Expand Up @@ -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"""
Expand Down
27 changes: 24 additions & 3 deletions api/features/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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"],
Expand Down
Original file line number Diff line number Diff line change
@@ -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.",
),
),
]
4 changes: 4 additions & 0 deletions api/projects/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,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()

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
# 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


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"])


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(
default=None,
null=True,
choices=(("STALE", "Stale"),),
help_text="Field used to provide a consistent identifier for the FE and API to use for business logic.",
max_length=100,
)
),
]
20 changes: 20 additions & 0 deletions api/projects/tags/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@
from projects.models import Project


class TagType(models.Choices):
STALE = "STALE"


class Tag(AbstractBaseExportableModel):
label = models.CharField(max_length=100)
color = models.CharField(
Expand All @@ -12,6 +16,22 @@ 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=None,
null=True,
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

Expand Down
14 changes: 12 additions & 2 deletions api/projects/tags/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Original file line number Diff line number Diff line change
Expand Up @@ -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
68 changes: 68 additions & 0 deletions api/tests/unit/features/test_unit_features_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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()
)