fix(flags): Alter constraints to not delete flags and experiments (#1…

…8514)

ee/api/feature_flag_role_access.py

@@ -35,7 +35,11 @@ def has_permission(self, request, view):
             return True
         try:
             feature_flag: FeatureFlag = FeatureFlag.objects.get(id=view.parents_query_dict["feature_flag_id"])
-            if feature_flag.created_by.uuid == request.user.uuid:
+            if (
+                hasattr(feature_flag, "created_by")
+                and feature_flag.created_by
+                and feature_flag.created_by.uuid == request.user.uuid
+            ):
                 return True
         except FeatureFlag.DoesNotExist:
             raise exceptions.NotFound("Feature flag not found.")

ee/api/test/test_feature_flag_role_access.py

@@ -37,6 +37,26 @@ def test_can_always_add_role_access_if_creator_of_feature_flag(self):
         self.assertEqual(flag_role.role.name, self.eng_role.name)
         self.assertEqual(flag_role.feature_flag.id, self.feature_flag.id)
 
+    def test_role_access_with_deleted_creator_of_feature_flag(self):
+        OrganizationResourceAccess.objects.create(
+            resource=OrganizationResourceAccess.Resources.FEATURE_FLAGS,
+            access_level=OrganizationResourceAccess.AccessLevel.CAN_ONLY_VIEW,
+            organization=self.organization,
+        )
+
+        flag = FeatureFlag.objects.create(
+            created_by=None,
+            team=self.team,
+            key="flag_role_access_none",
+            name="Flag role access",
+        )
+        self.assertEqual(self.user.role_memberships.count(), 0)
+        flag_role_access_create_res = self.client.post(
+            f"/api/projects/@current/feature_flags/{flag.id}/role_access",
+            {"role_id": self.eng_role.id},
+        )
+        self.assertEqual(flag_role_access_create_res.status_code, status.HTTP_403_FORBIDDEN)
+
     def test_cannot_add_role_access_if_feature_flags_access_level_too_low_and_not_creator(self):
         OrganizationResourceAccess.objects.create(
             resource=OrganizationResourceAccess.Resources.FEATURE_FLAGS,

ee/clickhouse/models/test/__snapshots__/test_property.ambr

@@ -57,7 +57,7 @@
 ---
 # name: test_parse_groups_persons_edge_case_with_single_filter
   <class 'tuple'> (
-    'AND (  has(%(vglobalperson_0)s, "pmat_email"))',
+    'AND (  has(%(vglobalperson_0)s, replaceRegexpAll(JSONExtractRaw(person_props, %(kglobalperson_0)s), \'^"|"$\', \'\')))',
     <class 'dict'> {
       'kglobalperson_0': 'email',
       'vglobalperson_0': <class 'list'> [

ee/clickhouse/views/test/__snapshots__/test_clickhouse_experiment_secondary_results.ambr

@@ -1,6 +1,6 @@
 # name: ClickhouseTestExperimentSecondaryResults.test_basic_secondary_metric_results
   '
-  /* user_id:132 celery:posthog.celery.sync_insight_caching_state */
+  /* user_id:133 celery:posthog.celery.sync_insight_caching_state */
   SELECT team_id,
          date_diff('second', max(timestamp), now()) AS age
   FROM events

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: 0364_team_external_data_workspace_rows
+posthog: 0365_update_created_by_flag_constraint
 sessions: 0001_initial
 social_django: 0010_uid_db_index
 two_factor: 0007_auto_20201201_1019

posthog/api/organization_feature_flag.py

@@ -12,6 +12,7 @@
 from posthog.api.routing import StructuredViewSetMixin
 from posthog.api.feature_flag import FeatureFlagSerializer
 from posthog.api.feature_flag import CanEditFeatureFlag
+from posthog.api.shared import UserBasicSerializer
 from posthog.models import FeatureFlag, Team
 from posthog.models.cohort import Cohort
 from posthog.models.filters.filter import Filter
@@ -44,15 +45,10 @@ def retrieve(self, request, *args, **kwargs):
             {
                 "flag_id": flag.id,
                 "team_id": flag.team_id,
-                "created_by": {
-                    "id": flag.created_by.id,
-                    "uuid": flag.created_by.uuid,
-                    "distinct_id": flag.created_by.distinct_id,
-                    "first_name": flag.created_by.first_name,
-                    "email": flag.created_by.email,
-                    "is_email_verified": flag.created_by.is_email_verified,
-                },
-                "filters": flag.filters,
+                "created_by": UserBasicSerializer(flag.created_by).data
+                if hasattr(flag, "created_by") and flag.created_by
+                else None,
+                "filters": flag.get_filters(),
                 "created_at": flag.created_at,
                 "active": flag.active,
             }
@@ -168,7 +164,7 @@ def copy_flags(self, request, *args, **kwargs):
             flag_data = {
                 "key": flag_to_copy.key,
                 "name": flag_to_copy.name,
-                "filters": flag_to_copy.filters,
+                "filters": flag_to_copy.get_filters(),
                 "active": flag_to_copy.active,
                 "rollout_percentage": flag_to_copy.rollout_percentage,
                 "ensure_experience_continuity": flag_to_copy.ensure_experience_continuity,

posthog/api/test/__snapshots__/test_organization_feature_flag.ambr

@@ -1440,8 +1440,8 @@
          "posthog_experiment"."filters",
          "posthog_experiment"."parameters",
          "posthog_experiment"."secondary_metrics",
-         "posthog_experiment"."feature_flag_id",
          "posthog_experiment"."created_by_id",
+         "posthog_experiment"."feature_flag_id",
          "posthog_experiment"."start_date",
          "posthog_experiment"."end_date",
          "posthog_experiment"."created_at",

posthog/api/test/test_feature_flag.py

@@ -1171,6 +1171,34 @@ def test_getting_flags_is_not_nplus1(self) -> None:
             response = self.client.get(f"/api/projects/{self.team.id}/feature_flags")
             self.assertEqual(response.status_code, status.HTTP_200_OK)
 
+    def test_getting_flags_with_no_creator(self) -> None:
+        FeatureFlag.objects.all().delete()
+
+        self.client.post(
+            f"/api/projects/{self.team.id}/feature_flags/",
+            data={
+                "name": f"flag",
+                "key": f"flag_0",
+                "filters": {"groups": [{"rollout_percentage": 5}]},
+            },
+            format="json",
+        ).json()
+
+        FeatureFlag.objects.create(
+            created_by=None,
+            team=self.team,
+            key="flag_role_access",
+            name="Flag role access",
+        )
+
+        with self.assertNumQueries(FuzzyInt(11, 12)):
+            response = self.client.get(f"/api/projects/{self.team.id}/feature_flags")
+            self.assertEqual(response.status_code, status.HTTP_200_OK)
+            self.assertEqual(len(response.json()["results"]), 2)
+            sorted_results = sorted(response.json()["results"], key=lambda x: x["key"])
+            self.assertEqual(sorted_results[1]["created_by"], None)
+            self.assertEqual(sorted_results[1]["key"], "flag_role_access")
+
     @patch("posthog.api.feature_flag.report_user_action")
     def test_my_flags(self, mock_capture):
         self.client.post(

posthog/api/test/test_organization_feature_flag.py

@@ -53,7 +53,7 @@ def test_get_feature_flag_success(self):
                     "email": self.user.email,
                     "is_email_verified": self.user.is_email_verified,
                 },
-                "filters": flag.filters,
+                "filters": flag.get_filters(),
                 "created_at": flag.created_at.strftime("%Y-%m-%dT%H:%M:%S.%f") + "Z",
                 "active": flag.active,
             }
@@ -243,6 +243,29 @@ def test_copy_feature_flag_update_existing(self):
             set(flag_response.keys()),
         )
 
+    def test_copy_feature_flag_with_old_legacy_flags(self):
+        url = f"/api/organizations/{self.organization.id}/feature_flags/copy_flags"
+        target_project = self.team_2
+
+        flag_to_copy = FeatureFlag.objects.create(
+            team=self.team_1,
+            created_by=self.user,
+            key="flag-to-copy-here",
+            filters={},
+            rollout_percentage=self.rollout_percentage_to_copy,
+        )
+
+        data = {
+            "feature_flag_key": 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.assertEqual(len(response.json()["success"]), 1)
+        self.assertEqual(len(response.json()["failed"]), 0)
+
     def test_copy_feature_flag_update_override_deleted(self):
         target_project = self.team_2
         target_project_2 = Team.objects.create(organization=self.organization)

posthog/migrations/0365_update_created_by_flag_constraint.py

@@ -0,0 +1,85 @@
+# Generated by Django 3.2.19 on 2023-11-09 10:35
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("posthog", "0364_team_external_data_workspace_rows"),
+    ]
+
+    # :TRICKY:
+    # We are replacing the original generated migration:
+    # migrations.AlterField(
+    #     model_name='experiment',
+    #     name='created_by',
+    #     field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL),
+    # ),
+    # migrations.AlterField(
+    #     model_name='featureflag',
+    #     name='created_by',
+    #     field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL),
+    # ),
+    # with one that adds the 'NOT VALID' directive, which applies the constraint only for inserts/updates.
+    # This ensures the table is not locked when creating the new constraint.
+    # A follow up migration will validate the constraint.
+    # The code here is exactly the same as the one generated by the default migration, except for the 'NOT VALID' directive.
+
+    operations = [
+        # make the created_by column nullable in experiments & flags
+        migrations.SeparateDatabaseAndState(
+            state_operations=[
+                migrations.AlterField(
+                    model_name="experiment",
+                    name="created_by",
+                    field=models.ForeignKey(
+                        null=True,
+                        on_delete=django.db.models.deletion.SET_NULL,
+                        to="posthog.user",
+                    ),
+                ),
+                migrations.AlterField(
+                    model_name="featureflag",
+                    name="created_by",
+                    field=models.ForeignKey(
+                        null=True,
+                        on_delete=django.db.models.deletion.SET_NULL,
+                        to="posthog.user",
+                    ),
+                ),
+            ],
+            database_operations=[
+                # We add -- existing-table-constraint-ignore to ignore the constraint validation in CI.
+                # This should be safe, because we are making the constraint NOT VALID, so doesn't lock things up for long.
+                migrations.RunSQL(
+                    """
+                    SET CONSTRAINTS "posthog_experiment_created_by_id_b40aea95_fk_posthog_user_id" IMMEDIATE; -- existing-table-constraint-ignore
+                    ALTER TABLE "posthog_experiment" DROP CONSTRAINT "posthog_experiment_created_by_id_b40aea95_fk_posthog_user_id"; -- existing-table-constraint-ignore
+                    ALTER TABLE "posthog_experiment" ALTER COLUMN "created_by_id" DROP NOT NULL;
+                    ALTER TABLE "posthog_experiment" ADD CONSTRAINT "posthog_experiment_created_by_id_b40aea95_fk_posthog_user_id" FOREIGN KEY ("created_by_id") REFERENCES "posthog_user" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID; -- existing-table-constraint-ignore
+                    """,
+                    reverse_sql="""
+                        SET CONSTRAINTS "posthog_experiment_created_by_id_b40aea95_fk_posthog_user_id" IMMEDIATE;
+                        ALTER TABLE "posthog_experiment" DROP CONSTRAINT "posthog_experiment_created_by_id_b40aea95_fk_posthog_user_id";
+                        ALTER TABLE "posthog_experiment" ALTER COLUMN "created_by_id" SET NOT NULL;
+                        ALTER TABLE "posthog_experiment" ADD CONSTRAINT "posthog_experiment_created_by_id_b40aea95_fk_posthog_user_id" FOREIGN KEY ("created_by_id") REFERENCES "posthog_user" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
+                    """,
+                ),
+                migrations.RunSQL(
+                    """SET CONSTRAINTS "posthog_featureflag_created_by_id_4571fe1a_fk_posthog_user_id" IMMEDIATE; -- existing-table-constraint-ignore
+                    ALTER TABLE "posthog_featureflag" DROP CONSTRAINT "posthog_featureflag_created_by_id_4571fe1a_fk_posthog_user_id"; -- existing-table-constraint-ignore
+                    ALTER TABLE "posthog_featureflag" ALTER COLUMN "created_by_id" DROP NOT NULL;
+                    ALTER TABLE "posthog_featureflag" ADD CONSTRAINT "posthog_featureflag_created_by_id_4571fe1a_fk_posthog_user_id" FOREIGN KEY ("created_by_id") REFERENCES "posthog_user" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID; -- existing-table-constraint-ignore
+                    """,
+                    reverse_sql="""
+                        SET CONSTRAINTS "posthog_featureflag_created_by_id_4571fe1a_fk_posthog_user_id" IMMEDIATE;
+                        ALTER TABLE "posthog_featureflag" DROP CONSTRAINT "posthog_featureflag_created_by_id_4571fe1a_fk_posthog_user_id";
+                        ALTER TABLE "posthog_featureflag" ALTER COLUMN "created_by_id" SET NOT NULL;
+                        ALTER TABLE "posthog_featureflag" ADD CONSTRAINT "posthog_featureflag_created_by_id_4571fe1a_fk_posthog_user_id" FOREIGN KEY ("created_by_id") REFERENCES "posthog_user" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
+                        -- existing-table-constraint-ignore
+                    """,
+                ),
+            ],
+        ),
+    ]

posthog/models/experiment.py

@@ -23,8 +23,8 @@ class Experiment(models.Model):
     # A list of filters for secondary metrics
     secondary_metrics: models.JSONField = models.JSONField(default=list, null=True)
 
+    created_by: models.ForeignKey = models.ForeignKey("User", on_delete=models.SET_NULL, null=True)
     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)
     created_at: models.DateTimeField = models.DateTimeField(default=timezone.now)

posthog/models/feature_flag/feature_flag.py

@@ -37,7 +37,7 @@ class Meta:
     rollout_percentage: models.IntegerField = models.IntegerField(null=True, blank=True)
 
     team: models.ForeignKey = models.ForeignKey("Team", on_delete=models.CASCADE)
-    created_by: models.ForeignKey = models.ForeignKey("User", on_delete=models.CASCADE)
+    created_by: models.ForeignKey = models.ForeignKey("User", on_delete=models.SET_NULL, null=True)
     created_at: models.DateTimeField = models.DateTimeField(default=timezone.now)
     deleted: models.BooleanField = models.BooleanField(default=False)
     active: models.BooleanField = models.BooleanField(default=True)

posthog/models/feature_flag/permissions.py

@@ -12,7 +12,7 @@ def can_user_edit_feature_flag(request, feature_flag):
     else:
         if not request.user.organization.is_feature_available(AvailableFeature.ROLE_BASED_ACCESS):
             return True
-        if feature_flag.created_by == request.user:
+        if hasattr(feature_flag, "created_by") and feature_flag.created_by and feature_flag.created_by == request.user:
             return True
         if (
             request.user.organization_memberships.get(organization=request.user.organization).level