mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-08-12 11:49:28 +00:00
v3 library: artist sort orders titles within an artist (tree-view feel) (#720)
* v3 library: artist sort orders titles within an artist (tree-view feel) Tester report: "the list is set up by artist, but the cards are alphabetical(-ish random)". Real: the tree orders artist -> album -> title, while the grid's artist sort ordered within an artist by RAW FILENAME — community-pack filename noise, so an artist's cards looked shuffled. - artist / artist-desc gain a title secondary (direction baked per entry so the legacy `dir=desc` append can't land on the title term; titles stay A->Z under Z->A artists). - The two-term (value, filename) keyset cursor can't seek a three-term order, so artist sorts leave _KEYSET_SORTS and page by OFFSET — measured trivial at real library sizes; title/recent keep their keyset. Restore via a composite sort-key column if 50k-song libraries ever hurt. - The tree view says "List view groups by artist — the selected sort applies to the card grid" when a non-artist sort is active, instead of silently ignoring the picker. - Keyset proof-tests repinned to the title sort (same property, a sort that still keysets); 2 new tests pin the title-within-artist order and the OFFSET pagination's no-skip/no-dupe across pages. Full-suite failure set identical to the same-main baseline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nm7tHs1Yvjjtnnu4nzJgdN * v3 library: honor legacy sort=artist&dir=desc (fold dir into effective sort) Codex/review follow-up to the title-within-artist change: the new artist ORDER BY bakes in `ASC` (for the title secondary), so the global `dir=desc` append is suppressed and `sort=artist&dir=desc` silently returned A->Z instead of Z->A — a regression on the legacy /api/library dir contract. Fold `dir=desc` into the canonical sort key BEFORE the sort_map lookup via the existing _effective_keyset_sort helper (same fold the cursor side already does), so the ORDER BY is built from the effective sort. Only artist/title fold (they have `-desc` twins); title/recent/tuning/year/mastery are unaffected — verified by the keyset/filter suites. New test pins that legacy `sort=artist&dir=desc` matches the explicit `artist-desc` ordering (Z->A artists, A->Z titles within each). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: byrongamatos <xasiklas@gmail.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
byrongamatos
parent
7564934d06
commit
55060c4f67
@@ -0,0 +1,81 @@
|
||||
"""The grid's artist sort orders WITHIN an artist by title (tree-view feel),
|
||||
not raw filename — the tester's "list is organised by artist, but the cards
|
||||
look alphabetical/random" report. Artist sorts page by OFFSET now (the title
|
||||
secondary can't ride the two-term keyset cursor), so pagination across an
|
||||
artist boundary is pinned too."""
|
||||
|
||||
import importlib
|
||||
import sys
|
||||
|
||||
import pytest
|
||||
from fastapi.testclient import TestClient
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def server(tmp_path, monkeypatch, isolate_logging):
|
||||
monkeypatch.setenv("CONFIG_DIR", str(tmp_path))
|
||||
monkeypatch.setenv("FEEDBACK_SKIP_STARTUP_TASKS", "1")
|
||||
sys.modules.pop("server", None)
|
||||
srv = importlib.import_module("server")
|
||||
try:
|
||||
yield srv
|
||||
finally:
|
||||
conn = getattr(getattr(srv, "meta_db", None), "conn", None)
|
||||
if conn is not None:
|
||||
conn.close()
|
||||
sys.modules.pop("server", None)
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def client(server):
|
||||
return TestClient(server.app)
|
||||
|
||||
|
||||
def _put(server, fn, artist, title):
|
||||
server.meta_db.put(fn, 0, 0, {
|
||||
"title": title, "artist": artist, "album": "", "year": "",
|
||||
"duration": 100, "arrangements": [{"name": "Lead", "index": 0}],
|
||||
})
|
||||
|
||||
|
||||
def test_artist_sort_orders_titles_within_artist(server, client):
|
||||
# Filenames deliberately REVERSE the title order, so filename ordering
|
||||
# (the old behaviour) and title ordering disagree.
|
||||
_put(server, "z1.sloppak", "Alpha Band", "Aardvark")
|
||||
_put(server, "a9.sloppak", "Alpha Band", "Zebra")
|
||||
_put(server, "m5.sloppak", "Alpha Band", "Mango")
|
||||
_put(server, "q1.sloppak", "Beta Band", "Only Song")
|
||||
body = client.get("/api/library", params={"sort": "artist", "size": 50}).json()
|
||||
assert [s["title"] for s in body["songs"]] == ["Aardvark", "Mango", "Zebra", "Only Song"]
|
||||
# Z→A flips the ARTIST order; titles stay A→Z within each artist.
|
||||
body = client.get("/api/library", params={"sort": "artist-desc", "size": 50}).json()
|
||||
assert [s["title"] for s in body["songs"]] == ["Only Song", "Aardvark", "Mango", "Zebra"]
|
||||
|
||||
|
||||
def test_legacy_dir_desc_flips_artist_like_artist_desc(server, client):
|
||||
# The legacy `sort=artist&dir=desc` shape must match the explicit
|
||||
# `artist-desc` key: the artist clause now bakes in ASC (for the title
|
||||
# secondary), so `dir=desc` is folded into the effective sort BEFORE the
|
||||
# ORDER BY lookup — otherwise the append is suppressed and dir=desc would
|
||||
# silently return A→Z.
|
||||
_put(server, "z1.sloppak", "Alpha Band", "Aardvark")
|
||||
_put(server, "a9.sloppak", "Alpha Band", "Zebra")
|
||||
_put(server, "m5.sloppak", "Alpha Band", "Mango")
|
||||
_put(server, "q1.sloppak", "Beta Band", "Only Song")
|
||||
legacy = client.get("/api/library", params={"sort": "artist", "dir": "desc", "size": 50}).json()
|
||||
explicit = client.get("/api/library", params={"sort": "artist-desc", "size": 50}).json()
|
||||
assert [s["title"] for s in legacy["songs"]] == ["Only Song", "Aardvark", "Mango", "Zebra"]
|
||||
assert [s["title"] for s in legacy["songs"]] == [s["title"] for s in explicit["songs"]]
|
||||
|
||||
|
||||
def test_artist_sort_offset_pagination_no_skip_or_dupe(server, client):
|
||||
for i in range(7):
|
||||
_put(server, f"f{6 - i}.sloppak", "One Artist", f"Title {chr(65 + i)}")
|
||||
seen = []
|
||||
for page in range(4):
|
||||
body = client.get("/api/library", params={"sort": "artist", "size": 2, "page": page}).json()
|
||||
seen += [s["title"] for s in body["songs"]]
|
||||
assert seen == [f"Title {chr(65 + i)}" for i in range(7)]
|
||||
# And no keyset cursor is offered for artist sorts (OFFSET path).
|
||||
body = client.get("/api/library", params={"sort": "artist", "size": 2}).json()
|
||||
assert body["next_cursor"] is None
|
||||
@@ -184,7 +184,9 @@ def test_grouped_keyset_pagination_with_intrinsic_filter(client, server):
|
||||
arrangements=1, tuning="Drop D")
|
||||
seen, cursor = [], None
|
||||
for _ in range(10):
|
||||
params = {"group": 1, "tunings": "Drop D", "size": 2, "sort": "artist"}
|
||||
# Title sort — the keyset proof needs a sort that still keysets
|
||||
# (artist sorts page by OFFSET since the title-secondary change).
|
||||
params = {"group": 1, "tunings": "Drop D", "size": 2, "sort": "title"}
|
||||
if cursor:
|
||||
params["after"] = cursor
|
||||
body = client.get("/api/library", params=params).json()
|
||||
|
||||
@@ -73,7 +73,10 @@ def _walk_offset(client, sort, size, total):
|
||||
return seen
|
||||
|
||||
|
||||
@pytest.mark.parametrize("sort", ["artist", "artist-desc", "title", "title-desc", "recent"])
|
||||
# artist/artist-desc left out: their ORDER BY carries a title secondary
|
||||
# (grid cards read alphabetically within an artist, like the tree), which
|
||||
# the two-term cursor cannot seek — they page by OFFSET (covered below).
|
||||
@pytest.mark.parametrize("sort", ["title", "title-desc", "recent"])
|
||||
def test_keyset_matches_offset_exactly(client, server_mod, sort):
|
||||
_seed(server_mod, 25)
|
||||
offset_order = _walk_offset(client, sort, 7, 25)
|
||||
@@ -87,15 +90,17 @@ def test_stable_tiebreak_on_equal_keys(client, server_mod):
|
||||
# 25 songs, all the SAME artist → the artist sort is decided entirely by the
|
||||
# filename tiebreak. Both pagers must still cover all 25 with no dupe.
|
||||
_seed(server_mod, 25, shared_artist=True)
|
||||
keyset_order = _walk_keyset(client, "artist", 6, 25)
|
||||
keyset_order = _walk_offset(client, "artist", 6, 25)
|
||||
assert len(keyset_order) == 25 and len(set(keyset_order)) == 25
|
||||
assert keyset_order == sorted(keyset_order) # tiebreak is filename ASC
|
||||
|
||||
|
||||
def test_first_page_has_cursor_and_no_after_is_offset(client, server_mod):
|
||||
_seed(server_mod, 5)
|
||||
body = client.get("/api/library", params={"sort": "title", "size": 2}).json()
|
||||
assert body["next_cursor"] # keyset sort: cursor offered
|
||||
body = client.get("/api/library", params={"sort": "artist", "size": 2}).json()
|
||||
assert body["next_cursor"] # cursor offered
|
||||
assert body["next_cursor"] is None # artist sorts page by OFFSET
|
||||
assert [s["filename"] for s in body["songs"]] == ["song00.archive", "song01.archive"]
|
||||
|
||||
|
||||
@@ -111,7 +116,7 @@ def test_legacy_dir_desc_keysets_correctly(client, server_mod):
|
||||
_seed(server_mod, 20)
|
||||
offset_order, page = [], 0
|
||||
while True:
|
||||
body = client.get("/api/library", params={"sort": "artist", "dir": "desc", "size": 6, "page": page}).json()
|
||||
body = client.get("/api/library", params={"sort": "title", "dir": "desc", "size": 6, "page": page}).json()
|
||||
if not body["songs"]:
|
||||
break
|
||||
offset_order.extend(s["filename"] for s in body["songs"])
|
||||
@@ -119,7 +124,7 @@ def test_legacy_dir_desc_keysets_correctly(client, server_mod):
|
||||
keyset, cursor, guard = [], "", 0
|
||||
while len(keyset) < 20 and guard < 25:
|
||||
guard += 1
|
||||
params = {"sort": "artist", "dir": "desc", "size": 6}
|
||||
params = {"sort": "title", "dir": "desc", "size": 6}
|
||||
if cursor:
|
||||
params["after"] = cursor
|
||||
body = client.get("/api/library", params=params).json()
|
||||
@@ -131,7 +136,7 @@ def test_legacy_dir_desc_keysets_correctly(client, server_mod):
|
||||
assert len(set(keyset)) == 20
|
||||
|
||||
|
||||
@pytest.mark.parametrize("sort", ["artist", "artist-desc", "recent"])
|
||||
@pytest.mark.parametrize("sort", ["recent"])
|
||||
def test_keyset_handles_null_sort_keys(client, server_mod, sort):
|
||||
# NULL artist/mtime (corrupt/legacy rows past put()'s '' defaults) sort
|
||||
# first in ASC / last in DESC; keyset must cover them exactly like OFFSET.
|
||||
|
||||
@@ -158,11 +158,13 @@ def test_grouped_keyset_pagination(client, server):
|
||||
for i in range(5):
|
||||
_put(server, f"w{i}_a.archive", f"Song {i}", "Artist")
|
||||
_put(server, f"w{i}_b.archive", f"Song {i}", "Artist") # 2 charts per work
|
||||
body = client.get("/api/library", params={"group": 1, "size": 3}).json()
|
||||
# Title sort: artist sorts page by OFFSET now (title-secondary ordering),
|
||||
# and this test PROVES the grouped keyset, so it pins a keyset sort.
|
||||
body = client.get("/api/library", params={"group": 1, "size": 3, "sort": "title"}).json()
|
||||
assert body["total"] == 5 and len(body["songs"]) == 3
|
||||
cur = body["next_cursor"]
|
||||
assert cur
|
||||
body2 = client.get("/api/library", params={"group": 1, "size": 3, "after": cur}).json()
|
||||
body2 = client.get("/api/library", params={"group": 1, "size": 3, "sort": "title", "after": cur}).json()
|
||||
assert len(body2["songs"]) == 2
|
||||
p1 = {s["filename"] for s in body["songs"]}
|
||||
p2 = {s["filename"] for s in body2["songs"]}
|
||||
|
||||
Reference in New Issue
Block a user