diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 539863e..5137428 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -95,8 +95,8 @@ Columns ending in `_ct` hold AES-256-GCM ciphertext (via `secretbox`). | --- | --- | | `household_settings` | One row: timezone, dinner_time (17:30), reminder_time (16:30), review_time (09:00), plan_draft day/time (Fri 09:00), suggestions day/time (Sun 08:00), chat_autosend day/time (Sat 08:00), onboarding_completed_at, updates_seen_at. | | `recipes` | A real recipe page: source_url (unique), site_name, title, total/prep/cook minutes (the page's JSON-LD), yield, raw_jsonld, fetched_at; and its grounding: photo_id, method, difficulty, minutes_total (normalized prep to table). | -| `recipe_ingredients` | recipe_id, position, raw_text, name, qty, unit, note. | -| `recipe_steps` | recipe_id, position, text. | +| `recipe_ingredients` | recipe_id, position (unique per recipe: the recipe order), raw_text, name, qty, unit, note. | +| `recipe_steps` | recipe_id, position (unique per recipe: the recipe order), 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 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. | @@ -105,7 +105,7 @@ Columns ending in `_ct` hold AES-256-GCM ciphertext (via `secretbox`). | `plan_days` | plan_id, date (unique per plan), dinner_id, is_new, calendar_event_id (set while the chosen calendar shows the day as planned; cleared when its dinner changes, another calendar is chosen or Google Calendar is disconnected). | | `rotation_reviews` | dinner_id + plan_date (unique), status, decision (`keep` / `drop` / `not_made`), name (set only by a drop: the dinner's name when dropped, kept whatever it is later renamed, so a drop stays among the passed names), decided_at, updated_by. | | `shopping_lists` | week_start (unique), plan_id, title (the AI-written post title; NULL until written or when the AI fails, when the week label is shown instead; never empty), status (`current` / `archived`), sent_to_chat_at, chat_send_unconfirmed_at (a send to Google Chat recorded as begun before posting and cleared once its outcome is known; left set when it may or may not have posted), frozen_at. | -| `shopping_groups` / `shopping_items` | One group per plan day with ingredients: dinner_id, date, position (by date). Item name, qty (the display quantity, NULL when the recipe gives none), needed (bool), position. Made by `shopping.Generate` from the plan, with no model call: an item is a grounded recipe ingredient's normalized name with qty and unit joined ("2 cups"), or a manual recipe's line as typed with no qty. Regenerating keeps the id and needed flag of an item whose (dinner_id, name) is unchanged. | +| `shopping_groups` / `shopping_items` | One group per plan day with ingredients: dinner_id, date, position (by date), frozen_dinner_name (the dinner's name when its list was frozen into the Supply archive, kept whatever it is later renamed; NULL while the list is current, which shows its dinners' names now; never empty). Item name, qty (the display quantity, NULL when the recipe gives none), needed (bool), position. Made by `shopping.Generate` from the plan, with no model call: an item is a grounded recipe ingredient's normalized name with qty and unit joined ("2 cups"), or a manual recipe's line as typed with no qty. Regenerating keeps the id and needed flag of an item whose (dinner_id, name) is unchanged. | | `ai_settings` | One row: provider, base_url, model, temperature, system_prompt, api_key_ct, api_key_last4, key_verified_at. | | `recipe_search_settings` | One row: provider (`searxng` / `brave`), base_url (the SearXNG instance), api_key_ct and api_key_last4 (Brave). | | `integrations_google` | One row: refresh_token_ct, calendar_id, calendar_name, connected_at, last_sync_error. | @@ -121,6 +121,16 @@ Columns ending in `_ct` hold AES-256-GCM ciphertext (via `secretbox`). `recipes` holds a fetched recipe page; `dinners` holds the household's meal record and its lifecycle, optionally linked to a recipe and a photo. +Migration `0018_recipe_item_position_unique` makes position unique per +recipe and fails, so the service refuses to start, on a database already +holding a duplicate. Before deploying it, check the database; both queries +must return no rows: + +```sql +SELECT recipe_id, position FROM recipe_ingredients GROUP BY 1, 2 HAVING count(*) > 1; +SELECT recipe_id, position FROM recipe_steps GROUP BY 1, 2 HAVING count(*) > 1; +``` + ## Dinner lifecycle ``` @@ -215,17 +225,17 @@ automatic** (D-021). raw_text}]` and `steps` the step texts, both in recipe position order and empty (`[]`) with no recipe, as for a dinner whose grounding is `pending` or `failed`; a recipe entered by hand has only `raw_text`. - Two items at one position leave recipe order unknown, so such a recipe - is 500 rather than shown out of order. `source` is `{site_name}` of the - page the recipe was fetched from, null for a manual recipe or none; it - has no `url`, as the page's link may carry a personal token. - `site_name` is the name the page gives its site (publisher or - `og:site_name`) and null otherwise: a page naming no site is stored - under its host, which may be the pasted link's — a token in a - subdomain — or a redirect's, so any stored name that could be a host - (the link's host, an IP address or a dotted host name, even a real - name like "Food.com") is withheld. The detail screen shows "Recipe - from {site}" only when `site_name` is not null. `why_picked` is the reason of the + `source` is `{site_name, url}` of the page the recipe was fetched from, + null for a manual recipe or none, as `dinners.SourceOf` gives it — the + one rule for every fetched recipe's source, also used by idea results + and recipe candidates. `url` + is the page's link only when it is free of secrets (https, no user, + query or fragment), else null. `site_name` is the name the page gives + its site (publisher or `og:site_name`), else the link's host without + "www." when `url` is shown, else null. Extraction never stores a host + as a site name, so a stored name is always the page's; migration 0020 + cleared the host-shaped names stored before that. The detail screen + shows "Recipe from {site}" only when `site_name` is not null. `why_picked` is the reason of the suggestion that created the dinner — its pending or accepted one, as declining a suggestion archives its dinner — null for one no suggestion created; a dinner with more than one such suggestion is 500, as nothing @@ -262,7 +272,8 @@ automatic** (D-021). `GET /api/v1/dinners/{id}/recipe-candidates` 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, + ranks them, with `site_name` as the recipe detail's (`dinners.SourceOf`, + null when the site cannot be named without leaking the link); it fetches pages but downloads no photo and runs no model, 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 @@ -319,15 +330,22 @@ automatic** (D-021). suggestion with the name it is suggested under, the reason and `cited_regular_ids` — unless, since the job read its input, a dinner it repeats was added, a suggestion it repeats passed, or a Regular it - cites renamed or archived (checked in the storing transaction). The job + cites renamed or archived (checked in the storing transaction). A + notifying batch also rechecks the Side quests it holds after each round + and lets go of those no longer eligible, so a later round grounds a + spare in place of each; only a change committed between the last + recheck and the store can leave it short of 5 with spares unused. The job publishes `{batch_id, stage, suggestion_ids, dropped}` as it goes (`suggesting`, `grounding`, `done`). It fails with `no_regulars`, `ai_failed` (the skill or `normalize_recipe` failed; no Side quest is made up instead, D-017), `search_not_configured` or `search_failed`, and the batch is set - `failed`. A failed `scheduled` or `manual` batch has stored nothing; a + `failed`. A `scheduled` or `manual` batch that fails before its final + store (the AI, grounding, or the store itself) has stored nothing; a failed `replenish` batch's Side quests stored before the failure stay in the deck. If - recording the batch's end fails, its job fails; the job runner retries + recording the batch's end fails, its job fails, even after the final + store committed: a notifying batch's Side quests and their + `new_suggestions` then stay; the job runner retries a job's final status write with backoff until it lands, each attempt bounded at 5 seconds; at shutdown an attempt that fails or times out is not retried, and the next start fails the job as interrupted. Recording @@ -635,7 +653,10 @@ automatic** (D-021). `shopping.Freeze(weekStart)` moves a week's Supply run into the Supply archive: `status` becomes `archived` and `frozen_at` is set, after which -generating the list or changing an item is 409 `archived`. It freezes only +generating the list or changing an item is 409 `archived`. In the same +transaction each group keeps its dinner's name as `frozen_dinner_name`, so +the frozen list and its post show the names they had when frozen, whatever +a dinner is renamed later. It freezes only a week that has ended in the household's time zone (its Monday is before the Monday of the household's current week), so a week not yet ended is `ErrWeekNotEnded` and a week with no list `ErrListNotFound`; freezing a @@ -1187,7 +1208,8 @@ Fake-clock tests of a schedule live in `internal/scheduler`, where Monday is a household-local date, so the week of Monday 28 December is in December. `label` is the week label ("Week 41 · Oct 5 – 11"; the UI sets it in capitals), `title` null while untitled, `dinners` the - list's dinners in day order and `dinnerCount` their number, + list's dinners in day order (as named when the list was frozen; the + current week's as named now) and `dinnerCount` their number, `itemCount` all items, `onHandCount` the items marked "have it", and `current` true for the household's current week. 400 `invalid_request` for any other `by`. A frozen list is read through `GET diff --git a/internal/db/models.go b/internal/db/models.go index d451225..be54524 100644 --- a/internal/db/models.go +++ b/internal/db/models.go @@ -255,13 +255,14 @@ type Session struct { } type ShoppingGroup struct { - ID string - ListID string - DinnerID string - Date string - Position int64 - CreatedAt string - UpdatedAt string + ID string + ListID string + DinnerID string + Date string + Position int64 + CreatedAt string + UpdatedAt string + FrozenDinnerName sql.NullString } type ShoppingItem struct { diff --git a/internal/db/querier.go b/internal/db/querier.go index d1623b1..d87ba6c 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -107,6 +107,9 @@ type Querier interface { // Records a pending or running batch as done or failed; a batch already // finished is left as it is (0 rows). FinishSuggestionBatch(ctx context.Context, arg FinishSuggestionBatchParams) (int64, error) + // Keeps the name each dinner of list list_id has now as its group's + // frozen_dinner_name, as the list is frozen into the Supply archive. + FreezeShoppingGroupNames(ctx context.Context, listID string) error // Freezes list id into the Supply archive at frozen_at, unless it is // frozen already. FreezeShoppingList(ctx context.Context, arg FreezeShoppingListParams) (int64, error) @@ -228,7 +231,8 @@ type Querier interface { ListRecentPassedNames(ctx context.Context, limit int64) ([]string, error) ListRecipeIngredients(ctx context.Context, recipeID string) ([]RecipeIngredient, error) ListRecipeSteps(ctx context.Context, recipeID string) ([]RecipeStep, error) - // The groups of list list_id with their dinner's name, in list order. + // The groups of list list_id with their dinner's name now, in list order. + // A group of a frozen list shows its frozen_dinner_name instead. ListShoppingGroupDinners(ctx context.Context, listID string) ([]ListShoppingGroupDinnersRow, error) // The groups of list list_id, in list order. ListShoppingGroups(ctx context.Context, listID string) ([]ShoppingGroup, error) diff --git a/internal/db/recipes.sql.go b/internal/db/recipes.sql.go index b43b465..cc218c1 100644 --- a/internal/db/recipes.sql.go +++ b/internal/db/recipes.sql.go @@ -220,7 +220,7 @@ func (q *Queries) GetRecipeBySourceURL(ctx context.Context, sourceUrl string) (R const listRecipeIngredients = `-- name: ListRecipeIngredients :many SELECT id, recipe_id, position, raw_text, name, qty, unit, note, created_at, updated_at FROM recipe_ingredients WHERE recipe_id = ? -ORDER BY position, id +ORDER BY position ` func (q *Queries) ListRecipeIngredients(ctx context.Context, recipeID string) ([]RecipeIngredient, error) { @@ -260,7 +260,7 @@ func (q *Queries) ListRecipeIngredients(ctx context.Context, recipeID string) ([ const listRecipeSteps = `-- name: ListRecipeSteps :many SELECT id, recipe_id, position, text, created_at, updated_at FROM recipe_steps WHERE recipe_id = ? -ORDER BY position, id +ORDER BY position ` func (q *Queries) ListRecipeSteps(ctx context.Context, recipeID string) ([]RecipeStep, error) { diff --git a/internal/db/shopping.sql.go b/internal/db/shopping.sql.go index 91b1b9b..4d5caa4 100644 --- a/internal/db/shopping.sql.go +++ b/internal/db/shopping.sql.go @@ -155,6 +155,19 @@ func (q *Queries) DeleteShoppingItem(ctx context.Context, id string) error { return err } +const freezeShoppingGroupNames = `-- name: FreezeShoppingGroupNames :exec +UPDATE shopping_groups +SET frozen_dinner_name = (SELECT dinners.name FROM dinners WHERE dinners.id = shopping_groups.dinner_id) +WHERE list_id = ? +` + +// Keeps the name each dinner of list list_id has now as its group's +// frozen_dinner_name, as the list is frozen into the Supply archive. +func (q *Queries) FreezeShoppingGroupNames(ctx context.Context, listID string) error { + _, err := q.db.ExecContext(ctx, freezeShoppingGroupNames, listID) + return err +} + const freezeShoppingList = `-- name: FreezeShoppingList :execrows UPDATE shopping_lists SET status = 'archived', frozen_at = ?, updated_at = ? @@ -235,7 +248,7 @@ func (q *Queries) GetShoppingList(ctx context.Context, weekStart string) (Shoppi } const listShoppingGroupDinners = `-- name: ListShoppingGroupDinners :many -SELECT shopping_groups.id, shopping_groups.list_id, shopping_groups.dinner_id, shopping_groups.date, shopping_groups.position, shopping_groups.created_at, shopping_groups.updated_at, dinners.name AS dinner_name +SELECT shopping_groups.id, shopping_groups.list_id, shopping_groups.dinner_id, shopping_groups.date, shopping_groups.position, shopping_groups.created_at, shopping_groups.updated_at, shopping_groups.frozen_dinner_name, dinners.name AS dinner_name FROM shopping_groups JOIN dinners ON dinners.id = shopping_groups.dinner_id WHERE shopping_groups.list_id = ? @@ -247,7 +260,8 @@ type ListShoppingGroupDinnersRow struct { DinnerName string } -// The groups of list list_id with their dinner's name, in list order. +// The groups of list list_id with their dinner's name now, in list order. +// A group of a frozen list shows its frozen_dinner_name instead. func (q *Queries) ListShoppingGroupDinners(ctx context.Context, listID string) ([]ListShoppingGroupDinnersRow, error) { rows, err := q.db.QueryContext(ctx, listShoppingGroupDinners, listID) if err != nil { @@ -265,6 +279,7 @@ func (q *Queries) ListShoppingGroupDinners(ctx context.Context, listID string) ( &i.ShoppingGroup.Position, &i.ShoppingGroup.CreatedAt, &i.ShoppingGroup.UpdatedAt, + &i.ShoppingGroup.FrozenDinnerName, &i.DinnerName, ); err != nil { return nil, err @@ -281,7 +296,7 @@ func (q *Queries) ListShoppingGroupDinners(ctx context.Context, listID string) ( } const listShoppingGroups = `-- name: ListShoppingGroups :many -SELECT id, list_id, dinner_id, date, position, created_at, updated_at FROM shopping_groups +SELECT id, list_id, dinner_id, date, position, created_at, updated_at, frozen_dinner_name FROM shopping_groups WHERE list_id = ? ORDER BY position, id ` @@ -304,6 +319,7 @@ func (q *Queries) ListShoppingGroups(ctx context.Context, listID string) ([]Shop &i.Position, &i.CreatedAt, &i.UpdatedAt, + &i.FrozenDinnerName, ); err != nil { return nil, err } diff --git a/internal/dinners/detail.go b/internal/dinners/detail.go index 6ccb079..c0f8da3 100644 --- a/internal/dinners/detail.go +++ b/internal/dinners/detail.go @@ -58,14 +58,14 @@ type Ingredient struct { RawText string `json:"raw_text"` } -// Source is the site a recipe was fetched from. SiteName is the name the -// page itself gives its site, and null when it gives none or the stored -// name could have come from a link (see pageSiteName). URL is the page's -// link only when it is verifiably free of secrets (see safeSourceURL), and -// null otherwise: a link pasted or offered as a candidate may carry a -// personal token, so a client links the credit only when URL is not null. -// Source is set for every fetched recipe, so a client always credits it: -// by SiteName, else by URL's host, else without naming the site. +// Source is the site a recipe was fetched from, as SourceOf gives it. +// SiteName is the name the page gives its site, else the host of URL when +// URL is shown, else null. URL is the page's link only when it is +// verifiably free of secrets (see safeSourceURL), and null otherwise: a +// link pasted or offered as a candidate may carry a personal token, so a +// client links the credit only when URL is not null. Source is set for +// every fetched recipe, so a client always credits it: by SiteName, else +// without naming the site. type Source struct { SiteName *string `json:"site_name"` URL *string `json:"url"` @@ -157,10 +157,7 @@ func readRecipe(ctx context.Context, q *db.Queries, recipeID string, d *Detail) if err != nil { return fmt.Errorf("ingredients: %w", err) } - for i, ing := range ings { - if i > 0 && ing.Position == ings[i-1].Position { - return duplicatePosition("ingredient", ing.Position) - } + for _, ing := range ings { d.Ingredients = append(d.Ingredients, Ingredient{ Name: nullString(ing.Name), Qty: nullString(ing.Qty), @@ -173,52 +170,46 @@ func readRecipe(ctx context.Context, q *db.Queries, recipeID string, d *Detail) if err != nil { return fmt.Errorf("steps: %w", err) } - for i, step := range steps { - if i > 0 && step.Position == steps[i-1].Position { - return duplicatePosition("step", step.Position) - } + for _, step := range steps { d.Steps = append(d.Steps, step.Text) } return nil } // SourceOf is the Source of a recipe fetched from sourceURL whose stored -// site name is siteName: the name only when the page gave it (see -// pageSiteName) and the link only when it is free of secrets (see -// safeSourceURL). It is the one way a fetched recipe's source reaches a -// client. A sourceURL that does not parse is an error, whether or not a -// site name is stored. +// site name is siteName, the name its page gave (recipes.Recipe.SiteName), +// null when the page named none. It is the one rule by which a fetched +// recipe's source reaches a client — the recipe detail, an idea and a +// recipe candidate offered for a dinner alike: +// +// - the link is shown only when it is free of secrets (see +// safeSourceURL); +// - a site name the page gave is shown as it stands, as the page +// publishes it to everyone who reads it; +// - otherwise the site is named by the link's host without a leading +// "www.", and only when the link itself is shown, so the name tells +// nothing the shown link does not; else it is null. +// +// A host is never stored as a site name, so a stored name is always the +// page's: migration 0020 cleared the names stored before that, when a +// page naming no site was stored under its host. A sourceURL that does not +// parse is an error, whether or not a site name is stored. func SourceOf(siteName sql.NullString, sourceURL string) (Source, error) { link, err := url.Parse(sourceURL) if err != nil { // Not wrapped: url.Parse's error quotes the link. return Source{}, errors.New("source url does not parse") } - return Source{SiteName: pageSiteName(siteName, link), URL: safeSourceURL(sourceURL)}, nil -} - -// pageSiteName is the stored site name of a recipe fetched from link -// when it is one the page gave, else nil. A page that names no site is -// stored under recipes.HostSiteName of the URL it was served from: the -// pasted link's host, which may hold a personal token (a private -// subdomain), or that of a page the link redirected to, which is not -// stored. Nothing records which a stored name is, so any name that could -// be a host — the link's own host, an IP address, or a dotted host name — -// is treated as one and withheld. A site that names itself as a domain, -// such as "Food.com", loses its name with them: a credit that does not -// name the site is better than one that leaks a link. -func pageSiteName(stored sql.NullString, link *url.URL) *string { - if !stored.Valid { - return nil + src := Source{URL: safeSourceURL(sourceURL)} + switch { + case siteName.Valid && siteName.String != "": + name := siteName.String + src.SiteName = &name + case src.URL != nil: + host := strings.TrimPrefix(link.Hostname(), "www.") + src.SiteName = &host } - name := stored.String - if strings.EqualFold(name, recipes.HostSiteName(link)) || net.ParseIP(name) != nil { - return nil - } - if u, err := url.Parse("https://" + name + "/"); err == nil && u.Host == name && strings.Contains(name, ".") { - return nil - } - return &name + return src, nil } // safeSourceURL is sourceURL when it is verifiably free of secrets — an @@ -283,11 +274,3 @@ func browserHost(u *url.URL) bool { } return true } - -// duplicatePosition is the error for two recipe items of kind at position. -// Position is the only record of recipe order, and the id that breaks the -// tie says nothing about it, so a recipe with a duplicate is refused rather -// than shown in an order nobody can trust. -func duplicatePosition(kind string, position int64) error { - return fmt.Errorf("two %ss at position %d: recipe order is unknown", kind, position) -} diff --git a/internal/dinners/dinners_test.go b/internal/dinners/dinners_test.go index 14e0ff6..31a1ef3 100644 --- a/internal/dinners/dinners_test.go +++ b/internal/dinners/dinners_test.go @@ -76,35 +76,40 @@ func TestCreateRegularNeverNamesFromLink(t *testing.T) { check("duplicate error", dup.Error()) } -// TestPageSiteNameWithholdsHosts: a stored site name that could be a host — -// the link's own, in any case and without "www.", an IP address or a dotted -// host name — is withheld; a name the page gave, dotless or with spaces, is -// kept. -func TestPageSiteNameWithholdsHosts(t *testing.T) { +// TestSourceOfSiteName: a site name the page gave is shown as it stands, +// even one shaped like a host and even over a link that is withheld; a +// recipe whose page named none is named by its link's host without "www." +// only when the link itself is shown, and is unnamed otherwise. +func TestSourceOfSiteName(t *testing.T) { t.Parallel() - const link = "https://www.TokenHost/recipes/stew" + const ( + ordinary = "https://www.Kitchen.example/recipes/stew" + token = "https://private-token.example.com/recipes/stew?access_token=abc123" + ) for _, tc := range []struct { - stored string - valid bool - want *string + stored string + valid bool + link string + want, wantURL *string }{ - {"", false, nil}, - {"tokenhost", true, nil}, - {"TOKENHOST", true, nil}, - {"private-token.example.com", true, nil}, - {"Food.com", true, nil}, - {"203.0.113.7", true, nil}, - {"2001:db8::1", true, nil}, - {"Delish", true, ptr("Delish")}, - {"Bon Appétit", true, ptr("Bon Appétit")}, - {"Mr. Food Test Kitchen", true, ptr("Mr. Food Test Kitchen")}, + {"Delish", true, ordinary, ptr("Delish"), ptr(ordinary)}, + {"Food.com", true, ordinary, ptr("Food.com"), ptr(ordinary)}, + {"Bon Appétit", true, token, ptr("Bon Appétit"), nil}, + {"", false, ordinary, ptr("Kitchen.example"), ptr(ordinary)}, + {"", true, ordinary, ptr("Kitchen.example"), ptr(ordinary)}, + {"", false, token, nil, nil}, + {"", false, "https://203.0.113.7/stew", ptr("203.0.113.7"), ptr("https://203.0.113.7/stew")}, + {"", false, "http://kitchen.example/stew", nil, nil}, } { - src, err := dinners.SourceOf(sql.NullString{String: tc.stored, Valid: tc.valid}, link) + src, err := dinners.SourceOf(sql.NullString{String: tc.stored, Valid: tc.valid}, tc.link) if err != nil { - t.Fatalf("SourceOf(%q): %v", tc.stored, err) + t.Fatalf("SourceOf(%q, %q): %v", tc.stored, tc.link, err) } if got := src.SiteName; (got == nil) != (tc.want == nil) || (got != nil && *got != *tc.want) { - t.Errorf("SourceOf(%q) site name = %v, want %v", tc.stored, deref(got), deref(tc.want)) + t.Errorf("SourceOf(%q, %q) site name = %v, want %v", tc.stored, tc.link, deref(got), deref(tc.want)) + } + if got := src.URL; (got == nil) != (tc.wantURL == nil) || (got != nil && *got != *tc.wantURL) { + t.Errorf("SourceOf(%q, %q) url = %v, want %v", tc.stored, tc.link, deref(got), deref(tc.wantURL)) } } // A link that does not parse is an error with or without a site name. diff --git a/internal/dinners/grounding.go b/internal/dinners/grounding.go index 93aeee2..895cfdc 100644 --- a/internal/dinners/grounding.go +++ b/internal/dinners/grounding.go @@ -304,7 +304,9 @@ type Candidate struct { // may carry a token. CandidateID string `json:"candidate_id"` Title string `json:"title"` - SiteName string `json:"site_name"` + // SiteName is the page's site as SourceOf names it, null when it + // cannot be named without leaking the URL. + SiteName *string `json:"site_name"` // PhotoURL is the cached photo of a page already grounded, else null. PhotoURL *string `json:"photo_url"` Minutes *int `json:"minutes"` @@ -367,9 +369,13 @@ func (s *Service) Candidates(ctx context.Context, id string) ([]Candidate, error out := make([]Candidate, len(offers)) urls := make(map[string]string, len(offers)) for i, o := range offers { + src, err := SourceOf(sql.NullString{String: o.SiteName, Valid: o.SiteName != ""}, o.URL) + if err != nil { + return nil, fmt.Errorf("%w: dinner %s: candidate %d: %w", ErrCandidatesFailed, id, i+1, err) + } 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} + out[i] = Candidate{CandidateID: cid, Title: o.Title, SiteName: src.SiteName, Minutes: o.Minutes} if o.PhotoID != "" { u := PhotoURL(o.PhotoID) out[i].PhotoURL = &u diff --git a/internal/dinners/grounding_test.go b/internal/dinners/grounding_test.go index 11fb4c0..9693ff8 100644 --- a/internal/dinners/grounding_test.go +++ b/internal/dinners/grounding_test.go @@ -718,11 +718,11 @@ func TestCandidates(t *testing.T) { } first, second := got[0], got[1] wantPhoto := dinners.PhotoURL(e.dinner(t, c.ID).PhotoID.String) - if first.Title != "Chicken Curry" || first.SiteName != "Curry House" || + if first.Title != "Chicken Curry" || deref(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.Title != "Easy Weeknight Curry" || second.SiteName != "B Kitchen" || + if second.Title != "Easy Weeknight Curry" || deref(second.SiteName) != "B Kitchen" || second.PhotoURL != nil || second.Minutes == nil || *second.Minutes != 25 { t.Errorf("second = %+v, want curryB without a photo", second) } diff --git a/internal/ideas/ideas.go b/internal/ideas/ideas.go index 557b512..ea11811 100644 --- a/internal/ideas/ideas.go +++ b/internal/ideas/ideas.go @@ -486,7 +486,8 @@ func sourceOf(gr recipes.Grounded) (dinners.Source, error) { // link sourceURL and site name siteName, as a client may see it (see // dinners.SourceOf). A result stored now already holds it, and one stored // before sources were made safe holds the grounded recipe's own, with "" -// for no site name, as sourceOf reads it; a null link, which only a result +// for no site name, as sourceOf reads it, less any name that could be a +// host, which migration 0020 cleared; a null link, which only a result // stored now holds, is checked as "". func shownSource(sourceURL, siteName *string) (dinners.Source, error) { var link string diff --git a/internal/recipes/extract.go b/internal/recipes/extract.go index 9de3383..64cea03 100644 --- a/internal/recipes/extract.go +++ b/internal/recipes/extract.go @@ -28,8 +28,10 @@ var ( type Recipe struct { // Title is the recipe's name. Title string - // SiteName is the publisher's name, else the page's og:site_name, else - // HostSiteName of the page's URL. + // SiteName is the name the page gives its site: the publisher's name, + // else the page's og:site_name; empty when it names none. It is never + // taken from the page's URL, whose host may carry a personal token, so + // a stored site name is always one a page gave (see dinners.SourceOf). SiteName string // ImageURL is the largest image, absolute; empty when the recipe has // none. @@ -173,7 +175,7 @@ func recipeFrom(n map[string]any, g graph, base *url.URL, ogSiteName string) (Re if r.Title = cleanText(name); !ok || r.Title == "" { return Recipe{}, errors.New("name is missing") } - if r.SiteName, err = siteName(n["publisher"], g, ogSiteName, base); err != nil { + if r.SiteName, err = siteName(n["publisher"], g, ogSiteName); err != nil { return Recipe{}, err } if r.ImageURL, err = imageURL(n["image"], g, base); err != nil { @@ -213,7 +215,7 @@ func cleanText(s string) string { return strings.TrimSpace(html.UnescapeString(s)) } -func siteName(publisher any, g graph, ogSiteName string, base *url.URL) (string, error) { +func siteName(publisher any, g graph, ogSiteName string) (string, error) { pubs, ok := publisher.([]any) if !ok { pubs = []any{publisher} @@ -229,16 +231,7 @@ func siteName(publisher any, g graph, ogSiteName string, base *url.URL) (string, } } } - if ogSiteName != "" { - return ogSiteName, nil - } - return HostSiteName(base), nil -} - -// HostSiteName is the site name of a page that names none: its host -// without a leading "www.". -func HostSiteName(u *url.URL) string { - return strings.TrimPrefix(u.Hostname(), "www.") + return ogSiteName, nil } // imageURL picks the largest of the recipe's images by width × height; diff --git a/internal/recipes/extract_test.go b/internal/recipes/extract_test.go index 1a6799e..23361f9 100644 --- a/internal/recipes/extract_test.go +++ b/internal/recipes/extract_test.go @@ -251,10 +251,10 @@ func TestExtractShapes(t *testing.T) { }, }, { - name: "site name from host without publisher or og:site_name", + name: "no site name without publisher or og:site_name, never the host", html: page("", `{"@type":"Recipe","name":"Stew"}`), check: func(t *testing.T, r Recipe) { - if r.SiteName != "example.com" { + if r.SiteName != "" { t.Errorf("SiteName = %q", r.SiteName) } }, diff --git a/internal/recipes/ground.go b/internal/recipes/ground.go index 3a69404..a94f03c 100644 --- a/internal/recipes/ground.go +++ b/internal/recipes/ground.go @@ -65,7 +65,9 @@ type Grounded struct { // Difficulty is easy, medium or hard. Difficulty string `json:"difficulty"` SourceURL string `json:"source_url"` - SiteName string `json:"site_name"` + // SiteName is the name the page gives its site, "" when it names none + // (see Recipe.SiteName). + SiteName string `json:"site_name"` } // SearchSource returns the household's current search provider; @@ -200,8 +202,10 @@ func (g *Grounder) searchRanked(ctx context.Context, dishName, query string) (ra // Offer is a ranked candidate offered for the household to ground a dish // in, with GroundURL on its URL. type Offer struct { - URL string - Title string + URL string + Title string + // SiteName is the name the page gives its site, "" when it names none + // (see Recipe.SiteName). SiteName string // PhotoID is the cached photo of a candidate already stored, "" for // one that is not: a candidate's photo is cached only when it grounds. diff --git a/internal/server/dinnerdetail_test.go b/internal/server/dinnerdetail_test.go index 1789e1c..b2208b3 100644 --- a/internal/server/dinnerdetail_test.go +++ b/internal/server/dinnerdetail_test.go @@ -3,6 +3,7 @@ package server import ( "database/sql" "encoding/json" + "io/fs" "net/http" "net/http/httptest" "os" @@ -14,6 +15,7 @@ import ( "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/dinners" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes/recipestest" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/migrations" ) // getDetail answers GET /dinners/{id} for id, failing unless it is 200, and @@ -70,8 +72,8 @@ func TestGetDinnerGroundedSideQuest(t *testing.T) { } for _, stmt := range []string{ `UPDATE dinners SET status = 'quest_log', origin = 'side_quest', is_favorite = 1 WHERE id = ?`, - `UPDATE recipe_ingredients SET position = 1 - position WHERE recipe_id = (SELECT recipe_id FROM dinners WHERE id = ?)`, - `UPDATE recipe_steps SET position = 1 - position WHERE recipe_id = (SELECT recipe_id FROM dinners WHERE id = ?)`, + `UPDATE recipe_ingredients SET position = 9 - position WHERE recipe_id = (SELECT recipe_id FROM dinners WHERE id = ?)`, + `UPDATE recipe_steps SET position = 9 - position WHERE recipe_id = (SELECT recipe_id FROM dinners WHERE id = ?)`, } { if _, err := e.st.Writer.Exec(stmt, id); err != nil { t.Fatal(err) @@ -328,14 +330,33 @@ func TestGetDinnerSourceURLOnlyWhenSafe(t *testing.T) { } } -// TestGetDinnerSiteNameNeverFromLink: a page that names no site is stored -// under its host, so a recipe grounded in a pasted link whose host carries a -// token has site_name null and nothing from the link in its detail; a page -// that names its site keeps that name, even on a token-bearing host; and a -// stored name shaped like another host, as a redirect leaves, is withheld. -// The links carry a query, so source.url is withheld too and nothing of the -// link may appear. -func TestGetDinnerSiteNameNeverFromLink(t *testing.T) { +// siteNameMigration is migration 0020, which clears the host-shaped site +// names stored before extraction stopped storing hosts. +const siteNameMigration = "0020_site_name_from_page.sql" + +// applySiteNameMigration runs siteNameMigration again over rows a test +// stored as the code before it did, as upgrading a database holding them +// does. +func applySiteNameMigration(t *testing.T, st *store.Store) { + t.Helper() + body, err := fs.ReadFile(migrations.FS, siteNameMigration) + if err != nil { + t.Fatal(err) + } + if _, err := st.Writer.ExecContext(t.Context(), string(body)); err != nil { + t.Fatalf("%s: %v", siteNameMigration, err) + } +} + +// TestGetDinnerSiteNameProvenance: extraction stores only a site name the +// page gave, never the URL's host, and the detail applies one rule to what +// is stored. A name from page metadata is shown even over a token-bearing +// link; a page naming no site stores no name, and is named by its link's +// host only when the link is shown — so over a link with a token nothing +// of the link reaches the client. A name stored before extraction stopped +// storing hosts is read by the same rule once migration 0020 has cleared +// it if it could be a host. +func TestGetDinnerSiteNameProvenance(t *testing.T) { t.Parallel() e := newDinnersEnv(t) const ( @@ -343,11 +364,14 @@ func TestGetDinnerSiteNameNeverFromLink(t *testing.T) { namedToken = "named-token-def456" bare = "https://" + bareToken + ".example.com/recipe?print=1" named = "https://www." + namedToken + ".example.com/recipe?print=1" + ordinary = "https://www.kitchen.example/recipes/curry" ) e.pages.Bodies[bare] = recipestest.RecipePage("Chicken Curry", "", "https://a.example/curry.jpg", 40) e.pages.Bodies[named] = recipestest.RecipePage("Weeknight Curry", "Curry House", "https://b.example/curry.jpg", 25) + e.pages.Bodies[ordinary] = recipestest.RecipePage("Green Curry", "", "https://c.example/curry.jpg", 30) e.images.JPEGs["https://a.example/curry.jpg"] = recipestest.JPEG(t, 400, 320, 1) e.images.JPEGs["https://b.example/curry.jpg"] = recipestest.JPEG(t, 400, 320, 2) + e.images.JPEGs["https://c.example/curry.jpg"] = recipestest.JPEG(t, 400, 320, 3) ground := func(dish, page string) (id, recipeID string) { t.Helper() id = createDinner(t, e.h, `{"name":"`+dish+`"}`) // runner not started: stays pending @@ -360,15 +384,18 @@ func TestGetDinnerSiteNameNeverFromLink(t *testing.T) { } return id, recipeID } - storedSite := func(recipeID string) string { + storedSite := func(recipeID string) any { t.Helper() - var site string + var site sql.NullString if err := e.st.Reader.QueryRow(`SELECT site_name FROM recipes WHERE id = ?`, recipeID).Scan(&site); err != nil { t.Fatal(err) } - return site + if !site.Valid { + return nil + } + return site.String } - check := func(what, id string, leaks []string, want any) { + check := func(what, id string, leaks []string, want map[string]any) { t.Helper() rec, _ := callDinners(t, e.h, http.MethodGet, dinnersPath+"/"+id, "") if rec.Code != http.StatusOK { @@ -380,28 +407,48 @@ func TestGetDinnerSiteNameNeverFromLink(t *testing.T) { } } raw, _ := getDetail(t, e.h, id) - if src, _ := raw["source"].(map[string]any); !reflect.DeepEqual(src, map[string]any{"site_name": want, "url": nil}) { - t.Errorf("source of %s = %v, want site_name %v and url null", what, raw["source"], want) + if src, _ := raw["source"].(map[string]any); !reflect.DeepEqual(src, want) { + t.Errorf("source of %s = %v, want %v", what, raw["source"], want) } } - bareID, bareRecipe := ground("Chicken curry", bare) - if got := storedSite(bareRecipe); got != bareToken+".example.com" { - t.Fatalf("stored site_name = %q, want the link's host", got) - } - check("a page naming no site", bareID, []string{bare, bareToken, "example.com"}, nil) - namedID, namedRecipe := ground("Weeknight curry", named) if got := storedSite(namedRecipe); got != "Curry House" { - t.Fatalf("stored site_name = %q, want the page's", got) + t.Fatalf("stored site_name = %v, want the page's", got) } - check("a page naming its site", namedID, []string{named, namedToken, "example.com"}, "Curry House") + check("a metadata-derived name over a token-bearing link", namedID, []string{named, namedToken, "example.com"}, + map[string]any{"site_name": "Curry House", "url": nil}) - const redirectHost = "redirect-token-ghi789.example.net" - if _, err := e.st.Writer.Exec(`UPDATE recipes SET site_name = ? WHERE id = ?`, redirectHost, namedRecipe); err != nil { - t.Fatal(err) + bareID, bareRecipe := ground("Chicken curry", bare) + if got := storedSite(bareRecipe); got != nil { + t.Fatalf("stored site_name = %v, want null: the page names no site", got) } - check("a page named by a redirect's host", namedID, []string{redirectHost, "redirect-token", namedToken}, nil) + check("a hostname-derived name over a token-bearing link", bareID, []string{bare, bareToken, "example.com"}, + map[string]any{"site_name": nil, "url": nil}) + + ordinaryID, ordinaryRecipe := ground("Green curry", ordinary) + if got := storedSite(ordinaryRecipe); got != nil { + t.Fatalf("stored site_name = %v, want null: the page names no site", got) + } + check("a hostname-derived name over an ordinary link", ordinaryID, nil, + map[string]any{"site_name": "kitchen.example", "url": ordinary}) + + // Rows as extraction stored them before it stopped storing hosts: the + // token-bearing link's own host, and a redirect's host under a page + // that named its site, are cleared by 0020; a page's own name stays. + const redirectHost = "redirect-token-ghi789.example.net" + for recipeID, site := range map[string]string{bareRecipe: bareToken + ".example.com", namedRecipe: redirectHost, ordinaryRecipe: "Green Kitchen"} { + if _, err := e.st.Writer.Exec(`UPDATE recipes SET site_name = ? WHERE id = ?`, site, recipeID); err != nil { + t.Fatal(err) + } + } + applySiteNameMigration(t, e.st) + check("a row stored before, named by its token-bearing link's host", bareID, []string{bare, bareToken, "example.com"}, + map[string]any{"site_name": nil, "url": nil}) + check("a row stored before, named by a redirect's host", namedID, []string{redirectHost, "redirect-token", namedToken}, + map[string]any{"site_name": nil, "url": nil}) + check("a row stored before, named by its page", ordinaryID, nil, + map[string]any{"site_name": "Green Kitchen", "url": ordinary}) } // TestGetDinnerWhyPickedIsCreatingSuggestion: why_picked is the reason of @@ -445,25 +492,3 @@ func TestGetDinnerWhyPickedIsCreatingSuggestion(t *testing.T) { t.Errorf("why_picked = %v, want %q", got.WhyPicked, reason) } } - -// TestGetDinnerDuplicatePositionFails: a recipe with two ingredients or two -// steps at one position has no order anyone can trust, so its detail is a -// 500 rather than the recipe in some order. -func TestGetDinnerDuplicatePositionFails(t *testing.T) { - t.Parallel() - e := newDinnersEnv(t) - for _, table := range []string{"recipe_ingredients", "recipe_steps"} { - id := createDinner(t, e.h, `{"name":"Family chili `+table+`"}`) - if rec, _ := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground", - `{"manual":{"ingredients":["1 lb beef","1 can beans"],"steps":["Brown.","Simmer."],"minutes":240,"method":"crockpot"}}`); rec.Code != http.StatusOK { - t.Fatalf("POST ground = %d: %s", rec.Code, rec.Body) - } - if _, err := e.st.Writer.Exec(`UPDATE `+table+` SET position = 0 WHERE recipe_id = (SELECT recipe_id FROM dinners WHERE id = ?)`, id); err != nil { - t.Fatal(err) - } - rec, _ := callDinners(t, e.h, http.MethodGet, dinnersPath+"/"+id, "") - if rec.Code != http.StatusInternalServerError || strings.Contains(rec.Body.String(), "Simmer.") || strings.Contains(rec.Body.String(), "1 lb beef") { - t.Errorf("GET with duplicate %s positions = %d: %s, want 500 and no recipe", table, rec.Code, rec.Body) - } - } -} diff --git a/internal/server/dinners.go b/internal/server/dinners.go index 5f0d54e..989fa4f 100644 --- a/internal/server/dinners.go +++ b/internal/server/dinners.go @@ -72,11 +72,11 @@ func getDinners(logger *slog.Logger, svc *dinners.Service) http.HandlerFunc { // getDinner answers GET /dinners/{id} with the dinner, its recipe and the // reason it was suggested. An archived dinner is 404, like an unknown one, -// not a 200 with its status: it has no detail to show. source never has the -// recipe page's link, and source.site_name is null unless the page itself -// named its site — a name that could be a host, the link's or a redirect's, -// is withheld (dinners.Source) — so the detail screen shows "Recipe from -// {site}" only when site_name is not null. +// not a 200 with its status: it has no detail to show. source is as +// dinners.SourceOf gives it: the recipe page's link only when it is free of +// secrets, and site_name the name the page gave its site, else the shown +// link's host, else null — so the detail screen shows "Recipe from {site}" +// only when site_name is not null. func getDinner(logger *slog.Logger, svc *dinners.Service) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { d, err := svc.Get(r.Context(), chi.URLParam(r, "id")) @@ -134,7 +134,8 @@ func patchDinner(logger *slog.Logger, svc *dinners.Service) http.HandlerFunc { } // getDinnerCandidates answers GET /dinners/{id}/recipe-candidates with up -// to dinners.MaxCandidates recipe pages for the dinner's name. +// to dinners.MaxCandidates recipe pages for the dinner's name, each +// site_name as dinners.SourceOf gives it. func getDinnerCandidates(logger *slog.Logger, svc *dinners.Service) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { list, err := svc.Candidates(r.Context(), chi.URLParam(r, "id")) diff --git a/internal/server/grounding_test.go b/internal/server/grounding_test.go index dfac21a..608e555 100644 --- a/internal/server/grounding_test.go +++ b/internal/server/grounding_test.go @@ -184,6 +184,51 @@ func TestGetRecipeCandidates(t *testing.T) { } } +// TestGetRecipeCandidatesSiteNames: a candidate's site_name follows the +// recipe detail's rule (dinners.SourceOf), not a hostname fallback of its +// own: a name the page gave is offered as it stands, even over a link with +// a token; a page naming no site is named by its link's host only when +// the link is free of secrets, and is null over a token-bearing link, so +// nothing of that link reaches the client. +func TestGetRecipeCandidatesSiteNames(t *testing.T) { + t.Parallel() + e := newDinnersEnv(t) + const ( + token = "offer-token-abc123" + named = "https://named.example/curry?access_token=" + token + bare = "https://" + token + ".example.com/curry?print=1" + ordinary = "https://www.kitchen.example/recipes/curry" + ) + e.search.Results["Chicken curry"] = []recipes.Candidate{{URL: named}, {URL: bare}, {URL: ordinary}} + e.pages.Bodies[named] = recipestest.RecipePage("Named Curry", "Curry House", "https://a.example/curry.jpg", 40) + e.pages.Bodies[bare] = recipestest.RecipePage("Bare Curry", "", "https://b.example/curry.jpg", 30) + e.pages.Bodies[ordinary] = recipestest.RecipePage("Ordinary Curry", "", "https://c.example/curry.jpg", 25) + 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) + } + for _, leak := range []string{token, "access_token", "example.com"} { + if strings.Contains(rec.Body.String(), leak) { + t.Errorf("candidates contain %q: %s", leak, rec.Body) + } + } + want := map[string]any{"Named Curry": "Curry House", "Bare Curry": nil, "Ordinary Curry": "kitchen.example"} + list, _ := body["candidates"].([]any) + if len(list) != len(want) { + t.Fatalf("candidates = %v, want %d", body, len(want)) + } + for _, c := range list { + c := c.(map[string]any) + title, _ := c["title"].(string) + site, ok := c["site_name"] + if w, known := want[title]; !known || !ok || site != w { + t.Errorf("candidate %q site_name = %#v (present %v), want %#v", title, site, ok, w) + } + } +} + func TestGetRecipeCandidatesSearchFails(t *testing.T) { t.Parallel() e := newDinnersEnv(t) diff --git a/internal/server/ideasource_test.go b/internal/server/ideasource_test.go index 2aae059..19111d0 100644 --- a/internal/server/ideasource_test.go +++ b/internal/server/ideasource_test.go @@ -16,7 +16,7 @@ import ( ) // sourceCase is a recipe page an idea grounds in: its link and the site -// name its page gives (empty for none, so the host is stored instead). +// name its page gives (empty for none, so no name is stored). type sourceCase struct { dish, link, site string // want is the idea's source_url and site_name; nil is null. @@ -33,7 +33,7 @@ const ( // sourceCases are a clean link, credited by its page's name, and links // carrying a token in the query and the userinfo whose pages name no site, -// so the stored name is the link's host. +// so no name is stored and, the links being unsafe, none is shown. var sourceCases = []sourceCase{ {"Chicken Curry", cleanSourceLink, "Curry House", cleanSourceLink, "Curry House"}, {"Beef Stew", "https://" + queryHost + "/stew?access_token=" + queryToken, "", nil, nil}, @@ -137,8 +137,9 @@ func legacyIdea(name, link, site string) map[string]any { } // A job of either idea kind whose result was stored before sources were -// made safe is served with the sources a job stored now holds: neither a -// token-bearing link nor a name taken from its host. +// made safe, and upgraded by migration 0020, is served with the sources a +// job stored now holds: neither a token-bearing link nor a name taken from +// its host. func TestJobServesLegacyIdeaResultsWithOnlySafeSources(t *testing.T) { t.Parallel() model, normalize, search, pages, images := newQuickFakes() @@ -162,7 +163,10 @@ func TestJobServesLegacyIdeaResultsWithOnlySafeSources(t *testing.T) { id, kind, string(js)); err != nil { t.Fatal(err) } - checkIdeaSources(t, h, id) + } + applySiteNameMigration(t, st) + for kind := range map[string]bool{ideas.KindFromIngredients: true, ideas.KindQuick: true} { + checkIdeaSources(t, h, "legacy-"+kind) } } diff --git a/internal/server/plans_test.go b/internal/server/plans_test.go index 9b62108..9e85ee8 100644 --- a/internal/server/plans_test.go +++ b/internal/server/plans_test.go @@ -108,7 +108,7 @@ func newPlansRouterTitled(t *testing.T, title shopping.Titler) plansEnv { t.Fatal(err) } h, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestSearch(t), newTestAI(t), chatSvc, - newTestDinners(t), newTestSuggest(t), svc, shop, googleSvc, newTestPush(t), newTestIdeas(t), newTestHome(t), runner, newTestPhotos(t), fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}}) + newTestDinnersOn(t, st), newTestSuggest(t), svc, shop, googleSvc, newTestPush(t), newTestIdeas(t), newTestHome(t), runner, newTestPhotos(t), fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}}) if err != nil { t.Fatal(err) } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index c46ff0f..6628455 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -168,7 +168,12 @@ func newTestSearch(t *testing.T) *recipes.SearchSettingsService { // with its own unstarted job runner and a grounder that finds nothing. func newTestDinners(t *testing.T) *dinners.Service { t.Helper() - st := newTestStore(t) + return newTestDinnersOn(t, newTestStore(t)) +} + +// newTestDinnersOn is newTestDinners over st. +func newTestDinnersOn(t *testing.T, st *store.Store) *dinners.Service { + t.Helper() logger := slog.New(slog.NewTextHandler(io.Discard, nil)) runner, err := jobs.NewRunner(st, logger, 1) if err != nil { diff --git a/internal/server/shopping_test.go b/internal/server/shopping_test.go index ff8ec76..0002c43 100644 --- a/internal/server/shopping_test.go +++ b/internal/server/shopping_test.go @@ -75,7 +75,7 @@ func (e plansEnv) groundWeek(t *testing.T) { func (e plansEnv) archive(t *testing.T) { t.Helper() if _, err := e.st.Writer.ExecContext(t.Context(), - `UPDATE shopping_lists SET status = 'archived', frozen_at = '2026-10-19T04:05:00Z'`); err != nil { + `UPDATE shopping_lists SET status = 'archived', frozen_at = '2026-10-19T04:05:00Z'; UPDATE shopping_groups SET frozen_dinner_name = (SELECT name FROM dinners WHERE dinners.id = dinner_id) WHERE list_id IN (SELECT id FROM shopping_lists WHERE frozen_at IS NOT NULL)`); err != nil { t.Fatal(err) } } @@ -917,6 +917,73 @@ func TestSupplyArchiveUnfrozenPastWeek(t *testing.T) { } } +// TestSupplyArchiveKeepsFrozenNames freezes last week's Supply run, then +// renames its dinner and this week's through the dinners API: the frozen +// post and list keep the name the dinner had when frozen, while this +// week's list, not frozen, shows the new name. +func TestSupplyArchiveKeepsFrozenNames(t *testing.T) { + t.Parallel() + e := newPlansRouter(t) + if _, err := e.st.Writer.ExecContext(t.Context(), `DELETE FROM plan_days; DELETE FROM week_plans`); err != nil { + t.Fatal(err) + } + zone, err := household.LoadZone("America/New_York") // the env's household zone + if err != nil { + t.Fatal(err) + } + thisWeek := household.WeekStart(household.Today(time.Now(), zone)) + y, m, d := thisWeek.Date() + lastWeek := store.LocalDateOf(time.Date(y, m, d-7, 0, 0, 0, 0, time.UTC)) + + last := e.weekSupplyRun(t, lastWeek, "Beef Stew", shopIng{"stewing beef", "2", "lb"}) + current := e.weekSupplyRun(t, thisWeek, "Lemon Chicken", shopIng{"lemon", "1", ""}) + if err := e.shop.Freeze(t.Context(), lastWeek); err != nil { + t.Fatal(err) + } + for id, name := range map[string]string{ + last.Groups[0].Dinner.ID: "Sunday Beef Stew", + current.Groups[0].Dinner.ID: "Lemon Garlic Chicken", + } { + if status, body := callShopping(t, e, http.MethodPatch, dinnersPath+"/"+id, `{"name":"`+name+`"}`, nil); status != http.StatusOK { + t.Fatalf("PATCH dinner %s = %d %v", id, status, body) + } + } + + var got archiveJSON + if status, raw := callShopping(t, e, http.MethodGet, shoppingPath+"archive?by=week", "", &got); status != http.StatusOK { + t.Fatalf("GET archive = %d %v", status, raw) + } + var posts []string + for _, p := range got.Periods { + for _, post := range p.Posts { + posts = append(posts, fmt.Sprintf("%s %v current=%t", post.WeekStart, post.Dinners, post.Current)) + } + } + want := []string{ + thisWeek.String() + " [Lemon Garlic Chicken] current=true", + lastWeek.String() + " [Beef Stew] current=false", + } + if !reflect.DeepEqual(posts, want) { + t.Errorf("archive after rename =\n %s\nwant\n %s", strings.Join(posts, "\n "), strings.Join(want, "\n ")) + } + + for _, tc := range []struct { + week store.LocalDate + want string + }{ + {lastWeek, "Monday " + lastWeek.String() + " Beef Stew: stewing beef [2 lb] needed"}, + {thisWeek, "Monday " + thisWeek.String() + " Lemon Garlic Chicken: lemon [1] needed"}, + } { + var l supplyRunJSON + if status, body := callShopping(t, e, http.MethodGet, shoppingPath+tc.week.String(), "", &l); status != http.StatusOK { + t.Fatalf("GET %s = %d %v", tc.week, status, body) + } + if got := l.render(); !reflect.DeepEqual(got, []string{tc.want}) { + t.Errorf("list %s after rename = %v, want [%s]", tc.week, got, tc.want) + } + } +} + // TestSupplyRunSendChat posts the week's Supply run to Google Chat once, // records sent_to_chat_at, answers a second send with the status as it // is, and posts again only with resend. diff --git a/internal/shopping/archive.go b/internal/shopping/archive.go index 55ec094..a69cbfb 100644 --- a/internal/shopping/archive.go +++ b/internal/shopping/archive.go @@ -22,7 +22,9 @@ const ( // Freeze moves the Supply run of the week starting weekStart into the // Supply archive: its status becomes archived and frozen_at is set, after -// which it is read-only (Generate and SetNeeded are ErrFrozen). Freezing a +// which it is read-only (Generate and SetNeeded are ErrFrozen). The list +// keeps the names its dinners have as it is frozen, so its post in the +// Supply archive shows them whatever a dinner is renamed later. Freezing a // frozen list does nothing and is no error. // // The week must have ended in the household's time zone: a week not yet @@ -79,14 +81,25 @@ func (s *Service) freeze(ctx context.Context, weekStart store.LocalDate, now tim "week", list.WeekStart, "list", list.ID, "error", err) } } - // A list frozen meanwhile is left as it is: the freeze is done. + // A list frozen meanwhile is left as it is, its names as they were + // frozen: the freeze is done. stamp := store.Now().String() - if _, err := q.FreezeShoppingList(ctx, db.FreezeShoppingListParams{ - FrozenAt: sql.NullString{String: stamp, Valid: true}, UpdatedAt: stamp, ID: list.ID, - }); err != nil { - return fmt.Errorf("shopping: freeze %s: %w", weekStart, err) - } - return nil + return s.st.WithTx(ctx, func(tx *sql.Tx) error { + q := db.New(tx) + n, err := q.FreezeShoppingList(ctx, db.FreezeShoppingListParams{ + FrozenAt: sql.NullString{String: stamp, Valid: true}, UpdatedAt: stamp, ID: list.ID, + }) + if err != nil { + return fmt.Errorf("shopping: freeze %s: %w", weekStart, err) + } + if n == 0 { + return nil + } + if err := q.FreezeShoppingGroupNames(ctx, list.ID); err != nil { + return fmt.Errorf("shopping: freeze %s: keep dinner names: %w", weekStart, err) + } + return nil + }) } // freezeTitleJob is the title job of list as Generate would enqueue it: @@ -145,7 +158,8 @@ type Post struct { Label string // Title is nil while the list is untitled, when Label stands in. Title *string - // Dinners are the names of the list's dinners, in day order. + // Dinners are the names of the list's dinners, in day order: as they + // were when the list was frozen, or as they are now while it is not. Dinners []string // Items is how many items the list has, and OnHand how many of them // the household marked "have it". diff --git a/internal/shopping/archive_test.go b/internal/shopping/archive_test.go index 977bab4..bb67b89 100644 --- a/internal/shopping/archive_test.go +++ b/internal/shopping/archive_test.go @@ -425,7 +425,7 @@ func TestArchiveGroups(t *testing.T) { if _, err := e.st.Writer.Exec(`UPDATE shopping_lists SET title = 'Chili, Noodles & Co' WHERE week_start = '2026-12-28'`); err != nil { t.Fatal(err) } - if _, err := e.st.Writer.Exec(`UPDATE shopping_lists SET status = 'archived', frozen_at = '2027-01-03T11:05:00.000Z' WHERE week_start < '2027-01-04'`); err != nil { + if _, err := e.st.Writer.Exec(`UPDATE shopping_lists SET status = 'archived', frozen_at = '2027-01-03T11:05:00.000Z' WHERE week_start < '2027-01-04'; UPDATE shopping_groups SET frozen_dinner_name = (SELECT name FROM dinners WHERE dinners.id = dinner_id) WHERE list_id IN (SELECT id FROM shopping_lists WHERE frozen_at IS NOT NULL)`); err != nil { t.Fatal(err) } diff --git a/internal/shopping/notify_test.go b/internal/shopping/notify_test.go index 873f3a1..5d43c10 100644 --- a/internal/shopping/notify_test.go +++ b/internal/shopping/notify_test.go @@ -152,7 +152,7 @@ func TestSendToChatFailuresRecordNothing(t *testing.T) { t.Errorf("after failed sends sent_to_chat_at = %v, chat_send_unconfirmed_at = %v, %v, want both unset", l.SentToChatAt, l.ChatSendUnconfirmedAt, err) } - if _, err := e.st.Writer.Exec(`UPDATE shopping_lists SET status = 'archived', frozen_at = '2026-10-19T04:05:00Z'`); err != nil { + if _, err := e.st.Writer.Exec(`UPDATE shopping_lists SET status = 'archived', frozen_at = '2026-10-19T04:05:00Z'; UPDATE shopping_groups SET frozen_dinner_name = (SELECT name FROM dinners WHERE dinners.id = dinner_id) WHERE list_id IN (SELECT id FROM shopping_lists WHERE frozen_at IS NOT NULL)`); err != nil { t.Fatal(err) } posts := len(e.chat.Posts()) diff --git a/internal/shopping/shopping.go b/internal/shopping/shopping.go index 595d321..24a1743 100644 --- a/internal/shopping/shopping.go +++ b/internal/shopping/shopping.go @@ -492,7 +492,9 @@ func (s *Service) Get(ctx context.Context, weekStart store.LocalDate) (l List, e return readList(ctx, q, list) } -// readList reads the groups and items of list through q. +// readList reads the groups and items of list through q. A group of a +// frozen list is named as its dinner was when the list was frozen; one +// with no such name is an error. func readList(ctx context.Context, q *db.Queries, list db.ShoppingList) (List, error) { rows, err := q.ListShoppingGroupDinners(ctx, list.ID) if err != nil { @@ -512,8 +514,15 @@ func readList(ctx context.Context, q *db.Queries, list db.ShoppingList) (List, e groups := make([]Group, 0, len(rows)) for _, r := range rows { g := r.ShoppingGroup + name := r.DinnerName + if frozen(list) { + if !g.FrozenDinnerName.Valid { + return List{}, fmt.Errorf("shopping: group %s of frozen list %s has no frozen dinner name", g.ID, list.WeekStart) + } + name = g.FrozenDinnerName.String + } groups = append(groups, Group{ - ID: g.ID, DinnerID: g.DinnerID, DinnerName: r.DinnerName, Date: g.Date, Position: int(g.Position), + ID: g.ID, DinnerID: g.DinnerID, DinnerName: name, Date: g.Date, Position: int(g.Position), Items: byGroup[g.ID], }) } diff --git a/internal/shopping/shopping_test.go b/internal/shopping/shopping_test.go index 9f359b6..7fa4514 100644 --- a/internal/shopping/shopping_test.go +++ b/internal/shopping/shopping_test.go @@ -692,7 +692,7 @@ func TestGenerateRefusesFrozenList(t *testing.T) { e.dinner(t, "Pad Thai", ing{"rice noodles", "8", "oz"}) e.plan(t, "Beef Chili") e.generate(t) - if _, err := e.st.Writer.Exec(`UPDATE shopping_lists SET status = 'archived', frozen_at = '2026-10-19T04:05:00Z'`); err != nil { + if _, err := e.st.Writer.Exec(`UPDATE shopping_lists SET status = 'archived', frozen_at = '2026-10-19T04:05:00Z'; UPDATE shopping_groups SET frozen_dinner_name = (SELECT name FROM dinners WHERE dinners.id = dinner_id) WHERE list_id IN (SELECT id FROM shopping_lists WHERE frozen_at IS NOT NULL)`); err != nil { t.Fatal(err) } e.swap(t, "2026-10-12", "Pad Thai") @@ -834,7 +834,7 @@ func TestSetNeededRefusesFrozenList(t *testing.T) { e.dinner(t, "Beef Chili", ing{"ground beef", "1", "lb"}) e.plan(t, "Beef Chili") l := e.generate(t).List - if _, err := e.st.Writer.Exec(`UPDATE shopping_lists SET status = 'archived', frozen_at = '2026-10-19T04:05:00Z'`); err != nil { + if _, err := e.st.Writer.Exec(`UPDATE shopping_lists SET status = 'archived', frozen_at = '2026-10-19T04:05:00Z'; UPDATE shopping_groups SET frozen_dinner_name = (SELECT name FROM dinners WHERE dinners.id = dinner_id) WHERE list_id IN (SELECT id FROM shopping_lists WHERE frozen_at IS NOT NULL)`); err != nil { t.Fatal(err) } diff --git a/internal/shopping/title_test.go b/internal/shopping/title_test.go index 701bfa7..4ac4729 100644 --- a/internal/shopping/title_test.go +++ b/internal/shopping/title_test.go @@ -239,7 +239,7 @@ func TestGenerateTitleNotStoredOnFrozenList(t *testing.T) { e := titleWeek(t) e.model.QueueGenerate(titled("Noodles, Tacos and Chili")) e.beforeTitle = func() { - if _, err := e.st.Writer.Exec(`UPDATE shopping_lists SET status = 'archived', frozen_at = '2026-10-19T04:05:00Z'`); err != nil { + if _, err := e.st.Writer.Exec(`UPDATE shopping_lists SET status = 'archived', frozen_at = '2026-10-19T04:05:00Z'; UPDATE shopping_groups SET frozen_dinner_name = (SELECT name FROM dinners WHERE dinners.id = dinner_id) WHERE list_id IN (SELECT id FROM shopping_lists WHERE frozen_at IS NOT NULL)`); err != nil { t.Error(err) } } diff --git a/internal/store/migrate_test.go b/internal/store/migrate_test.go index bf09f0c..cd60a15 100644 --- a/internal/store/migrate_test.go +++ b/internal/store/migrate_test.go @@ -2,6 +2,7 @@ package store import ( "context" + "fmt" "io/fs" "reflect" "slices" @@ -176,8 +177,8 @@ func TestMigrateEmbeddedMigrations(t *testing.T) { if err := Migrate(context.Background(), s.Writer, migrations.FS); err != nil { t.Fatal(err) } - if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed" { - t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed", got) + if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed,0018_recipe_item_position_unique,0019_shopping_group_frozen_name,0020_site_name_from_page" { + t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed,0018_recipe_item_position_unique,0019_shopping_group_frozen_name,0020_site_name_from_page", got) } } @@ -197,8 +198,8 @@ func TestMigrateBootstrapThenEmbeddedInitApplies(t *testing.T) { if err := Migrate(ctx, s.Writer, migrations.FS); err != nil { t.Fatal(err) } - if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed" { - t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed", got) + if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed,0018_recipe_item_position_unique,0019_shopping_group_frozen_name,0020_site_name_from_page" { + t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed,0018_recipe_item_position_unique,0019_shopping_group_frozen_name,0020_site_name_from_page", got) } if !tableExists(t, s, "dinners") { t.Error("table from 0001_init was not created") @@ -922,3 +923,227 @@ func TestMigrateShoppingListChatSendUnconfirmed(t *testing.T) { t.Errorf("shopping list after 0017 = %s, want %s", got, want) } } + +// seedRecipeItems inserts two recipes, each with an ingredient and a step +// at position 0, then the extra statements. +func seedRecipeItems(t *testing.T, s *Store, extra ...string) { + t.Helper() + stmts := []string{ + `INSERT INTO recipes (id, source_url, title, raw_jsonld, fetched_at, created_at, updated_at) + VALUES ('recipe-1', 'https://example.com/chili', 'Chili', '{}', 'f', 'c', 'u')`, + `INSERT INTO recipes (id, source_url, title, raw_jsonld, fetched_at, created_at, updated_at) + VALUES ('recipe-2', 'https://example.com/curry', 'Curry', '{}', 'f', 'c', 'u')`, + `INSERT INTO recipe_ingredients (id, recipe_id, position, raw_text, created_at, updated_at) + VALUES ('ing-1', 'recipe-1', 0, '1 onion', 'c', 'u'), ('ing-2', 'recipe-2', 0, '1 lime', 'c', 'u')`, + `INSERT INTO recipe_steps (id, recipe_id, position, text, created_at, updated_at) + VALUES ('step-1', 'recipe-1', 0, 'Chop.', 'c', 'u'), ('step-2', 'recipe-2', 0, 'Squeeze.', 'c', 'u')`, + } + for _, stmt := range append(stmts, extra...) { + if _, err := s.Writer.Exec(stmt); err != nil { + t.Fatalf("seed: %v", err) + } + } +} + +// TestMigrateRecipeItemPositionUnique applies 0018 over two recipes with +// items at the same position: each recipe keeps its items, and a second +// ingredient or step at a position its recipe already uses is refused. +func TestMigrateRecipeItemPositionUnique(t *testing.T) { + ctx := context.Background() + s := openTestStore(t) + migrateBefore(t, s, "0018") + seedRecipeItems(t, s) + + if err := Migrate(ctx, s.Writer, migrations.FS); err != nil { + t.Fatal(err) + } + + var got string + if err := s.Reader.QueryRowContext(ctx, `SELECT + (SELECT group_concat(id || '@' || recipe_id || ':' || position, ',') FROM (SELECT * FROM recipe_ingredients ORDER BY id)) || ' / ' || + (SELECT group_concat(id || '@' || recipe_id || ':' || position, ',') FROM (SELECT * FROM recipe_steps ORDER BY id))`).Scan(&got); err != nil { + t.Fatal(err) + } + if want := "ing-1@recipe-1:0,ing-2@recipe-2:0 / step-1@recipe-1:0,step-2@recipe-2:0"; got != want { + t.Errorf("recipe items after 0018 = %s, want %s", got, want) + } + + for _, tc := range []struct{ name, stmt string }{ + {"second ingredient at a position", `INSERT INTO recipe_ingredients (id, recipe_id, position, raw_text, created_at, updated_at) + VALUES ('ing-3', 'recipe-1', 0, '2 onions', 'c', 'u')`}, + {"second step at a position", `INSERT INTO recipe_steps (id, recipe_id, position, text, created_at, updated_at) + VALUES ('step-3', 'recipe-1', 0, 'Dice.', 'c', 'u')`}, + {"ingredient moved onto another's position", `UPDATE recipe_ingredients SET recipe_id = 'recipe-1' WHERE id = 'ing-2'`}, + {"step moved onto another's position", `UPDATE recipe_steps SET recipe_id = 'recipe-1' WHERE id = 'step-2'`}, + } { + if _, err := s.Writer.Exec(tc.stmt); err == nil || !strings.Contains(err.Error(), "UNIQUE constraint failed") { + t.Errorf("%s: error %v, want UNIQUE constraint failed", tc.name, err) + } + } +} + +// TestMigrateRecipeItemPositionUniqueRefusesDuplicate applies 0018 over a +// recipe already holding two ingredients, or two steps, at one position: +// its order is unknown, so the migration fails and is not recorded. +func TestMigrateRecipeItemPositionUniqueRefusesDuplicate(t *testing.T) { + for _, tc := range []struct{ table, stmt string }{ + {"recipe_ingredients", `INSERT INTO recipe_ingredients (id, recipe_id, position, raw_text, created_at, updated_at) + VALUES ('ing-3', 'recipe-1', 0, '2 onions', 'c', 'u')`}, + {"recipe_steps", `INSERT INTO recipe_steps (id, recipe_id, position, text, created_at, updated_at) + VALUES ('step-3', 'recipe-1', 0, 'Dice.', 'c', 'u')`}, + } { + t.Run(tc.table, func(t *testing.T) { + ctx := context.Background() + s := openTestStore(t) + migrateBefore(t, s, "0018") + seedRecipeItems(t, s, tc.stmt) + + err := Migrate(ctx, s.Writer, migrations.FS) + if err == nil || !strings.Contains(err.Error(), "UNIQUE constraint failed: "+tc.table+".recipe_id, "+tc.table+".position") { + t.Fatalf("Migrate = %v, want the duplicate position refused", err) + } + var n int + if err := s.Reader.QueryRowContext(ctx, `SELECT count(*) FROM schema_migrations WHERE version = '0018_recipe_item_position_unique'`).Scan(&n); err != nil { + t.Fatal(err) + } + if n != 0 { + t.Error("0018 recorded as applied") + } + }) + } +} + +// TestMigrateShoppingGroupFrozenName applies 0019 over a frozen list and +// a current one: the frozen list's group takes its dinner's name now, the +// current list's group has none, and a frozen name is never empty. +func TestMigrateShoppingGroupFrozenName(t *testing.T) { + ctx := context.Background() + s := openTestStore(t) + migrateBefore(t, s, "0019") + for _, stmt := range []string{ + `INSERT INTO dinners (id, name, normalized_name, status, origin, is_favorite, grounding_status, created_at, updated_at) + VALUES ('dinner-1', 'Beef Stew', 'beef stew', 'regular', 'regular', 0, 'manual', 'c', 'u'), + ('dinner-2', 'Lemon Chicken', 'lemon chicken', 'regular', 'regular', 0, 'manual', 'c', 'u')`, + `INSERT INTO week_plans (id, week_start, status, created_at, updated_at) + VALUES ('plan-1', '2026-10-05', 'synced', 'c', 'u'), + ('plan-2', '2026-10-12', 'synced', 'c', 'u')`, + `INSERT INTO shopping_lists (id, week_start, plan_id, status, frozen_at, created_at, updated_at) + VALUES ('list-1', '2026-10-05', 'plan-1', 'archived', 'f', 'c', 'u'), + ('list-2', '2026-10-12', 'plan-2', 'current', NULL, 'c', 'u')`, + `INSERT INTO shopping_groups (id, list_id, dinner_id, date, position, created_at, updated_at) + VALUES ('group-1', 'list-1', 'dinner-1', '2026-10-05', 0, 'c', 'u'), + ('group-2', 'list-2', 'dinner-2', '2026-10-12', 0, 'c', 'u')`, + } { + if _, err := s.Writer.Exec(stmt); err != nil { + t.Fatalf("seed: %v", err) + } + } + + if err := Migrate(ctx, s.Writer, migrations.FS); err != nil { + t.Fatal(err) + } + + rows, err := s.Reader.QueryContext(ctx, `SELECT id || '|' || COALESCE(frozen_dinner_name, 'NULL') || '|' || updated_at FROM shopping_groups ORDER BY id`) + if err != nil { + t.Fatal(err) + } + defer rows.Close() + var got []string + for rows.Next() { + var row string + if err := rows.Scan(&row); err != nil { + t.Fatal(err) + } + got = append(got, row) + } + if err := rows.Err(); err != nil { + t.Fatal(err) + } + if want := []string{"group-1|Beef Stew|u", "group-2|NULL|u"}; strings.Join(got, ",") != strings.Join(want, ",") { + t.Errorf("shopping groups after 0019 = %v, want %v", got, want) + } + if _, err := s.Writer.Exec(`UPDATE shopping_groups SET frozen_dinner_name = '' WHERE id = 'group-1'`); err == nil || !strings.Contains(err.Error(), "CHECK constraint failed") { + t.Errorf("empty frozen name: error %v, want CHECK constraint failed", err) + } +} + +// TestMigrateSiteNameFromPage applies 0020 over recipes and idea job +// results stored when a page naming no site was stored under its host: a +// stored name that could be a host — one with no space that is dotted, an +// IP address or spelled in the recipe's own link — is cleared, so every +// stored name is one a page gave; a name with a space is kept, as is +// everything else in the rows. +func TestMigrateSiteNameFromPage(t *testing.T) { + ctx := context.Background() + s := openTestStore(t) + migrateBefore(t, s, "0020") + sites := []struct { + stored any + link string + want any + }{ + {"private-token.example.com", "https://private-token.example.com/stew", nil}, + {"redirect_token.example.net", "https://kitchen.example/stew", nil}, + {"bücher.example", "https://kitchen.example/stew", nil}, + {"Food.com", "https://www.food.com/stew", nil}, + {"203.0.113.7", "https://203.0.113.7/stew", nil}, + {"2001:db8::1", "https://[2001:db8::1]/stew", nil}, + {"TokenHost", "https://www.tokenhost/stew", nil}, + {"Delish", "https://www.delish.com/stew", nil}, + {"Delish", "https://kitchen.example/stew", "Delish"}, + {"Curry House", "https://curry-house.example/stew", "Curry House"}, + {"Mr. Food Test Kitchen", "https://kitchen.example/stew", "Mr. Food Test Kitchen"}, + {nil, "https://kitchen.example/stew", nil}, + } + for i, c := range sites { + if _, err := s.Writer.ExecContext(ctx, `INSERT INTO recipes (id, source_url, site_name, title, raw_jsonld, fetched_at, created_at, updated_at) + VALUES (?, ?, ?, 'Stew', '{}', 'f', 'c', 'u')`, fmt.Sprintf("recipe-%d", i), fmt.Sprintf("%s?row=%d", c.link, i), c.stored); err != nil { + t.Fatal(err) + } + } + const ( + legacy = `{"stage":"done","ideas":[{"name":"A","site_name":"query-host.example","source_url":"https://query-host.example/a?t=1"},{"name":"B","site_name":"Curry House","source_url":"https://b.example/b"},{"name":"C","site_name":null,"source_url":null},{"name":"D","site_name":"Kitchen","source_url":"https://kitchen.example/d"}],"dropped":1}` + cleaned = `{"stage":"done","ideas":[{"name":"A","site_name":null,"source_url":"https://query-host.example/a?t=1"},{"name":"B","site_name":"Curry House","source_url":"https://b.example/b"},{"name":"C","site_name":null,"source_url":null},{"name":"D","site_name":null,"source_url":"https://kitchen.example/d"}],"dropped":1}` + other = `{"ideas":[{"site_name":"host.example"}]}` + ) + for _, j := range [][3]string{ + {"job-ideas", "ideas_from_ingredients", legacy}, + {"job-quick", "quick_ideas", legacy}, + {"job-other", "ground_dinner", other}, + {"job-empty", "quick_ideas", `{"stage":"searching","ideas":[]}`}, + } { + if _, err := s.Writer.ExecContext(ctx, `INSERT INTO jobs (id, kind, status, payload, result, created_at, updated_at) + VALUES (?, ?, 'succeeded', '{}', ?, 'c', 'u')`, j[0], j[1], j[2]); err != nil { + t.Fatal(err) + } + } + if _, err := s.Writer.ExecContext(ctx, `INSERT INTO jobs (id, kind, status, payload, created_at, updated_at) + VALUES ('job-running', 'quick_ideas', 'running', '{}', 'c', 'u')`); err != nil { + t.Fatal(err) + } + + if err := Migrate(ctx, s.Writer, migrations.FS); err != nil { + t.Fatal(err) + } + + for i, c := range sites { + var got any + var title string + if err := s.Reader.QueryRowContext(ctx, `SELECT site_name, title FROM recipes WHERE id = ?`, fmt.Sprintf("recipe-%d", i)).Scan(&got, &title); err != nil { + t.Fatal(err) + } + if got != c.want || title != "Stew" { + t.Errorf("recipe stored with site_name %v: site_name %v, title %q after 0020; want %v, Stew", c.stored, got, title, c.want) + } + } + for id, want := range map[string]any{"job-ideas": cleaned, "job-quick": cleaned, "job-other": other, + "job-empty": `{"stage":"searching","ideas":[]}`, "job-running": nil} { + var got any + if err := s.Reader.QueryRowContext(ctx, `SELECT result FROM jobs WHERE id = ?`, id).Scan(&got); err != nil { + t.Fatal(err) + } + if got != want { + t.Errorf("%s result after 0020 = %v, want %v", id, got, want) + } + } +} diff --git a/internal/store/schema_test.go b/internal/store/schema_test.go index 1607353..f57c212 100644 --- a/internal/store/schema_test.go +++ b/internal/store/schema_test.go @@ -408,6 +408,8 @@ func TestSchemaUniqueConstraintsRejectDuplicates(t *testing.T) { {"notification_outbox", []string{"event_key", "channel"}}, {"schedule_runs", []string{"schedule", "period_key"}}, {"notification_prefs", []string{"event"}}, + {"recipe_ingredients", []string{"recipe_id", "position"}}, + {"recipe_steps", []string{"recipe_id", "position"}}, } s := seededStore(t) for _, c := range cases { @@ -445,6 +447,8 @@ func TestSchemaCompositeUniqueAllowsPartialOverlap(t *testing.T) { {"same plan_date, different dinner", "rotation_reviews", "plan_date", "dinners", map[string]any{"dinner_id": rowID("dinners", 1)}}, {"same event_key, different channel", "notification_outbox", "event_key", "", map[string]any{"channel": "chat"}}, {"same period_key, different schedule", "schedule_runs", "period_key", "", map[string]any{"schedule": "week_rollover"}}, + {"same position, different recipe", "recipe_ingredients", "position", "recipes", map[string]any{"recipe_id": rowID("recipes", 1)}}, + {"same position, different recipe", "recipe_steps", "position", "recipes", map[string]any{"recipe_id": rowID("recipes", 1)}}, } s := seededStore(t) for _, c := range cases { diff --git a/internal/suggest/batch.go b/internal/suggest/batch.go index a6e130c..b88d246 100644 --- a/internal/suggest/batch.go +++ b/internal/suggest/batch.go @@ -54,7 +54,8 @@ func (r batchResult) PublicJSON() ([]byte, error) { return json.Marshal(r) } // no recipe grounds is dropped; any other grounding failure fails the // batch, as does suggest_side_quests failing, with no Side quest made up // instead. The batch ends done or failed. When recording the batch's end -// fails, so does the job; when it times out at shutdown, the job is left +// fails, so does the job, with what the batch stored, and any +// new_suggestions it emitted, kept; when it times out at shutdown, the job is left // running and the batch with it, for the next start to fail. // // When a batch stores its Side quests depends on whether it notifies (see @@ -64,7 +65,12 @@ func (r batchResult) PublicJSON() ([]byte, error) { return json.Marshal(r) } // Side quest stored. So the household is told of exactly what the batch // added, once; and a batch that fails at any round, including a later // round's normalize_recipe, stores nothing and sends nothing, leaving no -// Side quest in the deck that was never announced. A replenish batch, +// Side quest in the deck that was never announced. After each round it +// rechecks every Side quest it holds (see eligible) and lets go of those +// the household's changes made ineligible, so a later round grounds a +// spare Side quest in place of each; only a change committed between the +// last recheck and the store can still leave the batch short of +// BatchSize while spare Side quests remain. A replenish batch, // which tops up the deck as the household swipes and never notifies, // stores each round's Side quests before the next round starts, so the // deck fills as it goes; those stored before a failure stay in the deck. @@ -146,7 +152,13 @@ func (s *Service) fill(ctx context.Context, trigger string, job BatchJob, publis } res.Dropped += len(round) - len(grounded) if note != nil { - held = append(held, grounded...) + // Recheck every held Side quest, not just this round's, so + // one the household's changes made ineligible while this + // round grounded is replaced by a later round, not dropped by + // the store once no round is left to replace it. + if held, err = s.recheck(ctx, &res, append(held, grounded...)); err != nil { + return batchResult{}, err + } } else if err := s.storeInto(ctx, &res, nil, grounded); err != nil { return batchResult{}, err } @@ -163,6 +175,22 @@ func (s *Service) fill(ctx context.Context, trigger string, job BatchJob, publis return res, nil } +// recheck returns, in order, those of a notifying batch's held Side +// quests that store would still keep (see eligible), adding those left +// out to res.Dropped. +func (s *Service) recheck(ctx context.Context, res *batchResult, held []groundedQuest) ([]groundedQuest, error) { + kept, err := s.eligible(ctx, db.New(s.st.Reader), res.BatchID, held) + if err != nil { + return nil, err + } + res.Dropped += len(held) - len(kept) + out := make([]groundedQuest, len(kept)) + for i, k := range kept { + out[i] = k.groundedQuest + } + return out, nil +} + // storeInto stores quests for res's batch with note (see store) and adds // the suggestions stored, and the Side quests left out, to res. func (s *Service) storeInto(ctx context.Context, res *batchResult, note *notice, quests []groundedQuest) error { @@ -277,11 +305,11 @@ func (s *Service) ground(ctx context.Context, quests []skills.SideQuest) ([]grou // store stores each grounded Side quest of batchID as a dinner with status // suggested and origin side_quest, grounded in its recipe, and a pending // suggestion citing its Regulars by id, all in one transaction, and -// returns the suggestion ids. A Side quest is left out when, since -// suggest_side_quests ran, a dinner it repeats was added, a suggestion it -// repeats passed or a Rotation check dropped, or a Regular it cites renamed -// or archived. Passed names are read in the same transaction as the store, -// so a Pass or drop committed before it is never missed. +// returns the suggestion ids. A Side quest eligible leaves out is not +// stored: since suggest_side_quests ran, a dinner it repeats was added, a +// suggestion it repeats passed or a Rotation check dropped, or a Regular +// it cites renamed or archived. eligible reads in the same transaction as +// the store, so a Pass or drop committed before it is never missed. // // When note is not nil and a Side quest is stored, new_suggestions keyed by // note, counting and naming those stored, is emitted in the same @@ -295,28 +323,13 @@ func (s *Service) store(ctx context.Context, batchID string, note *notice, quest var stored []string err := s.st.WithTx(ctx, func(tx *sql.Tx) error { q := db.New(tx) - existing, err := q.ListUnarchivedDinners(ctx) + kept, err := s.eligible(ctx, q, batchID, quests) if err != nil { - return fmt.Errorf("suggest: list dinners for dedupe: %w", err) + return err } - names, err := q.ListPassedNames(ctx) - if err != nil { - return fmt.Errorf("suggest: list passed names for dedupe: %w", err) - } - regulars := map[string][]string{} - for _, row := range existing { - names = append(names, row.Dinner.Name) - if row.Dinner.Status == dinners.StatusRegular { - regulars[row.Dinner.Name] = append(regulars[row.Dinner.Name], row.Dinner.ID) - } - } - for _, gq := range quests { - cited, ok := citedIDs(gq.quest.CitedRegulars, regulars) - if !ok || repeats(gq.quest.Name, names) { - s.logger.InfoContext(ctx, "suggested dinner dropped: changed while grounding", "batch_id", batchID) - continue - } - citedJSON, err := json.Marshal(cited) + for _, k := range kept { + gq := k.groundedQuest + citedJSON, err := json.Marshal(k.cited) if err != nil { return fmt.Errorf("suggest: encode cited Regulars: %w", err) } @@ -354,7 +367,6 @@ func (s *Service) store(ctx context.Context, batchID string, note *notice, quest }); err != nil { return fmt.Errorf("suggest: create suggestion: %w", err) } - names = append(names, d.Name) ids = append(ids, id) stored = append(stored, d.Name) } @@ -372,6 +384,50 @@ func (s *Service) store(ctx context.Context, batchID string, note *notice, quest return ids, nil } +// eligibleQuest is a Side quest store would keep, with the ids of the +// Regulars it cites. +type eligibleQuest struct { + groundedQuest + cited []string +} + +// eligible returns, in order, those of batchID's quests that q's dinners +// and passed names still allow, each with the ids of the Regulars it +// cites. A Side quest is left out when a dinner it repeats is unarchived, +// a suggestion it repeats passed or a Rotation check dropped, a Regular it +// cites is renamed or archived, or an earlier Side quest of quests kept +// repeats it. store checks with this in its transaction; a notifying +// batch checks the Side quests it holds with it after each round (see +// fill). +func (s *Service) eligible(ctx context.Context, q *db.Queries, batchID string, quests []groundedQuest) ([]eligibleQuest, error) { + existing, err := q.ListUnarchivedDinners(ctx) + if err != nil { + return nil, fmt.Errorf("suggest: list dinners for dedupe: %w", err) + } + names, err := q.ListPassedNames(ctx) + if err != nil { + return nil, fmt.Errorf("suggest: list passed names for dedupe: %w", err) + } + regulars := map[string][]string{} + for _, row := range existing { + names = append(names, row.Dinner.Name) + if row.Dinner.Status == dinners.StatusRegular { + regulars[row.Dinner.Name] = append(regulars[row.Dinner.Name], row.Dinner.ID) + } + } + var kept []eligibleQuest + for _, gq := range quests { + cited, ok := citedIDs(gq.quest.CitedRegulars, regulars) + if !ok || repeats(gq.quest.Name, names) { + s.logger.InfoContext(ctx, "suggested dinner dropped: changed while grounding", "batch_id", batchID) + continue + } + names = append(names, gq.quest.Name) + kept = append(kept, eligibleQuest{groundedQuest: gq, cited: cited}) + } + return kept, nil +} + // citedIDs returns the ids of the Regulars named cited, false when one // names no Regular or more than one. func citedIDs(cited []string, regulars map[string][]string) ([]string, bool) { diff --git a/internal/suggest/suggest_test.go b/internal/suggest/suggest_test.go index fc0f79d..24e38ec 100644 --- a/internal/suggest/suggest_test.go +++ b/internal/suggest/suggest_test.go @@ -660,6 +660,74 @@ func TestBatchKeepsAtMostBatchSize(t *testing.T) { } } +// TestBatchReplacesSideQuestMadeIneligibleWhileHeld: a notifying batch +// holds each round's Side quests until its last round is done; when the +// household adds a dinner repeating one held from an earlier round while a +// later round grounds, the batch lets it go and grounds a spare Side quest +// in its place, so it still stores BatchSize and announces them once. +func TestBatchReplacesSideQuestMadeIneligibleWhileHeld(t *testing.T) { + for _, trigger := range []string{suggest.TriggerScheduled, suggest.TriggerManual} { + t.Run(trigger, func(t *testing.T) { + e := newEnv(t) + e.start(t) + // Fajita Dish B is ungroundable, so the first round holds + // BatchSize-1 and a second round grounds the next; the last is + // the spare a third round grounds in place of Fajita Dish A. + var quests, want []string + for i := range suggest.BatchSize + 2 { + name := fmt.Sprintf("Fajita Dish %c", 'A'+i) + e.search.Results[name] = nil + if i != 1 { + e.recipe(t, name) + } + quests = append(quests, sideQuest(name, "Like your Chicken Fajitas.", "Chicken Fajitas")) + if i > 1 { + want = append(want, name) + } + } + e.reply(quests...) + // The second round's search waits, so the household adds a + // dinner repeating Fajita Dish A, held from the first round, + // while that round grounds. + second := fmt.Sprintf("Fajita Dish %c", 'A'+suggest.BatchSize) + gate := make(chan struct{}) + e.search.Gates = map[string]chan struct{}{second: gate} + + b, key := e.startBatch(t, trigger) + deadline := time.Now().Add(10 * time.Second) + for !slices.Contains(e.search.Queries(), second) { + if time.Now().After(deadline) { + close(gate) + t.Fatalf("search for %s never ran", second) + } + time.Sleep(5 * time.Millisecond) + } + e.dinner(t, "Fajita Dish A", "regular") + close(gate) + + job := e.waitJob(t, b.JobID) + if job.Status != jobs.StatusSucceeded { + t.Fatalf("batch job %s, error %v", job.Status, job.Error) + } + var res batchResult + if err := json.Unmarshal(job.Result, &res); err != nil { + t.Fatal(err) + } + if len(res.SuggestionIDs) != suggest.BatchSize || res.Dropped != 2 { + t.Errorf("batch result = %+v, want %d stored and 2 dropped", res, suggest.BatchSize) + } + if got := e.deckNames(t); !slices.Equal(got, want) { + t.Errorf("deck = %v, want %v", got, want) + } + rows := notifytest.Rows(t, e.st, notify.NewSuggestions) + title := fmt.Sprintf("%d new Side quests are in", suggest.BatchSize) + if len(rows) != 1 || rows[0].Key != key || rows[0].Payload.Title != title { + t.Errorf("new_suggestions rows = %+v, want one keyed %s titled %q", rows, key, title) + } + }) + } +} + func TestDeckIsOldestFirst(t *testing.T) { e := newEnv(t) e.start(t) diff --git a/migrations/0018_recipe_item_position_unique.sql b/migrations/0018_recipe_item_position_unique.sql new file mode 100644 index 0000000..1d257d6 --- /dev/null +++ b/migrations/0018_recipe_item_position_unique.sql @@ -0,0 +1,9 @@ +-- 0018_recipe_item_position_unique: position is the only record of recipe +-- order, so a recipe never has two ingredients or two steps at one +-- position. Writers replace a recipe's items whole, numbered from 0, and +-- the detail read orders by position alone. A database already holding a +-- duplicate fails this migration rather than keep an order nobody can +-- trust. + +CREATE UNIQUE INDEX recipe_ingredients_recipe_position ON recipe_ingredients (recipe_id, position); +CREATE UNIQUE INDEX recipe_steps_recipe_position ON recipe_steps (recipe_id, position); diff --git a/migrations/0019_shopping_group_frozen_name.sql b/migrations/0019_shopping_group_frozen_name.sql new file mode 100644 index 0000000..8ea136f --- /dev/null +++ b/migrations/0019_shopping_group_frozen_name.sql @@ -0,0 +1,14 @@ +-- 0019_shopping_group_frozen_name: a Supply run frozen into the Supply +-- archive keeps the names its dinners had when it was frozen. A dinner can +-- be renamed at any time, so a frozen list reads each group's dinner name +-- from frozen_dinner_name, never from the dinner, as 0012 does for a Pass. +-- Only a group of a frozen list has one; a current list reads its dinners' +-- names live. A list frozen before this takes its dinners' names now. + +ALTER TABLE shopping_groups ADD COLUMN frozen_dinner_name TEXT CHECK (frozen_dinner_name <> ''); + +UPDATE shopping_groups +SET frozen_dinner_name = (SELECT dinners.name FROM dinners WHERE dinners.id = shopping_groups.dinner_id) +WHERE list_id IN ( + SELECT id FROM shopping_lists WHERE status = 'archived' OR frozen_at IS NOT NULL +); diff --git a/migrations/0020_site_name_from_page.sql b/migrations/0020_site_name_from_page.sql new file mode 100644 index 0000000..53b1beb --- /dev/null +++ b/migrations/0020_site_name_from_page.sql @@ -0,0 +1,41 @@ +-- 0020_site_name_from_page: recipes.site_name now holds only the name a +-- recipe page gives its site (its publisher's name or og:site_name), and is +-- null when the page names none; a client is credited with the link's host +-- only when dinners.SourceOf finds the link safe to show. Before this, a +-- page naming no site was stored under the host of the URL it was served +-- from, which may carry a personal token, and nothing recorded which a +-- stored name was. A stored name that could be a host is cleared here, so +-- every stored name is one a page gave: one with no space (a host has +-- none) that is dotted or holds a colon — a host name or an IP address — +-- or that appears in the recipe's own link, as a dotless host does. This +-- clears every name the read rule before it withheld, and more: a site +-- that names itself as a domain, such as "Food.com", or by a word its link +-- spells, such as "Delish" on delish.com, loses its stored name until its +-- recipe is fetched again, and is credited by its link's host meanwhile +-- when the link is safe. +-- +-- Idea job results stored before sources were made safe hold the grounded +-- recipe's site name and link as they were stored, and are cleared the +-- same way: an idea's stored site name is shown as it stands. The ideas +-- array is rebuilt from a plain scan of json_each, which yields its +-- elements in array order, so the ideas keep their order (sqlc's parser +-- refuses an ORDER BY inside json_group_array). + +UPDATE recipes SET site_name = NULL +WHERE instr(site_name, ' ') = 0 + AND (site_name GLOB '*[.:]*' OR instr(lower(source_url), lower(site_name)) > 0); + +UPDATE jobs SET result = json_set(result, '$.ideas', ( + SELECT json_group_array( + CASE + WHEN instr(json_extract(idea.value, '$.site_name'), ' ') = 0 + AND (json_extract(idea.value, '$.site_name') GLOB '*[.:]*' + OR instr(lower(json_extract(idea.value, '$.source_url')), lower(json_extract(idea.value, '$.site_name'))) > 0) + THEN json_set(idea.value, '$.site_name', json('null')) + ELSE json(idea.value) + END + ) + FROM json_each(jobs.result, '$.ideas') AS idea +)) +WHERE kind IN ('ideas_from_ingredients', 'quick_ideas') + AND json_type(result, '$.ideas') = 'array'; diff --git a/migrations/queries/recipes.sql b/migrations/queries/recipes.sql index bbca037..1f17450 100644 --- a/migrations/queries/recipes.sql +++ b/migrations/queries/recipes.sql @@ -48,7 +48,7 @@ INSERT INTO recipe_ingredients ( -- name: ListRecipeIngredients :many SELECT * FROM recipe_ingredients WHERE recipe_id = ? -ORDER BY position, id; +ORDER BY position; -- name: DeleteRecipeIngredients :exec DELETE FROM recipe_ingredients @@ -64,7 +64,7 @@ INSERT INTO recipe_steps ( -- name: ListRecipeSteps :many SELECT * FROM recipe_steps WHERE recipe_id = ? -ORDER BY position, id; +ORDER BY position; -- name: DeleteRecipeSteps :exec DELETE FROM recipe_steps diff --git a/migrations/queries/shopping.sql b/migrations/queries/shopping.sql index b4ade22..e8d144d 100644 --- a/migrations/queries/shopping.sql +++ b/migrations/queries/shopping.sql @@ -60,7 +60,8 @@ DELETE FROM shopping_items WHERE id = ?; -- name: ListShoppingGroupDinners :many --- The groups of list list_id with their dinner's name, in list order. +-- The groups of list list_id with their dinner's name now, in list order. +-- A group of a frozen list shows its frozen_dinner_name instead. SELECT sqlc.embed(shopping_groups), dinners.name AS dinner_name FROM shopping_groups JOIN dinners ON dinners.id = shopping_groups.dinner_id @@ -117,6 +118,13 @@ UPDATE shopping_lists SET status = 'archived', frozen_at = ?, updated_at = ? WHERE id = ? AND frozen_at IS NULL; +-- name: FreezeShoppingGroupNames :exec +-- Keeps the name each dinner of list list_id has now as its group's +-- frozen_dinner_name, as the list is frozen into the Supply archive. +UPDATE shopping_groups +SET frozen_dinner_name = (SELECT dinners.name FROM dinners WHERE dinners.id = shopping_groups.dinner_id) +WHERE list_id = ?; + -- name: ListShoppingListsThrough :many -- The Supply runs of the weeks starting on or before week_start, newest -- week first. diff --git a/web/src/dinners/api.ts b/web/src/dinners/api.ts index 2c0c081..84f8541 100644 --- a/web/src/dinners/api.ts +++ b/web/src/dinners/api.ts @@ -54,7 +54,9 @@ export type RecipeFix = { candidate_id: string } | { recipe_url: string } | { ma export type RecipeCandidate = { candidate_id: string title: string - site_name: string + // site_name is null when the server cannot name the page's site without + // leaking its link (dinners.SourceOf). + site_name: string | null photo_id: string | undefined minutes: number | null } @@ -130,7 +132,7 @@ function parseCandidate(where: string, body: unknown): RecipeCandidate { !isRecord(body) || typeof body.candidate_id !== 'string' || typeof body.title !== 'string' || - typeof body.site_name !== 'string' + (body.site_name !== null && typeof body.site_name !== 'string') ) { throw new Error(`${where}: response is not a recipe candidate`) } diff --git a/web/src/settings/RegularsSettingsPage.test.tsx b/web/src/settings/RegularsSettingsPage.test.tsx index afae963..adb1188 100644 --- a/web/src/settings/RegularsSettingsPage.test.tsx +++ b/web/src/settings/RegularsSettingsPage.test.tsx @@ -344,7 +344,7 @@ describe('RegularsSettingsPage', () => { minutes: 45, }, { candidate_id: 'https://cook.test/pizza', title: 'Sheet-pan pizza', site_name: 'Cook Test', photo_url: null, minutes: null }, - { candidate_id: 'https://food.test/p', title: 'Pizza margherita', site_name: 'Food Test', photo_url: null, minutes: 30 }, + { candidate_id: 'https://food.test/p', title: 'Pizza margherita', site_name: null, photo_url: null, minutes: 30 }, ], } @@ -372,7 +372,7 @@ describe('RegularsSettingsPage', () => { expect(options.map((option) => option.getAttribute('aria-label'))).toEqual([ 'Use Weeknight pizza from Example Kitchen', 'Use Sheet-pan pizza from Cook Test', - 'Use Pizza margherita from Food Test', + 'Use Pizza margherita', ]) expect(within(options[0]!).getByRole('img', { name: 'Weeknight pizza' })).toHaveAttribute('src', '/media/photos/p7-400.jpg') expect(within(options[0]!).getByText('Example Kitchen')).toBeInTheDocument() diff --git a/web/src/settings/RegularsSettingsPage.tsx b/web/src/settings/RegularsSettingsPage.tsx index 3c9eae0..5600b11 100644 --- a/web/src/settings/RegularsSettingsPage.tsx +++ b/web/src/settings/RegularsSettingsPage.tsx @@ -527,13 +527,13 @@ function PickCandidate({ id, onPick }: { id: string; onPick: (candidateId: strin