diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index fb0927d..0b266bb 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -101,7 +101,7 @@ Columns ending in `_ct` hold AES-256-GCM ciphertext (via `secretbox`). | `suggestions` | dinner_id, batch_id, name (the name the dish was suggested under, kept whatever its dinner is later renamed, so a Pass stays an immutable record every later batch dedupes against), reason, cited_regular_ids (JSON), status, decided_at. | | `week_plans` | week_start (unique), status (`draft` / `synced` / `changed`), ai_note, ai_run_id, calendar_synced_at, revision (raised by every swap and stored draft). | | `plan_days` | plan_id, date (unique per plan), dinner_id, is_new, calendar_event_id. | -| `rotation_reviews` | dinner_id + plan_date (unique), status, decision (`keep` / `drop` / `not_made`), decided_at, updated_by. | +| `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, status (`current` / `archived`), sent_to_chat_at, frozen_at. | | `shopping_groups` / `shopping_items` | One group per dinner/day; item name, qty, needed (bool), position. | | `ai_settings` | One row: provider, base_url, model, temperature, system_prompt, api_key_ct, api_key_last4, key_verified_at. | @@ -378,12 +378,14 @@ automatic** (D-021). The check is closed with `decided_at` and the session's `updated_by`, and the answer is `{dinner, plan_date, decision, dinner_status, decided_at, updated_by}`. Any other decision is 400 `invalid_decision`, - an unknown dinner 404, a dinner with no pending check (including a - second decision) 409 `no_pending_review`, and one no longer tried 409 - `dinner_not_tried`. Keep is the only path from tried to Regular. A - dropped dinner's name joins the passed names (`ListPassedNames`) the - Side quest dedupe and household context use, so it is never suggested - again. + an unknown dinner or a dinner never checked 404 `not_found`, a dinner + whose checks are all decided (including a second decision) 409 + `no_pending_review`, and one no longer tried 409 `dinner_not_tried`. + Keep is the only path from tried to Regular. A drop records the + dinner's name on the check (`rotation_reviews.name`), and that name + joins the passed names (`ListPassedNames`) the Side quest dedupe and + household context use, so it is never suggested again, even after the + dinner is renamed. - `/health` — liveness. - Everything else serves the embedded PWA with SPA fallback. diff --git a/internal/db/household_context.sql.go b/internal/db/household_context.sql.go index a84a4dd..e03efdf 100644 --- a/internal/db/household_context.sql.go +++ b/internal/db/household_context.sql.go @@ -59,10 +59,9 @@ SELECT name FROM ( FROM suggestions WHERE status = 'declined' UNION ALL - SELECT dinners.name, rotation_reviews.decided_at, rotation_reviews.id + SELECT name, decided_at, id FROM rotation_reviews - JOIN dinners ON dinners.id = rotation_reviews.dinner_id - WHERE rotation_reviews.decision = 'drop' + WHERE decision = 'drop' ) ORDER BY decided_at DESC, id DESC LIMIT ? diff --git a/internal/db/models.go b/internal/db/models.go index 6a3998f..9cdf5d0 100644 --- a/internal/db/models.go +++ b/internal/db/models.go @@ -228,6 +228,7 @@ type RotationReview struct { PlanDate string Status string Decision sql.NullString + Name sql.NullString DecidedAt sql.NullString UpdatedBy sql.NullString CreatedAt string diff --git a/internal/db/querier.go b/internal/db/querier.go index 55be29f..88053ef 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -25,6 +25,8 @@ type Querier interface { CountDeck(ctx context.Context) (int64, error) // How many Regulars the household has; archived dinners are not counted. CountRegularDinners(ctx context.Context) (int64, error) + // How many Rotation checks dinner_id has had, pending or decided. + CountRotationReviews(ctx context.Context, dinnerID string) (int64, error) CreateAIRun(ctx context.Context, arg CreateAIRunParams) error CreateDinner(ctx context.Context, arg CreateDinnerParams) (Dinner, error) CreateJob(ctx context.Context, arg CreateJobParams) error @@ -40,8 +42,9 @@ type Querier interface { CreateSuggestion(ctx context.Context, arg CreateSuggestionParams) error CreateSuggestionBatch(ctx context.Context, arg CreateSuggestionBatchParams) (SuggestionBatch, error) CreateWeekPlan(ctx context.Context, arg CreateWeekPlanParams) error - // Records the household's decision on a pending Rotation check; one - // already decided is left as it is (0 rows). + // Records the household's decision on a pending Rotation check, with the + // dinner's name for a drop and NULL otherwise; one already decided is left + // as it is (0 rows). DecideRotationReview(ctx context.Context, arg DecideRotationReviewParams) (int64, error) // Records the household's decision on a pending suggestion; one already // decided is left as it is (0 rows). @@ -136,8 +139,8 @@ type Querier interface { ListDraftPool(ctx context.Context, arg ListDraftPoolParams) ([]Dinner, error) // Every name the household turned down, oldest decision first: the name // each declined suggestion was suggested under, which renaming its dinner - // does not change, and the name of each dinner a Rotation check dropped - // ("Not for us"), which archiving keeps. + // does not change, and the name each dinner a Rotation check dropped + // ("Not for us") had when dropped, which renaming it does not change. ListPassedNames(ctx context.Context) ([]string, error) // The pending Rotation checks whose dinner is still tried, oldest plan // date first, with their dinners. diff --git a/internal/db/rotation_reviews.sql.go b/internal/db/rotation_reviews.sql.go index cb613cd..bd819f0 100644 --- a/internal/db/rotation_reviews.sql.go +++ b/internal/db/rotation_reviews.sql.go @@ -10,6 +10,18 @@ import ( "database/sql" ) +const countRotationReviews = `-- name: CountRotationReviews :one +SELECT COUNT(*) FROM rotation_reviews WHERE dinner_id = ? +` + +// How many Rotation checks dinner_id has had, pending or decided. +func (q *Queries) CountRotationReviews(ctx context.Context, dinnerID string) (int64, error) { + row := q.db.QueryRowContext(ctx, countRotationReviews, dinnerID) + var count int64 + err := row.Scan(&count) + return count, err +} + const createRotationReview = `-- name: CreateRotationReview :exec INSERT INTO rotation_reviews (id, dinner_id, plan_date, status, created_at, updated_at) VALUES (?, ?, ?, 'pending', ?, ?) @@ -38,24 +50,28 @@ const decideRotationReview = `-- name: DecideRotationReview :execrows UPDATE rotation_reviews SET status = 'decided', decision = ?1, - decided_at = ?2, - updated_by = ?3, - updated_at = ?2 -WHERE id = ?4 AND status = 'pending' + name = ?2, + decided_at = ?3, + updated_by = ?4, + updated_at = ?3 +WHERE id = ?5 AND status = 'pending' ` type DecideRotationReviewParams struct { Decision sql.NullString + Name sql.NullString Now sql.NullString UpdatedBy sql.NullString ID string } -// Records the household's decision on a pending Rotation check; one -// already decided is left as it is (0 rows). +// Records the household's decision on a pending Rotation check, with the +// dinner's name for a drop and NULL otherwise; one already decided is left +// as it is (0 rows). func (q *Queries) DecideRotationReview(ctx context.Context, arg DecideRotationReviewParams) (int64, error) { result, err := q.db.ExecContext(ctx, decideRotationReview, arg.Decision, + arg.Name, arg.Now, arg.UpdatedBy, arg.ID, @@ -67,7 +83,7 @@ func (q *Queries) DecideRotationReview(ctx context.Context, arg DecideRotationRe } const getPendingRotationReview = `-- name: GetPendingRotationReview :one -SELECT id, dinner_id, plan_date, status, decision, decided_at, updated_by, created_at, updated_at FROM rotation_reviews +SELECT id, dinner_id, plan_date, status, decision, name, decided_at, updated_by, created_at, updated_at FROM rotation_reviews WHERE dinner_id = ? AND status = 'pending' ` @@ -83,6 +99,7 @@ func (q *Queries) GetPendingRotationReview(ctx context.Context, dinnerID string) &i.PlanDate, &i.Status, &i.Decision, + &i.Name, &i.DecidedAt, &i.UpdatedBy, &i.CreatedAt, diff --git a/internal/db/suggestions.sql.go b/internal/db/suggestions.sql.go index 1588fa2..e7f12d7 100644 --- a/internal/db/suggestions.sql.go +++ b/internal/db/suggestions.sql.go @@ -354,18 +354,17 @@ SELECT name FROM ( FROM suggestions WHERE status = 'declined' UNION ALL - SELECT dinners.name, rotation_reviews.decided_at, rotation_reviews.id + SELECT name, decided_at, id FROM rotation_reviews - JOIN dinners ON dinners.id = rotation_reviews.dinner_id - WHERE rotation_reviews.decision = 'drop' + WHERE decision = 'drop' ) ORDER BY decided_at, id ` // Every name the household turned down, oldest decision first: the name // each declined suggestion was suggested under, which renaming its dinner -// does not change, and the name of each dinner a Rotation check dropped -// ("Not for us"), which archiving keeps. +// does not change, and the name each dinner a Rotation check dropped +// ("Not for us") had when dropped, which renaming it does not change. func (q *Queries) ListPassedNames(ctx context.Context) ([]string, error) { rows, err := q.db.QueryContext(ctx, listPassedNames) if err != nil { diff --git a/internal/plan/rotation.go b/internal/plan/rotation.go index 6b4aa8d..129987e 100644 --- a/internal/plan/rotation.go +++ b/internal/plan/rotation.go @@ -32,8 +32,12 @@ var decisionStatus = map[string]string{ DecisionNotMade: dinners.StatusQuestLog, } -// ErrNoPendingReview is a decision on a dinner with no pending Rotation -// check: it was never tried, or its check is already decided. +// ErrReviewNotFound is a decision on a dinner that has never had a +// Rotation check: it was never tried. +var ErrReviewNotFound = errors.New("the dinner has no Rotation check") + +// ErrNoPendingReview is a decision on a dinner whose Rotation checks are +// all decided, such as a second decision. var ErrNoPendingReview = errors.New("the dinner has no pending Rotation check") // ErrDinnerNotTried is a decision on a pending Rotation check whose dinner @@ -130,9 +134,11 @@ func (s *Service) PendingReviews(ctx context.Context) ([]Review, error) { // DecideReview applies decision to the pending Rotation check of dinnerID // and closes it, recording updatedBy: keep makes the dinner a Regular, // drop archives it and not_made returns it to the Quest log; its origin is -// kept. An unknown dinner is ErrDinnerNotFound, a dinner with no pending -// check (including a second decision) ErrNoPendingReview, and one no -// longer tried ErrDinnerNotTried; none of them changes anything. +// kept; a drop records the dinner's name on the check, so renaming the +// dinner later never takes it off the passed names. An unknown dinner is +// ErrDinnerNotFound, a dinner never checked ErrReviewNotFound, one whose +// checks are all decided (such as a second decision) ErrNoPendingReview, +// and one no longer tried ErrDinnerNotTried; none of them changes anything. func (s *Service) DecideReview(ctx context.Context, dinnerID, decision, updatedBy string) (Decided, error) { status, ok := decisionStatus[decision] if !ok { @@ -153,6 +159,13 @@ func (s *Service) DecideReview(ctx context.Context, dinnerID, decision, updatedB } review, err := q.GetPendingRotationReview(ctx, d.ID) if errors.Is(err, sql.ErrNoRows) { + checks, err := q.CountRotationReviews(ctx, d.ID) + if err != nil { + return fmt.Errorf("plan: count Rotation checks of %s: %w", d.ID, err) + } + if checks == 0 { + return ErrReviewNotFound + } return ErrNoPendingReview } if err != nil { @@ -173,6 +186,7 @@ func (s *Service) DecideReview(ctx context.Context, dinnerID, decision, updatedB } closed, err := q.DecideRotationReview(ctx, db.DecideRotationReviewParams{ Decision: sql.NullString{String: decision, Valid: true}, + Name: sql.NullString{String: d.Name, Valid: decision == DecisionDrop}, Now: sql.NullString{String: now, Valid: true}, UpdatedBy: sql.NullString{String: updatedBy, Valid: true}, ID: review.ID, diff --git a/internal/plan/rotation_test.go b/internal/plan/rotation_test.go index 092cde7..f5b1f03 100644 --- a/internal/plan/rotation_test.go +++ b/internal/plan/rotation_test.go @@ -55,14 +55,14 @@ func (e *env) dinnerRow(t *testing.T, name string) db.Dinner { // review is a rotation_reviews row as stored. type review struct { - planDate, status string - decision, decidedAt, updatedBy sql.NullString + planDate, status string + decision, name, decidedAt, updatedBy sql.NullString } func (e *env) reviews(t *testing.T, name string) []review { t.Helper() rows, err := e.st.Reader.QueryContext(t.Context(), - `SELECT plan_date, status, decision, decided_at, updated_by FROM rotation_reviews WHERE dinner_id = ? ORDER BY plan_date`, e.ids[name]) + `SELECT plan_date, status, decision, name, decided_at, updated_by FROM rotation_reviews WHERE dinner_id = ? ORDER BY plan_date`, e.ids[name]) if err != nil { t.Fatal(err) } @@ -70,7 +70,7 @@ func (e *env) reviews(t *testing.T, name string) []review { var out []review for rows.Next() { var r review - if err := rows.Scan(&r.planDate, &r.status, &r.decision, &r.decidedAt, &r.updatedBy); err != nil { + if err := rows.Scan(&r.planDate, &r.status, &r.decision, &r.name, &r.decidedAt, &r.updatedBy); err != nil { t.Fatal(err) } out = append(out, r) @@ -103,7 +103,7 @@ func TestMarkTried(t *testing.T) { } got := e.reviews(t, "Pad Thai") if len(got) != 1 || got[0].planDate != padThaiDay.String() || got[0].status != "pending" || - got[0].decision.Valid || got[0].decidedAt.Valid || got[0].updatedBy.Valid { + got[0].decision.Valid || got[0].name.Valid || got[0].decidedAt.Valid || got[0].updatedBy.Valid { t.Fatalf("Pad Thai reviews = %+v, want one pending for %s", got, padThaiDay) } if got := e.dinnerRow(t, "Shawarma Bowls").Status; got != "quest_log" { @@ -197,10 +197,17 @@ func TestDecideReview(t *testing.T) { t.Errorf("Pad Thai = status %s origin %s name %s updated_by %v, want %s side_quest Pad Thai %s", row.Status, row.Origin, row.Name, row.UpdatedBy, tc.status, reviewer) } + // Only a drop records the dinner's name. + wantPassed := tc.decision == plan.DecisionDrop + wantName := sql.NullString{} + if wantPassed { + wantName = sql.NullString{String: "Pad Thai", Valid: true} + } got := e.reviews(t, "Pad Thai") if len(got) != 1 || got[0].status != "decided" || got[0].decision.String != tc.decision || - got[0].decidedAt.String != d.DecidedAt || got[0].updatedBy.String != reviewer { - t.Errorf("reviews = %+v, want one decided %s by %s at %s", got, tc.decision, reviewer, d.DecidedAt) + got[0].decidedAt.String != d.DecidedAt || got[0].updatedBy.String != reviewer || + got[0].name != wantName { + t.Errorf("reviews = %+v, want one decided %s by %s at %s, named only by a drop", got, tc.decision, reviewer, d.DecidedAt) } if pending, err := e.svc.PendingReviews(t.Context()); err != nil || len(pending) != 0 { t.Errorf("pending after deciding = %+v, %v, want none", pending, err) @@ -224,7 +231,6 @@ func TestDecideReview(t *testing.T) { if err != nil { t.Fatal(err) } - wantPassed := tc.decision == plan.DecisionDrop if slices.Contains(passed, "Pad Thai") != wantPassed || slices.Contains(recent, "Pad Thai") != wantPassed { t.Errorf("passed names = %v, recent %v; want Pad Thai among them: %t", passed, recent, wantPassed) } @@ -246,8 +252,8 @@ func TestDecideReviewRefused(t *testing.T) { t.Errorf("unknown dinner: %v, want ErrDinnerNotFound", err) } // Never tried: Shawarma Bowls is in the Quest log with no check. - if _, err := e.svc.DecideReview(ctx, e.ids["Shawarma Bowls"], plan.DecisionKeep, reviewer); !errors.Is(err, plan.ErrNoPendingReview) { - t.Errorf("dinner never tried: %v, want ErrNoPendingReview", err) + if _, err := e.svc.DecideReview(ctx, e.ids["Shawarma Bowls"], plan.DecisionKeep, reviewer); !errors.Is(err, plan.ErrReviewNotFound) { + t.Errorf("dinner never tried: %v, want ErrReviewNotFound", err) } if got := e.dinnerRow(t, "Shawarma Bowls").Status; got != "quest_log" { t.Errorf("Shawarma Bowls status = %s, want quest_log", got) diff --git a/internal/server/rotation.go b/internal/server/rotation.go index f59b2bd..a9f6232 100644 --- a/internal/server/rotation.go +++ b/internal/server/rotation.go @@ -64,9 +64,9 @@ func postRotationReview(logger *slog.Logger, svc *plan.Service) http.HandlerFunc } // writeRotationError maps a Rotation check error to its API error: a -// decision other than keep, drop or not_made is 400, an unknown dinner -// 404, and a dinner with no pending check (such as a second decision) or -// no longer tried 409. +// decision other than keep, drop or not_made is 400, an unknown dinner or +// a dinner never checked 404, and a dinner whose checks are all decided +// (such as a second decision) or no longer tried 409. func writeRotationError(w http.ResponseWriter, r *http.Request, logger *slog.Logger, action string, err error) { var invalid *plan.InvalidDecisionError switch { @@ -74,6 +74,8 @@ func writeRotationError(w http.ResponseWriter, r *http.Request, logger *slog.Log writeError(w, r, logger, http.StatusBadRequest, "invalid_decision", invalid.Error()) case errors.Is(err, plan.ErrDinnerNotFound): writeError(w, r, logger, http.StatusNotFound, "not_found", "dinner not found") + case errors.Is(err, plan.ErrReviewNotFound): + writeError(w, r, logger, http.StatusNotFound, "not_found", "rotation check not found") case errors.Is(err, plan.ErrNoPendingReview): writeError(w, r, logger, http.StatusConflict, "no_pending_review", err.Error()) case errors.Is(err, plan.ErrDinnerNotTried): diff --git a/internal/server/rotation_test.go b/internal/server/rotation_test.go index 503f1b8..05e4c15 100644 --- a/internal/server/rotation_test.go +++ b/internal/server/rotation_test.go @@ -97,6 +97,24 @@ func TestPostRotationReview(t *testing.T) { } } +// A dinner that has never had a Rotation check is 404; a second decision +// on one already decided is 409 (TestPostRotationReview). +func TestPostRotationReviewNeverChecked(t *testing.T) { + t.Parallel() + e := newPlansRouter(t) + rec, body := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+e.questLog+"/rotation-review", `{"decision":"keep"}`) + if rec.Code != http.StatusNotFound || errorCode(body) != "not_found" { + t.Errorf("never checked = %d %v, want 404 not_found", rec.Code, body) + } + var status string + if err := e.st.Reader.QueryRowContext(t.Context(), `SELECT status FROM dinners WHERE id = ?`, e.questLog).Scan(&status); err != nil { + t.Fatal(err) + } + if status != "quest_log" { + t.Errorf("status after a refused decision = %s, want quest_log", status) + } +} + func TestPostRotationReviewRefused(t *testing.T) { t.Parallel() e := newPlansRouter(t) diff --git a/internal/store/migrate_test.go b/internal/store/migrate_test.go index 9662138..5533fdc 100644 --- a/internal/store/migrate_test.go +++ b/internal/store/migrate_test.go @@ -176,8 +176,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" { - 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", 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" { + 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", got) } } @@ -197,8 +197,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" { - 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", 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" { + 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", got) } if !tableExists(t, s, "dinners") { t.Error("table from 0001_init was not created") @@ -633,3 +633,88 @@ func TestMigrateSuggestionName(t *testing.T) { t.Error("suggestions_copy left behind") } } + +// TestMigrateRotationReviewName upgrades a database at 0013 holding a +// pending, a kept and a dropped Rotation check and checks 0014 keeps all +// three, gives only the drop its dinner's name, and refuses a drop with no +// name or a name on any other check. +func TestMigrateRotationReviewName(t *testing.T) { + ctx := context.Background() + s := openTestStore(t) + before := fstest.MapFS{} + for _, name := range []string{ + "0000_bootstrap.sql", "0001_init.sql", "0002_household_settings_updated_by.sql", "0003_jobs_status.sql", + "0004_ai_provider_presets.sql", "0005_ai_provider_xai.sql", "0006_ai_runs_status.sql", "0007_recipe_grounding.sql", + "0008_dinner_recipe_url.sql", "0009_dinner_grounding_error.sql", "0010_dinner_grounding_revision.sql", + "0011_ground_dinner_job_revision.sql", "0012_suggestion_name.sql", "0013_week_plan_revision.sql", + } { + body, err := fs.ReadFile(migrations.FS, name) + if err != nil { + t.Fatal(err) + } + before[name] = &fstest.MapFile{Data: body} + } + if err := Migrate(ctx, s.Writer, before); err != nil { + t.Fatal(err) + } + for _, stmt := range []string{ + `INSERT INTO dinners (id, name, normalized_name, status, origin, is_favorite, grounding_status, created_at, updated_at) + VALUES ('dinner-1', 'Pho', 'pho', 'tried', 'side_quest', 0, 'grounded', 'c', 'u'), + ('dinner-2', 'Ramen', 'ramen', 'regular', 'side_quest', 0, 'grounded', 'c', 'u'), + ('dinner-3', 'Laksa', 'laksa', 'archived', 'side_quest', 0, 'grounded', 'c', 'u')`, + `INSERT INTO rotation_reviews (id, dinner_id, plan_date, status, decision, decided_at, updated_by, created_at, updated_at) + VALUES ('review-1', 'dinner-1', '2026-10-12', 'pending', NULL, NULL, NULL, 'c1', 'u1'), + ('review-2', 'dinner-2', '2026-10-13', 'decided', 'keep', 'd2', 'member-1', 'c2', 'u2'), + ('review-3', 'dinner-3', '2026-10-14', 'decided', 'drop', 'd3', 'member-1', 'c3', 'u3')`, + } { + 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.Query(`SELECT id, dinner_id, plan_date, status, COALESCE(decision, ''), COALESCE(name, ''), + COALESCE(decided_at, ''), COALESCE(updated_by, ''), created_at, updated_at + FROM rotation_reviews ORDER BY id`) + if err != nil { + t.Fatal(err) + } + var got [][]string + for rows.Next() { + r := make([]string, 10) + if err := rows.Scan(&r[0], &r[1], &r[2], &r[3], &r[4], &r[5], &r[6], &r[7], &r[8], &r[9]); err != nil { + t.Fatal(err) + } + got = append(got, r) + } + if err := rows.Close(); err != nil { + t.Fatal(err) + } + want := [][]string{ + {"review-1", "dinner-1", "2026-10-12", "pending", "", "", "", "", "c1", "u1"}, + {"review-2", "dinner-2", "2026-10-13", "decided", "keep", "", "d2", "member-1", "c2", "u2"}, + {"review-3", "dinner-3", "2026-10-14", "decided", "drop", "Laksa", "d3", "member-1", "c3", "u3"}, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("rotation_reviews after 0014 =\n %v\nwant\n %v", got, want) + } + for _, tc := range []struct{ name, stmt, want string }{ + {"drop with no name", `UPDATE rotation_reviews SET name = NULL WHERE id = 'review-3'`, "CHECK constraint failed"}, + {"empty name", `UPDATE rotation_reviews SET name = '' WHERE id = 'review-3'`, "CHECK constraint failed"}, + {"name on a keep", `UPDATE rotation_reviews SET name = 'Ramen' WHERE id = 'review-2'`, "CHECK constraint failed"}, + {"name on a pending check", `UPDATE rotation_reviews SET name = 'Pho' WHERE id = 'review-1'`, "CHECK constraint failed"}, + {"unknown dinner", `UPDATE rotation_reviews SET dinner_id = 'nope' WHERE id = 'review-1'`, "FOREIGN KEY constraint failed"}, + {"second check of a dinner and date", `INSERT INTO rotation_reviews (id, dinner_id, plan_date, status, created_at, updated_at) + VALUES ('review-4', 'dinner-1', '2026-10-12', 'pending', 'c', 'u')`, "UNIQUE constraint failed"}, + } { + if _, err := s.Writer.Exec(tc.stmt); err == nil || !strings.Contains(err.Error(), tc.want) { + t.Errorf("%s: error %v, want %s", tc.name, err, tc.want) + } + } + if tableExists(t, s, "rotation_reviews_copy") { + t.Error("rotation_reviews_copy left behind") + } +} diff --git a/internal/suggest/suggest_test.go b/internal/suggest/suggest_test.go index 7bc5b24..469579a 100644 --- a/internal/suggest/suggest_test.go +++ b/internal/suggest/suggest_test.go @@ -21,6 +21,7 @@ import ( "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/dinners" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/household" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/jobs" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/plan" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes/recipestest" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store" @@ -912,6 +913,74 @@ func TestLaterBatchNeverResuggestsRenamedPass(t *testing.T) { } } +// TestLaterBatchNeverResuggestsRenamedDrop drops a tried dinner at its +// Rotation check ("Not for us"), renames its archived dinner, and checks +// the next batch still refuses the name it was dropped under. +func TestLaterBatchNeverResuggestsRenamedDrop(t *testing.T) { + e := newEnv(t) + ctx := t.Context() + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + dinnerSvc, err := dinners.NewService(e.st, e.grounder, e.runner, logger) + if err != nil { + t.Fatal(err) + } + fail := func(context.Context, string, skills.WeekRequest) (skills.Week, error) { + return skills.Week{}, errors.New("no draft in this test") + } + planSvc, err := plan.NewService(e.st, fail, e.runner, logger) + if err != nil { + t.Fatal(err) + } + id := e.dinner(t, "Pad Thai", "quest_log") + q := db.New(e.st.Writer) + now := store.Now().String() + planID := store.NewID() + if err := q.CreateWeekPlan(ctx, db.CreateWeekPlanParams{ID: planID, WeekStart: "2026-10-12", CreatedAt: now, UpdatedAt: now}); err != nil { + t.Fatal(err) + } + if err := q.PutPlanDay(ctx, db.PutPlanDayParams{ID: store.NewID(), PlanID: planID, Date: "2026-10-14", + DinnerID: sql.NullString{String: id, Valid: true}, CreatedAt: now, UpdatedAt: now}); err != nil { + t.Fatal(err) + } + day, err := store.ParseLocalDate("2026-10-14") + if err != nil { + t.Fatal(err) + } + if n, err := planSvc.MarkTried(ctx, day); err != nil || n != 1 { + t.Fatalf("MarkTried = %d, %v, want 1", n, err) + } + if _, err := planSvc.DecideReview(ctx, id, plan.DecisionDrop, member); err != nil { + t.Fatal(err) + } + renamed := "Peanut Noodles" + if _, err := dinnerSvc.Update(ctx, id, dinners.Patch{Name: &renamed}, member); err != nil { + t.Fatal(err) + } + + // The respelled dropped dish would ground, so only the passed names + // keep it out of the deck. + e.recipe(t, "pad thai!") + e.recipe(t, "Beef Ragu") + e.reply( + sideQuest("pad thai!", "Noodles, like your Chicken Fajitas.", "Chicken Fajitas"), + sideQuest("Beef Ragu", "Slow beef like your Beef Chili.", "Beef Chili"), + ) + e.start(t) + res := e.runBatch(t) + if len(res.SuggestionIDs) != 1 || res.Dropped != 1 { + t.Errorf("batch result = %+v, want 1 kept and the dropped dish dropped", res) + } + if got := e.deckNames(t); !slices.Equal(got, []string{"Beef Ragu"}) { + t.Errorf("deck = %v, want [Beef Ragu]", got) + } + e.mu.Lock() + in := e.inputs[0] + e.mu.Unlock() + if !slices.Equal(in.Declined, []string{"Pad Thai"}) || !slices.Equal(in.Context.Passed, []string{"Pad Thai"}) { + t.Errorf("batch declined %v, passed %v; want the name it was dropped under in both", in.Declined, in.Context.Passed) + } +} + // TestDecisionNeverCommitsWithoutItsReplenish makes the replenish batch's // job fail to store: the decision must not be recorded either, so the // retry decides again and starts the replenish. diff --git a/migrations/0014_rotation_review_name.sql b/migrations/0014_rotation_review_name.sql new file mode 100644 index 0000000..b1fd14c --- /dev/null +++ b/migrations/0014_rotation_review_name.sql @@ -0,0 +1,43 @@ +-- 0014_rotation_review_name: a Rotation check decided drop ("Not for us") +-- keeps the name its dinner had when the household dropped it. The dinner +-- can be renamed, even once archived, so the passed names every later +-- batch refuses are read from here, never from the dinner, as 0012 does +-- for a Pass. Only a drop has a name. A check dropped before this takes +-- its dinner's name now. +-- SQLite cannot add a column with a CHECK that existing drops would fail +-- before they are filled in, so the table is rebuilt as 0012 does: copy +-- the rows aside, drop the table, create it again, copy the rows back and +-- drop the copy. Nothing references rotation_reviews. + +CREATE TABLE rotation_reviews_copy AS SELECT * FROM rotation_reviews; + +DROP TABLE rotation_reviews; + +CREATE TABLE rotation_reviews ( + id TEXT NOT NULL PRIMARY KEY, + dinner_id TEXT NOT NULL REFERENCES dinners (id), + plan_date TEXT NOT NULL, + status TEXT NOT NULL CHECK (status IN ('pending', 'decided')), + decision TEXT CHECK (decision IN ('keep', 'drop', 'not_made')), + name TEXT CHECK (name <> ''), -- the dinner's name when dropped + decided_at TEXT, + updated_by TEXT, + created_at TEXT NOT NULL, + updated_at TEXT NOT NULL, + UNIQUE (dinner_id, plan_date), + CHECK ((decision IS 'drop') = (name IS NOT NULL)) +); + +INSERT INTO rotation_reviews ( + id, dinner_id, plan_date, status, decision, name, decided_at, updated_by, created_at, updated_at +) +SELECT + rotation_reviews_copy.id, rotation_reviews_copy.dinner_id, rotation_reviews_copy.plan_date, + rotation_reviews_copy.status, rotation_reviews_copy.decision, + CASE WHEN rotation_reviews_copy.decision = 'drop' THEN dinners.name END, + rotation_reviews_copy.decided_at, rotation_reviews_copy.updated_by, + rotation_reviews_copy.created_at, rotation_reviews_copy.updated_at +FROM rotation_reviews_copy +JOIN dinners ON dinners.id = rotation_reviews_copy.dinner_id; + +DROP TABLE rotation_reviews_copy; diff --git a/migrations/queries/household_context.sql b/migrations/queries/household_context.sql index 78f5b20..38f5c3c 100644 --- a/migrations/queries/household_context.sql +++ b/migrations/queries/household_context.sql @@ -8,10 +8,9 @@ SELECT name FROM ( FROM suggestions WHERE status = 'declined' UNION ALL - SELECT dinners.name, rotation_reviews.decided_at, rotation_reviews.id + SELECT name, decided_at, id FROM rotation_reviews - JOIN dinners ON dinners.id = rotation_reviews.dinner_id - WHERE rotation_reviews.decision = 'drop' + WHERE decision = 'drop' ) ORDER BY decided_at DESC, id DESC LIMIT ?; diff --git a/migrations/queries/rotation_reviews.sql b/migrations/queries/rotation_reviews.sql index 073bd4d..2d857d2 100644 --- a/migrations/queries/rotation_reviews.sql +++ b/migrations/queries/rotation_reviews.sql @@ -40,12 +40,18 @@ ORDER BY rotation_reviews.plan_date, rotation_reviews.id; SELECT * FROM rotation_reviews WHERE dinner_id = ? AND status = 'pending'; +-- name: CountRotationReviews :one +-- How many Rotation checks dinner_id has had, pending or decided. +SELECT COUNT(*) FROM rotation_reviews WHERE dinner_id = ?; + -- name: DecideRotationReview :execrows --- Records the household's decision on a pending Rotation check; one --- already decided is left as it is (0 rows). +-- Records the household's decision on a pending Rotation check, with the +-- dinner's name for a drop and NULL otherwise; one already decided is left +-- as it is (0 rows). UPDATE rotation_reviews SET status = 'decided', decision = sqlc.arg(decision), + name = sqlc.narg(name), decided_at = sqlc.arg(now), updated_by = sqlc.arg(updated_by), updated_at = sqlc.arg(now) diff --git a/migrations/queries/suggestions.sql b/migrations/queries/suggestions.sql index 8f23acd..1068622 100644 --- a/migrations/queries/suggestions.sql +++ b/migrations/queries/suggestions.sql @@ -89,17 +89,16 @@ WHERE suggestions.status = 'pending' AND dinners.status = 'suggested'; -- name: ListPassedNames :many -- Every name the household turned down, oldest decision first: the name -- each declined suggestion was suggested under, which renaming its dinner --- does not change, and the name of each dinner a Rotation check dropped --- ("Not for us"), which archiving keeps. +-- does not change, and the name each dinner a Rotation check dropped +-- ("Not for us") had when dropped, which renaming it does not change. SELECT name FROM ( SELECT name, decided_at, id FROM suggestions WHERE status = 'declined' UNION ALL - SELECT dinners.name, rotation_reviews.decided_at, rotation_reviews.id + SELECT name, decided_at, id FROM rotation_reviews - JOIN dinners ON dinners.id = rotation_reviews.dinner_id - WHERE rotation_reviews.decision = 'drop' + WHERE decision = 'drop' ) ORDER BY decided_at, id;