amadan

rastrillo / abono Public

abono: reconcile recovers lost subscriptions on known prices; checkout learns price intervals

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Summary

R16: a sync with no hint now records a subscription that grants on a known price (sold or legacy), finding its holder by customer mapping, then Holder with the customer's address, so a reconcile recovers a webhook Stripe gave up on. Ended subscriptions, and subscriptions on an unknown price, still need a hint. R17: before choosing a price, Checkout reads from Stripe every sold price of the plan whose interval is unknown and records its interval, amount, currency and refreshed_at; R15 then applies.
Agent notes
R16 as worded conflicts with itself: its rule says unknown-price subscriptions are never created without a hint, but its test bullet says granting + unknown price + no hint gives an unrecognised row. Built to the rule, which its stated reason backs (keep the account's other sales out); the other reading is a one-condition change in syncTx. Task 6's TestWebhookUpdatedCannotCreate no longer holds for a granting known-price subscription unless the webhook skips sync for non-creating events. refreshPrice is there for Task 6's reconcile to reuse. A one-off price stays at interval '' and is re-read on every Checkout of its plan.
claude 1a767d543869b2bfa9b2cc73ce57faa507898d14 parent 0c3822f
5 files changed, +169 −53
  • checkout.go +34 −4
  • checkout_test.go +48 −21
  • stripe_double_test.go +34 −1
  • sync.go +13 −11
  • sync_test.go +40 −16

CI passed — run details

CI log 
=== vet ===
go vet ./...
=== fmt ===
=== test ===
go test ./... -count=1
ok  	amadan.net/rastrillo/abono	0.427s
ok  	amadan.net/rastrillo/abono/internal/stripeapi	0.005s
=== race ===
CGO_ENABLED=1 go test ./... -race -count=1
ok  	amadan.net/rastrillo/abono	3.522s
ok  	amadan.net/rastrillo/abono/internal/stripeapi	1.014s
=== browser ===
browser: no browser test file yet, nothing to run

Intent

full ledger: v0.1

diff --git a/checkout.go b/checkout.go
index d03a9bc..74c9b90 100644
--- a/checkout.go
+++ b/checkout.go
@@ -48,6 +48,9 @@ func (s *Service) Checkout(ctx context.Context, req CheckoutRequest) (string, er
return "", errOff
}
g := s.cfg.DB.WithContext(ctx)
+ if err := s.learnIntervals(ctx, req.Plan); err != nil {
+ return "", err
+ }
price, err := soldPrice(g, req.Plan, req.Interval)
if err != nil {
return "", err
@@ -104,11 +107,38 @@ func (s *Service) Checkout(ctx context.Context, req CheckoutRequest) (string, er
return cs.URL, nil
}
+// learnIntervals reads from Stripe every sold price of plan whose interval
+// is not known yet (a price seeded from the environment, or added by id),
+// and records its interval, amount and currency, so the price can be chosen
+// by interval (ruling R17). A price Stripe cannot read stops the sale.
+func (s *Service) learnIntervals(ctx context.Context, plan string) error {
+ var ids []string
+ if err := s.cfg.DB.WithContext(ctx).Raw(`SELECT stripe_price_id FROM abono_prices
+ WHERE plan = ? AND sold = 1 AND interval = '' ORDER BY stripe_price_id`, plan).Scan(&ids).Error; err != nil {
+ return err
+ }
+ for _, id := range ids {
+ if err := s.refreshPrice(ctx, id); err != nil {
+ return err
+ }
+ }
+ return nil
+}
+
+// refreshPrice records what Stripe says of price id: its interval
+// ("" for a one-off price), amount and currency.
+func (s *Service) refreshPrice(ctx context.Context, id string) error {
+ p, err := s.stripe.GetPrice(ctx, id)
+ if err != nil {
+ return fmt.Errorf("abono: reading price %q: %w", id, err)
+ }
+ return s.cfg.DB.WithContext(ctx).Exec(`UPDATE abono_prices
+ SET interval = ?, amount = ?, currency = ?, refreshed_at = ? WHERE stripe_price_id = ?`,
+ p.Recurring.Interval, p.UnitAmount, p.Currency, s.now(), id).Error
+}
+
// soldPrice is the price to sell for plan at interval: the sold price with
-// that interval, or with no interval the plan's only sold price. A price's
-// interval is "" until a reconcile has read it from Stripe, so a plan
-// seeded from the environment sells with no interval asked for, and not
-// with one.
+// that interval, or with no interval the plan's only sold price.
func soldPrice(g *gorm.DB, plan, interval string) (string, error) {
var prices []priceRow
if err := g.Raw(`SELECT p.stripe_price_id, p.interval FROM abono_prices p
diff --git a/checkout_test.go b/checkout_test.go
index e91dd30..6de18f1 100644
--- a/checkout_test.go
+++ b/checkout_test.go
@@ -4,6 +4,7 @@ import (
"context"
"errors"
"net/url"
+ "reflect"
"testing"
"amadan.net/rastrillo/abono"
@@ -88,8 +89,10 @@ func TestCheckoutForm(t *testing.T) {
}
}
-// TestCheckoutChoosesThePriceByInterval is ruling R15: an interval picks
-// the sold price with it; no interval needs exactly one sold price.
+// TestCheckoutChoosesThePriceByInterval is rulings R15 and R17: Checkout
+// first learns the interval of any sold price it does not know yet, then
+// an interval picks the sold price with it, and no interval needs exactly
+// one sold price.
func TestCheckoutChoosesThePriceByInterval(t *testing.T) {
now := t0
d := newStripeDouble(t)
@@ -100,32 +103,40 @@ func TestCheckoutChoosesThePriceByInterval(t *testing.T) {
return abono.CheckoutRequest{Plan: plan, Interval: interval, SuccessPath: "/done", CancelPath: "/"}
}
- // Before a refresh, intervals are unknown: one sold price is sold
- // with no interval asked for, and not when one is.
- if _, err := s.Checkout(ctx, req("annual", "")); err != nil {
- t.Fatalf("one sold price, unknown interval: %v", err)
+ // The plan sold yearly and monthly, neither interval known yet.
+ d.price("price_A_month", "month", 600, "eur")
+ if err := s.AddPrice(ctx, "annual", "price_A_month", true); err != nil {
+ t.Fatalf("AddPrice: %v", err)
+ }
+ if got := db.dump(`SELECT stripe_price_id, interval FROM abono_prices WHERE plan = 'annual' AND sold = 1 ORDER BY stripe_price_id`); got != "price_A|\nprice_A_month|\n" {
+ t.Fatalf("sold prices before Checkout = %q, want both intervals unknown", got)
+ }
+
+ if _, err := s.Checkout(ctx, req("annual", "year")); err != nil {
+ t.Fatalf("interval year: %v", err)
}
if got := d.lastCheckout(t).Get("line_items[0][price]"); got != "price_A" {
- t.Errorf("price = %q, want price_A", got)
+ t.Errorf("year: price = %q, want price_A", got)
}
- if _, err := s.Checkout(ctx, req("annual", "year")); !errors.Is(err, abono.ErrUnknownPlan) {
- t.Errorf("an interval no price is known to have: err = %v, want ErrUnknownPlan", err)
+ if got := d.fetches(); !reflect.DeepEqual(got, map[string]int{"price_A": 1, "price_A_month": 1}) {
+ t.Errorf("price fetches = %v, want one each of the sold prices", got)
+ }
+ if got := db.dump(`SELECT stripe_price_id, interval, amount, currency, refreshed_at IS NOT NULL FROM abono_prices ORDER BY stripe_price_id`); got !=
+ "price_A|year|6000|eur|1\nprice_A_month|month|600|eur|1\nprice_M||0||0\nprice_old||0||0\n" {
+ t.Errorf("prices after Checkout =\n%s", got)
}
- // The plan sold yearly and monthly.
- if err := s.AddPrice(ctx, "annual", "price_A_month", true); err != nil {
- t.Fatalf("AddPrice: %v", err)
+ // Known now: no more reads.
+ if _, err := s.Checkout(ctx, req("annual", "month")); err != nil {
+ t.Fatalf("interval month: %v", err)
}
- db.exec(`UPDATE abono_prices SET interval = 'year' WHERE stripe_price_id = 'price_A'`)
- db.exec(`UPDATE abono_prices SET interval = 'month' WHERE stripe_price_id = 'price_A_month'`)
- for interval, want := range map[string]string{"year": "price_A", "month": "price_A_month"} {
- if _, err := s.Checkout(ctx, req("annual", interval)); err != nil {
- t.Fatalf("interval %s: %v", interval, err)
- }
- if got := d.lastCheckout(t).Get("line_items[0][price]"); got != want {
- t.Errorf("interval %s: price = %q, want %q", interval, got, want)
- }
+ if got := d.lastCheckout(t).Get("line_items[0][price]"); got != "price_A_month" {
+ t.Errorf("month: price = %q, want price_A_month", got)
}
+ if got := d.fetches(); !reflect.DeepEqual(got, map[string]int{"price_A": 1, "price_A_month": 1}) {
+ t.Errorf("price fetches after a second Checkout = %v, want no more", got)
+ }
+
if _, err := s.Checkout(ctx, req("annual", "")); !errors.Is(err, abono.ErrUnknownPlan) {
t.Errorf("two sold prices and no interval: err = %v, want ErrUnknownPlan", err)
}
@@ -133,11 +144,27 @@ func TestCheckoutChoosesThePriceByInterval(t *testing.T) {
t.Errorf("an interval the plan is not sold at: err = %v, want ErrUnknownPlan", err)
}
+ // One sold price, no interval asked for: that one.
+ if _, err := s.Checkout(ctx, req("monthly", "")); err != nil {
+ t.Fatalf("one sold price: %v", err)
+ }
+ if got := d.lastCheckout(t).Get("line_items[0][price]"); got != "price_M" {
+ t.Errorf("monthly: price = %q, want price_M", got)
+ }
+
// A legacy price is never sold.
db.exec(`UPDATE abono_prices SET sold = 0 WHERE plan = 'monthly'`)
if _, err := s.Checkout(ctx, req("monthly", "")); !errors.Is(err, abono.ErrUnknownPlan) {
t.Errorf("a plan with no sold price: err = %v, want ErrUnknownPlan", err)
}
+
+ // A sold price Stripe no longer has stops the sale rather than guessing.
+ if err := s.AddPrice(ctx, "annual", "price_gone", true); err != nil {
+ t.Fatalf("AddPrice: %v", err)
+ }
+ if _, err := s.Checkout(ctx, req("annual", "year")); err == nil {
+ t.Error("Checkout with a sold price Stripe cannot find succeeded")
+ }
}
func TestPortalWithoutCustomer(t *testing.T) {
diff --git a/stripe_double_test.go b/stripe_double_test.go
index 45f4345..a83cc5a 100644
--- a/stripe_double_test.go
+++ b/stripe_double_test.go
@@ -31,14 +31,28 @@ type stripeDouble struct {
checkouts []url.Values
portals []url.Values
+ prices map[string]any
+ priceFetches map[string]int
+
// fetched, when set, is called each time a subscription is read.
fetched func()
}
func newStripeDouble(t *testing.T) *stripeDouble {
t.Helper()
- d := &stripeDouble{subs: map[string]any{}, sessions: map[string]any{}, promos: map[string]string{}}
+ d := &stripeDouble{subs: map[string]any{}, sessions: map[string]any{}, promos: map[string]string{},
+ prices: map[string]any{}, priceFetches: map[string]int{}}
+ // The prices seedPlans names, so Checkout can learn their intervals.
+ d.price("price_A", "year", 6000, "eur")
+ d.price("price_old", "year", 4000, "eur")
+ d.price("price_M", "month", 600, "eur")
mux := http.NewServeMux()
+ mux.HandleFunc("GET /v1/prices/{id}", func(w http.ResponseWriter, r *http.Request) {
+ d.mu.Lock()
+ d.priceFetches[r.PathValue("id")]++
+ d.mu.Unlock()
+ d.reply(w, d.prices, r.PathValue("id"))
+ })
mux.HandleFunc("GET /v1/subscriptions/{id}", func(w http.ResponseWriter, r *http.Request) {
d.reply(w, d.subs, r.PathValue("id"))
if d.fetched != nil {
@@ -145,6 +159,25 @@ func (d *stripeDouble) put(t *testing.T, sp subSpec) stripeapi.Subscription {
return asSubscription(t, obj)
}
+// price stores a recurring price.
+func (d *stripeDouble) price(id, interval string, amount int64, currency string) {
+ d.mu.Lock()
+ defer d.mu.Unlock()
+ d.prices[id] = map[string]any{"id": id, "unit_amount": amount, "currency": currency,
+ "recurring": map[string]any{"interval": interval}}
+}
+
+// fetches is how many times each price has been read.
+func (d *stripeDouble) fetches() map[string]int {
+ d.mu.Lock()
+ defer d.mu.Unlock()
+ out := map[string]int{}
+ for k, v := range d.priceFetches {
+ out[k] = v
+ }
+ return out
+}
+
// session stores a Checkout session.
func (d *stripeDouble) session(id, status, subID, ref, email string) {
d.mu.Lock()
diff --git a/sync.go b/sync.go
index 78979f8..6e7733d 100644
--- a/sync.go
+++ b/sync.go
@@ -15,10 +15,10 @@ import (
// customer: Checkout's client_reference_id (or the subscription's
// abono_subject metadata) and the address typed there.
//
-// A hint is also the permission to record a subscription abono has no row
-// for: a sync with the zero Hint only updates rows that already exist (the
-// spec's "no resurrection" rule). The webhook passes a non-zero hint for
-// the events that may create a row.
+// A hint is also what lets a subscription on a price no offer has be
+// recorded as unrecognised: with the zero Hint only a subscription that
+// grants on a known price is recorded (ruling R16), so a reconcile recovers
+// a webhook Stripe gave up on and the account's other sales stay out.
type Hint struct {
Subject, Email string
}
@@ -58,10 +58,12 @@ func (s *Service) sync(ctx context.Context, sub stripeapi.Subscription, hint Hin
// first's row and updates it, and Holder is asked at most once. Holder is
// only ever called from here, inside tx.
//
-// With no row yet, one is made only when the subscription grants and there
-// is a hint, or when adopt is set (an owner said so). The plan is the one
-// whose prices include the subscription's first price, or "" when none
-// does: such a subscription is recorded as unrecognised and never grants.
+// With no row yet, one is made when adopt is set (an owner said so), or when
+// the subscription grants and either its price belongs to a plan or there
+// is a hint. An ended subscription first seen now is never recorded (the
+// spec's "no resurrection" rule). The plan is the one whose prices include
+// the subscription's first price, or "" when none does: such a subscription
+// is recorded as unrecognised and never grants.
//
// OnChange runs for the subject before and after whenever the subject,
// plan, status, period end or cancel flag changed, but only for a side that
@@ -100,10 +102,10 @@ func (s *Service) syncTx(ctx context.Context, tx *gorm.DB, sub stripeapi.Subscri
if len(have) == 0 {
ok, _ := grants(row.Status, row.CurrentPeriodEnd, row.CancelAtPeriodEnd, now)
- if !adopt && (hint == (Hint{}) || !ok) {
+ if !adopt && (!ok || (plan == "" && hint == (Hint{}))) {
// An ended subscription first seen now (a reconcile reading the
- // account's history), or one nobody vouched for: not ours to
- // record.
+ // account's history), or another sale on the account that nobody
+ // tied to this site: not ours to record.
return "", nil
}
// Holder is never asked about a price no offer recognises.
diff --git a/sync_test.go b/sync_test.go
index b53713b..88cfee9 100644
--- a/sync_test.go
+++ b/sync_test.go
@@ -45,6 +45,10 @@ func eq(t *testing.T, what string, got, want []string) {
}
}
+// TestSyncCreatesOnlyWithAHint is the creation rule (spec 4.1, ruling R16):
+// with no hint, only a subscription that grants on a known price is
+// recorded, so a reconcile recovers a webhook Stripe gave up on; an ended
+// one, or one on a price no offer has, needs a hint.
func TestSyncCreatesOnlyWithAHint(t *testing.T) {
now := t0
d := newStripeDouble(t)
@@ -53,28 +57,35 @@ func TestSyncCreatesOnlyWithAHint(t *testing.T) {
seedPlans(t, s)
ctx := context.Background()
- sub := d.put(t, subSpec{ID: "sub_1", Status: "active", Price: "price_A", Customer: "cus_1", Email: "Ana@Example.com", End: t0.Add(300 * day)})
- if err := s.Sync(ctx, sub, abono.Hint{}); err != nil {
- t.Fatalf("Sync with no hint: %v", err)
+ // Ended, on a known price: nothing, with or without a hint.
+ ended := d.put(t, subSpec{ID: "sub_0", Status: "canceled", Price: "price_A", Customer: "cus_1", Email: "ana@example.com", End: t0.Add(-day)})
+ for _, hint := range []abono.Hint{{}, {Email: "ana@example.com"}} {
+ if err := s.Sync(ctx, ended, hint); err != nil {
+ t.Fatalf("Sync of an ended subscription, hint %+v: %v", hint, err)
+ }
}
if got := db.dump(rowsQuery); got != "" {
- t.Fatalf("a sync with no hint and no customer mapping made rows:\n%s", got)
+ t.Fatalf("an ended subscription first seen now was recorded:\n%s", got)
}
- asked, changes := rec.take()
- eq(t, "Holder asked", asked, nil)
- eq(t, "OnChange", changes, nil)
- // A hinted sync of a subscription that does not grant records nothing.
- ended := d.put(t, subSpec{ID: "sub_0", Status: "canceled", Price: "price_A", Customer: "cus_1", Email: "ana@example.com", End: t0.Add(-day)})
- if err := s.Sync(ctx, ended, abono.Hint{Email: "ana@example.com"}); err != nil {
- t.Fatalf("Sync of an ended subscription: %v", err)
+ // Granting, on a price no offer has, no hint: the account's other
+ // sales stay out, and Holder is not asked.
+ other := d.put(t, subSpec{ID: "sub_z", Status: "active", Price: "price_Z", Customer: "cus_9", Email: "zed@example.com", End: t0.Add(30 * day)})
+ if err := s.Sync(ctx, other, abono.Hint{}); err != nil {
+ t.Fatalf("Sync of an unknown price with no hint: %v", err)
}
if got := db.dump(rowsQuery); got != "" {
- t.Fatalf("an ended subscription first seen now was recorded:\n%s", got)
+ t.Fatalf("an unknown price with no hint was recorded:\n%s", got)
}
+ asked, changes := rec.take()
+ eq(t, "Holder asked", asked, nil)
+ eq(t, "OnChange", changes, nil)
- if err := s.Sync(ctx, sub, abono.Hint{Email: "ana@example.com"}); err != nil {
- t.Fatalf("Sync with a hint: %v", err)
+ // Granting, on a known price, no hint: recovered, its holder found by
+ // the customer's address.
+ sub := d.put(t, subSpec{ID: "sub_1", Status: "active", Price: "price_A", Customer: "cus_1", Email: "Ana@Example.com", End: t0.Add(300 * day)})
+ if err := s.Sync(ctx, sub, abono.Hint{}); err != nil {
+ t.Fatalf("Sync with no hint: %v", err)
}
if got, want := db.dump(rowsQuery), "sub_1|cus_1|42|annual|price_A|active|0|ana@example.com\n"; got != want {
t.Fatalf("rows =\n%s\nwant\n%s", got, want)
@@ -85,8 +96,21 @@ func TestSyncCreatesOnlyWithAHint(t *testing.T) {
asked, changes = rec.take()
eq(t, "Holder asked", asked, []string{"ana@example.com"})
eq(t, "OnChange", changes, []string{"42"})
+
+ // A legacy price is a known price too, and a mapped customer decides
+ // before Holder is asked.
+ legacy := d.put(t, subSpec{ID: "sub_L", Status: "active", Price: "price_old", Customer: "cus_1", Email: "ana@example.com", End: t0.Add(30 * day)})
+ if err := s.Sync(ctx, legacy, abono.Hint{}); err != nil {
+ t.Fatalf("Sync of a legacy price: %v", err)
+ }
+ if got := db.dump("SELECT subject, plan FROM abono_subscriptions WHERE stripe_subscription_id = 'sub_L'"); got != "42|annual\n" {
+ t.Errorf("legacy row = %q, want 42|annual", got)
+ }
+ asked, changes = rec.take()
+ eq(t, "Holder asked for a mapped customer", asked, nil)
+ eq(t, "OnChange for the legacy row", changes, []string{"42"})
if !hasPlan(t, s, "42", "annual") {
- t.Error("HasPlan(42, annual) is false after the hinted sync")
+ t.Error("HasPlan(42, annual) is false after the recovered sync")
}
// The same subscription again changes nothing and tells nobody.
@@ -102,7 +126,7 @@ func TestSyncCreatesOnlyWithAHint(t *testing.T) {
if err := s.Sync(ctx, sub, abono.Hint{}); err != nil {
t.Fatalf("Sync of a cancelling subscription: %v", err)
}
- if got := db.dump("SELECT cancel_at_period_end FROM abono_subscriptions"); got != "1\n" {
+ if got := db.dump("SELECT cancel_at_period_end FROM abono_subscriptions WHERE stripe_subscription_id = 'sub_1'"); got != "1\n" {
t.Errorf("cancel_at_period_end = %q, want 1", got)
}
_, changes = rec.take()