From 955a61194e15a5e57e445106ebe5be52344a0cec Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 17 Aug 2026 13:38:08 -0400 Subject: [PATCH] =?UTF-8?q?fix(library):=20a=20fully-missing=20album=20lea?= =?UTF-8?q?ves=20the=20year=20axis=20too=20=E2=80=94=20#2702?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Filed as a product decision, but the code had already made it: the genre queries filter tracks.missing_since inside their EXISTS, so an album whose every file had gone was ALREADY absent from genre while still listed under its year — where opening it found nothing playable. The two browse axes disagreed, and whichever answer won, one of them had to change. Hiding is the answer. Browsing is how you go looking for something to play, and the rule for that case is to take it out of view; the admin missing-files surface is where absence gets reported, with far more detail than a silent gap in a grid. It also means changing the axis that was inconsistent rather than the one that was already right. All three year queries move together — index, list and count. That is the invariant #367 needed care for at the genre level: if the index groups differently from the filter, a year leads to an empty page, and if the count disagrees with the list then "Load more" promises rows that never arrive. The predicate is "has at least one playable track", which also excludes an album carrying no tracks at all. Same answer for the same reason — nothing to play, nothing to browse to — and it is what genre has always done, since an album with no tracks contributes no genres either. That last part changed two existing tests, which had been seeding trackless albums as a convenience. Their intent (undated albums never appear in a range) is untouched; they now seed a track each, which is what a real album looks like anyway. Two new tests pin the actual behaviour: a fully-missing album leaves the axis while a half-missing one stays, and the count agrees with the filtered list. --- internal/api/library_browse_test.go | 118 ++++++++++++++++++++++++++-- internal/db/dbq/browse.sql.go | 35 ++++++++- internal/db/queries/browse.sql | 37 +++++++-- 3 files changed, 174 insertions(+), 16 deletions(-) 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