Skip to content

Commit

Permalink
feat(feature flags): check linked instances of an existing flag don't…
Browse files Browse the repository at this point in the history
… get overwritten (#18508)

---------

Co-authored-by: Neil Kakkar <[email protected]>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
  • Loading branch information
3 people authored Nov 9, 2023
1 parent 30df1e7 commit 982f7a3
Show file tree
Hide file tree
Showing 6 changed files with 140 additions and 9 deletions.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,9 @@ export function FeatureFlagReleaseConditions({
return <></>
}

// TODO: EarlyAccessFeatureType is not the correct type for featureFlag.features, hence bypassing TS check
const hasMatchingEarlyAccessFeature = featureFlag.features?.find((f: any) => f.flagKey === featureFlag.key)

return (
<Col span={24} md={24} key={`${index}-${filterGroups.length}`}>
{index > 0 && <div className="condition-set-separator">OR</div>}
Expand Down Expand Up @@ -365,6 +368,10 @@ export function FeatureFlagReleaseConditions({
<div className="flex items-center justify-between">
<div />
<LemonButton
disabledReason={
!hasMatchingEarlyAccessFeature &&
'The matching Early Access Feature was not found. You can create it in the Early Access Management tab.'
}
aria-label="more"
data-attr={'feature-flag-feature-list-button'}
status="primary"
Expand All @@ -375,7 +382,7 @@ export function FeatureFlagReleaseConditions({
router.actions.push(urls.earlyAccessFeature(featureFlag.features[0].id))
}
>
View Early Access Feature
{hasMatchingEarlyAccessFeature ? 'View Early Access Feature' : 'No Early Access Feature'}
</LemonButton>
</div>
</div>
Expand Down
4 changes: 3 additions & 1 deletion posthog/api/organization_feature_flag.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,9 @@ def copy_flags(self, request, *args, **kwargs):
"deleted": False,
}

existing_flag = FeatureFlag.objects.filter(key=feature_flag_key, team_id=target_project_id).first()
existing_flag = FeatureFlag.objects.filter(
key=feature_flag_key, team_id=target_project_id, deleted=False
).first()
# Update existing flag
if existing_flag:
feature_flag_serializer = FeatureFlagSerializer(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1203,7 +1203,8 @@
"posthog_featureflag"."usage_dashboard_id",
"posthog_featureflag"."has_enriched_analytics"
FROM "posthog_featureflag"
WHERE ("posthog_featureflag"."key" = 'copied-flag-key'
WHERE (NOT "posthog_featureflag"."deleted"
AND "posthog_featureflag"."key" = 'copied-flag-key'
AND "posthog_featureflag"."team_id" = 2)
ORDER BY "posthog_featureflag"."id" ASC
LIMIT 1 /*controller='organization_feature_flags-copy-flags',route='api/organizations/%28%3FP%3Cparent_lookup_organization_id%3E%5B%5E/.%5D%2B%29/feature_flags/copy_flags/%3F%24'*/
Expand Down
133 changes: 127 additions & 6 deletions posthog/api/test/test_organization_feature_flag.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
from rest_framework import status
from posthog.models.team.team import Team
from posthog.models import FeatureFlag
from posthog.models.experiment import Experiment
from posthog.models.feedback.survey import Survey
from posthog.models.early_access_feature import EarlyAccessFeature
from posthog.api.dashboards.dashboard import Dashboard
from posthog.test.base import APIBaseTest, QueryMatchingTest, snapshot_postgres_queries
from typing import Any, Dict

Expand Down Expand Up @@ -130,7 +134,8 @@ def test_copy_feature_flag_create_new(self):
def test_copy_feature_flag_update_existing(self):
target_project = self.team_2
rollout_percentage_existing = 99
self.feature_flag_existing = FeatureFlag.objects.create(

existing_flag = FeatureFlag.objects.create(
team=target_project,
created_by=self.user,
key=self.feature_flag_key,
Expand All @@ -140,6 +145,25 @@ def test_copy_feature_flag_update_existing(self):
ensure_experience_continuity=False,
)

# The following instances must remain linked to the existing flag after overwriting it
experiment = Experiment.objects.create(team=self.team_2, created_by=self.user, feature_flag_id=existing_flag.id)
survey = Survey.objects.create(team=self.team, created_by=self.user, linked_flag=existing_flag)
feature = EarlyAccessFeature.objects.create(
team=self.team,
feature_flag=existing_flag,
)
analytics_dashboard = Dashboard.objects.create(
team=self.team,
created_by=self.user,
)
existing_flag.analytics_dashboards.set([analytics_dashboard])
usage_dashboard = Dashboard.objects.create(
team=self.team,
created_by=self.user,
)
existing_flag.usage_dashboard = usage_dashboard
existing_flag.save()

url = f"/api/organizations/{self.organization.id}/feature_flags/copy_flags"

data = {
Expand All @@ -163,21 +187,112 @@ def test_copy_feature_flag_update_existing(self):
"rollout_percentage": self.rollout_percentage_to_copy,
"deleted": False,
"created_by": self.user.id,
"is_simple_flag": True,
"rollback_conditions": None,
"performed_rollback": False,
"can_edit": True,
"has_enriched_analytics": False,
"tags": [],
"id": "__ignore__",
"created_at": "__ignore__",
"usage_dashboard": "__ignore__",
"experiment_set": "__ignore__",
"surveys": "__ignore__",
"features": "__ignore__",
"analytics_dashboards": "__ignore__",
}

flag_response = response.json()["success"][0]

for key, expected_value in expected_flag_response.items():
self.assertIn(key, flag_response)
if expected_value != "__ignore__":
if key == "created_by":
self.assertEqual(flag_response[key]["id"], expected_value)
else:
self.assertEqual(flag_response[key], expected_value)

# Linked instances must remain linked
self.assertEqual(experiment.id, flag_response["experiment_set"][0])
self.assertEqual(str(survey.id), flag_response["surveys"][0]["id"])
self.assertEqual(str(feature.id), flag_response["features"][0]["id"])
self.assertEqual(analytics_dashboard.id, flag_response["analytics_dashboards"][0])
self.assertEqual(usage_dashboard.id, flag_response["usage_dashboard"])

self.assertSetEqual(
set(expected_flag_response.keys()),
set(flag_response.keys()),
)

def test_copy_feature_flag_update_override_deleted(self):
target_project = self.team_2
rollout_percentage_existing = 99

existing_deleted_flag = FeatureFlag.objects.create(
team=target_project,
created_by=self.user,
key=self.feature_flag_key,
name="Existing flag",
filters={"groups": [{"rollout_percentage": rollout_percentage_existing}]},
rollout_percentage=rollout_percentage_existing,
ensure_experience_continuity=False,
deleted=True,
)

# The following instances must be overriden for a soft-deleted flag
Experiment.objects.create(team=self.team_2, created_by=self.user, feature_flag_id=existing_deleted_flag.id)
Survey.objects.create(team=self.team, created_by=self.user, linked_flag=existing_deleted_flag)

analytics_dashboard = Dashboard.objects.create(
team=self.team,
created_by=self.user,
)
existing_deleted_flag.analytics_dashboards.set([analytics_dashboard])
usage_dashboard = Dashboard.objects.create(
team=self.team,
created_by=self.user,
)

existing_deleted_flag.usage_dashboard = usage_dashboard
existing_deleted_flag.save()

url = f"/api/organizations/{self.organization.id}/feature_flags/copy_flags"

data = {
"feature_flag_key": self.feature_flag_to_copy.key,
"from_project": self.feature_flag_to_copy.team_id,
"target_project_ids": [target_project.id],
}
response = self.client.post(url, data)

self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertIn("success", response.json())
self.assertIn("failed", response.json())

# Check copied flag in the response
expected_flag_response = {
"key": self.feature_flag_to_copy.key,
"name": self.feature_flag_to_copy.name,
"filters": self.feature_flag_to_copy.filters,
"active": self.feature_flag_to_copy.active,
"ensure_experience_continuity": self.feature_flag_to_copy.ensure_experience_continuity,
"rollout_percentage": self.rollout_percentage_to_copy,
"deleted": False,
"created_by": self.user.id,
"is_simple_flag": True,
"experiment_set": [],
"surveys": [],
"features": [],
"rollback_conditions": None,
"performed_rollback": False,
"can_edit": True,
"analytics_dashboards": [],
"has_enriched_analytics": False,
"tags": [],
"id": "__ignore__",
"created_at": "__ignore__",
"usage_dashboard": "__ignore__",
"experiment_set": "__ignore__",
"surveys": "__ignore__",
"features": "__ignore__",
"analytics_dashboards": "__ignore__",
}

flag_response = response.json()["success"][0]

for key, expected_value in expected_flag_response.items():
Expand All @@ -188,6 +303,12 @@ def test_copy_feature_flag_update_existing(self):
else:
self.assertEqual(flag_response[key], expected_value)

# Linked instances must be overriden for a soft-deleted flag
self.assertEqual(flag_response["experiment_set"], [])
self.assertEqual(flag_response["surveys"], [])
self.assertNotEqual(flag_response["usage_dashboard"], existing_deleted_flag.usage_dashboard.id)
self.assertEqual(flag_response["analytics_dashboards"], [])

self.assertSetEqual(
set(expected_flag_response.keys()),
set(flag_response.keys()),
Expand Down

0 comments on commit 982f7a3

Please sign in to comment.