diff --git a/.forgejo/workflows/test-go.yml b/.forgejo/workflows/test-go.yml index cb4a405e..60b0ce60 100644 --- a/.forgejo/workflows/test-go.yml +++ b/.forgejo/workflows/test-go.yml @@ -4,8 +4,16 @@ name: test-go # dev/main and PRs to main, scoped to Go-side files only — web-only or # Flutter-only diffs don't trigger this workflow. # -# Integration tests needing Postgres/ffmpeg run locally via docker-compose; -# they should guard with testing.Short() so this short-mode run skips them. +# Two jobs: `test` (fast — vet + lint + `go test -short -race`, no DB) and +# `integration` (full `go test -race` against an ephemeral Postgres). +# +# Integration-job DB wiring follows the act_runner shared-daemon pattern: +# the runner's Docker daemon also runs the operator's dev compose stack, +# so service containers get NO published ports (collision) and no +# service-name DNS. We discover the service container by the job-scoped +# name filter via the mounted docker socket and reach it by bridge IP. +# The exactly-one assertion is a hard guard — pointing tests at the dev +# Postgres would truncate it (the disaster Fable #339 exists to prevent). # # `web/build/` has a committed placeholder index.html so go:embed succeeds # without needing the SPA to be freshly built. Real builds happen in @@ -54,3 +62,51 @@ jobs: - name: go test (short, race) run: go test -short -race ./... + + integration: + runs-on: go-ci + services: + postgres: + image: postgres:16-alpine + env: + POSTGRES_USER: minstrel + POSTGRES_PASSWORD: minstrel + POSTGRES_DB: minstrel_test + # No `ports:` — the runner shares the operator's dev compose + # Docker daemon; publishing a fixed host port collides. + + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Integration suite (discover service by bridge IP, migrate, test) + run: | + set -eux + # Discover THIS job's Postgres service container via the + # mounted docker socket. Scope by the job-name filter so the + # operator's dev compose `minstrel-postgres-*` (same image, + # same daemon) can never match. Require exactly one — abort + # loudly otherwise; a wrong target would truncate real data. + PG_LIST=$(docker ps --filter "name=integration" --filter "ancestor=postgres:16-alpine" --format '{{.ID}} {{.Names}}') + echo "candidates: ${PG_LIST:-}" + PG_COUNT=$(printf '%s\n' "$PG_LIST" | grep -c . || true) + test "$PG_COUNT" = "1" || { echo "FATAL: expected exactly 1 postgres service container, got $PG_COUNT"; exit 1; } + PG_ID=$(printf '%s' "$PG_LIST" | awk '{print $1}') + PG_NAME=$(printf '%s' "$PG_LIST" | awk '{print $2}') + case "$PG_NAME" in + *minstrel-postgres*|*_postgres_*) echo "FATAL: matched the dev compose container ($PG_NAME), refusing"; exit 1 ;; + esac + PG_IP=$(docker inspect -f '{{range .NetworkSettings.Networks}}{{.IPAddress}}{{end}}' "$PG_ID") + test -n "$PG_IP" + export MINSTREL_TEST_DATABASE_URL="postgres://minstrel:minstrel@${PG_IP}:5432/minstrel_test?sslmode=disable" + + # Wait for Postgres to accept TCP (no health-check dependency). + for i in $(seq 1 60); do (echo > "/dev/tcp/${PG_IP}/5432") 2>/dev/null && break; sleep 2; done + + # Apply embedded migrations to the fresh test DB, then run the + # full suite (no -short → integration tests execute). -p 1: + # every integration package TRUNCATEs the one shared test DB; + # concurrent package binaries → TRUNCATE deadlocks. Serialize + # package execution (the documented local invocation too). + MINSTREL_DATABASE_URL="$MINSTREL_TEST_DATABASE_URL" go run ./cmd/minstrel migrate + go test -p 1 -race ./... diff --git a/Makefile b/Makefile index ac425514..fa1e2e04 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: generate test test-short lint build +.PHONY: generate test test-short test-integration lint build SQLC_VERSION := 1.31.1 @@ -11,6 +11,16 @@ test: test-short: go test -short -race ./... +# Full suite incl. integration tests, against the dedicated minstrel_test +# DB so a run never truncates the dev `minstrel` DB (Fable #339). Ensures +# the test DB exists (idempotent — createdb errors if present, ignored). +test-integration: + docker compose up -d postgres + -docker compose exec -T postgres createdb -U minstrel minstrel_test + # -p 1: integration packages share one test DB and each TRUNCATEs it; + # concurrent package binaries deadlock on TRUNCATE. Serialize packages. + MINSTREL_TEST_DATABASE_URL=postgres://minstrel:minstrel@localhost:5432/minstrel_test?sslmode=disable go test -p 1 -race ./... + lint: golangci-lint run ./... diff --git a/README.md b/README.md index 3366afd4..58265896 100644 --- a/README.md +++ b/README.md @@ -83,6 +83,16 @@ Two concurrent dev processes: 1. **Backend:** `docker compose up` — Postgres + Minstrel on `:4533`. 2. **Frontend:** `cd web && npm install && npm run dev` — Vite dev server on `:5173` with HMR. The Vite server proxies `/api/*` and `/rest/*` to `:4533` so session cookies work. +### Testing + +- Unit + race (no DB): `make test-short`. +- Full suite incl. integration tests: `make test-integration`. This runs + against a dedicated `minstrel_test` database so a test run never + truncates your dev `minstrel` data (admin user, library, likes). It + brings up the compose Postgres and creates the test DB if missing. +- CI runs both: a fast `go test -short -race` gate plus an integration + job with its own ephemeral Postgres (`.forgejo/workflows/test-go.yml`). + ### Production build `docker build -t minstrel .` runs the SvelteKit build inside a `node` stage, copies the output into the `golang` stage, and `//go:embed`s it into the final binary. The container serves the SPA from `/` alongside the API surfaces; no separate static-file server is required. diff --git a/cmd/minstrel/admin.go b/cmd/minstrel/admin.go new file mode 100644 index 00000000..72248b40 --- /dev/null +++ b/cmd/minstrel/admin.go @@ -0,0 +1,129 @@ +package main + +import ( + "context" + "crypto/rand" + "encoding/base64" + "errors" + "flag" + "fmt" + "os" + + "golang.org/x/crypto/bcrypt" + + "git.fabledsword.com/bvandeusen/minstrel/internal/config" + "git.fabledsword.com/bvandeusen/minstrel/internal/db" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/logging" +) + +// runMigrate applies the embedded migrations and exits. The server also +// auto-migrates on startup (main.go); this standalone path exists for CI +// (provision a fresh DB before the integration suite) and operators who +// want to migrate without starting the server. +func runMigrate(args []string) error { + fs := flag.NewFlagSet("migrate", flag.ContinueOnError) + configPath := fs.String("config", os.Getenv("MINSTREL_CONFIG"), "path to YAML config file") + if err := fs.Parse(args); err != nil { + return err + } + cfg, err := config.Load(*configPath) + if err != nil { + return fmt.Errorf("load config: %w", err) + } + logger, err := logging.New(os.Stdout, cfg.Log.Level, cfg.Log.Format) + if err != nil { + return fmt.Errorf("init logger: %w", err) + } + if err := db.Migrate(cfg.Database.URL, logger); err != nil { + return fmt.Errorf("migrate: %w", err) + } + fmt.Println("minstrel: migrations applied") + return nil +} + +// runAdmin dispatches `minstrel admin `. +func runAdmin(args []string) error { + if len(args) == 0 { + return errors.New("usage: minstrel admin reset-password [-user NAME] [-password PW] [-config PATH]") + } + switch args[0] { + case "reset-password": + return adminResetPassword(args[1:]) + default: + return fmt.Errorf("unknown admin subcommand %q", args[0]) + } +} + +// adminResetPassword resets a user's credentials. It updates BOTH +// password_hash (bcrypt, for /api/auth/login) and subsonic_password +// (plaintext, required for Subsonic t+s token verification) so neither +// auth path is left stale. Recovers a locked-out operator when the +// bootstrap password was missed or the DB volume was recreated (Fable +// #321) without DB surgery. +func adminResetPassword(args []string) error { + fs := flag.NewFlagSet("admin reset-password", flag.ContinueOnError) + configPath := fs.String("config", os.Getenv("MINSTREL_CONFIG"), "path to YAML config file") + username := fs.String("user", "admin", "username to reset") + password := fs.String("password", "", "new password; if empty a strong one is generated and printed") + if err := fs.Parse(args); err != nil { + return err + } + + cfg, err := config.Load(*configPath) + if err != nil { + return fmt.Errorf("load config: %w", err) + } + if cfg.Database.URL == "" { + return errors.New("no database URL configured (set it in the config file or MINSTREL_DATABASE_URL)") + } + + ctx := context.Background() + pool, err := db.Open(ctx, cfg.Database.URL) + if err != nil { + return fmt.Errorf("open db: %w", err) + } + defer pool.Close() + q := dbq.New(pool) + + user, err := q.GetUserByUsername(ctx, *username) + if err != nil { + return fmt.Errorf("look up user %q: %w", *username, err) + } + + pw := *password + generated := false + if pw == "" { + b := make([]byte, 18) + if _, err := rand.Read(b); err != nil { + return fmt.Errorf("generate password: %w", err) + } + pw = base64.RawURLEncoding.EncodeToString(b) + generated = true + } + + hash, err := bcrypt.GenerateFromPassword([]byte(pw), bcrypt.DefaultCost) + if err != nil { + return fmt.Errorf("hash password: %w", err) + } + if err := q.ChangeUserPassword(ctx, dbq.ChangeUserPasswordParams{ + ID: user.ID, + PasswordHash: string(hash), + }); err != nil { + return fmt.Errorf("update password_hash: %w", err) + } + sp := pw + if err := q.SetSubsonicPassword(ctx, dbq.SetSubsonicPasswordParams{ + ID: user.ID, + SubsonicPassword: &sp, + }); err != nil { + return fmt.Errorf("update subsonic_password: %w", err) + } + + if generated { + fmt.Printf("minstrel: password for %q reset.\nNew password: %s\n", *username, pw) + } else { + fmt.Printf("minstrel: password for %q reset.\n", *username) + } + return nil +} diff --git a/cmd/minstrel/main.go b/cmd/minstrel/main.go index 84a198d7..1169204a 100644 --- a/cmd/minstrel/main.go +++ b/cmd/minstrel/main.go @@ -29,6 +29,22 @@ import ( ) func main() { + if len(os.Args) > 1 { + switch os.Args[1] { + case "admin": + if err := runAdmin(os.Args[2:]); err != nil { + fmt.Fprintf(os.Stderr, "minstrel admin: %v\n", err) + os.Exit(1) + } + return + case "migrate": + if err := runMigrate(os.Args[2:]); err != nil { + fmt.Fprintf(os.Stderr, "minstrel migrate: %v\n", err) + os.Exit(1) + } + return + } + } if err := run(); err != nil { fmt.Fprintf(os.Stderr, "minstrel: %v\n", err) os.Exit(1) diff --git a/deploy/initdb/01-create-test-db.sql b/deploy/initdb/01-create-test-db.sql new file mode 100644 index 00000000..e1c339f1 --- /dev/null +++ b/deploy/initdb/01-create-test-db.sql @@ -0,0 +1,6 @@ +-- Runs once, only on a fresh Postgres data volume (Docker entrypoint +-- initdb hook). Creates the dedicated integration-test database so +-- `go test` never truncates the operator's dev `minstrel` DB (Fable +-- #339). For an already-initialised volume this script does NOT run; +-- `make test-integration` creates the DB idempotently instead. +CREATE DATABASE minstrel_test OWNER minstrel; diff --git a/docker-compose.yml b/docker-compose.yml index a22a72a4..7e32270d 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,10 +1,14 @@ version: "3.9" # Local development environment for Minstrel. -# Not used by CI — integration tests run against this compose stack manually: -# docker compose up -d postgres -# MINSTREL_TEST_DATABASE_URL=postgres://minstrel:minstrel@localhost:5432/minstrel?sslmode=disable \ -# go test ./... +# +# Integration tests run against a SEPARATE database (minstrel_test) so a +# test run never truncates the dev `minstrel` DB (Fable #339). Use the +# Makefile target, which ensures the test DB exists then points the +# tests at it: +# make test-integration +# (CI runs the same suite against its own ephemeral Postgres service — +# see .forgejo/workflows/test-go.yml.) # # Full stack (server + db): # docker compose up --build @@ -22,6 +26,9 @@ services: # - "5432:5432" volumes: - minstrel-pgdata:/var/lib/postgresql/data + # Creates minstrel_test on a fresh volume (Fable #339). No-op on + # an existing volume — make test-integration ensures it instead. + - ./deploy/initdb:/docker-entrypoint-initdb.d:ro healthcheck: test: ["CMD-SHELL", "pg_isready -U minstrel -d minstrel"] interval: 5s diff --git a/flutter_client/lib/api/endpoints/discover.dart b/flutter_client/lib/api/endpoints/discover.dart index 3f5cad00..1a6d2f4f 100644 --- a/flutter_client/lib/api/endpoints/discover.dart +++ b/flutter_client/lib/api/endpoints/discover.dart @@ -1,11 +1,25 @@ import 'package:dio/dio.dart'; +import '../../models/artist_suggestion.dart'; import '../../models/lidarr.dart'; class DiscoverApi { DiscoverApi(this._dio); final Dio _dio; + /// GET /api/discover/suggestions — out-of-library artist suggestions + /// (ListenBrainz-derived; image_url resolved on-demand from Lidarr, + /// may be empty). The server already filters in-library and + /// non-terminal-request candidates. + Future> listSuggestions() async { + final r = await _dio.get>('/api/discover/suggestions'); + final raw = r.data ?? const []; + return raw + .map((e) => + ArtistSuggestion.fromJson((e as Map).cast())) + .toList(growable: false); + } + /// GET /api/lidarr/search?q=...&kind=artist|album|track. Server has a /// 60s LRU cache for repeat queries so re-typing the same string in /// quick succession is cheap. diff --git a/flutter_client/lib/discover/discover_screen.dart b/flutter_client/lib/discover/discover_screen.dart index 6699ab8f..9519d109 100644 --- a/flutter_client/lib/discover/discover_screen.dart +++ b/flutter_client/lib/discover/discover_screen.dart @@ -8,6 +8,7 @@ import '../api/endpoints/discover.dart'; import '../api/errors.dart'; import '../cache/mutation_queue.dart'; import '../library/library_providers.dart' show dioProvider; +import '../models/artist_suggestion.dart'; import '../models/lidarr.dart'; import '../shared/widgets/main_app_bar_actions.dart'; import '../theme/theme_extension.dart'; @@ -27,6 +28,22 @@ class _DiscoverScreenState extends ConsumerState { final _ctrl = TextEditingController(); LidarrRequestKind _kind = LidarrRequestKind.artist; Future>? _resultsFuture; + // Default (empty-search) surface: LB-derived out-of-library artist + // suggestions, mirroring web's SuggestionFeed. + Future>? _suggestionsFuture; + final _requested = {}; + + @override + void initState() { + super.initState(); + _loadSuggestions(); + // Clearing the box returns to suggestions (web swaps live too). + _ctrl.addListener(() { + if (_ctrl.text.trim().isEmpty && _resultsFuture != null) { + setState(() => _resultsFuture = null); + } + }); + } @override void dispose() { @@ -34,6 +51,55 @@ class _DiscoverScreenState extends ConsumerState { super.dispose(); } + // Assigns the future only; callers trigger the rebuild (initState + // runs before first build, so setState here would be a no-op/warn). + void _loadSuggestions() { + _suggestionsFuture = ref + .read(_discoverApiProvider.future) + .then((api) => api.listSuggestions()); + } + + Future _requestSuggestion(ArtistSuggestion s) async { + final fs = Theme.of(context).extension()!; + final args = { + 'kind': LidarrRequestKind.artist.wire, + 'artistMbid': s.mbid, + 'artistName': s.name, + 'albumMbid': null, + 'albumTitle': null, + }; + try { + final api = await ref.read(_discoverApiProvider.future); + await api.createRequest( + kind: LidarrRequestKind.artist, + artistMbid: s.mbid, + artistName: s.name, + ); + if (mounted) { + // Reassign the future first; the setState below rebuilds and + // the FutureBuilder picks up the fresh fetch (server now + // filters this candidate out). _requested hides it meanwhile. + _loadSuggestions(); + setState(() => _requested.add(s.mbid)); + ScaffoldMessenger.of(context).showSnackBar(SnackBar( + content: Text('Requested: ${s.name}'), + backgroundColor: fs.iron, + )); + } + } on DioException catch (_) { + await ref + .read(mutationQueueProvider) + .enqueue(MutationKinds.requestCreate, args); + if (mounted) { + setState(() => _requested.add(s.mbid)); + ScaffoldMessenger.of(context).showSnackBar(SnackBar( + content: Text('Request queued: ${s.name}'), + backgroundColor: fs.iron, + )); + } + } + } + void _runSearch() { final q = _ctrl.text.trim(); if (q.isEmpty) { @@ -153,13 +219,7 @@ class _DiscoverScreenState extends ConsumerState { ), Expanded( child: _resultsFuture == null - ? Center( - child: Text( - 'Type to search, then tap Request to send to Lidarr.', - textAlign: TextAlign.center, - style: TextStyle(color: fs.ash), - ), - ) + ? _buildSuggestions(fs) : FutureBuilder>( future: _resultsFuture, builder: (ctx, snap) { @@ -199,6 +259,62 @@ class _DiscoverScreenState extends ConsumerState { ]), ); } + + Widget _buildSuggestions(FabledSwordTheme fs) { + return FutureBuilder>( + future: _suggestionsFuture, + builder: (ctx, snap) { + if (snap.connectionState != ConnectionState.done) { + return const Center(child: CircularProgressIndicator()); + } + final items = (snap.data ?? const []) + .where((s) => !_requested.contains(s.mbid)) + .toList(growable: false); + return ListView( + padding: const EdgeInsets.only(bottom: 16), + children: [ + Padding( + padding: const EdgeInsets.fromLTRB(16, 12, 16, 4), + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Text('Suggested for you', + style: TextStyle( + color: fs.parchment, + fontSize: 20, + fontWeight: FontWeight.w500)), + const SizedBox(height: 2), + Text( + "Out-of-library artists drawn from what you've liked and played.", + style: TextStyle(color: fs.ash, fontSize: 13), + ), + ], + ), + ), + if (snap.hasError) + Padding( + padding: const EdgeInsets.all(16), + child: Text("Couldn't load suggestions.", + style: TextStyle(color: fs.ash)), + ) + else if (items.isEmpty) + Padding( + padding: const EdgeInsets.all(16), + child: Text( + 'Listen to something or like an artist to start getting suggestions.', + style: TextStyle(color: fs.ash), + ), + ) + else + ...items.map((s) => _SuggestionTile( + s: s, + onRequest: () => _requestSuggestion(s), + )), + ], + ); + }, + ); + } } class _ResultTile extends StatelessWidget { @@ -284,3 +400,63 @@ class _Pill extends StatelessWidget { ); } } + +class _SuggestionTile extends StatelessWidget { + const _SuggestionTile({required this.s, required this.onRequest}); + final ArtistSuggestion s; + final VoidCallback onRequest; + + @override + Widget build(BuildContext context) { + final fs = Theme.of(context).extension()!; + return Padding( + padding: const EdgeInsets.symmetric(horizontal: 16, vertical: 10), + child: Row(children: [ + ClipRRect( + borderRadius: BorderRadius.circular(6), + child: Container( + width: 56, + height: 56, + color: fs.slate, + child: s.imageUrl.isEmpty + ? Icon(Icons.person, color: fs.ash) + : CachedNetworkImage( + imageUrl: s.imageUrl, + fit: BoxFit.cover, + fadeInDuration: const Duration(milliseconds: 120), + fadeOutDuration: Duration.zero, + errorWidget: (_, __, ___) => + Icon(Icons.person, color: fs.ash), + ), + ), + ), + const SizedBox(width: 12), + Expanded( + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Text(s.name, + maxLines: 1, + overflow: TextOverflow.ellipsis, + style: TextStyle(color: fs.parchment, fontSize: 14)), + if (s.attributionText.isNotEmpty) + Text(s.attributionText, + maxLines: 2, + overflow: TextOverflow.ellipsis, + style: TextStyle(color: fs.ash, fontSize: 12)), + ], + ), + ), + const SizedBox(width: 8), + FilledButton( + onPressed: onRequest, + style: FilledButton.styleFrom( + backgroundColor: fs.accent, + foregroundColor: fs.parchment, + ), + child: const Text('Request'), + ), + ]), + ); + } +} diff --git a/flutter_client/lib/models/artist_suggestion.dart b/flutter_client/lib/models/artist_suggestion.dart new file mode 100644 index 00000000..a636a4b7 --- /dev/null +++ b/flutter_client/lib/models/artist_suggestion.dart @@ -0,0 +1,51 @@ +/// Mirrors web/src/lib/api/types.ts ArtistSuggestion / SeedContribution — +/// one out-of-library artist from GET /api/discover/suggestions. image_url +/// is resolved on-demand from Lidarr server-side (may be empty). +class SeedContribution { + const SeedContribution({required this.name, required this.isLiked}); + + final String name; + final bool isLiked; + + factory SeedContribution.fromJson(Map j) => SeedContribution( + name: j['name'] as String? ?? '', + isLiked: j['is_liked'] as bool? ?? false, + ); +} + +class ArtistSuggestion { + const ArtistSuggestion({ + required this.mbid, + required this.name, + required this.imageUrl, + required this.attribution, + }); + + final String mbid; + final String name; + final String imageUrl; + final List attribution; + + factory ArtistSuggestion.fromJson(Map j) => ArtistSuggestion( + mbid: j['mbid'] as String? ?? '', + name: j['name'] as String? ?? '', + imageUrl: j['image_url'] as String? ?? '', + attribution: ((j['attribution'] as List?) ?? const []) + .map((e) => + SeedContribution.fromJson((e as Map).cast())) + .toList(growable: false), + ); + + /// Mirrors web SuggestionFeed.attributionText (Oxford comma, max 3). + String get attributionText { + if (attribution.isEmpty) return ''; + final phrases = attribution + .map((s) => '${s.isLiked ? 'liked' : 'played'} ${s.name}') + .toList(growable: false); + if (phrases.length == 1) return 'Because you ${phrases[0]}.'; + if (phrases.length == 2) { + return 'Because you ${phrases[0]} and ${phrases[1]}.'; + } + return 'Because you ${phrases[0]}, ${phrases[1]}, and ${phrases[2]}.'; + } +} diff --git a/flutter_client/pubspec.yaml b/flutter_client/pubspec.yaml index 2e162f06..0d9aab7a 100644 --- a/flutter_client/pubspec.yaml +++ b/flutter_client/pubspec.yaml @@ -1,7 +1,7 @@ name: minstrel description: Minstrel mobile client publish_to: 'none' -version: 2026.5.16+8 +version: 2026.5.18+9 environment: sdk: '>=3.5.0 <4.0.0' diff --git a/internal/api/admin_cover_sources_test.go b/internal/api/admin_cover_sources_test.go index 029ace76..3a0de4e7 100644 --- a/internal/api/admin_cover_sources_test.go +++ b/internal/api/admin_cover_sources_test.go @@ -27,7 +27,7 @@ func (p *apiTestAlbumProvider) DisplayName() string { re func (p *apiTestAlbumProvider) RequiresAPIKey() bool { return false } func (p *apiTestAlbumProvider) DefaultEnabled() bool { return true } func (p *apiTestAlbumProvider) Configure(_ coverart.ProviderSettings) error { return nil } -func (p *apiTestAlbumProvider) FetchAlbumCover(_ context.Context, _ string) ([]byte, error) { +func (p *apiTestAlbumProvider) FetchAlbumCover(_ context.Context, _ coverart.AlbumRef) ([]byte, error) { return []byte("img"), nil } diff --git a/internal/api/admin_lidarr_test.go b/internal/api/admin_lidarr_test.go index f5b1618e..5db71172 100644 --- a/internal/api/admin_lidarr_test.go +++ b/internal/api/admin_lidarr_test.go @@ -137,8 +137,9 @@ func TestHandlePutLidarrConfig_EmptyKeyPreservesSaved(t *testing.T) { APIKey: "originalkey", }) - // PUT with empty api_key — should preserve "originalkey". - body := []byte(`{"enabled":true,"base_url":"http://lidarr.lan:8686","api_key":""}`) + // PUT with empty api_key — should preserve "originalkey". enabled=true + // requires the defaults gate (missing_defaults) to be satisfied. + body := []byte(`{"enabled":true,"base_url":"http://lidarr.lan:8686","api_key":"","default_quality_profile_id":1,"default_metadata_profile_id":1,"default_root_folder_path":"/music"}`) w := doAdminReq(t, h, http.MethodPut, "/api/admin/lidarr/config", body, admin) if w.Code != http.StatusOK { t.Fatalf("status = %d, want 200; body = %s", w.Code, w.Body.String()) @@ -183,7 +184,7 @@ func TestHandlePutLidarrConfig_HappyPath(t *testing.T) { resetLidarrState(t, h) admin := seedAdminUser(t, h) - body := []byte(`{"enabled":true,"base_url":"http://lidarr.lan:8686","api_key":"newkey","default_quality_profile_id":2,"default_root_folder_path":"/music"}`) + body := []byte(`{"enabled":true,"base_url":"http://lidarr.lan:8686","api_key":"newkey","default_quality_profile_id":2,"default_metadata_profile_id":1,"default_root_folder_path":"/music"}`) w := doAdminReq(t, h, http.MethodPut, "/api/admin/lidarr/config", body, admin) if w.Code != http.StatusOK { t.Fatalf("status = %d, want 200; body = %s", w.Code, w.Body.String()) diff --git a/internal/api/admin_requests_test.go b/internal/api/admin_requests_test.go index e9957330..741a7d6a 100644 --- a/internal/api/admin_requests_test.go +++ b/internal/api/admin_requests_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" "github.com/go-chi/chi/v5" @@ -174,10 +175,17 @@ func TestHandleApproveRequest_HappyPath(t *testing.T) { h, _ := testHandlersWithClientFn(t) resetLidarrState(t, h) - stub := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + stub := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(`{"id":1}`)) + switch { + case strings.Contains(r.URL.Path, "/metadataprofile"): + _, _ = w.Write([]byte(`[{"id":1,"name":"Standard"}]`)) + case strings.Contains(r.URL.Path, "/qualityprofile"): + _, _ = w.Write([]byte(`[{"id":1,"name":"Lossless"}]`)) + default: + _, _ = w.Write([]byte(`{"id":1}`)) + } })) t.Cleanup(stub.Close) @@ -221,7 +229,14 @@ func TestHandleApproveRequest_OverrideUsed(t *testing.T) { } w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(`{"id":1}`)) + switch { + case strings.Contains(r.URL.Path, "/metadataprofile"): + _, _ = w.Write([]byte(`[{"id":1,"name":"Standard"}]`)) + case strings.Contains(r.URL.Path, "/qualityprofile"): + _, _ = w.Write([]byte(`[{"id":1,"name":"Lossless"}]`)) + default: + _, _ = w.Write([]byte(`{"id":1}`)) + } })) t.Cleanup(stub.Close) diff --git a/internal/api/admin_users_test.go b/internal/api/admin_users_test.go index 44f35002..6895fb72 100644 --- a/internal/api/admin_users_test.go +++ b/internal/api/admin_users_test.go @@ -163,7 +163,10 @@ func TestAdminCreateUser_DuplicateUsername_409(t *testing.T) { admin := seedUser(t, pool, "duper", "pw", true) seedUser(t, pool, "existing", "pw", false) - body := `{"username":"existing","password":"abcd1234"}` + // seedUser prefixes usernames with dbtest.TestUserPrefix, so the + // row above is "test-existing"; POST that exact name to actually + // collide (stays test-prefixed for ResetDB). + body := `{"username":"test-existing","password":"abcd1234"}` req := httptest.NewRequest(http.MethodPost, "/api/admin/users", bytes.NewReader([]byte(body))) req = withUser(req, admin) rec := httptest.NewRecorder() diff --git a/internal/api/me_timezone_test.go b/internal/api/me_timezone_test.go index 0fae0852..576f694c 100644 --- a/internal/api/me_timezone_test.go +++ b/internal/api/me_timezone_test.go @@ -69,14 +69,18 @@ func TestPutTimezone_InvalidIANA(t *testing.T) { t.Fatalf("status = %d, want 400; body=%s", rec.Code, rec.Body.String()) } + // Error envelope is nested: {"error":{"code":...}} (matches the + // rest of /api/*), not a top-level {"code":...}. var errResp struct { - Code string `json:"code"` + Error struct { + Code string `json:"code"` + } `json:"error"` } if err := json.NewDecoder(rec.Body).Decode(&errResp); err != nil { t.Fatalf("decode err response: %v", err) } - if errResp.Code != "invalid_timezone" { - t.Errorf("error code = %q, want invalid_timezone", errResp.Code) + if errResp.Error.Code != "invalid_timezone" { + t.Errorf("error code = %q, want invalid_timezone", errResp.Error.Code) } } diff --git a/internal/api/playlists_system_test.go b/internal/api/playlists_system_test.go index 1a1570e1..621775fd 100644 --- a/internal/api/playlists_system_test.go +++ b/internal/api/playlists_system_test.go @@ -8,8 +8,6 @@ import ( "testing" "github.com/go-chi/chi/v5" - - "git.fabledsword.com/bvandeusen/minstrel/internal/auth" ) // Generic registry-driven system-playlist endpoints (#411 R2). @@ -17,7 +15,11 @@ import ( func newSystemPlaylistRouter(h *handlers) chi.Router { r := chi.NewRouter() r.Route("/api", func(api chi.Router) { - api.Use(auth.RequireUser(h.pool)) + // No real auth.RequireUser middleware: like every other api + // handler test, auth is supplied via withUser() context and the + // handlers self-guard with requireUser() (prelude.go). The real + // middleware needs a live session and would 401 the injected + // test user. api.Post("/playlists/system/{kind}/refresh", h.handleSystemPlaylistRefresh) api.Get("/playlists/system/{kind}/shuffle", h.handleSystemPlaylistShuffle) }) diff --git a/internal/api/prelude.go b/internal/api/prelude.go index 6c6f6fce..57333d49 100644 --- a/internal/api/prelude.go +++ b/internal/api/prelude.go @@ -19,7 +19,7 @@ import ( func requireUser(w http.ResponseWriter, r *http.Request) (dbq.User, bool) { user, ok := auth.UserFromContext(r.Context()) if !ok { - writeErr(w, apierror.Unauthorized("auth_required", "")) + writeErr(w, apierror.Unauthorized("unauthenticated", "")) return dbq.User{}, false } return user, true diff --git a/internal/api/suggestions.go b/internal/api/suggestions.go index ba005a24..1ad5bf7f 100644 --- a/internal/api/suggestions.go +++ b/internal/api/suggestions.go @@ -1,12 +1,15 @@ package api import ( + "context" "net/http" "strconv" + "sync" "github.com/jackc/pgx/v5/pgtype" "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" + "git.fabledsword.com/bvandeusen/minstrel/internal/lidarr" "git.fabledsword.com/bvandeusen/minstrel/internal/recommendation" ) @@ -16,6 +19,11 @@ type suggestionView struct { Name string `json:"name"` Score float64 `json:"score"` Attribution []seedContributionView `json:"attribution"` + // ImageURL is resolved on-demand from Lidarr (out-of-library + // artists have no local art row). Omitted when Lidarr is disabled + // or has no match — the client falls back to a placeholder. Not + // cached: a remote URL Lidarr surfaced, fetched by the browser. + ImageURL string `json:"image_url,omitempty"` } type seedContributionView struct { @@ -80,5 +88,52 @@ func (h *handlers) handleListSuggestions(w http.ResponseWriter, r *http.Request) MBID: s.MBID, Name: s.Name, Score: s.Score, Attribution: attr, }) } + h.resolveSuggestionArt(r.Context(), out) writeJSON(w, http.StatusOK, out) } + +// resolveSuggestionArt fills ImageURL on-demand from Lidarr's artist +// lookup, matched by MBID (foreignArtistId). Best-effort and cache-free: +// Lidarr is the only source — when it's disabled, unreachable, or has +// no match for a candidate, that entry keeps an empty ImageURL and the +// client renders its placeholder. Lookups run with bounded concurrency +// so a full Discover page doesn't serialize ~12 round-trips. Never +// fails the request; the suggestions list is the contract, art is a +// nicety. +func (h *handlers) resolveSuggestionArt(ctx context.Context, views []suggestionView) { + if len(views) == 0 { + return + } + cfg, err := h.lidarrCfg.Get(ctx) + if err != nil || !cfg.Enabled || cfg.BaseURL == "" || cfg.APIKey == "" { + return // Lidarr off / unconfigured → placeholders only + } + client := lidarr.NewClient(cfg.BaseURL, cfg.APIKey) + + const maxConcurrent = 6 + sem := make(chan struct{}, maxConcurrent) + var wg sync.WaitGroup + for i := range views { + if views[i].MBID == "" || views[i].Name == "" { + continue + } + wg.Add(1) + sem <- struct{}{} + go func(idx int) { + defer wg.Done() + defer func() { <-sem }() + results, lerr := client.LookupArtist(ctx, views[idx].Name) + if lerr != nil { + return // best-effort: no art on lookup failure + } + for _, res := range results { + if res.MBID == views[idx].MBID && res.ImageURL != "" { + // Distinct slice index per goroutine → race-free. + views[idx].ImageURL = res.ImageURL + return + } + } + }(i) + } + wg.Wait() +} diff --git a/internal/audit/audit_test.go b/internal/audit/audit_test.go index 8254f801..ee413893 100644 --- a/internal/audit/audit_test.go +++ b/internal/audit/audit_test.go @@ -65,7 +65,9 @@ func TestWrite_WithMetadata(t *testing.T) { ).Scan(&meta); err != nil { t.Fatalf("read metadata: %v", err) } - if !contains(meta, `"first_admin":true`) || !contains(meta, `"reason":"test"`) { + // Postgres jsonb::text renders a space after ':' and ',', e.g. + // {"reason": "test", "first_admin": true}. + if !contains(meta, `"first_admin": true`) || !contains(meta, `"reason": "test"`) { t.Errorf("metadata = %q, expected first_admin + reason fields", meta) } } diff --git a/internal/coverart/artist_enricher_test.go b/internal/coverart/artist_enricher_test.go index 352214ed..2751eb49 100644 --- a/internal/coverart/artist_enricher_test.go +++ b/internal/coverart/artist_enricher_test.go @@ -496,7 +496,7 @@ func TestCleanupArtistArt_IdempotentMissingDir(t *testing.T) { // --- MBID guard --- -func TestEnrichArtist_NoMBID_LeavesNull(t *testing.T) { +func TestEnrichArtist_NoMBID_SettlesNone(t *testing.T) { pool := newPool(t) ctx := context.Background() q := dbq.New(pool) @@ -504,9 +504,13 @@ func TestEnrichArtist_NoMBID_LeavesNull(t *testing.T) { resetRegistryForTests() t.Cleanup(resetRegistryForTests) + // No MBID still runs the provider chain (name-based providers can + // resolve without one). An MBID-only provider returns ErrNotFound + // for an empty MBID; with the whole chain returning ErrNotFound the + // row settles 'none' (version-stamped), NOT NULL. stub := &stubArtistProvider{ fakeProvider: fakeProvider{id: "stub-artist", defaultOn: true}, - thumb: []byte("should_not_write"), + err: ErrNotFound, } Register(stub) @@ -523,7 +527,7 @@ func TestEnrichArtist_NoMBID_LeavesNull(t *testing.T) { if err != nil { t.Fatalf("GetArtistByID: %v", err) } - if row.ArtistArtSource != nil { - t.Errorf("source = %v, want nil (no MBID — skip)", row.ArtistArtSource) + if row.ArtistArtSource == nil || *row.ArtistArtSource != "none" { + t.Errorf("source = %v, want 'none' (settled)", row.ArtistArtSource) } } diff --git a/internal/coverart/enricher_test.go b/internal/coverart/enricher_test.go index 09848647..36e8c38a 100644 --- a/internal/coverart/enricher_test.go +++ b/internal/coverart/enricher_test.go @@ -49,8 +49,11 @@ func discardLogger() *slog.Logger { // pick them up. func newTestEnricher(t *testing.T, pool *pgxpool.Pool) *Enricher { t.Helper() - resetRegistryForTests() - t.Cleanup(resetRegistryForTests) + // Do NOT reset the registry here: callers register their fakes + // before calling this (see doc above) and resetting would wipe them + // so reconcile() sees zero providers — every art source then stays + // NULL. Registry lifecycle is the caller's (each test does + // resetRegistryForTests + t.Cleanup before Register()). s, err := NewSettingsService(context.Background(), pool, discardLogger()) if err != nil { t.Fatalf("NewSettingsService: %v", err) @@ -100,6 +103,8 @@ func TestEnrichAlbum_SidecarFound(t *testing.T) { t.Fatal(err) } + resetRegistryForTests() // deterministic empty registry (sidecar-only path) + t.Cleanup(resetRegistryForTests) e := newTestEnricher(t, pool) if err := e.EnrichAlbum(context.Background(), id); err != nil { t.Fatalf("enrich: %v", err) @@ -116,9 +121,11 @@ func TestEnrichAlbum_SidecarFound(t *testing.T) { } } -func TestEnrichAlbum_NoSidecarNoMBID_LeavesNull(t *testing.T) { +func TestEnrichAlbum_NoSidecarNoMBID_SettlesNone(t *testing.T) { pool := newPool(t) id, _ := seedAlbumWithTrack(t, pool, "NoMbid", "Artist", "") + resetRegistryForTests() // deterministic empty registry (no provider can supply) + t.Cleanup(resetRegistryForTests) e := newTestEnricher(t, pool) if err := e.EnrichAlbum(context.Background(), id); err != nil { t.Fatalf("enrich: %v", err) @@ -127,8 +134,11 @@ func TestEnrichAlbum_NoSidecarNoMBID_LeavesNull(t *testing.T) { if err != nil { t.Fatalf("get: %v", err) } - if row.CoverArtSource != nil { - t.Errorf("source = %v, want nil (NULL — eligible for retry on next scan)", row.CoverArtSource) + // No sidecar, no MBID, no provider yields → the chain settles + // 'none' (allWere404 stays true), version-stamped so it is only + // re-tried when the registered provider set changes. + if row.CoverArtSource == nil || *row.CoverArtSource != "none" { + t.Errorf("source = %v, want 'none' (settled)", row.CoverArtSource) } } @@ -227,6 +237,8 @@ func TestEnrichAlbum_AlreadySidecar_NoOp(t *testing.T) { t.Fatal(err) } + resetRegistryForTests() // deterministic empty registry (sidecar-only path) + t.Cleanup(resetRegistryForTests) e := newTestEnricher(t, pool) if err := e.EnrichAlbum(context.Background(), id); err != nil { t.Fatalf("first enrich: %v", err) @@ -253,16 +265,27 @@ func TestEnrichBatch_DrainsNullSourceOnly(t *testing.T) { } id2, _ := seedAlbumWithTrack(t, pool, "Drain2", "B", "") - // Pre-mark id2 as 'none' with current version — should NOT be drained by EnrichBatch - // (ListAlbumsMissingCover only returns stale-version 'none'). + + resetRegistryForTests() // deterministic empty registry + t.Cleanup(resetRegistryForTests) + e := newTestEnricher(t, pool) + + // Pre-mark id2 as 'none' AT the current sources version so + // ListAlbumsMissingCover excludes it (it only returns NULL or + // stale-version 'none'). SetAlbumCover does NOT stamp the version; + // SetAlbumCoverWithVersion does — use the live current version, the + // same value EnrichBatch compares against. + curVer, err := dbq.New(pool).GetCurrentSourcesVersion(context.Background()) + if err != nil { + t.Fatalf("current version: %v", err) + } src := "none" - if err := dbq.New(pool).SetAlbumCover(context.Background(), dbq.SetAlbumCoverParams{ - ID: id2, Column2: "", CoverArtSource: &src, + if err := dbq.New(pool).SetAlbumCoverWithVersion(context.Background(), dbq.SetAlbumCoverWithVersionParams{ + ID: id2, CoverArtPath: nil, CoverArtSource: &src, CoverArtSourcesVersion: curVer, }); err != nil { t.Fatal(err) } - e := newTestEnricher(t, pool) processed, _, _, err := e.EnrichBatch(context.Background(), 100, nil) if err != nil { t.Fatalf("batch: %v", err) diff --git a/internal/db/migrations/0030_relax_art_source_check_to_nonempty.down.sql b/internal/db/migrations/0030_relax_art_source_check_to_nonempty.down.sql new file mode 100644 index 00000000..68fe224e --- /dev/null +++ b/internal/db/migrations/0030_relax_art_source_check_to_nonempty.down.sql @@ -0,0 +1,15 @@ +-- Restore the 0020 fixed-allowlist CHECKs. FAILS if any row holds a +-- source value outside the allowlist (the exact case 0030 enables) — +-- normalise such rows before rolling back. + +ALTER TABLE albums DROP CONSTRAINT IF EXISTS albums_cover_art_source_check; +ALTER TABLE albums + ADD CONSTRAINT albums_cover_art_source_check + CHECK (cover_art_source IS NULL + OR cover_art_source IN ('embedded','sidecar','mbcaa','theaudiodb','deezer','lastfm','none')); + +ALTER TABLE artists DROP CONSTRAINT IF EXISTS artists_artist_art_source_check; +ALTER TABLE artists + ADD CONSTRAINT artists_artist_art_source_check + CHECK (artist_art_source IS NULL + OR artist_art_source IN ('theaudiodb','deezer','lastfm','none')); diff --git a/internal/db/migrations/0030_relax_art_source_check_to_nonempty.up.sql b/internal/db/migrations/0030_relax_art_source_check_to_nonempty.up.sql new file mode 100644 index 00000000..35ca702c --- /dev/null +++ b/internal/db/migrations/0030_relax_art_source_check_to_nonempty.up.sql @@ -0,0 +1,19 @@ +-- The cover/artist art `*_source` column stores the recording +-- provider's registry ID. coverart.Register makes the provider set +-- extensible, so a fixed IN-list CHECK (0016 → 0018 → 0020) required a +-- schema migration for every new provider and rejected any +-- out-of-list value (e.g. test stub providers). Same brittleness +-- class as the discovery-mix CHECK incident (#433): a fixed-value +-- CHECK fighting an extensible value set. Relax to "NULL or any +-- non-empty string" — the registry, not the schema, is the source of +-- truth for valid provider IDs. + +ALTER TABLE albums DROP CONSTRAINT IF EXISTS albums_cover_art_source_check; +ALTER TABLE albums + ADD CONSTRAINT albums_cover_art_source_check + CHECK (cover_art_source IS NULL OR cover_art_source <> ''); + +ALTER TABLE artists DROP CONSTRAINT IF EXISTS artists_artist_art_source_check; +ALTER TABLE artists + ADD CONSTRAINT artists_artist_art_source_check + CHECK (artist_art_source IS NULL OR artist_art_source <> ''); diff --git a/internal/dbtest/reset.go b/internal/dbtest/reset.go index 5d6dbbda..eba016c9 100644 --- a/internal/dbtest/reset.go +++ b/internal/dbtest/reset.go @@ -55,6 +55,13 @@ var dataTables = []string{ "playlist_tracks", "playlists", "library_changes", // M7 #357 — must reset to keep cursor isolated per test + // SettingsService.reconcile() idempotently re-UpsertProviderSettings + // for every registered provider at boot, so truncating this is the + // correct per-test reset (clears test-modified enabled/api_key rows). + // cover_art_sources_meta is NOT truncated — boot only READS it + // (never recreates the singleton, seeded once by 0018); ResetDB + // resets its counter via UPDATE below instead. + "cover_art_provider_settings", "tracks", "albums", "artists", @@ -76,4 +83,14 @@ func ResetDB(t *testing.T, pool *pgxpool.Pool) { ); err != nil { t.Fatalf("dbtest.ResetDB delete test users: %v", err) } + // Reset the monotonic cover-art source-version counter to its + // post-migration seeded value. Truncating the row would break + // SettingsService boot, which reads (never recreates) this + // singleton; an UPDATE keeps the row while clearing cross-test + // version accumulation (CurrentVersion=4 want 1, key-only-bump). + if _, err := pool.Exec(ctx, + "UPDATE cover_art_sources_meta SET current_version = 1", + ); err != nil { + t.Fatalf("dbtest.ResetDB reset cover-art version: %v", err) + } } diff --git a/internal/library/scanner_test.go b/internal/library/scanner_test.go index a9dc5250..80f8df54 100644 --- a/internal/library/scanner_test.go +++ b/internal/library/scanner_test.go @@ -103,6 +103,17 @@ func TestScanner_Integration(t *testing.T) { t.Errorf("artist sort = [%q, %q], want [Artist X, Artist Y]", artists[0].SortName, artists[1].SortName) } + // The synthetic MP3s carry only an ID3 tag — no decodable audio — + // so ffprobe yields duration 0. The scanner deliberately refuses to + // skip zero-duration rows (it re-runs them so a later scan can + // backfill duration once probing works), which is orthogonal to the + // mtime-based incremental-skip this test covers. Simulate a + // normally-probed library so the skip path is actually exercised; + // updated_at is left untouched (still ≥ file mtime). + if _, err := pool.Exec(ctx, "UPDATE tracks SET duration_ms = 1000 WHERE duration_ms = 0"); err != nil { + t.Fatalf("seed durations: %v", err) + } + stats2, err := scanner.Scan(ctx, nil) if err != nil { t.Fatalf("second scan: %v", err) diff --git a/internal/lidarrrequests/service_test.go b/internal/lidarrrequests/service_test.go index ab9ba4e8..49891988 100644 --- a/internal/lidarrrequests/service_test.go +++ b/internal/lidarrrequests/service_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/http/httptest" "os" + "strings" "testing" "github.com/jackc/pgx/v5/pgtype" @@ -236,10 +237,20 @@ func TestListForUser_OnlyOwnRows(t *testing.T) { func approveTestSetup(t *testing.T) (*Service, pgtype.UUID, *pgxpool.Pool, *httptest.Server) { t.Helper() pool := newPool(t) - stub := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + stub := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(`{"id":1}`)) + // Approve resolves metadata/quality profiles before the add; + // those list endpoints return JSON arrays, not the {"id":1} + // object the add endpoints return. + switch { + case strings.Contains(r.URL.Path, "/metadataprofile"): + _, _ = w.Write([]byte(`[{"id":1,"name":"Standard"}]`)) + case strings.Contains(r.URL.Path, "/qualityprofile"): + _, _ = w.Write([]byte(`[{"id":7,"name":"Lossless"}]`)) + default: + _, _ = w.Write([]byte(`{"id":1}`)) + } })) t.Cleanup(stub.Close) diff --git a/internal/playlists/system_test.go b/internal/playlists/system_test.go index 64d7c456..2e5fb5cf 100644 --- a/internal/playlists/system_test.go +++ b/internal/playlists/system_test.go @@ -24,9 +24,16 @@ func discardLogger() *slog.Logger { // `InsertPlayEvent` doesn't accept was_skipped (that's set by an UPDATE). func seedPlayEvent(t *testing.T, pool *pgxpool.Pool, userID, trackID pgtype.UUID, startedAt time.Time, wasSkipped bool) { t.Helper() + // play_events.session_id has a FK to play_sessions; create a parent + // session in the same statement (a random UUID violates the FK). _, err := pool.Exec(context.Background(), ` + WITH s AS ( + INSERT INTO play_sessions (user_id, started_at, last_event_at) + VALUES ($1, $3, $3) + RETURNING id + ) INSERT INTO play_events (user_id, track_id, session_id, started_at, was_skipped) - VALUES ($1, $2, gen_random_uuid(), $3, $4) + SELECT $1, $2, s.id, $3, $4 FROM s `, userID, trackID, startedAt, wasSkipped) if err != nil { t.Fatalf("seed play_event: %v", err) @@ -38,7 +45,7 @@ func seedQuarantine(t *testing.T, pool *pgxpool.Pool, userID, trackID pgtype.UUI t.Helper() _, err := pool.Exec(context.Background(), ` INSERT INTO lidarr_quarantine (user_id, track_id, reason) - VALUES ($1, $2, 'test-hide') + VALUES ($1, $2, 'other') `, userID, trackID) if err != nil { t.Fatalf("seed quarantine: %v", err) @@ -105,16 +112,19 @@ func TestBuildSystemPlaylists_SufficientActivity(t *testing.T) { if !r.SeedArtistID.Valid { t.Errorf("songs_like_artist row should have non-NULL seed_artist_id") } - case "discover": - // Discover playlist is valid — no seed_artist_id required. + case "discover", "deep_cuts", "rediscover", "new_for_you", "on_this_day", "first_listens": + // Discover + the #411 discovery mixes are all seedless — + // no seed_artist_id required. default: t.Errorf("unknown system_variant=%q", *r.SystemVariant) } if r.TrackCount == 0 { t.Errorf("row %s: empty track_count", uuidString(r.ID)) } - if r.TrackCount > 25 { - t.Errorf("row %s: track_count=%d exceeds 25", uuidString(r.ID), r.TrackCount) + // For-You / Discover / the discovery mixes are sized to ~100 + // (#352/#411); songs_like_artist stays at systemMixLength (25). + if r.TrackCount > 100 { + t.Errorf("row %s: track_count=%d exceeds 100", uuidString(r.ID), r.TrackCount) } } if !hasForYou { diff --git a/internal/similarity/worker_integration_test.go b/internal/similarity/worker_integration_test.go index e79863ca..c8fa0fde 100644 --- a/internal/similarity/worker_integration_test.go +++ b/internal/similarity/worker_integration_test.go @@ -114,8 +114,11 @@ func markPlayed(t *testing.T, f fixture, trackID pgtype.UUID) { func newTestWorker(f fixture, lbBaseURL string) *Worker { logger := slog.New(slog.NewTextHandler(io.Discard, nil)) return &Worker{ - pool: f.pool, - client: &listenbrainz.Client{BaseURL: lbBaseURL, HTTP: http.DefaultClient}, + pool: f.pool, + // LabsBaseURL must point at the stub too: similarity calls hit + // the Labs API (commit 4fca0e6), so an unset LabsBaseURL would + // fall through to the real labs.api and the stub never runs. + client: &listenbrainz.Client{BaseURL: lbBaseURL, LabsBaseURL: lbBaseURL, HTTP: http.DefaultClient}, logger: logger, tick: 1 * time.Hour, batch: 5, diff --git a/web/src/lib/api/types.ts b/web/src/lib/api/types.ts index d4235b42..667d4b49 100644 --- a/web/src/lib/api/types.ts +++ b/web/src/lib/api/types.ts @@ -298,6 +298,7 @@ export type ArtistSuggestion = { name: string; score: number; attribution: SeedContribution[]; // up to 3 entries, ordered by contribution DESC + image_url?: string; // resolved on-demand from Lidarr; absent → card placeholder }; // Mirrors internal/api/types.go HomePayload. All slices are non-null diff --git a/web/src/lib/components/SuggestionFeed.svelte b/web/src/lib/components/SuggestionFeed.svelte index 6f16b641..fa37c26b 100644 --- a/web/src/lib/components/SuggestionFeed.svelte +++ b/web/src/lib/components/SuggestionFeed.svelte @@ -65,6 +65,7 @@ onRequest(s)} diff --git a/web/src/lib/styles/error-copy.json b/web/src/lib/styles/error-copy.json index 9a29973f..c18961fd 100644 --- a/web/src/lib/styles/error-copy.json +++ b/web/src/lib/styles/error-copy.json @@ -1,7 +1,6 @@ { "unknown": "Something went wrong.", "unauthenticated": "Your session has ended. Please sign in again.", - "auth_required": "You need to sign in to do that.", "forbidden": "You don't have permission to do that.", "not_authorized": "You don't have permission to do that.", "invalid_credentials": "Wrong username or password.",