From 837c9924d1eea2f7e95c5a1bc0ee62b52487655a Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 19:24:47 -0400 Subject: [PATCH] =?UTF-8?q?Fix=20Feature=2012=20review=20findings=20F2?= =?UTF-8?q?=E2=80=93F8=20on=20Regular=20grounding=20and=20onboarding?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Grounding (F2–F5): - Each ground_dinner job carries the dinner's new grounding_revision (migration 0010). Renaming a pending Regular raises the revision, and a job only grounds and writes a dinner still at its revision, so a queued original job no longer grounds the new name or decides the outcome. - Background grounding keeps a method, minutes or difficulty a member saved while the Regular was pending, and fills only those left empty. - Grounding by hand clears the pasted recipe_url, and link deduplication uses the source_url of the attached recipe, else the pending link. - Archived dinners are not grounded: the job checks before any search, fetch or model call, and its final update requires an unarchived row. Onboarding (F6, F7): - Step 3 shows "Add Regulars" back to step 2 when the server refuses to finish with fewer than three Regulars. - Common picks are hidden by a copy of harness Normalize/Similar, checked against the same shared cases as the Go code. Docs (F8): ARCHITECTURE.md covers the three-Regular minimum, step-3 recovery, the grounding revision and the dedupe rules. F1 was fixed in 1304487; its test now also asserts the old link is never grounded and recipe_url stays cleared. Maestro task 5037ff86-181d-448e-927b-c2fd2258a25d. Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 42 +++- internal/ai/harness/dedupe_test.go | 35 +++ .../ai/harness/testdata/dedupe_cases.json | 119 ++++++++++ internal/db/db_test.go | 2 +- internal/db/dinners.sql.go | 197 +++++++++------- internal/db/models.go | 37 +-- internal/db/querier.go | 33 +-- internal/dinners/dinners.go | 100 ++++---- internal/dinners/export_test.go | 9 + internal/dinners/grounding.go | 50 ++-- internal/dinners/grounding_test.go | 217 ++++++++++++++++++ internal/store/migrate_test.go | 8 +- migrations/0010_dinner_grounding_revision.sql | 7 + migrations/queries/dinners.sql | 64 ++++-- web/src/dinners/names.test.ts | 20 ++ web/src/dinners/names.ts | 74 ++++++ web/src/onboarding/NotificationsStep.test.tsx | 36 +++ web/src/onboarding/NotificationsStep.tsx | 22 +- web/src/onboarding/Onboarding.module.css | 5 + web/src/onboarding/OnboardingPage.test.tsx | 8 + web/src/onboarding/RegularsStep.tsx | 10 +- web/src/settings/api.ts | 12 + 22 files changed, 875 insertions(+), 232 deletions(-) create mode 100644 internal/ai/harness/testdata/dedupe_cases.json create mode 100644 migrations/0010_dinner_grounding_revision.sql create mode 100644 web/src/dinners/names.test.ts create mode 100644 web/src/dinners/names.ts diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 5b5ed88..ee6f75b 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -96,7 +96,7 @@ Columns ending in `_ct` hold AES-256-GCM ciphertext (via `secretbox`). | `recipe_ingredients` | recipe_id, position, raw_text, name, qty, unit, note. | | `recipe_steps` | recipe_id, position, text. | | `photos` | source_url, local_path, width, height, sha256, fetched_at. | -| `dinners` | The meal record and its lifecycle: name, normalized_name, status (`suggested` / `quest_log` / `tried` / `regular` / `archived`), origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, recipe_url (a pasted recipe link awaiting grounding), photo_id, grounding_status (`pending` / `grounded` / `failed` / `manual`), grounding_error (why grounding failed), updated_by. | +| `dinners` | The meal record and its lifecycle: name, normalized_name, status (`suggested` / `quest_log` / `tried` / `regular` / `archived`), origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, recipe_url (a pasted recipe link awaiting grounding), photo_id, grounding_status (`pending` / `grounded` / `failed` / `manual`), grounding_error (why grounding failed), grounding_revision (raised when a pending dinner is renamed, so only the latest `ground_dinner` job may write it), updated_by. | | `suggestion_batches` | trigger, status, ai_run_id. | | `suggestions` | dinner_id, batch_id, reason, cited_regular_ids (JSON), status, decided_at. | | `week_plans` | week_start (unique), status (`draft` / `synced` / `changed`), ai_note, ai_run_id, calendar_synced_at. | @@ -187,8 +187,10 @@ automatic** (D-021). "Beef stew"); a link that names nothing is 400 — add it by name. A name is trimmed, at most 120 characters and must keep a word after `harness.Normalize`, which sets `normalized_name`. A name - `harness.Similar` to an unarchived dinner's, or a link one already has, - is 409 `duplicate_dinner`; archived names may be added again. + `harness.Similar` to an unarchived dinner's, or a link one already has + (the source_url of the recipe it is grounded in, else its pasted + `recipe_url`), is 409 `duplicate_dinner`; archived names may be added + again. `GET /api/v1/dinners?status=regular|quest_log` returns `{dinners: [...]}` ordered by normalized name, each with `photo_url` (`/media/photos/{photo_id}-400.jpg`, or null) and `grounding_status`; @@ -211,11 +213,19 @@ automatic** (D-021). `failed` with `grounding_error` — also the job's failure code — one of `no_recipe_found`, `recipe_unusable` (the pasted link), `search_not_configured`, `search_failed`, `ai_failed`, `interrupted` (the server stopped) or - `grounding_error`. It only writes a dinner still `pending`, so a fix-up - made meanwhile wins. At boot, after the job runner fails the jobs a - previous process left running, every dinner still `pending` with no - `ground_dinner` job queued or running is set `failed` with `interrupted`, - so no dinner stays pending with nothing left to ground it. + `grounding_error`. Each job carries the dinner's `grounding_revision` + from when it was enqueued; renaming a `pending` dinner clears its + `recipe_url`, raises the revision and enqueues a job for the new name. + A job for a dinner archived, no longer `pending` or at another revision + does no work, and a job only writes a dinner still unarchived, `pending` + and at its revision, so a fix-up made meanwhile wins and an earlier job + for an old name — queued or running — never decides the outcome. A + method, minutes_total or difficulty a member saved while the dinner was + `pending` is kept; grounding fills only those left empty. At boot, after + the job runner fails the jobs a previous process left running, every + dinner still `pending` with no `ground_dinner` job for its revision + queued or running is set `failed` with `interrupted`, so no dinner stays + pending with nothing left to ground it. `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` @@ -229,8 +239,9 @@ automatic** (D-021). — no model runs — as a `recipes` row whose source_url is `mealquest:manual:{recipe_id}` and whose raw_jsonld is a schema.org Recipe built from it, and sets `manual` with its method and minutes, no - photo and the dinner's difficulty unchanged. It answers 200 with the - dinner; an invalid body is 400, an unknown dinner 404, a page that cannot + photo and the dinner's difficulty unchanged. Either clears the dinner's + pasted `recipe_url`: the recipe chosen replaces it. It answers 200 with + the dinner; an invalid body is 400, an unknown dinner 404, a page that cannot ground it 422 `ungroundable` (the dinner is unchanged), recipe search not set up 409 `not_configured`, a search or model failure 502 `search_failed` / `ai_failed` (only the reason code is logged, never the @@ -488,13 +499,22 @@ First run: while `household_settings.onboarding_completed_at` (read from `GET /api/v1/settings/household`) is null, every route redirects to `/onboarding`. Onboarding is a full screen outside the app shell, in three steps: the welcome (`/onboarding`), the Regulars (`/onboarding/regulars`, -mock 1h, adding and archiving through the dinners API optimistically) and +mock 1h, adding and archiving through the dinners API optimistically; Next +waits for three saved Regulars, and a common pick is not offered once a +Regular has a name `harness.Similar` to it — the PWA's copy of the name +rules in `web/src/dinners/names.ts` runs the same cases, +`internal/ai/harness/testdata/dedupe_cases.json`, as the server's) and notifications (`/onboarding/notifications`: what MealQuest notifies about, the iOS install hint, "Turn on notifications" and "Maybe later"). Either button sends `PATCH /api/v1/settings/household` with `{"onboarding_completed": true}`; the server sets `onboarding_completed_at` to the time of that update (keeping the first time if it is already set; the field itself is read-only and `false` is refused) and the app goes Home. +Completing needs at least three Regulars (`household.MinRegulars`); with +fewer the server answers 400 `invalid_settings` naming +`onboarding_completed` and changes nothing. Step 3 can be reached with +fewer — by its URL, or after another member removes a Regular — so it then +shows the server's reason with an "Add Regulars" button back to step 2. Notification permission is never asked for on page load; until Feature 19 wires Web Push, "Turn on notifications" asks for nothing. Offline on the saved pages the settings cannot be read (the service worker never caches `/api/v1/settings`), diff --git a/internal/ai/harness/dedupe_test.go b/internal/ai/harness/dedupe_test.go index 99332d3..2f64739 100644 --- a/internal/ai/harness/dedupe_test.go +++ b/internal/ai/harness/dedupe_test.go @@ -1,10 +1,45 @@ package harness import ( + "encoding/json" + "os" "testing" "unicode" ) +// TestSharedDedupeCases runs testdata/dedupe_cases.json, which the PWA's +// copy of Normalize and Similar (web/src/dinners/names.ts) runs too, so the +// two cannot drift apart on these names. +func TestSharedDedupeCases(t *testing.T) { + raw, err := os.ReadFile("testdata/dedupe_cases.json") + if err != nil { + t.Fatal(err) + } + var cases struct { + Normalize []struct{ Name, Want string } + Similar []struct { + A, B string + Want bool + } + } + if err := json.Unmarshal(raw, &cases); err != nil { + t.Fatal(err) + } + if len(cases.Normalize) == 0 || len(cases.Similar) == 0 { + t.Fatal("dedupe_cases.json has no cases") + } + for _, c := range cases.Normalize { + if got := Normalize(c.Name); got != c.Want { + t.Errorf("Normalize(%q) = %q, want %q", c.Name, got, c.Want) + } + } + for _, c := range cases.Similar { + if got := Similar(c.A, c.B); got != c.Want { + t.Errorf("Similar(%q, %q) = %v, want %v", c.A, c.B, got, c.Want) + } + } +} + func TestNormalize(t *testing.T) { tests := []struct { name, want string diff --git a/internal/ai/harness/testdata/dedupe_cases.json b/internal/ai/harness/testdata/dedupe_cases.json new file mode 100644 index 0000000..2488e8b --- /dev/null +++ b/internal/ai/harness/testdata/dedupe_cases.json @@ -0,0 +1,119 @@ +{ + "normalize": [ + { + "name": "Easy Chicken Tikka-Masala!", + "want": "chicken tikka masala" + }, + { + "name": " Homemade Chili ", + "want": "chili" + }, + { + "name": "Grandma's Stew", + "want": "grandmas stew" + }, + { + "name": "Grandma’s Stew", + "want": "grandmas stew" + }, + { + "name": "İskender Kebab", + "want": "iskender kebab" + }, + { + "name": "Paella à la Valenciana", + "want": "paella la valenciana" + }, + { + "name": "Crème Brûlée", + "want": "creme brulee" + }, + { + "name": "Crème Brûlée", + "want": "creme brulee" + }, + { + "name": "Smørrebrød", + "want": "smorrebrod" + }, + { + "name": "Pierogi z Łososiem", + "want": "pierogi z lososiem" + }, + { + "name": "Straße Würstchen", + "want": "strasse wurstchen" + }, + { + "name": "Œufs en Cocotte", + "want": "oeufs en cocotte" + }, + { + "name": "Æbleskiver", + "want": "aebleskiver" + }, + { + "name": "Phở Bò", + "want": "pho bo" + }, + { + "name": "The Best Quick Recipe", + "want": "" + }, + { + "name": "Chicken stir-fry", + "want": "chicken stir fry" + }, + { + "name": "7-layer dip", + "want": "7 layer dip" + } + ], + "similar": [ + { + "a": "Homemade chili", + "b": "Chili", + "want": true + }, + { + "a": "Easy Spaghetti Bolognese", + "b": "spaghetti bolognese", + "want": true + }, + { + "a": "Chicken stir fry", + "b": "Chicken stir-fry", + "want": true + }, + { + "a": "Crème brûlée", + "b": "Creme brulee", + "want": true + }, + { + "a": "Smørrebrød", + "b": "smorrebrod", + "want": true + }, + { + "a": "Beef tacos", + "b": "Fish tacos", + "want": false + }, + { + "a": "Chili", + "b": "White chicken chili", + "want": false + }, + { + "a": "Pot roast", + "b": "Roast", + "want": false + }, + { + "a": "The best", + "b": "Easy recipe", + "want": false + } + ] +} diff --git a/internal/db/db_test.go b/internal/db/db_test.go index 5ec14a0..01f0a80 100644 --- a/internal/db/db_test.go +++ b/internal/db/db_test.go @@ -268,7 +268,7 @@ func TestDinners(t *testing.T) { } var unarchivedIDs []string for _, d := range unarchived { - unarchivedIDs = append(unarchivedIDs, d.ID) + unarchivedIDs = append(unarchivedIDs, d.Dinner.ID) } assertIDs(t, "unarchived", unarchivedIDs, other.ID, third.ID) _, err = q.ArchiveDinner(ctx, db.ArchiveDinnerParams{ID: "missing", UpdatedAt: archivedAt}) diff --git a/internal/db/dinners.sql.go b/internal/db/dinners.sql.go index 725dadc..37c723a 100644 --- a/internal/db/dinners.sql.go +++ b/internal/db/dinners.sql.go @@ -16,7 +16,7 @@ SET status = 'archived', updated_by = ?, updated_at = ? WHERE id = ? -RETURNING id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error +RETURNING id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error, grounding_revision ` type ArchiveDinnerParams struct { @@ -48,6 +48,7 @@ func (q *Queries) ArchiveDinner(ctx context.Context, arg ArchiveDinnerParams) (D &i.UpdatedAt, &i.RecipeUrl, &i.GroundingError, + &i.GroundingRevision, ) return i, err } @@ -75,7 +76,7 @@ INSERT INTO dinners ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ? ) -RETURNING id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error +RETURNING id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error, grounding_revision ` type CreateDinnerParams struct { @@ -138,6 +139,7 @@ func (q *Queries) CreateDinner(ctx context.Context, arg CreateDinnerParams) (Din &i.UpdatedAt, &i.RecipeUrl, &i.GroundingError, + &i.GroundingRevision, ) return i, err } @@ -153,6 +155,7 @@ WHERE grounding_status = 'pending' WHERE jobs.kind = ?3 AND jobs.status IN ('queued', 'running') AND json_extract(jobs.payload, '$.dinner_id') = dinners.id + AND json_extract(jobs.payload, '$.revision') = dinners.grounding_revision ) ` @@ -162,10 +165,10 @@ type FailOrphanedPendingDinnerGroundingsParams struct { Kind string } -// Fails every dinner still pending whose grounding job is no longer queued -// or running: the process stopped before the job, or before its handler -// recorded a result. Run at boot, after the job runner fails the jobs a -// previous process left running. +// Fails every dinner still pending whose grounding job for its current +// grounding_revision is no longer queued or running: the process stopped +// before the job, or before its handler recorded a result. Run at boot, +// after the job runner fails the jobs a previous process left running. func (q *Queries) FailOrphanedPendingDinnerGroundings(ctx context.Context, arg FailOrphanedPendingDinnerGroundingsParams) (int64, error) { result, err := q.db.ExecContext(ctx, failOrphanedPendingDinnerGroundings, arg.GroundingError, arg.UpdatedAt, arg.Kind) if err != nil { @@ -179,25 +182,29 @@ UPDATE dinners SET grounding_status = 'failed', grounding_error = ?, updated_at = ? -WHERE id = ? AND grounding_status = 'pending' AND normalized_name = ? +WHERE id = ? + AND status <> 'archived' + AND grounding_status = 'pending' + AND grounding_revision = ? ` type FailPendingDinnerGroundingParams struct { - GroundingError sql.NullString - UpdatedAt string - ID string - NormalizedName string + GroundingError sql.NullString + UpdatedAt string + ID string + GroundingRevision int64 } -// Records why the background grounding of a dinner still pending under the -// normalized name it was grounded for failed; a dinner grounded some other -// way or renamed meanwhile is left as it is (0 rows). +// Records why the background grounding of an unarchived dinner still +// pending at the grounding_revision it was grounded for failed; a dinner +// grounded some other way, renamed or archived meanwhile is left as it is +// (0 rows). func (q *Queries) FailPendingDinnerGrounding(ctx context.Context, arg FailPendingDinnerGroundingParams) (int64, error) { result, err := q.db.ExecContext(ctx, failPendingDinnerGrounding, arg.GroundingError, arg.UpdatedAt, arg.ID, - arg.NormalizedName, + arg.GroundingRevision, ) if err != nil { return 0, err @@ -206,7 +213,7 @@ func (q *Queries) FailPendingDinnerGrounding(ctx context.Context, arg FailPendin } const getDinner = `-- name: GetDinner :one -SELECT id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error FROM dinners +SELECT id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error, grounding_revision FROM dinners WHERE id = ? ` @@ -232,6 +239,7 @@ func (q *Queries) GetDinner(ctx context.Context, id string) (Dinner, error) { &i.UpdatedAt, &i.RecipeUrl, &i.GroundingError, + &i.GroundingRevision, ) return i, err } @@ -239,6 +247,7 @@ func (q *Queries) GetDinner(ctx context.Context, id string) (Dinner, error) { const groundDinner = `-- name: GroundDinner :one UPDATE dinners SET recipe_id = ?, + recipe_url = NULL, photo_id = ?, method = ?, minutes_total = ?, @@ -248,7 +257,7 @@ SET recipe_id = ?, updated_by = ?, updated_at = ? WHERE id = ? -RETURNING id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error +RETURNING id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error, grounding_revision ` type GroundDinnerParams struct { @@ -263,8 +272,9 @@ type GroundDinnerParams struct { ID string } -// Records a dinner's grounding (grounded or manual) and clears any -// grounding_error. An unknown id returns sql.ErrNoRows. +// Records a dinner's grounding by hand (grounded or manual) and clears any +// grounding_error and pasted recipe_url: the recipe chosen replaces the +// link. An unknown id returns sql.ErrNoRows. func (q *Queries) GroundDinner(ctx context.Context, arg GroundDinnerParams) (Dinner, error) { row := q.db.QueryRowContext(ctx, groundDinner, arg.RecipeID, @@ -297,37 +307,43 @@ func (q *Queries) GroundDinner(ctx context.Context, arg GroundDinnerParams) (Din &i.UpdatedAt, &i.RecipeUrl, &i.GroundingError, + &i.GroundingRevision, ) return i, err } const groundPendingDinner = `-- name: GroundPendingDinner :execrows UPDATE dinners -SET recipe_id = ?, - photo_id = ?, - method = ?, - minutes_total = ?, - difficulty = ?, +SET recipe_id = ?1, + photo_id = ?2, + method = COALESCE(method, ?3), + minutes_total = COALESCE(minutes_total, ?4), + difficulty = COALESCE(difficulty, ?5), grounding_status = 'grounded', grounding_error = NULL, - updated_at = ? -WHERE id = ? AND grounding_status = 'pending' AND normalized_name = ? + updated_at = ?6 +WHERE id = ?7 + AND status <> 'archived' + AND grounding_status = 'pending' + AND grounding_revision = ?8 ` type GroundPendingDinnerParams struct { - RecipeID sql.NullString - PhotoID sql.NullString - Method sql.NullString - MinutesTotal sql.NullInt64 - Difficulty sql.NullString - UpdatedAt string - ID string - NormalizedName string + RecipeID sql.NullString + PhotoID sql.NullString + Method sql.NullString + MinutesTotal sql.NullInt64 + Difficulty sql.NullString + UpdatedAt string + ID string + GroundingRevision int64 } -// Records the background grounding of a dinner still pending under the -// normalized name it was grounded for; a dinner grounded some other way or -// renamed meanwhile is left as it is (0 rows). +// Records the background grounding of an unarchived dinner still pending at +// the grounding_revision it was grounded for; a dinner grounded some other +// way, renamed or archived meanwhile is left as it is (0 rows). A method, +// minutes_total or difficulty already set is kept: a pending dinner has +// them only when a member edited them while it was grounding. func (q *Queries) GroundPendingDinner(ctx context.Context, arg GroundPendingDinnerParams) (int64, error) { result, err := q.db.ExecContext(ctx, groundPendingDinner, arg.RecipeID, @@ -337,7 +353,7 @@ func (q *Queries) GroundPendingDinner(ctx context.Context, arg GroundPendingDinn arg.Difficulty, arg.UpdatedAt, arg.ID, - arg.NormalizedName, + arg.GroundingRevision, ) if err != nil { return 0, err @@ -346,7 +362,7 @@ func (q *Queries) GroundPendingDinner(ctx context.Context, arg GroundPendingDinn } const listDinnersByStatus = `-- name: ListDinnersByStatus :many -SELECT id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error FROM dinners +SELECT id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error, grounding_revision FROM dinners WHERE status = ? ORDER BY normalized_name, id ` @@ -379,6 +395,7 @@ func (q *Queries) ListDinnersByStatus(ctx context.Context, status string) ([]Din &i.UpdatedAt, &i.RecipeUrl, &i.GroundingError, + &i.GroundingRevision, ); err != nil { return nil, err } @@ -394,40 +411,50 @@ func (q *Queries) ListDinnersByStatus(ctx context.Context, status string) ([]Din } const listUnarchivedDinners = `-- name: ListUnarchivedDinners :many -SELECT id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error FROM dinners -WHERE status <> 'archived' -ORDER BY id +SELECT dinners.id, dinners.name, dinners.normalized_name, dinners.status, dinners.origin, dinners.method, dinners.minutes_total, dinners.difficulty, dinners.cuisine, dinners.is_favorite, dinners.recipe_id, dinners.photo_id, dinners.grounding_status, dinners.updated_by, dinners.created_at, dinners.updated_at, dinners.recipe_url, dinners.grounding_error, dinners.grounding_revision, recipes.source_url AS recipe_source_url +FROM dinners +LEFT JOIN recipes ON recipes.id = dinners.recipe_id +WHERE dinners.status <> 'archived' +ORDER BY dinners.id ` -// Every dinner a new or renamed dinner must not repeat. -func (q *Queries) ListUnarchivedDinners(ctx context.Context) ([]Dinner, error) { +type ListUnarchivedDinnersRow struct { + Dinner Dinner + RecipeSourceUrl sql.NullString +} + +// Every dinner a new or renamed dinner must not repeat, with the +// source_url of the recipe it is grounded in (NULL while it has none). +func (q *Queries) ListUnarchivedDinners(ctx context.Context) ([]ListUnarchivedDinnersRow, error) { rows, err := q.db.QueryContext(ctx, listUnarchivedDinners) if err != nil { return nil, err } defer rows.Close() - var items []Dinner + var items []ListUnarchivedDinnersRow for rows.Next() { - var i Dinner + var i ListUnarchivedDinnersRow if err := rows.Scan( - &i.ID, - &i.Name, - &i.NormalizedName, - &i.Status, - &i.Origin, - &i.Method, - &i.MinutesTotal, - &i.Difficulty, - &i.Cuisine, - &i.IsFavorite, - &i.RecipeID, - &i.PhotoID, - &i.GroundingStatus, - &i.UpdatedBy, - &i.CreatedAt, - &i.UpdatedAt, - &i.RecipeUrl, - &i.GroundingError, + &i.Dinner.ID, + &i.Dinner.Name, + &i.Dinner.NormalizedName, + &i.Dinner.Status, + &i.Dinner.Origin, + &i.Dinner.Method, + &i.Dinner.MinutesTotal, + &i.Dinner.Difficulty, + &i.Dinner.Cuisine, + &i.Dinner.IsFavorite, + &i.Dinner.RecipeID, + &i.Dinner.PhotoID, + &i.Dinner.GroundingStatus, + &i.Dinner.UpdatedBy, + &i.Dinner.CreatedAt, + &i.Dinner.UpdatedAt, + &i.Dinner.RecipeUrl, + &i.Dinner.GroundingError, + &i.Dinner.GroundingRevision, + &i.RecipeSourceUrl, ); err != nil { return nil, err } @@ -448,7 +475,7 @@ SET is_favorite = ?, updated_by = ?, updated_at = ? WHERE id = ? -RETURNING id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error +RETURNING id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error, grounding_revision ` type SetDinnerFavoriteParams struct { @@ -486,6 +513,7 @@ func (q *Queries) SetDinnerFavorite(ctx context.Context, arg SetDinnerFavoritePa &i.UpdatedAt, &i.RecipeUrl, &i.GroundingError, + &i.GroundingRevision, ) return i, err } @@ -496,7 +524,7 @@ SET is_favorite = 1 - is_favorite, updated_by = ?, updated_at = ? WHERE id = ? -RETURNING id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error +RETURNING id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error, grounding_revision ` type ToggleDinnerFavoriteParams struct { @@ -528,6 +556,7 @@ func (q *Queries) ToggleDinnerFavorite(ctx context.Context, arg ToggleDinnerFavo &i.UpdatedAt, &i.RecipeUrl, &i.GroundingError, + &i.GroundingRevision, ) return i, err } @@ -546,28 +575,30 @@ SET name = ?, recipe_url = ?, photo_id = ?, grounding_status = ?, + grounding_revision = ?, updated_by = ?, updated_at = ? WHERE id = ? -RETURNING id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error +RETURNING id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error, grounding_revision ` type UpdateDinnerParams struct { - Name string - NormalizedName string - Status string - Origin string - Method sql.NullString - MinutesTotal sql.NullInt64 - Difficulty sql.NullString - Cuisine sql.NullString - RecipeID sql.NullString - RecipeUrl sql.NullString - PhotoID sql.NullString - GroundingStatus string - UpdatedBy sql.NullString - UpdatedAt string - ID string + Name string + NormalizedName string + Status string + Origin string + Method sql.NullString + MinutesTotal sql.NullInt64 + Difficulty sql.NullString + Cuisine sql.NullString + RecipeID sql.NullString + RecipeUrl sql.NullString + PhotoID sql.NullString + GroundingStatus string + GroundingRevision int64 + UpdatedBy sql.NullString + UpdatedAt string + ID string } // An unknown id returns sql.ErrNoRows. @@ -585,6 +616,7 @@ func (q *Queries) UpdateDinner(ctx context.Context, arg UpdateDinnerParams) (Din arg.RecipeUrl, arg.PhotoID, arg.GroundingStatus, + arg.GroundingRevision, arg.UpdatedBy, arg.UpdatedAt, arg.ID, @@ -609,6 +641,7 @@ func (q *Queries) UpdateDinner(ctx context.Context, arg UpdateDinnerParams) (Din &i.UpdatedAt, &i.RecipeUrl, &i.GroundingError, + &i.GroundingRevision, ) return i, err } diff --git a/internal/db/models.go b/internal/db/models.go index ac5ef62..b026169 100644 --- a/internal/db/models.go +++ b/internal/db/models.go @@ -51,24 +51,25 @@ type ChatSetting struct { } type Dinner struct { - ID string - Name string - NormalizedName string - Status string - Origin string - Method sql.NullString - MinutesTotal sql.NullInt64 - Difficulty sql.NullString - Cuisine sql.NullString - IsFavorite int64 - RecipeID sql.NullString - PhotoID sql.NullString - GroundingStatus string - UpdatedBy sql.NullString - CreatedAt string - UpdatedAt string - RecipeUrl sql.NullString - GroundingError sql.NullString + ID string + Name string + NormalizedName string + Status string + Origin string + Method sql.NullString + MinutesTotal sql.NullInt64 + Difficulty sql.NullString + Cuisine sql.NullString + IsFavorite int64 + RecipeID sql.NullString + PhotoID sql.NullString + GroundingStatus string + UpdatedBy sql.NullString + CreatedAt string + UpdatedAt string + RecipeUrl sql.NullString + GroundingError sql.NullString + GroundingRevision int64 } type HouseholdSetting struct { diff --git a/internal/db/querier.go b/internal/db/querier.go index 2c6e15f..c75f774 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -36,14 +36,15 @@ type Querier interface { DeleteRecipeIngredients(ctx context.Context, recipeID string) error DeleteRecipeSteps(ctx context.Context, recipeID string) error FailJob(ctx context.Context, arg FailJobParams) (int64, error) - // Fails every dinner still pending whose grounding job is no longer queued - // or running: the process stopped before the job, or before its handler - // recorded a result. Run at boot, after the job runner fails the jobs a - // previous process left running. + // Fails every dinner still pending whose grounding job for its current + // grounding_revision is no longer queued or running: the process stopped + // before the job, or before its handler recorded a result. Run at boot, + // after the job runner fails the jobs a previous process left running. FailOrphanedPendingDinnerGroundings(ctx context.Context, arg FailOrphanedPendingDinnerGroundingsParams) (int64, error) - // Records why the background grounding of a dinner still pending under the - // normalized name it was grounded for failed; a dinner grounded some other - // way or renamed meanwhile is left as it is (0 rows). + // Records why the background grounding of an unarchived dinner still + // pending at the grounding_revision it was grounded for failed; a dinner + // grounded some other way, renamed or archived meanwhile is left as it is + // (0 rows). FailPendingDinnerGrounding(ctx context.Context, arg FailPendingDinnerGroundingParams) (int64, error) // Fails every job left running, at boot before any worker starts. FailRunningJobs(ctx context.Context, arg FailRunningJobsParams) (int64, error) @@ -65,12 +66,15 @@ type Querier interface { GetRecipeBySourceURL(ctx context.Context, sourceUrl string) (Recipe, error) // Returns sql.ErrNoRows until the row exists. GetRecipeSearchSettings(ctx context.Context) (RecipeSearchSetting, error) - // Records a dinner's grounding (grounded or manual) and clears any - // grounding_error. An unknown id returns sql.ErrNoRows. + // Records a dinner's grounding by hand (grounded or manual) and clears any + // grounding_error and pasted recipe_url: the recipe chosen replaces the + // link. An unknown id returns sql.ErrNoRows. GroundDinner(ctx context.Context, arg GroundDinnerParams) (Dinner, error) - // Records the background grounding of a dinner still pending under the - // normalized name it was grounded for; a dinner grounded some other way or - // renamed meanwhile is left as it is (0 rows). + // Records the background grounding of an unarchived dinner still pending at + // the grounding_revision it was grounded for; a dinner grounded some other + // way, renamed or archived meanwhile is left as it is (0 rows). A method, + // minutes_total or difficulty already set is kept: a pending dinner has + // them only when a member edited them while it was grounding. GroundPendingDinner(ctx context.Context, arg GroundPendingDinnerParams) (int64, error) ListDinnersByStatus(ctx context.Context, status string) ([]Dinner, error) // The planned dinners dated from from_date up to but not including @@ -81,8 +85,9 @@ type Querier interface { ListRecentDeclinedNames(ctx context.Context, limit int64) ([]string, error) ListRecipeIngredients(ctx context.Context, recipeID string) ([]RecipeIngredient, error) ListRecipeSteps(ctx context.Context, recipeID string) ([]RecipeStep, error) - // Every dinner a new or renamed dinner must not repeat. - ListUnarchivedDinners(ctx context.Context) ([]Dinner, error) + // Every dinner a new or renamed dinner must not repeat, with the + // source_url of the recipe it is grounded in (NULL while it has none). + ListUnarchivedDinners(ctx context.Context) ([]ListUnarchivedDinnersRow, error) // Stores a partial result of a running job. PublishJobResult(ctx context.Context, arg PublishJobResultParams) (int64, error) // An unknown id returns sql.ErrNoRows. diff --git a/internal/dinners/dinners.go b/internal/dinners/dinners.go index a336bd8..7fac6f9 100644 --- a/internal/dinners/dinners.go +++ b/internal/dinners/dinners.go @@ -176,7 +176,7 @@ func (s *Service) CreateRegular(ctx context.Context, r NewRegular, updatedBy str if err != nil { return Created{}, err } - jobID, err := s.enqueueGrounding(ctx, d.ID, harness.Normalize(d.Name)) + jobID, err := s.enqueueGrounding(ctx, d.ID, d.GroundingRevision) if err != nil { return Created{}, err } @@ -187,60 +187,59 @@ func (s *Service) CreateRegular(ctx context.Context, r NewRegular, updatedBy str } return Created{Dinner: fromRow(row)}, nil } - return Created{Dinner: d, GroundingJobID: jobID}, nil + return Created{Dinner: fromRow(d), GroundingJobID: jobID}, nil } -// enqueueGrounding enqueues the KindGroundDinner job for pending dinner id, -// whose normalized name is normalizedName, and returns its id. When the job -// cannot be enqueued, the dinner's grounding is failed with -// ReasonGroundingError, so it is not left pending with no job, and the id -// is nil. -func (s *Service) enqueueGrounding(ctx context.Context, id, normalizedName string) (*string, error) { - jobID, err := s.groundJob.Enqueue(ctx, GroundJob{DinnerID: id}) +// enqueueGrounding enqueues the KindGroundDinner job for pending dinner id +// at grounding_revision revision and returns its id. When the job cannot be +// enqueued, the dinner's grounding is failed with ReasonGroundingError, so +// it is not left pending with no job, and the id is nil. +func (s *Service) enqueueGrounding(ctx context.Context, id string, revision int64) (*string, error) { + jobID, err := s.groundJob.Enqueue(ctx, GroundJob{DinnerID: id, Revision: revision}) if err == nil { return &jobID, nil } // The error is a database error, never a dependency's text. s.logger.ErrorContext(ctx, "dinner grounding not enqueued", "dinner_id", id, "error", err) if _, err := db.New(s.st.Writer).FailPendingDinnerGrounding(context.WithoutCancel(ctx), db.FailPendingDinnerGroundingParams{ - GroundingError: sql.NullString{String: ReasonGroundingError, Valid: true}, - UpdatedAt: store.Now().String(), - ID: id, - NormalizedName: normalizedName, + GroundingError: sql.NullString{String: ReasonGroundingError, Valid: true}, + UpdatedAt: store.Now().String(), + ID: id, + GroundingRevision: revision, }); err != nil { return nil, fmt.Errorf("dinners: record failed grounding of %s: %w", id, err) } return nil, nil } -func (s *Service) createRegular(ctx context.Context, r NewRegular, updatedBy string) (Dinner, error) { +func (s *Service) createRegular(ctx context.Context, r NewRegular, updatedBy string) (db.Dinner, error) { if updatedBy == "" { - return Dinner{}, errors.New("dinners: create: no member to record as updated_by") + return db.Dinner{}, errors.New("dinners: create: no member to record as updated_by") } var name string var recipeURL sql.NullString switch { case r.Name != nil && r.RecipeURL != nil: - return Dinner{}, invalid("name", "give a name or a recipe_url, not both") + return db.Dinner{}, invalid("name", "give a name or a recipe_url, not both") case r.Name != nil: n, err := checkName(*r.Name) if err != nil { - return Dinner{}, err + return db.Dinner{}, err } name = n case r.RecipeURL != nil: u, err := checkRecipeURL(*r.RecipeURL) if err != nil { - return Dinner{}, err + return db.Dinner{}, err } n, ok := nameFromURL(u) if !ok { - return Dinner{}, invalid("recipe_url", "the link does not name a dinner; add it by name instead") + return db.Dinner{}, invalid("recipe_url", "the link does not name a dinner; add it by name instead") } name = n recipeURL = sql.NullString{String: u.String(), Valid: true} default: - return Dinner{}, invalid("name", "give a name or a recipe_url") + return db.Dinner{}, invalid("name", "give a name or a recipe_url") } var row db.Dinner @@ -269,9 +268,9 @@ func (s *Service) createRegular(ctx context.Context, r NewRegular, updatedBy str return nil }) if err != nil { - return Dinner{}, err + return db.Dinner{}, err } - return fromRow(row), nil + return row, nil } // List returns the dinners with status, which must be regular or @@ -294,9 +293,10 @@ func (s *Service) List(ctx context.Context, status string) ([]Dinner, error) { // Update applies p to dinner id, recording updatedBy. An invalid patch is a // *ValidationError and a new name repeating another unarchived dinner a // *DuplicateError; either changes nothing. Renaming a dinner still pending -// clears its recipe_url and enqueues a fresh KindGroundDinner job that -// grounds the new name: the job already running was for the old name or -// link, and its result no longer applies. +// clears its recipe_url, raises its grounding_revision and enqueues a fresh +// KindGroundDinner job that grounds the new name: a job queued or running +// for the old name or link carries the old revision, so it can no longer +// write the dinner. func (s *Service) Update(ctx context.Context, id string, p Patch, updatedBy string) (Dinner, error) { if updatedBy == "" { return Dinner{}, errors.New("dinners: update: no member to record as updated_by") @@ -318,21 +318,22 @@ func (s *Service) Update(ctx context.Context, id string, p Patch, updatedBy stri return fmt.Errorf("dinners: read %s: %w", id, err) } params := db.UpdateDinnerParams{ - ID: cur.ID, - Name: cur.Name, - NormalizedName: cur.NormalizedName, - Status: cur.Status, - Origin: cur.Origin, - Method: cur.Method, - MinutesTotal: cur.MinutesTotal, - Difficulty: cur.Difficulty, - Cuisine: cur.Cuisine, - RecipeID: cur.RecipeID, - RecipeUrl: cur.RecipeUrl, - PhotoID: cur.PhotoID, - GroundingStatus: cur.GroundingStatus, - UpdatedBy: sql.NullString{String: updatedBy, Valid: true}, - UpdatedAt: store.Now().String(), + ID: cur.ID, + Name: cur.Name, + NormalizedName: cur.NormalizedName, + Status: cur.Status, + Origin: cur.Origin, + Method: cur.Method, + MinutesTotal: cur.MinutesTotal, + Difficulty: cur.Difficulty, + Cuisine: cur.Cuisine, + RecipeID: cur.RecipeID, + RecipeUrl: cur.RecipeUrl, + PhotoID: cur.PhotoID, + GroundingStatus: cur.GroundingStatus, + GroundingRevision: cur.GroundingRevision, + UpdatedBy: sql.NullString{String: updatedBy, Valid: true}, + UpdatedAt: store.Now().String(), } if p.Name != nil { if err := checkUnique(ctx, q, cur.ID, name, sql.NullString{}); err != nil { @@ -344,6 +345,7 @@ func (s *Service) Update(ctx context.Context, id string, p Patch, updatedBy stri // The link was the old dinner's recipe; the new name is // grounded by name, and a link can be given by hand. params.RecipeUrl = sql.NullString{} + params.GroundingRevision++ } } if p.Method != nil { @@ -359,14 +361,14 @@ func (s *Service) Update(ctx context.Context, id string, p Patch, updatedBy stri if err != nil { return fmt.Errorf("dinners: update %s: %w", id, err) } - regrounding = row.GroundingStatus == GroundingPending && row.NormalizedName != cur.NormalizedName + regrounding = row.GroundingRevision != cur.GroundingRevision return nil }) if err != nil { return Dinner{}, err } if regrounding { - jobID, err := s.enqueueGrounding(ctx, row.ID, row.NormalizedName) + jobID, err := s.enqueueGrounding(ctx, row.ID, row.GroundingRevision) if err != nil { return Dinner{}, err } @@ -514,18 +516,24 @@ func nameFromURL(u *url.URL) (name string, ok bool) { } // checkUnique returns a *DuplicateError when name is harness.Similar to an -// unarchived dinner other than self, or recipeURL is one's recipe link. +// unarchived dinner other than self, or recipeURL is one's recipe link: the +// source_url of the recipe it is grounded in, else the recipe_url still +// awaiting grounding. func checkUnique(ctx context.Context, q *db.Queries, self, name string, recipeURL sql.NullString) error { rows, err := q.ListUnarchivedDinners(ctx) if err != nil { return fmt.Errorf("dinners: list for dedupe: %w", err) } for _, row := range rows { - if row.ID == self { + if row.Dinner.ID == self { continue } - if harness.Similar(name, row.Name) || (recipeURL.Valid && row.RecipeUrl == recipeURL) { - return &DuplicateError{Existing: fromRow(row)} + link := row.Dinner.RecipeUrl + if row.RecipeSourceUrl.Valid { + link = row.RecipeSourceUrl + } + if harness.Similar(name, row.Dinner.Name) || (recipeURL.Valid && link == recipeURL) { + return &DuplicateError{Existing: fromRow(row.Dinner)} } } return nil diff --git a/internal/dinners/export_test.go b/internal/dinners/export_test.go index 9eb3511..ca06c60 100644 --- a/internal/dinners/export_test.go +++ b/internal/dinners/export_test.go @@ -1,6 +1,7 @@ package dinners import ( + "context" "testing" "time" ) @@ -12,3 +13,11 @@ func SetGroundTimeout(t *testing.T, d time.Duration) { groundTimeout = d t.Cleanup(func() { groundTimeout = old }) } + +// RunGroundJob runs the KindGroundDinner handler for p in the caller's +// goroutine, as the job runner would, and returns the grounding status it +// reports. +func (s *Service) RunGroundJob(ctx context.Context, p GroundJob) (string, error) { + r, err := s.runGroundJob(ctx, p, nil) + return r.GroundingStatus, err +} diff --git a/internal/dinners/grounding.go b/internal/dinners/grounding.go index b3ff4f7..4d43b13 100644 --- a/internal/dinners/grounding.go +++ b/internal/dinners/grounding.go @@ -76,14 +76,17 @@ var groundTimeout = GroundTimeout // runs past GroundTimeout; the dinner is left as it was. var ErrGroundTimeout = errors.New("dinners: grounding the recipe took too long") -// GroundJob is the KindGroundDinner payload. +// GroundJob is the KindGroundDinner payload: the dinner to ground and its +// grounding_revision when the job was enqueued. type GroundJob struct { DinnerID string `json:"dinner_id"` + Revision int64 `json:"revision"` } // groundResult is the KindGroundDinner result: the dinner's grounding -// status once the job ends. It is not grounded when the dinner was -// grounded by hand before the job finished. +// status once the job ends. It is not grounded by this job when the dinner +// was grounded by hand before the job finished, renamed (a later job +// grounds it) or archived; the job then reports the status it found. type groundResult struct { DinnerID string `json:"dinner_id"` GroundingStatus string `json:"grounding_status"` @@ -93,18 +96,21 @@ func (r groundResult) PublicJSON() ([]byte, error) { return json.Marshal(r) } // runGroundJob grounds a pending dinner as its creation asked: its // recipe_url with GroundURL, else its name with Ground. Success records -// the recipe, photo, method, minutes and difficulty with grounding_status -// grounded; failure records grounding_status failed with its Reason, which -// is also the job's failure code. Either is recorded only while the dinner -// is still pending under the normalized name read here: a dinner grounded -// by hand meanwhile is left as it is, and one renamed meanwhile is left -// pending for the job its rename enqueued. +// the recipe and photo with grounding_status grounded, and the recipe's +// method, minutes and difficulty where a member has not set them; failure +// records grounding_status failed with its Reason, which is also the job's +// failure code. A dinner archived, no longer pending, or at a +// grounding_revision other than p.Revision is not grounded at all, and +// either result is recorded only while the dinner is still unarchived, +// pending and at p.Revision: a dinner grounded by hand meanwhile is left as +// it is, and one renamed or archived meanwhile is left pending — for the +// job its rename enqueued, or for none. func (s *Service) runGroundJob(ctx context.Context, p GroundJob, _ func(groundResult) error) (groundResult, error) { row, err := db.New(s.st.Reader).GetDinner(ctx, p.DinnerID) if err != nil { return groundResult{}, fmt.Errorf("dinners: ground: read %s: %w", p.DinnerID, err) } - if row.GroundingStatus != GroundingPending { + if row.Status == StatusArchived || row.GroundingStatus != GroundingPending || row.GroundingRevision != p.Revision { return groundResult{DinnerID: row.ID, GroundingStatus: row.GroundingStatus}, nil } @@ -124,10 +130,10 @@ func (s *Service) runGroundJob(ctx context.Context, p GroundJob, _ func(groundRe s.logger.InfoContext(ctx, "dinner grounding failed", "dinner_id", row.ID, "reason", reason) write := context.WithoutCancel(ctx) if _, failErr := db.New(s.st.Writer).FailPendingDinnerGrounding(write, db.FailPendingDinnerGroundingParams{ - GroundingError: sql.NullString{String: reason, Valid: true}, - UpdatedAt: store.Now().String(), - ID: row.ID, - NormalizedName: row.NormalizedName, + GroundingError: sql.NullString{String: reason, Valid: true}, + UpdatedAt: store.Now().String(), + ID: row.ID, + GroundingRevision: p.Revision, }); failErr != nil { return groundResult{}, fmt.Errorf("dinners: ground: record failure of %s: %w", row.ID, failErr) } @@ -138,14 +144,14 @@ func (s *Service) runGroundJob(ctx context.Context, p GroundJob, _ func(groundRe } n, err := db.New(s.st.Writer).GroundPendingDinner(ctx, db.GroundPendingDinnerParams{ - RecipeID: sql.NullString{String: gr.RecipeID, Valid: true}, - PhotoID: sql.NullString{String: gr.PhotoID, Valid: true}, - Method: sql.NullString{String: gr.Method, Valid: true}, - MinutesTotal: nullInt(gr.MinutesTotal), - Difficulty: sql.NullString{String: gr.Difficulty, Valid: true}, - UpdatedAt: store.Now().String(), - ID: row.ID, - NormalizedName: row.NormalizedName, + RecipeID: sql.NullString{String: gr.RecipeID, Valid: true}, + PhotoID: sql.NullString{String: gr.PhotoID, Valid: true}, + Method: sql.NullString{String: gr.Method, Valid: true}, + MinutesTotal: nullInt(gr.MinutesTotal), + Difficulty: sql.NullString{String: gr.Difficulty, Valid: true}, + UpdatedAt: store.Now().String(), + ID: row.ID, + GroundingRevision: p.Revision, }) if err != nil { return groundResult{}, fmt.Errorf("dinners: ground: record %s: %w", row.ID, err) diff --git a/internal/dinners/grounding_test.go b/internal/dinners/grounding_test.go index 928afea..47f0f31 100644 --- a/internal/dinners/grounding_test.go +++ b/internal/dinners/grounding_test.go @@ -2,6 +2,8 @@ package dinners_test import ( "context" + "database/sql" + "encoding/json" "errors" "io" "log/slog" @@ -385,6 +387,168 @@ func TestRenameLinkWhilePendingGroundsNewName(t *testing.T) { if got := e.search.Queries(); !slices.Equal(got, []string{"Beef stew"}) { t.Errorf("searched %q, want only the new name", got) } + if row.RecipeUrl.Valid { + t.Errorf("recipe_url = %q, want the old link dropped", row.RecipeUrl.String) + } + if _, err := db.New(e.st.Reader).GetRecipeBySourceURL(t.Context(), curryA); !errors.Is(err, sql.ErrNoRows) { + t.Errorf("old link's recipe lookup err = %v, want it never grounded (sql.ErrNoRows)", err) + } +} + +// queuedJob reads the payload of queued KindGroundDinner job id. +func (e *env) queuedJob(t *testing.T, id string) dinners.GroundJob { + t.Helper() + var payload string + if err := e.st.Reader.QueryRowContext(t.Context(), `SELECT payload FROM jobs WHERE id = ?`, id).Scan(&payload); err != nil { + t.Fatal(err) + } + var p dinners.GroundJob + if err := json.Unmarshal([]byte(payload), &p); err != nil { + t.Fatal(err) + } + return p +} + +// TestRenameBeforeJobStartsSkipsStaleJob: a Regular renamed before its +// first job starts is grounded only by the job its rename enqueued. The +// first job, though it would read the new name, does no work and writes +// nothing, so its failure cannot leave the dinner "Needs a recipe" while +// the rename's job finds a recipe. +func TestRenameBeforeJobStartsSkipsStaleJob(t *testing.T) { + t.Parallel() + e := newUnstartedEnv(t) + e.search.Results["Beef stew"] = []recipes.Candidate{{URL: stew}} + + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Chicken curry")}, member) + if err != nil { + t.Fatal(err) + } + first := e.queuedJob(t, *c.GroundingJobID) + if first.DinnerID != c.ID || first.Revision != e.dinner(t, c.ID).GroundingRevision { + t.Fatalf("first job = %+v, want dinner %s at its revision", first, c.ID) + } + if _, err := e.svc.Update(t.Context(), c.ID, dinners.Patch{Name: ptr("Beef stew")}, member); err != nil { + t.Fatal(err) + } + renamed := e.dinner(t, c.ID) + if renamed.GroundingRevision != first.Revision+1 { + t.Fatalf("revision after rename = %d, want %d", renamed.GroundingRevision, first.Revision+1) + } + + // The first job would fail if it searched. + e.search.Err = recipes.ErrSearchRateLimited + status, err := e.svc.RunGroundJob(t.Context(), first) + if err != nil || status != dinners.GroundingPending { + t.Fatalf("first job = %q, %v; want pending, no error", status, err) + } + if q := e.search.Queries(); len(q) != 0 { + t.Errorf("first job searched %q, want no work for a stale revision", q) + } + if row := e.dinner(t, c.ID); row != renamed { + t.Fatalf("first job wrote the dinner: %+v, was %+v", row, renamed) + } + + e.search.Err = nil + e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150)) + status, err = e.svc.RunGroundJob(t.Context(), dinners.GroundJob{DinnerID: c.ID, Revision: renamed.GroundingRevision}) + if err != nil || status != dinners.GroundingGrounded { + t.Fatalf("rename's job = %q, %v; want grounded", status, err) + } + row := e.dinner(t, c.ID) + rec, err := db.New(e.st.Reader).GetRecipe(t.Context(), row.RecipeID.String) + if err != nil { + t.Fatal(err) + } + if row.GroundingStatus != dinners.GroundingGrounded || rec.SourceUrl != stew { + t.Errorf("dinner = %+v recipe %s, want grounded in %s", row, rec.SourceUrl, stew) + } + + // A stale job finishing last changes nothing either. + if status, err := e.svc.RunGroundJob(t.Context(), first); err != nil || status != dinners.GroundingGrounded { + t.Errorf("first job run last = %q, %v; want grounded reported, no error", status, err) + } + if after := e.dinner(t, c.ID); after != row { + t.Errorf("stale job changed the grounded dinner: %+v, was %+v", after, row) + } +} + +// TestEditWhilePendingIsKept: a method or minutes a member saves while a +// Regular is grounding is kept when grounding succeeds; what the member +// did not set comes from the recipe. +func TestEditWhilePendingIsKept(t *testing.T) { + t.Parallel() + e := newEnv(t) + e.search.Gate = make(chan struct{}) + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Chicken curry")}, member) + if err != nil { + t.Fatal(err) + } + e.waitQuery(t, "Chicken curry") + if _, err := e.svc.Update(t.Context(), c.ID, dinners.Patch{Method: ptr("grill"), MinutesTotal: ptr(15)}, member); err != nil { + t.Fatal(err) + } + close(e.search.Gate) + + row := e.waitGrounding(t, c.ID) + if row.GroundingStatus != dinners.GroundingGrounded || !row.RecipeID.Valid || !row.PhotoID.Valid { + t.Fatalf("dinner = %+v, want grounded", row) + } + if row.Method.String != "grill" || row.MinutesTotal.Int64 != 15 || row.Difficulty.String != "easy" { + t.Errorf("dinner = %s, %d minutes, %s; want the member's grill and 15 minutes, the recipe's easy", + row.Method.String, row.MinutesTotal.Int64, row.Difficulty.String) + } +} + +// TestArchiveWhilePendingStopsGrounding: a pending Regular archived before +// its job starts is not searched for, and one archived while its job runs +// is not written; neither ends up grounded. +func TestArchiveWhilePendingStopsGrounding(t *testing.T) { + t.Parallel() + t.Run("before the job starts", func(t *testing.T) { + t.Parallel() + e := newUnstartedEnv(t) + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Chicken curry")}, member) + if err != nil { + t.Fatal(err) + } + if _, err := e.svc.Archive(t.Context(), c.ID, member); err != nil { + t.Fatal(err) + } + e.start(t) + if job := e.waitJob(t, *c.GroundingJobID); job.Status != jobs.StatusSucceeded { + t.Fatalf("job = %+v, want succeeded with nothing to do", job) + } + if q := e.search.Queries(); len(q) != 0 { + t.Errorf("searched %q for an archived dinner", q) + } + if row := e.dinner(t, c.ID); row.Status != dinners.StatusArchived || row.GroundingStatus == dinners.GroundingGrounded || row.RecipeID.Valid { + t.Errorf("dinner = %+v, want archived and not grounded", row) + } + }) + t.Run("while the job runs", func(t *testing.T) { + t.Parallel() + e := newEnv(t) + e.search.Gate = make(chan struct{}) + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Chicken curry")}, member) + if err != nil { + t.Fatal(err) + } + e.waitQuery(t, "Chicken curry") + if _, err := e.svc.Archive(t.Context(), c.ID, member); err != nil { + t.Fatal(err) + } + close(e.search.Gate) + if job := e.waitJob(t, *c.GroundingJobID); job.Status != jobs.StatusSucceeded { + t.Fatalf("job = %+v", job) + } + if row := e.dinner(t, c.ID); row.Status != dinners.StatusArchived || row.GroundingStatus == dinners.GroundingGrounded || row.RecipeID.Valid { + t.Errorf("dinner = %+v, want archived and not grounded", row) + } + }) } // TestRenameWhilePendingEnqueueFailure: a pending Regular renamed when the @@ -530,6 +694,59 @@ func TestFixViaCandidate(t *testing.T) { } } +// TestFixReplacesPendingLink: fixing a Regular added from a link that held +// no recipe drops that link, so it no longer blocks adding the link again, +// while the link of the recipe chosen does. +func TestFixReplacesPendingLink(t *testing.T) { + t.Parallel() + const lost = "https://gone.example/recipes/lamb-tagine" + tests := []struct { + name string + fix dinners.Fix + // linkTaken is the recipe link the fixed dinner now has, if any. + linkTaken string + }{ + {"candidate", dinners.Fix{CandidateID: ptr(curryB)}, curryB}, + {"manual", dinners.Fix{Manual: &dinners.ManualRecipe{Ingredients: []string{"1 lamb"}, Minutes: ptr(90), Method: ptr("oven")}}, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + e := newEnv(t) + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(lost)}, member) + if err != nil { + t.Fatal(err) + } + if job := e.waitJob(t, *c.GroundingJobID); job.Status != jobs.StatusFailed { + t.Fatalf("job = %+v, want failed", job) + } + // Renamed, so only the link could repeat it. + if _, err := e.svc.Update(t.Context(), c.ID, dinners.Patch{Name: ptr("Weeknight dinner")}, member); err != nil { + t.Fatal(err) + } + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 25)) + d, err := e.svc.Ground(t.Context(), c.ID, tt.fix, member) + if err != nil { + t.Fatal(err) + } + if d.RecipeURL != nil { + t.Errorf("recipe_url = %q after the fix, want the unusable link dropped", *d.RecipeURL) + } + + if _, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(lost)}, member); err != nil { + t.Errorf("adding the dropped link again: %v, want added", err) + } + if tt.linkTaken != "" { + _, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(tt.linkTaken)}, member) + var dup *dinners.DuplicateError + if !errors.As(err, &dup) || dup.Existing.ID != c.ID { + t.Errorf("adding the fixed dinner's recipe link: err = %v, want a duplicate of %s", err, c.ID) + } + } + }) + } +} + func TestFixViaRecipeURL(t *testing.T) { t.Parallel() e := newEnv(t) diff --git a/internal/store/migrate_test.go b/internal/store/migrate_test.go index 1c0c2a6..75b6491 100644 --- a/internal/store/migrate_test.go +++ b/internal/store/migrate_test.go @@ -175,8 +175,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" { - 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", 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" { + 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", got) } } @@ -196,8 +196,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" { - 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", 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" { + 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", got) } if !tableExists(t, s, "dinners") { t.Error("table from 0001_init was not created") diff --git a/migrations/0010_dinner_grounding_revision.sql b/migrations/0010_dinner_grounding_revision.sql new file mode 100644 index 0000000..5f13bd9 --- /dev/null +++ b/migrations/0010_dinner_grounding_revision.sql @@ -0,0 +1,7 @@ +-- 0010_dinner_grounding_revision: each ground_dinner job carries the +-- dinner's grounding_revision when it was enqueued, and records its result +-- only while the dinner still has that revision. Renaming a pending dinner +-- raises the revision before enqueuing the job for the new name, so an +-- earlier job — queued or running — can no longer write the dinner. + +ALTER TABLE dinners ADD COLUMN grounding_revision INTEGER NOT NULL DEFAULT 0; diff --git a/migrations/queries/dinners.sql b/migrations/queries/dinners.sql index a3782d8..90333a9 100644 --- a/migrations/queries/dinners.sql +++ b/migrations/queries/dinners.sql @@ -25,10 +25,13 @@ SELECT COUNT(*) FROM dinners WHERE status = 'regular'; -- name: ListUnarchivedDinners :many --- Every dinner a new or renamed dinner must not repeat. -SELECT * FROM dinners -WHERE status <> 'archived' -ORDER BY id; +-- Every dinner a new or renamed dinner must not repeat, with the +-- source_url of the recipe it is grounded in (NULL while it has none). +SELECT sqlc.embed(dinners), recipes.source_url AS recipe_source_url +FROM dinners +LEFT JOIN recipes ON recipes.id = dinners.recipe_id +WHERE dinners.status <> 'archived' +ORDER BY dinners.id; -- name: UpdateDinner :one -- An unknown id returns sql.ErrNoRows. @@ -45,6 +48,7 @@ SET name = ?, recipe_url = ?, photo_id = ?, grounding_status = ?, + grounding_revision = ?, updated_by = ?, updated_at = ? WHERE id = ? @@ -78,10 +82,12 @@ WHERE id = ? RETURNING *; -- name: GroundDinner :one --- Records a dinner's grounding (grounded or manual) and clears any --- grounding_error. An unknown id returns sql.ErrNoRows. +-- Records a dinner's grounding by hand (grounded or manual) and clears any +-- grounding_error and pasted recipe_url: the recipe chosen replaces the +-- link. An unknown id returns sql.ErrNoRows. UPDATE dinners SET recipe_id = ?, + recipe_url = NULL, photo_id = ?, method = ?, minutes_total = ?, @@ -94,35 +100,44 @@ WHERE id = ? RETURNING *; -- name: GroundPendingDinner :execrows --- Records the background grounding of a dinner still pending under the --- normalized name it was grounded for; a dinner grounded some other way or --- renamed meanwhile is left as it is (0 rows). +-- Records the background grounding of an unarchived dinner still pending at +-- the grounding_revision it was grounded for; a dinner grounded some other +-- way, renamed or archived meanwhile is left as it is (0 rows). A method, +-- minutes_total or difficulty already set is kept: a pending dinner has +-- them only when a member edited them while it was grounding. UPDATE dinners -SET recipe_id = ?, - photo_id = ?, - method = ?, - minutes_total = ?, - difficulty = ?, +SET recipe_id = sqlc.arg(recipe_id), + photo_id = sqlc.arg(photo_id), + method = COALESCE(method, sqlc.arg(method)), + minutes_total = COALESCE(minutes_total, sqlc.arg(minutes_total)), + difficulty = COALESCE(difficulty, sqlc.arg(difficulty)), grounding_status = 'grounded', grounding_error = NULL, - updated_at = ? -WHERE id = ? AND grounding_status = 'pending' AND normalized_name = ?; + updated_at = sqlc.arg(updated_at) +WHERE id = sqlc.arg(id) + AND status <> 'archived' + AND grounding_status = 'pending' + AND grounding_revision = sqlc.arg(grounding_revision); -- name: FailPendingDinnerGrounding :execrows --- Records why the background grounding of a dinner still pending under the --- normalized name it was grounded for failed; a dinner grounded some other --- way or renamed meanwhile is left as it is (0 rows). +-- Records why the background grounding of an unarchived dinner still +-- pending at the grounding_revision it was grounded for failed; a dinner +-- grounded some other way, renamed or archived meanwhile is left as it is +-- (0 rows). UPDATE dinners SET grounding_status = 'failed', grounding_error = ?, updated_at = ? -WHERE id = ? AND grounding_status = 'pending' AND normalized_name = ?; +WHERE id = ? + AND status <> 'archived' + AND grounding_status = 'pending' + AND grounding_revision = ?; -- name: FailOrphanedPendingDinnerGroundings :execrows --- Fails every dinner still pending whose grounding job is no longer queued --- or running: the process stopped before the job, or before its handler --- recorded a result. Run at boot, after the job runner fails the jobs a --- previous process left running. +-- Fails every dinner still pending whose grounding job for its current +-- grounding_revision is no longer queued or running: the process stopped +-- before the job, or before its handler recorded a result. Run at boot, +-- after the job runner fails the jobs a previous process left running. UPDATE dinners SET grounding_status = 'failed', grounding_error = sqlc.arg(grounding_error), @@ -133,4 +148,5 @@ WHERE grounding_status = 'pending' WHERE jobs.kind = sqlc.arg(kind) AND jobs.status IN ('queued', 'running') AND json_extract(jobs.payload, '$.dinner_id') = dinners.id + AND json_extract(jobs.payload, '$.revision') = dinners.grounding_revision ); diff --git a/web/src/dinners/names.test.ts b/web/src/dinners/names.test.ts new file mode 100644 index 0000000..f2677fd --- /dev/null +++ b/web/src/dinners/names.test.ts @@ -0,0 +1,20 @@ +import { describe, expect, it } from 'vitest' +import cases from '../../../internal/ai/harness/testdata/dedupe_cases.json' +import { normalizeName, similarNames } from './names.ts' + +// The cases internal/ai/harness's tests run against Normalize and Similar. +describe('dinner names, as the server compares them', () => { + it('has cases to run', () => { + expect(cases.normalize.length).toBeGreaterThan(0) + expect(cases.similar.length).toBeGreaterThan(0) + }) + + it.each(cases.normalize)('normalizes "$name" to "$want"', ({ name, want }) => { + expect(normalizeName(name)).toBe(want) + }) + + it.each(cases.similar)('"$a" and "$b" similar: $want', ({ a, b, want }) => { + expect(similarNames(a, b)).toBe(want) + expect(similarNames(b, a)).toBe(want) + }) +}) diff --git a/web/src/dinners/names.ts b/web/src/dinners/names.ts new file mode 100644 index 0000000..b5afe11 --- /dev/null +++ b/web/src/dinners/names.ts @@ -0,0 +1,74 @@ +// The PWA's copy of the server's dinner-name rules (internal/ai/harness +// Normalize and Similar), so a screen can tell beforehand that the dinners +// API would refuse a name as a duplicate. The server still decides. +// internal/ai/harness/testdata/dedupe_cases.json is run against both. + +// SIMILAR_THRESHOLD is harness.SimilarThreshold. +const SIMILAR_THRESHOLD = 0.85 + +// fillerWords is harness's fillerWords. +const fillerWords = new Set(['easy', 'best', 'homemade', 'quick', 'simple', 'the', 'a', 'recipe', 'classic']) + +// letterFolds are the letters harness folds that canonical decomposition +// leaves whole: its ligatures and extra letters, and the common stroked +// Latin letters. Letters with accents fold by decomposing and dropping the +// marks, as harness's table does; rarer stroked letters are compared as +// they are. +const letterFolds: Record = { + ß: 'ss', + æ: 'ae', + œ: 'oe', + þ: 'th', + ð: 'd', + ı: 'i', + ʼn: 'n', + ij: 'ij', + ø: 'o', + đ: 'd', + ħ: 'h', + ŀ: 'l', + ł: 'l', + ŧ: 't', + ƀ: 'b', + ƶ: 'z', + ǥ: 'g', + ɨ: 'i', +} + +const combiningMark = /\p{Mn}/u +const wordChar = /[\p{L}\p{Nd}]/u + +// normalizeName is harness.Normalize: lowercased, accents dropped, letters +// folded, apostrophes removed, other punctuation turned into spaces and +// filler words dropped. "Easy Chicken Tikka-Masala!" is +// "chicken tikka masala". +export function normalizeName(name: string): string { + let out = '' + for (const ch of name.toLowerCase().normalize('NFD')) { + if (combiningMark.test(ch) || ch === "'" || ch === '’') continue + const fold = letterFolds[ch] + if (fold !== undefined) out += fold + else if (wordChar.test(ch)) out += ch + else out += ' ' + } + return out + .split(' ') + .filter((word) => word !== '' && !fillerWords.has(word)) + .join(' ') +} + +function words(name: string): Set { + const normalized = normalizeName(name) + return new Set(normalized === '' ? [] : normalized.split(' ')) +} + +// similarNames is harness.Similar: whether a and b name the same dinner, by +// the share of the words of their normalized forms they have in common. +export function similarNames(a: string, b: string): boolean { + const left = words(a) + const right = words(b) + if (left.size === 0 || right.size === 0) return false + let shared = 0 + for (const word of left) if (right.has(word)) shared++ + return shared / (left.size + right.size - shared) >= SIMILAR_THRESHOLD +} diff --git a/web/src/onboarding/NotificationsStep.test.tsx b/web/src/onboarding/NotificationsStep.test.tsx index a34c56c..e606aa9 100644 --- a/web/src/onboarding/NotificationsStep.test.tsx +++ b/web/src/onboarding/NotificationsStep.test.tsx @@ -100,6 +100,7 @@ async function renderStep(enableNotifications?: EnableNotifications) { element={} /> Home} /> + Regulars step} /> @@ -257,6 +258,41 @@ describe('onboarding step 3', () => { }) }) +describe('onboarding step 3 with too few Regulars', () => { + it('says why and offers a way back to add Regulars', async () => { + const server = await renderStep() + fireEvent.click(later()) + await vi.waitFor(() => expect(server.held).toHaveLength(1)) + server.held[0]!.answer( + jsonResponse(400, { + error: { + code: 'invalid_settings', + message: 'invalid household settings: onboarding_completed: add at least 3 Regulars first (have 2)', + retryable: false, + }, + }), + ) + + expect(await screen.findByRole('alert')).toHaveTextContent( + 'Could not finish setting up: invalid household settings: onboarding_completed: add at least 3 Regulars first (have 2)', + ) + const addRegulars = screen.getByRole('button', { name: 'Add Regulars' }) + expectMinHitArea(addRegulars) + fireEvent.click(addRegulars) + expect(await screen.findByRole('heading', { level: 1, name: 'Regulars step' })).toBeInTheDocument() + }) + + it('offers no way back for any other refusal', async () => { + const server = await renderStep() + fireEvent.click(later()) + await vi.waitFor(() => expect(server.held).toHaveLength(1)) + server.held[0]!.answer(errorResponse(500, 'internal')) + + expect(await screen.findByRole('alert')).toHaveTextContent('Could not finish setting up: internal message') + expect(screen.queryByRole('button', { name: 'Add Regulars' })).not.toBeInTheDocument() + }) +}) + describe('notificationsUnavailable', () => { it('answers "unavailable" without asking for permission', async () => { await expect(notificationsUnavailable()).resolves.toBe('unavailable') diff --git a/web/src/onboarding/NotificationsStep.tsx b/web/src/onboarding/NotificationsStep.tsx index d8d5ed0..f17ddd1 100644 --- a/web/src/onboarding/NotificationsStep.tsx +++ b/web/src/onboarding/NotificationsStep.tsx @@ -2,7 +2,7 @@ import { useState } from 'react' import { CalendarDays, ShoppingBasket, Sparkles, UtensilsCrossed, type LucideIcon } from 'lucide-react' import { useNavigate } from 'react-router' import { InstallHint } from '../pwa/InstallHint.tsx' -import { useCompleteOnboarding } from '../settings/api.ts' +import { needsMoreRegulars, useCompleteOnboarding } from '../settings/api.ts' import { Button } from '../ui/Button.tsx' import { Icon } from '../ui/Icon.tsx' import { OnboardingStep } from './OnboardingStep.tsx' @@ -50,12 +50,15 @@ export type NotificationsStepProps = { // NotificationsStep is onboarding step 3: what MealQuest notifies about, the // iOS install hint, and finishing onboarding. Either button records that -// onboarding is finished and goes Home. +// onboarding is finished and goes Home. When the server refuses because the +// household has fewer than three Regulars, the reason shows with a way back +// to step 2 to add them. export function NotificationsStep({ enableNotifications = notificationsUnavailable }: NotificationsStepProps) { const navigate = useNavigate() const complete = useCompleteOnboarding() const [enabling, setEnabling] = useState(false) const [problem, setProblem] = useState(null) + const [fewRegulars, setFewRegulars] = useState(false) const busy = enabling || complete.isPending async function finish() { @@ -63,13 +66,19 @@ export function NotificationsStep({ enableNotifications = notificationsUnavailab await complete.mutateAsync() } catch (error) { setProblem(`Could not finish setting up: ${(error as Error).message}`) + setFewRegulars(needsMoreRegulars(error as Error)) return } void navigate('/', { replace: true }) } - async function turnOn() { + function clearProblem() { setProblem(null) + setFewRegulars(false) + } + + async function turnOn() { + clearProblem() setEnabling(true) try { await enableNotifications() @@ -83,7 +92,7 @@ export function NotificationsStep({ enableNotifications = notificationsUnavailab } function later() { - setProblem(null) + clearProblem() void finish() } @@ -124,6 +133,11 @@ export function NotificationsStep({ enableNotifications = notificationsUnavailab {problem}

)} + {fewRegulars && ( + + )} ) } diff --git a/web/src/onboarding/Onboarding.module.css b/web/src/onboarding/Onboarding.module.css index 477e2da..ef2703c 100644 --- a/web/src/onboarding/Onboarding.module.css +++ b/web/src/onboarding/Onboarding.module.css @@ -229,6 +229,11 @@ color: var(--color-accent-700); } +/* Step 3: back to step 2 when the server wants more Regulars. */ +.recover { + margin-top: var(--space-3); +} + .status { margin-top: 10px; } diff --git a/web/src/onboarding/OnboardingPage.test.tsx b/web/src/onboarding/OnboardingPage.test.tsx index e6209d4..ec00b9d 100644 --- a/web/src/onboarding/OnboardingPage.test.tsx +++ b/web/src/onboarding/OnboardingPage.test.tsx @@ -146,6 +146,14 @@ describe('onboarding step 2 (mock 1h)', () => { expect(declaredStyle(pills[0]!).height).toBe('6px') }) + it('does not offer a common pick the server would refuse as a duplicate of a Regular', async () => { + await regularsStep([dinnerBody('d1', 'Homemade chili'), dinnerBody('d2', 'Easy Chicken Stir Fry')]) + + expect(screen.queryByRole('button', { name: 'Add Chili' })).not.toBeInTheDocument() + expect(screen.queryByRole('button', { name: 'Add Chicken stir-fry' })).not.toBeInTheDocument() + expect(screen.getByRole('button', { name: 'Add Beef tacos' })).toBeInTheDocument() + }) + it('sizes the input pill, "+" button, picks and Next as the mock does', async () => { await regularsStep() expect(hitArea(input()).height).toBe(48) diff --git a/web/src/onboarding/RegularsStep.tsx b/web/src/onboarding/RegularsStep.tsx index c238670..8c991f3 100644 --- a/web/src/onboarding/RegularsStep.tsx +++ b/web/src/onboarding/RegularsStep.tsx @@ -2,6 +2,7 @@ import { useState, type FormEvent } from 'react' import { Plus } from 'lucide-react' import { useNavigate } from 'react-router' import { useAddRegular, useRegulars, useRemoveRegular, type Regular } from '../dinners/api.ts' +import { similarNames } from '../dinners/names.ts' import { useRefusals } from '../dinners/useRefusals.ts' import { Button } from '../ui/Button.tsx' import { Chip } from '../ui/Chip.tsx' @@ -16,7 +17,8 @@ import styles from './Onboarding.module.css' // household moves on. export const MIN_REGULARS = 3 -// commonPicks are offered as one-tap Regulars until the household has them. +// commonPicks are offered as one-tap Regulars until the household has them, +// or a Regular the server would count as the same dinner. export const commonPicks = [ 'Beef tacos', 'Spaghetti bolognese', @@ -28,10 +30,6 @@ export const commonPicks = [ 'Fried rice', ] -function sameName(a: string, b: string): boolean { - return a.trim().toLowerCase() === b.trim().toLowerCase() -} - // RegularsStep is onboarding step 2, handoff mock 1h: the household adds // the dinners it makes on repeat. Each add and remove shows at once and is // undone if the server refuses it. @@ -43,7 +41,7 @@ export function RegularsStep() { const [draft, setDraft] = useState('') const { refusals, track } = useRefusals() const list = regulars.data ?? [] - const picks = commonPicks.filter((pick) => !list.some((regular) => sameName(regular.name, pick))) + const picks = commonPicks.filter((pick) => !list.some((regular) => similarNames(regular.name, pick))) // Next waits for every add to be answered, so a refused add's error shows // here and an add the server takes back never counts towards the three. const adding = list.some((regular) => regular.pending) diff --git a/web/src/settings/api.ts b/web/src/settings/api.ts index b32b127..be27f9d 100644 --- a/web/src/settings/api.ts +++ b/web/src/settings/api.ts @@ -441,6 +441,18 @@ export function householdSettingsProblems(error: Error): HouseholdSettingsProble return problems } +// needsMoreRegulars reports whether error is the server refusing to finish +// onboarding because the household has fewer than three Regulars +// (internal/household MinRegulars) — the only refusal of +// {"onboarding_completed": true}. +export function needsMoreRegulars(error: Error): boolean { + return ( + error instanceof ApiError && + error.code === 'invalid_settings' && + error.message.startsWith(`${invalidHouseholdPrefix}onboarding_completed: `) + ) +} + // aiStatus is the AI assistant row's status line: the provider and the // model's display name, or that no model is chosen yet (the seed). export function aiStatus(settings: Pick): string {