Skip to content

Commit

Permalink
fix(periodic-digest): use derived names for playlists without real na…
Browse files Browse the repository at this point in the history
…mes (#27188)
  • Loading branch information
raquelmsmith authored Jan 3, 2025
1 parent 9d22f0e commit 2082705
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 44 deletions.
24 changes: 16 additions & 8 deletions posthog/tasks/periodic_digest.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +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,
name__isnull=False,
name__gt="", # Excludes empty strings
).values("team_id", "name", "short_id")
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:
Expand Down Expand Up @@ -159,9 +168,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=[
{
Expand Down
46 changes: 10 additions & 36 deletions posthog/tasks/test/test_periodic_digest.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,17 @@ def test_periodic_digest_report(self, mock_capture: MagicMock) -> None:
team=self.team,
name="Test Playlist",
)
# These should be excluded from the digest
# 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=None,
)
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",
)

# Create experiments
Expand Down Expand Up @@ -163,7 +166,11 @@ 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,
},
],
"new_experiments_launched": [
{
Expand Down Expand Up @@ -366,36 +373,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)

0 comments on commit 2082705

Please sign in to comment.