From 85c42ec85821d5cce9002f590452687fce7a4bc2 Mon Sep 17 00:00:00 2001 From: Vladimir Zagainov Date: Sun, 19 Jul 2026 22:13:57 +0300 Subject: [PATCH] feat: add mbid column to artist_settings Additive migration 008 adds an mbid column to artist_settings, extend the ArtistSettings struct and persistence functions (SaveArtistSettings, GetArtistSettings, GetAllArtistSettings, UpdateArtistSettings) to carry the MusicBrainz ID, and add round-trip tests covering empty and set MBID values. --- docs/plans/2026-07-19-notifier-webui-sync.md | 163 +++++++++++++++++++ internal/database/artist_settings.go | 25 ++- internal/database/artist_settings_test.go | 155 ++++++++++++++++++ internal/database/database.go | 5 + internal/database/database_test.go | 9 +- 5 files changed, 346 insertions(+), 11 deletions(-) create mode 100644 docs/plans/2026-07-19-notifier-webui-sync.md diff --git a/docs/plans/2026-07-19-notifier-webui-sync.md b/docs/plans/2026-07-19-notifier-webui-sync.md new file mode 100644 index 0000000..dc3dad0 --- /dev/null +++ b/docs/plans/2026-07-19-notifier-webui-sync.md @@ -0,0 +1,163 @@ +# Notifier, Web UI & Sync Wiring + +## Overview +Transform NaviWatcher from a compute-only daemon (which only logs missing +releases) into a working service: +1. **Sync wiring** in `main.run()` — a periodic loop that pulls artists/albums + from Navidrome into the DB, resolves each artist's MusicBrainz ID, syncs + their discography, then runs the scanner. This is the missing data pipeline + that currently makes every other component a no-op against an empty DB. +2. **Notifier** — Telegram bot + cron scheduler that sends a daily digest of + newly-found missing releases (using existing `notifications_sent` primitives). +3. **Web UI** — `net/http` + `html/template` dashboard with artist detail view, + archive of ignored releases, ignore actions, and basic auth. + +Problem solved: today `main.run()` calls `scanner.ScanAll` over empty tables and +blocks on `<-ctx.Done()`. Nothing populates `artist_settings` / `local_albums` / +`external_releases`, so Notifier and Web UI have nothing to show. This plan +closes that gap end-to-end. + +Out of scope (deferred): Docker packaging, secondary-type filtering +(`Live`/`Remix`/`Soundtrack`), bootleg exclusion at the engine level, full +type-filtering config toggles. Per-artist `ignore_singles`/`ignore_compilations` +filtering already works inside `musicbrainz.SyncArtistDiscography`. + +## Context (from discovery) +- `internal/navidrome/{client,sync}.go` — `SyncArtists`, `SyncAlbums` exist and + write `artist_settings` / `local_albums`. **Gap:** `ArtistInfo` only carries + Navidrome `ID`/`Name`; there is **no MusicBrainz ID (MBID)**, but + `musicbrainz.SyncArtistDiscography` requires `artistMBID`. +- `internal/musicbrainz/{client,sync,api}.go` — `SyncArtistDiscography(ctx, client, + db, artistID, artistMBID, ttl)` works once MBID is known. `getArtistFilterOptions` + already reads `ignore_singles`/`ignore_compilations`. +- `internal/database/` — `GetAllArtistSettings`, `GetExternalReleasesByArtist`, + `GetUnnotifiedReleases`, `MarkNotificationSent`, `GetIgnoredReleases`, + `SetReleaseIgnored` all exist and are tested. `artist_settings` schema has no + MBID column. +- `internal/scanner/scan.go` — `ScanAll(ctx, db, threshold)` returns + `[]MissingRelease`; tested and working. +- `cmd/naviwatcher/main.go` — `App` holds cfg/db/mbClient only; `run()` is the + compute-only stub. `NewApp` constructs the MusicBrainz client but **not** the + Navidrome client. No goroutines for sync/notifier/web. +- `internal/config/config.go` — `TelegramConfig{Enabled,Token,ChatID,CronSchedule}` + and `ServerConfig{Host,Port,Username,Password}` already defined but unused. +- `config.yaml.example` exists. + +### Key decision: how to obtain the MusicBrainz ID +`SyncArtistDiscography` needs an MBID. Navidrome's Subsonic API does not return +MBIDs via `getArtists`/`getArtist`. Resolution: **add an `mbid` column to +`artist_settings`** and resolve it lazily during sync by querying MusicBrainz +artist search (`/ws/2/artist/?query=artist:&fmt=json`). Cache the MBID on +the artist row. This avoids manual config and keeps the schema the single source +of truth. (Alternative considered: resolve by name on every sync without +storing — rejected because it doubles rate-limited MB calls and is flaky on +name collisions.) + +## Development Approach +- **Testing approach**: TDD — write tests before implementation for each task. +- Complete each task fully (code + tests passing) before the next. +- Every task MUST include new/updated tests (success + error/edge cases). +- All tests must pass before starting the next task. +- Run `go test ./...` and `go vet ./...` after each task. +- Maintain backward compatibility of existing DB schema (additive migration only). + +## Testing Strategy +- **Unit tests** for every new function/method (success + error paths). +- **Integration-style tests** for sync/resolver using a `:memory:` DB and a + stubbed MusicBrainz HTTP client (the existing `client_test.go` already shows + the httptest pattern — reuse it). +- **Web UI**: table-driven tests for handlers (status codes, auth rejection, + ignore action effects on DB) using `httptest.NewServer` + in-memory DB. No + Playwright/Cypress in this project, so no e2e suite; handler tests cover the + equivalent surface. +- **Notifier**: test digest formatting and the sent-tracking logic against + `:memory:` DB with a stubbed Telegram sender (interface so the real HTTP bot + is injectable). + +## Progress Tracking +- Mark completed items with `[x]` immediately when done. +- Add newly discovered tasks with ➕ prefix. +- Document issues/blockers with ⚠️ prefix. +- Keep plan in sync with actual work. + +## Implementation Steps + +### Task 1: Add `mbid` column to artist_settings +- [x] add migration `006_add_mbid_to_artist_settings` (`ALTER TABLE artist_settings ADD COLUMN mbid TEXT;`) +- [x] extend `ArtistSettings` struct + `SaveArtistSettings`/`UpsertArtist` to persist `MBID` +- [x] write tests for migration + struct round-trip (empty MBID allowed, set/get) +- [x] run tests - must pass before task 2 + +### Task 2: MusicBrainz artist-ID resolver +- [ ] add `ResolveArtistMBID(ctx, client, name) (string, error)` in `internal/musicbrainz` using `/ws/2/artist/?query=artist:&fmt=json` +- [ ] parse first matching artist ID from JSON response; return error if none +- [ ] write tests with httptest stub (match found, no match, HTTP error) +- [ ] run tests - must pass before task 3 + +### Task 3: Periodic sync pipeline +- [ ] add `SyncAll(ctx, ndClient, mbClient, db, ttl)` orchestrator: for each monitored artist → ensure MBID (resolve + persist if missing) → `musicbrainz.SyncArtistDiscography` → `navidrome.SyncAlbums` +- [ ] wire `navidrome.NewClient` into `App`; add `ndClient` field +- [ ] write tests for `SyncAll` with stubbed clients + `:memory:` DB (new artist gets MBID, existing MBID reused, unmonitored skipped) +- [ ] run tests - must pass before task 4 + +### Task 4: Main loop wiring (sync → scan) +- [ ] replace compute-only `run()` with: one immediate sync+scan, then a ticker-driven periodic sync+scan goroutine; keep graceful shutdown via ctx +- [ ] add a `syncInterval` config field (default e.g. 6h) to `config.go` + defaults + validation +- [ ] write tests for the loop scheduling logic where feasible (ticker fires, ctx cancels cleanly) +- [ ] run tests - must pass before task 5 + +### Task 5: Notifier — Telegram sender + digest +- [ ] define `Sender` interface (`Send(ctx, message string) error`) and a `telegramSender` using `TelegramConfig` (bot API `sendMessage`) +- [ ] add `FormatDigest(missing []scanner.MissingRelease, uiBaseURL string) string` (artist names + counts + Web UI link) +- [ ] write tests: digest formatting, sender failure handling (stub sender) +- [ ] run tests - must pass before task 6 + +### Task 6: Notifier — scheduler + sent-tracking +- [ ] add `NotifyOnce(ctx, db, sender, cfg)` : query `GetUnnotifiedReleases`, build digest, send, `MarkNotificationSent` per rgid +- [ ] add cron-based scheduler goroutine honoring `TelegramConfig.CronSchedule` (use a lightweight cron lib or robfig/cron); no-op if `Enabled=false` +- [ ] write tests: `NotifyOnce` marks sent and skips already-sent; scheduler parses cron and fires (inject fixed time / use every-minute for test) +- [ ] run tests - must pass before task 7 + +### Task 7: Web UI — server + auth + dashboard +- [ ] create `internal/web` with `Server` (net/http), `//go:embed` templates, basic-auth middleware using `ServerConfig.Username/Password` +- [ ] dashboard handler: list monitored artists with missing-release counts (join scanner result / external vs local) +- [ ] write tests: unauthenticated request → 401; authenticated → 200 with expected artist rendered +- [ ] run tests - must pass before task 8 + +### Task 8: Web UI — artist detail + archive + ignore actions +- [ ] artist page: local albums (Subsonic) + found missing (MB cache) + ignore buttons +- [ ] archive page: `GetIgnoredReleases` with restore action +- [ ] POST handlers: `SetReleaseIgnored(rgid, true/false)`; "ignore all singles of artist" toggles `artist_settings.ignore_singles` +- [ ] write tests: ignore sets flag + removes from dashboard missing; restore clears flag; auth enforced on POST +- [ ] run tests - must pass before task 9 + +### Task 9: Verify acceptance criteria +- [ ] run full suite `go test ./...` — all pass +- [ ] run `go vet ./...` and `go build -o naviwatcher` — clean +- [ ] verify scan→notify→web data flow with a seeded `:memory:`/file DB smoke check +- [ ] verify config.yaml.example documents new `sync_interval` field + +### Task 10: Update documentation +- [ ] add a short "How it works now" note to README/CLAUDE.md if present +- [ ] note the new `sync_interval` config key in `config.yaml.example` + +## Technical Details +- New migration `006` is additive; existing rows get `mbid = NULL` and are + resolved lazily on first sync. +- `SyncAll` ordering matters: Navidrome first (populates `artist_settings`), + then MBID resolution, then MusicBrainz discography, then albums. +- Notifier `Sender` interface keeps the real Telegram HTTP call injectable for + tests; respects MusicBrainz-style rate limiting only on the MB client, not TG. +- Web UI basic auth uses `crypto/subtle.ConstantTimeCompare` on + `base64(user:pass)` per RFC 7617; no session/cookie needed. +- Cron: `robfig/cron/v3` is the conventional choice; if dependency minimalism is + preferred, a tiny "every N hours" ticker can replace cron — will confirm at + implementation if not specified. + +## Post-Completion +*Informational — no checkboxes* +- **Manual verification**: run binary against a real Navidrome + MusicBrainz, + confirm dashboard populates, Telegram digest arrives at `cron_schedule`, + ignore/restore actions persist. +- **External**: ensure `config.yaml.example` matches deployed config; Telegram + bot token/chat_id must be supplied by operator. diff --git a/internal/database/artist_settings.go b/internal/database/artist_settings.go index 6978bf1..1b3fc8b 100644 --- a/internal/database/artist_settings.go +++ b/internal/database/artist_settings.go @@ -1,28 +1,33 @@ package database import ( + "database/sql" "fmt" ) // GetArtistSettings retrieves an artist_settings row by ID. // Returns sql.ErrNoRows if the artist is not found. func GetArtistSettings(db *DB, id string) (*ArtistSettings, error) { - var s ArtistSettings + var ( + s ArtistSettings + mbid sql.NullString + ) err := db.Conn().QueryRow( - "SELECT id, name, ignore_singles, ignore_compilations, monitored FROM artist_settings WHERE id = ?", + "SELECT id, name, mbid, ignore_singles, ignore_compilations, monitored FROM artist_settings WHERE id = ?", id, - ).Scan(&s.ID, &s.Name, &s.IgnoreSingles, &s.IgnoreCompilations, &s.Monitored) + ).Scan(&s.ID, &s.Name, &mbid, &s.IgnoreSingles, &s.IgnoreCompilations, &s.Monitored) if err != nil { return nil, err } + s.MBID = mbid.String return &s, nil } // SaveArtistSettings inserts or replaces an artist_settings row. func SaveArtistSettings(db *DB, settings *ArtistSettings) error { _, err := db.Conn().Exec( - "INSERT OR REPLACE INTO artist_settings (id, name, ignore_singles, ignore_compilations, monitored) VALUES (?, ?, ?, ?, ?)", - settings.ID, settings.Name, settings.IgnoreSingles, settings.IgnoreCompilations, settings.Monitored, + "INSERT OR REPLACE INTO artist_settings (id, name, mbid, ignore_singles, ignore_compilations, monitored) VALUES (?, ?, ?, ?, ?, ?)", + settings.ID, settings.Name, settings.MBID, settings.IgnoreSingles, settings.IgnoreCompilations, settings.Monitored, ) if err != nil { return fmt.Errorf("save artist settings: %w", err) @@ -33,7 +38,7 @@ func SaveArtistSettings(db *DB, settings *ArtistSettings) error { // GetAllArtistSettings returns all rows from artist_settings. func GetAllArtistSettings(db *DB) ([]ArtistSettings, error) { rows, err := db.Conn().Query( - "SELECT id, name, ignore_singles, ignore_compilations, monitored FROM artist_settings", + "SELECT id, name, mbid, ignore_singles, ignore_compilations, monitored FROM artist_settings", ) if err != nil { return nil, fmt.Errorf("query all artist settings: %w", err) @@ -43,7 +48,7 @@ func GetAllArtistSettings(db *DB) ([]ArtistSettings, error) { var results []ArtistSettings for rows.Next() { var s ArtistSettings - if err := rows.Scan(&s.ID, &s.Name, &s.IgnoreSingles, &s.IgnoreCompilations, &s.Monitored); err != nil { + if err := rows.Scan(&s.ID, &s.Name, &s.MBID, &s.IgnoreSingles, &s.IgnoreCompilations, &s.Monitored); err != nil { return nil, fmt.Errorf("scan artist settings: %w", err) } results = append(results, s) @@ -74,6 +79,12 @@ func UpdateArtistSettings(db *DB, id string, updates map[string]interface{}) err } setClause += "name = ?" args = append(args, val) + case "mbid": + if setClause != "" { + setClause += ", " + } + setClause += "mbid = ?" + args = append(args, val) case "ignore_singles": if setClause != "" { setClause += ", " diff --git a/internal/database/artist_settings_test.go b/internal/database/artist_settings_test.go index cbcf92f..7b50977 100644 --- a/internal/database/artist_settings_test.go +++ b/internal/database/artist_settings_test.go @@ -305,3 +305,158 @@ func TestUpdateArtistSettings_EmptyUpdates(t *testing.T) { t.Error("expected error for empty updates, got nil") } } + +// TestMigration008_MbidColumnExists verifies the 008 migration adds the mbid +// column and that rows created before resolution have a NULL/empty MBID. +func TestMigration008_MbidColumnExists(t *testing.T) { + db, err := New(":memory:") + if err != nil { + t.Fatalf("New() error: %v", err) + } + defer db.Close() + + // Insert a row without supplying mbid (simulates a pre-resolution row). + if _, err := db.Conn().Exec( + "INSERT INTO artist_settings (id, name) VALUES (?, ?)", + "artist-1", "No MBID Yet", + ); err != nil { + t.Fatalf("insert without mbid: %v", err) + } + + var mbid sql.NullString + if err := db.Conn().QueryRow( + "SELECT mbid FROM artist_settings WHERE id = ?", "artist-1", + ).Scan(&mbid); err != nil { + t.Fatalf("query mbid: %v", err) + } + if mbid.Valid && mbid.String != "" { + t.Errorf("expected empty mbid for pre-resolution row, got %q", mbid.String) + } +} + +// TestArtistSettings_MbidRoundTrip verifies Save/Get round-trips an MBID, +// and that an empty MBID is preserved (not overwritten with garbage). +func TestArtistSettings_MbidRoundTrip(t *testing.T) { + db, err := New(":memory:") + if err != nil { + t.Fatalf("New() error: %v", err) + } + defer db.Close() + + s := &ArtistSettings{ + ID: "artist-1", + Name: "Test Artist", + MBID: "f27e6623-8771-4a2e-8dcb-6c8b1a4f8b9a", + Monitored: true, + } + if err := SaveArtistSettings(db, s); err != nil { + t.Fatalf("SaveArtistSettings() error: %v", err) + } + + got, err := GetArtistSettings(db, "artist-1") + if err != nil { + t.Fatalf("GetArtistSettings() error: %v", err) + } + if got.MBID != s.MBID { + t.Errorf("expected MBID %q, got %q", s.MBID, got.MBID) + } +} + +// TestArtistSettings_MbidEmptyAllowed verifies an artist can be saved and +// retrieved with no MBID set (lazy resolution not yet performed). +func TestArtistSettings_MbidEmptyAllowed(t *testing.T) { + db, err := New(":memory:") + if err != nil { + t.Fatalf("New() error: %v", err) + } + defer db.Close() + + s := &ArtistSettings{ID: "artist-1", Name: "No MBID", Monitored: true} + if err := SaveArtistSettings(db, s); err != nil { + t.Fatalf("SaveArtistSettings() error: %v", err) + } + + got, err := GetArtistSettings(db, "artist-1") + if err != nil { + t.Fatalf("GetArtistSettings() error: %v", err) + } + if got.MBID != "" { + t.Errorf("expected empty MBID, got %q", got.MBID) + } +} + +// TestArtistSettings_MbidUpdatePersists verifies UpdateArtistSettings can set +// and clear the MBID column. +func TestArtistSettings_MbidUpdatePersists(t *testing.T) { + db, err := New(":memory:") + if err != nil { + t.Fatalf("New() error: %v", err) + } + defer db.Close() + + if err := SaveArtistSettings(db, &ArtistSettings{ID: "artist-1", Name: "Test", Monitored: true}); err != nil { + t.Fatalf("SaveArtistSettings() error: %v", err) + } + + mbid := "f27e6623-8771-4a2e-8dcb-6c8b1a4f8b9a" + if err := UpdateArtistSettings(db, "artist-1", map[string]interface{}{"mbid": mbid}); err != nil { + t.Fatalf("UpdateArtistSettings(set mbid) error: %v", err) + } + got, err := GetArtistSettings(db, "artist-1") + if err != nil { + t.Fatalf("GetArtistSettings() error: %v", err) + } + if got.MBID != mbid { + t.Errorf("expected MBID %q after set, got %q", mbid, got.MBID) + } + + // Clear it again. + if err := UpdateArtistSettings(db, "artist-1", map[string]interface{}{"mbid": ""}); err != nil { + t.Fatalf("UpdateArtistSettings(clear mbid) error: %v", err) + } + got, err = GetArtistSettings(db, "artist-1") + if err != nil { + t.Fatalf("GetArtistSettings() error: %v", err) + } + if got.MBID != "" { + t.Errorf("expected empty MBID after clear, got %q", got.MBID) + } +} + +// TestArtistSettings_MbidInGetAll verifies GetAllArtistSettings returns the +// MBID field for all rows. +func TestArtistSettings_MbidInGetAll(t *testing.T) { + db, err := New(":memory:") + if err != nil { + t.Fatalf("New() error: %v", err) + } + defer db.Close() + + artists := []ArtistSettings{ + {ID: "a1", Name: "Artist One", MBID: "mbid-1", Monitored: true}, + {ID: "a2", Name: "Artist Two", Monitored: true}, + } + for _, a := range artists { + if err := SaveArtistSettings(db, &a); err != nil { + t.Fatalf("SaveArtistSettings(%s) error: %v", a.ID, err) + } + } + + results, err := GetAllArtistSettings(db) + if err != nil { + t.Fatalf("GetAllArtistSettings() error: %v", err) + } + if len(results) != 2 { + t.Fatalf("expected 2 results, got %d", len(results)) + } + byID := make(map[string]ArtistSettings) + for _, r := range results { + byID[r.ID] = r + } + if byID["a1"].MBID != "mbid-1" { + t.Errorf("artist a1: expected MBID 'mbid-1', got %q", byID["a1"].MBID) + } + if byID["a2"].MBID != "" { + t.Errorf("artist a2: expected empty MBID, got %q", byID["a2"].MBID) + } +} diff --git a/internal/database/database.go b/internal/database/database.go index 2ac5adc..ae84623 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -125,6 +125,10 @@ func (db *DB) migrate() error { name: "007_index_external_releases_artist_id", sql: `CREATE INDEX IF NOT EXISTS idx_external_releases_artist_id ON external_releases(artist_id);`, }, + { + name: "008_add_mbid_to_artist_settings", + sql: `ALTER TABLE artist_settings ADD COLUMN mbid TEXT;`, + }, } for _, m := range migrations { @@ -173,6 +177,7 @@ func (db *DB) isMigrationApplied(name string) (bool, error) { type ArtistSettings struct { ID string `json:"id"` Name string `json:"name"` + MBID string `json:"mbid"` IgnoreSingles bool `json:"ignore_singles"` IgnoreCompilations bool `json:"ignore_compilations"` Monitored bool `json:"monitored"` diff --git a/internal/database/database_test.go b/internal/database/database_test.go index b3b08df..67d74b5 100644 --- a/internal/database/database_test.go +++ b/internal/database/database_test.go @@ -205,10 +205,11 @@ func TestMigrationTracking(t *testing.T) { t.Fatalf("query migrations count: %v", err) } - // We have 7 recorded migrations: artist_settings, external_releases, + // We have 8 recorded migrations: artist_settings, external_releases, // local_albums, notifications_sent, cached_at column, secondary_types - // column, and the external_releases.artist_id index. - if count != 7 { - t.Errorf("expected 7 applied migrations, got %d", count) + // column, the external_releases.artist_id index, and the artist_settings + // mbid column. + if count != 8 { + t.Errorf("expected 8 applied migrations, got %d", count) } }