From 6b104d6f4595c4aec73b08a477494e7710d89c8b Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Tue, 7 Jan 2025 11:22:24 -0800 Subject: [PATCH 1/7] Fix: Update feature flag insights when key changes This only updates the insights that the PostHog system creates when a feature flag creates. If a user adds another insight to the feature flag dashboard, it's on them to keep it updated. If the user changes the name of a system created insight, then we will not update it. We don't want to stomp on user changes. Fixes #24938 --- posthog/api/feature_flag.py | 15 ++++++++ posthog/helpers/dashboard_templates.py | 47 ++++++++++++++++++++++++-- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/posthog/api/feature_flag.py b/posthog/api/feature_flag.py index f20c1a4a6105a..e07dd845a61b2 100644 --- a/posthog/api/feature_flag.py +++ b/posthog/api/feature_flag.py @@ -385,6 +385,7 @@ def update(self, instance: FeatureFlag, validated_data: dict, *args: Any, **kwar request = self.context["request"] validated_key = validated_data.get("key", None) if validated_key: + # Delete any soft deleted feature flags with the same key to prevent conflicts FeatureFlag.objects.filter( key=validated_key, team__project_id=instance.team.project_id, deleted=True ).delete() @@ -396,6 +397,8 @@ def update(self, instance: FeatureFlag, validated_data: dict, *args: Any, **kwar for dashboard in analytics_dashboards: FeatureFlagDashboards.objects.get_or_create(dashboard=dashboard, feature_flag=instance) + old_key = instance.key + instance = super().update(instance, validated_data) # Propagate the new variants and aggregation group type index to the linked experiments @@ -415,6 +418,9 @@ def update(self, instance: FeatureFlag, validated_data: dict, *args: Any, **kwar experiment.parameters.pop("aggregation_group_type_index", None) experiment.save() + if old_key != instance.key: + _update_feature_flag_dashboard(instance, old_key) + report_user_action(request.user, "feature flag updated", instance.get_analytics_metadata()) return instance @@ -446,6 +452,15 @@ def _create_usage_dashboard(feature_flag: FeatureFlag, user): return usage_dashboard +def _update_feature_flag_dashboard(feature_flag: FeatureFlag, old_key: str) -> None: + from posthog.helpers.dashboard_templates import update_feature_flag_dashboard + + if not old_key or not feature_flag.usage_dashboard: + return + + update_feature_flag_dashboard(feature_flag, old_key) + + class MinimalFeatureFlagSerializer(serializers.ModelSerializer): filters = serializers.DictField(source="get_filters", required=False) diff --git a/posthog/helpers/dashboard_templates.py b/posthog/helpers/dashboard_templates.py index 313f8c6722a2a..668201a44892f 100644 --- a/posthog/helpers/dashboard_templates.py +++ b/posthog/helpers/dashboard_templates.py @@ -572,7 +572,7 @@ def create_feature_flag_dashboard(feature_flag, dashboard: Dashboard) -> None: _create_tile_for_insight( dashboard, name="Feature Flag Called Total Volume", - description="Shows the number of total calls made on feature flag with key: " + feature_flag.key, + description=_get_feature_flag_total_volume_insight_description(feature_flag.key), query={ "kind": "InsightVizNode", "source": { @@ -628,8 +628,7 @@ def create_feature_flag_dashboard(feature_flag, dashboard: Dashboard) -> None: _create_tile_for_insight( dashboard, name="Feature Flag calls made by unique users per variant", - description="Shows the number of unique user calls made on feature flag per variant with key: " - + feature_flag.key, + description=_get_feature_flag_unique_users_insight_description(feature_flag.key), query={ "kind": "InsightVizNode", "source": { @@ -690,6 +689,48 @@ def create_feature_flag_dashboard(feature_flag, dashboard: Dashboard) -> None: ) +def _get_feature_flag_total_volume_insight_description(feature_flag_key: str) -> str: + return f"Shows the number of total calls made on feature flag with key: {feature_flag_key}" + + +def _get_feature_flag_unique_users_insight_description(feature_flag_key: str) -> str: + return f"Shows the number of unique user calls made on feature flag per variant with key: {feature_flag_key}" + + +def update_feature_flag_dashboard(feature_flag, old_key: str) -> None: + # We need to update the *system* created insights with the new key, so we search for them by name + dashboard = feature_flag.usage_dashboard + total_volume_insight = dashboard.insights.get(name="Feature Flag Called Total Volume") + if total_volume_insight: + _update_tile_with_new_key( + total_volume_insight, + feature_flag.key, + old_key, + "Shows the number of total calls made on feature flag with key:", + ) + + unique_users_insight = dashboard.insights.get(name="Feature Flag calls made by unique users per variant") + if unique_users_insight: + _update_tile_with_new_key( + unique_users_insight, + feature_flag.key, + old_key, + "Shows the number of unique user calls made on feature flag per variant with key:", + ) + + +def _update_tile_with_new_key(insight, new_key: str, old_key: str, description_prefix: str) -> None: + insight.description = f"{description_prefix} {new_key}" + if insight.query: + if insight.query.get("source", {}).get("properties", {}).get("values"): + for property_group in insight.query["source"]["properties"]["values"]: + for property_value in property_group.get("values", []): + if property_value.get("key") == "$feature_flag" and property_value.get("value") == old_key: + property_value["value"] = new_key + insight.query = insight.query # Trigger field update + insight.save() + + def add_enriched_insights_to_feature_flag_dashboard(feature_flag, dashboard: Dashboard) -> None: # 1 row _create_tile_for_insight( From 88a13c74bbed3f1c31dc8c23d0c55542a84abff4 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Tue, 7 Jan 2025 11:31:10 -0800 Subject: [PATCH 2/7] Ensure description is defined in a single place. --- posthog/helpers/dashboard_templates.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/posthog/helpers/dashboard_templates.py b/posthog/helpers/dashboard_templates.py index 668201a44892f..5c00b0458c1a3 100644 --- a/posthog/helpers/dashboard_templates.py +++ b/posthog/helpers/dashboard_templates.py @@ -701,26 +701,32 @@ def update_feature_flag_dashboard(feature_flag, old_key: str) -> None: # We need to update the *system* created insights with the new key, so we search for them by name dashboard = feature_flag.usage_dashboard total_volume_insight = dashboard.insights.get(name="Feature Flag Called Total Volume") - if total_volume_insight: + total_volume_insight_description = _get_feature_flag_total_volume_insight_description(feature_flag.key) + if total_volume_insight and total_volume_insight_description == _get_feature_flag_total_volume_insight_description( + old_key + ): _update_tile_with_new_key( total_volume_insight, feature_flag.key, old_key, - "Shows the number of total calls made on feature flag with key:", + total_volume_insight_description, ) unique_users_insight = dashboard.insights.get(name="Feature Flag calls made by unique users per variant") - if unique_users_insight: + unique_users_insight_description = _get_feature_flag_unique_users_insight_description(feature_flag.key) + if unique_users_insight and unique_users_insight_description == _get_feature_flag_unique_users_insight_description( + old_key + ): _update_tile_with_new_key( unique_users_insight, feature_flag.key, old_key, - "Shows the number of unique user calls made on feature flag per variant with key:", + unique_users_insight_description, ) -def _update_tile_with_new_key(insight, new_key: str, old_key: str, description_prefix: str) -> None: - insight.description = f"{description_prefix} {new_key}" +def _update_tile_with_new_key(insight, new_key: str, old_key: str, description: str) -> None: + insight.description = description if insight.query: if insight.query.get("source", {}).get("properties", {}).get("values"): for property_group in insight.query["source"]["properties"]["values"]: From ff3f9256eabb8dc12d6ada5d5e4aefb03a134c01 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Tue, 7 Jan 2025 11:34:34 -0800 Subject: [PATCH 3/7] Use constants to avoid drift If we change the name of these insights, we want to still be able to update them if the feature flag key changes. --- posthog/api/feature_flag.py | 2 +- posthog/helpers/dashboard_templates.py | 40 +++++++++++++++----------- 2 files changed, 24 insertions(+), 18 deletions(-) diff --git a/posthog/api/feature_flag.py b/posthog/api/feature_flag.py index e07dd845a61b2..949fd33241a4b 100644 --- a/posthog/api/feature_flag.py +++ b/posthog/api/feature_flag.py @@ -455,7 +455,7 @@ def _create_usage_dashboard(feature_flag: FeatureFlag, user): def _update_feature_flag_dashboard(feature_flag: FeatureFlag, old_key: str) -> None: from posthog.helpers.dashboard_templates import update_feature_flag_dashboard - if not old_key or not feature_flag.usage_dashboard: + if not old_key: return update_feature_flag_dashboard(feature_flag, old_key) diff --git a/posthog/helpers/dashboard_templates.py b/posthog/helpers/dashboard_templates.py index 5c00b0458c1a3..add6585b87a09 100644 --- a/posthog/helpers/dashboard_templates.py +++ b/posthog/helpers/dashboard_templates.py @@ -556,6 +556,9 @@ def create_dashboard_from_template(template_key: str, dashboard: Dashboard) -> N create_from_template(dashboard, template) +FEATURE_FLAG_TOTAL_VOLUME_INSIGHT_NAME = "Feature Flag Called Total Volume" +FEATURE_FLAG_UNIQUE_USERS_INSIGHT_NAME = "Feature Flag calls made by unique users per variant" + def create_feature_flag_dashboard(feature_flag, dashboard: Dashboard) -> None: dashboard.filters = {"date_from": "-30d"} @@ -571,7 +574,7 @@ def create_feature_flag_dashboard(feature_flag, dashboard: Dashboard) -> None: # 1 row _create_tile_for_insight( dashboard, - name="Feature Flag Called Total Volume", + name=FEATURE_FLAG_TOTAL_VOLUME_INSIGHT_NAME, description=_get_feature_flag_total_volume_insight_description(feature_flag.key), query={ "kind": "InsightVizNode", @@ -627,7 +630,7 @@ def create_feature_flag_dashboard(feature_flag, dashboard: Dashboard) -> None: _create_tile_for_insight( dashboard, - name="Feature Flag calls made by unique users per variant", + name=FEATURE_FLAG_UNIQUE_USERS_INSIGHT_NAME, description=_get_feature_flag_unique_users_insight_description(feature_flag.key), query={ "kind": "InsightVizNode", @@ -688,7 +691,6 @@ def create_feature_flag_dashboard(feature_flag, dashboard: Dashboard) -> None: color="green", ) - def _get_feature_flag_total_volume_insight_description(feature_flag_key: str) -> str: return f"Shows the number of total calls made on feature flag with key: {feature_flag_key}" @@ -700,33 +702,37 @@ def _get_feature_flag_unique_users_insight_description(feature_flag_key: str) -> def update_feature_flag_dashboard(feature_flag, old_key: str) -> None: # We need to update the *system* created insights with the new key, so we search for them by name dashboard = feature_flag.usage_dashboard - total_volume_insight = dashboard.insights.get(name="Feature Flag Called Total Volume") - total_volume_insight_description = _get_feature_flag_total_volume_insight_description(feature_flag.key) - if total_volume_insight and total_volume_insight_description == _get_feature_flag_total_volume_insight_description( - old_key - ): + + if not dashboard: + return + + total_volume_insight = dashboard.insights.get(name=FEATURE_FLAG_TOTAL_VOLUME_INSIGHT_NAME) + if total_volume_insight: _update_tile_with_new_key( total_volume_insight, feature_flag.key, old_key, - total_volume_insight_description, + _get_feature_flag_total_volume_insight_description, ) - unique_users_insight = dashboard.insights.get(name="Feature Flag calls made by unique users per variant") - unique_users_insight_description = _get_feature_flag_unique_users_insight_description(feature_flag.key) - if unique_users_insight and unique_users_insight_description == _get_feature_flag_unique_users_insight_description( - old_key - ): + unique_users_insight = dashboard.insights.get(name=FEATURE_FLAG_UNIQUE_USERS_INSIGHT_NAME) + if unique_users_insight: _update_tile_with_new_key( unique_users_insight, feature_flag.key, old_key, - unique_users_insight_description, + _get_feature_flag_unique_users_insight_description, ) -def _update_tile_with_new_key(insight, new_key: str, old_key: str, description: str) -> None: - insight.description = description +def _update_tile_with_new_key(insight, new_key: str, old_key: str, descriptionFunction: Callable[[str], str]) -> None: + old_description = descriptionFunction(old_key) + new_description = descriptionFunction(new_key) + + if insight.description != old_description: # We don't touch insights that have been manually edited + return + + insight.description = new_description if insight.query: if insight.query.get("source", {}).get("properties", {}).get("values"): for property_group in insight.query["source"]["properties"]["values"]: From 994783080035cf2449b563ad493b7860f6ff804e Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Tue, 7 Jan 2025 14:45:10 -0800 Subject: [PATCH 4/7] Add unit test for feature flag insights --- posthog/api/test/test_feature_flag.py | 171 ++++++++++++++++++++++++++ 1 file changed, 171 insertions(+) diff --git a/posthog/api/test/test_feature_flag.py b/posthog/api/test/test_feature_flag.py index 4ae2d364d1f03..9585e96ff467d 100644 --- a/posthog/api/test/test_feature_flag.py +++ b/posthog/api/test/test_feature_flag.py @@ -792,6 +792,177 @@ def test_updating_feature_flag(self, mock_capture): ], ) + @patch("posthog.api.feature_flag.report_user_action") + def test_updating_feature_flag_key(self, mock_capture): + with freeze_time("2021-08-25T22:09:14.252Z") as frozen_datetime: + response = self.client.post( + f"/api/projects/{self.team.id}/feature_flags/", + {"name": "original name", "key": "a-feature-flag-that-is-updated"}, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + flag_id = response.json()["id"] + + frozen_datetime.tick(delta=datetime.timedelta(minutes=10)) + + # Assert that the insights were created properly. + feature_flag = FeatureFlag.objects.get(id=flag_id) + insights = feature_flag.usage_dashboard.insights.all() + total_volume_insight = insights.get(name="Feature Flag Called Total Volume") + self.assertEqual( + total_volume_insight.description, + "Shows the number of total calls made on feature flag with key: a-feature-flag-that-is-updated", + ) + self.assertEqual( + total_volume_insight.query["source"]["properties"]["values"][0]["values"][0]["value"], + "a-feature-flag-that-is-updated", + ) + unique_users_insight = insights.get(name="Feature Flag calls made by unique users per variant") + self.assertEqual( + unique_users_insight.description, + "Shows the number of unique user calls made on feature flag per variant with key: a-feature-flag-that-is-updated", + ) + self.assertEqual( + unique_users_insight.query["source"]["properties"]["values"][0]["values"][0]["value"], + "a-feature-flag-that-is-updated", + ) + + # Update the feature flag key + response = self.client.patch( + f"/api/projects/{self.team.id}/feature_flags/{flag_id}", + { + "key": "a-new-feature-flag-key", + "filters": { + "groups": [ + { + "rollout_percentage": 65, + "properties": [ + { + "key": "email", + "type": "person", + "value": "@posthog.com", + "operator": "icontains", + } + ], + } + ] + }, + }, + format="json", + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + + self.assertEqual(response.json()["key"], "a-new-feature-flag-key") + self.assertEqual(response.json()["filters"]["groups"][0]["rollout_percentage"], 65) + + # Assert analytics are sent + mock_capture.assert_called_with( + self.user, + "feature flag updated", + { + "groups_count": 1, + "has_variants": False, + "variants_count": 0, + "has_rollout_percentage": True, + "has_filters": True, + "filter_count": 1, + "created_at": datetime.datetime.fromisoformat("2021-08-25T22:09:14.252000+00:00"), + "aggregating_by_groups": False, + "payload_count": 0, + }, + ) + + self.assert_feature_flag_activity( + flag_id, + [ + { + "user": { + "first_name": self.user.first_name, + "email": self.user.email, + }, + "activity": "updated", + "created_at": "2021-08-25T22:19:14.252000Z", + "scope": "FeatureFlag", + "item_id": str(flag_id), + "detail": { + "changes": [ + { + "type": "FeatureFlag", + "action": "changed", + "field": "key", + "before": "a-feature-flag-that-is-updated", + "after": "a-new-feature-flag-key", + }, + { + "type": "FeatureFlag", + "action": "created", + "field": "filters", + "before": None, + "after": { + "groups": [ + { + "properties": [ + { + "key": "email", + "type": "person", + "value": "@posthog.com", + "operator": "icontains", + } + ], + "rollout_percentage": 65, + } + ] + }, + }, + ], + "trigger": None, + "type": None, + "name": "a-new-feature-flag-key", + "short_id": None, + }, + }, + { + "user": { + "first_name": self.user.first_name, + "email": self.user.email, + }, + "activity": "created", + "created_at": "2021-08-25T22:09:14.252000Z", + "scope": "FeatureFlag", + "item_id": str(flag_id), + "detail": { + "changes": None, + "trigger": None, + "type": None, + "name": "a-feature-flag-that-is-updated", + "short_id": None, + }, + }, + ], + ) + + feature_flag = FeatureFlag.objects.get(id=flag_id) + insights = feature_flag.usage_dashboard.insights.all() + total_volume_insight = insights.get(name="Feature Flag Called Total Volume") + self.assertEqual( + total_volume_insight.description, + "Shows the number of total calls made on feature flag with key: a-new-feature-flag-key", + ) + self.assertEqual( + total_volume_insight.query["source"]["properties"]["values"][0]["values"][0]["value"], + "a-new-feature-flag-key", + ) + unique_users_insight = insights.get(name="Feature Flag calls made by unique users per variant") + self.assertEqual( + unique_users_insight.description, + "Shows the number of unique user calls made on feature flag per variant with key: a-new-feature-flag-key", + ) + self.assertEqual( + unique_users_insight.query["source"]["properties"]["values"][0]["values"][0]["value"], + "a-new-feature-flag-key", + ) + def test_hard_deleting_feature_flag_is_forbidden(self): new_user = User.objects.create_and_join(self.organization, "new_annotations@posthog.com", None) From 11b68692ab7d1c0c6ca582f4f92df49fd2200d0e Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Tue, 7 Jan 2025 15:21:39 -0800 Subject: [PATCH 5/7] Use filter and first and not get --- posthog/api/test/test_feature_flag.py | 87 ++++++++++++++++++++++++++ posthog/helpers/dashboard_templates.py | 12 ++-- 2 files changed, 94 insertions(+), 5 deletions(-) diff --git a/posthog/api/test/test_feature_flag.py b/posthog/api/test/test_feature_flag.py index 9585e96ff467d..eb67f05d18a67 100644 --- a/posthog/api/test/test_feature_flag.py +++ b/posthog/api/test/test_feature_flag.py @@ -963,6 +963,93 @@ def test_updating_feature_flag_key(self, mock_capture): "a-new-feature-flag-key", ) + @patch("posthog.api.feature_flag.report_user_action") + def test_updating_feature_flag_key_does_not_update_changed_insights(self, mock_capture): + with freeze_time("2021-08-25T22:09:14.252Z") as frozen_datetime: + response = self.client.post( + f"/api/projects/{self.team.id}/feature_flags/", + {"name": "original name", "key": "a-feature-flag-that-is-updated"}, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + flag_id = response.json()["id"] + + frozen_datetime.tick(delta=datetime.timedelta(minutes=10)) + + # Assert that the insights were created properly. + feature_flag = FeatureFlag.objects.get(id=flag_id) + insights = feature_flag.usage_dashboard.insights.all() + total_volume_insight = insights.get(name="Feature Flag Called Total Volume") + self.assertEqual( + total_volume_insight.description, + "Shows the number of total calls made on feature flag with key: a-feature-flag-that-is-updated", + ) + self.assertEqual( + total_volume_insight.query["source"]["properties"]["values"][0]["values"][0]["value"], + "a-feature-flag-that-is-updated", + ) + unique_users_insight = insights.get(name="Feature Flag calls made by unique users per variant") + self.assertEqual( + unique_users_insight.description, + "Shows the number of unique user calls made on feature flag per variant with key: a-feature-flag-that-is-updated", + ) + self.assertEqual( + unique_users_insight.query["source"]["properties"]["values"][0]["values"][0]["value"], + "a-feature-flag-that-is-updated", + ) + total_volume_insight.name = "This is a changed description" + total_volume_insight.save() + + # Update the feature flag key + response = self.client.patch( + f"/api/projects/{self.team.id}/feature_flags/{flag_id}", + { + "key": "a-new-feature-flag-key", + "filters": { + "groups": [ + { + "rollout_percentage": 65, + "properties": [ + { + "key": "email", + "type": "person", + "value": "@posthog.com", + "operator": "icontains", + } + ], + } + ] + }, + }, + format="json", + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + + # Total volume insight should not be updated because we changed its description + # unique users insight should still be updated + feature_flag = FeatureFlag.objects.get(id=flag_id) + insights = feature_flag.usage_dashboard.insights.all() + self.assertIsNone(insights.filter(name="Feature Flag Called Total Volume").first()) + total_volume_insight = insights.get(name="This is a changed description") + self.assertEqual( + total_volume_insight.description, + "Shows the number of total calls made on feature flag with key: a-feature-flag-that-is-updated", + ) + self.assertEqual( + total_volume_insight.query["source"]["properties"]["values"][0]["values"][0]["value"], + "a-feature-flag-that-is-updated", + ) + unique_users_insight = insights.get(name="Feature Flag calls made by unique users per variant") + self.assertEqual( + unique_users_insight.description, + "Shows the number of unique user calls made on feature flag per variant with key: a-new-feature-flag-key", + ) + self.assertEqual( + unique_users_insight.query["source"]["properties"]["values"][0]["values"][0]["value"], + "a-new-feature-flag-key", + ) + def test_hard_deleting_feature_flag_is_forbidden(self): new_user = User.objects.create_and_join(self.organization, "new_annotations@posthog.com", None) diff --git a/posthog/helpers/dashboard_templates.py b/posthog/helpers/dashboard_templates.py index add6585b87a09..5fba8d1d5f477 100644 --- a/posthog/helpers/dashboard_templates.py +++ b/posthog/helpers/dashboard_templates.py @@ -556,6 +556,7 @@ def create_dashboard_from_template(template_key: str, dashboard: Dashboard) -> N create_from_template(dashboard, template) + FEATURE_FLAG_TOTAL_VOLUME_INSIGHT_NAME = "Feature Flag Called Total Volume" FEATURE_FLAG_UNIQUE_USERS_INSIGHT_NAME = "Feature Flag calls made by unique users per variant" @@ -691,6 +692,7 @@ def create_feature_flag_dashboard(feature_flag, dashboard: Dashboard) -> None: color="green", ) + def _get_feature_flag_total_volume_insight_description(feature_flag_key: str) -> str: return f"Shows the number of total calls made on feature flag with key: {feature_flag_key}" @@ -706,7 +708,7 @@ def update_feature_flag_dashboard(feature_flag, old_key: str) -> None: if not dashboard: return - total_volume_insight = dashboard.insights.get(name=FEATURE_FLAG_TOTAL_VOLUME_INSIGHT_NAME) + total_volume_insight = dashboard.insights.filter(name=FEATURE_FLAG_TOTAL_VOLUME_INSIGHT_NAME).first() if total_volume_insight: _update_tile_with_new_key( total_volume_insight, @@ -715,7 +717,7 @@ def update_feature_flag_dashboard(feature_flag, old_key: str) -> None: _get_feature_flag_total_volume_insight_description, ) - unique_users_insight = dashboard.insights.get(name=FEATURE_FLAG_UNIQUE_USERS_INSIGHT_NAME) + unique_users_insight = dashboard.insights.filter(name=FEATURE_FLAG_UNIQUE_USERS_INSIGHT_NAME).first() if unique_users_insight: _update_tile_with_new_key( unique_users_insight, @@ -728,10 +730,10 @@ def update_feature_flag_dashboard(feature_flag, old_key: str) -> None: def _update_tile_with_new_key(insight, new_key: str, old_key: str, descriptionFunction: Callable[[str], str]) -> None: old_description = descriptionFunction(old_key) new_description = descriptionFunction(new_key) - - if insight.description != old_description: # We don't touch insights that have been manually edited + + if insight.description != old_description: # We don't touch insights that have been manually edited return - + insight.description = new_description if insight.query: if insight.query.get("source", {}).get("properties", {}).get("values"): From 9d669a6172e91b0fb7227077518ad94559dac08c Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Tue, 7 Jan 2025 15:52:33 -0800 Subject: [PATCH 6/7] Ensure we only change insight that match generated ones exactly And added more tests --- posthog/api/test/test_feature_flag.py | 177 ++++++++++++++++++++++++- posthog/helpers/dashboard_templates.py | 22 +-- 2 files changed, 190 insertions(+), 9 deletions(-) diff --git a/posthog/api/test/test_feature_flag.py b/posthog/api/test/test_feature_flag.py index eb67f05d18a67..15f0972e2ff48 100644 --- a/posthog/api/test/test_feature_flag.py +++ b/posthog/api/test/test_feature_flag.py @@ -964,7 +964,7 @@ def test_updating_feature_flag_key(self, mock_capture): ) @patch("posthog.api.feature_flag.report_user_action") - def test_updating_feature_flag_key_does_not_update_changed_insights(self, mock_capture): + def test_updating_feature_flag_key_does_not_update_insight_with_changed_description(self, mock_capture): with freeze_time("2021-08-25T22:09:14.252Z") as frozen_datetime: response = self.client.post( f"/api/projects/{self.team.id}/feature_flags/", @@ -1050,6 +1050,181 @@ def test_updating_feature_flag_key_does_not_update_changed_insights(self, mock_c "a-new-feature-flag-key", ) + @patch("posthog.api.feature_flag.report_user_action") + def test_updating_feature_flag_key_does_not_update_insight_with_changed_filter(self, mock_capture): + with freeze_time("2021-08-25T22:09:14.252Z") as frozen_datetime: + response = self.client.post( + f"/api/projects/{self.team.id}/feature_flags/", + {"name": "original name", "key": "a-feature-flag-that-is-updated"}, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + flag_id = response.json()["id"] + + frozen_datetime.tick(delta=datetime.timedelta(minutes=10)) + + # Assert that the insights were created properly. + feature_flag = FeatureFlag.objects.get(id=flag_id) + insights = feature_flag.usage_dashboard.insights.all() + total_volume_insight = insights.get(name="Feature Flag Called Total Volume") + self.assertEqual( + total_volume_insight.description, + "Shows the number of total calls made on feature flag with key: a-feature-flag-that-is-updated", + ) + self.assertEqual( + total_volume_insight.query["source"]["properties"]["values"][0]["values"][0]["value"], + "a-feature-flag-that-is-updated", + ) + unique_users_insight = insights.get(name="Feature Flag calls made by unique users per variant") + self.assertEqual( + unique_users_insight.description, + "Shows the number of unique user calls made on feature flag per variant with key: a-feature-flag-that-is-updated", + ) + self.assertEqual( + unique_users_insight.query["source"]["properties"]["values"][0]["values"][0]["value"], + "a-feature-flag-that-is-updated", + ) + total_volume_insight.query["source"]["properties"]["values"][0]["values"][0]["value"] = ( + "something_unexpected" + ) + total_volume_insight.save() + + # Update the feature flag key + response = self.client.patch( + f"/api/projects/{self.team.id}/feature_flags/{flag_id}", + { + "key": "a-new-feature-flag-key", + "filters": { + "groups": [ + { + "rollout_percentage": 65, + "properties": [ + { + "key": "email", + "type": "person", + "value": "@posthog.com", + "operator": "icontains", + } + ], + } + ] + }, + }, + format="json", + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + + # Total volume insight should not be updated because we changed its description + # unique users insight should still be updated + feature_flag = FeatureFlag.objects.get(id=flag_id) + insights = feature_flag.usage_dashboard.insights.all() + total_volume_insight = insights.get(name="Feature Flag Called Total Volume") + self.assertEqual( + total_volume_insight.description, + "Shows the number of total calls made on feature flag with key: a-feature-flag-that-is-updated", + ) + self.assertEqual( + total_volume_insight.query["source"]["properties"]["values"][0]["values"][0]["value"], + "something_unexpected", + ) + unique_users_insight = insights.get(name="Feature Flag calls made by unique users per variant") + self.assertEqual( + unique_users_insight.description, + "Shows the number of unique user calls made on feature flag per variant with key: a-new-feature-flag-key", + ) + self.assertEqual( + unique_users_insight.query["source"]["properties"]["values"][0]["values"][0]["value"], + "a-new-feature-flag-key", + ) + + @patch("posthog.api.feature_flag.report_user_action") + def test_updating_feature_flag_key_does_not_update_insight_with_removed_filter(self, mock_capture): + with freeze_time("2021-08-25T22:09:14.252Z") as frozen_datetime: + response = self.client.post( + f"/api/projects/{self.team.id}/feature_flags/", + {"name": "original name", "key": "a-feature-flag-that-is-updated"}, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + flag_id = response.json()["id"] + + frozen_datetime.tick(delta=datetime.timedelta(minutes=10)) + + # Assert that the insights were created properly. + feature_flag = FeatureFlag.objects.get(id=flag_id) + insights = feature_flag.usage_dashboard.insights.all() + total_volume_insight = insights.get(name="Feature Flag Called Total Volume") + self.assertEqual( + total_volume_insight.description, + "Shows the number of total calls made on feature flag with key: a-feature-flag-that-is-updated", + ) + self.assertEqual( + total_volume_insight.query["source"]["properties"]["values"][0]["values"][0]["value"], + "a-feature-flag-that-is-updated", + ) + unique_users_insight = insights.get(name="Feature Flag calls made by unique users per variant") + self.assertEqual( + unique_users_insight.description, + "Shows the number of unique user calls made on feature flag per variant with key: a-feature-flag-that-is-updated", + ) + self.assertEqual( + unique_users_insight.query["source"]["properties"]["values"][0]["values"][0]["value"], + "a-feature-flag-that-is-updated", + ) + # clear the values from total_volume_insight.query["source"]["properties"]["values"] + total_volume_insight.query["source"]["properties"]["values"] = [] + total_volume_insight.save() + + # Update the feature flag key + response = self.client.patch( + f"/api/projects/{self.team.id}/feature_flags/{flag_id}", + { + "key": "a-new-feature-flag-key", + "filters": { + "groups": [ + { + "rollout_percentage": 65, + "properties": [ + { + "key": "email", + "type": "person", + "value": "@posthog.com", + "operator": "icontains", + } + ], + } + ] + }, + }, + format="json", + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + + # Total volume insight should not be updated because we changed its description + # unique users insight should still be updated + feature_flag = FeatureFlag.objects.get(id=flag_id) + insights = feature_flag.usage_dashboard.insights.all() + total_volume_insight = insights.get(name="Feature Flag Called Total Volume") + self.assertEqual( + total_volume_insight.description, + "Shows the number of total calls made on feature flag with key: a-feature-flag-that-is-updated", + ) + self.assertEqual( + total_volume_insight.query["source"]["properties"]["values"], + [], + ) + unique_users_insight = insights.get(name="Feature Flag calls made by unique users per variant") + self.assertEqual( + unique_users_insight.description, + "Shows the number of unique user calls made on feature flag per variant with key: a-new-feature-flag-key", + ) + self.assertEqual( + unique_users_insight.query["source"]["properties"]["values"][0]["values"][0]["value"], + "a-new-feature-flag-key", + ) + def test_hard_deleting_feature_flag_is_forbidden(self): new_user = User.objects.create_and_join(self.organization, "new_annotations@posthog.com", None) diff --git a/posthog/helpers/dashboard_templates.py b/posthog/helpers/dashboard_templates.py index 5fba8d1d5f477..7cfb390128335 100644 --- a/posthog/helpers/dashboard_templates.py +++ b/posthog/helpers/dashboard_templates.py @@ -734,15 +734,21 @@ def _update_tile_with_new_key(insight, new_key: str, old_key: str, descriptionFu if insight.description != old_description: # We don't touch insights that have been manually edited return - insight.description = new_description if insight.query: - if insight.query.get("source", {}).get("properties", {}).get("values"): - for property_group in insight.query["source"]["properties"]["values"]: - for property_value in property_group.get("values", []): - if property_value.get("key") == "$feature_flag" and property_value.get("value") == old_key: - property_value["value"] = new_key - insight.query = insight.query # Trigger field update - insight.save() + property_values = insight.query.get("source", {}).get("properties", {}).get("values", []) + if len(property_values) != 1: # Exit if not exactly one property group + return + + property_group = property_values[0] + values = property_group.get("values", []) + # Only proceed if there's exactly one value and it's a feature flag + if len(values) == 1 and values[0].get("key") == "$feature_flag" and values[0].get("value") == old_key: + values[0]["value"] = new_key + insight.query = insight.query # Trigger field update + # Only update the insight if it matches what we expect for the system created insights + insight.description = new_description + insight.save() + return def add_enriched_insights_to_feature_flag_dashboard(feature_flag, dashboard: Dashboard) -> None: From 8671c6093db0b613947d3db878d02febcfd052ae Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Tue, 7 Jan 2025 16:07:21 -0800 Subject: [PATCH 7/7] Fix mypy static analysis errors --- posthog/api/test/test_feature_flag.py | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/posthog/api/test/test_feature_flag.py b/posthog/api/test/test_feature_flag.py index 15f0972e2ff48..35bfa12fe3c34 100644 --- a/posthog/api/test/test_feature_flag.py +++ b/posthog/api/test/test_feature_flag.py @@ -807,7 +807,8 @@ def test_updating_feature_flag_key(self, mock_capture): # Assert that the insights were created properly. feature_flag = FeatureFlag.objects.get(id=flag_id) - insights = feature_flag.usage_dashboard.insights.all() + assert feature_flag.usage_dashboard is not None, "Usage dashboard was not created" + insights = feature_flag.usage_dashboard.insights total_volume_insight = insights.get(name="Feature Flag Called Total Volume") self.assertEqual( total_volume_insight.description, @@ -943,7 +944,8 @@ def test_updating_feature_flag_key(self, mock_capture): ) feature_flag = FeatureFlag.objects.get(id=flag_id) - insights = feature_flag.usage_dashboard.insights.all() + assert feature_flag.usage_dashboard is not None, "Usage dashboard was not created" + insights = feature_flag.usage_dashboard.insights total_volume_insight = insights.get(name="Feature Flag Called Total Volume") self.assertEqual( total_volume_insight.description, @@ -978,7 +980,8 @@ def test_updating_feature_flag_key_does_not_update_insight_with_changed_descript # Assert that the insights were created properly. feature_flag = FeatureFlag.objects.get(id=flag_id) - insights = feature_flag.usage_dashboard.insights.all() + assert feature_flag.usage_dashboard is not None, "Usage dashboard was not created" + insights = feature_flag.usage_dashboard.insights total_volume_insight = insights.get(name="Feature Flag Called Total Volume") self.assertEqual( total_volume_insight.description, @@ -1029,7 +1032,8 @@ def test_updating_feature_flag_key_does_not_update_insight_with_changed_descript # Total volume insight should not be updated because we changed its description # unique users insight should still be updated feature_flag = FeatureFlag.objects.get(id=flag_id) - insights = feature_flag.usage_dashboard.insights.all() + assert feature_flag.usage_dashboard is not None, "Usage dashboard was not created" + insights = feature_flag.usage_dashboard.insights self.assertIsNone(insights.filter(name="Feature Flag Called Total Volume").first()) total_volume_insight = insights.get(name="This is a changed description") self.assertEqual( @@ -1065,7 +1069,8 @@ def test_updating_feature_flag_key_does_not_update_insight_with_changed_filter(s # Assert that the insights were created properly. feature_flag = FeatureFlag.objects.get(id=flag_id) - insights = feature_flag.usage_dashboard.insights.all() + assert feature_flag.usage_dashboard is not None, "Usage dashboard was not created" + insights = feature_flag.usage_dashboard.insights total_volume_insight = insights.get(name="Feature Flag Called Total Volume") self.assertEqual( total_volume_insight.description, @@ -1118,7 +1123,8 @@ def test_updating_feature_flag_key_does_not_update_insight_with_changed_filter(s # Total volume insight should not be updated because we changed its description # unique users insight should still be updated feature_flag = FeatureFlag.objects.get(id=flag_id) - insights = feature_flag.usage_dashboard.insights.all() + assert feature_flag.usage_dashboard is not None, "Usage dashboard was not created" + insights = feature_flag.usage_dashboard.insights total_volume_insight = insights.get(name="Feature Flag Called Total Volume") self.assertEqual( total_volume_insight.description, @@ -1153,7 +1159,8 @@ def test_updating_feature_flag_key_does_not_update_insight_with_removed_filter(s # Assert that the insights were created properly. feature_flag = FeatureFlag.objects.get(id=flag_id) - insights = feature_flag.usage_dashboard.insights.all() + assert feature_flag.usage_dashboard is not None, "Usage dashboard was not created" + insights = feature_flag.usage_dashboard.insights total_volume_insight = insights.get(name="Feature Flag Called Total Volume") self.assertEqual( total_volume_insight.description, @@ -1205,7 +1212,8 @@ def test_updating_feature_flag_key_does_not_update_insight_with_removed_filter(s # Total volume insight should not be updated because we changed its description # unique users insight should still be updated feature_flag = FeatureFlag.objects.get(id=flag_id) - insights = feature_flag.usage_dashboard.insights.all() + assert feature_flag.usage_dashboard is not None, "Usage dashboard was not created" + insights = feature_flag.usage_dashboard.insights total_volume_insight = insights.get(name="Feature Flag Called Total Volume") self.assertEqual( total_volume_insight.description,