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)
release / govulncheck (push) Successful in 14s
release / web (push) Successful in 1m22s
release / go (push) Successful in 1m33s
release / android (push) Failing after 1m42s
release / integration (push) Failing after 4m42s
release / Build signed APK (releases and dev) (push) Successful in 4m55s
release / Attach APK to the Release (tag releases only) (push) Skipped
release / Build + push container image (push) Skipped
release / Verify release artifacts (tag releases only) (push) Skipped

- 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 <noreply@anthropic.com>
This commit is contained in:
2026-10-19 22:44:19 -04:00
co-authored by Claude Opus 5.5
parent 2365514e5b
commit 854c307834
26 changed files with 486 additions and 190 deletions
@@ -30,8 +30,9 @@ internal data class CatchUp(
* - The mark moves to the newest notice in the page, announced or not, so a * - The mark moves to the newest notice in the page, announced or not, so a
* kind with the phone off is never announced later. * kind with the phone off is never announced later.
* *
* A coalesced notice (tracks missing, now 5) moves its time forward when its * A coalesced notice (tracks missing, now 5) keeps the time it arrived when
* count grows, so it is announced again with the new count. * 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( internal fun planCatchUp(
page: List<CachedNotificationEntity>, page: List<CachedNotificationEntity>,
@@ -51,18 +52,32 @@ internal fun planCatchUp(
internal sealed interface Announcement { internal sealed interface Announcement {
data class Each(val items: List<CachedNotificationEntity>) : Announcement data class Each(val items: List<CachedNotificationEntity>) : 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<String> = emptyList(),
) : Announcement
} }
internal fun announcementFor(fresh: List<CachedNotificationEntity>): Announcement? = when { /** How many titles an expanded pile lists, newest first. */
fresh.isEmpty() -> null internal const val PILE_LINES = 5
fresh.size <= INDIVIDUAL_LIMIT -> Announcement.Each(fresh)
internal fun announcementFor(fresh: List<CachedNotificationEntity>): 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 // 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. // one of them is, so a listener's own news is never filed under it.
fresh.all { shadeChannelFor(it.kind) == ShadeChannel.LIBRARY_HEALTH } -> val channel = if (fresh.all { shadeChannelFor(it.kind) == ShadeChannel.LIBRARY_HEALTH }) {
Announcement.Pile(fresh.size, ShadeChannel.LIBRARY_HEALTH) ShadeChannel.LIBRARY_HEALTH
else -> Announcement.Pile(fresh.size, ShadeChannel.YOUR_REQUESTS) } 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. */ /** 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. */ /** One line for a pile. */
internal fun pileText(count: Int): String = "$count new notifications" 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" }
@@ -1,6 +1,7 @@
package com.fabledsword.minstrel.notifications.delivery package com.fabledsword.minstrel.notifications.delivery
import android.annotation.SuppressLint import android.annotation.SuppressLint
import android.app.Notification
import android.app.NotificationChannel import android.app.NotificationChannel
import android.app.NotificationManager import android.app.NotificationManager
import android.app.PendingIntent import android.app.PendingIntent
@@ -50,14 +51,20 @@ class NotificationPoster @Inject constructor(
.setContentIntent(openIntent(item.link, item.id.hashCode())) .setContentIntent(openIntent(item.link, item.id.hashCode()))
.build() .build()
private fun pile(p: Announcement.Pile) = /** Collapsed, the count; expanded, the newest titles and how many more. */
NotificationCompat.Builder(context, p.channel.id) 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) .setSmallIcon(R.drawable.ic_notification)
.setContentTitle("Minstrel") .setContentTitle(pileText(p.count))
.setContentText(pileText(p.count)) .setContentText(p.lines.joinToString(" · "))
.setStyle(style)
.setAutoCancel(true) .setAutoCancel(true)
.setContentIntent(openIntent(link = null, requestCode = PILE_ID)) .setContentIntent(openIntent(link = null, requestCode = PILE_ID))
.build() .build()
}
/** Each notice gets its own request code, so their links stay apart. */ /** Each notice gets its own request code, so their links stay apart. */
private fun openIntent(link: String?, requestCode: Int): PendingIntent = private fun openIntent(link: String?, requestCode: Int): PendingIntent =
@@ -99,6 +99,16 @@ class DeliveryPlanTest {
assertEquals(5, pile.count) assertEquals(5, pile.count)
assertEquals(ShadeChannel.YOUR_REQUESTS, pile.channel) assertEquals(ShadeChannel.YOUR_REQUESTS, pile.channel)
assertEquals("5 new notifications", pileText(pile.count)) 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<Announcement.Pile>(announcementFor(many))
assertEquals(listOf("t-n17", "t-n16", "t-n15", "t-n14", "t-n13"), pile.lines)
assertEquals("and 12 more", pileMore(pile))
} }
@Test @Test
+11 -12
View File
@@ -6,6 +6,7 @@ import (
"github.com/jackc/pgx/v5/pgtype" "github.com/jackc/pgx/v5/pgtype"
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "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/notifications"
"git.fabledsword.com/bvandeusen/minstrel/internal/requests" "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 { if err != nil {
return "" return ""
} }
if a, aerr := q.GetArtistByID(ctx, t.ArtistID); aerr == nil && a.Name != "" { if a, aerr := q.GetArtistByID(ctx, t.ArtistID); aerr == nil {
return a.Name + " – " + t.Title return library.TrackLabel(a.Name, t.Title)
} }
return t.Title return t.Title
} }
// notifyPlaybackErrors tells admins how many playback errors await review. // notifyPlaybackErrors tells admins a track failed to play, naming it and who
// The notice coalesces, so while it is unread a new report only updates the // it failed for (M542). The notice coalesces: while it is unread each report
// count. // adds one to its count and its name to the front of the list.
func (h *handlers) notifyPlaybackErrors(ctx context.Context) { func (h *handlers) notifyPlaybackErrors(ctx context.Context, trackID pgtype.UUID, user dbq.User) {
n, err := dbq.New(h.pool).CountUnresolvedPlaybackErrors(ctx) p := notifications.Payload{Count: 1}
if err != nil { if name := h.trackLabel(ctx, trackID); name != "" {
h.logger.Warn("api: playback_error: count unresolved", "err", err) p.Items = []string{name + " (" + userLabel(user) + ")"}
return
} }
h.notifier.NotifyLogged(ctx, notifications.KindPlaybackErrors, notifications.ToAdmins(pgtype.UUID{}), h.notifier.NotifyLogged(ctx, notifications.KindPlaybackErrors, notifications.ToAdmins(pgtype.UUID{}), p.Map())
notifications.Payload{Count: n}.Map())
} }
+3 -2
View File
@@ -211,8 +211,9 @@ func TestNotify_PlaybackErrorsCoalesceIntoOneCount(t *testing.T) {
got := inboxOf(t, pool, admin) got := inboxOf(t, pool, admin)
wantInbox(t, "admin", got, notifications.KindPlaybackErrors) wantInbox(t, "admin", got, notifications.KindPlaybackErrors)
if got[0].Payload.Count != 2 { if got[0].Payload.Count != 2 || len(got[0].Payload.Items) != 2 ||
t.Errorf("count = %d, want 2 unresolved", got[0].Payload.Count) !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)) wantInbox(t, "reporter", inboxOf(t, pool, alice))
} }
+1 -1
View File
@@ -130,7 +130,7 @@ func (h *handlers) handleReportPlaybackError(w http.ResponseWriter, r *http.Requ
writeErr(w, apierror.Internal(err)) writeErr(w, apierror.Internal(err))
return return
} }
h.notifyPlaybackErrors(r.Context()) h.notifyPlaybackErrors(r.Context(), trackID, user)
writeJSON(w, http.StatusCreated, map[string]string{"id": uuidToString(row.ID)}) writeJSON(w, http.StatusCreated, map[string]string{"id": uuidToString(row.ID)})
} }
+52 -18
View File
@@ -27,24 +27,6 @@ func (q *Queries) AddDuplicateGroupMember(ctx context.Context, arg AddDuplicateG
return err 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 const countPendingDuplicateGroups = `-- name: CountPendingDuplicateGroups :one
SELECT count(*)::bigint SELECT count(*)::bigint
FROM duplicate_groups g FROM duplicate_groups g
@@ -311,6 +293,58 @@ func (q *Queries) ListDuplicateCandidates(ctx context.Context, arg ListDuplicate
return items, nil 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 const listDuplicateGroupsForResolve = `-- name: ListDuplicateGroupsForResolve :many
SELECT g.id AS group_id, SELECT g.id AS group_id,
g.tier, g.tier,
+20 -22
View File
@@ -346,8 +346,9 @@ type MarkAllNotificationsReadParams struct {
// up_to, when set, limits it to what existed when the user asked: a "mark // 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 // all read" queued offline and replayed later must not mark notices that
// arrived in between, which the user never saw. A coalesced row updated // arrived in between, which the user never saw. A coalesced row that grew
// since then carries a newer created_at, so it stays unread too. // 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) { func (q *Queries) MarkAllNotificationsRead(ctx context.Context, arg MarkAllNotificationsReadParams) (int64, error) {
result, err := q.db.Exec(ctx, markAllNotificationsRead, arg.UserID, arg.UpTo) result, err := q.db.Exec(ctx, markAllNotificationsRead, arg.UserID, arg.UpTo)
if err != nil { if err != nil {
@@ -457,15 +458,15 @@ VALUES ($1, $2, $3, $4,
CASE WHEN $5::boolean THEN NULL ELSE now() END) 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 ON CONFLICT (user_id, coalesce_key) WHERE read_at IS NULL AND coalesce_key IS NOT NULL
DO UPDATE SET DO UPDATE SET
payload = CASE payload = EXCLUDED.payload || jsonb_build_object(
WHEN $6::boolean THEN 'count', COALESCE((user_notifications.payload->>'count')::bigint, 0)
EXCLUDED.payload || jsonb_build_object('count', + COALESCE((EXCLUDED.payload->>'count')::bigint, 0),
COALESCE((user_notifications.payload->>'count')::bigint, 0) 'items', (SELECT COALESCE(jsonb_agg(e.item ORDER BY e.n), '[]'::jsonb)
+ COALESCE((EXCLUDED.payload->>'count')::bigint, 0)) FROM jsonb_array_elements(
ELSE EXCLUDED.payload COALESCE(EXCLUDED.payload->'items', '[]'::jsonb)
END, || COALESCE(user_notifications.payload->'items', '[]'::jsonb)
created_at = now(), ) WITH ORDINALITY AS e(item, n)
emailed_at = EXCLUDED.emailed_at WHERE e.n <= 10))
RETURNING id RETURNING id
` `
@@ -475,20 +476,18 @@ type UpsertCoalescedNotificationParams struct {
Payload []byte Payload []byte
CoalesceKey *string CoalesceKey *string
EmailWanted bool EmailWanted bool
SumCount bool
} }
// One unread row per (user, coalesce_key). A new event while that row is // 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 // created_at and emailed_at are left alone (M542): an update changes what the
// replacing it. For kinds whose event is "N more happened" (tracks marked // notice says, not when it arrived. Moving created_at made the phone announce
// missing). Kinds whose event states the whole current total (pending // it again and clearing emailed_at sent it again, so a notice the hourly
// duplicate groups) pass false and the payload simply replaces. // duplicate pass kept growing alerted every hour.
//
// 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.
func (q *Queries) UpsertCoalescedNotification(ctx context.Context, arg UpsertCoalescedNotificationParams) (pgtype.UUID, error) { func (q *Queries) UpsertCoalescedNotification(ctx context.Context, arg UpsertCoalescedNotificationParams) (pgtype.UUID, error) {
row := q.db.QueryRow(ctx, upsertCoalescedNotification, row := q.db.QueryRow(ctx, upsertCoalescedNotification,
arg.UserID, arg.UserID,
@@ -496,7 +495,6 @@ func (q *Queries) UpsertCoalescedNotification(ctx context.Context, arg UpsertCoa
arg.Payload, arg.Payload,
arg.CoalesceKey, arg.CoalesceKey,
arg.EmailWanted, arg.EmailWanted,
arg.SumCount,
) )
var id pgtype.UUID var id pgtype.UUID
err := row.Scan(&id) err := row.Scan(&id)
-11
View File
@@ -11,17 +11,6 @@ import (
"github.com/jackc/pgx/v5/pgtype" "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 const insertPlaybackError = `-- name: InsertPlaybackError :one
INSERT INTO playback_errors (track_id, user_id, client_id, kind, detail) INSERT INTO playback_errors (track_id, user_id, client_id, kind, detail)
VALUES ($1, $2, $3, $4, $5) VALUES ($1, $2, $3, $4, $5)
+37
View File
@@ -646,6 +646,43 @@ func (q *Queries) ListRandomTracksForUser(ctx context.Context, arg ListRandomTra
return items, nil 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 const listSuspectSourceTracks = `-- name: ListSuspectSourceTracks :many
SELECT t.id, SELECT t.id,
t.title, t.title,
+18 -8
View File
@@ -184,15 +184,25 @@ UPDATE duplicate_groups
SET status = 'dismissed', resolved_at = now() SET status = 'dismissed', resolved_at = now()
WHERE id = sqlc.arg(id) AND status = 'pending'; 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 -- 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, -- then found for the first time, each named by one of its songs, and `total`
-- so a sweep that only re-finds known groups counts none (M489: admins are -- of them however many are listed. A refreshed proposal keeps its detected_at,
-- told about new duplicates, not reminded of the same ones every sweep). -- so a sweep that only re-finds known groups lists none (M489: admins are told
SELECT count(*)::bigint -- about new duplicates, not reminded of the same ones every sweep).
FROM duplicate_groups SELECT ar.name AS artist_name, t.title, count(*) OVER ()::bigint AS total
WHERE status = 'pending' FROM duplicate_groups g
AND detected_at >= sqlc.arg(since); 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 -- name: ListDuplicateGroupsForResolve :many
-- Every pending group's present copies, for the resolver (M498 #5435): what it -- Every pending group's present copies, for the resolver (M498 #5435): what it
+20 -20
View File
@@ -13,30 +13,29 @@ RETURNING id;
-- name: UpsertCoalescedNotification :one -- name: UpsertCoalescedNotification :one
-- One unread row per (user, coalesce_key). A new event while that row is -- 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 -- created_at and emailed_at are left alone (M542): an update changes what the
-- replacing it. For kinds whose event is "N more happened" (tracks marked -- notice says, not when it arrived. Moving created_at made the phone announce
-- missing). Kinds whose event states the whole current total (pending -- it again and clearing emailed_at sent it again, so a notice the hourly
-- duplicate groups) pass false and the payload simply replaces. -- duplicate pass kept growing alerted every hour.
--
-- 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.
INSERT INTO user_notifications (user_id, kind, payload, coalesce_key, emailed_at) 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), 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) 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 ON CONFLICT (user_id, coalesce_key) WHERE read_at IS NULL AND coalesce_key IS NOT NULL
DO UPDATE SET DO UPDATE SET
payload = CASE payload = EXCLUDED.payload || jsonb_build_object(
WHEN sqlc.arg(sum_count)::boolean THEN 'count', COALESCE((user_notifications.payload->>'count')::bigint, 0)
EXCLUDED.payload || jsonb_build_object('count', + COALESCE((EXCLUDED.payload->>'count')::bigint, 0),
COALESCE((user_notifications.payload->>'count')::bigint, 0) 'items', (SELECT COALESCE(jsonb_agg(e.item ORDER BY e.n), '[]'::jsonb)
+ COALESCE((EXCLUDED.payload->>'count')::bigint, 0)) FROM jsonb_array_elements(
ELSE EXCLUDED.payload COALESCE(EXCLUDED.payload->'items', '[]'::jsonb)
END, || COALESCE(user_notifications.payload->'items', '[]'::jsonb)
created_at = now(), ) WITH ORDINALITY AS e(item, n)
emailed_at = EXCLUDED.emailed_at WHERE e.n <= 10))
RETURNING id; RETURNING id;
-- name: ListNotifications :many -- name: ListNotifications :many
@@ -65,8 +64,9 @@ UPDATE user_notifications
-- name: MarkAllNotificationsRead :execrows -- name: MarkAllNotificationsRead :execrows
-- up_to, when set, limits it to what existed when the user asked: a "mark -- 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 -- all read" queued offline and replayed later must not mark notices that
-- arrived in between, which the user never saw. A coalesced row updated -- arrived in between, which the user never saw. A coalesced row that grew
-- since then carries a newer created_at, so it stays unread too. -- 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 UPDATE user_notifications
SET read_at = now() SET read_at = now()
WHERE user_id = sqlc.arg(user_id) WHERE user_id = sqlc.arg(user_id)
-3
View File
@@ -61,6 +61,3 @@ UPDATE playback_errors
resolved_by = $2, resolved_by = $2,
resolution = $3 resolution = $3
WHERE track_id = $1 AND resolved_at IS NULL; WHERE track_id = $1 AND resolved_at IS NULL;
-- name: CountUnresolvedPlaybackErrors :one
SELECT count(*)::bigint FROM playback_errors WHERE resolved_at IS NULL;
+11
View File
@@ -335,3 +335,14 @@ UPDATE tracks
SET source_verdict = sqlc.arg(verdict)::text, source_verdict_at = now() SET source_verdict = sqlc.arg(verdict)::text, source_verdict_at = now()
WHERE id = sqlc.arg(id) WHERE id = sqlc.arg(id)
AND source_verdict IS NOT NULL; 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;
+5 -3
View File
@@ -15,7 +15,6 @@ import (
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
"git.fabledsword.com/bvandeusen/minstrel/internal/notifications" "git.fabledsword.com/bvandeusen/minstrel/internal/notifications"
syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync" syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync"
"git.fabledsword.com/bvandeusen/minstrel/internal/words"
) )
// Duplicate resolver (M498 #5435, #5437; M502 #5517). // 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. // Merges before links, so a rip merged away is not linked first.
merged := map[pgtype.UUID]bool{} merged := map[pgtype.UUID]bool{}
var mergedNames []string
for _, g := range groups { for _, g := range groups {
if res.Merged >= resolveMergeCap { if res.Merged >= resolveMergeCap {
break break
@@ -167,6 +167,9 @@ func ResolveDuplicates(
} }
merged[g.id] = true merged[g.id] = true
res.Merged++ 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) linked, err := linkCrossRelease(ctx, pool, logger, groups, merged)
@@ -177,8 +180,7 @@ func ResolveDuplicates(
if res.Merged > 0 { if res.Merged > 0 {
notifyAdmins(ctx, notifications.KindDuplicatesResolved, notifications.Payload{ notifyAdmins(ctx, notifications.KindDuplicatesResolved, notifications.Payload{
Count: int64(res.Merged), Count: int64(res.Merged), Items: mergedNames,
Detail: "Merged " + words.Count(res.Merged, "duplicate copy", "duplicate copies") + ".",
}) })
} }
return res, nil return res, nil
+13 -12
View File
@@ -112,25 +112,26 @@ func runDuplicateSweep(
return res, runErr return res, runErr
} }
// notifyNewDuplicates tells admins when a sweep proposed a group it had not // notifyNewDuplicates tells admins when a sweep proposed groups it had not
// proposed before (M489), counting every proposal awaiting review. A sweep // proposed before (M489), counting and naming those (M542). A sweep that only
// that only re-finds known groups says nothing, so reading the notice once // re-finds known groups says nothing, so reading the notice once is enough
// is enough until something new turns up. // until something new turns up.
func notifyNewDuplicates(ctx context.Context, q *dbq.Queries, sweepStarted pgtype.Timestamptz, logger *slog.Logger) { 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 { if err != nil {
logger.Warn("duplicate sweep: counting new proposals failed", "err", err) logger.Warn("duplicate sweep: listing new proposals failed", "err", err)
return return
} }
if fresh == 0 { if len(fresh) == 0 {
return return
} }
pending, err := q.CountPendingDuplicateGroups(ctx) items := make([]string, 0, len(fresh))
if err != nil { for _, g := range fresh {
logger.Warn("duplicate sweep: counting pending proposals failed", "err", err) items = append(items, TrackLabel(g.ArtistName, g.Title))
return
} }
notifyAdmins(ctx, notifications.KindDuplicatesFound, notifications.Payload{Count: pending}) notifyAdmins(ctx, notifications.KindDuplicatesFound, notifications.Payload{Count: fresh[0].Total, Items: items})
} }
func sweepDuplicates( func sweepDuplicates(
+37
View File
@@ -6,6 +6,7 @@ import (
"github.com/jackc/pgx/v5/pgtype" "github.com/jackc/pgx/v5/pgtype"
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
"git.fabledsword.com/bvandeusen/minstrel/internal/notifications" "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()) 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 // notifyScanFinished tells admins a scan run ended in error. A scan cut short
// by shutdown is not a failure anyone needs telling about. // by shutdown is not a failure anyone needs telling about.
func notifyScanFinished(ctx context.Context, errMsg string) { func notifyScanFinished(ctx context.Context, errMsg string) {
+65 -23
View File
@@ -10,6 +10,7 @@ import (
"path/filepath" "path/filepath"
"testing" "testing"
"github.com/jackc/pgx/v5/pgtype"
"github.com/jackc/pgx/v5/pgxpool" "github.com/jackc/pgx/v5/pgxpool"
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
@@ -36,6 +37,7 @@ type notice struct {
kind string kind string
count int64 count int64
body string body string
items []string
} }
func unreadNotices(t *testing.T, pool *pgxpool.Pool, user dbq.User) []notice { 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 { if err := json.Unmarshal(r.Payload, &p); err != nil {
t.Fatalf("payload: %v", err) 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 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) pool := newPool(t)
admin := notifyingAdmin(t, pool) admin := notifyingAdmin(t, pool)
s := testScanner(t, populatedRoot(t)) s := testScanner(t, populatedRoot(t))
// Two scans, each losing 2 of 10 tracks (under the mark cap). rows := make([]dbq.ListTrackPathsForReconcileRow, 0, 10)
for pass := 0; pass < 2; pass++ { seen := map[string]struct{}{}
rows := make([]dbq.ListTrackPathsForReconcileRow, 0, 10) for i := 0; i < 10; i++ {
seen := map[string]struct{}{} p := fmt.Sprintf("/music/%02d.mp3", i)
for i := 0; i < 10; i++ { rows = append(rows, row(byte(i), p, false))
p := fmt.Sprintf("/music/pass-%d-%02d.mp3", pass, i) if i >= 2 {
rows = append(rows, row(byte(pass*10+i), p, false)) seen[p] = struct{}{}
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)
} }
} }
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) got := unreadNotices(t, pool, admin)
if len(got) != 1 || got[0].kind != string(notifications.KindTracksMissing) || got[0].count != 4 { if len(got) != 1 || got[0].kind != string(notifications.KindTracksMissing) || got[0].count != 1 ||
t.Fatalf("notices = %+v, want one tracks_missing counting 4", got) 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) pair("first", 1, 100)
sweep() sweep()
got := unreadNotices(t, pool, admin) got := unreadNotices(t, pool, admin)
if len(got) != 1 || got[0].kind != string(notifications.KindDuplicatesFound) || got[0].count != 1 { 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) 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. // 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) 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) pair("second", 2, 200)
sweep() sweep()
got = unreadNotices(t, pool, admin) got = unreadNotices(t, pool, admin)
if len(got) != 1 || got[0].count != 2 { 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 counting 2", got) t.Fatalf("after a new pair notices = %+v, want one naming only the new pair", got)
} }
} }
+3 -4
View File
@@ -9,7 +9,6 @@ import (
"github.com/jackc/pgx/v5/pgtype" "github.com/jackc/pgx/v5/pgtype"
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
"git.fabledsword.com/bvandeusen/minstrel/internal/notifications"
syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync" syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync"
) )
@@ -129,9 +128,9 @@ func (s *Scanner) reconcileMissing(
// this line. // this line.
s.logger.Warn("library scan: tracks marked missing (files not found)", s.logger.Warn("library scan: tracks marked missing (files not found)",
"count", n, "library_total", len(rows)) "count", n, "library_total", len(rows))
if n > 0 { // Admins are told after the scan has adopted moves and folded
notifyAdmins(ctx, notifications.KindTracksMissing, notifications.Payload{Count: n}) // replacements, about the ones still gone (notifyStillMissing).
} stats.marked = toMark
return nil return nil
} }
+7
View File
@@ -83,6 +83,10 @@ type Stats struct {
// Only a full Scan sets these — see reconcileMissing. // Only a full Scan sets these — see reconcileMissing.
Missing int `json:"missing"` Missing int `json:"missing"`
Restored int `json:"restored"` 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 { 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 { if _, err := PairMissingTracks(ctx, s.pool, s.logger); err != nil && ctx.Err() == nil {
s.logger.Warn("library scan: missing-pair pass failed", "err", err) 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 return stats, nil
} }
+8 -1
View File
@@ -31,6 +31,13 @@ type LibraryStageTallies struct {
Restored int `json:"restored"` 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. // MBIDBackfillStageTallies wires BackfillMBIDsResult into the scan_runs jsonb column.
type MBIDBackfillStageTallies struct { type MBIDBackfillStageTallies struct {
Processed int `json:"processed"` Processed int `json:"processed"`
@@ -105,7 +112,7 @@ func RunScan(
var lastStats Stats var lastStats Stats
libPub := newStagePublisher(ctx, 500, time.Second, libPub := newStagePublisher(ctx, 500, time.Second,
func() []byte { func() []byte {
blob, _ := json.Marshal(LibraryStageTallies(lastStats)) blob, _ := json.Marshal(lastStats.tallies())
return blob return blob
}, },
func(blob []byte) error { func(blob []byte) error {
+11 -15
View File
@@ -69,12 +69,9 @@ func (c Channels) Effective() Channels {
type spec struct { type spec struct {
audience Audience audience Audience
// coalesce: while one of these is unread, a new event updates it in place // 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 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 group EmailGroup
defaults Channels defaults Channels
} }
@@ -88,16 +85,15 @@ var (
) )
var specs = map[Kind]spec{ var specs = map[Kind]spec{
KindRequestApproved: {audience: AudienceRequester, group: EmailBatch, defaults: allOn}, KindRequestApproved: {audience: AudienceRequester, group: EmailBatch, defaults: allOn},
KindRequestRejected: {audience: AudienceRequester, group: EmailBatch, defaults: allOn}, KindRequestRejected: {audience: AudienceRequester, group: EmailBatch, defaults: allOn},
KindRequestCompleted: {audience: AudienceRequester, group: EmailSummary, defaults: allOn}, KindRequestCompleted: {audience: AudienceRequester, group: EmailSummary, defaults: allOn},
KindRequestPending: {audience: AudienceAdmin, group: EmailBatch, defaults: allOn}, KindRequestPending: {audience: AudienceAdmin, group: EmailBatch, defaults: allOn},
KindQuarantineFlagged: {audience: AudienceAdmin, group: EmailBatch, defaults: allOn}, KindQuarantineFlagged: {audience: AudienceAdmin, group: EmailBatch, defaults: allOn},
KindScanFailed: {audience: AudienceAdmin, coalesce: true, sumCount: true, group: EmailBatch, defaults: healthAlert}, KindScanFailed: {audience: AudienceAdmin, coalesce: true, group: EmailBatch, defaults: healthAlert},
KindTracksMissing: {audience: AudienceAdmin, coalesce: true, sumCount: true, group: EmailBatch, defaults: healthAlert}, KindTracksMissing: {audience: AudienceAdmin, coalesce: true, group: EmailBatch, defaults: healthAlert},
KindDuplicatesFound: {audience: AudienceAdmin, coalesce: 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, group: EmailBatch, defaults: inboxOnly},
KindDuplicatesResolved: {audience: AudienceAdmin, coalesce: true, sumCount: true, group: EmailBatch, defaults: inboxOnly},
KindPlaybackErrors: {audience: AudienceAdmin, coalesce: true, group: EmailBatch, defaults: healthAlert}, KindPlaybackErrors: {audience: AudienceAdmin, coalesce: true, group: EmailBatch, defaults: healthAlert},
} }
+3 -3
View File
@@ -57,8 +57,9 @@ func New(db dbq.DBTX, bus *eventbus.Bus, logger *slog.Logger) *Notifier {
// then nudges their open clients. // then nudges their open clients.
// //
// For a coalescing kind, an unread row of that kind is updated in place // 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 // rather than a new one added: payload["count"] adds to the row's count and
// row's count for kinds whose events each mean "N more". // 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 // 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. // 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, Payload: body,
CoalesceKey: &key, CoalesceKey: &key,
EmailWanted: emailWanted, EmailWanted: emailWanted,
SumCount: specs[kind].sumCount,
}) })
return err return err
} }
+27 -5
View File
@@ -146,19 +146,41 @@ func TestNotify_CoalescedCountAddsUpWhileUnread_AndStartsAfreshOnceRead(t *testi
require.True(t, rows[1].ReadAt.Valid) 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) pool := testPool(t)
q := dbq.New(pool) q := dbq.New(pool)
ctx := context.Background() ctx := context.Background()
admin := mkUser(t, q, "admin", true) admin := mkUser(t, q, "admin", true)
n := notifications.New(pool, nil, nil) 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. notify("A", "B")
require.NoError(t, n.Notify(ctx, notifications.KindDuplicatesFound, notifications.ToUser(admin), map[string]any{"count": 5})) first := rowsFor(t, q, admin)
require.NoError(t, n.Notify(ctx, notifications.KindDuplicatesFound, notifications.ToUser(admin), map[string]any{"count": 3})) 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) rows := rowsFor(t, q, admin)
require.Len(t, rows, 1) 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) { func TestNotify_RequestKindsNeverCoalesce(t *testing.T) {
+54 -13
View File
@@ -3,6 +3,7 @@ package notifications
import ( import (
"encoding/json" "encoding/json"
"fmt" "fmt"
"strings"
"git.fabledsword.com/bvandeusen/minstrel/internal/words" "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 is the other person involved: who asked, who flagged.
Actor string `json:"actor,omitempty"` Actor string `json:"actor,omitempty"`
Reason string `json:"reason,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"` Count int64 `json:"count,omitempty"`
// Detail is free text: scan_failed's error message, and what the duplicate // Items name what Count counts, one each, newest first: "Weezer – Buddy
// resolver merged for duplicates_resolved. // 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"` Detail string `json:"detail,omitempty"`
} }
@@ -101,37 +107,52 @@ func Render(kind Kind, payload []byte) Rendered {
if p.Count > 1 { if p.Count > 1 {
title = fmt.Sprintf("Library scan failed %d times", p.Count) title = fmt.Sprintf("Library scan failed %d times", p.Count)
} }
body := p.Detail body := "The last scan stopped with an error."
if body == "" { if p.Detail != "" {
body = "The last scan stopped with an error." 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: 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{ return Rendered{
Title: plural(p.Count, "track went missing", "tracks went missing"), 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", Link: "/collection/missing",
} }
case KindDuplicatesFound: 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{ return Rendered{
Title: plural(p.Count, "duplicate group to review", "duplicate groups to review"), Title: plural(p.Count, "new duplicate to review", "new duplicates to review"),
Body: "The duplicate sweep found tracks holding the same recording.", Body: body,
Link: "/collection/duplicates", Link: "/collection/duplicates",
} }
case KindDuplicatesResolved: case KindDuplicatesResolved:
body := "Extra copies of a song were merged into the one the album keeps." 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 body = p.Detail
} }
return Rendered{ return Rendered{
Title: plural(p.Count, "duplicate resolved automatically", "duplicates resolved automatically"), Title: plural(p.Count, "duplicate merged automatically", "duplicates merged automatically"),
Body: body, Body: body,
Link: "/collection/duplicates?view=resolved", Link: "/collection/duplicates?view=resolved",
} }
case KindPlaybackErrors: 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{ return Rendered{
Title: plural(p.Count, "playback error reported", "playback errors reported"), Title: plural(p.Count, "playback error reported", "playback errors reported"),
Body: "A track failed to play for someone.", Body: body,
Link: "/collection/reports", Link: "/collection/reports",
} }
default: 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 { func plural(n int64, one, many string) string {
if n < 1 { if n < 1 {
// A coalesced kind always carries a count; this only guards a // A coalesced kind always carries a count; this only guards a
+30
View File
@@ -45,6 +45,36 @@ func TestRender_CoalescedCountsReadNaturally(t *testing.T) {
require.Equal(t, "Some tracks went missing", Render(KindTracksMissing, []byte(`{}`)).Title) 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) { func TestRender_AnUnreadablePayloadStillRenders(t *testing.T) {
r := Render(KindRequestApproved, []byte(`not json`)) r := Render(KindRequestApproved, []byte(`not json`))
require.Equal(t, "Request approved", r.Title) require.Equal(t, "Request approved", r.Title)