diff --git a/internal/api/library_browse_test.go b/internal/api/library_browse_test.go index dee9b7ed..1b591a5d 100644 --- a/internal/api/library_browse_test.go +++ b/internal/api/library_browse_test.go @@ -1,11 +1,16 @@ package api import ( + "context" "encoding/json" "net/http" "net/http/httptest" "net/url" "testing" + + "github.com/jackc/pgx/v5/pgtype" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" ) // parseYearFilter is pure, so this runs in the fast lane rather than waiting @@ -218,11 +223,21 @@ func TestListLibraryAlbums_GenreWithSlashSurvives(t *testing.T) { func TestListLibraryAlbums_YearRangeFilter(t *testing.T) { h, pool := testHandlers(t) artist := seedArtist(t, pool, "Chronology") - seedAlbum(t, pool, artist.ID, "Old Record", 1972) - seedAlbum(t, pool, artist.ID, "Middle Record", 1995) - seedAlbum(t, pool, artist.ID, "New Record", 2020) - // An undated album must not appear in ANY year range. - seedAlbum(t, pool, artist.ID, "Undated Record", 0) + // Each album needs a playable track: since #2702 the year axis lists only + // albums with something to play, matching what the genre axis already did. + for _, a := range []struct { + title string + year int + }{ + {"Old Record", 1972}, + {"Middle Record", 1995}, + {"New Record", 2020}, + // An undated album must not appear in ANY year range. + {"Undated Record", 0}, + } { + album := seedAlbum(t, pool, artist.ID, a.title, a.year) + seedTrack(t, pool, album.ID, artist.ID, a.title+" T1", 1, 120_000) + } titles := func(query string) map[string]bool { t.Helper() @@ -281,8 +296,10 @@ func TestListLibraryAlbums_RejectsGenreAndYearTogether(t *testing.T) { func TestListAlbumYears_ExcludesUndatedAlbums(t *testing.T) { h, pool := testHandlers(t) artist := seedArtist(t, pool, "Years Only") - seedAlbum(t, pool, artist.ID, "Dated One", 1984) - seedAlbum(t, pool, artist.ID, "No Date", 0) + dated := seedAlbum(t, pool, artist.ID, "Dated One", 1984) + seedTrack(t, pool, dated.ID, artist.ID, "Dated T1", 1, 120_000) + undated := seedAlbum(t, pool, artist.ID, "No Date", 0) + seedTrack(t, pool, undated.ID, artist.ID, "Undated T1", 1, 120_000) req := httptest.NewRequest(http.MethodGet, "/api/library/years", nil) w := httptest.NewRecorder() @@ -323,3 +340,90 @@ func keysOf(m map[string]bool) []string { } return out } + +// #2702: an album whose every file has gone leaves the year axis, matching +// what the genre axis already did. Before this the two disagreed — the same +// album was absent from genre and still listed under its year, where opening +// it found nothing playable. +func TestListAlbumYears_ExcludesFullyMissingAlbums(t *testing.T) { + h, pool := testHandlers(t) + q := dbq.New(pool) + artist := seedArtist(t, pool, "Gone Records") + + // Every file missing — should vanish from the axis entirely. + dead := seedAlbum(t, pool, artist.ID, "All Gone", 1991) + deadTrack := seedTrack(t, pool, dead.ID, artist.ID, "Gone A", 1, 120_000) + // One of two missing — the album still has something to play, so it stays. + partial := seedAlbum(t, pool, artist.ID, "Half Gone", 1992) + partialGone := seedTrack(t, pool, partial.ID, artist.ID, "Half A", 1, 120_000) + seedTrack(t, pool, partial.ID, artist.ID, "Half B", 2, 120_000) + + if _, err := q.MarkTracksMissing( + context.Background(), []pgtype.UUID{deadTrack.ID, partialGone.ID}, + ); err != nil { + t.Fatalf("mark missing: %v", err) + } + + req := httptest.NewRequest(http.MethodGet, "/api/library/years", nil) + w := httptest.NewRecorder() + h.handleListAlbumYears(w, req) + var years []yearCount + if err := json.NewDecoder(w.Body).Decode(&years); err != nil { + t.Fatalf("decode years: %v", err) + } + for _, y := range years { + if y.Year == 1991 { + t.Error("1991 still on the axis — its only album has no playable files") + } + } + found1992 := false + for _, y := range years { + if y.Year == 1992 { + found1992 = true + } + } + if !found1992 { + t.Error("1992 missing — its album still has a playable track") + } +} + +// The index, the list and the count must agree. #367 needed care for exactly +// this reason at the genre level: if they diverge, a year leads to an empty +// page or "Load more" promises rows that never arrive. +func TestListLibraryAlbums_YearFilterAndCountAgreeOnMissing(t *testing.T) { + h, pool := testHandlers(t) + q := dbq.New(pool) + artist := seedArtist(t, pool, "Agreement") + + dead := seedAlbum(t, pool, artist.ID, "Vanished", 2003) + deadTrack := seedTrack(t, pool, dead.ID, artist.ID, "Vanished A", 1, 120_000) + alive := seedAlbum(t, pool, artist.ID, "Present", 2003) + seedTrack(t, pool, alive.ID, artist.ID, "Present A", 1, 120_000) + + if _, err := q.MarkTracksMissing( + context.Background(), []pgtype.UUID{deadTrack.ID}, + ); err != nil { + t.Fatalf("mark missing: %v", err) + } + + req := httptest.NewRequest(http.MethodGet, "/api/library/albums?year_from=2003&year_to=2003&limit=200", nil) + w := httptest.NewRecorder() + h.handleListLibraryAlbums(w, req) + var page Page[AlbumRef] + if err := json.NewDecoder(w.Body).Decode(&page); err != nil { + t.Fatalf("decode: %v", err) + } + + for _, a := range page.Items { + if a.Title == "Vanished" { + t.Error("a fully-missing album is still listed under its year") + } + } + if len(page.Items) != 1 { + t.Fatalf("want 1 album listed, got %d", len(page.Items)) + } + // The count drives paging; a stale one is how "Load more" starts lying. + if page.Total != 1 { + t.Errorf("total = %d, want 1 — the count must match the filtered list", page.Total) + } +} diff --git a/internal/db/dbq/browse.sql.go b/internal/db/dbq/browse.sql.go index bf428524..3b22be31 100644 --- a/internal/db/dbq/browse.sql.go +++ b/internal/db/dbq/browse.sql.go @@ -38,6 +38,11 @@ SELECT COUNT(*) FROM albums WHERE release_date IS NOT NULL AND EXTRACT(YEAR FROM release_date)::int BETWEEN $1::int AND $2::int + AND EXISTS ( + SELECT 1 FROM tracks + WHERE tracks.album_id = albums.id + AND tracks.missing_since IS NULL + ) ` type CountAlbumsByYearRangeParams struct { @@ -56,6 +61,11 @@ const listAlbumYearsWithCount = `-- name: ListAlbumYearsWithCount :many SELECT EXTRACT(YEAR FROM release_date)::int AS year, COUNT(*)::bigint AS album_count FROM albums WHERE release_date IS NOT NULL + AND EXISTS ( + SELECT 1 FROM tracks + WHERE tracks.album_id = albums.id + AND tracks.missing_since IS NULL + ) GROUP BY year ORDER BY year DESC ` @@ -161,6 +171,11 @@ JOIN artists ON artists.id = albums.artist_id WHERE albums.release_date IS NOT NULL AND EXTRACT(YEAR FROM albums.release_date)::int BETWEEN $1::int AND $2::int + AND EXISTS ( + SELECT 1 FROM tracks + WHERE tracks.album_id = albums.id + AND tracks.missing_since IS NULL + ) ORDER BY albums.sort_title, albums.id LIMIT $4 OFFSET $3 ` @@ -304,10 +319,22 @@ type ListGenresWithCountRow struct { // Browsing is a way of finding something to play, so a track that cannot play // should not shape it. // -// Year queries below join albums only and are deliberately left alone: an album -// is still a real release even if some of its tracks are gone. An album whose -// EVERY track is missing will linger on the year axis; that's a narrower case, -// tracked with the rest of the cleanup work. +// The year axis filters too (#2702). It used to join albums only, on the +// reasoning that an album is a real release even when some of its tracks are +// gone — true, but it left the two browse axes disagreeing: the genre queries +// above filter tracks.missing_since, so an album whose every file had vanished +// was already absent from genre while still listed under its year, where +// opening it found nothing playable. +// +// Hiding is the answer rather than showing-and-marking because browsing is how +// you go looking for something to play, and the operator's rule for that case +// is to take it out of view; the admin missing-files surface is where absence +// is reported. It also means changing the axis that was inconsistent rather +// than the one that was already right. +// +// The predicate is "has at least one playable track", so it also excludes an +// album with no tracks at all. That is the same answer for the same reason: +// nothing to play, nothing to browse to. // Genre browse index (#367). // // Genres live inline on tracks.genre as a delimited string, so this splits on diff --git a/internal/db/queries/browse.sql b/internal/db/queries/browse.sql index 00d47d29..4b3d9ced 100644 --- a/internal/db/queries/browse.sql +++ b/internal/db/queries/browse.sql @@ -5,10 +5,22 @@ -- Browsing is a way of finding something to play, so a track that cannot play -- should not shape it. -- --- Year queries below join albums only and are deliberately left alone: an album --- is still a real release even if some of its tracks are gone. An album whose --- EVERY track is missing will linger on the year axis; that's a narrower case, --- tracked with the rest of the cleanup work. +-- The year axis filters too (#2702). It used to join albums only, on the +-- reasoning that an album is a real release even when some of its tracks are +-- gone — true, but it left the two browse axes disagreeing: the genre queries +-- above filter tracks.missing_since, so an album whose every file had vanished +-- was already absent from genre while still listed under its year, where +-- opening it found nothing playable. +-- +-- Hiding is the answer rather than showing-and-marking because browsing is how +-- you go looking for something to play, and the operator's rule for that case +-- is to take it out of view; the admin missing-files surface is where absence +-- is reported. It also means changing the axis that was inconsistent rather +-- than the one that was already right. +-- +-- The predicate is "has at least one playable track", so it also excludes an +-- album with no tracks at all. That is the same answer for the same reason: +-- nothing to play, nothing to browse to. -- name: ListGenresWithCount :many -- Genre browse index (#367). @@ -82,6 +94,11 @@ WHERE EXISTS ( SELECT EXTRACT(YEAR FROM release_date)::int AS year, COUNT(*)::bigint AS album_count FROM albums WHERE release_date IS NOT NULL + AND EXISTS ( + SELECT 1 FROM tracks + WHERE tracks.album_id = albums.id + AND tracks.missing_since IS NULL + ) GROUP BY year ORDER BY year DESC; @@ -93,6 +110,11 @@ JOIN artists ON artists.id = albums.artist_id WHERE albums.release_date IS NOT NULL AND EXTRACT(YEAR FROM albums.release_date)::int BETWEEN sqlc.arg(year_from)::int AND sqlc.arg(year_to)::int + AND EXISTS ( + SELECT 1 FROM tracks + WHERE tracks.album_id = albums.id + AND tracks.missing_since IS NULL + ) ORDER BY albums.sort_title, albums.id LIMIT sqlc.arg(lim) OFFSET sqlc.arg(off); @@ -100,7 +122,12 @@ LIMIT sqlc.arg(lim) OFFSET sqlc.arg(off); SELECT COUNT(*) FROM albums WHERE release_date IS NOT NULL AND EXTRACT(YEAR FROM release_date)::int - BETWEEN sqlc.arg(year_from)::int AND sqlc.arg(year_to)::int; + BETWEEN sqlc.arg(year_from)::int AND sqlc.arg(year_to)::int + AND EXISTS ( + SELECT 1 FROM tracks + WHERE tracks.album_id = albums.id + AND tracks.missing_since IS NULL + ); -- name: ListGenresForAlbum :many -- Distinct genres carried by an album's tracks, for the album detail page's