From 583789850cb65664144498e643df0e0960ae4080 Mon Sep 17 00:00:00 2001 From: Claudio Ortolina Date: Sun, 15 Feb 2026 20:35:23 +0000 Subject: [PATCH] Normalize top artist and top album counts - Make sure joins don't discard tracks - Make sure we group by names as MB ids can be empty --- lib/music_library/scrobble_activity.ex | 26 ++-- test/music_library/scrobble_activity_test.exs | 126 ++++++++++++++++++ 2 files changed, 134 insertions(+), 18 deletions(-) diff --git a/lib/music_library/scrobble_activity.ex b/lib/music_library/scrobble_activity.ex index 393fbad2..e2c55da2 100644 --- a/lib/music_library/scrobble_activity.ex +++ b/lib/music_library/scrobble_activity.ex @@ -352,26 +352,21 @@ defmodule MusicLibrary.ScrobbleActivity do @doc """ Gets the top artists by scrobble count across all time. Returns a list of maps with artist information and play counts. - - For a lot of artists, Last.fm doesn't have the correct artist musicbrainz_id - (often it's a ""), which makes this query not work as expected at the JOIN - level. To fix that, we need to update the data via Scrobble Rules. """ def get_top_artists(opts) do limit = Keyword.get(opts, :limit, 10) query = from t in Track, - join: ai in Artists.ArtistInfo, + left_join: ai in Artists.ArtistInfo, on: ai.id == fragment("json_extract(?, '$.musicbrainz_id')", t.artist), group_by: [ - fragment("json_extract(artist, '$.name')"), - fragment("json_extract(artist, '$.musicbrainz_id')") + fragment("json_extract(artist, '$.name')") ], select: %{ name: fragment("json_extract(artist, '$.name')"), - musicbrainz_id: fragment("json_extract(artist, '$.musicbrainz_id')"), - image_hash: ai.image_data_hash, + musicbrainz_id: max(fragment("json_extract(artist, '$.musicbrainz_id')")), + image_hash: max(ai.image_data_hash), play_count: count(t.scrobbled_at_uts, :distinct) }, order_by: [desc: count(t.scrobbled_at_uts, :distinct)], @@ -383,10 +378,6 @@ defmodule MusicLibrary.ScrobbleActivity do @doc """ Gets the top artists by scrobble count for the given number of days. Returns a list of maps with artist information and play counts. - - For a lot of artists, Last.fm doesn't have the correct artist musicbrainz_id - (often it's a ""), which makes this query not work as expected at the JOIN - level. To fix that, we need to update the data via Scrobble Rules. """ def get_top_artists_by_days(days, opts) do limit = Keyword.get(opts, :limit, 10) @@ -402,17 +393,16 @@ defmodule MusicLibrary.ScrobbleActivity do query = from t in Track, - join: ai in Artists.ArtistInfo, + left_join: ai in Artists.ArtistInfo, on: ai.id == fragment("json_extract(?, '$.musicbrainz_id')", t.artist), where: t.scrobbled_at_uts >= ^cutoff_timestamp, group_by: [ - fragment("json_extract(artist, '$.name')"), - fragment("json_extract(artist, '$.musicbrainz_id')") + fragment("json_extract(artist, '$.name')") ], select: %{ name: fragment("json_extract(artist, '$.name')"), - musicbrainz_id: fragment("json_extract(artist, '$.musicbrainz_id')"), - image_hash: ai.image_data_hash, + musicbrainz_id: max(fragment("json_extract(artist, '$.musicbrainz_id')")), + image_hash: max(ai.image_data_hash), play_count: count(t.scrobbled_at_uts, :distinct) }, order_by: [desc: count(t.scrobbled_at_uts, :distinct)], diff --git a/test/music_library/scrobble_activity_test.exs b/test/music_library/scrobble_activity_test.exs index ee754f21..01af99e2 100644 --- a/test/music_library/scrobble_activity_test.exs +++ b/test/music_library/scrobble_activity_test.exs @@ -271,6 +271,132 @@ defmodule MusicLibrary.ScrobbleActivityTest do end end + describe "get_top_artists/1" do + test "counts tracks with missing artist_infos records" do + artist_mbid = Ecto.UUID.generate() + + # Create an artist_info record for the known musicbrainz_id + MusicLibrary.Repo.insert!(%MusicLibrary.Artists.ArtistInfo{ + id: artist_mbid, + musicbrainz_data: %{"name" => "Thin Lizzy"} + }) + + now = System.system_time(:second) + + # Two tracks with a matching artist_infos record + track_fixture(%{ + artist_name: "Thin Lizzy", + artist_musicbrainz_id: artist_mbid, + title: "The Boys Are Back in Town", + scrobbled_at_uts: now - 100 + }) + + track_fixture(%{ + artist_name: "Thin Lizzy", + artist_musicbrainz_id: artist_mbid, + title: "Jailbreak", + scrobbled_at_uts: now - 200 + }) + + # One track with an empty musicbrainz_id (no matching artist_infos) + track_fixture(%{ + artist_name: "Thin Lizzy", + artist_musicbrainz_id: "", + title: "Rosalie / Cowgirl's Song", + scrobbled_at_uts: now - 300 + }) + + results = ScrobbleActivity.get_top_artists(limit: 10) + + # All 3 tracks should be counted as a single artist entry + assert [thin_lizzy] = Enum.filter(results, fn r -> r.name == "Thin Lizzy" end) + assert thin_lizzy.play_count == 3 + # MAX picks the real musicbrainz_id over the empty string + assert thin_lizzy.musicbrainz_id == artist_mbid + end + + test "returns image_hash from artist_infos when available" do + artist_mbid = Ecto.UUID.generate() + + MusicLibrary.Repo.insert!(%MusicLibrary.Artists.ArtistInfo{ + id: artist_mbid, + musicbrainz_data: %{"name" => "Test Artist"}, + image_data_hash: "abc123" + }) + + track_fixture(%{ + artist_name: "Test Artist", + artist_musicbrainz_id: artist_mbid, + title: "Track 1", + scrobbled_at_uts: System.system_time(:second) - 100 + }) + + [result] = ScrobbleActivity.get_top_artists(limit: 10) + + assert result.image_hash == "abc123" + end + + test "returns nil image_hash for tracks without artist_infos" do + track_fixture(%{ + artist_name: "Unknown Artist", + artist_musicbrainz_id: "", + title: "Track 1", + scrobbled_at_uts: System.system_time(:second) - 100 + }) + + [result] = ScrobbleActivity.get_top_artists(limit: 10) + + assert result.image_hash == nil + end + end + + describe "get_top_artists_by_days/2" do + test "counts tracks with missing artist_infos records within date range" do + artist_mbid = Ecto.UUID.generate() + + MusicLibrary.Repo.insert!(%MusicLibrary.Artists.ArtistInfo{ + id: artist_mbid, + musicbrainz_data: %{"name" => "Thin Lizzy"} + }) + + now = DateTime.utc_now() + now_unix = DateTime.to_unix(now) + + # Two tracks with matching artist_infos + track_fixture(%{ + artist_name: "Thin Lizzy", + artist_musicbrainz_id: artist_mbid, + title: "The Boys Are Back in Town", + scrobbled_at_uts: now_unix - 100 + }) + + track_fixture(%{ + artist_name: "Thin Lizzy", + artist_musicbrainz_id: artist_mbid, + title: "Jailbreak", + scrobbled_at_uts: now_unix - 200 + }) + + # One track with empty musicbrainz_id + track_fixture(%{ + artist_name: "Thin Lizzy", + artist_musicbrainz_id: "", + title: "Rosalie / Cowgirl's Song", + scrobbled_at_uts: now_unix - 300 + }) + + results = + ScrobbleActivity.get_top_artists_by_days(7, + limit: 10, + current_time: now, + timezone: "Etc/UTC" + ) + + assert [thin_lizzy] = Enum.filter(results, fn r -> r.name == "Thin Lizzy" end) + assert thin_lizzy.play_count == 3 + end + end + describe "integration with existing functions" do test "scrobble_count returns correct count" do initial_count = ScrobbleActivity.scrobble_count()