From 86af79bd2f7244b81c8eeb04a96b0d83f364d64c Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 1 Aug 2026 22:51:51 -0400 Subject: [PATCH] =?UTF-8?q?feat(discover):=20time-boxed=20suggestion=20sno?= =?UTF-8?q?oze,=20server=20side=20=E2=80=94=20#2374?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Migration 0049 adds suggestion_snoozes(user_id, candidate_mbid, candidate_name, snoozed_until), and SuggestArtistsForUser excludes rows whose snooze hasn't expired. This is NOT a dislike. Rule #101 forbids a "Not for me" / thumbs-down UI; a snooze is the approved shape instead because it records no verdict on the music, expires on its own (~90d), and never reaches the taste profile. It's acquisition triage — "not right now" — so the filter sits at the candidate stage rather than in the score, where it would become a ranking signal by the back door. Per-user throughout (rule #47): one household member parking a candidate leaves everyone else's deck untouched. candidate_name is denormalized because suggestions are out-of-library by definition — there is no artists row to resolve a display name from, and the un-snooze list has to show something. That list is why GET /discover/snoozes exists at all: a parked candidate is by definition absent from the deck, so without it the DELETE would be unreachable. Also fixes a hole in the codegen check from #2380: `git diff` ignores untracked paths, so a brand-new generated file would have passed it silently. `git add -N` first. This commit is the first to add one. Endpoints: POST /api/discover/suggestions/{mbid}/snooze (body: name, days) DELETE /api/discover/suggestions/{mbid}/snooze GET /api/discover/snoozes UI lands in slice 4 (#2375) before any of this merges — rule #27. Co-Authored-By: Claude Opus 5 (1M context) --- Makefile | 4 + internal/api/api.go | 5 + internal/api/suggestions.go | 150 ++++++++++++ internal/db/dbq/models.go | 8 + internal/db/dbq/recommendation.sql.go | 17 +- internal/db/dbq/suggestion_snoozes.sql.go | 126 +++++++++++ .../0049_suggestion_snoozes.down.sql | 2 + .../migrations/0049_suggestion_snoozes.up.sql | 33 +++ internal/db/queries/recommendation.sql | 17 +- internal/db/queries/suggestion_snoozes.sql | 39 ++++ internal/dbtest/reset.go | 6 + internal/gc/worker.go | 2 + .../suggestions_integration_test.go | 214 ++++++++++++++++++ 13 files changed, 619 insertions(+), 4 deletions(-) create mode 100644 internal/db/dbq/suggestion_snoozes.sql.go create mode 100644 internal/db/migrations/0049_suggestion_snoozes.down.sql create mode 100644 internal/db/migrations/0049_suggestion_snoozes.up.sql create mode 100644 internal/db/queries/suggestion_snoozes.sql diff --git a/Makefile b/Makefile index 69fca217..7b5f63c0 100644 --- a/Makefile +++ b/Makefile @@ -22,6 +22,10 @@ generate-go: # The diff is printed BEFORE the exit-code check on purpose: when this fails, # the log then contains sqlc's exact expected output, which is what you commit. verify-generate: generate-go + # -N (intent-to-add) so a BRAND-NEW generated file is visible to `git + # diff`, which otherwise ignores untracked paths entirely — a whole + # missing *.sql.go would sail through the check below. + git add -N -- internal/db/dbq git --no-pager diff -- internal/db/dbq git diff --quiet -- internal/db/dbq diff --git a/internal/api/api.go b/internal/api/api.go index a16f8f23..7e04e4c8 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -104,6 +104,11 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev authed.Get("/search", h.handleSearch) authed.Get("/radio", h.handleRadio) authed.Get("/discover/suggestions", h.handleListSuggestions) + // Snooze = "not right now", time-boxed and self-expiring + // (#2374). Not a dislike — see the migration for why. + authed.Post("/discover/suggestions/{mbid}/snooze", h.handleSnoozeSuggestion) + authed.Delete("/discover/suggestions/{mbid}/snooze", h.handleUnsnoozeSuggestion) + authed.Get("/discover/snoozes", h.handleListSuggestionSnoozes) authed.Get("/home", h.handleGetHome) authed.Get("/home/index", h.handleGetHomeIndex) authed.Post("/events", h.handleEvents) diff --git a/internal/api/suggestions.go b/internal/api/suggestions.go index 1ad5bf7f..da3bd18c 100644 --- a/internal/api/suggestions.go +++ b/internal/api/suggestions.go @@ -2,13 +2,19 @@ package api import ( "context" + "encoding/json" + "errors" + "io" "net/http" "strconv" + "strings" "sync" + "github.com/go-chi/chi/v5" "github.com/jackc/pgx/v5/pgtype" "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/lidarr" "git.fabledsword.com/bvandeusen/minstrel/internal/recommendation" ) @@ -92,6 +98,150 @@ func (h *handlers) handleListSuggestions(w http.ResponseWriter, r *http.Request) writeJSON(w, http.StatusOK, out) } +// Snooze duration bounds. 90 days is long enough that a parked suggestion +// stops feeling like it's nagging, short enough that a taste shift brings it +// back on its own — the whole point of a snooze over a dismissal (#2374). +const ( + defaultSnoozeDays = 90.0 + maxSnoozeDays = 365.0 +) + +// snoozeRequest is the POST body. Both fields are optional in the JSON sense +// (an absent body snoozes for the default), but Name is required in practice: +// candidates are out-of-library, so the server has no artists row to resolve a +// display name from and the un-snooze list would have nothing to show. The +// client always has it — it just rendered the card. +type snoozeRequest struct { + Name string `json:"name"` + Days float64 `json:"days"` +} + +// snoozeView is one row of GET /api/discover/snoozes. +type snoozeView struct { + MBID string `json:"mbid"` + Name string `json:"name"` + SnoozedUntil pgtype.Timestamptz `json:"snoozed_until"` + CreatedAt pgtype.Timestamptz `json:"created_at"` +} + +// handleSnoozeSuggestion implements +// POST /api/discover/suggestions/{mbid}/snooze. +// +// Parks a candidate for `days` (default 90, capped at 365). Idempotent: +// snoozing an already-snoozed candidate extends it rather than conflicting. +// +// This is NOT negative feedback. It records no verdict on the artist and is +// never read by internal/taste — see 0049_suggestion_snoozes.up.sql for the +// rule #101 reasoning. Returns 204. +func (h *handlers) handleSnoozeSuggestion(w http.ResponseWriter, r *http.Request) { + user, ok := requireUser(w, r) + if !ok { + return + } + mbid := strings.TrimSpace(chi.URLParam(r, "mbid")) + if mbid == "" { + writeErr(w, apierror.BadRequest("invalid_id", "missing mbid")) + return + } + + // An empty body is a valid "snooze this for the default period", so EOF + // is not an error here — decodeBody would reject it as a malformed body. + var body snoozeRequest + if err := json.NewDecoder(r.Body).Decode(&body); err != nil && !errors.Is(err, io.EOF) { + writeErr(w, apierror.BadRequest("invalid_body", "")) + return + } + name := strings.TrimSpace(body.Name) + if name == "" { + writeErr(w, apierror.BadRequest("invalid_body", "name is required")) + return + } + days := body.Days + if days <= 0 { + days = defaultSnoozeDays + } + if days > maxSnoozeDays { + // Clamp rather than reject: a client asking for longer than we allow + // still means "park this", and failing the write would leave the card + // sitting there as if the tap did nothing. + days = maxSnoozeDays + } + + q := dbq.New(h.pool) + if err := q.SnoozeSuggestion(r.Context(), dbq.SnoozeSuggestionParams{ + UserID: user.ID, + CandidateMbid: mbid, + CandidateName: name, + Column4: days, + }); err != nil { + h.logger.Error("api: snooze suggestion", "err", err) + writeErr(w, apierror.InternalMsg("failed to snooze suggestion", err)) + return + } + w.WriteHeader(http.StatusNoContent) +} + +// handleUnsnoozeSuggestion implements +// DELETE /api/discover/suggestions/{mbid}/snooze. +// +// Brings a parked candidate back immediately. 404s an MBID this user never +// snoozed, so the client can tell "undone" from "there was nothing there". +func (h *handlers) handleUnsnoozeSuggestion(w http.ResponseWriter, r *http.Request) { + user, ok := requireUser(w, r) + if !ok { + return + } + mbid := strings.TrimSpace(chi.URLParam(r, "mbid")) + if mbid == "" { + writeErr(w, apierror.BadRequest("invalid_id", "missing mbid")) + return + } + q := dbq.New(h.pool) + rows, err := q.UnsnoozeSuggestion(r.Context(), dbq.UnsnoozeSuggestionParams{ + UserID: user.ID, + CandidateMbid: mbid, + }) + if err != nil { + h.logger.Error("api: unsnooze suggestion", "err", err) + writeErr(w, apierror.InternalMsg("failed to unsnooze suggestion", err)) + return + } + if rows == 0 { + writeErr(w, apierror.NotFound("snooze")) + return + } + w.WriteHeader(http.StatusNoContent) +} + +// handleListSuggestionSnoozes implements GET /api/discover/snoozes. +// +// The un-snooze surface needs this: a parked candidate is by definition +// absent from the suggestion deck, so without a list there is no way to +// reach the DELETE above. Scoped to the caller (rule #47). Expired rows are +// already filtered by the query — the hourly gc sweep only reclaims space. +func (h *handlers) handleListSuggestionSnoozes(w http.ResponseWriter, r *http.Request) { + user, ok := requireUser(w, r) + if !ok { + return + } + rows, err := dbq.New(h.pool).ListActiveSuggestionSnoozes(r.Context(), user.ID) + if err != nil { + h.logger.Error("api: list suggestion snoozes", "err", err) + writeErr(w, apierror.InternalMsg("failed to load snoozes", err)) + return + } + out := make([]snoozeView, 0, len(rows)) + for _, row := range rows { + out = append(out, snoozeView{ + MBID: row.CandidateMbid, + Name: row.CandidateName, + SnoozedUntil: row.SnoozedUntil, + CreatedAt: row.CreatedAt, + }) + } + writeJSON(w, http.StatusOK, out) +} + // resolveSuggestionArt fills ImageURL on-demand from Lidarr's artist // lookup, matched by MBID (foreignArtistId). Best-effort and cache-free: // Lidarr is the only source — when it's disabled, unreachable, or has diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index 10e990ef..cc61f970 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -518,6 +518,14 @@ type SmtpConfig struct { UpdatedAt pgtype.Timestamptz } +type SuggestionSnooze struct { + UserID pgtype.UUID + CandidateMbid string + CandidateName string + SnoozedUntil pgtype.Timestamptz + CreatedAt pgtype.Timestamptz +} + type SystemPlaylistRotationState struct { UserID pgtype.UUID PlaylistKind string diff --git a/internal/db/dbq/recommendation.sql.go b/internal/db/dbq/recommendation.sql.go index 443fc375..d6ed888c 100644 --- a/internal/db/dbq/recommendation.sql.go +++ b/internal/db/dbq/recommendation.sql.go @@ -1082,6 +1082,18 @@ contributions AS ( AND r.lidarr_artist_mbid = u.candidate_mbid AND r.status NOT IN ('rejected', 'failed') ) + -- Snoozed by this user and not yet expired (#2374). Time-boxed and + -- per-user: the candidate returns on its own once snoozed_until + -- passes, and stays visible to everyone else meanwhile. Deliberately + -- filtered at the candidate stage, NOT folded into the score — a + -- snooze carries no opinion about the music, so it must not become a + -- ranking signal. + AND NOT EXISTS ( + SELECT 1 FROM suggestion_snoozes s + WHERE s.user_id = $1 + AND s.candidate_mbid = u.candidate_mbid + AND s.snoozed_until > now() + ) ) SELECT candidate_mbid, candidate_name, @@ -1127,8 +1139,9 @@ type SuggestArtistsForUserRow struct { // undamped sum let one heavily-played artist's neighbours take every slot -- // entrenching harder the MORE the user listened (issue #2367 mechanism 2). // -// Candidates already in the library, or already requested and not terminal, -// are excluded. $1=user_id, $2=half_life_days (tier 2 decay), $3=limit. +// Candidates already in the library, already requested and not terminal, or +// snoozed by this user, are excluded. $1=user_id, $2=half_life_days (tier 2 +// decay), $3=limit. func (q *Queries) SuggestArtistsForUser(ctx context.Context, arg SuggestArtistsForUserParams) ([]SuggestArtistsForUserRow, error) { rows, err := q.db.Query(ctx, suggestArtistsForUser, arg.UserID, arg.Column2, arg.Limit) if err != nil { diff --git a/internal/db/dbq/suggestion_snoozes.sql.go b/internal/db/dbq/suggestion_snoozes.sql.go new file mode 100644 index 00000000..f1642a34 --- /dev/null +++ b/internal/db/dbq/suggestion_snoozes.sql.go @@ -0,0 +1,126 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: suggestion_snoozes.sql + +package dbq + +import ( + "context" + + "github.com/jackc/pgx/v5/pgtype" +) + +const gcDeleteExpiredSuggestionSnoozes = `-- name: GcDeleteExpiredSuggestionSnoozes :execrows +DELETE FROM suggestion_snoozes WHERE snoozed_until < now() +` + +// Keeps the table from growing without bound. Every read already filters on +// snoozed_until > now(), so deleting an expired row changes no behaviour — +// this is purely reclamation. +func (q *Queries) GcDeleteExpiredSuggestionSnoozes(ctx context.Context) (int64, error) { + result, err := q.db.Exec(ctx, gcDeleteExpiredSuggestionSnoozes) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + +const listActiveSuggestionSnoozes = `-- name: ListActiveSuggestionSnoozes :many +SELECT candidate_mbid, candidate_name, snoozed_until, created_at + FROM suggestion_snoozes + WHERE user_id = $1 AND snoozed_until > now() + ORDER BY snoozed_until, candidate_mbid +` + +type ListActiveSuggestionSnoozesRow struct { + CandidateMbid string + CandidateName string + SnoozedUntil pgtype.Timestamptz + CreatedAt pgtype.Timestamptz +} + +// Backs the manage / un-snooze surface. Expired rows are filtered HERE +// rather than left to the sweeper: gc runs on an hourly tick, so a row can +// outlive its expiry by up to a tick and must not read as still-snoozed in +// the meantime. +func (q *Queries) ListActiveSuggestionSnoozes(ctx context.Context, userID pgtype.UUID) ([]ListActiveSuggestionSnoozesRow, error) { + rows, err := q.db.Query(ctx, listActiveSuggestionSnoozes, userID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListActiveSuggestionSnoozesRow + for rows.Next() { + var i ListActiveSuggestionSnoozesRow + if err := rows.Scan( + &i.CandidateMbid, + &i.CandidateName, + &i.SnoozedUntil, + &i.CreatedAt, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const snoozeSuggestion = `-- name: SnoozeSuggestion :exec + +INSERT INTO suggestion_snoozes (user_id, candidate_mbid, candidate_name, snoozed_until) +VALUES ($1, $2, $3, now() + ($4::float8 * INTERVAL '1 day')) +ON CONFLICT (user_id, candidate_mbid) DO UPDATE + SET snoozed_until = EXCLUDED.snoozed_until, + candidate_name = EXCLUDED.candidate_name +` + +type SnoozeSuggestionParams struct { + UserID pgtype.UUID + CandidateMbid string + CandidateName string + Column4 float64 +} + +// Time-boxed "not right now" on a Discover artist suggestion (#2374). +// +// See 0049_suggestion_snoozes.up.sql for why this is a snooze and not a +// dismissal: it records no verdict on the music, expires on its own, and +// must never reach the taste profile. Nothing in internal/taste may read +// this table. +// Upsert, so re-snoozing an already-snoozed candidate EXTENDS it instead of +// erroring on the PK. The name is refreshed too — a later suggestion may +// carry a corrected spelling from the similarity feed. +// $1=user_id, $2=candidate_mbid, $3=candidate_name, $4=duration in days. +func (q *Queries) SnoozeSuggestion(ctx context.Context, arg SnoozeSuggestionParams) error { + _, err := q.db.Exec(ctx, snoozeSuggestion, + arg.UserID, + arg.CandidateMbid, + arg.CandidateName, + arg.Column4, + ) + return err +} + +const unsnoozeSuggestion = `-- name: UnsnoozeSuggestion :execrows +DELETE FROM suggestion_snoozes + WHERE user_id = $1 AND candidate_mbid = $2 +` + +type UnsnoozeSuggestionParams struct { + UserID pgtype.UUID + CandidateMbid string +} + +// Row count is returned so the handler can 404 an MBID that was never +// snoozed rather than reporting success for a no-op. +func (q *Queries) UnsnoozeSuggestion(ctx context.Context, arg UnsnoozeSuggestionParams) (int64, error) { + result, err := q.db.Exec(ctx, unsnoozeSuggestion, arg.UserID, arg.CandidateMbid) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} diff --git a/internal/db/migrations/0049_suggestion_snoozes.down.sql b/internal/db/migrations/0049_suggestion_snoozes.down.sql new file mode 100644 index 00000000..05f6770d --- /dev/null +++ b/internal/db/migrations/0049_suggestion_snoozes.down.sql @@ -0,0 +1,2 @@ +DROP INDEX IF EXISTS suggestion_snoozes_expiry_idx; +DROP TABLE IF EXISTS suggestion_snoozes; diff --git a/internal/db/migrations/0049_suggestion_snoozes.up.sql b/internal/db/migrations/0049_suggestion_snoozes.up.sql new file mode 100644 index 00000000..02b0907f --- /dev/null +++ b/internal/db/migrations/0049_suggestion_snoozes.up.sql @@ -0,0 +1,33 @@ +-- 0049_suggestion_snoozes.up.sql — time-boxed "not right now" on a Discover +-- artist suggestion (#2374, milestone #268 slice 3). +-- +-- This is NOT a dislike. Rule #101 forbids a "Not for me" / thumbs-down / +-- exclusion UI, and a snooze deliberately isn't one: it records no verdict on +-- the music, expires on its own, and MUST NEVER feed the taste profile. It is +-- acquisition triage — "I don't want to request this right now" — so the same +-- candidate is free to return once snoozed_until passes. Anything that reads +-- this table as negative preference signal is a bug. +-- +-- Per-user (rule #47), never global: one household member parking a +-- suggestion must not remove it from anyone else's deck. +-- +-- candidate_mbid is text with NO foreign key, on purpose. Suggestions come +-- from artist_similarity_unmatched and are out-of-library BY DEFINITION, so +-- there is no artists row to reference — the MBID is the only stable identity +-- available. candidate_name is denormalized for the same reason: the manage / +-- un-snooze list has nowhere else to resolve a display name from. + +CREATE TABLE suggestion_snoozes ( + user_id uuid NOT NULL REFERENCES users(id) ON DELETE CASCADE, + candidate_mbid text NOT NULL, + candidate_name text NOT NULL, + snoozed_until timestamptz NOT NULL, + created_at timestamptz NOT NULL DEFAULT now(), + PRIMARY KEY (user_id, candidate_mbid) +); + +-- Supports the gc sweep's unqualified `WHERE snoozed_until < now()` scan. The +-- composite PK already covers every per-user read, so this is the only extra +-- index worth its write cost at household-scale row counts (same reasoning as +-- lidarr_quarantine in 0011). +CREATE INDEX suggestion_snoozes_expiry_idx ON suggestion_snoozes (snoozed_until); diff --git a/internal/db/queries/recommendation.sql b/internal/db/queries/recommendation.sql index d4c8de68..fcb35e8b 100644 --- a/internal/db/queries/recommendation.sql +++ b/internal/db/queries/recommendation.sql @@ -272,8 +272,9 @@ LIMIT 1; -- undamped sum let one heavily-played artist's neighbours take every slot -- -- entrenching harder the MORE the user listened (issue #2367 mechanism 2). -- --- Candidates already in the library, or already requested and not terminal, --- are excluded. $1=user_id, $2=half_life_days (tier 2 decay), $3=limit. +-- Candidates already in the library, already requested and not terminal, or +-- snoozed by this user, are excluded. $1=user_id, $2=half_life_days (tier 2 +-- decay), $3=limit. WITH artist_plays AS ( -- Completed plays only. The previous seed query counted every play_event, -- so skipping an artist repeatedly INCREASED its signal and pushed more of @@ -334,6 +335,18 @@ contributions AS ( AND r.lidarr_artist_mbid = u.candidate_mbid AND r.status NOT IN ('rejected', 'failed') ) + -- Snoozed by this user and not yet expired (#2374). Time-boxed and + -- per-user: the candidate returns on its own once snoozed_until + -- passes, and stays visible to everyone else meanwhile. Deliberately + -- filtered at the candidate stage, NOT folded into the score — a + -- snooze carries no opinion about the music, so it must not become a + -- ranking signal. + AND NOT EXISTS ( + SELECT 1 FROM suggestion_snoozes s + WHERE s.user_id = $1 + AND s.candidate_mbid = u.candidate_mbid + AND s.snoozed_until > now() + ) ) SELECT candidate_mbid, candidate_name, diff --git a/internal/db/queries/suggestion_snoozes.sql b/internal/db/queries/suggestion_snoozes.sql new file mode 100644 index 00000000..952b1ef7 --- /dev/null +++ b/internal/db/queries/suggestion_snoozes.sql @@ -0,0 +1,39 @@ +-- Time-boxed "not right now" on a Discover artist suggestion (#2374). +-- +-- See 0049_suggestion_snoozes.up.sql for why this is a snooze and not a +-- dismissal: it records no verdict on the music, expires on its own, and +-- must never reach the taste profile. Nothing in internal/taste may read +-- this table. + +-- name: SnoozeSuggestion :exec +-- Upsert, so re-snoozing an already-snoozed candidate EXTENDS it instead of +-- erroring on the PK. The name is refreshed too — a later suggestion may +-- carry a corrected spelling from the similarity feed. +-- $1=user_id, $2=candidate_mbid, $3=candidate_name, $4=duration in days. +INSERT INTO suggestion_snoozes (user_id, candidate_mbid, candidate_name, snoozed_until) +VALUES ($1, $2, $3, now() + ($4::float8 * INTERVAL '1 day')) +ON CONFLICT (user_id, candidate_mbid) DO UPDATE + SET snoozed_until = EXCLUDED.snoozed_until, + candidate_name = EXCLUDED.candidate_name; + +-- name: UnsnoozeSuggestion :execrows +-- Row count is returned so the handler can 404 an MBID that was never +-- snoozed rather than reporting success for a no-op. +DELETE FROM suggestion_snoozes + WHERE user_id = $1 AND candidate_mbid = $2; + +-- name: ListActiveSuggestionSnoozes :many +-- Backs the manage / un-snooze surface. Expired rows are filtered HERE +-- rather than left to the sweeper: gc runs on an hourly tick, so a row can +-- outlive its expiry by up to a tick and must not read as still-snoozed in +-- the meantime. +SELECT candidate_mbid, candidate_name, snoozed_until, created_at + FROM suggestion_snoozes + WHERE user_id = $1 AND snoozed_until > now() + ORDER BY snoozed_until, candidate_mbid; + +-- name: GcDeleteExpiredSuggestionSnoozes :execrows +-- Keeps the table from growing without bound. Every read already filters on +-- snoozed_until > now(), so deleting an expired row changes no behaviour — +-- this is purely reclamation. +DELETE FROM suggestion_snoozes WHERE snoozed_until < now(); diff --git a/internal/dbtest/reset.go b/internal/dbtest/reset.go index b499dfeb..180690ce 100644 --- a/internal/dbtest/reset.go +++ b/internal/dbtest/reset.go @@ -52,6 +52,12 @@ var dataTables = []string{ "sessions", "lidarr_quarantine_actions", "lidarr_quarantine", + // #2374. Keyed by (user_id, candidate_mbid) with no FK to artists — + // candidates are out-of-library — so the CASCADE from artists/users + // does NOT reach it for a leftover row whose user survived. Truncate + // explicitly or a stale snooze silently hides a candidate from the + // next test's suggestion assertions. + "suggestion_snoozes", "playlist_tracks", "playlists", "library_changes", // M7 #357 — must reset to keep cursor isolated per test diff --git a/internal/gc/worker.go b/internal/gc/worker.go index b133e6c4..b38dc8f6 100644 --- a/internal/gc/worker.go +++ b/internal/gc/worker.go @@ -17,6 +17,7 @@ // - GcResetStuckSystemPlaylistRuns (#574) // - GcDeleteExpiredPasswordResets (#575) // - GcPruneDiagnostics (M9 — diagnostics 30d retention) +// - GcDeleteExpiredSuggestionSnoozes (#2374 — snoozes expire, then go) package gc import ( @@ -84,6 +85,7 @@ func (w *Worker) tickOnce(ctx context.Context) { w.runSweep(ctx, "reset_stuck_system_runs", q.GcResetStuckSystemPlaylistRuns) w.runSweep(ctx, "delete_expired_password_resets", q.GcDeleteExpiredPasswordResets) w.runSweep(ctx, "prune_diagnostics", q.GcPruneDiagnostics) + w.runSweep(ctx, "delete_expired_suggestion_snoozes", q.GcDeleteExpiredSuggestionSnoozes) } // runSweep is a small adapter so each sweep call site is a one-liner diff --git a/internal/recommendation/suggestions_integration_test.go b/internal/recommendation/suggestions_integration_test.go index 67a4cefd..1f82da8c 100644 --- a/internal/recommendation/suggestions_integration_test.go +++ b/internal/recommendation/suggestions_integration_test.go @@ -472,3 +472,217 @@ func TestSuggestArtists_SkippedPlaysDoNotSeedTier2(t *testing.T) { t.Errorf("len = %d, want 0 (skips are not affinity): %+v", len(out), out) } } + +// --- Slice 3 (#2374): time-boxed suggestion snooze --- +// +// The defining property under test is that a snooze EXPIRES. A permanent +// dismissal would pass most of these; only TestSuggestArtists_ExpiredSnooze +// distinguishes the two, and it is the reason this shape was approved over an +// exclusion UI (rule #101). + +// snoozeUntil inserts a snooze row with an explicit absolute expiry, so a +// test can place it in the past without depending on the duration arithmetic +// in SnoozeSuggestion. +func snoozeUntil(t *testing.T, pool *pgxpool.Pool, userID pgtype.UUID, mbid, name string, until time.Time) { + t.Helper() + if _, err := pool.Exec(context.Background(), + `INSERT INTO suggestion_snoozes (user_id, candidate_mbid, candidate_name, snoozed_until) + VALUES ($1, $2, $3, $4) + ON CONFLICT (user_id, candidate_mbid) DO UPDATE SET snoozed_until = EXCLUDED.snoozed_until`, + userID, mbid, name, until, + ); err != nil { + t.Fatalf("snoozeUntil: %v", err) + } +} + +// seedOneCandidate wires the minimum that puts exactly one candidate in the +// deck: a liked seed artist with one unmatched neighbour. +func seedOneCandidate(t *testing.T, pool *pgxpool.Pool, userID pgtype.UUID, mbid, name string) { + t.Helper() + seed := seedArtist(t, pool, "Seed", "") + likeArtist(t, pool, userID, seed.ID) + seedUnmatched(t, pool, seed.ID, mbid, name, 0.9) +} + +func TestSuggestArtists_ActiveSnoozeHidesCandidate(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + seedOneCandidate(t, pool, user.ID, "snoozed-mbid", "Parked Artist") + + if err := dbq.New(pool).SnoozeSuggestion(context.Background(), dbq.SnoozeSuggestionParams{ + UserID: user.ID, + CandidateMbid: "snoozed-mbid", + CandidateName: "Parked Artist", + Column4: 90, + }); err != nil { + t.Fatalf("SnoozeSuggestion: %v", err) + } + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 0 { + t.Errorf("len = %d, want 0 (active snooze should hide the candidate): %+v", len(out), out) + } +} + +// The whole point of a snooze over a dismissal: it comes back on its own. +func TestSuggestArtists_ExpiredSnoozeShowsCandidateAgain(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + seedOneCandidate(t, pool, user.ID, "expired-mbid", "Returning Artist") + snoozeUntil(t, pool, user.ID, "expired-mbid", "Returning Artist", time.Now().Add(-time.Hour)) + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 1 { + t.Fatalf("len = %d, want 1 (an expired snooze must not hide anything): %+v", len(out), out) + } + if out[0].MBID != "expired-mbid" { + t.Errorf("mbid = %q, want expired-mbid", out[0].MBID) + } +} + +// Rule #47: one household member parking a suggestion must not remove it +// from anyone else's deck. +func TestSuggestArtists_SnoozeIsPerUser(t *testing.T) { + pool := newPool(t) + alice := seedUser(t, pool, "alice") + bob := seedUser(t, pool, "bob") + + seed := seedArtist(t, pool, "Seed", "") + likeArtist(t, pool, alice.ID, seed.ID) + likeArtist(t, pool, bob.ID, seed.ID) + seedUnmatched(t, pool, seed.ID, "shared-mbid", "Shared Candidate", 0.9) + + snoozeUntil(t, pool, alice.ID, "shared-mbid", "Shared Candidate", time.Now().Add(24*time.Hour)) + + aliceOut, err := SuggestArtists(context.Background(), pool, alice.ID, 30, 12) + if err != nil { + t.Fatalf("SuggestArtists(alice): %v", err) + } + if len(aliceOut) != 0 { + t.Errorf("alice len = %d, want 0 (she snoozed it)", len(aliceOut)) + } + bobOut, err := SuggestArtists(context.Background(), pool, bob.ID, 30, 12) + if err != nil { + t.Fatalf("SuggestArtists(bob): %v", err) + } + if len(bobOut) != 1 { + t.Errorf("bob len = %d, want 1 (alice's snooze is not his)", len(bobOut)) + } +} + +func TestUnsnoozeSuggestion_RestoresImmediately(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + seedOneCandidate(t, pool, user.ID, "undo-mbid", "Undo Artist") + snoozeUntil(t, pool, user.ID, "undo-mbid", "Undo Artist", time.Now().Add(90*24*time.Hour)) + + q := dbq.New(pool) + rows, err := q.UnsnoozeSuggestion(context.Background(), dbq.UnsnoozeSuggestionParams{ + UserID: user.ID, CandidateMbid: "undo-mbid", + }) + if err != nil { + t.Fatalf("UnsnoozeSuggestion: %v", err) + } + if rows != 1 { + t.Errorf("rows = %d, want 1", rows) + } + // A second delete affects nothing — the handler turns this into a 404 + // rather than reporting success for a no-op. + rows, err = q.UnsnoozeSuggestion(context.Background(), dbq.UnsnoozeSuggestionParams{ + UserID: user.ID, CandidateMbid: "undo-mbid", + }) + if err != nil { + t.Fatalf("UnsnoozeSuggestion (repeat): %v", err) + } + if rows != 0 { + t.Errorf("repeat rows = %d, want 0", rows) + } + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 1 { + t.Errorf("len = %d, want 1 (unsnooze restores the candidate)", len(out)) + } +} + +// Re-snoozing must extend, not conflict on the PK. +func TestSnoozeSuggestion_UpsertExtends(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + q := dbq.New(pool) + + snoozeUntil(t, pool, user.ID, "extend-mbid", "Old Name", time.Now().Add(time.Hour)) + if err := q.SnoozeSuggestion(context.Background(), dbq.SnoozeSuggestionParams{ + UserID: user.ID, + CandidateMbid: "extend-mbid", + CandidateName: "New Name", + Column4: 90, + }); err != nil { + t.Fatalf("SnoozeSuggestion (re-snooze): %v", err) + } + + rows, err := q.ListActiveSuggestionSnoozes(context.Background(), user.ID) + if err != nil { + t.Fatalf("ListActiveSuggestionSnoozes: %v", err) + } + if len(rows) != 1 { + t.Fatalf("len = %d, want 1 (upsert, not a second row)", len(rows)) + } + if rows[0].CandidateName != "New Name" { + t.Errorf("name = %q, want New Name (upsert refreshes it)", rows[0].CandidateName) + } + if got := time.Until(rows[0].SnoozedUntil.Time); got < 80*24*time.Hour { + t.Errorf("snoozed_until is %v away, want ~90d (re-snooze should extend)", got) + } +} + +// ListActiveSuggestionSnoozes filters expired rows itself rather than +// trusting the hourly gc sweep to have run. +func TestListActiveSuggestionSnoozes_ExcludesExpired(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + snoozeUntil(t, pool, user.ID, "live-mbid", "Live", time.Now().Add(24*time.Hour)) + snoozeUntil(t, pool, user.ID, "dead-mbid", "Dead", time.Now().Add(-24*time.Hour)) + + rows, err := dbq.New(pool).ListActiveSuggestionSnoozes(context.Background(), user.ID) + if err != nil { + t.Fatalf("ListActiveSuggestionSnoozes: %v", err) + } + if len(rows) != 1 { + t.Fatalf("len = %d, want 1 (expired row must not be listed)", len(rows)) + } + if rows[0].CandidateMbid != "live-mbid" { + t.Errorf("mbid = %q, want live-mbid", rows[0].CandidateMbid) + } +} + +func TestGcDeleteExpiredSuggestionSnoozes_KeepsActive(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + snoozeUntil(t, pool, user.ID, "live-mbid", "Live", time.Now().Add(24*time.Hour)) + snoozeUntil(t, pool, user.ID, "dead-mbid", "Dead", time.Now().Add(-24*time.Hour)) + + q := dbq.New(pool) + deleted, err := q.GcDeleteExpiredSuggestionSnoozes(context.Background()) + if err != nil { + t.Fatalf("GcDeleteExpiredSuggestionSnoozes: %v", err) + } + if deleted != 1 { + t.Errorf("deleted = %d, want 1 (only the expired row)", deleted) + } + rows, err := q.ListActiveSuggestionSnoozes(context.Background(), user.ID) + if err != nil { + t.Fatalf("ListActiveSuggestionSnoozes: %v", err) + } + if len(rows) != 1 { + t.Errorf("len = %d, want 1 (active snooze survives the sweep)", len(rows)) + } +}