From 3fb091123193682f426903269e6bcec3946db216 Mon Sep 17 00:00:00 2001 From: Raquel Smith Date: Mon, 30 Dec 2024 10:27:15 -0800 Subject: [PATCH 1/2] use derived names for playlists without real names --- posthog/tasks/periodic_digest.py | 7 +-- posthog/tasks/test/test_periodic_digest.py | 51 ++++++---------------- 2 files changed, 16 insertions(+), 42 deletions(-) diff --git a/posthog/tasks/periodic_digest.py b/posthog/tasks/periodic_digest.py index 7fe4eb440bbe2..136c65a171101 100644 --- a/posthog/tasks/periodic_digest.py +++ b/posthog/tasks/periodic_digest.py @@ -69,9 +69,7 @@ def get_teams_with_new_playlists(end: datetime, begin: datetime) -> QuerySet: return SessionRecordingPlaylist.objects.filter( created_at__gt=begin, created_at__lte=end, - name__isnull=False, - name__gt="", # Excludes empty strings - ).values("team_id", "name", "short_id") + ).values("team_id", "name", "short_id", "derived_name") def get_teams_with_new_experiments_launched(end: datetime, begin: datetime) -> QuerySet: @@ -151,9 +149,8 @@ def get_periodic_digest_report(all_digest_data: dict[str, Any], team: Team) -> p for event_definition in all_digest_data["teams_with_new_event_definitions"].get(team.id, []) ], new_playlists=[ - {"name": playlist.get("name"), "id": playlist.get("short_id")} + {"name": playlist.get("name") or playlist.get("derived_name", "Untitled"), "id": playlist.get("short_id")} for playlist in all_digest_data["teams_with_new_playlists"].get(team.id, []) - if playlist.get("name") # Extra safety check to exclude any playlists without names ], new_experiments_launched=[ { diff --git a/posthog/tasks/test/test_periodic_digest.py b/posthog/tasks/test/test_periodic_digest.py index 6a4afdac8d155..e492e1ef95658 100644 --- a/posthog/tasks/test/test_periodic_digest.py +++ b/posthog/tasks/test/test_periodic_digest.py @@ -49,14 +49,16 @@ def test_periodic_digest_report(self, mock_capture: MagicMock) -> None: team=self.team, name="Test Playlist", ) - # These should be excluded from the digest - SessionRecordingPlaylist.objects.create( + # These should be included in the digest but use the derived name + derived_playlist = SessionRecordingPlaylist.objects.create( team=self.team, name=None, + derived_name="Derived Playlist", ) - SessionRecordingPlaylist.objects.create( + derived_playlist_2 = SessionRecordingPlaylist.objects.create( team=self.team, name="", + derived_name="Derived Playlist 2", ) # Create experiments @@ -163,7 +165,15 @@ def test_periodic_digest_report(self, mock_capture: MagicMock) -> None: { "name": "Test Playlist", "id": playlist.short_id, - } + }, + { + "name": "Derived Playlist", + "id": derived_playlist.short_id, + }, + { + "name": "Derived Playlist 2", + "id": derived_playlist_2.short_id, + }, ], "new_experiments_launched": [ { @@ -366,36 +376,3 @@ def test_periodic_digest_dry_run_no_record(self, mock_capture: MagicMock) -> Non # Verify no capture call and no messaging record mock_capture.delay.assert_not_called() self.assertEqual(MessagingRecord.objects.count(), 0) - - @freeze_time("2024-01-20T00:01:00Z") - @patch("posthog.tasks.periodic_digest.capture_report") - def test_periodic_digest_excludes_playlists_without_names(self, mock_capture: MagicMock) -> None: - # Create test data from "last week" - with freeze_time("2024-01-15T00:01:00Z"): - # Create playlists with various name states - valid_playlist = SessionRecordingPlaylist.objects.create( - team=self.team, - name="Valid Playlist", - ) - SessionRecordingPlaylist.objects.create( - team=self.team, - name=None, # Null name should be excluded - ) - SessionRecordingPlaylist.objects.create( - team=self.team, - name="", # Empty string name should be excluded - ) - - # Run the periodic digest report task - send_all_periodic_digest_reports() - - # Extract the playlists from the capture call - call_args = mock_capture.delay.call_args - self.assertIsNotNone(call_args) - full_report_dict = call_args[1]["full_report_dict"] - playlists = full_report_dict["new_playlists"] - - # Verify only the valid playlist is included - self.assertEqual(len(playlists), 1) - self.assertEqual(playlists[0]["name"], "Valid Playlist") - self.assertEqual(playlists[0]["id"], valid_playlist.short_id) From 6c8a3d4ddec59146b2e3ee134b06de715e7f3535 Mon Sep 17 00:00:00 2001 From: Raquel Smith Date: Mon, 30 Dec 2024 10:33:44 -0800 Subject: [PATCH 2/2] exclude playlists with no name and no derived name --- posthog/tasks/periodic_digest.py | 19 +++++++++++++++---- posthog/tasks/test/test_periodic_digest.py | 15 ++++++--------- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/posthog/tasks/periodic_digest.py b/posthog/tasks/periodic_digest.py index 136c65a171101..c1d9e020f0a94 100644 --- a/posthog/tasks/periodic_digest.py +++ b/posthog/tasks/periodic_digest.py @@ -66,10 +66,21 @@ def get_teams_with_new_event_definitions(end: datetime, begin: datetime) -> Quer def get_teams_with_new_playlists(end: datetime, begin: datetime) -> QuerySet: - return SessionRecordingPlaylist.objects.filter( - created_at__gt=begin, - created_at__lte=end, - ).values("team_id", "name", "short_id", "derived_name") + return ( + SessionRecordingPlaylist.objects.filter( + created_at__gt=begin, + created_at__lte=end, + ) + .exclude( + name__isnull=True, + derived_name__isnull=True, + ) + .exclude( + name="", + derived_name="", + ) + .values("team_id", "name", "short_id", "derived_name") + ) def get_teams_with_new_experiments_launched(end: datetime, begin: datetime) -> QuerySet: diff --git a/posthog/tasks/test/test_periodic_digest.py b/posthog/tasks/test/test_periodic_digest.py index e492e1ef95658..f03da75905224 100644 --- a/posthog/tasks/test/test_periodic_digest.py +++ b/posthog/tasks/test/test_periodic_digest.py @@ -49,16 +49,17 @@ def test_periodic_digest_report(self, mock_capture: MagicMock) -> None: team=self.team, name="Test Playlist", ) - # These should be included in the digest but use the derived name - derived_playlist = SessionRecordingPlaylist.objects.create( + # This should be excluded from the digest because it has no name and no derived name + SessionRecordingPlaylist.objects.create( team=self.team, name=None, - derived_name="Derived Playlist", + derived_name=None, ) - derived_playlist_2 = SessionRecordingPlaylist.objects.create( + # This should be included in the digest but use the derived name + derived_playlist = SessionRecordingPlaylist.objects.create( team=self.team, name="", - derived_name="Derived Playlist 2", + derived_name="Derived Playlist", ) # Create experiments @@ -170,10 +171,6 @@ def test_periodic_digest_report(self, mock_capture: MagicMock) -> None: "name": "Derived Playlist", "id": derived_playlist.short_id, }, - { - "name": "Derived Playlist 2", - "id": derived_playlist_2.short_id, - }, ], "new_experiments_launched": [ {