From 854c307834f458cabb8997d141b41372fab1b27c Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 19 Oct 2026 22:44:19 -0400 Subject: [PATCH] feat: notifications say what changed and alert once: a grouped notice names the tracks, grows without re-alerting, and a phone pile lists its titles (M542 #6146-#6151) - An update to a grouped notice keeps its arrival time and email state, so the phone and the email no longer go off again each time it grows. - Notices carry the names of what changed (newest ten), shown as three and "and N more": tracks still missing after the scan's move/replacement fold, duplicate groups new to a sweep, songs merged automatically, and the track and listener a playback error was for. - Duplicates found counts only the groups new to that sweep; playback errors count reports, not the unresolved total. - A scan failure says what stopped and links to Admin > Library. - Android: a pile of more than three alerts lists the newest five titles. Co-Authored-By: Claude Opus 5.5 --- .../notifications/delivery/DeliveryPlan.kt | 39 +++++--- .../delivery/NotificationPoster.kt | 15 +++- .../delivery/DeliveryPlanTest.kt | 10 +++ internal/api/notify_producers.go | 23 +++-- internal/api/notify_producers_test.go | 5 +- internal/api/playback_errors.go | 2 +- internal/db/dbq/duplicates.sql.go | 70 +++++++++++---- internal/db/dbq/notifications.sql.go | 42 +++++---- internal/db/dbq/playback_errors.sql.go | 11 --- internal/db/dbq/tracks.sql.go | 37 ++++++++ internal/db/queries/duplicates.sql | 26 ++++-- internal/db/queries/notifications.sql | 40 ++++----- internal/db/queries/playback_errors.sql | 3 - internal/db/queries/tracks.sql | 11 +++ internal/library/duplicate_resolve.go | 8 +- internal/library/duplicate_sweep.go | 25 +++--- internal/library/notify.go | 37 ++++++++ internal/library/notify_test.go | 88 ++++++++++++++----- internal/library/reconcile.go | 7 +- internal/library/scanner.go | 7 ++ internal/library/scanrun.go | 9 +- internal/notifications/kinds.go | 26 +++--- internal/notifications/notifier.go | 6 +- internal/notifications/notifier_db_test.go | 32 +++++-- internal/notifications/render.go | 67 +++++++++++--- internal/notifications/render_test.go | 30 +++++++ 26 files changed, 486 insertions(+), 190 deletions(-) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/notifications/delivery/DeliveryPlan.kt b/android/app/src/main/java/com/fabledsword/minstrel/notifications/delivery/DeliveryPlan.kt index 95682c9d..60f8c798 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/notifications/delivery/DeliveryPlan.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/notifications/delivery/DeliveryPlan.kt @@ -30,8 +30,9 @@ internal data class CatchUp( * - The mark moves to the newest notice in the page, announced or not, so a * kind with the phone off is never announced later. * - * A coalesced notice (tracks missing, now 5) moves its time forward when its - * count grows, so it is announced again with the new count. + * A coalesced notice (tracks missing, now 5) keeps the time it arrived when + * its count grows (M542), so it is announced once: the inbox shows it grown, + * the phone does not buzz for it again. */ internal fun planCatchUp( page: List, @@ -51,18 +52,32 @@ internal fun planCatchUp( internal sealed interface Announcement { data class Each(val items: List) : Announcement - /** Coming back from a day offline is one line, not forty buzzes. */ - data class Pile(val count: Int, val channel: ShadeChannel) : Announcement + /** + * Coming back from a day offline is one line, not forty buzzes. [lines] + * are the newest notices' titles, so the pile says what it holds (M542). + */ + data class Pile( + val count: Int, + val channel: ShadeChannel, + val lines: List = emptyList(), + ) : Announcement } -internal fun announcementFor(fresh: List): Announcement? = when { - fresh.isEmpty() -> null - fresh.size <= INDIVIDUAL_LIMIT -> Announcement.Each(fresh) +/** How many titles an expanded pile lists, newest first. */ +internal const val PILE_LINES = 5 + +internal fun announcementFor(fresh: List): Announcement? { + if (fresh.isEmpty()) return null + if (fresh.size <= INDIVIDUAL_LIMIT) return Announcement.Each(fresh) // A pile goes where its notices would: library health only when every // one of them is, so a listener's own news is never filed under it. - fresh.all { shadeChannelFor(it.kind) == ShadeChannel.LIBRARY_HEALTH } -> - Announcement.Pile(fresh.size, ShadeChannel.LIBRARY_HEALTH) - else -> Announcement.Pile(fresh.size, ShadeChannel.YOUR_REQUESTS) + val channel = if (fresh.all { shadeChannelFor(it.kind) == ShadeChannel.LIBRARY_HEALTH }) { + ShadeChannel.LIBRARY_HEALTH + } else { + ShadeChannel.YOUR_REQUESTS + } + val lines = fresh.sortedByDescending { it.createdAt }.take(PILE_LINES).map { it.title } + return Announcement.Pile(fresh.size, channel, lines) } /** The phone channel per kind, as the user set it. No settings cached: on, the default for every kind. */ @@ -92,3 +107,7 @@ internal fun deliveryWanted(signedIn: Boolean, enabled: Boolean): Boolean = sign /** One line for a pile. */ internal fun pileText(count: Int): String = "$count new notifications" + +/** Under an expanded pile's titles: how many it holds beyond them, or null. */ +internal fun pileMore(p: Announcement.Pile): String? = + (p.count - p.lines.size).takeIf { it > 0 }?.let { "and $it more" } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/notifications/delivery/NotificationPoster.kt b/android/app/src/main/java/com/fabledsword/minstrel/notifications/delivery/NotificationPoster.kt index 6cee03c7..6f0958a8 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/notifications/delivery/NotificationPoster.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/notifications/delivery/NotificationPoster.kt @@ -1,6 +1,7 @@ package com.fabledsword.minstrel.notifications.delivery import android.annotation.SuppressLint +import android.app.Notification import android.app.NotificationChannel import android.app.NotificationManager import android.app.PendingIntent @@ -50,14 +51,20 @@ class NotificationPoster @Inject constructor( .setContentIntent(openIntent(item.link, item.id.hashCode())) .build() - private fun pile(p: Announcement.Pile) = - NotificationCompat.Builder(context, p.channel.id) + /** Collapsed, the count; expanded, the newest titles and how many more. */ + private fun pile(p: Announcement.Pile): Notification { + val style = NotificationCompat.InboxStyle().setBigContentTitle(pileText(p.count)) + p.lines.forEach { style.addLine(it) } + pileMore(p)?.let { style.setSummaryText(it) } + return NotificationCompat.Builder(context, p.channel.id) .setSmallIcon(R.drawable.ic_notification) - .setContentTitle("Minstrel") - .setContentText(pileText(p.count)) + .setContentTitle(pileText(p.count)) + .setContentText(p.lines.joinToString(" · ")) + .setStyle(style) .setAutoCancel(true) .setContentIntent(openIntent(link = null, requestCode = PILE_ID)) .build() + } /** Each notice gets its own request code, so their links stay apart. */ private fun openIntent(link: String?, requestCode: Int): PendingIntent = diff --git a/android/app/src/test/java/com/fabledsword/minstrel/notifications/delivery/DeliveryPlanTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/notifications/delivery/DeliveryPlanTest.kt index e4e2c3a7..2cb85222 100644 --- a/android/app/src/test/java/com/fabledsword/minstrel/notifications/delivery/DeliveryPlanTest.kt +++ b/android/app/src/test/java/com/fabledsword/minstrel/notifications/delivery/DeliveryPlanTest.kt @@ -99,6 +99,16 @@ class DeliveryPlanTest { assertEquals(5, pile.count) assertEquals(ShadeChannel.YOUR_REQUESTS, pile.channel) assertEquals("5 new notifications", pileText(pile.count)) + assertEquals(listOf("t-n5", "t-n4", "t-n3", "t-n2", "t-n1"), pile.lines) + assertNull(pileMore(pile)) + } + + @Test + fun `a pile names its newest notices and counts the rest`() { + val many = (1..17).map { notice("n$it", it.toLong()) } + val pile = assertIs(announcementFor(many)) + assertEquals(listOf("t-n17", "t-n16", "t-n15", "t-n14", "t-n13"), pile.lines) + assertEquals("and 12 more", pileMore(pile)) } @Test diff --git a/internal/api/notify_producers.go b/internal/api/notify_producers.go index aba5f769..cc2c1d2a 100644 --- a/internal/api/notify_producers.go +++ b/internal/api/notify_producers.go @@ -6,6 +6,7 @@ import ( "github.com/jackc/pgx/v5/pgtype" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/library" "git.fabledsword.com/bvandeusen/minstrel/internal/notifications" "git.fabledsword.com/bvandeusen/minstrel/internal/requests" ) @@ -66,21 +67,19 @@ func (h *handlers) trackLabel(ctx context.Context, trackID pgtype.UUID) string { if err != nil { return "" } - if a, aerr := q.GetArtistByID(ctx, t.ArtistID); aerr == nil && a.Name != "" { - return a.Name + " – " + t.Title + if a, aerr := q.GetArtistByID(ctx, t.ArtistID); aerr == nil { + return library.TrackLabel(a.Name, t.Title) } return t.Title } -// notifyPlaybackErrors tells admins how many playback errors await review. -// The notice coalesces, so while it is unread a new report only updates the -// count. -func (h *handlers) notifyPlaybackErrors(ctx context.Context) { - n, err := dbq.New(h.pool).CountUnresolvedPlaybackErrors(ctx) - if err != nil { - h.logger.Warn("api: playback_error: count unresolved", "err", err) - return +// notifyPlaybackErrors tells admins a track failed to play, naming it and who +// it failed for (M542). The notice coalesces: while it is unread each report +// adds one to its count and its name to the front of the list. +func (h *handlers) notifyPlaybackErrors(ctx context.Context, trackID pgtype.UUID, user dbq.User) { + p := notifications.Payload{Count: 1} + if name := h.trackLabel(ctx, trackID); name != "" { + p.Items = []string{name + " (" + userLabel(user) + ")"} } - h.notifier.NotifyLogged(ctx, notifications.KindPlaybackErrors, notifications.ToAdmins(pgtype.UUID{}), - notifications.Payload{Count: n}.Map()) + h.notifier.NotifyLogged(ctx, notifications.KindPlaybackErrors, notifications.ToAdmins(pgtype.UUID{}), p.Map()) } diff --git a/internal/api/notify_producers_test.go b/internal/api/notify_producers_test.go index 5994444d..96d9ac15 100644 --- a/internal/api/notify_producers_test.go +++ b/internal/api/notify_producers_test.go @@ -211,8 +211,9 @@ func TestNotify_PlaybackErrorsCoalesceIntoOneCount(t *testing.T) { got := inboxOf(t, pool, admin) wantInbox(t, "admin", got, notifications.KindPlaybackErrors) - if got[0].Payload.Count != 2 { - t.Errorf("count = %d, want 2 unresolved", got[0].Payload.Count) + if got[0].Payload.Count != 2 || len(got[0].Payload.Items) != 2 || + !strings.HasSuffix(got[0].Payload.Items[0], "(np-play-alice)") { + t.Errorf("payload = %+v, want two reports, each naming the track and alice", got[0].Payload) } wantInbox(t, "reporter", inboxOf(t, pool, alice)) } diff --git a/internal/api/playback_errors.go b/internal/api/playback_errors.go index ccc358c7..99296522 100644 --- a/internal/api/playback_errors.go +++ b/internal/api/playback_errors.go @@ -130,7 +130,7 @@ func (h *handlers) handleReportPlaybackError(w http.ResponseWriter, r *http.Requ writeErr(w, apierror.Internal(err)) return } - h.notifyPlaybackErrors(r.Context()) + h.notifyPlaybackErrors(r.Context(), trackID, user) writeJSON(w, http.StatusCreated, map[string]string{"id": uuidToString(row.ID)}) } diff --git a/internal/db/dbq/duplicates.sql.go b/internal/db/dbq/duplicates.sql.go index 422b5e58..0ad1dd83 100644 --- a/internal/db/dbq/duplicates.sql.go +++ b/internal/db/dbq/duplicates.sql.go @@ -27,24 +27,6 @@ func (q *Queries) AddDuplicateGroupMember(ctx context.Context, arg AddDuplicateG return err } -const countDuplicateGroupsDetectedSince = `-- name: CountDuplicateGroupsDetectedSince :one -SELECT count(*)::bigint - FROM duplicate_groups - WHERE status = 'pending' - AND detected_at >= $1 -` - -// Pending proposals first made at or after `since`: what a sweep that started -// then found for the first time. A refreshed proposal keeps its detected_at, -// so a sweep that only re-finds known groups counts none (M489: admins are -// told about new duplicates, not reminded of the same ones every sweep). -func (q *Queries) CountDuplicateGroupsDetectedSince(ctx context.Context, since pgtype.Timestamptz) (int64, error) { - row := q.db.QueryRow(ctx, countDuplicateGroupsDetectedSince, since) - var column_1 int64 - err := row.Scan(&column_1) - return column_1, err -} - const countPendingDuplicateGroups = `-- name: CountPendingDuplicateGroups :one SELECT count(*)::bigint FROM duplicate_groups g @@ -311,6 +293,58 @@ func (q *Queries) ListDuplicateCandidates(ctx context.Context, arg ListDuplicate return items, nil } +const listDuplicateGroupsDetectedSince = `-- name: ListDuplicateGroupsDetectedSince :many +SELECT ar.name AS artist_name, t.title, count(*) OVER ()::bigint AS total + FROM duplicate_groups g + JOIN LATERAL (SELECT m.track_id + FROM duplicate_group_members m + WHERE m.group_id = g.id + ORDER BY m.track_id + LIMIT 1) one ON true + JOIN tracks t ON t.id = one.track_id + JOIN artists ar ON ar.id = t.artist_id + WHERE g.status = 'pending' + AND g.detected_at >= $1 + ORDER BY g.detected_at DESC, g.id + LIMIT $2 +` + +type ListDuplicateGroupsDetectedSinceParams struct { + Since pgtype.Timestamptz + N int32 +} + +type ListDuplicateGroupsDetectedSinceRow struct { + ArtistName string + Title string + Total int64 +} + +// Pending proposals first made at or after `since`: what a sweep that started +// then found for the first time, each named by one of its songs, and `total` +// of them however many are listed. A refreshed proposal keeps its detected_at, +// so a sweep that only re-finds known groups lists none (M489: admins are told +// about new duplicates, not reminded of the same ones every sweep). +func (q *Queries) ListDuplicateGroupsDetectedSince(ctx context.Context, arg ListDuplicateGroupsDetectedSinceParams) ([]ListDuplicateGroupsDetectedSinceRow, error) { + rows, err := q.db.Query(ctx, listDuplicateGroupsDetectedSince, arg.Since, arg.N) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListDuplicateGroupsDetectedSinceRow + for rows.Next() { + var i ListDuplicateGroupsDetectedSinceRow + if err := rows.Scan(&i.ArtistName, &i.Title, &i.Total); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listDuplicateGroupsForResolve = `-- name: ListDuplicateGroupsForResolve :many SELECT g.id AS group_id, g.tier, diff --git a/internal/db/dbq/notifications.sql.go b/internal/db/dbq/notifications.sql.go index d5c64d7f..cc113e6f 100644 --- a/internal/db/dbq/notifications.sql.go +++ b/internal/db/dbq/notifications.sql.go @@ -346,8 +346,9 @@ type MarkAllNotificationsReadParams struct { // up_to, when set, limits it to what existed when the user asked: a "mark // all read" queued offline and replayed later must not mark notices that -// arrived in between, which the user never saw. A coalesced row updated -// since then carries a newer created_at, so it stays unread too. +// arrived in between, which the user never saw. A coalesced row that grew +// since then is marked read with it: it arrived before they asked, and its +// growth is a quiet update to a notice they had seen (M542). func (q *Queries) MarkAllNotificationsRead(ctx context.Context, arg MarkAllNotificationsReadParams) (int64, error) { result, err := q.db.Exec(ctx, markAllNotificationsRead, arg.UserID, arg.UpTo) if err != nil { @@ -457,15 +458,15 @@ VALUES ($1, $2, $3, $4, CASE WHEN $5::boolean THEN NULL ELSE now() END) ON CONFLICT (user_id, coalesce_key) WHERE read_at IS NULL AND coalesce_key IS NOT NULL DO UPDATE SET - payload = CASE - WHEN $6::boolean THEN - EXCLUDED.payload || jsonb_build_object('count', - COALESCE((user_notifications.payload->>'count')::bigint, 0) - + COALESCE((EXCLUDED.payload->>'count')::bigint, 0)) - ELSE EXCLUDED.payload - END, - created_at = now(), - emailed_at = EXCLUDED.emailed_at + payload = EXCLUDED.payload || jsonb_build_object( + 'count', COALESCE((user_notifications.payload->>'count')::bigint, 0) + + COALESCE((EXCLUDED.payload->>'count')::bigint, 0), + 'items', (SELECT COALESCE(jsonb_agg(e.item ORDER BY e.n), '[]'::jsonb) + FROM jsonb_array_elements( + COALESCE(EXCLUDED.payload->'items', '[]'::jsonb) + || COALESCE(user_notifications.payload->'items', '[]'::jsonb) + ) WITH ORDINALITY AS e(item, n) + WHERE e.n <= 10)) RETURNING id ` @@ -475,20 +476,18 @@ type UpsertCoalescedNotificationParams struct { Payload []byte CoalesceKey *string EmailWanted bool - SumCount bool } // One unread row per (user, coalesce_key). A new event while that row is -// unread updates it in place and moves it back to the top of the inbox. +// unread updates it in place: its `count` adds to the row's, and its `items` +// (the names of what changed) go in front of the row's, keeping the newest +// ten. Every coalesced kind's event is "N more happened", never a restated +// total, so the row always says what is new since it was last read. // -// sum_count: the payload's `count` adds to the unread row's count rather than -// replacing it. For kinds whose event is "N more happened" (tracks marked -// missing). Kinds whose event states the whole current total (pending -// duplicate groups) pass false and the payload simply replaces. -// -// emailed_at is cleared so the newer state goes out in the next batch; an -// emailed-but-unread row that keeps growing is news the user hasn't seen. -// Unless email_wanted is false, as in InsertNotification. +// created_at and emailed_at are left alone (M542): an update changes what the +// notice says, not when it arrived. Moving created_at made the phone announce +// it again and clearing emailed_at sent it again, so a notice the hourly +// duplicate pass kept growing alerted every hour. func (q *Queries) UpsertCoalescedNotification(ctx context.Context, arg UpsertCoalescedNotificationParams) (pgtype.UUID, error) { row := q.db.QueryRow(ctx, upsertCoalescedNotification, arg.UserID, @@ -496,7 +495,6 @@ func (q *Queries) UpsertCoalescedNotification(ctx context.Context, arg UpsertCoa arg.Payload, arg.CoalesceKey, arg.EmailWanted, - arg.SumCount, ) var id pgtype.UUID err := row.Scan(&id) diff --git a/internal/db/dbq/playback_errors.sql.go b/internal/db/dbq/playback_errors.sql.go index 53988c4e..607441b9 100644 --- a/internal/db/dbq/playback_errors.sql.go +++ b/internal/db/dbq/playback_errors.sql.go @@ -11,17 +11,6 @@ import ( "github.com/jackc/pgx/v5/pgtype" ) -const countUnresolvedPlaybackErrors = `-- name: CountUnresolvedPlaybackErrors :one -SELECT count(*)::bigint FROM playback_errors WHERE resolved_at IS NULL -` - -func (q *Queries) CountUnresolvedPlaybackErrors(ctx context.Context) (int64, error) { - row := q.db.QueryRow(ctx, countUnresolvedPlaybackErrors) - var column_1 int64 - err := row.Scan(&column_1) - return column_1, err -} - const insertPlaybackError = `-- name: InsertPlaybackError :one INSERT INTO playback_errors (track_id, user_id, client_id, kind, detail) VALUES ($1, $2, $3, $4, $5) diff --git a/internal/db/dbq/tracks.sql.go b/internal/db/dbq/tracks.sql.go index 1280fbda..2feaeb21 100644 --- a/internal/db/dbq/tracks.sql.go +++ b/internal/db/dbq/tracks.sql.go @@ -646,6 +646,43 @@ func (q *Queries) ListRandomTracksForUser(ctx context.Context, arg ListRandomTra return items, nil } +const listStillMissingTrackLabels = `-- name: ListStillMissingTrackLabels :many +SELECT ar.name AS artist_name, t.title + FROM tracks t + JOIN artists ar ON ar.id = t.artist_id + WHERE t.id = ANY($1::uuid[]) + AND t.missing_since IS NOT NULL + ORDER BY ar.name, t.title +` + +type ListStillMissingTrackLabelsRow struct { + ArtistName string + Title string +} + +// Which of the tracks a scan marked missing still are once it has adopted +// moves and folded replacements (M542): the ones worth telling an admin +// about, by name. A quality upgrade folds into its replacement and drops out. +func (q *Queries) ListStillMissingTrackLabels(ctx context.Context, ids []pgtype.UUID) ([]ListStillMissingTrackLabelsRow, error) { + rows, err := q.db.Query(ctx, listStillMissingTrackLabels, ids) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListStillMissingTrackLabelsRow + for rows.Next() { + var i ListStillMissingTrackLabelsRow + if err := rows.Scan(&i.ArtistName, &i.Title); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listSuspectSourceTracks = `-- name: ListSuspectSourceTracks :many SELECT t.id, t.title, diff --git a/internal/db/queries/duplicates.sql b/internal/db/queries/duplicates.sql index d003bd71..71902168 100644 --- a/internal/db/queries/duplicates.sql +++ b/internal/db/queries/duplicates.sql @@ -184,15 +184,25 @@ UPDATE duplicate_groups SET status = 'dismissed', resolved_at = now() WHERE id = sqlc.arg(id) AND status = 'pending'; --- name: CountDuplicateGroupsDetectedSince :one +-- name: ListDuplicateGroupsDetectedSince :many -- Pending proposals first made at or after `since`: what a sweep that started --- then found for the first time. A refreshed proposal keeps its detected_at, --- so a sweep that only re-finds known groups counts none (M489: admins are --- told about new duplicates, not reminded of the same ones every sweep). -SELECT count(*)::bigint - FROM duplicate_groups - WHERE status = 'pending' - AND detected_at >= sqlc.arg(since); +-- then found for the first time, each named by one of its songs, and `total` +-- of them however many are listed. A refreshed proposal keeps its detected_at, +-- so a sweep that only re-finds known groups lists none (M489: admins are told +-- about new duplicates, not reminded of the same ones every sweep). +SELECT ar.name AS artist_name, t.title, count(*) OVER ()::bigint AS total + FROM duplicate_groups g + JOIN LATERAL (SELECT m.track_id + FROM duplicate_group_members m + WHERE m.group_id = g.id + ORDER BY m.track_id + LIMIT 1) one ON true + JOIN tracks t ON t.id = one.track_id + JOIN artists ar ON ar.id = t.artist_id + WHERE g.status = 'pending' + AND g.detected_at >= sqlc.arg(since) + ORDER BY g.detected_at DESC, g.id + LIMIT sqlc.arg(n); -- name: ListDuplicateGroupsForResolve :many -- Every pending group's present copies, for the resolver (M498 #5435): what it diff --git a/internal/db/queries/notifications.sql b/internal/db/queries/notifications.sql index fa82ca38..26f84b8a 100644 --- a/internal/db/queries/notifications.sql +++ b/internal/db/queries/notifications.sql @@ -13,30 +13,29 @@ RETURNING id; -- name: UpsertCoalescedNotification :one -- One unread row per (user, coalesce_key). A new event while that row is --- unread updates it in place and moves it back to the top of the inbox. +-- unread updates it in place: its `count` adds to the row's, and its `items` +-- (the names of what changed) go in front of the row's, keeping the newest +-- ten. Every coalesced kind's event is "N more happened", never a restated +-- total, so the row always says what is new since it was last read. -- --- sum_count: the payload's `count` adds to the unread row's count rather than --- replacing it. For kinds whose event is "N more happened" (tracks marked --- missing). Kinds whose event states the whole current total (pending --- duplicate groups) pass false and the payload simply replaces. --- --- emailed_at is cleared so the newer state goes out in the next batch; an --- emailed-but-unread row that keeps growing is news the user hasn't seen. --- Unless email_wanted is false, as in InsertNotification. +-- created_at and emailed_at are left alone (M542): an update changes what the +-- notice says, not when it arrived. Moving created_at made the phone announce +-- it again and clearing emailed_at sent it again, so a notice the hourly +-- duplicate pass kept growing alerted every hour. INSERT INTO user_notifications (user_id, kind, payload, coalesce_key, emailed_at) VALUES (sqlc.arg(user_id), sqlc.arg(kind), sqlc.arg(payload), sqlc.arg(coalesce_key), CASE WHEN sqlc.arg(email_wanted)::boolean THEN NULL ELSE now() END) ON CONFLICT (user_id, coalesce_key) WHERE read_at IS NULL AND coalesce_key IS NOT NULL DO UPDATE SET - payload = CASE - WHEN sqlc.arg(sum_count)::boolean THEN - EXCLUDED.payload || jsonb_build_object('count', - COALESCE((user_notifications.payload->>'count')::bigint, 0) - + COALESCE((EXCLUDED.payload->>'count')::bigint, 0)) - ELSE EXCLUDED.payload - END, - created_at = now(), - emailed_at = EXCLUDED.emailed_at + payload = EXCLUDED.payload || jsonb_build_object( + 'count', COALESCE((user_notifications.payload->>'count')::bigint, 0) + + COALESCE((EXCLUDED.payload->>'count')::bigint, 0), + 'items', (SELECT COALESCE(jsonb_agg(e.item ORDER BY e.n), '[]'::jsonb) + FROM jsonb_array_elements( + COALESCE(EXCLUDED.payload->'items', '[]'::jsonb) + || COALESCE(user_notifications.payload->'items', '[]'::jsonb) + ) WITH ORDINALITY AS e(item, n) + WHERE e.n <= 10)) RETURNING id; -- name: ListNotifications :many @@ -65,8 +64,9 @@ UPDATE user_notifications -- name: MarkAllNotificationsRead :execrows -- up_to, when set, limits it to what existed when the user asked: a "mark -- all read" queued offline and replayed later must not mark notices that --- arrived in between, which the user never saw. A coalesced row updated --- since then carries a newer created_at, so it stays unread too. +-- arrived in between, which the user never saw. A coalesced row that grew +-- since then is marked read with it: it arrived before they asked, and its +-- growth is a quiet update to a notice they had seen (M542). UPDATE user_notifications SET read_at = now() WHERE user_id = sqlc.arg(user_id) diff --git a/internal/db/queries/playback_errors.sql b/internal/db/queries/playback_errors.sql index 823a76ee..c89da41f 100644 --- a/internal/db/queries/playback_errors.sql +++ b/internal/db/queries/playback_errors.sql @@ -61,6 +61,3 @@ UPDATE playback_errors resolved_by = $2, resolution = $3 WHERE track_id = $1 AND resolved_at IS NULL; - --- name: CountUnresolvedPlaybackErrors :one -SELECT count(*)::bigint FROM playback_errors WHERE resolved_at IS NULL; diff --git a/internal/db/queries/tracks.sql b/internal/db/queries/tracks.sql index 275125ca..602764df 100644 --- a/internal/db/queries/tracks.sql +++ b/internal/db/queries/tracks.sql @@ -335,3 +335,14 @@ UPDATE tracks SET source_verdict = sqlc.arg(verdict)::text, source_verdict_at = now() WHERE id = sqlc.arg(id) AND source_verdict IS NOT NULL; + +-- name: ListStillMissingTrackLabels :many +-- Which of the tracks a scan marked missing still are once it has adopted +-- moves and folded replacements (M542): the ones worth telling an admin +-- about, by name. A quality upgrade folds into its replacement and drops out. +SELECT ar.name AS artist_name, t.title + FROM tracks t + JOIN artists ar ON ar.id = t.artist_id + WHERE t.id = ANY(sqlc.arg(ids)::uuid[]) + AND t.missing_since IS NOT NULL + ORDER BY ar.name, t.title; diff --git a/internal/library/duplicate_resolve.go b/internal/library/duplicate_resolve.go index 276de865..acb4e0d4 100644 --- a/internal/library/duplicate_resolve.go +++ b/internal/library/duplicate_resolve.go @@ -15,7 +15,6 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/notifications" syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync" - "git.fabledsword.com/bvandeusen/minstrel/internal/words" ) // Duplicate resolver (M498 #5435, #5437; M502 #5517). @@ -149,6 +148,7 @@ func ResolveDuplicates( // Merges before links, so a rip merged away is not linked first. merged := map[pgtype.UUID]bool{} + var mergedNames []string for _, g := range groups { if res.Merged >= resolveMergeCap { break @@ -167,6 +167,9 @@ func ResolveDuplicates( } merged[g.id] = true res.Merged++ + if len(mergedNames) < notifyItems { + mergedNames = append(mergedNames, TrackLabel(g.members[0].ArtistName, g.members[0].Title)) + } } linked, err := linkCrossRelease(ctx, pool, logger, groups, merged) @@ -177,8 +180,7 @@ func ResolveDuplicates( if res.Merged > 0 { notifyAdmins(ctx, notifications.KindDuplicatesResolved, notifications.Payload{ - Count: int64(res.Merged), - Detail: "Merged " + words.Count(res.Merged, "duplicate copy", "duplicate copies") + ".", + Count: int64(res.Merged), Items: mergedNames, }) } return res, nil diff --git a/internal/library/duplicate_sweep.go b/internal/library/duplicate_sweep.go index 46e6cba7..42e500e5 100644 --- a/internal/library/duplicate_sweep.go +++ b/internal/library/duplicate_sweep.go @@ -112,25 +112,26 @@ func runDuplicateSweep( return res, runErr } -// notifyNewDuplicates tells admins when a sweep proposed a group it had not -// proposed before (M489), counting every proposal awaiting review. A sweep -// that only re-finds known groups says nothing, so reading the notice once -// is enough until something new turns up. +// notifyNewDuplicates tells admins when a sweep proposed groups it had not +// proposed before (M489), counting and naming those (M542). A sweep that only +// re-finds known groups says nothing, so reading the notice once is enough +// until something new turns up. func notifyNewDuplicates(ctx context.Context, q *dbq.Queries, sweepStarted pgtype.Timestamptz, logger *slog.Logger) { - fresh, err := q.CountDuplicateGroupsDetectedSince(ctx, sweepStarted) + fresh, err := q.ListDuplicateGroupsDetectedSince(ctx, dbq.ListDuplicateGroupsDetectedSinceParams{ + Since: sweepStarted, N: notifyItems, + }) if err != nil { - logger.Warn("duplicate sweep: counting new proposals failed", "err", err) + logger.Warn("duplicate sweep: listing new proposals failed", "err", err) return } - if fresh == 0 { + if len(fresh) == 0 { return } - pending, err := q.CountPendingDuplicateGroups(ctx) - if err != nil { - logger.Warn("duplicate sweep: counting pending proposals failed", "err", err) - return + items := make([]string, 0, len(fresh)) + for _, g := range fresh { + items = append(items, TrackLabel(g.ArtistName, g.Title)) } - notifyAdmins(ctx, notifications.KindDuplicatesFound, notifications.Payload{Count: pending}) + notifyAdmins(ctx, notifications.KindDuplicatesFound, notifications.Payload{Count: fresh[0].Total, Items: items}) } func sweepDuplicates( diff --git a/internal/library/notify.go b/internal/library/notify.go index 01b4f361..b66c3afe 100644 --- a/internal/library/notify.go +++ b/internal/library/notify.go @@ -6,6 +6,7 @@ import ( "github.com/jackc/pgx/v5/pgtype" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/notifications" ) @@ -34,6 +35,42 @@ func notifyAdmins(ctx context.Context, kind notifications.Kind, p notifications. n.NotifyLogged(ctx, kind, notifications.ToAdmins(pgtype.UUID{}), p.Map()) } +// notifyItems is how many names a notice carries; the notifications package +// keeps the newest ten across a coalesced burst. +const notifyItems = 10 + +// TrackLabel names a track as a notice does: "Artist – Title", or the title +// alone when the artist is unknown. +func TrackLabel(artist, title string) string { + if artist == "" { + return title + } + return artist + " – " + title +} + +// notifyStillMissing tells admins which of the tracks a scan marked missing +// are still gone after it adopted moves and folded replacements (M542). A +// marked track the scan healed is not news: reporting it is what made a +// quality upgrade read as lost music. +func (s *Scanner) notifyStillMissing(ctx context.Context, marked []pgtype.UUID) { + if len(marked) == 0 || ctx.Err() != nil { + return + } + rows, err := dbq.New(s.pool).ListStillMissingTrackLabels(ctx, marked) + if err != nil { + s.logger.Warn("library scan: listing missing tracks for the notice failed", "err", err) + return + } + if len(rows) == 0 { + return + } + items := make([]string, 0, min(len(rows), notifyItems)) + for _, r := range rows[:min(len(rows), notifyItems)] { + items = append(items, TrackLabel(r.ArtistName, r.Title)) + } + notifyAdmins(ctx, notifications.KindTracksMissing, notifications.Payload{Count: int64(len(rows)), Items: items}) +} + // notifyScanFinished tells admins a scan run ended in error. A scan cut short // by shutdown is not a failure anyone needs telling about. func notifyScanFinished(ctx context.Context, errMsg string) { diff --git a/internal/library/notify_test.go b/internal/library/notify_test.go index 0ca635e2..e3c51163 100644 --- a/internal/library/notify_test.go +++ b/internal/library/notify_test.go @@ -10,6 +10,7 @@ import ( "path/filepath" "testing" + "github.com/jackc/pgx/v5/pgtype" "github.com/jackc/pgx/v5/pgxpool" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" @@ -36,6 +37,7 @@ type notice struct { kind string count int64 body string + items []string } func unreadNotices(t *testing.T, pool *pgxpool.Pool, user dbq.User) []notice { @@ -55,7 +57,7 @@ func unreadNotices(t *testing.T, pool *pgxpool.Pool, user dbq.User) []notice { if err := json.Unmarshal(r.Payload, &p); err != nil { t.Fatalf("payload: %v", err) } - out = append(out, notice{kind: r.Kind, count: p.Count, body: p.Detail}) + out = append(out, notice{kind: r.Kind, count: p.Count, body: p.Detail, items: p.Items}) } return out } @@ -86,31 +88,70 @@ func TestNotifyScanFinished_FailuresCoalesceAndShutdownIsSilent(t *testing.T) { } } -func TestReconcileMissing_NotifiesAdminsWithARunningCount(t *testing.T) { +// M542: reconcile only records what it marked. The scan tells admins after +// it has adopted moves and folded replacements, so a file it marked and then +// healed is never reported as lost. +func TestReconcileMissing_RecordsTheMarksWithoutNotifying(t *testing.T) { pool := newPool(t) admin := notifyingAdmin(t, pool) s := testScanner(t, populatedRoot(t)) - // Two scans, each losing 2 of 10 tracks (under the mark cap). - for pass := 0; pass < 2; pass++ { - rows := make([]dbq.ListTrackPathsForReconcileRow, 0, 10) - seen := map[string]struct{}{} - for i := 0; i < 10; i++ { - p := fmt.Sprintf("/music/pass-%d-%02d.mp3", pass, i) - rows = append(rows, row(byte(pass*10+i), p, false)) - if i >= 2 { - seen[p] = struct{}{} - } - } - var stats Stats - if err := s.reconcileMissing(context.Background(), &fakeReconciler{rows: rows}, seen, &stats); err != nil { - t.Fatalf("reconcile pass %d: %v", pass, err) + rows := make([]dbq.ListTrackPathsForReconcileRow, 0, 10) + seen := map[string]struct{}{} + for i := 0; i < 10; i++ { + p := fmt.Sprintf("/music/%02d.mp3", i) + rows = append(rows, row(byte(i), p, false)) + if i >= 2 { + seen[p] = struct{}{} } } + var stats Stats + if err := s.reconcileMissing(context.Background(), &fakeReconciler{rows: rows}, seen, &stats); err != nil { + t.Fatalf("reconcile: %v", err) + } + if len(stats.marked) != 2 { + t.Errorf("marked %d tracks; want the 2 the walk did not see", len(stats.marked)) + } + if got := unreadNotices(t, pool, admin); len(got) != 0 { + t.Fatalf("reconcile notified before the scan could heal anything: %+v", got) + } +} +func TestNotifyStillMissing_NamesOnlyTheTracksStillGone(t *testing.T) { + pool := newPool(t) + admin := notifyingAdmin(t, pool) + ctx := context.Background() + q := dbq.New(pool) + dir := t.TempDir() + first, album, artist := seedTrack(t, pool, filepath.Join(dir, "a.mp3")) + second, err := q.UpsertTrack(ctx, dbq.UpsertTrackParams{ + Title: "Another Track", AlbumID: album.ID, ArtistID: artist.ID, + DurationMs: 1000, FilePath: filepath.Join(dir, "b.mp3"), FileSize: 100, FileFormat: "mp3", + }) + if err != nil { + t.Fatal(err) + } + // The scan marked both; the second came back (a move it adopted). + marked := []pgtype.UUID{first.ID, second.ID} + if _, err := q.MarkTracksMissing(ctx, []pgtype.UUID{first.ID}); err != nil { + t.Fatal(err) + } + + s := New(pool, slog.New(slog.NewTextHandler(io.Discard, nil)), nil, nil) + s.notifyStillMissing(ctx, marked) got := unreadNotices(t, pool, admin) - if len(got) != 1 || got[0].kind != string(notifications.KindTracksMissing) || got[0].count != 4 { - t.Fatalf("notices = %+v, want one tracks_missing counting 4", got) + if len(got) != 1 || got[0].kind != string(notifications.KindTracksMissing) || got[0].count != 1 || + len(got[0].items) != 1 || got[0].items[0] != "Delete Test Artist – Delete Test Track" { + t.Fatalf("notices = %+v, want one tracks_missing naming the one track still gone", got) + } + + // Nothing still gone: nothing to say. + if _, err := q.MarkAllNotificationsRead(ctx, dbq.MarkAllNotificationsReadParams{UserID: admin.ID}); err != nil { + t.Fatal(err) + } + s.notifyStillMissing(ctx, []pgtype.UUID{second.ID}) + if got := unreadNotices(t, pool, admin); len(got) != 0 { + t.Fatalf("a healed track was reported: %+v", got) } } @@ -159,8 +200,9 @@ func TestDuplicateSweep_NotifiesOnlyWhenSomethingNewIsProposed(t *testing.T) { pair("first", 1, 100) sweep() got := unreadNotices(t, pool, admin) - if len(got) != 1 || got[0].kind != string(notifications.KindDuplicatesFound) || got[0].count != 1 { - t.Fatalf("after the first sweep notices = %+v, want one duplicates_found counting 1", got) + if len(got) != 1 || got[0].kind != string(notifications.KindDuplicatesFound) || got[0].count != 1 || + len(got[0].items) != 1 || got[0].items[0] != "Delete Test Artist – first" { + t.Fatalf("after the first sweep notices = %+v, want one duplicates_found naming the first pair", got) } // Read, then swept again with nothing new: no reminder. @@ -170,11 +212,11 @@ func TestDuplicateSweep_NotifiesOnlyWhenSomethingNewIsProposed(t *testing.T) { t.Fatalf("a sweep that found nothing new notified: %+v", got) } - // A new pair is news, and the count is everything awaiting review. + // A new pair is news, and the notice is about it alone: the first was read. pair("second", 2, 200) sweep() got = unreadNotices(t, pool, admin) - if len(got) != 1 || got[0].count != 2 { - t.Fatalf("after a new pair notices = %+v, want one counting 2", got) + if len(got) != 1 || got[0].count != 1 || len(got[0].items) != 1 || got[0].items[0] != "Delete Test Artist – second" { + t.Fatalf("after a new pair notices = %+v, want one naming only the new pair", got) } } diff --git a/internal/library/reconcile.go b/internal/library/reconcile.go index b384e6e1..6cc273ef 100644 --- a/internal/library/reconcile.go +++ b/internal/library/reconcile.go @@ -9,7 +9,6 @@ import ( "github.com/jackc/pgx/v5/pgtype" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" - "git.fabledsword.com/bvandeusen/minstrel/internal/notifications" syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync" ) @@ -129,9 +128,9 @@ func (s *Scanner) reconcileMissing( // this line. s.logger.Warn("library scan: tracks marked missing (files not found)", "count", n, "library_total", len(rows)) - if n > 0 { - notifyAdmins(ctx, notifications.KindTracksMissing, notifications.Payload{Count: n}) - } + // Admins are told after the scan has adopted moves and folded + // replacements, about the ones still gone (notifyStillMissing). + stats.marked = toMark return nil } diff --git a/internal/library/scanner.go b/internal/library/scanner.go index 1d0a4c27..7b1fd91a 100644 --- a/internal/library/scanner.go +++ b/internal/library/scanner.go @@ -83,6 +83,10 @@ type Stats struct { // Only a full Scan sets these — see reconcileMissing. Missing int `json:"missing"` Restored int `json:"restored"` + // marked are the tracks this scan's reconcile marked missing, so the scan + // can tell admins which of them are still gone once it has adopted moves + // and folded replacements (M542). + marked []pgtype.UUID } type Scanner struct { @@ -198,6 +202,9 @@ func (s *Scanner) Scan(ctx context.Context, progressCb func(Stats)) (Stats, erro if _, err := PairMissingTracks(ctx, s.pool, s.logger); err != nil && ctx.Err() == nil { s.logger.Warn("library scan: missing-pair pass failed", "err", err) } + // Told only now, so a file that moved or was upgraded, which this scan + // marked and then healed, is not reported as lost. + s.notifyStillMissing(ctx, stats.marked) return stats, nil } diff --git a/internal/library/scanrun.go b/internal/library/scanrun.go index f25081f8..c56c5f9c 100644 --- a/internal/library/scanrun.go +++ b/internal/library/scanrun.go @@ -31,6 +31,13 @@ type LibraryStageTallies struct { Restored int `json:"restored"` } +func (st Stats) tallies() LibraryStageTallies { + return LibraryStageTallies{ + Scanned: st.Scanned, Added: st.Added, Updated: st.Updated, Skipped: st.Skipped, + Errored: st.Errored, Missing: st.Missing, Restored: st.Restored, + } +} + // MBIDBackfillStageTallies wires BackfillMBIDsResult into the scan_runs jsonb column. type MBIDBackfillStageTallies struct { Processed int `json:"processed"` @@ -105,7 +112,7 @@ func RunScan( var lastStats Stats libPub := newStagePublisher(ctx, 500, time.Second, func() []byte { - blob, _ := json.Marshal(LibraryStageTallies(lastStats)) + blob, _ := json.Marshal(lastStats.tallies()) return blob }, func(blob []byte) error { diff --git a/internal/notifications/kinds.go b/internal/notifications/kinds.go index adcbf095..b7cfe1b1 100644 --- a/internal/notifications/kinds.go +++ b/internal/notifications/kinds.go @@ -69,12 +69,9 @@ func (c Channels) Effective() Channels { type spec struct { audience Audience // coalesce: while one of these is unread, a new event updates it in place - // rather than adding a row. For the burst-prone admin kinds. + // rather than adding a row. For the burst-prone admin kinds. Each event + // is "N more", so the update adds its count and names to the row's. coalesce bool - // sumCount: a coalesced update adds the payload's `count` to the unread - // row's, because each event is "N more". Without it the newer payload - // replaces the older, because each event states the whole current total. - sumCount bool group EmailGroup defaults Channels } @@ -88,16 +85,15 @@ var ( ) var specs = map[Kind]spec{ - KindRequestApproved: {audience: AudienceRequester, group: EmailBatch, defaults: allOn}, - KindRequestRejected: {audience: AudienceRequester, group: EmailBatch, defaults: allOn}, - KindRequestCompleted: {audience: AudienceRequester, group: EmailSummary, defaults: allOn}, - KindRequestPending: {audience: AudienceAdmin, group: EmailBatch, defaults: allOn}, - KindQuarantineFlagged: {audience: AudienceAdmin, group: EmailBatch, defaults: allOn}, - KindScanFailed: {audience: AudienceAdmin, coalesce: true, sumCount: true, group: EmailBatch, defaults: healthAlert}, - KindTracksMissing: {audience: AudienceAdmin, coalesce: true, sumCount: true, group: EmailBatch, defaults: healthAlert}, - KindDuplicatesFound: {audience: AudienceAdmin, coalesce: true, group: EmailBatch, defaults: healthAlert}, - // Each pass reports what it did itself, so the counts add up. - KindDuplicatesResolved: {audience: AudienceAdmin, coalesce: true, sumCount: true, group: EmailBatch, defaults: inboxOnly}, + KindRequestApproved: {audience: AudienceRequester, group: EmailBatch, defaults: allOn}, + KindRequestRejected: {audience: AudienceRequester, group: EmailBatch, defaults: allOn}, + KindRequestCompleted: {audience: AudienceRequester, group: EmailSummary, defaults: allOn}, + KindRequestPending: {audience: AudienceAdmin, group: EmailBatch, defaults: allOn}, + KindQuarantineFlagged: {audience: AudienceAdmin, group: EmailBatch, defaults: allOn}, + KindScanFailed: {audience: AudienceAdmin, coalesce: true, group: EmailBatch, defaults: healthAlert}, + KindTracksMissing: {audience: AudienceAdmin, coalesce: true, group: EmailBatch, defaults: healthAlert}, + KindDuplicatesFound: {audience: AudienceAdmin, coalesce: true, group: EmailBatch, defaults: healthAlert}, + KindDuplicatesResolved: {audience: AudienceAdmin, coalesce: true, group: EmailBatch, defaults: inboxOnly}, KindPlaybackErrors: {audience: AudienceAdmin, coalesce: true, group: EmailBatch, defaults: healthAlert}, } diff --git a/internal/notifications/notifier.go b/internal/notifications/notifier.go index 80e14eb4..faad7c3d 100644 --- a/internal/notifications/notifier.go +++ b/internal/notifications/notifier.go @@ -57,8 +57,9 @@ func New(db dbq.DBTX, bus *eventbus.Bus, logger *slog.Logger) *Notifier { // then nudges their open clients. // // For a coalescing kind, an unread row of that kind is updated in place -// rather than a new one added. payload["count"] then adds to the unread -// row's count for kinds whose events each mean "N more". +// rather than a new one added: payload["count"] adds to the row's count and +// payload["items"] joins its names. The row keeps the time it arrived, so an +// update is not announced again. // // The error is for logging. A notification must never fail the action that // caused it, so producers log and carry on; NotifyLogged does exactly that. @@ -183,7 +184,6 @@ func write(ctx context.Context, q *dbq.Queries, kind Kind, userID pgtype.UUID, b Payload: body, CoalesceKey: &key, EmailWanted: emailWanted, - SumCount: specs[kind].sumCount, }) return err } diff --git a/internal/notifications/notifier_db_test.go b/internal/notifications/notifier_db_test.go index a551e135..c8a782c6 100644 --- a/internal/notifications/notifier_db_test.go +++ b/internal/notifications/notifier_db_test.go @@ -146,19 +146,41 @@ func TestNotify_CoalescedCountAddsUpWhileUnread_AndStartsAfreshOnceRead(t *testi require.True(t, rows[1].ReadAt.Valid) } -func TestNotify_CoalescedTotalReplacesWhenEventsStateTheWhole(t *testing.T) { +// M542: an update says more, newest names first, without arriving again. A +// moved created_at was announced again on the phone, and a cleared emailed_at +// was emailed again. +func TestNotify_ACoalescedUpdateMergesNamesAndKeepsItsArrival(t *testing.T) { pool := testPool(t) q := dbq.New(pool) ctx := context.Background() admin := mkUser(t, q, "admin", true) n := notifications.New(pool, nil, nil) + to := notifications.ToUser(admin) + notify := func(items ...string) { + t.Helper() + require.NoError(t, n.Notify(ctx, notifications.KindDuplicatesFound, to, + notifications.Payload{Count: int64(len(items)), Items: items}.Map())) + } - // duplicates_found states the pending total each time; it must not sum. - require.NoError(t, n.Notify(ctx, notifications.KindDuplicatesFound, notifications.ToUser(admin), map[string]any{"count": 5})) - require.NoError(t, n.Notify(ctx, notifications.KindDuplicatesFound, notifications.ToUser(admin), map[string]any{"count": 3})) + notify("A", "B") + first := rowsFor(t, q, admin) + require.Len(t, first, 1) + _, err := pool.Exec(ctx, `UPDATE user_notifications SET emailed_at = now() WHERE id = $1`, first[0].ID) + require.NoError(t, err) + + notify("C", "D", "E", "F", "G", "H", "I", "J", "K") rows := rowsFor(t, q, admin) require.Len(t, rows, 1) - require.Equal(t, int64(3), count(t, rows[0].Payload)) + var p notifications.Payload + require.NoError(t, json.Unmarshal(rows[0].Payload, &p)) + require.Equal(t, int64(11), p.Count) + require.Equal(t, []string{"C", "D", "E", "F", "G", "H", "I", "J", "K", "A"}, p.Items, + "the newest names first, ten kept") + require.True(t, rows[0].CreatedAt.Time.Equal(first[0].CreatedAt.Time), "an update is not a new arrival") + + var emailed pgtype.Timestamptz + require.NoError(t, pool.QueryRow(ctx, `SELECT emailed_at FROM user_notifications WHERE id = $1`, rows[0].ID).Scan(&emailed)) + require.True(t, emailed.Valid, "an emailed notice is not emailed again because it grew") } func TestNotify_RequestKindsNeverCoalesce(t *testing.T) { diff --git a/internal/notifications/render.go b/internal/notifications/render.go index 993bfea7..96aeac20 100644 --- a/internal/notifications/render.go +++ b/internal/notifications/render.go @@ -3,6 +3,7 @@ package notifications import ( "encoding/json" "fmt" + "strings" "git.fabledsword.com/bvandeusen/minstrel/internal/words" ) @@ -37,10 +38,15 @@ type Payload struct { // Actor is the other person involved: who asked, who flagged. Actor string `json:"actor,omitempty"` Reason string `json:"reason,omitempty"` - // Count is the coalesced kinds' running total. + // Count is the coalesced kinds' running total since the notice was last + // read. Count int64 `json:"count,omitempty"` - // Detail is free text: scan_failed's error message, and what the duplicate - // resolver merged for duplicates_resolved. + // Items name what Count counts, one each, newest first: "Weezer – Buddy + // Holly". A coalesced update puts its own in front and keeps ten (M542), + // so the body can say which, not only how many. + Items []string `json:"items,omitempty"` + // Detail is free text: scan_failed's error message. Rows from before + // Items also carry duplicates_resolved's summary here. Detail string `json:"detail,omitempty"` } @@ -101,37 +107,52 @@ func Render(kind Kind, payload []byte) Rendered { if p.Count > 1 { title = fmt.Sprintf("Library scan failed %d times", p.Count) } - body := p.Detail - if body == "" { - body = "The last scan stopped with an error." + body := "The last scan stopped with an error." + if p.Detail != "" { + body = "The last scan stopped: " + p.Detail } - return Rendered{Title: title, Body: body, Link: "/admin"} + return Rendered{Title: title, Body: body, Link: "/admin/library"} case KindTracksMissing: + body := "Their files are no longer where the library expects them." + if names := nameList(p.Items, p.Count); names != "" { + body = "No longer on disk: " + names + "." + } return Rendered{ Title: plural(p.Count, "track went missing", "tracks went missing"), - Body: "Their files are no longer where the library expects them.", + Body: body, Link: "/collection/missing", } case KindDuplicatesFound: + body := "The duplicate sweep found tracks holding the same recording." + if names := nameList(p.Items, p.Count); names != "" { + body = "Held more than once: " + names + "." + } return Rendered{ - Title: plural(p.Count, "duplicate group to review", "duplicate groups to review"), - Body: "The duplicate sweep found tracks holding the same recording.", + Title: plural(p.Count, "new duplicate to review", "new duplicates to review"), + Body: body, Link: "/collection/duplicates", } case KindDuplicatesResolved: body := "Extra copies of a song were merged into the one the album keeps." - if p.Detail != "" { + switch names := nameList(p.Items, p.Count); { + case names != "": + body = "Extra copies merged into the one the album keeps: " + names + "." + case p.Detail != "": body = p.Detail } return Rendered{ - Title: plural(p.Count, "duplicate resolved automatically", "duplicates resolved automatically"), + Title: plural(p.Count, "duplicate merged automatically", "duplicates merged automatically"), Body: body, Link: "/collection/duplicates?view=resolved", } case KindPlaybackErrors: + body := "A track failed to play for someone." + if names := nameList(p.Items, p.Count); names != "" { + body = "Failed to play: " + names + "." + } return Rendered{ Title: plural(p.Count, "playback error reported", "playback errors reported"), - Body: "A track failed to play for someone.", + Body: body, Link: "/collection/reports", } default: @@ -139,6 +160,26 @@ func Render(kind Kind, payload []byte) Rendered { } } +// namesShown is how many of a notice's items its body names. +const namesShown = 3 + +// nameList names the first few items, then how many more of the total there +// are: "A, B, C and 4 more". Empty when there are no items. +func nameList(items []string, total int64) string { + if len(items) == 0 { + return "" + } + shown := items[:min(len(items), namesShown)] + more := max(total, int64(len(items))) - int64(len(shown)) + if more > 0 { + return strings.Join(shown, ", ") + " and " + words.Count(more, "more", "more") + } + if len(shown) == 1 { + return shown[0] + } + return strings.Join(shown[:len(shown)-1], ", ") + " and " + shown[len(shown)-1] +} + func plural(n int64, one, many string) string { if n < 1 { // A coalesced kind always carries a count; this only guards a diff --git a/internal/notifications/render_test.go b/internal/notifications/render_test.go index 6bd6444a..f4509f73 100644 --- a/internal/notifications/render_test.go +++ b/internal/notifications/render_test.go @@ -45,6 +45,36 @@ func TestRender_CoalescedCountsReadNaturally(t *testing.T) { require.Equal(t, "Some tracks went missing", Render(KindTracksMissing, []byte(`{}`)).Title) } +// M542: a notice says which, not only how many. +func TestRender_NamesWhatChanged(t *testing.T) { + require.Equal(t, "No longer on disk: Weezer – Buddy Holly.", + render(KindTracksMissing, Payload{Count: 1, Items: []string{"Weezer – Buddy Holly"}}).Body) + require.Equal(t, "No longer on disk: A – 1 and B – 2.", + render(KindTracksMissing, Payload{Count: 2, Items: []string{"A – 1", "B – 2"}}).Body) + // Three are named; the rest of the count, not only the rest of the + // stored names, is "more". + require.Equal(t, "Held more than once: A, B, C and 9 more.", + render(KindDuplicatesFound, Payload{Count: 12, Items: []string{"A", "B", "C", "D", "E"}}).Body) + require.Equal(t, "12 new duplicates to review", render(KindDuplicatesFound, Payload{Count: 12}).Title) + require.Equal(t, "Extra copies merged into the one the album keeps: A and B.", + render(KindDuplicatesResolved, Payload{Count: 2, Items: []string{"A", "B"}}).Body) + require.Equal(t, "Failed to play: Weezer – Buddy Holly (alice).", + render(KindPlaybackErrors, Payload{Count: 1, Items: []string{"Weezer – Buddy Holly (alice)"}}).Body) +} + +func TestRender_RowsFromBeforeItemsReadAsTheyDid(t *testing.T) { + require.Equal(t, "Their files are no longer where the library expects them.", + render(KindTracksMissing, Payload{Count: 4}).Body) + require.Equal(t, "Merged 8 duplicate copies.", + render(KindDuplicatesResolved, Payload{Count: 716, Detail: "Merged 8 duplicate copies."}).Body) +} + +func TestRender_AScanFailureSaysWhatStopped(t *testing.T) { + r := render(KindScanFailed, Payload{Count: 1, Detail: "scan root \"/music\" unavailable"}) + require.Equal(t, `The last scan stopped: scan root "/music" unavailable`, r.Body) + require.Equal(t, "/admin/library", r.Link) +} + func TestRender_AnUnreadablePayloadStillRenders(t *testing.T) { r := Render(KindRequestApproved, []byte(`not json`)) require.Equal(t, "Request approved", r.Title)