tests and mutation docs

pkg/cart/MUTATIONS.md

@@ -0,0 +1,89 @@
+# `pkg/cart` — mutation inventory
+
+Every mutation file under `pkg/cart/mutation_*.go` plus the slice of cart
+state it touches. The grouping is **subjective** ("Subscription lifecycle",
+"Discount surface", etc.) — used as a starting point for collapsing the
+file-per-mutation pattern flagged in `agents.md`. Move two related
+mutations into one file first; if the test surface stays smaller than the
+two-file version, the grouping is real.
+
+| File                                                | Concern              | State it READS                | State it WRITES                          |
+| --------------------------------------------------- | -------------------- | ----------------------------- | ---------------------------------------- |
+| `mutation_items.go` (`mutation_add_item.go` and `mutation_remove_item.go` merged; tests stay in `mutation_add_item_test.go` + `mutation_remove_item_test.go`) | Line item add/remove  | `state.Items`                   | `state.Items`, `state.UpdatedAt`         |
+| `mutation_change_quantity.go`                       | Line item quantity   | `state.Items[idx]`             | `state.Items[idx]`, `state.UpdatedAt`    |
+| `mutation_clear_cart.go`                            | Wholesale clear      | —                             | `state.Items`, `state.Vouchers`, `state.UpdatedAt` |
+| `mutation_set_user_id.go`                           | Ownership            | —                             | `state.UserID`, `state.UpdatedAt`        |
+| `mutation_set_cart_type.go` (+ test)                | Cart type            | —                             | `state.Type`, `state.UpdatedAt`          |
+| `mutation_add_voucher.go`                           | Discount surface     | `state.Vouchers`              | `state.Vouchers`, `state.LineTotals` (re-eval), `state.UpdatedAt` |
+| `mutation_set_custom_fields.go` (+ tests)           | Custom K/V           | `state.CustomFields`          | `state.CustomFields`, `state.UpdatedAt`  |
+| `mutation_set_recovery_contact.go` (+ tests)        | Recovery             | —                             | `state.Recovery`, `state.UpdatedAt`      |
+| `mutation_subscription_added.go`                    | Subscription         | `state.Items`                 | `state.Subscriptions`, `state.UpdatedAt` |
+| `mutation_upsert_subscriptiondetails.go`            | Subscription details | `state.Subscriptions`         | `state.Subscriptions`, `state.UpdatedAt` |
+| `mutation_remove_line_item_marking.go`              | Markings             | `state.Items[idx].Markings`   | `state.Items[idx].Markings`, `state.UpdatedAt` |
+| `mutation_line_item_marking.go`                     | Markings apply       | `state.Items[idx].Markings`   | `state.Items[idx].Markings`, `state.UpdatedAt` |
+
+## Delete-test quick check
+
+For any proposed grouping (e.g. merge all `marking*` mutations into one
+file), ask: does deleting the merged file concentrate complexity, or just
+move lines? For markings — yes, the shared logic (cascade to pricing) makes
+the merge worthwhile. For `voucher` vs `custom_fields` — no, those read
+different state and have different error contracts; keep separate.
+
+### What is already merged
+
+`pkg/cart/mutation_items.go` (added 2025-07) — AddItem and RemoveItem
+both touch `state.Items` and share the `ErrPaymentInProgress`,
+`decodeExtra`, and `getOrgPrice` helpers. Merge passed the delete-test
+checks: the receiver `*CartMutationContext`, same grain (`*CartGrain`),
+same mutation-registry dispatch. Both `mutation_*_test.go` files remain
+because they assert specific method behaviour, not generic package
+behaviour. Verified: `go test -count=1 ./pkg/cart/...` clean (voucher test
+which references `ErrPaymentInProgress` still green).
+
+#### Table corrections when merging
+
+When the two source rows collapsed into one, two corrections landed in
+the merge:
+
+  - AddItem's old READS column listed `state.Vouchers (for stacking)`.
+    That clause was **incorrect** — voucher stacking is `AddVoucher`'s
+    concern, not AddItem's. The merge drops the clause because reading
+    `mutation_add_item.go` confirms AddItem only reads `state.Items`.
+    This is a documentation correction, not a behaviour change.
+  - Both `State it READS / WRITES` columns originally had `(both)`
+    annotations to remind that two distinct mutations were sharing the
+    row. After collapse the row already says `Line item add/remove`, so
+    the `(both)` markers are noise — dropped.
+
+If a future merge lands, follow the same pattern: read the actual source
+to verify the row's claimed state surface, and prune label-noise before
+compressing two rows into one.
+
+### NOT to merge next
+
+`mutation_change_quantity.go` looks like an obvious candidate for the next
+merge (same receiver, same grain, same `state.Items` write surface). It
+is **not** — quantity has a partial-line-drop arithmetic contract that
+add/remove do not:
+
+  - `AddItem` always merges or appends; never sets quantity down.
+  - `RemoveItem` always removes whole lines; never decrements quantity.
+  - `ChangeQuantity` may drop a line entirely if quantity reaches 0.
+
+If you merge `ChangeQuantity` first, doing the same delete-test check
+yields: deleting the merged file leaves the partial-line arithmetic
+logic stranded in `Apply`, which IS a real concentration (good). But
+the inverse: if `AddItem` later grows a "decrement" edge case, the
+merged file will balloon and a re-split becomes expensive. Either
+keep `ChangeQuantity` separate, OR merge all three with the merge
+scoped to a "line item lifecycle" package and the partial-line
+arithmetic under a clearly-named helper.
+
+## How this maps onto a refactor
+
+1. Pick a row with shared logic (markings, subscription details).
+2. Move related mutations + their tests into one file under
+   `pkg/cart/<concern>.go`.
+3. Re-run `go test ./pkg/cart/...` — same assertions, smaller surface.
+4. Update this table to reflect the new file layout.

pkg/cart/mutation_items.go

@@ -1,3 +1,27 @@
+// mutation_items.go
+//
+// Single concern file for the cart line-item lifecycle: AddItem +
+// RemoveItem (and the helpers they share). Pulled out of the per-mutation
+// file layout as a proof-of-grouping for the pattern in
+// pkg/cart/MUTATIONS.md. AddItem and RemoveItem are dispatched through the
+// same MutationRegistry (cart-mutation-helper.go) and operate on the same
+// *CartGrain, so keeping them adjacent lets future reviewers compare
+// "what does add do when an item already exists" against "what does
+// remove do when a parent is removed" without bouncing between files.
+//
+// What lives here:
+//
+//   - AddItem: validation, merge-into-existing by ItemId, reservation
+//     handling, totals.
+//   - RemoveItem: by-line-id removal with transitive parent→child
+//     cascade, reservation release, totals.
+//   - Shared package helpers: decodeExtra (dynamic product extra JSON),
+//     getOrgPrice (origin price helper), ErrPaymentInProgress (used by
+//     mutation_add_voucher.go to refuse stacking during checkout).
+//
+// Out of scope here: change_quantity, set_custom_fields, line markings,
+// subscription details, etc. Those are separate concerns and stay in
+// their own files until they grow enough behaviour to invite merging.
 package cart
 
 import (
@@ -12,6 +36,11 @@ import (
 	"google.golang.org/protobuf/types/known/timestamppb"
 )
 
+// ErrPaymentInProgress is returned by mutations that must not proceed
+// while the cart is mid-payment (currently referenced by
+// mutation_add_voucher.go to short-circuit adding a voucher mid-checkout).
+var ErrPaymentInProgress = errors.New("payment in progress")
+
 // decodeExtra unpacks the dynamic product data carried as raw JSON. Invalid
 // payloads are logged and dropped rather than failing the mutation.
 func decodeExtra(b []byte) map[string]json.RawMessage {
@@ -26,12 +55,19 @@ func decodeExtra(b []byte) map[string]json.RawMessage {
 	return m
 }
 
-// mutation_add_item.go
+func getOrgPrice(orgPrice int64, rateBp int) *Price {
+	if orgPrice <= 0 {
+		return nil
+	}
+	return NewPriceFromIncVat(orgPrice, rateBp)
+}
+
+// AddItem (formerly mutation_add_item.go).
 //
 // Registers the AddItem cart mutation in the generic mutation registry.
 // This replaces the legacy switch-based logic previously found in CartGrain.Apply.
 //
-// Behavior:
+// Behaviour:
 //   - Validates quantity > 0
 //   - If an item with the same item id (ItemId) exists -> increases quantity
 //   - Else creates a new CartItem with computed tax amounts
@@ -40,10 +76,8 @@ func decodeExtra(b []byte) map[string]json.RawMessage {
 // Item identity is the catalog item id (ItemId), not the SKU: the product
 // service is looked up by id and the returned SKU is reference-only.
 //
-// NOTE: Any future field additions in messages.AddItem that affect pricing / tax
-// must keep this handler in sync.
-var ErrPaymentInProgress = errors.New("payment in progress")
-
+// NOTE: Any future field additions in messages.AddItem that affect pricing /
+// tax must keep this handler in sync.
 func (c *CartMutationContext) AddItem(g *CartGrain, m *cart_messages.AddItem) error {
 	ctx := context.Background()
 	if m == nil {
@@ -169,9 +203,70 @@ func (c *CartMutationContext) AddItem(g *CartGrain, m *cart_messages.AddItem) er
 	return nil
 }
 
-func getOrgPrice(orgPrice int64, rateBp int) *Price {
-	if orgPrice <= 0 {
-		return nil
+// RemoveItem (formerly mutation_remove_item.go).
+//
+// Registers the RemoveItem mutation.
+//
+// Behaviour:
+//   - Removes the cart line whose local cart line Id == payload.Id
+//   - Cascades: also removes any line whose ParentId points at a removed line
+//     (transitively), so removing a parent removes its child sub-articles
+//   - If no such line exists returns an error
+//   - Releases reservations for every removed line and recalculates totals
+//
+// Notes:
+//   - This removes only the line items; any deliveries referencing a removed
+//     item are NOT automatically adjusted (mirrors prior logic). If future
+//     semantics require pruning delivery.item_ids, extend this handler.
+//   - If multiple lines somehow shared the same Id (should not happen), all
+//     matches are removed — data integrity relies on unique line Ids.
+func (c *CartMutationContext) RemoveItem(g *CartGrain, m *cart_messages.RemoveItem) error {
+	if m == nil {
+		return fmt.Errorf("RemoveItem: nil payload")
 	}
-	return NewPriceFromIncVat(orgPrice, rateBp)
+
+	targetID := uint32(m.Id)
+
+	found := false
+	for _, it := range g.Items {
+		if it.Id == targetID {
+			found = true
+			break
+		}
+	}
+	if !found {
+		return fmt.Errorf("RemoveItem: item id %d not found", m.Id)
+	}
+
+	// Collect the target and, transitively, any children pointing at a removed
+	// line. Loops until no further descendants are found (handles nesting).
+	remove := map[uint32]bool{targetID: true}
+	for {
+		grew := false
+		for _, it := range g.Items {
+			if it.ParentId != nil && remove[*it.ParentId] && !remove[it.Id] {
+				remove[it.Id] = true
+				grew = true
+			}
+		}
+		if !grew {
+			break
+		}
+	}
+
+	kept := g.Items[:0]
+	for _, it := range g.Items {
+		if remove[it.Id] {
+			if it.ReservationEndTime != nil && it.ReservationEndTime.After(time.Now()) {
+				if err := c.ReleaseItem(context.Background(), g.Id, it.Sku, it.StoreId); err != nil {
+					log.Printf("unable to release reservation for item %d: %v", it.Id, err)
+				}
+			}
+			continue
+		}
+		kept = append(kept, it)
+	}
+	g.Items = kept
+	g.UpdateTotals()
+	return nil
 }

pkg/cart/mutation_remove_item.go

@@ -1,79 +0,0 @@
-package cart
-
-import (
-	"context"
-	"fmt"
-	"log"
-	"time"
-
-	messages "git.k6n.net/mats/go-cart-actor/proto/cart"
-)
-
-// mutation_remove_item.go
-//
-// Registers the RemoveItem mutation.
-//
-// Behavior:
-//   - Removes the cart line whose local cart line Id == payload.Id
-//   - Cascades: also removes any line whose ParentId points at a removed line
-//     (transitively), so removing a parent removes its child sub-articles
-//   - If no such line exists returns an error
-//   - Releases reservations for every removed line and recalculates totals
-//
-// Notes:
-//   - This removes only the line items; any deliveries referencing a removed
-//     item are NOT automatically adjusted (mirrors prior logic). If future
-//     semantics require pruning delivery.item_ids you can extend this handler.
-//   - If multiple lines somehow shared the same Id (should not happen), all
-//     matches are removed—data integrity relies on unique line Ids.
-
-func (c *CartMutationContext) RemoveItem(g *CartGrain, m *messages.RemoveItem) error {
-	if m == nil {
-		return fmt.Errorf("RemoveItem: nil payload")
-	}
-
-	targetID := uint32(m.Id)
-
-	found := false
-	for _, it := range g.Items {
-		if it.Id == targetID {
-			found = true
-			break
-		}
-	}
-	if !found {
-		return fmt.Errorf("RemoveItem: item id %d not found", m.Id)
-	}
-
-	// Collect the target and, transitively, any children pointing at a removed
-	// line. Loops until no further descendants are found (handles nesting).
-	remove := map[uint32]bool{targetID: true}
-	for {
-		grew := false
-		for _, it := range g.Items {
-			if it.ParentId != nil && remove[*it.ParentId] && !remove[it.Id] {
-				remove[it.Id] = true
-				grew = true
-			}
-		}
-		if !grew {
-			break
-		}
-	}
-
-	kept := g.Items[:0]
-	for _, it := range g.Items {
-		if remove[it.Id] {
-			if it.ReservationEndTime != nil && it.ReservationEndTime.After(time.Now()) {
-				if err := c.ReleaseItem(context.Background(), g.Id, it.Sku, it.StoreId); err != nil {
-					log.Printf("unable to release reservation for item %d: %v", it.Id, err)
-				}
-			}
-			continue
-		}
-		kept = append(kept, it)
-	}
-	g.Items = kept
-	g.UpdateTotals()
-	return nil
-}