From 34ff4b99f5d8cf1698962165e0c81d00446bb8d5 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 01:47:14 -0400 Subject: [PATCH] Scope the season's plant lookup to the season ListReferencedPlants(includeRemoved=true) returned every plant the garden has ever held, so drawing one season loaded a decade of catalog on a decade-old garden. It also meant the plant list and the plop list were selected by different rules, which is the kind of near-miss that stays correct only by accident. It now takes the same year the plops do, so the two selections always agree. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- internal/service/objects.go | 2 +- internal/service/objects_test.go | 56 ++++++++++++++++++++++++++++++++ internal/store/plants.go | 24 +++++++++----- 3 files changed, 72 insertions(+), 10 deletions(-) diff --git a/internal/service/objects.go b/internal/service/objects.go index da81818..a1a26ad 100644 --- a/internal/service/objects.go +++ b/internal/service/objects.go @@ -271,7 +271,7 @@ func (s *Service) assembleFullFor(ctx context.Context, g *domain.Garden, year *i if err != nil { return nil, err } - plants, err := s.store.ListReferencedPlants(ctx, g.ID, year != nil) + plants, err := s.store.ListReferencedPlants(ctx, g.ID, year) if err != nil { return nil, err } diff --git a/internal/service/objects_test.go b/internal/service/objects_test.go index 9fb3d50..b6f7b41 100644 --- a/internal/service/objects_test.go +++ b/internal/service/objects_test.go @@ -432,3 +432,59 @@ func TestSeasonYearIsBounded(t *testing.T) { } } } + +// TestSeasonPlantsAreScopedToTheYear — the plant list has to match the plops it +// is there to render. A garden with a decade of history shouldn't load a decade +// of catalog to draw one season, and a season shouldn't carry plants that had +// nothing in the ground that year. +func TestSeasonPlantsAreScopedToTheYear(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) + ctx := context.Background() + + oldPlant := seedOwnPlant(t, s, owner, 15) + newPlant, err := s.CreatePlant(ctx, owner, PlantInput{ + Name: "Later", Category: domain.CategoryHerb, SpacingCM: 20, Color: "#4a7c3f", Icon: "🌱", + }) + if err != nil { + t.Fatalf("CreatePlant: %v", err) + } + + plant2020, err := s.CreatePlanting(ctx, owner, bed.ID, PlantingInput{ + PlantID: oldPlant.ID, XCM: -40, YCM: 0, RadiusCM: 20, PlantedAt: strPtr("2020-04-01"), + }) + if err != nil { + t.Fatalf("CreatePlanting: %v", err) + } + if _, err := s.UpdatePlanting(ctx, owner, plant2020.ID, PlantingPatch{ + SetRemovedAt: true, RemovedAt: strPtr("2020-09-01"), + }, plant2020.Version); err != nil { + t.Fatalf("remove: %v", err) + } + if _, err := s.CreatePlanting(ctx, owner, bed.ID, PlantingInput{ + PlantID: newPlant.ID, XCM: 40, YCM: 0, RadiusCM: 20, PlantedAt: strPtr("2026-04-01"), + }); err != nil { + t.Fatalf("CreatePlanting: %v", err) + } + + names := func(year int) []string { + t.Helper() + full, err := s.GardenFull(ctx, owner, g.ID, &year) + if err != nil { + t.Fatalf("GardenFull(%d): %v", year, err) + } + out := []string{} + for _, p := range full.Plants { + out = append(out, p.Name) + } + return out + } + if got := names(2020); len(got) != 1 || got[0] != oldPlant.Name { + t.Errorf("2020 plants = %v, want just %q", got, oldPlant.Name) + } + if got := names(2026); len(got) != 1 || got[0] != "Later" { + t.Errorf("2026 plants = %v, want just \"Later\"", got) + } +} diff --git a/internal/store/plants.go b/internal/store/plants.go index 52e27ab..149ffb5 100644 --- a/internal/store/plants.go +++ b/internal/store/plants.go @@ -27,21 +27,27 @@ func scanPlant(s scanner) (*domain.Plant, error) { // ListReferencedPlants returns the distinct plants used by a garden's plantings // — the catalog subset the editor needs to render them. Always a non-nil slice. // -// includeRemoved widens it past the active plops to every plop the garden has -// ever held, which is what a past season needs: a plant pulled last July is not -// active, but its plops still have to render with the right icon and color. -func (d *DB) ListReferencedPlants(ctx context.Context, gardenID int64, includeRemoved bool) ([]domain.Plant, error) { - activeOnly := ` AND pl.removed_at IS NULL` - if includeRemoved { - activeOnly = `` +// year nil means the active plops only. A year widens it to the plops that were +// in the ground THAT YEAR, which is what a past season needs: a plant pulled +// last July is not active, but its plops still have to render with the right +// icon and colour. It is scoped to the same year as the plops rather than to the +// garden's whole history, so the two selections always agree and a decade-old +// garden doesn't load a decade of catalog to draw one season. +func (d *DB) ListReferencedPlants(ctx context.Context, gardenID int64, year *int) ([]domain.Plant, error) { + where := ` AND pl.removed_at IS NULL` + args := []any{gardenID} + if year != nil { + where = ` AND (pl.planted_at IS NULL OR pl.planted_at <= ?) + AND (pl.removed_at IS NULL OR pl.removed_at >= ?)` + args = append(args, fmt.Sprintf("%04d-12-31", *year), fmt.Sprintf("%04d-01-01", *year)) } rows, err := d.sql.QueryContext(ctx, `SELECT DISTINCT `+qualifyColumns("p", plantColumns)+` FROM plants p JOIN plantings pl ON pl.plant_id = p.id JOIN garden_objects o ON o.id = pl.object_id - WHERE o.garden_id = ?`+activeOnly+` + WHERE o.garden_id = ?`+where+` ORDER BY p.id`, - gardenID, + args..., ) if err != nil { return nil, fmt.Errorf("store: list referenced plants: %w", err)