From 8bb0a7a78620bfbcc055ad2d699d59441a82d07b Mon Sep 17 00:00:00 2001 From: matst80 Date: Fri, 3 Jul 2026 19:30:02 +0200 Subject: [PATCH] tests and mutation docs --- pkg/cart/MUTATIONS.md | 89 +++++++++++++ ...mutation_add_item.go => mutation_items.go} | 121 ++++++++++++++++-- pkg/cart/mutation_remove_item.go | 79 ------------ 3 files changed, 197 insertions(+), 92 deletions(-) create mode 100644 pkg/cart/MUTATIONS.md rename pkg/cart/{mutation_add_item.go => mutation_items.go} (56%) delete mode 100644 pkg/cart/mutation_remove_item.go diff --git a/pkg/cart/MUTATIONS.md b/pkg/cart/MUTATIONS.md new file mode 100644 index 0000000..fb4299a --- /dev/null +++ b/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/.go`. +3. Re-run `go test ./pkg/cart/...` — same assertions, smaller surface. +4. Update this table to reflect the new file layout. diff --git a/pkg/cart/mutation_add_item.go b/pkg/cart/mutation_items.go similarity index 56% rename from pkg/cart/mutation_add_item.go rename to pkg/cart/mutation_items.go index fab6326..462133b 100644 --- a/pkg/cart/mutation_add_item.go +++ b/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 { @@ -146,10 +180,10 @@ func (c *CartMutationContext) AddItem(g *CartGrain, m *cart_messages.AddItem) er StoreId: m.StoreId, - Extra: decodeExtra(m.ExtraJson), - CustomFields: m.CustomFields, + Extra: decodeExtra(m.ExtraJson), + CustomFields: m.CustomFields, InventoryTracked: m.InventoryTracked, - DropShip: m.DropShip, + DropShip: m.DropShip, } if g.Type == cart_messages.CartType_REGULAR && needsReservation && c.UseReservations(cartItem) { @@ -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 } diff --git a/pkg/cart/mutation_remove_item.go b/pkg/cart/mutation_remove_item.go deleted file mode 100644 index 2ee7ba0..0000000 --- a/pkg/cart/mutation_remove_item.go +++ /dev/null @@ -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 -}