diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index d50ac13..740d7f9 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -96,7 +96,7 @@ Columns ending in `_ct` hold AES-256-GCM ciphertext (via `secretbox`). | `recipe_ingredients` | recipe_id, position, raw_text, name, qty, unit, note. | | `recipe_steps` | recipe_id, position, text. | | `photos` | source_url, local_path, width, height, sha256, fetched_at. | -| `dinners` | The meal record and its lifecycle: name, normalized_name, status (`suggested` / `quest_log` / `tried` / `regular` / `archived`), origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, recipe_url (a pasted recipe link awaiting grounding), photo_id, grounding_status (`pending` / `grounded` / `failed` / `manual`), grounding_error (why grounding failed), grounding_revision (raised when a pending dinner is renamed, so only the latest `ground_dinner` job may write it), updated_by. | +| `dinners` | The meal record and its lifecycle: name, normalized_name, status (`suggested` / `quest_log` / `tried` / `regular` / `archived`), origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, recipe_url (a pasted recipe link awaiting grounding), photo_id, grounding_status (`pending` / `grounded` / `failed` / `manual`), grounding_error (why grounding failed), grounding_revision (raised whenever a dinner is renamed, so only the latest `ground_dinner` job may write it and no candidate offered before the rename grounds it), updated_by. | | `suggestion_batches` | trigger, status, ai_run_id. | | `suggestions` | dinner_id, batch_id, name (the name the dish was suggested under, kept whatever its dinner is later renamed, so a Pass stays an immutable record every later batch dedupes against), reason, cited_regular_ids (JSON), status, decided_at. | | `week_plans` | week_start (unique), status (`draft` / `synced` / `changed`), ai_note, ai_run_id, calendar_synced_at. | @@ -211,13 +211,15 @@ automatic** (D-021). and `grounding_job_id` null. The job grounds the dinner's `recipe_url` directly with `GroundURL`, else its name with `Ground` (see Recipe grounding), and sets grounding_status `grounded` with the - recipe's recipe_id, photo_id, method, minutes_total and difficulty, or + recipe's recipe_id, photo_id, method, minutes_total and difficulty, + clearing the pasted `recipe_url` (the recipe replaces it), or `failed` with `grounding_error` — also the job's failure code — one of `no_recipe_found`, `recipe_unusable` (the pasted link), `search_not_configured`, `search_failed`, `ai_failed`, `interrupted` (the server stopped) or `grounding_error`. Each job carries the dinner's `grounding_revision` - from when it was enqueued; renaming a `pending` dinner clears its - `recipe_url`, raises the revision and enqueues a job for the new name. + from when it was enqueued; renaming a dinner raises the revision, and + renaming a `pending` one enqueues a job for the new name, clearing its + `recipe_url` when the normalized name changes. A job for a dinner archived, no longer `pending` or at another revision does no work, and a job only writes a dinner still unarchived, `pending` and at its revision, so a fix-up made meanwhile wins and an earlier job @@ -235,10 +237,19 @@ automatic** (D-021). searches for the dinner's name and returns `{candidates: [{candidate_id, title, site_name, photo_url, minutes}]}`, at most 3, ranked as `Ground` ranks them; it fetches pages but downloads no photo and runs no model, - so `photo_url` is set only for a page already stored, and `candidate_id` - is the page's URL. `POST /api/v1/dinners/{id}/ground` takes exactly one - of `{candidate_id}`, `{recipe_url}` (both grounded with `GroundURL`, - setting `grounded`) or `{manual: {ingredients[], steps[], minutes, + so `photo_url` is set only for a page already stored. `candidate_id` is + an opaque id the server resolves to the page it last offered that dinner + (kept in memory, so a restart means fetching the candidates again) at + the `grounding_revision` it was searched at, so a candidate offered + before any rename is refused even if the name is changed back; the + page's URL, which may carry a token, is never returned or cached. An + unknown dinner is 404, recipe search not set up 409 `not_configured` and + a search failure 502 `search_failed`; any other failure, such as a + stored recipe that cannot be read, is 500, logged with a fixed reason + and the dinner id only — never a candidate's URL or dependency error + text. + `POST /api/v1/dinners/{id}/ground` takes exactly one of `{candidate_id}`, + `{recipe_url}` (both grounded with `GroundURL`, setting `grounded`) or `{manual: {ingredients[], steps[], minutes, method}}` (1–100 ingredient lines, up to 100 steps, each non-blank and at most 500 characters; minutes 1–1440). A manual recipe is stored as typed — no model runs — as a `recipes` row whose source_url is @@ -587,11 +598,11 @@ Regular has a name `harness.Similar` to it — the PWA's copy of the name rules in `web/src/dinners/names.ts` folds letters with `web/src/dinners/letterFolds.json`, which `gen_folds.py` writes from the same table as the server's, and runs the same cases, -`internal/ai/harness/testdata/dedupe_cases.json`, as the server's) and +`web/src/dinners/testdata/dedupe_cases.json`, as the server's) and notifications (`/onboarding/notifications`: what MealQuest notifies about, -the iOS install hint, "Turn on notifications" and "Maybe later"). Either -button sends `PATCH /api/v1/settings/household` with -`{"onboarding_completed": true}`; the server sets `onboarding_completed_at` +the iOS install hint, "Turn on notifications" and "Maybe later"). "Maybe +later", and "Turn on notifications" once notifications are enabled, send +`PATCH /api/v1/settings/household` with `{"onboarding_completed": true}`; the server sets `onboarding_completed_at` to the time of that update (keeping the first time if it is already set; the field itself is read-only and `false` is refused) and the app goes Home. Completing needs at least three Regulars (`household.MinRegulars`); with @@ -600,8 +611,10 @@ fewer the server answers 400 `invalid_settings` naming fewer — by its URL, or after another member removes a Regular — so it then shows the server's reason with an "Add Regulars" button back to step 2. Notification permission is never asked for on page load; until Feature 19 -wires Web Push, "Turn on notifications" asks for nothing. Offline on the saved pages the -settings cannot be read (the service worker never caches `/api/v1/settings`), +wires Web Push, "Turn on notifications" asks for nothing and says plainly +that notifications are not on yet, offering "Finish setting up" (which +completes onboarding as above) instead of a silent success. Offline on the +saved pages the settings cannot be read (the service worker never caches `/api/v1/settings`), so the saved pages open as they are. Regular dinners (`/settings/regulars`) lists the Regulars from diff --git a/internal/ai/harness/dedupe_test.go b/internal/ai/harness/dedupe_test.go index 9f7ff2b..8653385 100644 --- a/internal/ai/harness/dedupe_test.go +++ b/internal/ai/harness/dedupe_test.go @@ -8,11 +8,11 @@ import ( "unicode/utf8" ) -// TestSharedDedupeCases runs testdata/dedupe_cases.json, which the PWA's -// copy of Normalize and Similar (web/src/dinners/names.ts) runs too, so the -// two cannot drift apart on these names. +// TestSharedDedupeCases runs web/src/dinners/testdata/dedupe_cases.json, +// which the PWA's copy of Normalize and Similar (web/src/dinners/names.ts) +// runs too, so the two cannot drift apart on these names. func TestSharedDedupeCases(t *testing.T) { - raw, err := os.ReadFile("testdata/dedupe_cases.json") + raw, err := os.ReadFile("../../../web/src/dinners/testdata/dedupe_cases.json") if err != nil { t.Fatal(err) } diff --git a/internal/db/dinners.sql.go b/internal/db/dinners.sql.go index e8bb637..986c0aa 100644 --- a/internal/db/dinners.sql.go +++ b/internal/db/dinners.sql.go @@ -347,6 +347,7 @@ func (q *Queries) GroundDinner(ctx context.Context, arg GroundDinnerParams) (Din const groundPendingDinner = `-- name: GroundPendingDinner :execrows UPDATE dinners SET recipe_id = ?1, + recipe_url = NULL, photo_id = ?2, method = COALESCE(method, ?3), minutes_total = COALESCE(minutes_total, ?4), @@ -375,7 +376,8 @@ type GroundPendingDinnerParams struct { // the grounding_revision it was grounded for; a dinner grounded some other // way, renamed or archived meanwhile is left as it is (0 rows). A method, // minutes_total or difficulty already set is kept: a pending dinner has -// them only when a member edited them while it was grounding. +// them only when a member edited them while it was grounding. Like +// GroundDinner it clears the pasted recipe_url: the recipe replaces the link. func (q *Queries) GroundPendingDinner(ctx context.Context, arg GroundPendingDinnerParams) (int64, error) { result, err := q.db.ExecContext(ctx, groundPendingDinner, arg.RecipeID, diff --git a/internal/db/querier.go b/internal/db/querier.go index 4765334..c9754c9 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -103,7 +103,8 @@ type Querier interface { // the grounding_revision it was grounded for; a dinner grounded some other // way, renamed or archived meanwhile is left as it is (0 rows). A method, // minutes_total or difficulty already set is kept: a pending dinner has - // them only when a member edited them while it was grounding. + // them only when a member edited them while it was grounding. Like + // GroundDinner it clears the pasted recipe_url: the recipe replaces the link. GroundPendingDinner(ctx context.Context, arg GroundPendingDinnerParams) (int64, error) // The pending suggestions whose dinner is still suggested, oldest first, // with their dinners. diff --git a/internal/dinners/dinners.go b/internal/dinners/dinners.go index f68c771..54f355c 100644 --- a/internal/dinners/dinners.go +++ b/internal/dinners/dinners.go @@ -7,6 +7,7 @@ package dinners import ( "context" + "crypto/rand" "database/sql" "errors" "fmt" @@ -142,6 +143,18 @@ type Service struct { // by dinner id, so archiving or renaming a dinner can stop them. groundingsMu sync.Mutex groundings map[string]map[*runningGrounding]struct{} + + // offerKey keys the candidate ids Candidates offers, so they are + // opaque and stable for this process. + offerKey []byte + // offersMu guards offers and offerSearches. offers holds, by dinner + // id, the candidates last offered by Candidates until they expire, so + // a candidate id is resolved here and its URL never leaves the server. + // offerSearches holds the Candidates searches running, by dinner id, + // so clearing a dinner's offers keeps them from storing stale ones. + offersMu sync.Mutex + offers map[string]*offerSet + offerSearches map[string]map[*offerSearch]struct{} } // NewService returns a service over st that grounds dinners with grounder @@ -158,7 +171,8 @@ func NewService(st *store.Store, grounder *recipes.Grounder, runner *jobs.Runner case logger == nil: return nil, errors.New("dinners: nil logger") } - s := &Service{st: st, grounder: grounder, logger: logger, groundings: make(map[string]map[*runningGrounding]struct{})} + s := &Service{st: st, grounder: grounder, logger: logger, groundings: make(map[string]map[*runningGrounding]struct{}), offerKey: make([]byte, 32), offers: make(map[string]*offerSet), offerSearches: make(map[string]map[*offerSearch]struct{})} + rand.Read(s.offerKey) // never fails: it crashes the program instead kind, err := jobs.Register(runner, KindGroundDinner, s.runGroundJob, Reasons...) if err != nil { return nil, err @@ -324,11 +338,14 @@ func (s *Service) listQuestLog(ctx context.Context) (rows []db.Dinner, err error // Update applies p to dinner id, recording updatedBy. An invalid patch is a // *ValidationError and a new name repeating another unarchived dinner a -// *DuplicateError; either changes nothing. Renaming a dinner still pending -// clears its recipe_url, raises its grounding_revision and enqueues a fresh -// KindGroundDinner job that grounds the new name: a job queued or running -// for the old name or link carries the old revision, so it can no longer -// write the dinner, and one running is stopped. +// *DuplicateError; either changes nothing. Renaming a dinner raises its +// grounding_revision and drops the candidates offered for its old name: +// each was offered at an earlier revision, so none of them grounds it. +// Renaming a dinner still pending also enqueues a fresh KindGroundDinner +// job that grounds the new name, and clears its recipe_url when the +// normalized name changes: a job queued or running for the old name or +// link carries the old revision, so it can no longer write the dinner, and +// one running is stopped. func (s *Service) Update(ctx context.Context, id string, p Patch, updatedBy string) (Dinner, error) { if updatedBy == "" { return Dinner{}, errors.New("dinners: update: no member to record as updated_by") @@ -339,7 +356,7 @@ func (s *Service) Update(ctx context.Context, id string, p Patch, updatedBy stri } var row db.Dinner - var regrounding bool + var renamed, regrounding bool err = s.st.WithTx(ctx, func(tx *sql.Tx) error { q := db.New(tx) cur, err := q.GetDinner(ctx, id) @@ -373,11 +390,13 @@ func (s *Service) Update(ctx context.Context, id string, p Patch, updatedBy stri } params.Name = name params.NormalizedName = harness.Normalize(name) + if name != cur.Name { + params.GroundingRevision++ + } if cur.GroundingStatus == GroundingPending && params.NormalizedName != cur.NormalizedName { // The link was the old dinner's recipe; the new name is // grounded by name, and a link can be given by hand. params.RecipeUrl = sql.NullString{} - params.GroundingRevision++ } } if p.Method != nil { @@ -393,12 +412,17 @@ func (s *Service) Update(ctx context.Context, id string, p Patch, updatedBy stri if err != nil { return fmt.Errorf("dinners: update %s: %w", id, err) } - regrounding = row.GroundingRevision != cur.GroundingRevision + renamed = row.GroundingRevision != cur.GroundingRevision + regrounding = renamed && cur.GroundingStatus == GroundingPending return nil }) if err != nil { return Dinner{}, err } + if renamed { + updateCommitted() + s.clearOffers(row.ID, row.GroundingRevision) + } if regrounding { s.stopGroundings(row.ID, row.GroundingRevision) jobID, err := s.enqueueGrounding(ctx, row.ID, row.GroundingRevision) @@ -417,7 +441,8 @@ func (s *Service) Update(ctx context.Context, id string, p Patch, updatedBy stri // Archive sets dinner id's status to archived, recording updatedBy. The row // is kept, so its name still counts for Side quest dedupe. A -// KindGroundDinner job running for it is stopped. +// KindGroundDinner job running for it is stopped, and its candidates no +// longer ground it. func (s *Service) Archive(ctx context.Context, id, updatedBy string) (Dinner, error) { if updatedBy == "" { return Dinner{}, errors.New("dinners: archive: no member to record as updated_by") @@ -434,6 +459,7 @@ func (s *Service) Archive(ctx context.Context, id, updatedBy string) (Dinner, er return Dinner{}, fmt.Errorf("dinners: archive %s: %w", id, err) } s.stopGroundings(row.ID, math.MaxInt64) + s.clearOffers(row.ID, math.MaxInt64) return fromRow(row), nil } diff --git a/internal/dinners/dinners_test.go b/internal/dinners/dinners_test.go index 620fdc6..764f6cb 100644 --- a/internal/dinners/dinners_test.go +++ b/internal/dinners/dinners_test.go @@ -16,8 +16,10 @@ import ( func TestCreateRegularNeverNamesFromLink(t *testing.T) { t.Parallel() e := newUnstartedEnv(t) - const link = "https://c.example/stew-key-abcdefghijklmno?sig=zyxwvut#frag" - leaks := []string{"c.example", "key", "abcdefghijklmno", "sig", "zyxwvut", "frag"} + const link = "https://c.example/stew-key-abcdefghijklmno?sig=zyxwvut#lookup" + // Each leak holds a character no ULID has (Crockford base32 has no I, L, + // O, U, "-", "." or "/"), so a dinner's id never matches one. + leaks := []string{link, "c.example", "stew-key", "abcdefghijklmno", "sig", "zyxwvut", "lookup"} check := func(what, text string) { t.Helper() lower := strings.ToLower(text) diff --git a/internal/dinners/export_test.go b/internal/dinners/export_test.go index ca06c60..b143035 100644 --- a/internal/dinners/export_test.go +++ b/internal/dinners/export_test.go @@ -14,6 +14,56 @@ func SetGroundTimeout(t *testing.T, d time.Duration) { t.Cleanup(func() { groundTimeout = old }) } +// SetOfferLifetime sets how long candidates ground a dinner until t ends. +// A test calling it must not run in parallel. +func SetOfferLifetime(t *testing.T, d time.Duration) { + old := offerLifetime + offerLifetime = d + t.Cleanup(func() { offerLifetime = old }) +} + +// SetUpdateCommitted runs f in Update once a rename is committed, before +// the dinner's offers are cleared, until t ends. A test calling it must +// not run in parallel. +func SetUpdateCommitted(t *testing.T, f func()) { + old := updateCommitted + updateCommitted = f + t.Cleanup(func() { updateCommitted = old }) +} + +// AdvanceClock moves the service's clock d ahead of real time until t +// ends, without firing any timer. A test calling it must not run in +// parallel. +func AdvanceClock(t *testing.T, d time.Duration) { + old := now + now = func() time.Time { return old().Add(d) } + t.Cleanup(func() { now = old }) +} + +// HasOffers reports whether the service holds candidates offered for +// dinner id. +func (s *Service) HasOffers(id string) bool { + s.offersMu.Lock() + defer s.offersMu.Unlock() + _, ok := s.offers[id] + return ok +} + +// StoreOffers stores urls as dinner id's offers, as a Candidates search +// that read the dinner at grounding_revision revision would on finishing. +func (s *Service) StoreOffers(id string, urls map[string]string, revision int64) { + keep, done := s.trackOffers(id) + defer done() + keep(urls, revision) +} + +// OfferCount is the number of dinners whose candidates the service holds. +func (s *Service) OfferCount() int { + s.offersMu.Lock() + defer s.offersMu.Unlock() + return len(s.offers) +} + // RunGroundJob runs the KindGroundDinner handler for p in the caller's // goroutine, as the job runner would, and returns the grounding status it // reports. diff --git a/internal/dinners/grounding.go b/internal/dinners/grounding.go index c978505..a09df3c 100644 --- a/internal/dinners/grounding.go +++ b/internal/dinners/grounding.go @@ -2,10 +2,14 @@ package dinners import ( "context" + "crypto/hmac" + "crypto/sha256" "database/sql" + "encoding/base64" "encoding/json" "errors" "fmt" + "math" "slices" "strings" "time" @@ -81,6 +85,11 @@ var ErrGroundTimeout = errors.New("dinners: grounding the recipe took too long") // logged. var ErrGroundFailed = errors.New("dinners: grounding the recipe failed") +// ErrCandidatesFailed wraps any unexpected failure finding a dinner's +// candidates. Its text may quote a candidate's URL and dependency errors, +// so it is never logged. +var ErrCandidatesFailed = errors.New("dinners: finding recipe candidates failed") + // ErrArchived is grounding an archived dinner by hand. var ErrArchived = errors.New("dinner archived") @@ -282,7 +291,9 @@ func reasonFor(err error, fromURL bool) string { // Candidate is a recipe page offered to ground a dinner in. type Candidate struct { - // CandidateID is the page's URL; Fix.CandidateID grounds it. + // CandidateID is an opaque id for the page, resolved by the server: + // Fix.CandidateID grounds it. The page's URL is never returned, as it + // may carry a token. CandidateID string `json:"candidate_id"` Title string `json:"title"` SiteName string `json:"site_name"` @@ -291,29 +302,182 @@ type Candidate struct { Minutes *int `json:"minutes"` } +// offerLifetime is how long the candidates Candidates offers a dinner +// ground it; tests shorten it. +var offerLifetime = 30 * time.Minute + +// now is the clock offer lifetimes are measured on; tests advance it. +var now = time.Now + +// updateCommitted runs in Update once a rename is committed, before the +// dinner's offers are cleared; tests use it to act in between. +var updateCommitted = func() {} + +// offerSet is the candidates last offered for a dinner: the page URL of +// each candidate id, found by searching the dinner's name at its +// grounding_revision revision. They ground the dinner until deadline, and +// only while it is still at that revision, which every rename raises; +// expiry drops the set from offers once deadline has passed. +type offerSet struct { + urls map[string]string + revision int64 + deadline time.Time + expiry *time.Timer +} + +// offerSearch is a Candidates search running for a dinner. It stores no +// offers found at a grounding_revision below below, which clearing the +// dinner's offers raises. +type offerSearch struct { + below int64 +} + // Candidates searches for dinner id's name and returns up to MaxCandidates -// recipe pages to ground it in, best first; the list may be empty. A -// search that fails wraps recipes.ErrSearchFailed. +// recipe pages to ground it in, best first; the list may be empty. They +// replace the dinner's earlier candidates: one not offered again no longer +// grounds it. They ground it for offerLifetime, and not at all once it is +// grounded by hand, renamed or archived, even while this search runs. A +// missing dinner is ErrNotFound; any other failure wraps +// ErrCandidatesFailed, and a search that fails also wraps +// recipes.ErrSearchFailed. func (s *Service) Candidates(ctx context.Context, id string) ([]Candidate, error) { + // Tracked before the dinner is read: grounding, renaming or archiving + // it after the read keeps this search from storing what it found. + keep, done := s.trackOffers(id) + defer done() row, err := s.get(ctx, id) - if err != nil { + if errors.Is(err, ErrNotFound) { return nil, err } + if err != nil { + return nil, fmt.Errorf("%w: dinner %s: %w", ErrCandidatesFailed, id, err) + } offers, err := s.grounder.Candidates(ctx, row.Name, row.Name, MaxCandidates) if err != nil { - return nil, err + return nil, fmt.Errorf("%w: dinner %s: %w", ErrCandidatesFailed, id, err) } out := make([]Candidate, len(offers)) + urls := make(map[string]string, len(offers)) for i, o := range offers { - out[i] = Candidate{CandidateID: o.URL, Title: o.Title, SiteName: o.SiteName, Minutes: o.Minutes} + cid := s.candidateID(row.ID, o.URL) + urls[cid] = o.URL + out[i] = Candidate{CandidateID: cid, Title: o.Title, SiteName: o.SiteName, Minutes: o.Minutes} if o.PhotoID != "" { u := PhotoURL(o.PhotoID) out[i].PhotoURL = &u } } + if row.Status != StatusArchived { + keep(urls, row.GroundingRevision) + } return out, nil } +// trackOffers registers a Candidates search for dinner id. It returns keep, +// which stores urls, found for the dinner at grounding_revision revision, +// as its offers for offerLifetime unless clearOffers has run for it since +// with a revision above that, or offers found at a later revision are +// stored, and done, to call when the search ends. +func (s *Service) trackOffers(id string) (keep func(urls map[string]string, revision int64), done func()) { + search := &offerSearch{} + s.offersMu.Lock() + if s.offerSearches[id] == nil { + s.offerSearches[id] = make(map[*offerSearch]struct{}) + } + s.offerSearches[id][search] = struct{}{} + s.offersMu.Unlock() + keep = func(urls map[string]string, revision int64) { + s.offersMu.Lock() + defer s.offersMu.Unlock() + if revision < search.below { + return + } + if set := s.offers[id]; set != nil && set.revision > revision { + // Found at a later revision, so the earlier search's offers + // are already stale. + return + } + s.dropOffers(id) + set := &offerSet{urls: urls, revision: revision, deadline: now().Add(offerLifetime)} + // expiry waits for offersMu, so it finds set stored even when it + // fires at once, and drops set only, never offers stored later. + set.expiry = time.AfterFunc(offerLifetime, func() { + s.offersMu.Lock() + defer s.offersMu.Unlock() + if s.offers[id] == set { + delete(s.offers, id) + } + }) + s.offers[id] = set + } + done = func() { + s.offersMu.Lock() + delete(s.offerSearches[id], search) + if len(s.offerSearches[id]) == 0 { + delete(s.offerSearches, id) + } + s.offersMu.Unlock() + } + return keep, done +} + +// clearOffers drops dinner id's offers found at a grounding_revision below +// below and keeps every Candidates search running for it from storing +// such offers, so none of them grounds it. Offers found at below or above +// are kept. Call it with math.MaxInt64 once the dinner is grounded by hand +// or archived, and with its new revision once it is renamed: a search +// that read the rename meanwhile keeps its offers. +func (s *Service) clearOffers(id string, below int64) { + s.offersMu.Lock() + defer s.offersMu.Unlock() + if set := s.offers[id]; set != nil && set.revision < below { + s.dropOffers(id) + } + for search := range s.offerSearches[id] { + search.below = max(search.below, below) + } +} + +// dropOffers drops dinner id's offers and stops their expiry. The caller +// holds offersMu. +func (s *Service) dropOffers(id string) { + if set := s.offers[id]; set != nil { + set.expiry.Stop() + delete(s.offers, id) + } +} + +// candidateID is the opaque id of the page at rawURL as a candidate for +// dinner id: the same page is offered under the same id until the service +// restarts, and the id reveals nothing of the URL. +func (s *Service) candidateID(id, rawURL string) string { + mac := hmac.New(sha256.New, s.offerKey) + mac.Write([]byte(id + "\x00" + rawURL)) + return base64.RawURLEncoding.EncodeToString(mac.Sum(nil)[:16]) +} + +// candidateURL is the page URL of candidate cid last offered for dinner d. +// A cid not among the dinner's live offers, or whose page is not a valid +// recipe link, is a *ValidationError. Offers past their deadline, or found +// at a grounding_revision other than d's, are not live, even before they +// are dropped: d was renamed since they were found, if only to its name +// then. +func (s *Service) candidateURL(d db.Dinner, cid string) (string, error) { + var raw string + var ok bool + s.offersMu.Lock() + if set := s.offers[d.ID]; set != nil && set.revision == d.GroundingRevision && now().Before(set.deadline) { + raw, ok = set.urls[cid] + } + s.offersMu.Unlock() + if ok { + if u, err := checkRecipeURL(raw); err == nil { + return u.String(), nil + } + } + return "", invalid("candidate_id", "is not a candidate from recipe-candidates; reload the candidates") +} + // Fix grounds a dinner by hand: exactly one of a candidate from // Candidates, a pasted recipe link or a recipe typed in. type Fix struct { @@ -367,6 +531,11 @@ func (s *Service) Ground(ctx context.Context, id string, f Fix, updatedBy string if cur.Status == StatusArchived { return Dinner{}, ErrArchived } + if f.CandidateID != nil { + if link, err = s.candidateURL(cur, *f.CandidateID); err != nil { + return Dinner{}, err + } + } groundCtx, cancel := context.WithTimeout(ctx, groundTimeout) defer cancel() gr, err := s.grounder.GroundURL(groundCtx, link) @@ -401,6 +570,7 @@ func (s *Service) Ground(ctx context.Context, id string, f Fix, updatedBy string if err != nil { return Dinner{}, fmt.Errorf("%w: dinner %s: record: %w", ErrGroundFailed, id, err) } + s.clearOffers(cur.ID, math.MaxInt64) return fromRow(row), nil } @@ -422,9 +592,10 @@ func (s *Service) groundRejected(ctx context.Context, id string) error { } // groundManual stores m as dinner id's recipe, with its ingredients and -// steps, and records it on the dinner, all in one transaction. A missing -// or archived dinner is ErrNotFound or ErrArchived; any other failure -// wraps ErrGroundFailed. +// steps, and records it on the dinner, all in one transaction; the +// dinner's candidates then no longer ground it. A missing or archived +// dinner is ErrNotFound or ErrArchived; any other failure wraps +// ErrGroundFailed. func (s *Service) groundManual(ctx context.Context, id string, m ManualRecipe, updatedBy string) (Dinner, error) { var row db.Dinner err := s.st.WithTx(ctx, func(tx *sql.Tx) error { @@ -522,6 +693,7 @@ func (s *Service) groundManual(ctx context.Context, id string, m ManualRecipe, u if err != nil { return Dinner{}, fmt.Errorf("%w: dinner %s: %w", ErrGroundFailed, id, err) } + s.clearOffers(row.ID, math.MaxInt64) return fromRow(row), nil } @@ -550,11 +722,8 @@ func (f Fix) validate() (link string, manual *ManualRecipe, err error) { } switch { case f.CandidateID != nil: - u, err := checkRecipeURL(*f.CandidateID) - if err != nil { - return "", nil, invalid("candidate_id", "is not a candidate from recipe-candidates") - } - return u.String(), nil, nil + // Ground resolves the candidate to its page. + return "", nil, nil case f.RecipeURL != nil: u, err := checkRecipeURL(*f.RecipeURL) if err != nil { diff --git a/internal/dinners/grounding_test.go b/internal/dinners/grounding_test.go index 5cff022..fa17cb2 100644 --- a/internal/dinners/grounding_test.go +++ b/internal/dinners/grounding_test.go @@ -9,6 +9,7 @@ import ( "io/fs" "log/slog" "slices" + "strings" "testing" "testing/fstest" "time" @@ -214,6 +215,39 @@ func TestCreateByRecipeURLGroundsThatURL(t *testing.T) { } } +// TestCreateByRecipeURLClearsLink: once background grounding resolves a +// pasted link, which may carry a personal token, the dinner no longer +// stores it; the recipe it grounded in records the page. +func TestCreateByRecipeURLClearsLink(t *testing.T) { + t.Parallel() + e := newEnv(t) + const link = "https://c.example/recipes/beef-stew?access_token=personal-token-abc123" + e.pages.Bodies[link] = recipestest.RecipePage("Beef Stew", "Stew Co", "https://c.example/stew.jpg", 150) + e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150)) + + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Beef stew"), RecipeURL: ptr(link)}, member) + if err != nil { + t.Fatal(err) + } + if job := e.waitJob(t, *c.GroundingJobID); job.Status != jobs.StatusSucceeded { + t.Fatalf("job = %+v", job) + } + row := e.dinner(t, c.ID) + if row.GroundingStatus != "grounded" || !row.RecipeID.Valid { + t.Fatalf("dinner = %+v, want grounded", row) + } + if row.RecipeUrl.Valid { + t.Errorf("recipe_url = %q after grounding, want cleared", row.RecipeUrl.String) + } + rec, err := db.New(e.st.Reader).GetRecipe(t.Context(), row.RecipeID.String) + if err != nil { + t.Fatal(err) + } + if rec.SourceUrl != link { + t.Errorf("recipe source = %q, want %q", rec.SourceUrl, link) + } +} + func TestCreateGroundingFailureRecordsReason(t *testing.T) { t.Parallel() tests := []struct { @@ -642,6 +676,26 @@ func failedDinner(t *testing.T, e *env, name string) string { return c.ID } +// candidateID returns the candidate_id Candidates offers dinner id for the +// page titled title. +func candidateID(t *testing.T, e *env, id, title string) string { + t.Helper() + got, err := e.svc.Candidates(t.Context(), id) + if err != nil { + t.Fatal(err) + } + for _, c := range got { + if c.Title == title { + if strings.Contains(c.CandidateID, "://") { + t.Fatalf("candidate_id = %q, want an opaque id, not the page URL", c.CandidateID) + } + return c.CandidateID + } + } + t.Fatalf("candidates = %+v, want one titled %q", got, title) + return "" +} + func TestCandidates(t *testing.T) { t.Parallel() e := newEnv(t) @@ -664,14 +718,25 @@ func TestCandidates(t *testing.T) { } first, second := got[0], got[1] wantPhoto := dinners.PhotoURL(e.dinner(t, c.ID).PhotoID.String) - if first.CandidateID != curryA || first.Title != "Chicken Curry" || first.SiteName != "Curry House" || + if first.Title != "Chicken Curry" || first.SiteName != "Curry House" || first.PhotoURL == nil || *first.PhotoURL != wantPhoto || first.Minutes == nil || *first.Minutes != 40 { t.Errorf("first = %+v, want the stored curryA with photo %s", first, wantPhoto) } - if second.CandidateID != curryB || second.Title != "Easy Weeknight Curry" || second.SiteName != "B Kitchen" || + if second.Title != "Easy Weeknight Curry" || second.SiteName != "B Kitchen" || second.PhotoURL != nil || second.Minutes == nil || *second.Minutes != 25 { t.Errorf("second = %+v, want curryB without a photo", second) } + if first.CandidateID == "" || first.CandidateID == second.CandidateID || + first.CandidateID == curryA || second.CandidateID == curryB { + t.Errorf("candidate ids = %q, %q, want distinct opaque ids", first.CandidateID, second.CandidateID) + } + again, err := e.svc.Candidates(t.Context(), c.ID) + if err != nil { + t.Fatal(err) + } + if len(again) != 2 || again[0].CandidateID != first.CandidateID || again[1].CandidateID != second.CandidateID { + t.Errorf("candidates again = %+v, want the same ids", again) + } if _, err := e.svc.Candidates(t.Context(), "nope"); !errors.Is(err, dinners.ErrNotFound) { t.Errorf("unknown dinner: err = %v, want ErrNotFound", err) @@ -702,9 +767,11 @@ func TestFixViaCandidate(t *testing.T) { t.Parallel() e := newEnv(t) id := failedDinner(t, e, "Mystery curry") + e.search.Results["Mystery curry"] = []recipes.Candidate{{URL: curryA}, {URL: curryB}} + cid := candidateID(t, e, id, "Easy Weeknight Curry") e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 25)) - d, err := e.svc.Ground(t.Context(), id, dinners.Fix{CandidateID: ptr(curryB)}, "member-2") + d, err := e.svc.Ground(t.Context(), id, dinners.Fix{CandidateID: ptr(cid)}, "member-2") if err != nil { t.Fatal(err) } @@ -721,6 +788,435 @@ func TestFixViaCandidate(t *testing.T) { } } +// wantStaleCandidate checks that candidate cid no longer grounds dinner +// id: Ground refuses it as invalid, and the dinner is unchanged. +func wantStaleCandidate(t *testing.T, e *env, id, cid string) { + t.Helper() + before := e.dinner(t, id) + _, err := e.svc.Ground(t.Context(), id, dinners.Fix{CandidateID: ptr(cid)}, member) + var invalid *dinners.ValidationError + if !errors.As(err, &invalid) { + t.Fatalf("stale candidate: err = %v, want *ValidationError", err) + } + if after := e.dinner(t, id); after != before { + t.Errorf("dinner changed by a stale candidate: %+v, was %+v", after, before) + } +} + +// TestGroundingOrArchivingClearsCandidates: once a dinner is grounded by +// hand — by candidate, link or manual recipe — renamed or archived, the +// service holds none of its candidates' URLs and none of them grounds it. +func TestGroundingOrArchivingClearsCandidates(t *testing.T) { + t.Parallel() + tests := []struct { + name string + // clear grounds or archives dinner id, offered candidate cid. + clear func(t *testing.T, e *env, id, cid string) + // archived says the dinner ends archived, which Ground refuses + // before it reads a candidate. + archived bool + }{ + {"candidate", func(t *testing.T, e *env, id, cid string) { + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + if _, err := e.svc.Ground(t.Context(), id, dinners.Fix{CandidateID: ptr(cid)}, member); err != nil { + t.Fatal(err) + } + }, false}, + {"link", func(t *testing.T, e *env, id, _ string) { + e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150)) + if _, err := e.svc.Ground(t.Context(), id, dinners.Fix{RecipeURL: ptr(stew)}, member); err != nil { + t.Fatal(err) + } + }, false}, + {"manual", func(t *testing.T, e *env, id, _ string) { + m := &dinners.ManualRecipe{Ingredients: []string{"1 chicken"}, Minutes: ptr(40), Method: ptr("stovetop")} + if _, err := e.svc.Ground(t.Context(), id, dinners.Fix{Manual: m}, member); err != nil { + t.Fatal(err) + } + }, false}, + {"rename", func(t *testing.T, e *env, id, _ string) { + if _, err := e.svc.Update(t.Context(), id, dinners.Patch{Name: ptr("Beef stew")}, member); err != nil { + t.Fatal(err) + } + }, false}, + {"archive", func(t *testing.T, e *env, id, _ string) { + if _, err := e.svc.Archive(t.Context(), id, member); err != nil { + t.Fatal(err) + } + }, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + e := newEnv(t) + id := failedDinner(t, e, "Mystery curry") + e.search.Results["Mystery curry"] = []recipes.Candidate{{URL: curryA}, {URL: curryB}} + cidA := candidateID(t, e, id, "Chicken Curry") + cidB := candidateID(t, e, id, "Easy Weeknight Curry") + + tt.clear(t, e, id, cidA) + if e.svc.HasOffers(id) { + t.Errorf("service still holds the candidates of dinner %s", id) + } + if !tt.archived { + wantStaleCandidate(t, e, id, cidB) + } + }) + } +} + +// TestRenamePendingRefusesOldCandidates: candidates offered for a pending +// dinner's old name no longer ground it once it is renamed — Ground refuses +// them and leaves the old recipe off it — while candidates for the new name +// do. +func TestRenamePendingRefusesOldCandidates(t *testing.T) { + t.Parallel() + // Unstarted, so the dinner stays pending with its jobs queued. + e := newUnstartedEnv(t) + e.search.Results["Beef stew"] = []recipes.Candidate{{URL: stew}} + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Chicken curry")}, member) + if err != nil { + t.Fatal(err) + } + old := candidateID(t, e, c.ID, "Chicken Curry") + + d, err := e.svc.Update(t.Context(), c.ID, dinners.Patch{Name: ptr("Beef stew")}, member) + if err != nil { + t.Fatal(err) + } + if d.GroundingStatus != dinners.GroundingPending { + t.Fatalf("renamed = %+v, want still pending", d) + } + if e.svc.HasOffers(c.ID) { + t.Errorf("service still holds the old name's candidates of dinner %s", c.ID) + } + wantStaleCandidate(t, e, c.ID, old) + if row := e.dinner(t, c.ID); row.RecipeID.Valid || row.GroundingStatus != dinners.GroundingPending { + t.Fatalf("dinner = %+v, want pending with no recipe", row) + } + + e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150)) + g, err := e.svc.Ground(t.Context(), c.ID, dinners.Fix{CandidateID: ptr(candidateID(t, e, c.ID, "Beef Stew"))}, member) + if err != nil { + t.Fatal(err) + } + if g.GroundingStatus != dinners.GroundingGrounded || g.RecipeID == nil { + t.Fatalf("dinner = %+v, want grounded for the new name", g) + } + rec, err := db.New(e.st.Reader).GetRecipe(t.Context(), *g.RecipeID) + if err != nil { + t.Fatal(err) + } + if rec.SourceUrl != stew { + t.Errorf("recipe source = %s, want %s", rec.SourceUrl, stew) + } +} + +// TestRenameSameNormalizedNameRefusesOldCandidates: a rename that leaves +// the normalized name as it was still drops the candidates offered before +// it, and none of them grounds the dinner. +func TestRenameSameNormalizedNameRefusesOldCandidates(t *testing.T) { + t.Parallel() + // Unstarted, so the dinner stays pending with its jobs queued. + e := newUnstartedEnv(t) + e.search.Results["Quick Chicken curry"] = e.search.Results["Chicken curry"] + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Chicken curry")}, member) + if err != nil { + t.Fatal(err) + } + old := candidateID(t, e, c.ID, "Chicken Curry") + before := e.dinner(t, c.ID) + + d, err := e.svc.Update(t.Context(), c.ID, dinners.Patch{Name: ptr("Quick Chicken curry")}, member) + if err != nil { + t.Fatal(err) + } + if e.svc.HasOffers(c.ID) { + t.Errorf("service still holds the candidates offered before dinner %s was renamed", c.ID) + } + // A reply is queued, so only refusing the candidate keeps its recipe off. + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + wantStaleCandidate(t, e, c.ID, old) + row := e.dinner(t, c.ID) + if row.RecipeID.Valid { + t.Fatalf("dinner = %+v, want no recipe", row) + } + if row.NormalizedName != before.NormalizedName { + t.Fatalf("normalized name = %q, want %q unchanged", row.NormalizedName, before.NormalizedName) + } + // Still pending, now at the rename's revision, with a job for it. + if d.GroundingStatus != dinners.GroundingPending || row.GroundingRevision != before.GroundingRevision+1 { + t.Fatalf("renamed = %+v, want pending at revision %d", row, before.GroundingRevision+1) + } + var queued int + if err := e.st.Reader.QueryRowContext(t.Context(), + `SELECT count(*) FROM jobs WHERE status = 'queued' AND json_extract(payload, '$.revision') = ?`, + row.GroundingRevision).Scan(&queued); err != nil { + t.Fatal(err) + } + if queued != 1 { + t.Fatalf("queued jobs at revision %d = %d, want 1", row.GroundingRevision, queued) + } +} + +// TestCandidatesBetweenRenameAndClearKept: a candidate search that reads +// a dinner after its rename commits, and stores its candidates before the +// rename clears the dinner's offers, keeps them: they ground the renamed +// dinner, while one offered before the rename does not, even when a search +// that read the dinner before the rename finishes after them. +func TestCandidatesBetweenRenameAndClearKept(t *testing.T) { + // Unstarted, so the dinner stays pending with its jobs queued. + e := newUnstartedEnv(t) + e.search.Results["Beef stew"] = []recipes.Candidate{{URL: stew}} + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Chicken curry")}, member) + if err != nil { + t.Fatal(err) + } + old := candidateID(t, e, c.ID, "Chicken Curry") + before := e.dinner(t, c.ID) + + var fresh string + dinners.SetUpdateCommitted(t, func() { + fresh = candidateID(t, e, c.ID, "Beef Stew") + e.svc.StoreOffers(c.ID, map[string]string{old: curryA}, before.GroundingRevision) + }) + if _, err := e.svc.Update(t.Context(), c.ID, dinners.Patch{Name: ptr("Beef stew")}, member); err != nil { + t.Fatal(err) + } + if fresh == "" { + t.Fatal("no candidate search ran between the rename and clearing offers") + } + if !e.svc.HasOffers(c.ID) { + t.Fatalf("clearing offers after the rename dropped the candidates found at its revision for dinner %s", c.ID) + } + wantStaleCandidate(t, e, c.ID, old) + + e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150)) + g, err := e.svc.Ground(t.Context(), c.ID, dinners.Fix{CandidateID: ptr(fresh)}, member) + if err != nil { + t.Fatal(err) + } + if g.GroundingStatus != dinners.GroundingGrounded || g.RecipeID == nil { + t.Fatalf("dinner = %+v, want grounded for the new name", g) + } + rec, err := db.New(e.st.Reader).GetRecipe(t.Context(), *g.RecipeID) + if err != nil { + t.Fatal(err) + } + if rec.SourceUrl != stew { + t.Errorf("recipe source = %s, want %s", rec.SourceUrl, stew) + } +} + +// TestRenameBackDuringCandidatesRefusesThem: a candidate search that read +// a dinner as A and finishes after renames A -> B -> A have committed, but +// before either has cleared the dinner's offers, stores candidates that +// do not ground it, though its name is A again. +func TestRenameBackDuringCandidatesRefusesThem(t *testing.T) { + e := newEnv(t) + id := failedDinner(t, e, "Mystery curry") + e.search.Results["Mystery curry"] = []recipes.Candidate{{URL: curryA}} + asked := len(e.search.Queries()) + gate := make(chan struct{}) + e.search.Gate = gate + + type result struct { + got []dinners.Candidate + err error + } + done := make(chan result, 1) + go func() { + got, err := e.svc.Candidates(t.Context(), id) + done <- result{got, err} + }() + deadline := time.Now().Add(10 * time.Second) + for len(e.search.Queries()) == asked { + if time.Now().After(deadline) { + t.Fatal("candidate search never started") + } + time.Sleep(5 * time.Millisecond) + } + + // The first rename's hook makes the second; the second's lets the + // search finish and tries its candidate, before either clears offers. + var hooks int + var stale error + var groundedBy *dinners.Dinner + dinners.SetUpdateCommitted(t, func() { + hooks++ + switch hooks { + case 1: + if _, err := e.svc.Update(t.Context(), id, dinners.Patch{Name: ptr("Mystery curry")}, member); err != nil { + t.Error(err) + } + case 2: + close(gate) + r := <-done + if r.err != nil || len(r.got) != 1 { + t.Errorf("candidates = %+v, %v; want curryA", r.got, r.err) + return + } + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + d, err := e.svc.Ground(t.Context(), id, dinners.Fix{CandidateID: ptr(r.got[0].CandidateID)}, member) + if err == nil { + groundedBy = &d + } + stale = err + } + }) + if _, err := e.svc.Update(t.Context(), id, dinners.Patch{Name: ptr("Beef stew")}, member); err != nil { + t.Fatal(err) + } + if hooks != 2 { + t.Fatalf("renames committed = %d, want 2", hooks) + } + var invalid *dinners.ValidationError + if !errors.As(stale, &invalid) { + t.Fatalf("stale candidate: err = %v, grounded %+v; want *ValidationError", stale, groundedBy) + } + row := e.dinner(t, id) + if row.Name != "Mystery curry" || row.RecipeID.Valid || row.GroundingStatus != dinners.GroundingFailed { + t.Errorf("dinner = %+v, want Mystery curry still failed with no recipe", row) + } + if e.svc.HasOffers(id) { + t.Errorf("service still holds the stale candidates of dinner %s", id) + } +} + +// TestCandidatesRunningWhenCleared: a candidate search still running when +// its dinner is grounded by hand, renamed or archived stores none of the +// candidates it returns, so none of them grounds the dinner. +func TestCandidatesRunningWhenCleared(t *testing.T) { + t.Parallel() + tests := []struct { + name string + clear func(t *testing.T, e *env, id string) + // archived says the dinner ends archived. + archived bool + }{ + {"manual", func(t *testing.T, e *env, id string) { + m := &dinners.ManualRecipe{Ingredients: []string{"1 chicken"}, Minutes: ptr(40), Method: ptr("stovetop")} + if _, err := e.svc.Ground(t.Context(), id, dinners.Fix{Manual: m}, member); err != nil { + t.Fatal(err) + } + }, false}, + {"rename", func(t *testing.T, e *env, id string) { + if _, err := e.svc.Update(t.Context(), id, dinners.Patch{Name: ptr("Beef stew")}, member); err != nil { + t.Fatal(err) + } + }, false}, + {"archive", func(t *testing.T, e *env, id string) { + if _, err := e.svc.Archive(t.Context(), id, member); err != nil { + t.Fatal(err) + } + }, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + e := newEnv(t) + id := failedDinner(t, e, "Mystery curry") + e.search.Results["Mystery curry"] = []recipes.Candidate{{URL: curryA}} + asked := len(e.search.Queries()) + gate := make(chan struct{}) + e.search.Gate = gate + + type result struct { + got []dinners.Candidate + err error + } + done := make(chan result, 1) + go func() { + got, err := e.svc.Candidates(t.Context(), id) + done <- result{got, err} + }() + deadline := time.Now().Add(10 * time.Second) + for len(e.search.Queries()) == asked { + if time.Now().After(deadline) { + t.Fatal("candidate search never started") + } + time.Sleep(5 * time.Millisecond) + } + tt.clear(t, e, id) + close(gate) + r := <-done + if r.err != nil { + t.Fatal(r.err) + } + if len(r.got) != 1 { + t.Fatalf("candidates = %+v, want curryA", r.got) + } + if e.svc.HasOffers(id) { + t.Errorf("a search finishing after dinner %s was cleared stored its candidates", id) + } + if !tt.archived { + wantStaleCandidate(t, e, id, r.got[0].CandidateID) + } + }) + } +} + +// TestCandidatesExpire: once their lifetime passes, a dinner's candidates +// are dropped and no longer ground it, with no other search running. +func TestCandidatesExpire(t *testing.T) { + dinners.SetOfferLifetime(t, 0) + e := newEnv(t) + id := failedDinner(t, e, "Mystery curry") + e.search.Results["Mystery curry"] = []recipes.Candidate{{URL: curryA}} + cid := candidateID(t, e, id, "Chicken Curry") + + waitNoOffers(t, e) + wantStaleCandidate(t, e, id, cid) +} + +// TestCandidatesExpireBeforeEviction: once their lifetime passes, a +// dinner's candidates no longer ground it, even while the service still +// holds them because their eviction has not run yet. +func TestCandidatesExpireBeforeEviction(t *testing.T) { + dinners.SetOfferLifetime(t, time.Hour) + e := newEnv(t) + id := failedDinner(t, e, "Mystery curry") + e.search.Results["Mystery curry"] = []recipes.Candidate{{URL: curryA}} + cid := candidateID(t, e, id, "Chicken Curry") + + dinners.AdvanceClock(t, time.Hour) + if !e.svc.HasOffers(id) { + t.Fatal("candidates evicted already, want them held until their timer fires") + } + wantStaleCandidate(t, e, id, cid) +} + +// TestExpiredCandidatesNotHeld: the service holds no dinner's candidates +// past their lifetime, however many dinners were offered some. +func TestExpiredCandidatesNotHeld(t *testing.T) { + dinners.SetOfferLifetime(t, 50*time.Millisecond) + e := newEnv(t) + names := map[string]string{"Mystery curry": curryA, "Other curry": curryB} + ids := make([]string, 0, len(names)) + for name, u := range names { + ids = append(ids, failedDinner(t, e, name)) + e.search.Results[name] = []recipes.Candidate{{URL: u}} + } + for _, id := range ids { + if _, err := e.svc.Candidates(t.Context(), id); err != nil { + t.Fatal(err) + } + } + waitNoOffers(t, e) +} + +// waitNoOffers waits for the service to hold no dinner's candidates. +func waitNoOffers(t *testing.T, e *env) { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for n := e.svc.OfferCount(); n > 0; n = e.svc.OfferCount() { + if time.Now().After(deadline) { + t.Fatalf("service still holds the expired candidates of %d dinners", n) + } + time.Sleep(5 * time.Millisecond) + } +} + // TestFixReplacesPendingLink: fixing a Regular added from a link that held // no recipe drops that link, so it no longer blocks adding the link again, // while the link of the recipe chosen does. @@ -729,12 +1225,18 @@ func TestFixReplacesPendingLink(t *testing.T) { const lost = "https://gone.example/recipes/lamb-tagine" tests := []struct { name string - fix dinners.Fix + // fix is the fix for dinner id. + fix func(t *testing.T, e *env, id string) dinners.Fix // linkTaken is the recipe link the fixed dinner now has, if any. linkTaken string }{ - {"candidate", dinners.Fix{CandidateID: ptr(curryB)}, curryB}, - {"manual", dinners.Fix{Manual: &dinners.ManualRecipe{Ingredients: []string{"1 lamb"}, Minutes: ptr(90), Method: ptr("oven")}}, ""}, + {"candidate", func(t *testing.T, e *env, id string) dinners.Fix { + e.search.Results["Weeknight dinner"] = []recipes.Candidate{{URL: curryB}} + return dinners.Fix{CandidateID: ptr(candidateID(t, e, id, "Easy Weeknight Curry"))} + }, curryB}, + {"manual", func(*testing.T, *env, string) dinners.Fix { + return dinners.Fix{Manual: &dinners.ManualRecipe{Ingredients: []string{"1 lamb"}, Minutes: ptr(90), Method: ptr("oven")}} + }, ""}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -752,7 +1254,7 @@ func TestFixReplacesPendingLink(t *testing.T) { t.Fatal(err) } e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 25)) - if _, err := e.svc.Ground(t.Context(), c.ID, tt.fix, member); err != nil { + if _, err := e.svc.Ground(t.Context(), c.ID, tt.fix(t, e, c.ID), member); err != nil { t.Fatal(err) } row, err := db.New(e.st.Reader).GetDinner(t.Context(), c.ID) @@ -855,7 +1357,8 @@ func TestFixValidation(t *testing.T) { tests := map[string]dinners.Fix{ "nothing": {}, "two": {CandidateID: ptr(curryA), RecipeURL: ptr(stew)}, - "candidate not URL": {CandidateID: ptr("cand-1")}, + "unknown candidate": {CandidateID: ptr("cand-1")}, + "candidate a URL": {CandidateID: ptr(curryA)}, "http link": {RecipeURL: ptr("http://a.example/x")}, "no ingredients": {Manual: &dinners.ManualRecipe{Minutes: ptr(20), Method: ptr("oven")}}, "blank ingredient": {Manual: &dinners.ManualRecipe{Ingredients: []string{" "}, Minutes: ptr(20), Method: ptr("oven")}}, diff --git a/internal/server/dinners.go b/internal/server/dinners.go index d78cb9d..fab52ac 100644 --- a/internal/server/dinners.go +++ b/internal/server/dinners.go @@ -205,7 +205,8 @@ func actorOrFail(w http.ResponseWriter, r *http.Request, logger *slog.Logger) (a // an archived dinner is 409 dinner_archived and one changed while grounding // 409 ground_conflict. Any other grounding failure, by candidate, link or // manual recipe, wraps dinners.ErrGroundFailed and is 500, logged by reason -// and dinner id only. +// and dinner id only, as is any other failure finding candidates, which +// wraps dinners.ErrCandidatesFailed. func writeDinnerError(w http.ResponseWriter, r *http.Request, logger *slog.Logger, action string, err error) { var invalid *dinners.ValidationError var dup *dinners.DuplicateError @@ -231,6 +232,12 @@ func writeDinnerError(w http.ResponseWriter, r *http.Request, logger *slog.Logge // logged. logger.ErrorContext(r.Context(), action, "reason", "ground_failed", "dinner_id", chi.URLParam(r, "id")) writeError(w, r, logger, http.StatusInternalServerError, "internal", "internal error") + case errors.Is(err, dinners.ErrCandidatesFailed): + // The error may quote a candidate's URL, which can carry a + // personal token, and dependency errors, so only the reason and + // dinner are logged. + logger.ErrorContext(r.Context(), action, "reason", "candidates_failed", "dinner_id", chi.URLParam(r, "id")) + writeError(w, r, logger, http.StatusInternalServerError, "internal", "internal error") case errors.Is(err, dinners.ErrArchived): writeError(w, r, logger, http.StatusConflict, "dinner_archived", "the dinner is archived") case errors.Is(err, dinners.ErrGroundConflict): diff --git a/internal/server/dinners_test.go b/internal/server/dinners_test.go index 2d7861b..e0c71a7 100644 --- a/internal/server/dinners_test.go +++ b/internal/server/dinners_test.go @@ -298,9 +298,11 @@ func TestDinnerResponsesHideTokenInLinkPath(t *testing.T) { if rec.Code != http.StatusConflict { t.Fatalf("same link again = %d, want 409: %s", rec.Code, rec.Body) } + // Each secret holds a character no ULID has (Crockford base32 has no I, + // L, O, U, "-", "." or "/"), so the dinner's id never matches one. for _, resp := range []struct{ name, body string }{{"create", created}, {"list", listed}, {"duplicate", rec.Body.String()}} { lower := strings.ToLower(resp.body) - for _, secret := range []string{"c.example", "recipes", "key", "abcdefghijklmno"} { + for _, secret := range []string{link, "c.example", "recipes/stew", "stew-key", "abcdefghijklmno"} { if strings.Contains(lower, secret) { t.Errorf("%s response contains %q: %s", resp.name, secret, resp.body) } diff --git a/internal/server/grounding_test.go b/internal/server/grounding_test.go index e7333d9..dfac21a 100644 --- a/internal/server/grounding_test.go +++ b/internal/server/grounding_test.go @@ -68,6 +68,27 @@ func listedDinner(t *testing.T, h http.Handler, id string) map[string]any { return nil } +// candidateID returns the candidate_id GET recipe-candidates offers dinner +// id for the page titled title. +func candidateID(t *testing.T, h http.Handler, id, title string) string { + t.Helper() + rec, body := callDinners(t, h, http.MethodGet, dinnersPath+"/"+id+"/recipe-candidates", "") + if rec.Code != http.StatusOK { + t.Fatalf("GET candidates = %d: %s", rec.Code, rec.Body) + } + for _, c := range body["candidates"].([]any) { + if c := c.(map[string]any); c["title"] == title { + cid := c["candidate_id"].(string) + if strings.Contains(cid, "://") { + t.Fatalf("candidate_id = %q, want an opaque id, not the page URL", cid) + } + return cid + } + } + t.Fatalf("candidates = %v, want one titled %q", body, title) + return "" +} + func TestPostDinnerReturnsBeforeGroundingCompletes(t *testing.T) { t.Parallel() e := newDinnersEnv(t).withRecipes(t) @@ -138,8 +159,11 @@ func TestGetRecipeCandidates(t *testing.T) { t.Fatalf("candidates = %v, want 2", body) } first := list[0].(map[string]any) + if cid, _ := first["candidate_id"].(string); cid == "" || strings.Contains(cid, "a.example") { + t.Errorf("candidate_id = %#v, want an opaque id", first["candidate_id"]) + } want := map[string]any{ - "candidate_id": curryPage, + "candidate_id": first["candidate_id"], "title": "Chicken Curry", "site_name": "Curry House", "photo_url": nil, @@ -180,13 +204,65 @@ func TestGetRecipeCandidatesSearchFails(t *testing.T) { } } +// TestGetRecipeCandidatesStoredLookupFailure: a candidate whose stored +// recipe cannot be read, or is stored incomplete, is a 500 logged by reason +// and dinner id, never with the candidate's URL, which can carry a personal +// token, or the database error. +func TestGetRecipeCandidatesStoredLookupFailure(t *testing.T) { + t.Parallel() + const ( + token = "personal-token-abc123" + host = "secret-candidate.example" + page = "https://" + host + "/curry?access_token=" + token + ) + for _, tc := range []struct { + name, sabotage string + leaks []string + }{ + // Renaming a column the lookup selects fails the read. + {"lookup", `ALTER TABLE recipes RENAME COLUMN yield TO yield_gone`, []string{"no such column", "yield"}}, + // A recipe stored without its photo, method or difficulty is + // incomplete. + {"incomplete", `INSERT INTO recipes (id, source_url, title, raw_jsonld, fetched_at, created_at, updated_at) + VALUES ('r-incomplete', '` + page + `', 'Curry', '{}', 'now', 'now', 'now')`, []string{"r-incomplete", "no photo"}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + e := newDinnersEnv(t) + e.search.Results["Chicken curry"] = []recipes.Candidate{{URL: page}} + id := createDinner(t, e.h, `{"name":"Chicken curry"}`) // runner not started: stays pending + if _, err := e.st.Writer.ExecContext(t.Context(), tc.sabotage); err != nil { + t.Fatal(err) + } + + rec, body := callDinners(t, e.h, http.MethodGet, dinnersPath+"/"+id+"/recipe-candidates", "") + if rec.Code != http.StatusInternalServerError || errorCode(body) != "internal" { + t.Errorf("GET = %d %s, want 500 internal", rec.Code, rec.Body) + } + logs := e.logs.String() + for _, secret := range append([]string{page, host, token, "access_token"}, tc.leaks...) { + if strings.Contains(rec.Body.String(), secret) { + t.Errorf("response contains %q: %s", secret, rec.Body) + } + if strings.Contains(logs, secret) { + t.Errorf("logs contain %q: %s", secret, logs) + } + } + if !strings.Contains(logs, "reason=candidates_failed") || !strings.Contains(logs, "dinner_id="+id) { + t.Errorf("logs = %q, want reason=candidates_failed and the dinner id", logs) + } + }) + } +} + func TestPostGroundViaCandidate(t *testing.T) { t.Parallel() e := newDinnersEnv(t).withRecipes(t) id := createDinner(t, e.h, `{"name":"Chicken curry"}`) e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 25)) - rec, d := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground", `{"candidate_id":"`+curry2Page+`"}`) + cid := candidateID(t, e.h, id, "Weeknight Curry") + rec, d := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground", `{"candidate_id":"`+cid+`"}`) if rec.Code != http.StatusOK { t.Fatalf("POST = %d: %s", rec.Code, rec.Body) } @@ -198,12 +274,81 @@ func TestPostGroundViaCandidate(t *testing.T) { // The candidate is now stored, so it is offered with its photo. _, body := callDinners(t, e.h, http.MethodGet, dinnersPath+"/"+id+"/recipe-candidates", "") for _, c := range body["candidates"].([]any) { - if c := c.(map[string]any); c["candidate_id"] == curry2Page && c["photo_url"] != photo { + if c := c.(map[string]any); c["candidate_id"] == cid && c["photo_url"] != photo { t.Errorf("grounded candidate photo_url = %v, want %s", c["photo_url"], photo) } } } +// TestRecipeCandidatesHideTokenURLs: a candidate page whose URL carries a +// token in its path or query is offered by an opaque id alone, which +// grounds the dinner in that page. +func TestRecipeCandidatesHideTokenURLs(t *testing.T) { + t.Parallel() + e := newDinnersEnv(t) + const ( + pathToken = "path-token-abc123" + queryToken = "query-token-def456" + pathPage = "https://secret-path.example/u/" + pathToken + "/chicken-curry" + queryPage = "https://secret-query.example/curry?access_token=" + queryToken + ) + e.search.Results["Chicken curry"] = []recipes.Candidate{{URL: pathPage}, {URL: queryPage}} + e.pages.Bodies[pathPage] = recipestest.RecipePage("Chicken Curry", "Curry House", "https://a.example/curry.jpg", 40) + e.pages.Bodies[queryPage] = recipestest.RecipePage("Weeknight Curry", "B Kitchen", "https://b.example/curry.jpg", 25) + e.images.JPEGs["https://b.example/curry.jpg"] = recipestest.JPEG(t, 400, 320, 2) + id := createDinner(t, e.h, `{"name":"Chicken curry"}`) // runner not started: stays pending + + rec, body := callDinners(t, e.h, http.MethodGet, dinnersPath+"/"+id+"/recipe-candidates", "") + if rec.Code != http.StatusOK { + t.Fatalf("GET = %d: %s", rec.Code, rec.Body) + } + if list, _ := body["candidates"].([]any); len(list) != 2 { + t.Fatalf("candidates = %v, want 2", body) + } + for _, secret := range []string{pathPage, queryPage, "secret-path.example", "secret-query.example", pathToken, queryToken, "access_token"} { + if strings.Contains(rec.Body.String(), secret) { + t.Errorf("candidates response contains %q: %s", secret, rec.Body) + } + } + + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 25)) + cid := candidateID(t, e.h, id, "Weeknight Curry") + rec, d := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground", `{"candidate_id":"`+cid+`"}`) + if rec.Code != http.StatusOK { + t.Fatalf("POST = %d: %s", rec.Code, rec.Body) + } + if strings.Contains(rec.Body.String(), queryToken) { + t.Errorf("ground response contains the token: %s", rec.Body) + } + var source string + if err := e.st.Reader.QueryRow(`SELECT source_url FROM recipes WHERE id = ?`, d["recipe_id"]).Scan(&source); err != nil { + t.Fatal(err) + } + if d["grounding_status"] != "grounded" || source != queryPage { + t.Errorf("dinner = %v in recipe %s, want grounded in %s", d, source, queryPage) + } +} + +// TestPostGroundRejectsCandidateNotOffered: a candidate_id is only one +// offered for that dinner; a page URL, or another dinner's candidate, is +// refused. +func TestPostGroundRejectsCandidateNotOffered(t *testing.T) { + t.Parallel() + e := newDinnersEnv(t).withRecipes(t) + id := createDinner(t, e.h, `{"name":"Chicken curry"}`) + other := createDinner(t, e.h, `{"name":"Pad thai"}`) + cid := candidateID(t, e.h, id, "Weeknight Curry") + for _, tc := range []struct{ name, dinner, cid string }{ + {"page URL", id, curry2Page}, + {"another dinner's", other, cid}, + } { + rec, body := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+tc.dinner+"/ground", `{"candidate_id":"`+tc.cid+`"}`) + if rec.Code != http.StatusBadRequest || errorCode(body) != "invalid_dinner" { + t.Errorf("%s: %d %v, want 400 invalid_dinner", tc.name, rec.Code, body) + } + } +} + func TestPostGroundViaRecipeURL(t *testing.T) { t.Parallel() e := newDinnersEnv(t).withRecipes(t) @@ -365,7 +510,8 @@ func TestPostGroundRejects(t *testing.T) { for _, tc := range []struct{ name, body, code string }{ {"empty", `{}`, "invalid_dinner"}, {"two kinds", `{"candidate_id":"https://a.example/x","recipe_url":"https://a.example/x"}`, "invalid_dinner"}, - {"candidate not a link", `{"candidate_id":"c1"}`, "invalid_dinner"}, + {"candidate not offered", `{"candidate_id":"c1"}`, "invalid_dinner"}, + {"candidate a page URL", `{"candidate_id":"https://a.example/x"}`, "invalid_dinner"}, {"http link", `{"recipe_url":"http://a.example/x"}`, "invalid_dinner"}, {"null", `{"recipe_url":null}`, "invalid_request"}, {"unknown field", `{"url":"https://a.example/x"}`, "invalid_request"}, diff --git a/migrations/queries/dinners.sql b/migrations/queries/dinners.sql index ecd95bd..98795bf 100644 --- a/migrations/queries/dinners.sql +++ b/migrations/queries/dinners.sql @@ -131,9 +131,11 @@ RETURNING *; -- the grounding_revision it was grounded for; a dinner grounded some other -- way, renamed or archived meanwhile is left as it is (0 rows). A method, -- minutes_total or difficulty already set is kept: a pending dinner has --- them only when a member edited them while it was grounding. +-- them only when a member edited them while it was grounding. Like +-- GroundDinner it clears the pasted recipe_url: the recipe replaces the link. UPDATE dinners SET recipe_id = sqlc.arg(recipe_id), + recipe_url = NULL, photo_id = sqlc.arg(photo_id), method = COALESCE(method, sqlc.arg(method)), minutes_total = COALESCE(minutes_total, sqlc.arg(minutes_total)), diff --git a/web/src/App.test.tsx b/web/src/App.test.tsx index ba314af..fb9df89 100644 --- a/web/src/App.test.tsx +++ b/web/src/App.test.tsx @@ -1,4 +1,4 @@ -import { act, render, screen } from '@testing-library/react' +import { act, cleanup, render, screen } from '@testing-library/react' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { createApiClient, createVerifiedSession } from './api/index.ts' import { createNetworkStatus } from './pwa/networkStatus.ts' @@ -250,6 +250,13 @@ describe('App offline bootstrap', () => { vi.spyOn(navigator, 'onLine', 'get').mockImplementation(() => onLine) }) + // A test that emits an offline event leaves TanStack Query's online + // manager, shared by every test, offline until an online event. + afterEach(() => { + cleanup() + window.dispatchEvent(new Event('online')) + }) + it.each([ ['/plan', 'Plan'], ['/dinner/01J9ZK', 'Dinner'], @@ -264,6 +271,31 @@ describe('App offline bootstrap', () => { expect(screen.queryByRole('alert')).not.toBeInTheDocument() }) + it('opens the saved page when the browser has emitted an offline event', async () => { + await visitOnline() + // /me is still loading, with the app mounted, when the browser emits + // its offline event; only then does /me fail. + let failMe!: () => void + vi.stubGlobal( + 'fetch', + vi.fn().mockImplementationOnce( + () => new Promise((_, reject) => (failMe = () => reject(new TypeError('Failed to fetch')))), + ).mockRejectedValue(new TypeError('Failed to fetch')), + ) + renderAt('/plan') + await vi.waitFor(() => expect(failMe).toBeDefined()) + onLine = false + act(() => { + window.dispatchEvent(new Event('offline')) + }) + act(() => failMe()) + + expect(await screen.findByRole('heading', { level: 1, name: 'Plan' })).toBeInTheDocument() + expect(screen.queryByRole('main', { name: 'Loading MealQuest' })).not.toBeInTheDocument() + expect(screen.getByRole('status')).toHaveTextContent('Offline — showing saved data') + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + }) + it('opens the saved pages when the server is unreachable but the browser reports online', async () => { await visitOnline() vi.stubGlobal('fetch', vi.fn().mockRejectedValue(new TypeError('Failed to fetch'))) diff --git a/web/src/dinners/names.test.ts b/web/src/dinners/names.test.ts index f2677fd..9ac8ac5 100644 --- a/web/src/dinners/names.test.ts +++ b/web/src/dinners/names.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import cases from '../../../internal/ai/harness/testdata/dedupe_cases.json' +import cases from './testdata/dedupe_cases.json' import { normalizeName, similarNames } from './names.ts' // The cases internal/ai/harness's tests run against Normalize and Similar. diff --git a/web/src/dinners/names.ts b/web/src/dinners/names.ts index 52cc50d..9967834 100644 --- a/web/src/dinners/names.ts +++ b/web/src/dinners/names.ts @@ -1,7 +1,7 @@ // The PWA's copy of the server's dinner-name rules (internal/ai/harness // Normalize and Similar), so a screen can tell beforehand that the dinners // API would refuse a name as a duplicate. The server still decides. -// internal/ai/harness/testdata/dedupe_cases.json is run against both. +// web/src/dinners/testdata/dedupe_cases.json is run against both. import folds from './letterFolds.json' diff --git a/internal/ai/harness/testdata/dedupe_cases.json b/web/src/dinners/testdata/dedupe_cases.json similarity index 100% rename from internal/ai/harness/testdata/dedupe_cases.json rename to web/src/dinners/testdata/dedupe_cases.json diff --git a/web/src/onboarding/NotificationsStep.test.tsx b/web/src/onboarding/NotificationsStep.test.tsx index e606aa9..f3f3632 100644 --- a/web/src/onboarding/NotificationsStep.test.tsx +++ b/web/src/onboarding/NotificationsStep.test.tsx @@ -180,10 +180,22 @@ describe('onboarding step 3', () => { expect(requestPermission).not.toHaveBeenCalled() }) - it('"Turn on notifications" finishes onboarding and goes Home while notifications are unavailable', async () => { + it('"Turn on notifications" says plainly that nothing was turned on while notifications are unavailable', async () => { const server = await renderApp() fireEvent.click(turnOn()) + expect(await screen.findByRole('status')).toHaveTextContent( + "Notifications aren't on yet: MealQuest can't send them until the notification feature ships, so nothing was turned on.", + ) + // Not a silent success: onboarding is not finished behind the household's back. + expect(server.held).toHaveLength(0) + expect(screen.getByRole('heading', { level: 1, name: 'Stay in the loop' })).toBeInTheDocument() + expect(screen.queryByRole('button', { name: 'Turn on notifications' })).not.toBeInTheDocument() + expect(screen.queryByRole('button', { name: 'Maybe later' })).not.toBeInTheDocument() + + const finish = screen.getByRole('button', { name: 'Finish setting up' }) + expectMinHitArea(finish) + fireEvent.click(finish) await vi.waitFor(() => expect(server.held).toHaveLength(1)) expect(server.held[0]!.body).toEqual({ onboarding_completed: true }) completed(server.held[0]!) @@ -193,6 +205,19 @@ describe('onboarding step 3', () => { expect(requestPermission).not.toHaveBeenCalled() }) + it('"Turn on notifications" says the browser blocked them when denied, then finishes on request', async () => { + const server = await renderStep(() => Promise.resolve('denied')) + fireEvent.click(turnOn()) + + expect(await screen.findByRole('status')).toHaveTextContent("Notifications aren't on: this browser blocked them.") + expect(server.held).toHaveLength(0) + + fireEvent.click(screen.getByRole('button', { name: 'Finish setting up' })) + await vi.waitFor(() => expect(server.held).toHaveLength(1)) + completed(server.held[0]!) + expect(await screen.findByRole('heading', { level: 1, name: 'Home' })).toBeInTheDocument() + }) + it('"Turn on notifications" asks the injected enableNotifications first, then finishes', async () => { let answer: (result: 'enabled') => void = () => {} const enable = vi.fn( diff --git a/web/src/onboarding/NotificationsStep.tsx b/web/src/onboarding/NotificationsStep.tsx index f17ddd1..4c43f7b 100644 --- a/web/src/onboarding/NotificationsStep.tsx +++ b/web/src/onboarding/NotificationsStep.tsx @@ -14,11 +14,21 @@ export type NotificationsResult = 'enabled' | 'denied' | 'unavailable' export type EnableNotifications = () => Promise // TODO(Feature 19): Web Push. Until it is wired, notifications cannot be -// turned on, and nothing asks the browser for permission. +// turned on, and nothing asks the browser for permission. This placeholder +// is the interim state: "Turn on notifications" answers 'unavailable', and +// the step tells the household plainly that nothing was turned on. export function notificationsUnavailable(): Promise { return Promise.resolve('unavailable') } +// notOnReasons says why notifications are not on after asking, for each +// result other than 'enabled'. +const notOnReasons: Record, string> = { + unavailable: + "Notifications aren't on yet: MealQuest can't send them until the notification feature ships, so nothing was turned on. You can finish setting up now.", + denied: "Notifications aren't on: this browser blocked them. You can allow them in the browser's settings later.", +} + const topics: { icon: LucideIcon; name: string; meaning: string }[] = [ { icon: CalendarDays, @@ -49,16 +59,19 @@ export type NotificationsStepProps = { } // NotificationsStep is onboarding step 3: what MealQuest notifies about, the -// iOS install hint, and finishing onboarding. Either button records that -// onboarding is finished and goes Home. When the server refuses because the -// household has fewer than three Regulars, the reason shows with a way back -// to step 2 to add them. +// iOS install hint, and finishing onboarding. "Maybe later", and "Turn on +// notifications" once they are enabled, record that onboarding is finished +// and go Home. When they are not enabled, the step says so and offers only +// "Finish setting up". When the server refuses because the household has +// fewer than three Regulars, the reason shows with a way back to step 2 to +// add them. export function NotificationsStep({ enableNotifications = notificationsUnavailable }: NotificationsStepProps) { const navigate = useNavigate() const complete = useCompleteOnboarding() const [enabling, setEnabling] = useState(false) const [problem, setProblem] = useState(null) const [fewRegulars, setFewRegulars] = useState(false) + const [notOn, setNotOn] = useState(null) const busy = enabling || complete.isPending async function finish() { @@ -80,14 +93,19 @@ export function NotificationsStep({ enableNotifications = notificationsUnavailab async function turnOn() { clearProblem() setEnabling(true) + let result: NotificationsResult try { - await enableNotifications() + result = await enableNotifications() } catch (error) { setProblem(`Could not turn on notifications: ${(error as Error).message}`) return } finally { setEnabling(false) } + if (result !== 'enabled') { + setNotOn(notOnReasons[result]) + return + } await finish() } @@ -101,14 +119,22 @@ export function NotificationsStep({ enableNotifications = notificationsUnavailab step={3} title="Stay in the loop" action={ -
- - -
+ notOn ? ( +
+ +
+ ) : ( +
+ + +
+ ) } >

MealQuest can let the household know when there's something to look at:

@@ -128,6 +154,11 @@ export function NotificationsStep({ enableNotifications = notificationsUnavailab
+ {notOn && ( +

+ {notOn} +

+ )} {problem && (

{problem} diff --git a/web/src/onboarding/Onboarding.module.css b/web/src/onboarding/Onboarding.module.css index ef2703c..e2ac957 100644 --- a/web/src/onboarding/Onboarding.module.css +++ b/web/src/onboarding/Onboarding.module.css @@ -229,6 +229,18 @@ color: var(--color-accent-700); } +/* Step 3: why notifications are not on after asking for them. */ +.notOn { + margin: var(--space-4) 0 0; + padding: var(--space-4); + border-radius: var(--radius-lg); + background: var(--color-surface); + font-family: var(--font-body); + font-size: 15px; + line-height: 1.4; + color: var(--color-text); +} + /* Step 3: back to step 2 when the server wants more Regulars. */ .recover { margin-top: var(--space-3); diff --git a/web/src/onboarding/OnboardingGate.tsx b/web/src/onboarding/OnboardingGate.tsx index 473b33d..1e5c853 100644 --- a/web/src/onboarding/OnboardingGate.tsx +++ b/web/src/onboarding/OnboardingGate.tsx @@ -19,7 +19,8 @@ function inOnboarding(pathname: string): boolean { // The service worker never caches the settings, so offline on the saved // pages (see AuthBootstrap) they cannot be read: the saved pages open as they // are, since onboarding needs the server anyway, and the settings are read -// again when the browser is back online. +// again when the browser is back online. The read runs even while the +// browser reports offline, so that case fails fast instead of waiting. export function OnboardingGate() { const api = useApi() const { pathname } = useLocation() diff --git a/web/src/settings/api.ts b/web/src/settings/api.ts index be27f9d..058caf0 100644 --- a/web/src/settings/api.ts +++ b/web/src/settings/api.ts @@ -367,12 +367,16 @@ function parseHouseholdSettings(where: string, body: unknown): HouseholdSettings export const HOUSEHOLD_SETTINGS_KEY = ['settings', 'household'] as const +// useHouseholdSettings runs even while the browser reports offline, like +// /me: OnboardingGate reads it before every page, so offline it must fail +// into the saved pages instead of waiting for the network. export function useHouseholdSettings() { const { apiFetch } = useApi() return useQuery({ queryKey: HOUSEHOLD_SETTINGS_KEY, queryFn: async () => parseHouseholdSettings(`GET ${HOUSEHOLD_SETTINGS_PATH}`, await apiFetch(HOUSEHOLD_SETTINGS_PATH)), + networkMode: 'always', }) } diff --git a/web/src/sw.test.ts b/web/src/sw.test.ts index 75e6d20..d2f7b01 100644 --- a/web/src/sw.test.ts +++ b/web/src/sw.test.ts @@ -15,12 +15,16 @@ describe('service worker activation', () => { vi.unstubAllGlobals() }) - it('deletes the v1 API cache and keeps the precache, photo and font caches', async () => { - // A household on the v1 worker: its API cache held dinners with the - // pasted recipe_url; its photos and fonts are not in the precache. + it('deletes the v1 and v2 API caches and keeps the current caches', async () => { + // A household that ran the v1 and v2 workers: the v1 API cache held + // dinners with the pasted recipe_url, the v2 one recipe candidates whose + // candidate_id was the full URL. Its photos and fonts are not in the + // precache, and the current API cache may already exist. const names = [ 'mealquest-precache-v1', 'mealquest-api-v1', + 'mealquest-api-v2', + 'mealquest-api-v3', 'mealquest-media-v1', 'mealquest-fonts-v1', 'other-app-cache', @@ -42,6 +46,7 @@ describe('service worker activation', () => { expect(pending).toHaveLength(1) expect(cacheStorage.delete).toHaveBeenCalledWith('mealquest-api-v1') - expect(cacheStorage.delete).toHaveBeenCalledTimes(1) + expect(cacheStorage.delete).toHaveBeenCalledWith('mealquest-api-v2') + expect(cacheStorage.delete).toHaveBeenCalledTimes(2) }) }) diff --git a/web/src/sw/routes.test.ts b/web/src/sw/routes.test.ts index 62685b3..e4857b8 100644 --- a/web/src/sw/routes.test.ts +++ b/web/src/sw/routes.test.ts @@ -147,12 +147,13 @@ describe('cache names', () => { for (const name of Object.values(cacheNames)) expect(name).toMatch(/^mealquest-/) }) - it('retire only the v1 API cache', () => { - // v1 API responses held dinners with the pasted recipe_url, which can - // carry a personal token; offline, the worker would still serve them. + it('retire only the v1 and v2 API caches', () => { + // v1 API responses held dinners with the pasted recipe_url and v2 ones + // recipe candidates whose candidate_id was the full URL; either can carry + // a personal token, and offline the worker would still serve them. // The precache, photos and fonts keep their v1 caches. expect(cacheNames).toEqual({ - api: 'mealquest-api-v2', + api: 'mealquest-api-v3', media: 'mealquest-media-v1', fonts: 'mealquest-fonts-v1', }) diff --git a/web/src/sw/routes.ts b/web/src/sw/routes.ts index e1f0ea3..c741660 100644 --- a/web/src/sw/routes.ts +++ b/web/src/sw/routes.ts @@ -29,9 +29,10 @@ export interface RouteRequest { // photos and fonts stay available offline. export const cacheVersions = { precache: 'v1', - // v2 retires the v1 API cache, whose dinner responses still carried the - // pasted recipe_url. - api: 'v2', + // v2 retired the v1 API cache, whose dinner responses still carried the + // pasted recipe_url; v3 retires the v2 API cache, whose recipe-candidate + // responses used the full URL as candidate_id. + api: 'v3', media: 'v1', fonts: 'v1', } as const