diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 161102d..4da8dd8 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -179,12 +179,12 @@ automatic** (D-021). response holds the key. `Service.HarnessConfig` reads the settings for each skill run, so a saved change applies to the next run. - Dinners (`dinners.Service`). `POST /api/v1/dinners` adds a Regular from - exactly one of `{name}` or `{recipe_url}` (an https:// link, kept in - `recipe_url` for grounding) with status and origin `regular` and - grounding_status `pending`, answering 201 with the dinner. A link's - dinner is named from its last path segment (separators become spaces, - an extension and all-digit words are dropped: `…/12345/beef-stew/` is - "Beef stew"); a link that names nothing is 400 — add it by name. A name + `{name}` or `{name, recipe_url}` (an https:// link, kept in + `recipe_url` for grounding and never returned in a dinner response, as + it may carry a personal token) with status and origin `regular` and + grounding_status `pending`, answering 201 with the dinner. The name is + always the household's: no part of a link is ever used as a dinner's + name, and a link without a name is 400. A name is trimmed, at most 120 characters and must keep a word after `harness.Normalize`, which sets `normalized_name`. A name `harness.Similar` to an unarchived dinner's, or a link one already has diff --git a/internal/dinners/dinners.go b/internal/dinners/dinners.go index 605d675..ae2a20b 100644 --- a/internal/dinners/dinners.go +++ b/internal/dinners/dinners.go @@ -16,7 +16,6 @@ import ( "slices" "strings" "sync" - "unicode" "unicode/utf8" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/harness" @@ -55,7 +54,9 @@ var listable = []string{StatusRegular, StatusQuestLog} // ErrNotFound is returned for an unknown dinner id. var ErrNotFound = errors.New("dinner not found") -// Dinner is a dinner as the API shows it. +// Dinner is a dinner as the API shows it. It has no recipe_url: a pasted +// link may carry a personal token, so it stays in the database for +// grounding and never leaves the server. type Dinner struct { ID string `json:"id"` Name string `json:"name"` @@ -67,7 +68,6 @@ type Dinner struct { Cuisine *string `json:"cuisine"` IsFavorite bool `json:"is_favorite"` RecipeID *string `json:"recipe_id"` - RecipeURL *string `json:"recipe_url"` PhotoURL *string `json:"photo_url"` GroundingStatus string `json:"grounding_status"` // GroundingError is why grounding failed, one of the Reason codes; @@ -78,7 +78,10 @@ type Dinner struct { UpdatedAt string `json:"updated_at"` } -// NewRegular is a Regular to add: exactly one of Name and RecipeURL. +// NewRegular is a Regular to add: its Name, which the household always +// gives, and optionally the RecipeURL to ground it in. The name is never +// taken from the link: a pasted link may carry a personal token anywhere in +// it, and the name is shown in every dinner response. type NewRegular struct { Name *string RecipeURL *string @@ -170,10 +173,9 @@ type Created struct { GroundingJobID *string `json:"grounding_job_id"` } -// CreateRegular adds a Regular named by r.Name, or by the last path segment -// of r.RecipeURL (whose recipe grounding later replaces), with status and -// origin regular and grounding_status pending, recording updatedBy, and -// enqueues the KindGroundDinner job that grounds it. It returns without +// CreateRegular adds a Regular named r.Name, keeping r.RecipeURL for +// grounding when given, with status and origin regular and grounding_status +// pending, recording updatedBy, and enqueues the KindGroundDinner job that grounds it. It returns without // waiting for the job. A name repeating an unarchived dinner is a // *DuplicateError. When the job cannot be enqueued, the stored dinner is // returned with grounding_status failed (ReasonGroundingError) and no job @@ -223,34 +225,24 @@ func (s *Service) createRegular(ctx context.Context, r NewRegular, updatedBy str if updatedBy == "" { return db.Dinner{}, errors.New("dinners: create: no member to record as updated_by") } - var name string + if r.Name == nil { + return db.Dinner{}, invalid("name", "give the dinner's name, with or without a recipe_url") + } + name, err := checkName(*r.Name) + if err != nil { + return db.Dinner{}, err + } var recipeURL sql.NullString - switch { - case r.Name != nil && r.RecipeURL != nil: - return db.Dinner{}, invalid("name", "give a name or a recipe_url, not both") - case r.Name != nil: - n, err := checkName(*r.Name) - if err != nil { - return db.Dinner{}, err - } - name = n - case r.RecipeURL != nil: + if r.RecipeURL != nil { u, err := checkRecipeURL(*r.RecipeURL) if err != nil { return db.Dinner{}, err } - n, ok := nameFromURL(u) - if !ok { - return db.Dinner{}, invalid("recipe_url", "the link does not name a dinner; add it by name instead") - } - name = n recipeURL = sql.NullString{String: u.String(), Valid: true} - default: - return db.Dinner{}, invalid("name", "give a name or a recipe_url") } var row db.Dinner - err := s.st.WithTx(ctx, func(tx *sql.Tx) error { + err = s.st.WithTx(ctx, func(tx *sql.Tx) error { q := db.New(tx) if err := checkUnique(ctx, q, "", name, recipeURL); err != nil { return err @@ -495,36 +487,6 @@ func checkRecipeURL(raw string) (*url.URL, error) { return u, nil } -// nameFromURL names a dinner from the last path segment of u: separators -// become spaces, a file extension and words of only digits are dropped and -// the first letter is capitalized, so -// https://example.com/recipes/12345/chicken-tikka-masala/ is -// "Chicken tikka masala". ok is false when no usable name is left. -func nameFromURL(u *url.URL) (name string, ok bool) { - segments := strings.FieldsFunc(u.Path, func(r rune) bool { return r == '/' }) - if len(segments) == 0 { - return "", false - } - slug := segments[len(segments)-1] - if i := strings.LastIndexByte(slug, '.'); i > 0 { - slug = slug[:i] - } - var words []string - for _, w := range strings.FieldsFunc(slug, func(r rune) bool { return r == '-' || r == '_' || r == '+' || unicode.IsSpace(r) }) { - if strings.IndexFunc(w, func(r rune) bool { return !unicode.IsDigit(r) }) >= 0 { - words = append(words, strings.ToLower(w)) - } - } - name = strings.Join(words, " ") - if r, size := utf8.DecodeRuneInString(name); r != utf8.RuneError { - name = string(unicode.ToUpper(r)) + name[size:] - } - if _, err := checkName(name); err != nil { - return "", false - } - return name, true -} - // checkUnique returns a *DuplicateError when name is harness.Similar to an // unarchived dinner other than self, or recipeURL is one's recipe link: the // source_url of the recipe it is grounded in, else the recipe_url still @@ -565,7 +527,6 @@ func fromRow(r db.Dinner) Dinner { Cuisine: nullString(r.Cuisine), IsFavorite: r.IsFavorite == 1, RecipeID: nullString(r.RecipeID), - RecipeURL: nullString(r.RecipeUrl), GroundingStatus: r.GroundingStatus, GroundingError: nullString(r.GroundingError), UpdatedBy: nullString(r.UpdatedBy), diff --git a/internal/dinners/dinners_test.go b/internal/dinners/dinners_test.go index 9f8bd80..620fdc6 100644 --- a/internal/dinners/dinners_test.go +++ b/internal/dinners/dinners_test.go @@ -1,33 +1,74 @@ -package dinners +package dinners_test import ( - "net/url" + "encoding/json" + "errors" + "strings" "testing" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/dinners" ) -func TestNameFromURL(t *testing.T) { +// TestCreateRegularNeverNamesFromLink: a Regular added with a recipe link +// is named only by the household, so no part of the link — host, path, +// query or fragment — is stored as its name or shown in a dinner or a +// duplicate error, even a token of plain letters in the path. +func TestCreateRegularNeverNamesFromLink(t *testing.T) { t.Parallel() - tests := []struct { - link string - want string // "" means no usable name - }{ - {"https://example.com/recipes/12345/chicken-tikka-masala/", "Chicken tikka masala"}, - {"https://example.com/recipe/beef_stew.html", "Beef stew"}, - {"https://example.com/2024/05/best-ever-chili-123", "Best ever chili"}, - {"https://example.com/crème-brûlée", "Crème brûlée"}, - {"https://example.com/recipes/chili?utm=1#notes", "Chili"}, - {"https://example.com/", ""}, - {"https://example.com/recipe/98765", ""}, - {"https://example.com/the-best-easy-recipe", ""}, - } - for _, tt := range tests { - u, err := url.Parse(tt.link) - if err != nil { - t.Fatal(err) - } - got, ok := nameFromURL(u) - if got != tt.want || ok != (tt.want != "") { - t.Errorf("nameFromURL(%s) = %q, %v; want %q", tt.link, got, ok, tt.want) + e := newUnstartedEnv(t) + const link = "https://c.example/stew-key-abcdefghijklmno?sig=zyxwvut#frag" + leaks := []string{"c.example", "key", "abcdefghijklmno", "sig", "zyxwvut", "frag"} + check := func(what, text string) { + t.Helper() + lower := strings.ToLower(text) + for _, leak := range leaks { + if strings.Contains(lower, leak) { + t.Errorf("%s contains %q: %s", what, leak, text) + } } } + + _, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(link)}, member) + var verr *dinners.ValidationError + if !errors.As(err, &verr) { + t.Fatalf("link without a name: err = %v, want a *ValidationError", err) + } + check("validation error", verr.Error()) + + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr(" Sunday stew "), RecipeURL: ptr(link)}, member) + if err != nil { + t.Fatal(err) + } + if c.Name != "Sunday stew" { + t.Errorf("name = %q, want the household's %q", c.Name, "Sunday stew") + } + created, err := json.Marshal(c) + if err != nil { + t.Fatal(err) + } + check("create response", string(created)) + row := e.dinner(t, c.ID) + check("stored name", row.Name+" "+row.NormalizedName) + if row.RecipeUrl.String != link { + t.Errorf("stored recipe_url = %q, want %q kept for grounding", row.RecipeUrl.String, link) + } + + list, err := e.svc.List(t.Context(), dinners.StatusRegular) + if err != nil { + t.Fatal(err) + } + listed, err := json.Marshal(list) + if err != nil { + t.Fatal(err) + } + check("list response", string(listed)) + + // The same link under another name is still a duplicate, and its error + // names the existing dinner by the household's name. + _, err = e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Weeknight chili"), RecipeURL: ptr(link)}, member) + var dup *dinners.DuplicateError + if !errors.As(err, &dup) || dup.Existing.ID != c.ID { + t.Fatalf("same link again: err = %v, want a duplicate of %s", err, c.ID) + } + check("duplicate error", dup.Error()) } diff --git a/internal/dinners/grounding_test.go b/internal/dinners/grounding_test.go index 2578083..5cff022 100644 --- a/internal/dinners/grounding_test.go +++ b/internal/dinners/grounding_test.go @@ -194,7 +194,7 @@ func TestCreateByRecipeURLGroundsThatURL(t *testing.T) { e := newEnv(t) e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150)) - c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(stew)}, member) + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Beef stew"), RecipeURL: ptr(stew)}, member) if err != nil { t.Fatal(err) } @@ -223,7 +223,7 @@ func TestCreateGroundingFailureRecordsReason(t *testing.T) { reason string }{ {"no candidates", dinners.NewRegular{Name: ptr("Unknown dish")}, nil, dinners.ReasonNoRecipe}, - {"unusable link", dinners.NewRegular{RecipeURL: ptr("https://gone.example/recipes/lost-soup")}, nil, dinners.ReasonRecipeUnusable}, + {"unusable link", dinners.NewRegular{Name: ptr("Lost soup"), RecipeURL: ptr("https://gone.example/recipes/lost-soup")}, nil, dinners.ReasonRecipeUnusable}, {"search fails", dinners.NewRegular{Name: ptr("Chicken curry")}, func(e *env) { e.search.Err = recipes.ErrSearchRateLimited }, dinners.ReasonSearchFailed}, {"model fails", dinners.NewRegular{Name: ptr("Chicken curry")}, func(e *env) { // One provider failure per candidate tried. @@ -375,7 +375,7 @@ func TestRenameLinkWhilePendingGroundsNewName(t *testing.T) { e.search.Results["Beef stew"] = []recipes.Candidate{{URL: stew}} e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150)) - c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(curryA)}, member) + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Chicken curry"), RecipeURL: ptr(curryA)}, member) if err != nil { t.Fatal(err) } @@ -383,8 +383,11 @@ func TestRenameLinkWhilePendingGroundsNewName(t *testing.T) { if err != nil { t.Fatal(err) } - if d.GroundingStatus != dinners.GroundingPending || d.RecipeURL != nil { - t.Fatalf("renamed = %+v, want still pending with no recipe_url", d) + if d.GroundingStatus != dinners.GroundingPending { + t.Fatalf("renamed = %+v, want still pending", d) + } + if row, err := db.New(e.st.Reader).GetDinner(t.Context(), c.ID); err != nil || row.RecipeUrl.Valid { + t.Fatalf("renamed row = %+v (err %v), want no recipe_url", row, err) } e.start(t) @@ -737,7 +740,7 @@ func TestFixReplacesPendingLink(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() e := newEnv(t) - c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(lost)}, member) + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Lamb roast"), RecipeURL: ptr(lost)}, member) if err != nil { t.Fatal(err) } @@ -749,19 +752,22 @@ func TestFixReplacesPendingLink(t *testing.T) { t.Fatal(err) } e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 25)) - d, err := e.svc.Ground(t.Context(), c.ID, tt.fix, member) + if _, err := e.svc.Ground(t.Context(), c.ID, tt.fix, member); err != nil { + t.Fatal(err) + } + row, err := db.New(e.st.Reader).GetDinner(t.Context(), c.ID) if err != nil { t.Fatal(err) } - if d.RecipeURL != nil { - t.Errorf("recipe_url = %q after the fix, want the unusable link dropped", *d.RecipeURL) + if row.RecipeUrl.Valid { + t.Errorf("recipe_url = %q after the fix, want the unusable link dropped", row.RecipeUrl.String) } - if _, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(lost)}, member); err != nil { + if _, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Lentil soup"), RecipeURL: ptr(lost)}, member); err != nil { t.Errorf("adding the dropped link again: %v, want added", err) } if tt.linkTaken != "" { - _, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(tt.linkTaken)}, member) + _, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Curry night"), RecipeURL: ptr(tt.linkTaken)}, member) var dup *dinners.DuplicateError if !errors.As(err, &dup) || dup.Existing.ID != c.ID { t.Errorf("adding the fixed dinner's recipe link: err = %v, want a duplicate of %s", err, c.ID) diff --git a/internal/server/dinners.go b/internal/server/dinners.go index b6c627f..d78cb9d 100644 --- a/internal/server/dinners.go +++ b/internal/server/dinners.go @@ -19,7 +19,8 @@ const maxDinnerBody = 16 << 10 // manual recipe of dinners.MaxManualLines lines of each kind. const maxGroundBody = 512 << 10 -// dinnerCreate is the POST /dinners body: a name or a recipe_url. +// dinnerCreate is the POST /dinners body: a name, with or without a +// recipe_url. type dinnerCreate struct { Name *string `json:"name"` RecipeURL *string `json:"recipe_url"` diff --git a/internal/server/dinners_test.go b/internal/server/dinners_test.go index 402b90d..49e617c 100644 --- a/internal/server/dinners_test.go +++ b/internal/server/dinners_test.go @@ -159,7 +159,6 @@ func TestCreateDinnerByName(t *testing.T) { "origin": "regular", "grounding_status": "pending", "is_favorite": false, - "recipe_url": nil, "photo_url": nil, "method": nil, "minutes_total": nil, @@ -170,6 +169,9 @@ func TestCreateDinnerByName(t *testing.T) { t.Errorf("%s = %#v (present %v), want %#v", k, got, ok, v) } } + if got, ok := body["recipe_url"]; ok { + t.Errorf("recipe_url = %#v, want no recipe_url in a dinner response", got) + } var normalized, updatedBy string if err := st.Reader.QueryRow(`SELECT normalized_name, updated_by FROM dinners WHERE id = ?`, body["id"]).Scan(&normalized, &updatedBy); err != nil { t.Fatal(err) @@ -184,13 +186,12 @@ func TestCreateDinnerByURL(t *testing.T) { router, _ := newDinnersRouter(t) link := "https://www.example.com/recipes/12345/beef-and-broccoli-stir-fry/" - rec, body := callDinners(t, router, http.MethodPost, dinnersPath, `{"recipe_url":"`+link+`"}`) + rec, body := callDinners(t, router, http.MethodPost, dinnersPath, `{"name":"Stir fry night","recipe_url":"`+link+`"}`) if rec.Code != http.StatusCreated { t.Fatalf("status = %d, want 201: %s", rec.Code, rec.Body) } for k, v := range map[string]any{ - "name": "Beef and broccoli stir fry", - "recipe_url": link, + "name": "Stir fry night", "status": "regular", "origin": "regular", "grounding_status": "pending", @@ -200,24 +201,27 @@ func TestCreateDinnerByURL(t *testing.T) { t.Errorf("%s = %#v, want %#v", k, body[k], v) } } + // The link stays on the server for grounding; responses never carry it. + if got, ok := body["recipe_url"]; ok { + t.Errorf("recipe_url = %#v, want no recipe_url in a dinner response", got) + } // The same link again is a duplicate even though its name would differ. rec, body = callDinners(t, router, http.MethodPost, dinnersPath, `{"name":"Something else entirely"}`) if rec.Code != http.StatusCreated { t.Fatalf("unrelated name = %d: %s", rec.Code, rec.Body) } - rec, body = callDinners(t, router, http.MethodPost, dinnersPath, `{"recipe_url":"`+link+`"}`) + rec, body = callDinners(t, router, http.MethodPost, dinnersPath, `{"name":"Beef and broccoli","recipe_url":"`+link+`"}`) if rec.Code != http.StatusConflict || errorCode(body) != "duplicate_dinner" { t.Errorf("same link again = %d %v, want 409 duplicate_dinner", rec.Code, body) } for _, tc := range []struct{ name, body string }{ - {"plain http", `{"recipe_url":"http://example.com/recipes/chili"}`}, - {"not a URL", `{"recipe_url":"chili"}`}, - {"credentials", `{"recipe_url":"https://me:pw@example.com/recipes/chili"}`}, - {"no name in path", `{"recipe_url":"https://example.com/"}`}, - {"numeric slug", `{"recipe_url":"https://example.com/recipe/98765"}`}, - {"both fields", `{"name":"Chili","recipe_url":"https://example.com/recipes/chili"}`}, + {"plain http", `{"name":"Chili","recipe_url":"http://example.com/recipes/chili"}`}, + {"not a URL", `{"name":"Chili","recipe_url":"chili"}`}, + {"credentials", `{"name":"Chili","recipe_url":"https://me:pw@example.com/recipes/chili"}`}, + {"link without a name", `{"recipe_url":"https://example.com/recipes/chili"}`}, + {"filler name with a link", `{"name":"The Best Easy Recipe","recipe_url":"https://example.com/recipes/chili"}`}, {"neither field", `{}`}, } { rec, body := callDinners(t, router, http.MethodPost, dinnersPath, tc.body) @@ -227,6 +231,77 @@ func TestCreateDinnerByURL(t *testing.T) { } } +// TestDinnerResponsesHideRecipeURL: a pasted link may carry a personal +// token, so neither the create response nor a later list read shows any +// part of it, while the stored link still awaits grounding. +func TestDinnerResponsesHideRecipeURL(t *testing.T) { + t.Parallel() + router, st := newDinnersRouter(t) + + const link = "https://c.example/recipes/stew?access_token=personal-token-abc123" + rec, body := callDinners(t, router, http.MethodPost, dinnersPath, `{"name":"Stew","recipe_url":"`+link+`"}`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201: %s", rec.Code, rec.Body) + } + created := rec.Body.String() + rec, _ = callDinners(t, router, http.MethodGet, dinnersPath+"?status=regular", "") + if rec.Code != http.StatusOK { + t.Fatalf("GET status=regular = %d: %s", rec.Code, rec.Body) + } + for _, resp := range []struct{ name, body string }{{"create", created}, {"list", rec.Body.String()}} { + for _, secret := range []string{link, "c.example", "access_token", "personal-token-abc123", "recipe_url"} { + if strings.Contains(resp.body, secret) { + t.Errorf("%s response contains %q: %s", resp.name, secret, resp.body) + } + } + } + + var stored string + if err := st.Reader.QueryRow(`SELECT recipe_url FROM dinners WHERE id = ?`, body["id"]).Scan(&stored); err != nil { + t.Fatal(err) + } + if stored != link { + t.Errorf("stored recipe_url = %q, want %q kept for grounding", stored, link) + } +} + +// TestDinnerResponsesHideTokenInLinkPath: a personal token can sit +// anywhere in a link's path, even as plain letters, so a dinner added from +// a link is named only by the household and neither the create response, a +// later list read nor the duplicate error for the same link shows any of +// the link. +func TestDinnerResponsesHideTokenInLinkPath(t *testing.T) { + t.Parallel() + router, _ := newDinnersRouter(t) + + const link = "https://c.example/recipes/stew-key-abcdefghijklmno" + rec, body := callDinners(t, router, http.MethodPost, dinnersPath, `{"name":"Sunday stew","recipe_url":"`+link+`"}`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201: %s", rec.Code, rec.Body) + } + if body["name"] != "Sunday stew" { + t.Errorf("name = %v, want Sunday stew", body["name"]) + } + created := rec.Body.String() + rec, _ = callDinners(t, router, http.MethodGet, dinnersPath+"?status=regular", "") + if rec.Code != http.StatusOK { + t.Fatalf("GET status=regular = %d: %s", rec.Code, rec.Body) + } + listed := rec.Body.String() + rec, _ = callDinners(t, router, http.MethodPost, dinnersPath, `{"name":"Beef stew","recipe_url":"`+link+`"}`) + if rec.Code != http.StatusConflict { + t.Fatalf("same link again = %d, want 409: %s", rec.Code, rec.Body) + } + 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"} { + if strings.Contains(lower, secret) { + t.Errorf("%s response contains %q: %s", resp.name, secret, resp.body) + } + } + } +} + func TestCreateDinnerDuplicate(t *testing.T) { t.Parallel() router, st := newDinnersRouter(t) @@ -242,10 +317,10 @@ func TestCreateDinnerDuplicate(t *testing.T) { t.Errorf("%q: %d %v, want 409 duplicate_dinner", name, rec.Code, body) } } - // A slug naming the same dish is a duplicate too. - rec, body := callDinners(t, router, http.MethodPost, dinnersPath, `{"recipe_url":"https://example.com/easy-chicken-tikka-masala.html"}`) + // A name repeating the dish is a duplicate with a link too. + rec, body := callDinners(t, router, http.MethodPost, dinnersPath, `{"name":"Chicken tikka masala","recipe_url":"https://example.com/other-curry.html"}`) if rec.Code != http.StatusConflict { - t.Errorf("URL slug duplicate = %d %v, want 409", rec.Code, body) + t.Errorf("named duplicate with a link = %d %v, want 409", rec.Code, body) } var n int diff --git a/web/src/dinners/api.ts b/web/src/dinners/api.ts index b0a39e5..657d1de 100644 --- a/web/src/dinners/api.ts +++ b/web/src/dinners/api.ts @@ -33,8 +33,10 @@ export type Regular = { pending?: true } -// NewRegular is a POST /api/v1/dinners body: a name or a recipe link. -export type NewRegular = { name: string } | { recipe_url: string } +// NewRegular is a POST /api/v1/dinners body: the household's name for the +// dinner, with the recipe link to ground it in when one was pasted. A +// dinner is never named from its link, which may carry private details. +export type NewRegular = { name: string; recipe_url?: string } // RegularPatch is a PATCH /api/v1/dinners/{id} body; every field is optional. export type RegularPatch = Partial<{ name: string; method: Method; minutes_total: number; difficulty: Difficulty }> @@ -195,10 +197,9 @@ function settle(queryClient: QueryClient) { let pendingIds = 0 -// useAddRegular adds a Regular by name or recipe link (POST -// /api/v1/dinners). It shows in the list at once — a link under the link -// itself until the server names it — and is taken out again if the server -// refuses it. +// useAddRegular adds a Regular by name, with or without a recipe link (POST +// /api/v1/dinners). It shows in the list at once under its name and is +// taken out again if the server refuses it. export function useAddRegular() { const { apiFetch } = useApi() const queryClient = useQueryClient() @@ -210,7 +211,7 @@ export function useAddRegular() { const id = `pending-${++pendingIds}` const optimistic: CachedRegular = { id, - name: 'name' in add ? add.name : add.recipe_url, + name: add.name, method: null, minutes_total: null, difficulty: null, diff --git a/web/src/settings/RegularsSettingsPage.module.css b/web/src/settings/RegularsSettingsPage.module.css index e952791..0954766 100644 --- a/web/src/settings/RegularsSettingsPage.module.css +++ b/web/src/settings/RegularsSettingsPage.module.css @@ -43,6 +43,13 @@ font-weight: 700; } +/* The add form: the add row, then the name field a pasted link needs. */ +.addForm { + display: flex; + flex-direction: column; + gap: 8px; +} + /* The add field: the input fills the row beside the grape + button. */ .add { display: flex; diff --git a/web/src/settings/RegularsSettingsPage.test.tsx b/web/src/settings/RegularsSettingsPage.test.tsx index ba7371c..afae963 100644 --- a/web/src/settings/RegularsSettingsPage.test.tsx +++ b/web/src/settings/RegularsSettingsPage.test.tsx @@ -192,21 +192,33 @@ describe('RegularsSettingsPage', () => { expect(screen.getByRole('heading', { level: 2, name: 'On rotation · 2' })).toBeInTheDocument() }) - it('adds a Regular from a pasted recipe link', async () => { - const link = 'https://example.com/recipes/12345/beef-stew/' + it('adds a Regular from a pasted recipe link under the name the household gives it', async () => { + const link = 'https://example.com/recipes/stew-key-abcdefghijklmno' const server = stubServer([], { - 'POST /api/v1/dinners': () => { - const created = dinnerBody('d9', 'Beef stew', { grounding_status: 'pending', recipe_url: link }) + 'POST /api/v1/dinners': (body) => { + const created = dinnerBody('d9', (body as { name: string }).name, { grounding_status: 'pending' }) server.regulars = [created] return jsonResponse(201, { ...created, grounding_job_id: 'job-1' }) }, }) await renderPage() - add(link) - expect(await screen.findByText('Beef stew')).toBeInTheDocument() - expect(sent(server.fetchMock)).toEqual([{ request: 'POST /api/v1/dinners', body: { recipe_url: link } }]) - expect(within(row('Beef stew')).getByText('Finding recipe…')).toBeInTheDocument() + fireEvent.change(addField(), { target: { value: ` ${link} ` } }) + // A link alone cannot be added: it never names a dinner. + expect(screen.getByRole('button', { name: 'Add Regular' })).toBeDisabled() + const name = screen.getByRole('textbox', { name: 'Dinner name' }) + expect(name).toHaveAccessibleDescription(/never names a dinner/) + fireEvent.change(name, { target: { value: ' Sunday stew ' } }) + fireEvent.click(screen.getByRole('button', { name: 'Add Regular' })) + + expect(addField()).toHaveValue('') + expect(screen.queryByRole('textbox', { name: 'Dinner name' })).not.toBeInTheDocument() + expect(await screen.findByText('Sunday stew')).toBeInTheDocument() + expect(sent(server.fetchMock)).toEqual([ + { request: 'POST /api/v1/dinners', body: { name: 'Sunday stew', recipe_url: link } }, + ]) + expect(within(row('Sunday stew')).getByText('Finding recipe…')).toBeInTheDocument() + expect(document.body).not.toHaveTextContent(/example\.com|abcdefghijklmno|key/i) }) it('takes back an add the server refuses and says why', async () => { diff --git a/web/src/settings/RegularsSettingsPage.tsx b/web/src/settings/RegularsSettingsPage.tsx index 6464606..c816b49 100644 --- a/web/src/settings/RegularsSettingsPage.tsx +++ b/web/src/settings/RegularsSettingsPage.tsx @@ -49,12 +49,14 @@ export const methodLabels: Record = { const difficultyLabels: Record = { easy: 'Easy', medium: 'Medium', hard: 'Hard' } -// A pasted link is added as a recipe link; anything else is a dinner name. +// A pasted link is added as a recipe link under a name the household gives; +// anything else is a dinner name. const LINK = /^https?:\/\//i // Settings · Regular dinners: the household's Regulars, each with its photo, // method and minutes, favorite heart and whether its recipe was found. The -// field at the top adds one by name or pasted recipe link; tapping one +// field at the top adds one by name, or by pasted recipe link with a name +// the household gives it; tapping one // edits or removes it, or fixes its recipe when none was found. export function RegularsSettingsPage() { const navigate = useNavigate() @@ -85,19 +87,24 @@ function RegularsManager() { const favorite = useToggleFavorite() const { refusals, track } = useRefusals() const [draft, setDraft] = useState('') + const [linkName, setLinkName] = useState('') const [open, setOpen] = useState(null) const headingId = useId() + const linkHintId = useId() + const isLink = LINK.test(draft.trim()) + const ready = draft.trim() !== '' && (!isLink || linkName.trim() !== '') const list = regulars.data ?? [] // A Regular removed meanwhile closes its sheet. const opened = open && list.find((regular) => regular.id === open.id && !regular.pending) function onSubmit(event: FormEvent) { event.preventDefault() + if (!ready) return const text = draft.trim() - if (text === '') return - const body: NewRegular = LINK.test(text) ? { recipe_url: text } : { name: text } - track(`add:${text.toLowerCase()}`, `add ${text}`, add.mutateAsync(body)) + const body: NewRegular = isLink ? { name: linkName.trim(), recipe_url: text } : { name: text } + track(`add:${body.name.toLowerCase()}`, `add ${body.name}`, add.mutateAsync(body)) setDraft('') + setLinkName('') } function onRemove(regular: Regular) { @@ -111,20 +118,38 @@ function RegularsManager() { return ( <> -
- setDraft(event.target.value)} - /> - - - + +
+ setDraft(event.target.value)} + /> + + + +
+ {isLink && ( + <> + setLinkName(event.target.value)} + /> +

+ Name it yourself: a link can carry private details, so it never names a dinner. +

+ + )} {refusals.map((refusal) => (

diff --git a/web/src/sw.test.ts b/web/src/sw.test.ts new file mode 100644 index 0000000..75e6d20 --- /dev/null +++ b/web/src/sw.test.ts @@ -0,0 +1,47 @@ +// Runs the service worker's activate handler against mocked Cache Storage, +// with the Workbox precache and router stubbed out. tsconfig.sw.json checks +// this file with the worker's types, like src/sw.ts itself. +import { afterEach, describe, expect, it, vi } from 'vitest' + +vi.mock('workbox-precaching', () => ({ + PrecacheFallbackPlugin: class {}, + matchPrecache: vi.fn(), + precacheAndRoute: vi.fn(), +})) +vi.mock('workbox-routing', () => ({ registerRoute: vi.fn() })) + +describe('service worker activation', () => { + afterEach(() => { + 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. + const names = [ + 'mealquest-precache-v1', + 'mealquest-api-v1', + 'mealquest-media-v1', + 'mealquest-fonts-v1', + 'other-app-cache', + ] + const cacheStorage = { + keys: vi.fn(async () => names), + delete: vi.fn(async () => true), + } + vi.stubGlobal('caches', cacheStorage) + vi.stubGlobal('__WB_MANIFEST', []) + await import('./sw') + + const pending: Promise[] = [] + const activate = Object.assign(new Event('activate'), { + waitUntil: (promise: Promise) => pending.push(promise), + }) + self.dispatchEvent(activate) + await Promise.all(pending) + + expect(pending).toHaveLength(1) + expect(cacheStorage.delete).toHaveBeenCalledWith('mealquest-api-v1') + expect(cacheStorage.delete).toHaveBeenCalledTimes(1) + }) +}) diff --git a/web/src/sw.ts b/web/src/sw.ts index ca712ff..3983585 100644 --- a/web/src/sw.ts +++ b/web/src/sw.ts @@ -18,7 +18,7 @@ import { CacheFirst, NetworkOnly, type Strategy } from 'workbox-strategies' import { createApiStrategy } from './sw/apiStrategy' import { cacheNames, - cacheVersion, + cacheVersions, routeFor, staleCaches, type Route, @@ -29,7 +29,7 @@ declare const self: ServiceWorkerGlobalScope & { } // The precache becomes mealquest-precache-v1. -setCacheNameDetails({ prefix: 'mealquest', precache: 'precache', suffix: cacheVersion }) +setCacheNameDetails({ prefix: 'mealquest', precache: 'precache', suffix: cacheVersions.precache }) const day = 24 * 60 * 60 diff --git a/web/src/sw/routes.test.ts b/web/src/sw/routes.test.ts index 9b57d20..62685b3 100644 --- a/web/src/sw/routes.test.ts +++ b/web/src/sw/routes.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { cacheFallbackHeader, cacheNames, markCacheFallback, routeFor, staleCaches, type Route } from './routes' +import { cacheFallbackHeader, cacheNames, cacheVersions, markCacheFallback, routeFor, staleCaches, type Route } from './routes' const origin = 'https://mealquest.test' @@ -146,6 +146,18 @@ describe('cache names', () => { it('all start with mealquest-', () => { 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. + // The precache, photos and fonts keep their v1 caches. + expect(cacheNames).toEqual({ + api: 'mealquest-api-v2', + media: 'mealquest-media-v1', + fonts: 'mealquest-fonts-v1', + }) + expect(cacheVersions.precache).toBe('v1') + }) }) describe('staleCaches', () => { diff --git a/web/src/sw/routes.ts b/web/src/sw/routes.ts index 895fd40..e1f0ea3 100644 --- a/web/src/sw/routes.ts +++ b/web/src/sw/routes.ts @@ -24,14 +24,23 @@ export interface RouteRequest { mode: RequestMode } -// Bumping the version renames every cache; activate then deletes the old ones. -export const cacheVersion = 'v1' +// Each cache carries its own version. Bumping one renames only that cache; +// activate then deletes its old name and keeps every other cache, so saved +// 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', + media: 'v1', + fonts: 'v1', +} as const export const cachePrefix = 'mealquest-' export const cacheNames = { - api: `${cachePrefix}api-${cacheVersion}`, - media: `${cachePrefix}media-${cacheVersion}`, - fonts: `${cachePrefix}fonts-${cacheVersion}`, + api: `${cachePrefix}api-${cacheVersions.api}`, + media: `${cachePrefix}media-${cacheVersions.media}`, + fonts: `${cachePrefix}fonts-${cacheVersions.fonts}`, } as const export const networkTimeoutSeconds = 3 diff --git a/web/src/sw/worker.test.ts b/web/src/sw/worker.test.ts index 215b9cb..9c858c5 100644 --- a/web/src/sw/worker.test.ts +++ b/web/src/sw/worker.test.ts @@ -2,6 +2,7 @@ // source: they pin the parts routes.ts cannot express — the API cache's // strategy, the font strategy's expiration, the font route's place ahead of // the precache route and network-only requests left to the browser. +// src/sw.test.ts runs the worker's activation against mocked Cache Storage. import { describe, expect, it } from 'vitest' import source from '../sw.ts?raw' diff --git a/web/src/test/dinners.ts b/web/src/test/dinners.ts index f2a1cb2..f316391 100644 --- a/web/src/test/dinners.ts +++ b/web/src/test/dinners.ts @@ -10,7 +10,6 @@ export type DinnerBody = { cuisine: string | null is_favorite: boolean recipe_id: string | null - recipe_url: string | null photo_url: string | null grounding_status: string grounding_error: string | null @@ -33,7 +32,6 @@ export function dinnerBody(id: string, name: string, overrides: Partial