fix(insights): delete subscriptions with their insight

frontend/src/scenes/insights/InsightScene.test.tsx

@@ -0,0 +1,32 @@
+import '@testing-library/jest-dom'
+
+import { cleanup, render, screen } from '@testing-library/react'
+import { router } from 'kea-router'
+
+import { useMocks } from '~/mocks/jest'
+import { initKeaTests } from '~/test/init'
+
+import { InsightScene } from './InsightScene'
+
+jest.mock('scenes/insights/InsightAsScene', () => ({ InsightAsScene: () => null }))
+jest.mock('products/posthog_ai/frontend/api/logics', () => ({ useAttachedContext: jest.fn() }))
+
+describe('InsightScene', () => {
+    afterEach(cleanup)
+
+    it('replaces the skeleton with not found when a directly opened subscription has no parent insight', async () => {
+        useMocks({
+            get: {
+                '/api/environments/:team_id/insights/': { count: 0, results: [] },
+            },
+        })
+        initKeaTests()
+        router.actions.push('/insights/missing1/subscriptions/123')
+
+        render(<InsightScene />)
+
+        expect(screen.getAllByText('Loading…').length).toBeGreaterThan(0)
+        expect(await screen.findByText('Insight not found')).toBeInTheDocument()
+        expect(screen.queryByText('Loading…')).not.toBeInTheDocument()
+    })
+})

frontend/src/scenes/insights/InsightScene.tsx

@@ -14,8 +14,8 @@
 
 import { useAttachedContext } from 'products/posthog_ai/frontend/api/logics'
 
 export function InsightScene(): JSX.Element {
-    const { insightId, insight, insightLogicRef, insightMode, dashboardId } = useValues(insightSceneLogic)
+    const { insightId, insight, insightLoading, insightMode, dashboardId } = useValues(insightSceneLogic)
 
     useAttachedContext(
         insight?.short_id && insight?.query
@@ -46,7 +46,7 @@
         return <InsightAsScene insightId={insightId} attachTo={insightSceneLogic} />
     }
 
-    if (insightLogicRef?.logic?.values?.insightLoading) {
+    if (insightLoading) {
         return <InsightSkeleton />
     }
 

frontend/src/scenes/insights/insightLogic.tsx

@@ -1074,7 +1074,7 @@
         ],
     }),
     listeners(({ actions, values, props }) => ({
         saveInsight: async ({ redirectToViewMode, folder }) => {
             const insightNumericId =
                 values.insight.id || (values.insight.short_id ? await getInsightId(values.insight.short_id) : undefined)
             const { name, description, favorited, deleted, dashboards, tags } = values.insight
@@ -1327,7 +1327,8 @@
         deleteInsight: ({ dashboardId }) => {
             LemonDialog.open({
                 title: 'Delete insight?',
-                description: 'Are you sure you want to delete this insight? This action can be undone.',
+                description:
+                    'Are you sure you want to delete this insight? Associated alerts and subscriptions will also be removed. Their removal cannot be undone.',
                 primaryButton: {
                     children: 'Delete',
                     status: 'danger',

frontend/src/scenes/insights/insightSceneLogic.tsx

@@ -139,6 +139,7 @@
         | ((state: any, props?: InsightLogicProps<QuerySchema> | undefined) => Record<string, any>)
         | undefined
     insightId: InsightId | null
+    insightLoading: boolean
     insightLogicRef: {
         logic: BuiltLogic<insightLogicType>
         unmount: () => void
@@ -256,6 +257,7 @@
             } | null
         ) => ((state: any, props?: InsightLogicProps<QuerySchema> | undefined) => Record<string, any>) | undefined
         insightData: (arg: Record<string, any> | null | undefined) => Record<string, any> | null | undefined
+        insightLoading: (arg: boolean) => boolean
         insightSelector: (
             insightLogicRef: {
                 logic: BuiltLogic<insightLogicType>
@@ -502,6 +504,13 @@
             ],
             (insightData: Record<string, any> | null | undefined) => insightData,
         ],
+        insightLoading: [
+            (s) => [
+                (state, props) =>
+                    s.insightLogicRef(state, props)?.logic.selectors.insightLoading(state, props) ?? false,
+            ],
+            (insightLoading: boolean): boolean => insightLoading,
+        ],
         insightSelector: [
             (s) => [s.insightLogicRef],
             (
@@ -697,7 +706,7 @@
          * state, going back and editing that tile could show the copy instead. Remount when those disagree, and
          * when the URL insight does not match which insight this editor was opened from.
          */
         reloadInsightLogic: () => {
             const logicInsightId = values.insight?.short_id ?? null
             const insightId = values.insightId ?? null
             const mountedDashboardItemId = values.insightLogicRef?.logic.props.dashboardItemId ?? null
@@ -808,7 +817,7 @@
         },
     })),
     urlToAction(({ actions, values }) => ({
         '/insights/:shortId(/:mode)(/:itemId)': (
             { shortId, mode, itemId }, // url params
             { dashboard, alert_id, ...searchParams }, // search params
             { insight: insightType, q, sceneSource }, // hash params

frontend/src/scenes/saved-insights/SavedInsights.tsx

@@ -86,7 +86,7 @@
     return Icon ? <Icon className={className} /> : null
 }
 
 export function SavedInsights(): JSX.Element {
     const { push } = useActions(router)
     const {
         loadInsights,
@@ -295,7 +295,7 @@
                                             LemonDialog.open({
                                                 title: 'Delete insight?',
                                                 description:
-                                                    'Are you sure you want to delete this insight? This action can be undone.',
+                                                    'Are you sure you want to delete this insight? Associated alerts and subscriptions will also be removed. Their removal cannot be undone.',
                                                 primaryButton: {
                                                     children: 'Delete',
                                                     status: 'danger',
@@ -453,7 +453,7 @@
                                             const noun = count === 1 ? 'insight' : 'insights'
                                             LemonDialog.open({
                                                 title: `Delete ${count} ${noun}?`,
-                                                description: `Are you sure you want to delete ${count} ${noun}? This action can be undone.`,
+                                                description: `Are you sure you want to delete ${count} ${noun}? Associated alerts and subscriptions will also be removed. Their removal cannot be undone.`,
                                                 primaryButton: {
                                                     children: 'Delete',
                                                     status: 'danger',

products/exports/backend/facade/api.py

@@ -103,6 +103,11 @@
     _save_content_from_file(asset, file_path, max_database_bytes=max_database_bytes)
 
 
+def delete_insight_subscriptions(*, project_id: int, insight_ids: Collection[int]) -> None:
+    for subscription in Subscription.objects.filter(team__project_id=project_id, insight_id__in=insight_ids):
+        subscription.delete()
+
+
 def insight_ids_with_subscriptions(insight_ids: Collection[int]) -> set[int]:
     """Which of the given insights have a subscription that has not been deleted.
 
@@ -185,7 +190,7 @@
     return asset.get_subscription_delivery_content_url(expiry_delta=expiry_delta)
 
 
 def render_png_export(
     *,
     team: Team,
     created_by: User,

products/exports/backend/models/subscription.py

@@ -116,6 +116,8 @@ class Subscription(ModelActivityMixin, models.Model):
     Also see https://jakubroztocil.github.io/rrule/ for a helpful visual demonstration
     """
 
+    activity_logging_on_delete = True
+
     class SubscriptionTarget(models.TextChoices):
         EMAIL = "email"
         SLACK = "slack"

products/product_analytics/backend/presentation/insight.py

@@ -160,6 +160,7 @@
     update_insight_dashboard_membership,
 )
 from products.dashboards.backend.facade.enums import PrivilegeLevel, RestrictionLevel
+from products.exports.backend.facade.api import delete_insight_subscriptions
 from products.product_analytics.backend.facade.account_filters import plan_test_account_filter_update
 from products.product_analytics.backend.facade.api import (
     insight_variables_for_team,
@@ -868,17 +869,21 @@
                     "and this insight is publicly shared."
                 )
 
-        if validated_data.get("deleted", False):
-            hide_tiles_for_insights([instance.id])
-            for alert in instance.alertconfiguration_set.all():
-                alert.delete()
-        else:
-            dashboard_ids = validated_data.pop("dashboards", None)
-            if dashboard_ids is not None:
-                # The membership write runs before the query is saved, so gate on the incoming one.
-                self._update_insight_dashboards(dashboard_ids, instance, validated_data.get("query", instance.query))
+        with transaction.atomic():
+            if validated_data.get("deleted", False):
+                delete_insight_subscriptions(project_id=instance.team.project_id, insight_ids=[instance.id])
+                hide_tiles_for_insights([instance.id])
+                for alert in instance.alertconfiguration_set.all():
+                    alert.delete()
+            else:
+                dashboard_ids = validated_data.pop("dashboards", None)
+                if dashboard_ids is not None:
+                    # The membership write runs before the query is saved, so gate on the incoming one.
+                    self._update_insight_dashboards(
+                        dashboard_ids, instance, validated_data.get("query", instance.query)
+                    )
 
-        updated_insight = super().update(instance, validated_data)
+            updated_insight = super().update(instance, validated_data)
         # Delete linked alerts only when the insight can no longer carry any alert. A switch between
         # alertable kinds (e.g. trends -> SQL) is left alone: the config type no longer matches, but
         # the alert check cycle re-validates against the current query and auto-disables + notifies on
@@ -2002,7 +2007,7 @@
             tiebreakers=("name",),
         )
 
     def _filter_request(self, request: request.Request, queryset: QuerySet) -> QuerySet:
         filters = request.GET.dict()
         search_term: str | None = None
 
@@ -2365,6 +2370,7 @@
                 # Match InsightSerializer.update: hide the insights' tiles and remove linked alerts.
                 hide_tiles_for_insights(insight_ids)
                 delete_insight_alerts(insight_ids)
+                delete_insight_subscriptions(project_id=self.team.project_id, insight_ids=insight_ids)
 
                 activity_log_entries: list[LogActivityEntry] = []
                 for insight in insights:

products/product_analytics/backend/tests/api/test_insight.py

@@ -54,6 +54,7 @@
 from posthog.exceptions import ClickHouseQueryTimeOut
 from posthog.hogql_queries.query_runner import SHARED_FORCE_BLOCKING_STALENESS_WINDOW, ExecutionMode
 from posthog.models import Filter, OrganizationMembership, SharingConfiguration, Team, User
+from posthog.models.activity_logging.activity_log import ActivityLog
 from posthog.models.project import Project
 from posthog.query_scan.findings import build_warning
 from posthog.query_scan.flag import QueryScanFlag, QueryScanMode
@@ -66,6 +67,7 @@
 from products.dashboards.backend.facade.access import DashboardAccessMethod
 from products.dashboards.backend.models.dashboard import Dashboard
 from products.dashboards.backend.models.dashboard_tile import DashboardTile, Text
+from products.exports.backend.models.subscription import Subscription, SubscriptionDelivery
 from products.product_analytics.backend.facade.models import Insight, InsightVariable
 from products.product_analytics.backend.models.insight import InsightViewed
 
@@ -3073,14 +3075,38 @@ def test_hard_delete_is_forbidden(self) -> None:
     def test_soft_delete_causes_404(self) -> None:
         insight_id, _ = self.dashboard_api.create_insight({"name": "to be deleted"})
         self.dashboard_api.get_insight(insight_id=insight_id, expected_status=status.HTTP_200_OK)
+        subscription = Subscription.objects.create(
+            team=self.team, insight_id=insight_id, start_date=timezone.now(), frequency="daily"
+        )
+        unrelated = Subscription.objects.create(team=self.team, start_date=timezone.now(), frequency="daily")
+        deleted_subscription = Subscription.objects.create(
+            team=self.team, insight_id=insight_id, start_date=timezone.now(), frequency="daily", deleted=True
+        )
+        delivery = SubscriptionDelivery.objects.create(
+            team=self.team, subscription=subscription, idempotency_key="insight-delete-test", status="completed"
+        )
 
         update_response = self.client.patch(f"/api/projects/{self.team.id}/insights/{insight_id}", {"deleted": True})
         self.assertEqual(update_response.status_code, status.HTTP_200_OK)
 
         self.dashboard_api.get_insight(insight_id=insight_id, expected_status=status.HTTP_404_NOT_FOUND)
 
+        self.assertFalse(Subscription.objects.filter(pk=subscription.pk).exists())
+        deletion_log = ActivityLog.objects.get(
+            team_id=self.team.id, scope="Subscription", item_id=str(subscription.pk), activity="deleted"
+        )
+        self.assertEqual(deletion_log.user_id, self.user.id)
+        unrelated.refresh_from_db()
+        self.assertFalse(unrelated.deleted)
+
+        self.assertFalse(Subscription.objects.filter(pk=deleted_subscription.pk).exists())
+        self.assertFalse(SubscriptionDelivery.objects.filter(pk=delivery.pk).exists())
+
     def test_soft_delete_can_be_reversed_by_patch(self) -> None:
         insight_id, _ = self.dashboard_api.create_insight({"name": "an insight"})
+        subscription = Subscription.objects.create(
+            team=self.team, insight_id=insight_id, start_date=timezone.now(), frequency="daily"
+        )
 
         self.client.patch(
             f"/api/projects/{self.team.id}/insights/{insight_id}",
@@ -3124,6 +3150,8 @@ def test_soft_delete_can_be_reversed_by_patch(self) -> None:
             "type": "Insight",
         }
 
+        self.assertFalse(Subscription.objects.filter(pk=subscription.pk).exists())
+
     def test_soft_delete_cannot_be_reversed_for_another_team(self) -> None:
         other_team = Team.objects.create(organization=self.organization, name="other team")
         other_insight = Insight.objects.create(
@@ -4289,13 +4317,25 @@ def test_bulk_delete_hides_dashboard_tiles_and_removes_alerts(self) -> None:
         insight = self._create_insight()
         tile = DashboardTile.objects.create(insight=insight, dashboard=dashboard)
         alert = AlertConfiguration.objects.create(team=self.team, insight=insight, name="alert")
+        subscription = Subscription.objects.create(
+            team=self.team, insight_id=insight.id, start_date=timezone.now(), frequency="daily"
+        )
+        unrelated = Subscription.objects.create(team=self.team, start_date=timezone.now(), frequency="daily")
 
         response = self._bulk_delete([insight.id])
 
         self.assertEqual(response.status_code, status.HTTP_200_OK, response.json())
         self.assertTrue(DashboardTile.objects_including_soft_deleted.get(id=tile.id).deleted)
         self.assertFalse(AlertConfiguration.objects.filter(id=alert.id).exists())
 
+        self.assertFalse(Subscription.objects.filter(pk=subscription.pk).exists())
+        deletion_log = ActivityLog.objects.get(
+            team_id=self.team.id, scope="Subscription", item_id=str(subscription.pk), activity="deleted"
+        )
+        self.assertEqual(deletion_log.user_id, self.user.id)
+        unrelated.refresh_from_db()
+        self.assertFalse(unrelated.deleted)
+
     def test_bulk_delete_reports_unknown_ids_as_skipped(self) -> None:
         insight = self._create_insight()
 

tach.toml

@@ -714,6 +714,7 @@ depends_on = [
     "products.alerts",
     "products.dashboards",
     "products.event_definitions",
+    "products.exports",
     "products.annotations",
     "products.warehouse_sources", "products.access_control",
     # A trends query filtering on a first-pageview-aware property builds it through