fix: address code review findings

2026-07-20 00:05:22 +03:00
parent ce1c39e14b
commit e0211343e0
7 changed files with 159 additions and 53 deletions
--- a/internal/notifier/scheduler.go
+++ b/internal/notifier/scheduler.go
@@ -11,71 +11,94 @@ import (
 	"naviwatcher/internal/scanner"
 )

-// NotifyOnce queries for releases that have not yet been notified, builds a
-// digest, sends it through the given Sender, and marks each release as sent.
-// Releases already present in notifications_sent are excluded upstream by
-// GetUnnotifiedReleases, so this is idempotent across runs.
+// NotifyOnce computes the releases that are genuinely missing for the user,
+// intersects that set with the releases not yet notified, builds a digest of
+// the result, sends it through the given Sender, and marks each release as
+// sent.
 //
-// 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.
+// "Missing" is the authoritative definition produced by the scanner: an
+// external release with no sufficiently similar local album (see
+// scanner.ScanAll). This intersection is what keeps the digest honest: a
+// release the user already owns in Navidrome must never be reported as a new
+// missing release, even though it still counts as "unnotified" on a fresh
+// database. Releases already in notifications_sent are excluded by
+// GetUnnotifiedReleases, so the call is idempotent across runs.
+//
+// threshold is the fuzzy-similarity cutoff passed through to the scanner; 0
+// selects the scanner's default. It must match config.Scanner.FuzzyThreshold
+// so the digest honors the operator's configured tolerance.
+//
+// If there are no newly-missing 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.
-func NotifyOnce(ctx context.Context, db *database.DB, sender Sender, cfg config.TelegramConfig, uiBaseURL string) (int, error) {
+func NotifyOnce(ctx context.Context, db *database.DB, sender Sender, cfg config.TelegramConfig, uiBaseURL string, threshold float64) (int, error) {
 	if sender == nil {
 		return 0, fmt.Errorf("notifier: sender must not be nil")
 	}

+	// Authoritative missing set: external releases with no matching local album.
+	// This is what the Web UI dashboard also shows, so the digest stays
+	// consistent with what the operator sees in the UI.
+	missing, err := scanner.ScanAll(ctx, db, threshold)
+	if err != nil {
+		return 0, fmt.Errorf("notifier: scan missing releases: %w", err)
+	}
+	missingByRGID := make(map[string]scanner.MissingRelease, len(missing))
+	for _, m := range missing {
+		missingByRGID[m.RGID] = m
+	}
+
+	// Restrict to releases not yet notified. A release that is genuinely missing
+	// but was already announced is dropped here so it is never re-sent.
 	unnotified, err := database.GetUnnotifiedReleases(db)
 	if err != nil {
 		return 0, fmt.Errorf("notifier: query unnotified releases: %w", err)
 	}

-	missing := make([]scanner.MissingRelease, 0, len(unnotified))
+	toNotify := make([]scanner.MissingRelease, 0, len(unnotified))
 	for _, r := range unnotified {
-		missing = append(missing, scanner.MissingRelease{
-			RGID:        r.RGID,
-			ArtistID:    r.ArtistID,
-			Title:       r.Title,
-			Type:        r.Type,
-			ReleaseDate: r.ReleaseDate,
-		})
+		if m, ok := missingByRGID[r.RGID]; ok {
+			toNotify = append(toNotify, m)
+		}
 	}

 	// 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 {
+	names := make(map[string]string, len(toNotify))
+	for _, m := range toNotify {
+		if _, ok := names[m.ArtistID]; ok {
 			continue
 		}
-		settings, err := database.GetArtistSettings(db, r.ArtistID)
+		settings, err := database.GetArtistSettings(db, m.ArtistID)
 		if err == nil && settings.Name != "" {
-			names[r.ArtistID] = settings.Name
+			names[m.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 {
+	if len(toNotify) == 0 {
 		return 0, nil
 	}
+
+	message := FormatDigest(toNotify, uiBaseURL, names)
 	if err := sender.Send(ctx, message); err != nil {
 		return 0, fmt.Errorf("notifier: send digest: %w", err)
 	}

-	for _, r := range unnotified {
-		if err := database.MarkNotificationSent(db, r.RGID); err != nil {
-			return 0, fmt.Errorf("notifier: mark sent for %s: %w", r.RGID, err)
+	for _, m := range toNotify {
+		if err := database.MarkNotificationSent(db, m.RGID); err != nil {
+			return 0, fmt.Errorf("notifier: mark sent for %s: %w", m.RGID, err)
 		}
 	}
-	return len(unnotified), nil
+	return len(toNotify), nil
 }

 // Schedule produces the next firing time strictly after the given time. It
--- a/internal/notifier/scheduler_test.go
+++ b/internal/notifier/scheduler_test.go
@@ -23,9 +23,9 @@ func (f fixedSchedule) Next(t time.Time) time.Time {

 // collectSender records messages and can be told to fail.
 type collectSender struct {
-	mu      sync.Mutex
+	mu       sync.Mutex
 	messages []string
-	failErr error
+	failErr  error
 }

 func (s *collectSender) Send(ctx context.Context, message string) error {
@@ -79,7 +79,7 @@ func TestNotifyOnce_SendsAndMarksSent(t *testing.T) {

 	sender := &collectSender{}
 	cfg := config.TelegramConfig{Enabled: true}
-	n, err := NotifyOnce(context.Background(), db, sender, cfg, "http://ui:8080")
+	n, err := NotifyOnce(context.Background(), db, sender, cfg, "http://ui:8080", 0.85)
 	if err != nil {
 		t.Fatalf("NotifyOnce: %v", err)
 	}
@@ -100,6 +100,48 @@ func TestNotifyOnce_SendsAndMarksSent(t *testing.T) {
 	}
 }

+func TestNotifyOnce_OwnedReleaseNotNotified(t *testing.T) {
+	// Regression: a cached external release the user already owns locally must
+	// not be reported as "missing". Before the scanner intersection was added,
+	// NotifyOnce treated every unnotified external release as missing and would
+	// spam the operator with releases they already have in Navidrome.
+	db, err := database.New(":memory:")
+	if err != nil {
+		t.Fatalf("New(): %v", err)
+	}
+	defer db.Close()
+
+	seedRelease(t, db, "rgid-owned", "artist-1", false)
+	// The user already has this album locally, with a matching normalized title.
+	if _, err := db.Conn().Exec(
+		"INSERT INTO local_albums (id, artist_id, title) VALUES (?, ?, ?)",
+		"local-1", "artist-1", "Release rgid-owned",
+	); err != nil {
+		t.Fatalf("seed local album: %v", err)
+	}
+
+	sender := &collectSender{}
+	n, err := NotifyOnce(context.Background(), db, sender, config.TelegramConfig{}, "http://ui", 0.85)
+	if err != nil {
+		t.Fatalf("NotifyOnce: %v", err)
+	}
+	if n != 0 {
+		t.Fatalf("expected 0 notified (release already owned), got %d", n)
+	}
+	if sender.count() != 0 {
+		t.Fatalf("expected no digest for an owned release, got %d message(s)", sender.count())
+	}
+	// The owned release is genuinely missing per the scanner, so it must remain
+	// un-marked-sent to avoid corrupting notifications_sent state.
+	sent, err := database.IsNotificationSent(db, "rgid-owned")
+	if err != nil {
+		t.Fatalf("IsNotificationSent: %v", err)
+	}
+	if sent {
+		t.Error("owned release should NOT be marked sent")
+	}
+}
+
 func TestNotifyOnce_EmptyDoesNotSend(t *testing.T) {
 	db, err := database.New(":memory:")
 	if err != nil {
@@ -108,7 +150,7 @@ func TestNotifyOnce_EmptyDoesNotSend(t *testing.T) {
 	defer db.Close()

 	sender := &collectSender{}
-	n, err := NotifyOnce(context.Background(), db, sender, config.TelegramConfig{}, "http://ui")
+	n, err := NotifyOnce(context.Background(), db, sender, config.TelegramConfig{}, "http://ui", 0.85)
 	if err != nil {
 		t.Fatalf("NotifyOnce: %v", err)
 	}
@@ -132,7 +174,7 @@ func TestNotifyOnce_SkipsAlreadySent(t *testing.T) {
 	seedRelease(t, db, "rgid-new", "artist-1", false)

 	sender := &collectSender{}
-	n, err := NotifyOnce(context.Background(), db, sender, config.TelegramConfig{}, "http://ui")
+	n, err := NotifyOnce(context.Background(), db, sender, config.TelegramConfig{}, "http://ui", 0.85)
 	if err != nil {
 		t.Fatalf("NotifyOnce: %v", err)
 	}
@@ -168,7 +210,7 @@ func TestNotifyOnce_SendErrorNotMarked(t *testing.T) {

 	want := errors.New("send boom")
 	sender := &collectSender{failErr: want}
-	_, err = NotifyOnce(context.Background(), db, sender, config.TelegramConfig{}, "http://ui")
+	_, err = NotifyOnce(context.Background(), db, sender, config.TelegramConfig{}, "http://ui", 0.85)
 	if err == nil || !errors.Is(err, want) {
 		t.Fatalf("expected error %v, got %v", want, err)
 	}
@@ -189,7 +231,7 @@ func TestNotifyOnce_NilSender(t *testing.T) {
 	}
 	defer db.Close()

-	if _, err := NotifyOnce(context.Background(), db, nil, config.TelegramConfig{}, "http://ui"); err == nil {
+	if _, err := NotifyOnce(context.Background(), db, nil, config.TelegramConfig{}, "http://ui", 0.85); err == nil {
 		t.Fatal("expected error for nil sender")
 	}
 }