From e9e9087b488ea909e98a4fe79dd6959f535464ef Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 22:16:44 -0400 Subject: [PATCH] Hide candidate URLs, report unavailable notifications, clear grounded links Release corrections from the review of promotion pull request #98: - MQ-98-01: recipe-candidates returns an opaque candidate_id (an HMAC of the dinner and page under a per-process key) that the server resolves to the page it last offered that dinner. Candidate URLs, which may carry a token, no longer reach responses or the service-worker cache. - MQ-98-02: "Turn on notifications" no longer finishes onboarding silently when notifications are unavailable (or denied); the step says plainly that nothing was turned on and offers "Finish setting up". The placeholder stays until Feature 19 wires Web Push. - MQ-98-03: GroundPendingDinner clears the pasted recipe_url when background grounding succeeds, as manual grounding already did. Maestro task 6819ce33-3715-4bd4-9208-a54a8b5cee02. Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 23 ++-- internal/db/dinners.sql.go | 4 +- internal/db/querier.go | 3 +- internal/dinners/dinners.go | 13 ++- internal/dinners/grounding.go | 60 ++++++++-- internal/dinners/grounding_test.go | 90 +++++++++++++-- internal/frontend/dist/index.html | 10 +- internal/server/grounding_test.go | 103 +++++++++++++++++- migrations/queries/dinners.sql | 4 +- web/src/onboarding/NotificationsStep.test.tsx | 27 ++++- web/src/onboarding/NotificationsStep.tsx | 59 +++++++--- web/src/onboarding/Onboarding.module.css | 12 ++ 12 files changed, 356 insertions(+), 52 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 2c66c03..18a2509 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -209,7 +209,8 @@ 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 @@ -233,9 +234,11 @@ 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`, + 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); the + page's URL, which may carry a token, is never returned or cached. + `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 @@ -519,9 +522,9 @@ rules in `web/src/dinners/names.ts` folds letters with same table as the server's, and runs the same cases, `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 @@ -530,8 +533,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/db/dinners.sql.go b/internal/db/dinners.sql.go index 74c929d..2530b37 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 0a27a86..36966f8 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -80,7 +80,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) ListDinnersByStatus(ctx context.Context, status string) ([]Dinner, error) // The planned dinners dated from from_date up to but not including diff --git a/internal/dinners/dinners.go b/internal/dinners/dinners.go index ae2a20b..4f956a8 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" @@ -140,6 +141,15 @@ 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: by dinner id, the page URL of each candidate + // id last offered by Candidates, so a candidate id is resolved here + // and its URL never leaves the server. + offersMu sync.Mutex + offers map[string]map[string]string } // NewService returns a service over st that grounds dinners with grounder @@ -156,7 +166,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]map[string]string)} + 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 diff --git a/internal/dinners/grounding.go b/internal/dinners/grounding.go index c978505..ca668c1 100644 --- a/internal/dinners/grounding.go +++ b/internal/dinners/grounding.go @@ -2,7 +2,10 @@ package dinners import ( "context" + "crypto/hmac" + "crypto/sha256" "database/sql" + "encoding/base64" "encoding/json" "errors" "fmt" @@ -282,7 +285,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"` @@ -292,8 +297,9 @@ type Candidate struct { } // 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. A search that fails wraps recipes.ErrSearchFailed. func (s *Service) Candidates(ctx context.Context, id string) ([]Candidate, error) { row, err := s.get(ctx, id) if err != nil { @@ -304,16 +310,47 @@ func (s *Service) Candidates(ctx context.Context, id string) ([]Candidate, error return nil, 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 } } + s.offersMu.Lock() + s.offers[row.ID] = urls + s.offersMu.Unlock() return out, nil } +// 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 id. +// A cid not among the dinner's candidates last offered since the service +// started, or whose page is not a valid recipe link, is a +// *ValidationError. +func (s *Service) candidateURL(id, cid string) (string, error) { + s.offersMu.Lock() + raw, ok := s.offers[id][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 +404,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.ID, *f.CandidateID); err != nil { + return Dinner{}, err + } + } groundCtx, cancel := context.WithTimeout(ctx, groundTimeout) defer cancel() gr, err := s.grounder.GroundURL(groundCtx, link) @@ -401,6 +443,9 @@ 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.offersMu.Lock() + delete(s.offers, cur.ID) + s.offersMu.Unlock() return fromRow(row), nil } @@ -550,11 +595,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..ef2364e 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) } @@ -729,12 +796,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 +825,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 +928,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/frontend/dist/index.html b/internal/frontend/dist/index.html index 9f2b2eb..aacb365 100644 --- a/internal/frontend/dist/index.html +++ b/internal/frontend/dist/index.html @@ -2,10 +2,14 @@ + + + MealQuest - + + + - -

MealQuest web assets have not been built.

+
diff --git a/internal/server/grounding_test.go b/internal/server/grounding_test.go index e7333d9..1c29afc 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, @@ -186,7 +210,8 @@ func TestPostGroundViaCandidate(t *testing.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 +223,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 +459,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 43e96ae..d2cf5d9 100644 --- a/migrations/queries/dinners.sql +++ b/migrations/queries/dinners.sql @@ -118,9 +118,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/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);