From e3cb9cd05c5fd7785c3cca79198258499862d233 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Fri, 10 Nov 2023 13:56:00 +0000 Subject: [PATCH] fix(experiments): Restrict deleting linked flag (#18543) --- latest_migrations.manifest | 2 +- posthog/api/feature_flag.py | 11 ++++++-- .../test/test_organization_feature_flag.py | 27 +++++++++++++++++-- .../0362_alter_experiment_feature_flag.py | 18 +++++++++++++ posthog/models/experiment.py | 2 +- 5 files changed, 54 insertions(+), 6 deletions(-) create mode 100644 posthog/migrations/0362_alter_experiment_feature_flag.py diff --git a/latest_migrations.manifest b/latest_migrations.manifest index dcc40eb8e7ed1..92d33d07ec998 100644 --- a/latest_migrations.manifest +++ b/latest_migrations.manifest @@ -5,7 +5,7 @@ contenttypes: 0002_remove_content_type_name ee: 0015_add_verified_properties otp_static: 0002_throttling otp_totp: 0002_auto_20190420_0723 -posthog: 0361_add_plugin_config_ui_fields +posthog: 0362_alter_experiment_feature_flag sessions: 0001_initial social_django: 0010_uid_db_index two_factor: 0007_auto_20201201_1019 diff --git a/posthog/api/feature_flag.py b/posthog/api/feature_flag.py index 5123b8010a0b0..5d1654259eb36 100644 --- a/posthog/api/feature_flag.py +++ b/posthog/api/feature_flag.py @@ -1,7 +1,7 @@ import json from typing import Any, Dict, List, Optional, cast -from django.db.models import QuerySet, Q +from django.db.models import QuerySet, Q, deletion from django.conf import settings from rest_framework import ( authentication, @@ -256,7 +256,14 @@ def create(self, validated_data: Dict, *args: Any, **kwargs: Any) -> FeatureFlag "Invalid variant definitions: Variant rollout percentages must sum to 100." ) - FeatureFlag.objects.filter(key=validated_data["key"], team_id=self.context["team_id"], deleted=True).delete() + try: + FeatureFlag.objects.filter( + key=validated_data["key"], team_id=self.context["team_id"], deleted=True + ).delete() + except deletion.RestrictedError: + raise exceptions.ValidationError( + "Feature flag with this key already exists and is used in an experiment. Please delete the experiment before deleting the flag." + ) instance: FeatureFlag = super().create(validated_data) self._attempt_set_tags(tags, instance) diff --git a/posthog/api/test/test_organization_feature_flag.py b/posthog/api/test/test_organization_feature_flag.py index 285f25f5a3149..a12c147fa29ee 100644 --- a/posthog/api/test/test_organization_feature_flag.py +++ b/posthog/api/test/test_organization_feature_flag.py @@ -226,6 +226,7 @@ def test_copy_feature_flag_update_existing(self): def test_copy_feature_flag_update_override_deleted(self): target_project = self.team_2 + target_project_2 = Team.objects.create(organization=self.organization) rollout_percentage_existing = 99 existing_deleted_flag = FeatureFlag.objects.create( @@ -238,9 +239,18 @@ def test_copy_feature_flag_update_override_deleted(self): ensure_experience_continuity=False, deleted=True, ) + existing_deleted_flag2 = FeatureFlag.objects.create( + team=target_project_2, + 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( @@ -256,12 +266,17 @@ def test_copy_feature_flag_update_override_deleted(self): existing_deleted_flag.usage_dashboard = usage_dashboard existing_deleted_flag.save() + # Experiments restrict deleting soft-deleted flags + Experiment.objects.create( + team=target_project_2, created_by=self.user, feature_flag_id=existing_deleted_flag2.id + ) + 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], + "target_project_ids": [target_project.id, target_project_2.id], } response = self.client.post(url, data) @@ -314,6 +329,14 @@ def test_copy_feature_flag_update_override_deleted(self): set(flag_response.keys()), ) + # target_project_2 should have failed + self.assertEqual(len(response.json()["failed"]), 1) + self.assertEqual(response.json()["failed"][0]["project_id"], target_project_2.id) + self.assertEqual( + response.json()["failed"][0]["errors"], + "[ErrorDetail(string='Feature flag with this key already exists and is used in an experiment. Please delete the experiment before deleting the flag.', code='invalid')]", + ) + def test_copy_feature_flag_missing_fields(self): url = f"/api/organizations/{self.organization.id}/feature_flags/copy_flags" data: Dict[str, Any] = {} diff --git a/posthog/migrations/0362_alter_experiment_feature_flag.py b/posthog/migrations/0362_alter_experiment_feature_flag.py new file mode 100644 index 0000000000000..ac6eda940898e --- /dev/null +++ b/posthog/migrations/0362_alter_experiment_feature_flag.py @@ -0,0 +1,18 @@ +# Generated by Django 3.2.19 on 2023-11-10 11:44 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + dependencies = [ + ("posthog", "0361_add_plugin_config_ui_fields"), + ] + + operations = [ + migrations.AlterField( + model_name="experiment", + name="feature_flag", + field=models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, to="posthog.featureflag"), + ), + ] diff --git a/posthog/models/experiment.py b/posthog/models/experiment.py index 94a17778df27a..ea970c5b2db12 100644 --- a/posthog/models/experiment.py +++ b/posthog/models/experiment.py @@ -23,7 +23,7 @@ class Experiment(models.Model): # A list of filters for secondary metrics secondary_metrics: models.JSONField = models.JSONField(default=list, null=True) - feature_flag: models.ForeignKey = models.ForeignKey("FeatureFlag", blank=False, on_delete=models.CASCADE) + feature_flag: models.ForeignKey = models.ForeignKey("FeatureFlag", blank=False, on_delete=models.RESTRICT) created_by: models.ForeignKey = models.ForeignKey("User", on_delete=models.CASCADE) start_date: models.DateTimeField = models.DateTimeField(null=True) end_date: models.DateTimeField = models.DateTimeField(null=True)