From 8552f1d1526c474ba4eeff78e97b37ee26b22e6b Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 18:16:40 -0400 Subject: [PATCH 1/3] API-level tests for the /seed-lots route group (#83) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every other handler file had a sibling API test; this group had none, which is the state PATCH/DELETE /journal/:id shipped in — implemented, unit-tested through the service, and completely unreachable. Service tests cannot see a route that was never registered or one registered with the wrong :param. Four tests, covering what a broken route would silently take with it: - Full CRUD over HTTP, including GET/PATCH/DELETE by id, the ?plantId= filter, a rejected non-numeric plantId, and a stale-version 409 carrying the current row so the client can rebase. - `remaining` is derived through the HTTP surface, not just in the service. DESIGN.md makes derivation load-bearing, and the number is the whole reason anyone opens the seed shelf. - Lots are private to the buyer: another user gets 404 (not 403 — existence is masked per the project convention) on get/patch/delete, sees none in their listing, and the owner still has access afterwards. - The group is behind requireAuth: unauthenticated calls 401 rather than returning an empty list. Verified the tests actually catch the failure they exist for by unregistering GET /seed-lots/:id — all four fail with "404 page not found", the journal signature. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- internal/api/seed_lots_test.go | 245 +++++++++++++++++++++++++++++++++ 1 file changed, 245 insertions(+) create mode 100644 internal/api/seed_lots_test.go diff --git a/internal/api/seed_lots_test.go b/internal/api/seed_lots_test.go new file mode 100644 index 0000000..8260717 --- /dev/null +++ b/internal/api/seed_lots_test.go @@ -0,0 +1,245 @@ +package api + +import ( + "encoding/json" + "net/http" + "strconv" + "testing" + + "github.com/gin-gonic/gin" +) + +func seedLotPath(id int64) string { + return "/api/v1/seed-lots/" + strconv.FormatInt(id, 10) +} + +// decodeList decodes a bare JSON array body. Seed lot listing returns the array +// directly rather than wrapping it (unlike /journal's {"entries": …}), so a +// helper that assumed an object would quietly read nothing. +func decodeList(t *testing.T, body []byte) []any { + t.Helper() + var out []any + if err := json.Unmarshal(body, &out); err != nil { + t.Fatalf("decode list: %v (%s)", err, body) + } + return out +} + +// createPlantAPI makes a custom plant and returns its id. +func createPlantAPI(t *testing.T, r *gin.Engine, cookie *http.Cookie, name string, spacing float64) int64 { + t.Helper() + w := doJSON(t, r, http.MethodPost, "/api/v1/plants", map[string]any{ + "name": name, "category": "vegetable", "spacingCm": spacing, "color": "#4a7c3f", "icon": "🌱", + }, cookie) + if w.Code != http.StatusCreated { + t.Fatalf("create plant %q: status %d, body %s", name, w.Code, w.Body.String()) + } + return int64(decodeMap(t, w.Body.Bytes())["id"].(float64)) +} + +// TestSeedLotCrudAPI walks the whole seed lot lifecycle over HTTP. +// +// It exists for the reason CLAUDE.md gives: service tests cannot see a route +// that was never registered, or one registered with the wrong :param name. Every +// other handler file had a sibling API test; this group did not, which is the +// state PATCH/DELETE /journal/:id shipped in — implemented, unit-tested, and +// completely unreachable. +func TestSeedLotCrudAPI(t *testing.T) { + r := authEngine(t, localCfg()) + cookie := registerAndCookie(t, r, "lots@example.com") + plantID := createPlantAPI(t, r, cookie, "Music Garlic", 15) + + // Create. + w := doJSON(t, r, http.MethodPost, "/api/v1/seed-lots", map[string]any{ + "plantId": plantID, "vendor": "Johnny's", "sku": "2761", + "quantity": 100, "unit": "seeds", "packedForYear": 2026, "costCents": 495, + }, cookie) + if w.Code != http.StatusCreated { + t.Fatalf("create: status %d, body %s", w.Code, w.Body.String()) + } + lot := decodeMap(t, w.Body.Bytes()) + id := int64(lot["id"].(float64)) + if lot["vendor"] != "Johnny's" || lot["unit"] != "seeds" { + t.Errorf("unexpected lot: %+v", lot) + } + + // GET by id — the route most likely to be missing or mis-registered. + w = doJSON(t, r, http.MethodGet, seedLotPath(id), nil, cookie) + if w.Code != http.StatusOK { + t.Fatalf("get: status %d, body %s", w.Code, w.Body.String()) + } + if got := decodeMap(t, w.Body.Bytes()); int64(got["id"].(float64)) != id { + t.Errorf("get returned id %v, want %d", got["id"], id) + } + + // List, and the ?plantId= filter. + w = doJSON(t, r, http.MethodGet, "/api/v1/seed-lots", nil, cookie) + if w.Code != http.StatusOK { + t.Fatalf("list: status %d, body %s", w.Code, w.Body.String()) + } + if n := len(decodeList(t, w.Body.Bytes())); n != 1 { + t.Fatalf("list returned %d lots, want 1: %s", n, w.Body.String()) + } + + other := createPlantAPI(t, r, cookie, "Cherokee Purple", 45) + w = doJSON(t, r, http.MethodGet, "/api/v1/seed-lots?plantId="+strconv.FormatInt(other, 10), nil, cookie) + if w.Code != http.StatusOK { + t.Fatalf("filtered list: status %d, body %s", w.Code, w.Body.String()) + } + if n := len(decodeList(t, w.Body.Bytes())); n != 0 { + t.Errorf("filter by a plant with no lots returned %d, want 0", n) + } + if w := doJSON(t, r, http.MethodGet, "/api/v1/seed-lots?plantId=nope", nil, cookie); w.Code != http.StatusBadRequest { + t.Errorf("bad plantId = %d, want 400", w.Code) + } + + // PATCH with the current version. + w = doJSON(t, r, http.MethodPatch, seedLotPath(id), map[string]any{ + "vendor": "Fedco", "version": lot["version"], + }, cookie) + if w.Code != http.StatusOK { + t.Fatalf("patch: status %d, body %s", w.Code, w.Body.String()) + } + updated := decodeMap(t, w.Body.Bytes()) + if updated["vendor"] != "Fedco" { + t.Errorf("vendor = %v, want Fedco", updated["vendor"]) + } + if updated["version"].(float64) != lot["version"].(float64)+1 { + t.Errorf("patch didn't bump version: %v -> %v", lot["version"], updated["version"]) + } + + // A stale version conflicts and carries the current row back, so the client + // can rebase without a second request. + w = doJSON(t, r, http.MethodPatch, seedLotPath(id), map[string]any{ + "vendor": "stale", "version": lot["version"], + }, cookie) + if w.Code != http.StatusConflict { + t.Fatalf("stale patch: status %d, want 409", w.Code) + } + if cur, ok := decodeMap(t, w.Body.Bytes())["current"].(map[string]any); !ok || cur["vendor"] != "Fedco" { + t.Errorf("409 body missing the current row: %s", w.Body.String()) + } + + // DELETE. + if w := doJSON(t, r, http.MethodDelete, seedLotPath(id), nil, cookie); w.Code != http.StatusNoContent { + t.Fatalf("delete: status %d, body %s", w.Code, w.Body.String()) + } + if w := doJSON(t, r, http.MethodGet, seedLotPath(id), nil, cookie); w.Code != http.StatusNotFound { + t.Errorf("get after delete = %d, want 404", w.Code) + } +} + +// TestSeedLotRemainingIsDerivedAPI checks that `remaining` reflects plantings +// through the HTTP surface, not just in the service. +// +// DESIGN.md makes derivation load-bearing — "a decremented column drifts the +// moment a planting is edited behind its back" — so a route that returned a +// stored or stale figure would break the invariant silently, and the number is +// the whole reason anyone opens the seed shelf. +func TestSeedLotRemainingIsDerivedAPI(t *testing.T) { + r := authEngine(t, localCfg()) + cookie := registerAndCookie(t, r, "remaining@example.com") + plantID := createPlantAPI(t, r, cookie, "Beans", 10) + + w := doJSON(t, r, http.MethodPost, "/api/v1/seed-lots", map[string]any{ + "plantId": plantID, "quantity": 50, "unit": "seeds", + }, cookie) + if w.Code != http.StatusCreated { + t.Fatalf("create lot: status %d, body %s", w.Code, w.Body.String()) + } + lotID := int64(decodeMap(t, w.Body.Bytes())["id"].(float64)) + + gid := createGardenAPI(t, r, cookie, "G") + w = doJSON(t, r, http.MethodPost, objectsPath(gid), map[string]any{ + "kind": "bed", "widthCm": 100, "heightCm": 100, "plantable": true, + }, cookie) + if w.Code != http.StatusCreated { + t.Fatalf("create object: status %d, body %s", w.Code, w.Body.String()) + } + objID := int64(decodeMap(t, w.Body.Bytes())["id"].(float64)) + + // Plant 12 of them against the lot. + w = doJSON(t, r, http.MethodPost, objectPath(objID)+"/plantings", map[string]any{ + "plantId": plantID, "xCm": 0, "yCm": 0, "radiusCm": 20, "count": 12, "seedLotId": lotID, + }, cookie) + if w.Code != http.StatusCreated { + t.Fatalf("create planting: status %d, body %s", w.Code, w.Body.String()) + } + + w = doJSON(t, r, http.MethodGet, seedLotPath(lotID), nil, cookie) + if w.Code != http.StatusOK { + t.Fatalf("get lot: status %d, body %s", w.Code, w.Body.String()) + } + got := decodeMap(t, w.Body.Bytes()) + if rem, ok := got["remaining"].(float64); !ok || rem != 38 { + t.Errorf("remaining = %v, want 38 (50 bought - 12 planted): %s", got["remaining"], w.Body.String()) + } +} + +// TestSeedLotsArePrivateAPI checks the ACL through the router. +// +// Lots are private to the buyer and deliberately never travel with a shared +// garden, so another user must not be able to read or edit one. Per the +// project's convention, no-access is ErrNotFound rather than ErrForbidden — +// existence is masked — so every one of these is a 404, not a 403. +func TestSeedLotsArePrivateAPI(t *testing.T) { + r := authEngine(t, localCfg()) + alice := registerAndCookie(t, r, "alice-lots@example.com") + bob := registerAndCookie(t, r, "bob-lots@example.com") + + plantID := createPlantAPI(t, r, alice, "Alice's garlic", 15) + w := doJSON(t, r, http.MethodPost, "/api/v1/seed-lots", map[string]any{ + "plantId": plantID, "vendor": "Secret Vendor", "quantity": 10, "unit": "bulbs", + }, alice) + if w.Code != http.StatusCreated { + t.Fatalf("alice create: status %d, body %s", w.Code, w.Body.String()) + } + lotID := int64(decodeMap(t, w.Body.Bytes())["id"].(float64)) + + for _, tc := range []struct { + name string + method string + body any + }{ + {"get", http.MethodGet, nil}, + {"patch", http.MethodPatch, map[string]any{"vendor": "hijacked", "version": 1}}, + {"delete", http.MethodDelete, nil}, + } { + if w := doJSON(t, r, tc.method, seedLotPath(lotID), tc.body, bob); w.Code != http.StatusNotFound { + t.Errorf("bob %s = %d, want 404 (existence is masked)", tc.name, w.Code) + } + } + + // Bob's own listing must not include it either. + w = doJSON(t, r, http.MethodGet, "/api/v1/seed-lots", nil, bob) + if w.Code != http.StatusOK { + t.Fatalf("bob list: status %d", w.Code) + } + if n := len(decodeList(t, w.Body.Bytes())); n != 0 { + t.Errorf("bob sees %d of alice's lots, want 0: %s", n, w.Body.String()) + } + + // And it's still intact for alice. + if w := doJSON(t, r, http.MethodGet, seedLotPath(lotID), nil, alice); w.Code != http.StatusOK { + t.Errorf("alice lost access to her own lot: %d", w.Code) + } +} + +// TestSeedLotsRequireAuthAPI: the group is behind requireAuth, and an +// unauthenticated caller gets 401 rather than an empty list. +func TestSeedLotsRequireAuthAPI(t *testing.T) { + r := authEngine(t, localCfg()) + for _, tc := range []struct { + method, path string + }{ + {http.MethodGet, "/api/v1/seed-lots"}, + {http.MethodPost, "/api/v1/seed-lots"}, + {http.MethodGet, seedLotPath(1)}, + {http.MethodPatch, seedLotPath(1)}, + {http.MethodDelete, seedLotPath(1)}, + } { + if w := doJSON(t, r, tc.method, tc.path, nil, nil); w.Code != http.StatusUnauthorized { + t.Errorf("%s %s = %d, want 401", tc.method, tc.path, w.Code) + } + } +} From 5bdaf218287dbd7204e3a79a3a4ee1a1c8bff00a Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 20:39:43 -0400 Subject: [PATCH 2/3] Address Gadfly findings on #88 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Cover the plantId filter's out-of-range branch (0, -1), not just the non-numeric one — the handler rejects id < 1. - Add the documented negative-remaining case: over-planting a lot (57 against 50 bought) drives remaining to -7 through the HTTP surface. The number is a derived truth about what's committed, not a floor at zero, and that's the signal a gardener wants. - Rename objID → oid in the remaining test to match the package convention (shares_test etc.). Not taken: relocating createPlantAPI to plants_test.go — it's shared with #89's branch and the move is cleanest once both land (consolidating with that branch's makeFillPlant), noted on both PRs. The 409 "current" assertion style is deliberate and reads clearly; matching journal_test.go's exact phrasing isn't worth a divergence churn. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- internal/api/seed_lots_test.go | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/internal/api/seed_lots_test.go b/internal/api/seed_lots_test.go index 8260717..332a353 100644 --- a/internal/api/seed_lots_test.go +++ b/internal/api/seed_lots_test.go @@ -89,8 +89,12 @@ func TestSeedLotCrudAPI(t *testing.T) { if n := len(decodeList(t, w.Body.Bytes())); n != 0 { t.Errorf("filter by a plant with no lots returned %d, want 0", n) } - if w := doJSON(t, r, http.MethodGet, "/api/v1/seed-lots?plantId=nope", nil, cookie); w.Code != http.StatusBadRequest { - t.Errorf("bad plantId = %d, want 400", w.Code) + // A bad plantId filter is 400, whether non-numeric or out of range — the + // handler rejects id < 1, not just unparseable strings. + for _, bad := range []string{"nope", "0", "-1"} { + if w := doJSON(t, r, http.MethodGet, "/api/v1/seed-lots?plantId="+bad, nil, cookie); w.Code != http.StatusBadRequest { + t.Errorf("plantId=%q filter = %d, want 400", bad, w.Code) + } } // PATCH with the current version. @@ -156,10 +160,10 @@ func TestSeedLotRemainingIsDerivedAPI(t *testing.T) { if w.Code != http.StatusCreated { t.Fatalf("create object: status %d, body %s", w.Code, w.Body.String()) } - objID := int64(decodeMap(t, w.Body.Bytes())["id"].(float64)) + oid := int64(decodeMap(t, w.Body.Bytes())["id"].(float64)) // Plant 12 of them against the lot. - w = doJSON(t, r, http.MethodPost, objectPath(objID)+"/plantings", map[string]any{ + w = doJSON(t, r, http.MethodPost, objectPath(oid)+"/plantings", map[string]any{ "plantId": plantID, "xCm": 0, "yCm": 0, "radiusCm": 20, "count": 12, "seedLotId": lotID, }, cookie) if w.Code != http.StatusCreated { @@ -174,6 +178,21 @@ func TestSeedLotRemainingIsDerivedAPI(t *testing.T) { if rem, ok := got["remaining"].(float64); !ok || rem != 38 { t.Errorf("remaining = %v, want 38 (50 bought - 12 planted): %s", got["remaining"], w.Body.String()) } + + // Over-plant it: 45 more (57 total against 50 bought) drives remaining + // NEGATIVE. That's deliberate — the number is a derived truth about what + // you've committed, not a floor clamped at zero, and "you've planted more than + // you bought" is exactly the signal a gardener wants rather than a hidden -7. + w = doJSON(t, r, http.MethodPost, objectPath(oid)+"/plantings", map[string]any{ + "plantId": plantID, "xCm": 40, "yCm": 40, "radiusCm": 20, "count": 45, "seedLotId": lotID, + }, cookie) + if w.Code != http.StatusCreated { + t.Fatalf("over-plant: status %d, body %s", w.Code, w.Body.String()) + } + w = doJSON(t, r, http.MethodGet, seedLotPath(lotID), nil, cookie) + if rem, ok := decodeMap(t, w.Body.Bytes())["remaining"].(float64); !ok || rem != -7 { + t.Errorf("remaining after over-planting = %v, want -7 (50 - 57)", rem) + } } // TestSeedLotsArePrivateAPI checks the ACL through the router. From 4c4abe23c6a0a3cc1ed5c9f497b2eb9d0f55cb1e Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 22:06:57 -0400 Subject: [PATCH 3/3] Use objectPlantingsPath helper instead of inline URL (#88) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gadfly: I was wrong that no plantings-path helper existed — objectPlantingsPath is defined in plantings_test.go in the same package. Use it for the two planting POSTs in the remaining test. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- internal/api/seed_lots_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/api/seed_lots_test.go b/internal/api/seed_lots_test.go index 332a353..55735a6 100644 --- a/internal/api/seed_lots_test.go +++ b/internal/api/seed_lots_test.go @@ -163,7 +163,7 @@ func TestSeedLotRemainingIsDerivedAPI(t *testing.T) { oid := int64(decodeMap(t, w.Body.Bytes())["id"].(float64)) // Plant 12 of them against the lot. - w = doJSON(t, r, http.MethodPost, objectPath(oid)+"/plantings", map[string]any{ + w = doJSON(t, r, http.MethodPost, objectPlantingsPath(oid), map[string]any{ "plantId": plantID, "xCm": 0, "yCm": 0, "radiusCm": 20, "count": 12, "seedLotId": lotID, }, cookie) if w.Code != http.StatusCreated { @@ -183,7 +183,7 @@ func TestSeedLotRemainingIsDerivedAPI(t *testing.T) { // NEGATIVE. That's deliberate — the number is a derived truth about what // you've committed, not a floor clamped at zero, and "you've planted more than // you bought" is exactly the signal a gardener wants rather than a hidden -7. - w = doJSON(t, r, http.MethodPost, objectPath(oid)+"/plantings", map[string]any{ + w = doJSON(t, r, http.MethodPost, objectPlantingsPath(oid), map[string]any{ "plantId": plantID, "xCm": 40, "yCm": 40, "radiusCm": 20, "count": 45, "seedLotId": lotID, }, cookie) if w.Code != http.StatusCreated {