diff --git a/internal/api/api.go b/internal/api/api.go index c94a746..3e3c6f8 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -121,6 +121,12 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine { changeSets := v1.Group("/change-sets", h.requireAuth()) changeSets.POST("/:id/revert", h.revertChangeSet) + // Journal entries are addressed by their own id; the service resolves the + // owning garden for the permission check, same as objects and plantings. + journal := v1.Group("/journal", h.requireAuth()) + journal.PATCH("/:id", h.updateJournalEntry) + journal.DELETE("/:id", h.deleteJournalEntry) + // Plant catalog: built-ins (seeded, read-only) plus the actor's own rows. plants := v1.Group("/plants", h.requireAuth()) plants.GET("", h.listPlants) diff --git a/internal/api/journal_test.go b/internal/api/journal_test.go new file mode 100644 index 0000000..837b6f6 --- /dev/null +++ b/internal/api/journal_test.go @@ -0,0 +1,157 @@ +package api + +import ( + "net/http" + "strconv" + "testing" +) + +func journalPath(gardenID int64) string { + return "/api/v1/gardens/" + strconv.FormatInt(gardenID, 10) + "/journal" +} + +func journalEntryPath(id int64) string { + return "/api/v1/journal/" + strconv.FormatInt(id, 10) +} + +// TestJournalCrudAPI walks the whole entry lifecycle over HTTP. +// +// It exists because the service-level tests could not see that PATCH and DELETE +// were never registered on the router: the handlers were written, tested through +// the service, and completely unreachable. Anything addressed by its own id +// needs at least one test that goes through the router. +func TestJournalCrudAPI(t *testing.T) { + r := authEngine(t, localCfg()) + cookie := registerAndCookie(t, r, "journal@example.com") + gid := createGardenAPI(t, r, cookie, "G") + + w := doJSON(t, r, http.MethodPost, journalPath(gid), map[string]any{ + "body": "First frost tonight", "observedAt": "2026-10-12", + }, cookie) + if w.Code != http.StatusCreated { + t.Fatalf("create: status %d, body %s", w.Code, w.Body.String()) + } + entry := decodeMap(t, w.Body.Bytes()) + id := int64(entry["id"].(float64)) + if entry["observedAt"] != "2026-10-12" || entry["body"] != "First frost tonight" { + t.Errorf("unexpected entry: %+v", entry) + } + + // List it back. + w = doJSON(t, r, http.MethodGet, journalPath(gid), nil, cookie) + if w.Code != http.StatusOK { + t.Fatalf("list: status %d, body %s", w.Code, w.Body.String()) + } + body := decodeMap(t, w.Body.Bytes()) + entries, _ := body["entries"].([]any) + if len(entries) != 1 { + t.Fatalf("got %d entries, want 1: %s", len(entries), w.Body.String()) + } + if first := entries[0].(map[string]any); first["authorName"] == "" { + t.Error("author name missing from the list read") + } + + // PATCH — the route this test exists for. + w = doJSON(t, r, http.MethodPatch, journalEntryPath(id), map[string]any{ + "body": "First frost, tomatoes done", "version": entry["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["body"] != "First frost, tomatoes done" { + t.Errorf("body = %v", updated["body"]) + } + + // A stale version conflicts, carrying the current row. + w = doJSON(t, r, http.MethodPatch, journalEntryPath(id), map[string]any{ + "body": "stale", "version": entry["version"], + }, cookie) + if w.Code != http.StatusConflict { + t.Errorf("stale patch: status %d, want 409", w.Code) + } + if current, _ := decodeMap(t, w.Body.Bytes())["current"].(map[string]any); current == nil { + t.Error("409 should carry the current row") + } + + // DELETE — likewise. + w = doJSON(t, r, http.MethodDelete, journalEntryPath(id), nil, cookie) + if w.Code != http.StatusNoContent { + t.Fatalf("delete: status %d, body %s", w.Code, w.Body.String()) + } + w = doJSON(t, r, http.MethodGet, journalPath(gid), nil, cookie) + if entries, _ := decodeMap(t, w.Body.Bytes())["entries"].([]any); len(entries) != 0 { + t.Errorf("%d entries survived the delete", len(entries)) + } +} + +// TestJournalFiltersAPI covers the query parameters, including a malformed one. +func TestJournalFiltersAPI(t *testing.T) { + r := authEngine(t, localCfg()) + cookie := registerAndCookie(t, r, "filters@example.com") + gid := createGardenAPI(t, r, cookie, "G") + + w := doJSON(t, r, http.MethodPost, objectsPath(gid), map[string]any{ + "kind": "bed", "name": "North Bed", "xCm": 100, "yCm": 100, "widthCm": 100, "heightCm": 100, + }, cookie) + objectID := int64(decodeMap(t, w.Body.Bytes())["id"].(float64)) + + doJSON(t, r, http.MethodPost, journalPath(gid), map[string]any{ + "body": "garden level", "observedAt": "2026-05-01", + }, cookie) + doJSON(t, r, http.MethodPost, journalPath(gid), map[string]any{ + "objectId": objectID, "body": "bed level", "observedAt": "2026-06-01", + }, cookie) + + countAt := func(query string) int { + t.Helper() + w := doJSON(t, r, http.MethodGet, journalPath(gid)+query, nil, cookie) + if w.Code != http.StatusOK { + t.Fatalf("list%s: status %d, body %s", query, w.Code, w.Body.String()) + } + entries, _ := decodeMap(t, w.Body.Bytes())["entries"].([]any) + return len(entries) + } + if n := countAt(""); n != 2 { + t.Errorf("unfiltered = %d, want 2", n) + } + if n := countAt("?objectId=" + strconv.FormatInt(objectID, 10)); n != 1 { + t.Errorf("by object = %d, want 1", n) + } + if n := countAt("?from=2026-05-15&to=2026-12-31"); n != 1 { + t.Errorf("by date range = %d, want 1", n) + } + + // A malformed filter is an error, not a silently ignored one that widens the + // query back to the whole garden. + w = doJSON(t, r, http.MethodGet, journalPath(gid)+"?objectId=abc", nil, cookie) + if w.Code != http.StatusBadRequest { + t.Errorf("malformed objectId: status %d, want 400", w.Code) + } + w = doJSON(t, r, http.MethodGet, journalPath(gid)+"?from=nonsense", nil, cookie) + if w.Code != http.StatusBadRequest { + t.Errorf("malformed from: status %d, want 400", w.Code) + } +} + +// TestJournalRequiresAccessAPI — anonymous is 401, a stranger is 404. +func TestJournalRequiresAccessAPI(t *testing.T) { + r := authEngine(t, localCfg()) + owner := registerAndCookie(t, r, "jowner@example.com") + gid := createGardenAPI(t, r, owner, "G") + stranger := registerAndCookie(t, r, "jstranger@example.com") + + if w := doJSON(t, r, http.MethodGet, journalPath(gid), nil, nil); w.Code != http.StatusUnauthorized { + t.Errorf("anonymous list: status %d, want 401", w.Code) + } + if w := doJSON(t, r, http.MethodGet, journalPath(gid), nil, stranger); w.Code != http.StatusNotFound { + t.Errorf("stranger list: status %d, want 404", w.Code) + } + w := doJSON(t, r, http.MethodPost, journalPath(gid), map[string]any{"body": "nope"}, stranger) + if w.Code != http.StatusNotFound { + t.Errorf("stranger write: status %d, want 404", w.Code) + } + if w := doJSON(t, r, http.MethodDelete, journalEntryPath(1), nil, stranger); w.Code != http.StatusNotFound { + t.Errorf("stranger delete: status %d, want 404", w.Code) + } +} diff --git a/internal/service/journal.go b/internal/service/journal.go index c0bf9b5..99085b3 100644 --- a/internal/service/journal.go +++ b/internal/service/journal.go @@ -2,6 +2,7 @@ package service import ( "context" + "errors" "strings" "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" @@ -93,13 +94,14 @@ func (s *Service) CreateJournalEntry(ctx context.Context, actorID, gardenID int6 if _, err := s.requireGardenRole(ctx, actorID, gardenID, roleEditor); err != nil { return nil, err } - if err := s.checkJournalTarget(ctx, gardenID, in.ObjectID, in.PlantingID); err != nil { + objectID, err := s.resolveJournalTarget(ctx, gardenID, in.ObjectID, in.PlantingID) + if err != nil { return nil, err } e := &domain.JournalEntry{ GardenID: gardenID, - ObjectID: in.ObjectID, + ObjectID: objectID, PlantingID: in.PlantingID, AuthorID: actorID, Body: strings.TrimSpace(in.Body), @@ -115,31 +117,57 @@ func (s *Service) CreateJournalEntry(ctx context.Context, actorID, gardenID int6 return s.store.CreateJournalEntry(ctx, e) } -// checkJournalTarget verifies that an object/planting an entry points at really -// lives in this garden. -func (s *Service) checkJournalTarget(ctx context.Context, gardenID int64, objectID, plantingID *int64) error { +// resolveJournalTarget validates an entry's target and returns the object id to +// store against it. +// +// A plop-level entry gets its parent object filled in even when the caller +// didn't name one. Without that, "notes about this bed" would silently exclude +// every note written about a plant IN the bed — the bed filter and the plop +// filter would disagree about what an entry is about, which is exactly the +// question the reader is asking. +// +// A store failure that isn't "no such row" is passed through rather than +// flattened to ErrInvalidInput: a database problem is not the caller's bad +// request, and mapping it to a 400 would hide it from the logs entirely. +func (s *Service) resolveJournalTarget(ctx context.Context, gardenID int64, objectID, plantingID *int64) (*int64, error) { if objectID != nil { o, err := s.store.GetObject(ctx, *objectID) - if err != nil || o.GardenID != gardenID { - return domain.ErrInvalidInput + if errors.Is(err, domain.ErrNotFound) { + return nil, domain.ErrInvalidInput } - } - if plantingID != nil { - pl, err := s.store.GetPlanting(ctx, *plantingID) if err != nil { - return domain.ErrInvalidInput + return nil, err } - o, err := s.store.GetObject(ctx, pl.ObjectID) - if err != nil || o.GardenID != gardenID { - return domain.ErrInvalidInput - } - // A plop-level entry that also names an object must name the RIGHT one, - // or the two filters would disagree about which entries are about what. - if objectID != nil && *objectID != pl.ObjectID { - return domain.ErrInvalidInput + if o.GardenID != gardenID { + return nil, domain.ErrInvalidInput } } - return nil + if plantingID == nil { + return objectID, nil + } + + pl, err := s.store.GetPlanting(ctx, *plantingID) + if errors.Is(err, domain.ErrNotFound) { + return nil, domain.ErrInvalidInput + } + if err != nil { + return nil, err + } + o, err := s.store.GetObject(ctx, pl.ObjectID) + if errors.Is(err, domain.ErrNotFound) { + return nil, domain.ErrInvalidInput + } + if err != nil { + return nil, err + } + if o.GardenID != gardenID { + return nil, domain.ErrInvalidInput + } + // A plop-level entry that ALSO names an object must name the right one. + if objectID != nil && *objectID != pl.ObjectID { + return nil, domain.ErrInvalidInput + } + return &pl.ObjectID, nil } // journalEntryForWrite loads an entry and enforces who may change it: the author diff --git a/internal/service/journal_test.go b/internal/service/journal_test.go index 046d8bf..b464b01 100644 --- a/internal/service/journal_test.go +++ b/internal/service/journal_test.go @@ -292,3 +292,34 @@ func TestJournalStaysOutOfRevisionHistory(t *testing.T) { t.Errorf("journal writes produced %d change sets; they should produce none", got-before) } } + +// TestPlopEntryIsVisibleUnderItsBed — an entry about a plant IN a bed is an entry +// about that bed. Without deriving the parent object, "notes about this bed" +// would silently exclude every note written about something growing in it. +func TestPlopEntryIsVisibleUnderItsBed(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + bed := seedBed(t, s, owner, g.ID) + plant := seedOwnPlant(t, s, owner, 15) + ctx := context.Background() + + plop, err := s.CreatePlanting(ctx, owner, bed.ID, PlantingInput{ + PlantID: plant.ID, XCM: 0, YCM: 0, RadiusCM: 20, + }) + if err != nil { + t.Fatalf("CreatePlanting: %v", err) + } + + // Only the plop is named — no objectId. + e := writeEntry(t, s, owner, g.ID, JournalInput{PlantingID: &plop.ID, Body: "Scapes forming"}) + if e.ObjectID == nil || *e.ObjectID != bed.ID { + t.Fatalf("objectId = %v, want the plop's parent bed %d", e.ObjectID, bed.ID) + } + if byBed := listJournal(t, s, owner, g.ID, JournalQuery{ObjectID: &bed.ID}); len(byBed) != 1 { + t.Errorf("the bed filter found %d entries, want the plop's", len(byBed)) + } + if byPlop := listJournal(t, s, owner, g.ID, JournalQuery{PlantingID: &plop.ID}); len(byPlop) != 1 { + t.Errorf("the plop filter found %d entries, want 1", len(byPlop)) + } +} diff --git a/internal/store/journal.go b/internal/store/journal.go index 02b3e53..2ac598d 100644 --- a/internal/store/journal.go +++ b/internal/store/journal.go @@ -14,12 +14,19 @@ import ( const journalColumns = `id, garden_id, object_id, planting_id, author_id, body, observed_at, version, created_at, updated_at` -func scanJournalEntry(s scanner) (*domain.JournalEntry, error) { - var e domain.JournalEntry - if err := s.Scan( +// journalScanTargets returns the scan destinations for journalColumns, in order. +// The list read appends one more for the joined author name, so the two readers +// share a single definition of the column order rather than each repeating it. +func journalScanTargets(e *domain.JournalEntry) []any { + return []any{ &e.ID, &e.GardenID, &e.ObjectID, &e.PlantingID, &e.AuthorID, &e.Body, &e.ObservedAt, &e.Version, &e.CreatedAt, &e.UpdatedAt, - ); err != nil { + } +} + +func scanJournalEntry(s scanner) (*domain.JournalEntry, error) { + var e domain.JournalEntry + if err := s.Scan(journalScanTargets(&e)...); err != nil { return nil, err } return &e, nil @@ -105,10 +112,7 @@ func (d *DB) ListJournalEntries(ctx context.Context, gardenID int64, f JournalFi entries := []domain.JournalEntry{} for rows.Next() { var e domain.JournalEntry - if err := rows.Scan( - &e.ID, &e.GardenID, &e.ObjectID, &e.PlantingID, &e.AuthorID, &e.Body, - &e.ObservedAt, &e.Version, &e.CreatedAt, &e.UpdatedAt, &e.AuthorName, - ); err != nil { + if err := rows.Scan(append(journalScanTargets(&e), &e.AuthorName)...); err != nil { return nil, fmt.Errorf("store: scan journal entry: %w", err) } entries = append(entries, e)