From c53bcb6c990ab113bd3d5264cf2bc5724b7abb50 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 7 May 2026 07:59:40 -0400 Subject: [PATCH] feat(server/web/coverart): manual Re-search missing art button MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /api/admin/cover-sources/research bumps cover_art_sources_meta. current_version unconditionally; the next enrichment pass re-eligibles every 'none' row. Existing positively-sourced rows are not affected — the version-mismatch eligibility rule only kicks in for 'none'. Admin Cover Art panel gains a "Re-search missing art" button that calls the endpoint and shows a confirmation toast. Operator's escape hatch when: - They've added a Last.fm key and want to retry failed rows immediately rather than waiting for the next scheduled scan. - An upstream provider's catalog has updated (e.g. Deezer added a back-catalog import). - They want to verify a fix end-to-end before the next scheduled scan fires. SettingsService.BumpVersion (the unconditional version of T5's BumpVersionIfProvidersChanged) is the single-call DB writer; both endpoints route through it. Co-Authored-By: Claude Sonnet 4.6 --- internal/api/admin_coverart_research.go | 22 ++++++ internal/api/admin_coverart_research_test.go | 74 ++++++++++++++++++++ internal/api/api.go | 1 + internal/coverart/settings.go | 23 ++++++ internal/coverart/settings_test.go | 38 ++++++++++ web/src/lib/api/admin.research-art.test.ts | 19 +++++ web/src/lib/api/admin.ts | 11 +++ web/src/routes/admin/+page.svelte | 24 +++++++ web/src/routes/admin/admin.test.ts | 13 +++- 9 files changed, 224 insertions(+), 1 deletion(-) create mode 100644 internal/api/admin_coverart_research.go create mode 100644 internal/api/admin_coverart_research_test.go create mode 100644 web/src/lib/api/admin.research-art.test.ts diff --git a/internal/api/admin_coverart_research.go b/internal/api/admin_coverart_research.go new file mode 100644 index 00000000..98b835ce --- /dev/null +++ b/internal/api/admin_coverart_research.go @@ -0,0 +1,22 @@ +package api + +import "net/http" + +// handleResearchMissingArt bumps the cover-art version unconditionally +// so every 'none' row becomes eligible at the next enrichment pass. +// Operator's escape hatch when adding a Last.fm key, when an upstream +// provider's catalog has updated, or when verifying a fix without +// waiting for the next scheduled scan. +// +// Existing positively-sourced rows (sidecar/embedded/mbcaa/theaudiodb/ +// deezer/lastfm) are not affected — the version-mismatch eligibility +// rule only kicks in for 'none'. +func (h *handlers) handleResearchMissingArt(w http.ResponseWriter, r *http.Request) { + newVer, err := h.coverSettings.BumpVersion(r.Context()) + if err != nil { + h.logger.Error("admin: research missing art bump failed", "err", err) + writeErr(w, http.StatusInternalServerError, "server_error", "bump failed") + return + } + writeJSON(w, http.StatusOK, map[string]any{"version": newVer}) +} diff --git a/internal/api/admin_coverart_research_test.go b/internal/api/admin_coverart_research_test.go new file mode 100644 index 00000000..a44050f2 --- /dev/null +++ b/internal/api/admin_coverart_research_test.go @@ -0,0 +1,74 @@ +package api + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "testing" + + "github.com/go-chi/chi/v5" + + "git.fabledsword.com/bvandeusen/minstrel/internal/auth" + "git.fabledsword.com/bvandeusen/minstrel/internal/coverart" +) + +func newResearchRouter(h *handlers) chi.Router { + r := chi.NewRouter() + r.Route("/api/admin", func(admin chi.Router) { + admin.Use(auth.RequireAdmin()) + admin.Post("/cover-sources/research", h.handleResearchMissingArt) + }) + return r +} + +func TestAdminResearchMissingArt_BumpsAndReturnsVersion(t *testing.T) { + if os.Getenv("MINSTREL_TEST_DATABASE_URL") == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + coverart.ResetRegistryForTests() + t.Cleanup(coverart.ResetRegistryForTests) + + coverart.Register(&apiTestAlbumProvider{id: "research-prov", display: "Research Provider"}) + + h, _ := testHandlersWithCoverSettings(t) + admin := seedUser(t, h.pool, "researchadmin", "pw", true) + + req := httptest.NewRequest(http.MethodPost, "/api/admin/cover-sources/research", nil) + req = withUser(req, admin) + rec := httptest.NewRecorder() + newResearchRouter(h).ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + var resp map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v", err) + } + if _, ok := resp["version"]; !ok { + t.Errorf("response missing 'version' field; got %v", resp) + } +} + +func TestAdminResearchMissingArt_NonAdmin_403(t *testing.T) { + if os.Getenv("MINSTREL_TEST_DATABASE_URL") == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + coverart.ResetRegistryForTests() + t.Cleanup(coverart.ResetRegistryForTests) + + coverart.Register(&apiTestAlbumProvider{id: "research-prov2", display: "Research Provider 2"}) + + h, _ := testHandlersWithCoverSettings(t) + user := seedUser(t, h.pool, "researchnonadmin", "pw", false) + + req := httptest.NewRequest(http.MethodPost, "/api/admin/cover-sources/research", nil) + req = withUser(req, user) + rec := httptest.NewRecorder() + newResearchRouter(h).ServeHTTP(rec, req) + + if rec.Code != http.StatusForbidden { + t.Errorf("status = %d, want 403", rec.Code) + } +} diff --git a/internal/api/api.go b/internal/api/api.go index 8cda7a5c..0435d301 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -124,6 +124,7 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev admin.Get("/cover-sources", h.handleListCoverSources) admin.Patch("/cover-sources/{provider_id}", h.handleUpdateCoverSource) admin.Post("/cover-sources/{provider_id}/test", h.handleTestCoverSource) + admin.Post("/cover-sources/research", h.handleResearchMissingArt) }) authed.Get("/playlists", h.handleListPlaylists) diff --git a/internal/coverart/settings.go b/internal/coverart/settings.go index c1ce6ebb..ab025cb7 100644 --- a/internal/coverart/settings.go +++ b/internal/coverart/settings.go @@ -311,6 +311,29 @@ func registeredProvidersHash() string { return hex.EncodeToString(sum[:]) } +// BumpVersion unconditionally increments cover_art_sources_meta.current_version +// and returns the new value. Used by the manual "Re-search missing art" +// admin endpoint when the operator wants every 'none' row re-attempted +// on the next enrichment pass. +// +// Distinct from BumpVersionIfProvidersChanged: that method is the boot- +// time auto-bump and skips the DB write when the registered-provider +// hash matches. This method always writes. +func (s *SettingsService) BumpVersion(ctx context.Context) (int32, error) { + q := dbq.New(s.pool) + // Reuse BumpVersionAndSetProvidersHash with the current registered- + // providers hash; this keeps the stored hash consistent (a manual + // bump is conceptually equivalent to "force the auto-bump path"). + newVer, err := q.BumpVersionAndSetProvidersHash(ctx, registeredProvidersHash()) + if err != nil { + return 0, fmt.Errorf("bump version: %w", err) + } + s.mu.Lock() + s.currentVersion = newVer + s.mu.Unlock() + return newVer, nil +} + // TestProvider invokes TestConnection on the named provider if it // implements TestableProvider. Returns ErrNotTestable if the // provider doesn't, ErrProviderNotFound if not registered. diff --git a/internal/coverart/settings_test.go b/internal/coverart/settings_test.go index 9dd1a6e7..a43496cb 100644 --- a/internal/coverart/settings_test.go +++ b/internal/coverart/settings_test.go @@ -295,6 +295,44 @@ func TestSettingsService_BumpVersionIfProvidersChanged_NoChange(t *testing.T) { } } +// TestSettingsService_BumpVersion_Increments verifies the manual bump +// path. Distinct from BumpVersionIfProvidersChanged, this always writes +// regardless of the stored provider-set hash. +func TestSettingsService_BumpVersion_Increments(t *testing.T) { + pool := newPool(t) + resetRegistryForTests() + defer resetRegistryForTests() + + Register(&fakeAlbumProvider{fakeProvider: fakeProvider{id: "fake-album-bump", defaultOn: true}}) + + s, err := NewSettingsService(context.Background(), pool, discardLogger()) + if err != nil { + t.Fatalf("init: %v", err) + } + + before := s.CurrentVersion() + newVer, err := s.BumpVersion(context.Background()) + if err != nil { + t.Fatalf("bump: %v", err) + } + if newVer <= before { + t.Errorf("newVer = %d, want > %d", newVer, before) + } + if s.CurrentVersion() != newVer { + t.Errorf("in-memory version = %d, want %d", s.CurrentVersion(), newVer) + } + + // Calling again increments by another 1 (always writes). + afterFirst := newVer + newVer2, err := s.BumpVersion(context.Background()) + if err != nil { + t.Fatalf("second bump: %v", err) + } + if newVer2 != afterFirst+1 { + t.Errorf("second newVer = %d, want %d", newVer2, afterFirst+1) + } +} + // TestRegisteredProvidersHash_StableAcrossOrder: registry order // shouldn't matter because we sort before hashing. func TestRegisteredProvidersHash_StableAcrossOrder(t *testing.T) { diff --git a/web/src/lib/api/admin.research-art.test.ts b/web/src/lib/api/admin.research-art.test.ts new file mode 100644 index 00000000..b482700b --- /dev/null +++ b/web/src/lib/api/admin.research-art.test.ts @@ -0,0 +1,19 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { researchMissingArt } from './admin'; + +vi.mock('./client', () => ({ + api: { get: vi.fn(), post: vi.fn(), patch: vi.fn() } +})); + +import { api } from './client'; + +describe('admin research missing art API', () => { + beforeEach(() => vi.clearAllMocks()); + + it('researchMissingArt POSTs the correct path and returns version', async () => { + (api.post as unknown as ReturnType).mockResolvedValueOnce({ version: 7 }); + const got = await researchMissingArt(); + expect(api.post).toHaveBeenCalledWith('/api/admin/cover-sources/research', {}); + expect(got.version).toBe(7); + }); +}); diff --git a/web/src/lib/api/admin.ts b/web/src/lib/api/admin.ts index b2412954..2a3d1efd 100644 --- a/web/src/lib/api/admin.ts +++ b/web/src/lib/api/admin.ts @@ -320,6 +320,17 @@ export async function testCoverProvider(id: string): Promise(`/api/admin/cover-sources/${id}/test`, {}); } +// Re-search missing art -------------------------------------------------- + +export type ResearchMissingArtResponse = { version: number }; + +// Bumps cover_art_sources_meta.current_version. Every 'none' row becomes +// eligible for retry against the current provider chain on the next +// enrichment pass. Existing positively-sourced rows are not affected. +export async function researchMissingArt(): Promise { + return api.post('/api/admin/cover-sources/research', {}); +} + // Settings rarely change in operator time. 60s staleTime; refetches // on window focus + after any mutation rather than polling. export function createCoverProvidersQuery() { diff --git a/web/src/routes/admin/+page.svelte b/web/src/routes/admin/+page.svelte index 7bf0f223..fa0712c2 100644 --- a/web/src/routes/admin/+page.svelte +++ b/web/src/routes/admin/+page.svelte @@ -19,6 +19,7 @@ deleteQuarantineViaLidarr, refetchMissingCovers, triggerScan, + researchMissingArt, type ScanSchedule, type ScanScheduleMode } from '$lib/api/admin'; @@ -315,6 +316,21 @@ let bulkBusy = $state(false); let bulkResult = $state(null); + // ---- Re-search missing art ---- + let researchSaving = $state(false); + + async function onResearchMissingArt() { + researchSaving = true; + try { + await researchMissingArt(); + showToast('All previously-failed art will be re-attempted on the next scan.'); + } catch (e) { + showToast(`Re-search failed: ${(e as { code?: string })?.code ?? 'unknown'}`); + } finally { + researchSaving = false; + } + } + async function onBulkRefetch() { if (bulkBusy) return; bulkBusy = true; @@ -603,6 +619,14 @@ > {bulkBusy ? 'Queueing…' : 'Refetch missing covers'} + {#if coverage} {#if coverage.total === 0} diff --git a/web/src/routes/admin/admin.test.ts b/web/src/routes/admin/admin.test.ts index c76c2576..b911a9dc 100644 --- a/web/src/routes/admin/admin.test.ts +++ b/web/src/routes/admin/admin.test.ts @@ -42,6 +42,7 @@ vi.mock('$lib/api/admin', async () => { deleteQuarantineViaLidarr: vi.fn().mockResolvedValue({}), triggerScan: vi.fn().mockResolvedValue({}), refetchMissingCovers: vi.fn().mockResolvedValue({ queued: 0 }), + researchMissingArt: vi.fn().mockResolvedValue({ version: 1 }), createScanScheduleQuery: vi.fn(() => readable({ data: { @@ -111,7 +112,8 @@ import { rejectRequest, resolveQuarantine, deleteQuarantineFile, - deleteQuarantineViaLidarr + deleteQuarantineViaLidarr, + researchMissingArt } from '$lib/api/admin'; import OverviewPage from './+page.svelte'; @@ -264,6 +266,15 @@ describe('admin Overview page', () => { expect(select.value).toBe('off'); }); + test('renders Re-search missing art button and invokes on click', async () => { + setupOverview(); + render(OverviewPage); + const btn = screen.getByRole('button', { name: /re-search missing art/i }); + expect(btn).toBeInTheDocument(); + await fireEvent.click(btn); + expect(researchMissingArt).toHaveBeenCalled(); + }); + test('Library scan card shows Processing badge when library tally is populated and in-flight', async () => { const { readable } = await import('svelte/store'); (createScanStatusQuery as ReturnType).mockReturnValueOnce(