amadan

rastrillo / abono Public

abono: Finish checks payment, late addresses claim, success URLs keep their fragment

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

Summary

Finish reports Paid and syncs only for a complete session whose payment_status is paid or no_payment_required; a complete but unpaid session (a debit still processing) records nothing and is left to the webhook. stripeapi.CheckoutSession gains PaymentStatus. An existing row with no subject on a known plan now resolves its holder on every sync as a new row would (hint subject, customer mapping, then Holder for the address it has), so a row first recorded without an address is claimed once one arrives; a declined Holder keeps it unclaimed with the address stored. success_url is built by parsing the path: session_id replaces any already in the query, the rest of the query is kept as written, the fragment stays after the query, and the {CHECKOUT_SESSION_ID} braces are left literal.
Agent notes
Holder is now asked on each sync of an unclaimed row that has an address, so an app that wants sign-in first sees repeated calls that return ""; Holder must stay cheap and idempotent (it already runs as find-or-create). Review fix round 1 of Task 5.
claude c7a5c50fb524895bc79baa170cbd9675bf7249e8 parent a928c76
6 files changed, +195 −18
  • checkout.go +37 −9
  • checkout_test.go +31 −0
  • internal/stripeapi/client.go +7 −3
  • stripe_double_test.go +14 −2
  • sync.go +7 −4
  • sync_test.go +99 −0

CI passed — run details

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

Intent

full ledger: v0.1

diff --git a/checkout.go b/checkout.go
index 5d6c7a4..ec36f47 100644
--- a/checkout.go
+++ b/checkout.go
@@ -55,14 +55,10 @@ func (s *Service) Checkout(ctx context.Context, req CheckoutRequest) (string, er
if err != nil {
return "", err
}
- success, err := s.back(req.SuccessPath)
+ success, err := s.successURL(req.SuccessPath)
if err != nil {
return "", err
}
- sep := "?"
- if strings.Contains(success, "?") {
- sep = "&"
- }
cancel, err := s.back(req.CancelPath)
if err != nil {
return "", err
@@ -71,7 +67,7 @@ func (s *Service) Checkout(ctx context.Context, req CheckoutRequest) (string, er
"mode": {"subscription"},
"line_items[0][price]": {price},
"line_items[0][quantity]": {"1"},
- "success_url": {success + sep + "session_id={CHECKOUT_SESSION_ID}"},
+ "success_url": {success},
"cancel_url": {cancel},
"subscription_data[metadata][abono_plan]": {req.Plan},
}
@@ -175,6 +171,32 @@ func (s *Service) back(path string) (string, error) {
return strings.TrimRight(s.cfg.Origin, "/") + path, nil
}
+// successURL is Origin plus path with session_id={CHECKOUT_SESSION_ID} in
+// its query, for Finish: any session_id already there is replaced, the rest
+// of the query is kept as written, and a fragment stays after the query, so
+// the parameter reaches the server. The braces are left as they are:
+// Stripe looks for the placeholder literally.
+func (s *Service) successURL(path string) (string, error) {
+ base, err := s.back(path)
+ if err != nil {
+ return "", err
+ }
+ u, err := url.Parse(base)
+ if err != nil {
+ return "", fmt.Errorf("abono: success path %q: %w", path, err)
+ }
+ var keep []string
+ for _, kv := range strings.Split(u.RawQuery, "&") {
+ k, _, _ := strings.Cut(kv, "=")
+ if key, err := url.QueryUnescape(k); kv == "" || (err == nil && key == "session_id") {
+ continue
+ }
+ keep = append(keep, kv)
+ }
+ u.RawQuery = strings.Join(append(keep, "session_id={CHECKOUT_SESSION_ID}"), "&")
+ return u.String(), nil
+}
+
// customerOf is the Stripe customer subject pays as: their newest
// subscription's, else one mapped to them, else "".
func customerOf(g *gorm.DB, subject string) (string, error) {
@@ -208,8 +230,9 @@ type Finished struct {
// Finish is Checkout's return. It syncs the session's subscription at once
// rather than waiting for the webhook, so the next page already reflects
-// the payment. It signs nobody in. A session that is not complete is not
-// paid, and records nothing.
+// the payment. It signs nobody in. Paid is true only for a complete session
+// whose payment_status is paid or no_payment_required; any other session
+// records nothing.
func (s *Service) Finish(ctx context.Context, sessionID string) (Finished, error) {
if !s.On() {
return Finished{}, errOff
@@ -219,7 +242,12 @@ func (s *Service) Finish(ctx context.Context, sessionID string) (Finished, error
return Finished{}, err
}
f := Finished{Email: normalEmail(cs.CustomerDetails.Email)}
- if cs.Status != "complete" || cs.Subscription == "" {
+ // Complete is not paid: a bank debit completes the session while the
+ // money is still on its way. Then nothing is recorded here, and the
+ // webhook syncs when the payment lands. A trial or a 100% code has
+ // nothing to pay, and is paid.
+ paid := cs.PaymentStatus == "paid" || cs.PaymentStatus == "no_payment_required"
+ if cs.Status != "complete" || !paid || cs.Subscription == "" {
return f, nil
}
sub, err := s.stripe.GetSubscription(ctx, cs.Subscription)
diff --git a/checkout_test.go b/checkout_test.go
index 1c8edb6..043b77e 100644
--- a/checkout_test.go
+++ b/checkout_test.go
@@ -182,6 +182,37 @@ func TestCheckoutChoosesThePriceByInterval(t *testing.T) {
}
}
+// TestCheckoutSuccessURL: session_id goes in the query, before any
+// fragment, replacing one already there, and Stripe's placeholder reaches
+// it with its braces.
+func TestCheckoutSuccessURL(t *testing.T) {
+ now := t0
+ d := newStripeDouble(t)
+ s, _ := d.service(t, &now, abono.Config{Origin: "https://site.example/"})
+ seedPlans(t, s)
+ ctx := context.Background()
+
+ for path, want := range map[string]string{
+ "": "https://site.example/?session_id={CHECKOUT_SESSION_ID}",
+ "/done": "https://site.example/done?session_id={CHECKOUT_SESSION_ID}",
+ "/done#receipt": "https://site.example/done?session_id={CHECKOUT_SESSION_ID}#receipt",
+ "/done?session_id=old&x=1": "https://site.example/done?x=1&session_id={CHECKOUT_SESSION_ID}",
+ "/done?a=1&session_id=old#f": "https://site.example/done?a=1&session_id={CHECKOUT_SESSION_ID}#f",
+ } {
+ if _, err := s.Checkout(ctx, abono.CheckoutRequest{Plan: "annual", SuccessPath: path}); err != nil {
+ t.Fatalf("SuccessPath %q: %v", path, err)
+ }
+ if got := d.lastCheckout(t).Get("success_url"); got != want {
+ t.Errorf("SuccessPath %q: success_url = %q, want %q", path, got, want)
+ }
+ }
+ for _, bad := range []string{"done", "//evil.example/x", "https://evil.example/"} {
+ if _, err := s.Checkout(ctx, abono.CheckoutRequest{Plan: "annual", SuccessPath: bad}); err == nil {
+ t.Errorf("SuccessPath %q accepted", bad)
+ }
+ }
+}
+
func TestPortalWithoutCustomer(t *testing.T) {
now := t0
d := newStripeDouble(t)
diff --git a/internal/stripeapi/client.go b/internal/stripeapi/client.go
index 2d2dd79..d3b4d50 100644
--- a/internal/stripeapi/client.go
+++ b/internal/stripeapi/client.go
@@ -161,9 +161,13 @@ func (c *Client) GetPrice(ctx context.Context, id string) (Price, error) {
// CheckoutSession is a hosted Checkout page, or one that has finished.
type CheckoutSession struct {
- ID string `json:"id"`
- URL string `json:"url"`
- Status string `json:"status"`
+ ID string `json:"id"`
+ URL string `json:"url"`
+ Status string `json:"status"`
+ // PaymentStatus is paid, unpaid (a debit still processing) or
+ // no_payment_required (a trial, a 100% code). A complete session may
+ // still be unpaid.
+ PaymentStatus string `json:"payment_status"`
ClientReferenceID string `json:"client_reference_id"`
Subscription string `json:"subscription"`
CustomerDetails struct {
diff --git a/stripe_double_test.go b/stripe_double_test.go
index a83cc5a..90c4b57 100644
--- a/stripe_double_test.go
+++ b/stripe_double_test.go
@@ -178,16 +178,28 @@ func (d *stripeDouble) fetches() map[string]int {
return out
}
-// session stores a Checkout session.
+// session stores a Checkout session. A complete one is paid, an open one
+// unpaid; paying overrides that.
func (d *stripeDouble) session(id, status, subID, ref, email string) {
+ paying := "unpaid"
+ if status == "complete" {
+ paying = "paid"
+ }
d.mu.Lock()
defer d.mu.Unlock()
d.sessions[id] = map[string]any{
- "id": id, "status": status, "subscription": subID, "client_reference_id": ref,
+ "id": id, "status": status, "payment_status": paying, "subscription": subID, "client_reference_id": ref,
"customer_details": map[string]any{"email": email},
}
}
+// paying sets a session's payment_status.
+func (d *stripeDouble) paying(id, status string) {
+ d.mu.Lock()
+ defer d.mu.Unlock()
+ d.sessions[id].(map[string]any)["payment_status"] = status
+}
+
func (d *stripeDouble) lastCheckout(t *testing.T) url.Values {
t.Helper()
d.mu.Lock()
diff --git a/sync.go b/sync.go
index 1f7ac43..53e1bd7 100644
--- a/sync.go
+++ b/sync.go
@@ -137,10 +137,13 @@ func (s *Service) syncTx(ctx context.Context, tx *gorm.DB, sub stripeapi.Subscri
case old.Subject != "":
row.Subject = old.Subject
default:
- // Not tied to anyone yet. Holder is asked only if it never was: the
- // row was unrecognised and its price now belongs to a plan. An
- // unclaimed row was already refused by Holder and waits for Claim.
- if row.Subject, err = s.holder(ctx, tx, sub, hint, old.Plan == "" && plan != ""); err != nil {
+ // Not tied to anyone yet: found as a new row's holder is, the
+ // customer's mapping and then, on a known price, Holder for whatever
+ // address this sync has. The row may have been recorded before any
+ // address was known, or Holder may decide differently now; if it
+ // declines again the row stays unclaimed, with the address kept for
+ // Claim.
+ if row.Subject, err = s.holder(ctx, tx, sub, hint, plan != ""); err != nil {
return "", err
}
}
diff --git a/sync_test.go b/sync_test.go
index c3b3845..b96cf37 100644
--- a/sync_test.go
+++ b/sync_test.go
@@ -400,6 +400,105 @@ func TestFinishUnpaidIsNotPaid(t *testing.T) {
}
}
+// TestFinishCompleteButUnpaid: Checkout completes while a payment is still
+// processing (a bank debit). That is not paid, and Finish records nothing:
+// the webhook syncs once the payment lands. A session with nothing to pay
+// (a 100% code, a trial) is paid.
+func TestFinishCompleteButUnpaid(t *testing.T) {
+ now := t0
+ d := newStripeDouble(t)
+ rec := &recorder{subject: "42"}
+ s, db := d.service(t, &now, rec.config())
+ seedPlans(t, s)
+ ctx := context.Background()
+
+ d.put(t, subSpec{ID: "sub_1", Status: "active", Price: "price_A", Customer: "cus_1", Email: "ana@example.com", End: t0.Add(30 * day)})
+ d.session("cs_1", "complete", "sub_1", "", "ana@example.com")
+ d.paying("cs_1", "unpaid")
+ f, err := s.Finish(ctx, "cs_1")
+ if err != nil {
+ t.Fatalf("Finish: %v", err)
+ }
+ if f.Paid {
+ t.Errorf("Finish of a complete but unpaid session = %+v, want not paid", f)
+ }
+ if got := db.dump(rowsQuery); got != "" {
+ t.Errorf("an unpaid session made rows:\n%s", got)
+ }
+ if asked, _ := rec.take(); len(asked) != 0 {
+ t.Errorf("Holder asked %q for an unpaid session", asked)
+ }
+
+ d.paying("cs_1", "no_payment_required")
+ f, err = s.Finish(ctx, "cs_1")
+ if err != nil {
+ t.Fatalf("Finish with nothing to pay: %v", err)
+ }
+ if !f.Paid || f.Subject != "42" {
+ t.Errorf("Finish with nothing to pay = %+v, want paid by 42", f)
+ }
+ if got := db.dump("SELECT subject FROM abono_subscriptions"); got != "42\n" {
+ t.Errorf("rows after a no-payment session = %q, want one for 42", got)
+ }
+}
+
+// TestUnclaimedRowLearnsItsAddressLater: a reconcile records a granting
+// subscription whose customer has no address yet, so Holder cannot be
+// asked. Checkout's return then brings the address: Holder is asked, and
+// if it declines the address is kept for Claim.
+func TestUnclaimedRowLearnsItsAddressLater(t *testing.T) {
+ now := t0
+ d := newStripeDouble(t)
+ rec := &recorder{subject: ""}
+ s, db := d.service(t, &now, rec.config())
+ seedPlans(t, s)
+ ctx := context.Background()
+
+ sub := d.put(t, subSpec{ID: "sub_1", Status: "active", Price: "price_A", Customer: "cus_1", End: t0.Add(30 * day)})
+ if err := s.Sync(ctx, sub, abono.Hint{}); err != nil {
+ t.Fatalf("Sync with no address: %v", err)
+ }
+ if got, want := db.dump(rowsQuery), "sub_1|cus_1||annual|price_A|active|0|\n"; got != want {
+ t.Fatalf("rows =\n%s\nwant\n%s", got, want)
+ }
+ if asked, _ := rec.take(); len(asked) != 0 {
+ t.Fatalf("Holder asked %q with no address", asked)
+ }
+
+ // The address arrives; Holder declines: still unclaimed, address kept.
+ d.session("cs_1", "complete", "sub_1", "", "Ana@Example.com")
+ f, err := s.Finish(ctx, "cs_1")
+ if err != nil {
+ t.Fatalf("Finish: %v", err)
+ }
+ if f.Subject != "" || !f.Paid {
+ t.Errorf("Finish = %+v, want paid and unclaimed", f)
+ }
+ if got, want := db.dump(rowsQuery), "sub_1|cus_1||annual|price_A|active|0|ana@example.com\n"; got != want {
+ t.Errorf("rows =\n%s\nwant\n%s", got, want)
+ }
+ asked, changes := rec.take()
+ eq(t, "Holder asked", asked, []string{"ana@example.com"})
+ eq(t, "OnChange", changes, nil)
+
+ // Holder accepts on the next sync with the address: the row is theirs.
+ rec.mu.Lock()
+ rec.subject = "42"
+ rec.mu.Unlock()
+ if _, err := s.Finish(ctx, "cs_1"); err != nil {
+ t.Fatalf("Finish again: %v", err)
+ }
+ if got := db.dump("SELECT subject, email FROM abono_subscriptions"); got != "42|ana@example.com\n" {
+ t.Errorf("row = %q, want 42 with the address", got)
+ }
+ asked, changes = rec.take()
+ eq(t, "Holder asked", asked, []string{"ana@example.com"})
+ eq(t, "OnChange", changes, []string{"42"})
+ if !hasPlan(t, s, "42", "annual") {
+ t.Error("HasPlan(42, annual) is false")
+ }
+}
+
func TestUnclaimedThenClaim(t *testing.T) {
now := t0
d := newStripeDouble(t)