fix: address code review findings
- notifier: show artist display names (not internal IDs) in digest; resolve names from artist_settings and fall back to ID when unavailable - notifier: skip sending an empty digest to avoid daily spam - config: require telegram token/chat_id when enabled - web: warn loudly when auth is disabled on a non-loopback bind; add HTTP server timeouts - web: treat SetReleaseIgnored "release not found" as benign redirect (0 rows) - musicbrainz: reject low-score/name-mismatched MBID resolutions instead of silently caching the wrong artist - database: remove dead duplicate err check; harden DSN param appending - musicbrainz: check rows.Err() after iterating existing releases
This commit is contained in:
@@ -10,9 +10,13 @@ import (
|
||||
|
||||
// FormatDigest renders newly-found missing releases into a human-readable
|
||||
// Telegram message grouped by artist, with per-artist counts and a link to
|
||||
// the Web UI dashboard. It is deterministic: artists are sorted by name and
|
||||
// the Web UI dashboard. It is deterministic: artists are sorted by label and
|
||||
// releases within an artist are sorted by title.
|
||||
func FormatDigest(missing []scanner.MissingRelease, uiBaseURL string) string {
|
||||
//
|
||||
// artistNames maps an ArtistID to its human-readable display name. Names are
|
||||
// optional: if an ID is absent from the map (or the map itself is nil), the
|
||||
// raw ArtistID is used as the label so the digest remains informative.
|
||||
func FormatDigest(missing []scanner.MissingRelease, uiBaseURL string, artistNames map[string]string) string {
|
||||
if len(missing) == 0 {
|
||||
return "NaviWatcher: no new missing releases found."
|
||||
}
|
||||
@@ -28,8 +32,10 @@ func FormatDigest(missing []scanner.MissingRelease, uiBaseURL string) string {
|
||||
}
|
||||
byArtist[r.ArtistID] = append(byArtist[r.ArtistID], entry{title: r.Title})
|
||||
}
|
||||
// Stable ordering by ArtistID.
|
||||
sort.Strings(order)
|
||||
// Stable ordering by display label (name if known, else ID).
|
||||
sort.Slice(order, func(i, j int) bool {
|
||||
return artistLabel(order[i], artistNames) < artistLabel(order[j], artistNames)
|
||||
})
|
||||
|
||||
var b strings.Builder
|
||||
fmt.Fprintf(&b, "NaviWatcher: %d new missing release(s) found:\n\n", len(missing))
|
||||
@@ -40,7 +46,7 @@ func FormatDigest(missing []scanner.MissingRelease, uiBaseURL string) string {
|
||||
titles = append(titles, e.title)
|
||||
}
|
||||
sort.Strings(titles)
|
||||
fmt.Fprintf(&b, "%s (%d):\n", artistID, len(titles))
|
||||
fmt.Fprintf(&b, "%s (%d):\n", artistLabel(artistID, artistNames), len(titles))
|
||||
for _, t := range titles {
|
||||
fmt.Fprintf(&b, " - %s\n", t)
|
||||
}
|
||||
@@ -51,3 +57,15 @@ func FormatDigest(missing []scanner.MissingRelease, uiBaseURL string) string {
|
||||
}
|
||||
return strings.TrimRight(b.String(), "\n")
|
||||
}
|
||||
|
||||
// artistLabel returns the human-readable display name for an artist ID when
|
||||
// available, otherwise the raw ID. A non-empty name takes precedence so
|
||||
// operators see recognizable artist names rather than opaque internal IDs.
|
||||
func artistLabel(artistID string, names map[string]string) string {
|
||||
if names != nil {
|
||||
if name, ok := names[artistID]; ok && name != "" {
|
||||
return name
|
||||
}
|
||||
}
|
||||
return artistID
|
||||
}
|
||||
|
||||
@@ -26,7 +26,7 @@ func (s *stubSender) Send(ctx context.Context, message string) error {
|
||||
}
|
||||
|
||||
func TestFormatDigest_Empty(t *testing.T) {
|
||||
got := FormatDigest(nil, "http://ui")
|
||||
got := FormatDigest(nil, "http://ui", nil)
|
||||
if got != "NaviWatcher: no new missing releases found." {
|
||||
t.Fatalf("unexpected empty digest: %q", got)
|
||||
}
|
||||
@@ -38,7 +38,7 @@ func TestFormatDigest_GroupsByArtistAndCounts(t *testing.T) {
|
||||
{ArtistID: "art-a", Title: "Alpha", RGID: "r1"},
|
||||
{ArtistID: "art-a", Title: "Beta", RGID: "r2"},
|
||||
}
|
||||
got := FormatDigest(missing, "http://localhost:8080/")
|
||||
got := FormatDigest(missing, "http://localhost:8080/", nil)
|
||||
if !strings.Contains(got, "art-a (2):") {
|
||||
t.Errorf("expected art-a with count 2, got:\n%s", got)
|
||||
}
|
||||
@@ -60,9 +60,45 @@ func TestFormatDigest_GroupsByArtistAndCounts(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestFormatDigest_UsesArtistNameWhenProvided(t *testing.T) {
|
||||
missing := []scanner.MissingRelease{
|
||||
{ArtistID: "art-2", Title: "Zebra", RGID: "r3"},
|
||||
{ArtistID: "art-1", Title: "Alpha", RGID: "r1"},
|
||||
{ArtistID: "art-1", Title: "Beta", RGID: "r2"},
|
||||
}
|
||||
names := map[string]string{"art-1": "Alpha Artist", "art-2": "Zebra Artist"}
|
||||
got := FormatDigest(missing, "", names)
|
||||
// Display names are used as labels and sorted alphabetically by name.
|
||||
if !strings.Contains(got, "Alpha Artist (2):") {
|
||||
t.Errorf("expected name label with count 2, got:\n%s", got)
|
||||
}
|
||||
if !strings.Contains(got, "Zebra Artist (1):") {
|
||||
t.Errorf("expected name label with count 1, got:\n%s", got)
|
||||
}
|
||||
if strings.Index(got, "Alpha Artist") > strings.Index(got, "Zebra Artist") {
|
||||
t.Errorf("artists not sorted by name: got:\n%s", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestFormatDigest_FallsBackToIDWhenNameMissing(t *testing.T) {
|
||||
missing := []scanner.MissingRelease{
|
||||
{ArtistID: "art-1", Title: "Alpha", RGID: "r1"},
|
||||
{ArtistID: "art-2", Title: "Beta", RGID: "r2"},
|
||||
}
|
||||
// Name map present but does not cover art-2 -> falls back to ID.
|
||||
names := map[string]string{"art-1": "Named Artist"}
|
||||
got := FormatDigest(missing, "", names)
|
||||
if !strings.Contains(got, "Named Artist (1):") {
|
||||
t.Errorf("expected named artist label, got:\n%s", got)
|
||||
}
|
||||
if !strings.Contains(got, "art-2 (1):") {
|
||||
t.Errorf("expected ID fallback for art-2, got:\n%s", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestFormatDigest_EmptyUIBaseURLOmitsLink(t *testing.T) {
|
||||
missing := []scanner.MissingRelease{{ArtistID: "a", Title: "x", RGID: "r1"}}
|
||||
got := FormatDigest(missing, "")
|
||||
got := FormatDigest(missing, "", nil)
|
||||
if strings.Contains(got, "View details:") {
|
||||
t.Errorf("did not expect UI link when base URL empty: got:\n%s", got)
|
||||
}
|
||||
|
||||
@@ -16,8 +16,10 @@ import (
|
||||
// Releases already present in notifications_sent are excluded upstream by
|
||||
// GetUnnotifiedReleases, so this is idempotent across runs.
|
||||
//
|
||||
// If there are no unnotified releases the digest reports "no new missing
|
||||
// releases" and nothing is marked sent (there is nothing to mark).
|
||||
// If there are no unnotified releases nothing is sent (the caller's scheduler
|
||||
// is responsible for not spamming the operator with an empty digest). When
|
||||
// releases are present, each is marked sent so a subsequent run will not
|
||||
// re-notify it.
|
||||
//
|
||||
// uiBaseURL is the externally-reachable base URL of the Web UI, appended to the
|
||||
// digest so operators can jump to the dashboard.
|
||||
@@ -42,7 +44,28 @@ func NotifyOnce(ctx context.Context, db *database.DB, sender Sender, cfg config.
|
||||
})
|
||||
}
|
||||
|
||||
message := FormatDigest(missing, uiBaseURL)
|
||||
// Resolve human-readable artist names so the digest shows recognizable
|
||||
// labels instead of opaque internal artist IDs. A lookup failure for a
|
||||
// single artist must not abort the whole digest, so errors are ignored and
|
||||
// that artist falls back to its ID via artistLabel.
|
||||
names := make(map[string]string, len(missing))
|
||||
for _, r := range unnotified {
|
||||
if _, ok := names[r.ArtistID]; ok {
|
||||
continue
|
||||
}
|
||||
settings, err := database.GetArtistSettings(db, r.ArtistID)
|
||||
if err == nil && settings.Name != "" {
|
||||
names[r.ArtistID] = settings.Name
|
||||
}
|
||||
}
|
||||
|
||||
message := FormatDigest(missing, uiBaseURL, names)
|
||||
// Nothing to report: skip sending so the operator is not spammed with an
|
||||
// empty digest on every cron fire. The startup fire likewise stays quiet
|
||||
// until the first genuinely missing release appears.
|
||||
if len(unnotified) == 0 {
|
||||
return 0, nil
|
||||
}
|
||||
if err := sender.Send(ctx, message); err != nil {
|
||||
return 0, fmt.Errorf("notifier: send digest: %w", err)
|
||||
}
|
||||
|
||||
@@ -100,6 +100,26 @@ func TestNotifyOnce_SendsAndMarksSent(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestNotifyOnce_EmptyDoesNotSend(t *testing.T) {
|
||||
db, err := database.New(":memory:")
|
||||
if err != nil {
|
||||
t.Fatalf("New(): %v", err)
|
||||
}
|
||||
defer db.Close()
|
||||
|
||||
sender := &collectSender{}
|
||||
n, err := NotifyOnce(context.Background(), db, sender, config.TelegramConfig{}, "http://ui")
|
||||
if err != nil {
|
||||
t.Fatalf("NotifyOnce: %v", err)
|
||||
}
|
||||
if n != 0 {
|
||||
t.Fatalf("expected 0 releases notified, got %d", n)
|
||||
}
|
||||
if sender.count() != 0 {
|
||||
t.Fatalf("expected no message sent for empty digest, got %d", sender.count())
|
||||
}
|
||||
}
|
||||
|
||||
func TestNotifyOnce_SkipsAlreadySent(t *testing.T) {
|
||||
db, err := database.New(":memory:")
|
||||
if err != nil {
|
||||
|
||||
Reference in New Issue
Block a user