Three real bugs, two of which would have quietly corrupted inventory numbers. CopyGarden carried seed_lot_id into the copied plantings, so copying a garden double-counted the original lot's usage — the copy is a plan, not a second planting out of the same packet. It also quietly attached a private lot to a garden that may later be shared, contradicting the "lots are never shared with a garden" invariant this feature was built on. The copy now drops the link. RestorePlanting restored a seed_lot_id verbatim from a history snapshot. Live rows get their link nulled by ON DELETE SET NULL when a lot is deleted, but the snapshot still holds the old id, so undoing a planting deletion after retiring its lot would violate the foreign key and abort the whole revert. The revert now drops a dangling link before restoring: the plant really was in the ground, which is the part worth restoring. CreateSeedLot returned a lot with Used/Remaining at their zero values, so a packet you just bought reported "0 remaining" — the exact opposite of the truth. The version-conflict path had the same gap, and that row is what the client rebases on, so a 409 reporting remaining=0 is worse than no number at all. Both fill them now. Remaining can go negative and that is deliberate — it means more was planted than the lot recorded buying, which happens (a miscounted packet, a lot entered after the fact). Clamping to zero would hide the discrepancy at exactly the moment it is worth seeing. Now documented and tested rather than incidental. Performance and tidiness: SeedLotUsage is scoped to the lots being filled instead of scanning every planting the owner has, so reading one lot is one lot's work; ListSeedLotsForOwner gained the LIMIT every other list read has; parseNullable moved out of seed_lots.go into the shared request helpers, since every nullable field in the API needs that three-way absent/null/value distinction; finalizePlant validates against its own maxPlantVendorLen rather than borrowing a constant named for seed lots; seedLotForPlanting became checkSeedLotForPlanting, since it validates rather than fetches; and the duplicate intPtr test helper gave way to the existing ptrInt. Declined: recording seed-lot mutations in the revision history. change_sets are anchored to a garden and lots are user-scoped, deliberately — they belong to you, not to any one garden, so there is no garden to record them against. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
This commit is contained in:
@@ -417,6 +417,9 @@ type revertOps[T any] struct {
|
||||
// remove undoes a creation. It returns the changes it made, because deleting
|
||||
// an object also deletes the plantings hanging off it.
|
||||
remove func(ctx context.Context, row *T) ([]change, error)
|
||||
// beforeRestore fixes up a snapshot whose references may have gone stale
|
||||
// while it sat in history.
|
||||
beforeRestore func(ctx context.Context, row *T) error
|
||||
// blockRemoval optionally refuses a removal, e.g. an object that has gained
|
||||
// plantings since. Returns a conflict reason, or "" to allow it.
|
||||
blockRemoval func(ctx context.Context, row *T) (string, error)
|
||||
@@ -445,6 +448,11 @@ func revertEntity[T any](
|
||||
if err := unsnapshot(r.Before, &target); err != nil {
|
||||
return nil, nil, err
|
||||
}
|
||||
if ops.beforeRestore != nil {
|
||||
if err := ops.beforeRestore(ctx, &target); err != nil {
|
||||
return nil, nil, err
|
||||
}
|
||||
}
|
||||
restored, err := ops.restore(ctx, &target)
|
||||
if err != nil {
|
||||
// The id may have been taken between the check above and this insert.
|
||||
@@ -547,6 +555,22 @@ func plantingRevertOps(s *Service) revertOps[domain.Planting] {
|
||||
get: s.store.GetPlanting,
|
||||
update: s.store.UpdatePlanting,
|
||||
restore: s.store.RestorePlanting,
|
||||
// A snapshot can name a seed lot that has since been deleted. The live
|
||||
// rows got their link nulled by ON DELETE SET NULL, but the snapshot
|
||||
// still holds the old id, and restoring it would violate the foreign key
|
||||
// and abort the whole revert. Drop the dangling link instead: the plant
|
||||
// really was in the ground, which is the part worth restoring.
|
||||
beforeRestore: func(ctx context.Context, p *domain.Planting) error {
|
||||
if p.SeedLotID == nil {
|
||||
return nil
|
||||
}
|
||||
if _, err := s.store.GetSeedLot(ctx, *p.SeedLotID); errors.Is(err, domain.ErrNotFound) {
|
||||
p.SeedLotID = nil
|
||||
} else if err != nil {
|
||||
return err
|
||||
}
|
||||
return nil
|
||||
},
|
||||
remove: func(ctx context.Context, p *domain.Planting) ([]change, error) {
|
||||
if err := s.store.DeletePlanting(ctx, p.ID); err != nil {
|
||||
return nil, err
|
||||
|
||||
Reference in New Issue
Block a user