rastrillo / abono Public

abono: sandbox pages in the approved English, the webhook address on the Stripe page, and Adopt brings everyone on the price

Summary

Three bugs from the final review. The sandbox's stand-in Stripe pages read abono's shipped catalog (locales/en.toml) instead of an empty map, so they show the approved text, not keys. The generated Stripe page shows the approved label abono.owner.stripe_webhook and, under it, the webhook URL in its own <code>; the label has no {url} placeholder, so the address never appeared. Adopt now locks, fetches and syncs, in one transaction, every recorded subscription on the adopted price not yet on the plan, so they hold the plan and OnChange has run for them before Adopt returns, as the page's notice says.

Agent notes

Final review I8, I9 and I11, ruling R40. Adopt takes its locks in subscription-id order so two adoptions cannot deadlock; it finds the price from the recorded row (or a first read) and re-checks it inside the locks. TestAnOlderSnapshotNeverOverwritesANewerOne now records sub_1 first, so Adopt's first Stripe read is the in-lock one. Example pages regenerated. Regressions: TestSandboxRunsTheSameFlowWithoutStripe asserts the approved sandbox text and no keys; example TestStripePageShowsTheWebhookAddress (real catalog, expected endpoint); TestAdoptBringsEveryoneOnThePrice (two holders on one unknown price).

claude 2 weeks ago 05bd6a66b2b93086380c394eccd8d8b2bffe8454 parent bd3d884

8 files changed, +210 −24

adopt.go +100 −10
adopt_test.go +63 −0
cmd/abono/pages/owner-stripe.html.tmpl +2 −1
example/app_test.go +24 −0
example/templates/abono/owner-stripe.html +2 −1
sandbox.go +5 −8
sandbox_test.go +9 −3
sync_test.go +5 −1

CI passed — run details

CI log

=== vet ===
go vet ./...
=== fmt ===
=== test ===
go test ./... -count=1
ok  	amadan.net/rastrillo/abono	2.598s
ok  	amadan.net/rastrillo/abono/abonotest	0.053s
ok  	amadan.net/rastrillo/abono/cmd/abono	0.653s
ok  	amadan.net/rastrillo/abono/internal/fakestripe	0.019s
ok  	amadan.net/rastrillo/abono/internal/stripeapi	0.006s
ok  	example	0.173s
=== race ===
CGO_ENABLED=1 go test ./... -race -count=1
ok  	amadan.net/rastrillo/abono	8.197s
ok  	amadan.net/rastrillo/abono/abonotest	1.388s
ok  	amadan.net/rastrillo/abono/cmd/abono	1.840s
ok  	amadan.net/rastrillo/abono/internal/fakestripe	1.059s
ok  	amadan.net/rastrillo/abono/internal/stripeapi	1.018s
ok  	example	2.028s
=== browser ===
ok  	example	0.840s

Intent

full ledger: v0.1

diff --git a/adopt.go b/adopt.go

index a9a4671..3147882 100644

--- a/adopt.go

+++ b/adopt.go

@@ -4,8 +4,11 @@ import (

"context"

"errors"

"fmt"

+ "slices"

"gorm.io/gorm"

+ "amadan.net/rastrillo/abono/internal/stripeapi"

)

// Adopt takes an existing Stripe subscription as subject's holding of plan:

@@ -15,6 +18,11 @@ import (

// belongs to another plan is refused, since moving it would change what

// every subscription on it means.

+// Every other subscription abono has recorded on that price and not yet on

+// plan is synced again in the same transaction, so everyone paying that

+// price holds the plan, and the app has been told, by the time Adopt

+// returns (ruling R40), as the owner's page says.

+//

// With subject "", the holder is found as a sync finds it: the customer's

// recorded subject, then Holder for the customer's address. With a

// subject, that subject is pinned: Checkout's reference and the

@@ -28,22 +36,96 @@ func (s *Service) Adopt(ctx context.Context, subscriptionID, subject, plan strin

if subscriptionID == "" || plan == "" {

return errors.New("abono: Adopt needs a subscription and a plan")

}

- // The subscription's lock from the fetch to the commit (ruling R35).

- unlock, err := s.subs.lock(ctx, subscriptionID)

+ // The price decides which other subscriptions are locked with this

+ // one. It is read again inside the locks; should it have changed in

+ // between (a plan switch at that moment), the set is worked out again.

+ price, err := s.recordedPrice(ctx, subscriptionID)

if err != nil {

return err

}

- defer unlock()

+ for range 3 {

+ if price == "" {

+ sub, err := s.stripe.GetSubscription(ctx, subscriptionID)

+ if err != nil {

+ return err

+ }

+ if price, _ = sub.PriceID(); price == "" {

+ return fmt.Errorf("abono: subscription %q has no price", subscriptionID)

+ }

+ now, err := s.adopt(ctx, subscriptionID, subject, plan, price)

+ if !errors.Is(err, errPriceMoved) {

+ return err

+ }

+ price = now

+ }

+ return fmt.Errorf("abono: subscription %q kept changing its price while being adopted", subscriptionID)

+// errPriceMoved is adopt finding, inside the locks, that the subscription

+// is no longer on the price it locked for.

+var errPriceMoved = errors.New("abono: the subscription's price changed")

+// recordedPrice is the price abono last recorded for subscription id, or

+// "" when there is no row.

+func (s *Service) recordedPrice(ctx context.Context, id string) (string, error) {

+ var prices []string

+ err := s.cfg.DB.WithContext(ctx).Raw(`SELECT stripe_price_id FROM abono_subscriptions WHERE stripe_subscription_id = ?`, id).

+ Scan(&prices).Error

+ if err != nil || len(prices) == 0 {

+ return "", err

+ }

+ return prices[0], nil

+// adopt is Adopt for a subscription believed to be on price. It locks the

+// subscription and every other recorded one on price not yet on plan, in

+// id order (so two adoptions cannot wait on each other), fetches each from

+// Stripe inside its lock (ruling R35), and writes them all in one

+// transaction. When the subscription turns out to be on another price it

+// returns errPriceMoved and that price.

+func (s *Service) adopt(ctx context.Context, subscriptionID, subject, plan, price string) (string, error) {

+ var others []string

+ if err := s.cfg.DB.WithContext(ctx).Raw(`SELECT stripe_subscription_id FROM abono_subscriptions

+ WHERE stripe_price_id = ? AND stripe_subscription_id <> ? AND plan <> ?

+ ORDER BY stripe_subscription_id`, price, subscriptionID, plan).Scan(&others).Error; err != nil {

+ return "", err

+ }

+ ids := append([]string{subscriptionID}, others...)

+ slices.Sort(ids)

+ for _, id := range ids {

+ unlock, err := s.subs.lock(ctx, id)

+ if err != nil {

+ return "", err

+ }

+ defer unlock()

+ }

sub, err := s.stripe.GetSubscription(ctx, subscriptionID)

if err != nil {

- return err

+ return "", err

+ }

+ now, _ := sub.PriceID()

+ switch {

+ case now == "":

+ return "", fmt.Errorf("abono: subscription %q has no price", subscriptionID)

+ case now != price:

+ return now, errPriceMoved

}

- price, _ := sub.PriceID()

interval, count := sub.Recurring()

- if price == "" {

- return fmt.Errorf("abono: subscription %q has no price", subscriptionID)

+ var rest []stripeapi.Subscription

+ for _, id := range others {

+ o, err := s.stripe.GetSubscription(ctx, id)

+ switch {

+ case stripeapi.NotFound(err):

+ continue // gone from Stripe; nothing to bring onto the plan

+ case err != nil:

+ return "", err

+ }

+ rest = append(rest, o)

}

- return s.tx(ctx, func(tx *gorm.DB) error {

+ return "", s.tx(ctx, func(tx *gorm.DB) error {

var n int64

if err := tx.Table("abono_offers").Where("plan = ?", plan).Count(&n).Error; err != nil {

return err

@@ -63,7 +145,15 @@ func (s *Service) Adopt(ctx context.Context, subscriptionID, subject, plan strin

case on != plan:

return fmt.Errorf("abono: price %q belongs to plan %q, not %q", price, on, plan)

}

- _, err = s.syncTx(ctx, tx, sub, Hint{Subject: subject}, true)

- return err

+ if _, err := s.syncTx(ctx, tx, sub, Hint{Subject: subject}, true); err != nil {

+ return err

+ }

+ // Everyone else on the price, as their next event would sync them.

+ for _, o := range rest {

+ if _, err := s.syncTx(ctx, tx, o, subscriptionHint(o), false); err != nil {

+ return fmt.Errorf("abono: adopt: bringing subscription %s onto %q: %w", o.ID, plan, err)

+ }

+ return nil

})

}

diff --git a/adopt_test.go b/adopt_test.go

index 006c6ca..168e053 100644

--- a/adopt_test.go

+++ b/adopt_test.go

@@ -4,8 +4,13 @@ import (

"context"

"errors"

"net/http"

+ "sort"

+ "strings"

+ "sync"

"testing"

+ "gorm.io/gorm"

"amadan.net/rastrillo/abono"

)

@@ -173,3 +178,61 @@ func TestAnOwnersAdoptionOutlastsLaterHints(t *testing.T) {

t.Errorf("a claimed subscription went to %q on a later hint, want C", got)

}

+// TestAdoptBringsEveryoneOnThePrice is final review I11 (ruling R40): the

+// page promises that adopting one subscription puts everyone paying that

+// price onto the plan. Two holders are recorded on a price no plan has;

+// adopting one makes both members at once, each told through OnChange,

+// before Adopt returns.

+func TestAdoptBringsEveryoneOnThePrice(t *testing.T) {

+ now := t0

+ d := newStripeDouble(t)

+ var mu sync.Mutex

+ var told []string

+ s, db := d.service(t, &now, abono.Config{

+ Holder: func(_ context.Context, _ *gorm.DB, email, _ string) (string, error) {

+ return strings.TrimSuffix(email, "@example.com"), nil

+ },

+ OnChange: func(_ context.Context, _ *gorm.DB, subject string) error {

+ mu.Lock()

+ defer mu.Unlock()

+ told = append(told, subject)

+ return nil

+ },

+ })

+ seedPlans(t, s)

+ ctx := context.Background()

+ for _, sp := range []subSpec{

+ {ID: "sub_z1", Status: "active", Price: "price_Z", Customer: "cus_1", Email: "one@example.com", End: t0.Add(30 * day)},

+ {ID: "sub_z2", Status: "active", Price: "price_Z", Customer: "cus_2", Email: "two@example.com", End: t0.Add(30 * day)},

+ } {

+ if err := s.Sync(ctx, d.put(t, sp), abono.Hint{}); err != nil {

+ t.Fatalf("Sync %s: %v", sp.ID, err)

+ }

+ if un, err := s.Unrecognised(ctx); err != nil || len(un) != 2 {

+ t.Fatalf("Unrecognised = %+v, %v; want both", un, err)

+ }

+ if err := s.Adopt(ctx, "sub_z1", "", "monthly"); err != nil {

+ t.Fatalf("Adopt: %v", err)

+ }

+ if got := db.dump(`SELECT stripe_subscription_id, subject, plan FROM abono_subscriptions ORDER BY stripe_subscription_id`); got != "sub_z1|one|monthly\nsub_z2|two|monthly\n" {

+ t.Errorf("rows after Adopt =\n%s\nwant both on monthly, each with its holder", got)

+ }

+ for _, who := range []string{"one", "two"} {

+ if !hasPlan(t, s, who, "monthly") {

+ t.Errorf("%s does not hold monthly after the adoption", who)

+ }

+ mu.Lock()

+ sort.Strings(told)

+ got := strings.Join(told, ",")

+ mu.Unlock()

+ if got != "one,two" {

+ t.Errorf("OnChange told %q, want one and two", got)

+ }

+ if un, err := s.Unrecognised(ctx); err != nil || len(un) != 0 {

+ t.Errorf("Unrecognised after Adopt = %+v, %v; want none", un, err)

+ }

diff --git a/cmd/abono/pages/owner-stripe.html.tmpl b/cmd/abono/pages/owner-stripe.html.tmpl

index 826aa49..795fd7f 100644

--- a/cmd/abono/pages/owner-stripe.html.tmpl

+++ b/cmd/abono/pages/owner-stripe.html.tmpl

@@ -16,7 +16,8 @@

{{T $L "abono.owner.stripe_off_help"}}

{{if or (eq .Mode "test") (eq .Mode "live")}}<section rst-box>

-{{Tf $L "abono.owner.stripe_webhook" "url" .WebhookURL}}

+{{T $L "abono.owner.stripe_webhook"}}

+<code>{{.WebhookURL}}</code>

{{T $L "abono.owner.stripe_webhook_help"}}

</section>

diff --git a/example/app_test.go b/example/app_test.go

index 0cb83aa..f1f53b9 100644

--- a/example/app_test.go

+++ b/example/app_test.go

@@ -4,6 +4,7 @@ import (

"bytes"

"context"

"encoding/json"

+ "html"

"io"

"io/fs"

"log/slog"

@@ -682,3 +683,26 @@ func TestUnsupportedPriceIsShownNotSold(t *testing.T) {

t.Errorf("abonoCards made %d cards for an unsupported price", len(cards))

}

+// TestStripePageShowsTheWebhookAddress is final review I9 (ruling R40),

+// with the shipped English rather than the test catalog: the approved

+// label "Webhook address" has no {url} in it, so the address itself is

+// shown beside it, or the instruction under it points at nothing.

+func TestStripePageShowsTheWebhookAddress(t *testing.T) {

+ stripe := abonotest.New(t)

+ stripe.AddPrice("price_annual", "annual", 6000, "eur", "year")

+ stripe.AddPrice("price_monthly", "monthly", 600, "eur", "month")

+ app := newApp(t, func(c *Config) {

+ c.Abono = stripe.Config

+ c.Catalog = nil // abono.Catalog(), the approved English

+ })

+ boss := newClient(t, app)

+ boss.signin(owner)

+ status, body := boss.get("/admin/stripe")

+ approved := abono.Catalog()

+ want(t, "the Stripe page", status, http.StatusOK, body,

+ html.EscapeString(approved["abono.owner.stripe_test"]),

+ html.EscapeString(approved["abono.owner.stripe_webhook"]),

+ "<code>"+app.URL+abono.WebhookPath+"</code>",

+ html.EscapeString(approved["abono.owner.stripe_webhook_help"]))

diff --git a/example/templates/abono/owner-stripe.html b/example/templates/abono/owner-stripe.html

index 826aa49..795fd7f 100644

--- a/example/templates/abono/owner-stripe.html

+++ b/example/templates/abono/owner-stripe.html

@@ -16,7 +16,8 @@

{{T $L "abono.owner.stripe_off_help"}}

{{if or (eq .Mode "test") (eq .Mode "live")}}<section rst-box>

-{{Tf $L "abono.owner.stripe_webhook" "url" .WebhookURL}}

+{{T $L "abono.owner.stripe_webhook"}}

+<code>{{.WebhookURL}}</code>

{{T $L "abono.owner.stripe_webhook_help"}}

</section>

diff --git a/sandbox.go b/sandbox.go

index a088429..734ecf8 100644

--- a/sandbox.go

+++ b/sandbox.go

@@ -85,15 +85,12 @@ func (t inProcess) RoundTrip(req *http.Request) (*http.Response, error) {

return w.Result(), nil

}

-// catalog is abono's English, which arrives with the reviewed copy. Until

-// then it is empty, and every key reads as itself, as a key missing from

-// rastrillo's catalogs does.

-var catalog map[string]string

-// text is the string for key, its {name} placeholders filled from

-// alternating name/value args; the key itself when the catalog has none.

+// text is the sandbox pages' string for key from abono's shipped English

+// (locales/en.toml, the approved copy), its {name} placeholders filled

+// from alternating name/value args; the key itself when the catalog has

+// none, as a key missing from rastrillo's catalogs reads.

func text(key string, args ...any) string {

- s, ok := catalog[key]

+ s, ok := english[key]

if !ok {

return key

}

diff --git a/sandbox_test.go b/sandbox_test.go

index 8f98428..518b66c 100644

--- a/sandbox_test.go

+++ b/sandbox_test.go

@@ -4,6 +4,7 @@ import (

"bytes"

"context"

"errors"

+ "html"

"io"

"log/slog"

"net/http"

@@ -120,11 +121,16 @@ func TestSandboxRunsTheSameFlowWithoutStripe(t *testing.T) {

if code != http.StatusOK {

t.Fatalf("checkout page: status %d", code)

}

+ // The approved English, not the keys (final review I8, ruling R40).

+ approved := abono.Catalog()

for _, key := range []string{"abono.sandbox.banner", "abono.sandbox.checkout_title", "abono.sandbox.pay", "abono.sandbox.decline"} {

- if !strings.Contains(page, key) {

- t.Errorf("the checkout page does not show %s", key)

+ if text := html.EscapeString(approved[key]); approved[key] == "" || !strings.Contains(page, text) {

+ t.Errorf("the checkout page does not say %s (%q)", key, approved[key])

}

+ if strings.Contains(page, "abono.sandbox.") {

+ t.Errorf("the checkout page shows message keys:\n%s", page)

+ }

// The pages are browser forms: csrf.Protect turns away a POST from

// anywhere else.

@@ -154,7 +160,7 @@ func TestSandboxRunsTheSameFlowWithoutStripe(t *testing.T) {

if !strings.HasPrefix(pu, origin+abono.SandboxPath+"portal/") {

t.Fatalf("Portal URL = %q", pu)

}

- if code, _, page = b.do(http.MethodGet, pu, nil); code != http.StatusOK || !strings.Contains(page, "abono.sandbox.portal_cancel") {

+ if code, _, page = b.do(http.MethodGet, pu, nil); code != http.StatusOK || !strings.Contains(page, html.EscapeString(approved["abono.sandbox.portal_cancel"])) {

t.Fatalf("portal page: status %d\n%s", code, page)

}

hs, err := s.Holdings(ctx, "ana")

diff --git a/sync_test.go b/sync_test.go

index ab5c277..1dfc4b9 100644

--- a/sync_test.go

+++ b/sync_test.go

@@ -725,7 +725,11 @@ func TestAnOlderSnapshotNeverOverwritesANewerOne(t *testing.T) {

s, db := d.service(t, &now, rec.config())

seedPlans(t, s)

active := subSpec{ID: "sub_1", Status: "active", Price: "price_A", Customer: "cus_1", Email: "ana@example.com", End: t0.Add(30 * day)}

- d.put(t, active)

+ // Recorded already, so every entry point's first read of sub_1

+ // from Stripe is the one it writes from.

+ if err := s.Sync(context.Background(), d.put(t, active), abono.Hint{}); err != nil {

+ t.Fatal(err)

+ }

d.session("cs_1", "complete", "sub_1", "", "ana@example.com")

var bErr error