amadan
abono: AddPrice of a sold price reads it from Stripe and demotes the one it replaces
Summary
AddPrice(sold=true) now reads the price from Stripe first (and any of the plan's sold prices never read), records what Stripe said, and in the same transaction makes legacy every other sold price of the plan that bills the same way (interval and interval_count). A replacement yearly price no longer leaves two sold yearly prices that made Checkout refuse every yearly purchase. A sold AddPrice needs Stripe or the sandbox on; a legacy one does not.
Agent notes
Final review I6, ruling R38. With Stripe off, AddPrice(sold=true) returns the not-configured error, because the ruling's read cannot happen; TestOffersAndAddPrice now says so. TestCheckoutChoosesThePriceByInterval inserts its never-read sold prices directly, since AddPrice now reads them. Regression: TestAddPriceKeepsOneSoldPricePerInterval.
3 files changed,
+106
−14
checkout_test.go+4 −7offers.go+42 −2offers_test.go+60 −5
CI passed — run details
CI log
=== vet === go vet ./... === fmt === === test === go test ./... -count=1 ok amadan.net/rastrillo/abono 2.697s ok amadan.net/rastrillo/abono/abonotest 0.058s ok amadan.net/rastrillo/abono/cmd/abono 0.809s ok amadan.net/rastrillo/abono/internal/fakestripe 0.019s ok amadan.net/rastrillo/abono/internal/stripeapi 0.008s ok example 0.281s === race === CGO_ENABLED=1 go test ./... -race -count=1 ok amadan.net/rastrillo/abono 8.176s ok amadan.net/rastrillo/abono/abonotest 1.397s ok amadan.net/rastrillo/abono/cmd/abono 1.870s ok amadan.net/rastrillo/abono/internal/fakestripe 1.063s ok amadan.net/rastrillo/abono/internal/stripeapi 1.019s ok example 2.024s === browser === ok example 0.752s
Intent
diff --git a/checkout_test.go b/checkout_test.go| index 923bfe3..ff5cf3b 100644 |
| --- a/checkout_test.go |
| +++ b/checkout_test.go |
| @@ -107,11 +107,10 @@ func TestCheckoutChoosesThePriceByInterval(t *testing.T) { |
| return abono.CheckoutRequest{Plan: plan, Interval: interval, SuccessPath: "/done", CancelPath: "/"} |
| } |
| - // The plan sold yearly and monthly, neither interval known yet. |
| + // The plan sold yearly and monthly, neither interval known yet: a |
| + // second sold price recorded without a read, as a seed records one. |
| 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) |
| - } |
| + db.exec(`INSERT INTO abono_prices (stripe_price_id, plan, sold) VALUES ('price_A_month', 'annual', 1)`) |
| 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) |
| } |
| @@ -178,9 +177,7 @@ func TestCheckoutChoosesThePriceByInterval(t *testing.T) { |
| } |
| // 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) |
| - } |
| + db.exec(`INSERT INTO abono_prices (stripe_price_id, plan, sold) VALUES ('price_gone', 'annual', 1)`) |
| if _, err := s.Checkout(ctx, req("annual", "year")); err == nil { |
| t.Error("Checkout with a sold price Stripe cannot find succeeded") |
| } |
diff --git a/offers.go b/offers.go| index 11f7cfc..760d786 100644 |
| --- a/offers.go |
| +++ b/offers.go |
| @@ -9,6 +9,8 @@ import ( |
| "time" |
| "gorm.io/gorm" |
| + |
| + "amadan.net/rastrillo/abono/internal/stripeapi" |
| ) |
| // PlanSpec is one plan an app sells, as an environment variable or a seed |
| @@ -151,7 +153,8 @@ func (priceRow) TableName() string { return "abono_prices" } |
| // plan, listed or not, with sold=0. It is idempotent and it never deletes: |
| // a price dropped from the specs stays (no longer sold), because |
| // subscriptions already on it must still resolve to their plan. A price |
| -// added with AddPrice(sold) is demoted by the next seed of its plan. Seeding a plan again unarchives it, since the app just said it is |
| +// added with AddPrice(sold) is demoted by the next seed of its plan. |
| +// Seeding a plan again unarchives it, since the app just said it is |
| // for sale. A name an operator gave an offer, and what Stripe said of a |
| // price, are left alone. |
| func (s *Service) SeedPlans(ctx context.Context, specs []PlanSpec) error { |
| @@ -196,10 +199,33 @@ func upsertPrice(tx *gorm.DB, plan, priceID string, sold bool) error { |
| // AddPrice adds priceID to an existing offer, or changes whether it is |
| // sold. A price that belongs to another plan is refused: moving it would |
| // change what its subscriptions mean. |
| +// |
| +// A plan sells one price per interval, or Checkout cannot tell which to |
| +// sell (ruling R38). So a price added as sold is read from Stripe first, |
| +// which needs Stripe (or the sandbox) on, and any other sold price of the |
| +// plan that bills the same way (interval and interval_count) is made |
| +// legacy in the same transaction: adding a new yearly price replaces the |
| +// old yearly one and leaves a monthly one on sale. The plan's other sold |
| +// prices are read first too if they never have been. A price added as |
| +// legacy needs no read. |
| func (s *Service) AddPrice(ctx context.Context, plan, priceID string, sold bool) error { |
| if plan == "" || priceID == "" { |
| return errors.New("abono: AddPrice needs a plan and a price") |
| } |
| + var read stripeapi.Price |
| + if sold { |
| + if !s.On() { |
| + return errOff |
| + } |
| + // The others first, so the comparison below knows how each bills. |
| + if err := s.learnIntervals(ctx, plan); err != nil { |
| + return err |
| + } |
| + var err error |
| + if read, err = s.stripe.GetPrice(ctx, priceID); err != nil { |
| + return fmt.Errorf("abono: reading price %q: %w", priceID, err) |
| + } |
| + } |
| 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 { |
| @@ -215,7 +241,21 @@ func (s *Service) AddPrice(ctx context.Context, plan, priceID string, sold bool) |
| if len(have) == 1 && have[0].Plan != plan { |
| return fmt.Errorf("abono: price %q belongs to plan %q, not %q", priceID, have[0].Plan, plan) |
| } |
| - return upsertPrice(tx, plan, priceID, sold) |
| + if err := upsertPrice(tx, plan, priceID, sold); err != nil { |
| + return err |
| + } |
| + if !sold { |
| + return nil |
| + } |
| + count := max(read.Recurring.IntervalCount, 1) |
| + if err := tx.Exec(`UPDATE abono_prices SET interval = ?, interval_count = ?, amount = ?, currency = ?, refreshed_at = ? |
| + WHERE stripe_price_id = ?`, |
| + read.Recurring.Interval, read.Recurring.IntervalCount, read.UnitAmount, read.Currency, s.now(), priceID).Error; err != nil { |
| + return err |
| + } |
| + return tx.Exec(`UPDATE abono_prices SET sold = 0 |
| + WHERE plan = ? AND sold = 1 AND stripe_price_id <> ? AND interval = ? AND max(interval_count, 1) = ?`, |
| + plan, priceID, read.Recurring.Interval, count).Error |
| }) |
| } |
diff --git a/offers_test.go b/offers_test.go| index b10c544..0a275b2 100644 |
| --- a/offers_test.go |
| +++ b/offers_test.go |
| @@ -107,16 +107,18 @@ func TestOffersAndAddPrice(t *testing.T) { |
| t.Errorf("annual prices = %+v, want the sold price first", annual) |
| } |
| - if err := s.AddPrice(ctx, "monthly", "price_B2", true); err != nil { |
| - t.Fatalf("AddPrice: %v", err) |
| + // Selling a price reads it from Stripe first (ruling R38), so with |
| + // Stripe off only a legacy price can be added. |
| + if err := s.AddPrice(ctx, "monthly", "price_B2", true); err == nil { |
| + t.Fatal("AddPrice of a sold price succeeded with Stripe off") |
| } |
| if err := s.AddPrice(ctx, "monthly", "price_B2", false); err != nil { |
| - t.Fatalf("AddPrice again: %v", err) |
| + t.Fatalf("AddPrice: %v", err) |
| } |
| - if err := s.AddPrice(ctx, "nope", "price_X", true); err == nil { |
| + if err := s.AddPrice(ctx, "nope", "price_X", false); err == nil { |
| t.Error("AddPrice to an unknown plan succeeded") |
| } |
| - if err := s.AddPrice(ctx, "annual", "price_B", true); err == nil { |
| + if err := s.AddPrice(ctx, "annual", "price_B", false); err == nil { |
| t.Error("AddPrice moved a price that belongs to another plan") |
| } |
| offers, _ = s.Offers(ctx) |
| @@ -191,3 +193,56 @@ func TestAnUnsupportedPriceIsNeverSold(t *testing.T) { |
| t.Errorf("Offers price = %+v, want every 12 months and unsupported", p) |
| } |
| } |
| + |
| +// TestAddPriceKeepsOneSoldPricePerInterval is final review I6 (ruling |
| +// R38): adding a replacement sold price for a plan and interval demotes |
| +// the one it replaces, in the same transaction, so Checkout still finds |
| +// exactly one to sell. A sold price at another interval stays sold. |
| +func TestAddPriceKeepsOneSoldPricePerInterval(t *testing.T) { |
| + now := t0 |
| + d := newStripeDouble(t) |
| + s, db := d.service(t, &now, abono.Config{}) |
| + seedPlans(t, s) // annual: price_A (year), legacy price_old; monthly: price_M |
| + ctx := context.Background() |
| + sold := func() string { |
| + t.Helper() |
| + return db.dump(`SELECT stripe_price_id FROM abono_prices WHERE plan = 'annual' AND sold = 1 ORDER BY stripe_price_id`) |
| + } |
| + |
| + d.price("price_A2", "year", 7000, "eur") |
| + if err := s.AddPrice(ctx, "annual", "price_A2", true); err != nil { |
| + t.Fatalf("AddPrice: %v", err) |
| + } |
| + if got := sold(); got != "price_A2\n" { |
| + t.Errorf("sold annual prices = %q, want only the replacement", got) |
| + } |
| + if got := db.dump(`SELECT interval, interval_count, amount, currency, refreshed_at IS NOT NULL FROM abono_prices WHERE stripe_price_id = 'price_A2'`); got != "year|1|7000|eur|1\n" { |
| + t.Errorf("price_A2 as recorded = %q, want what Stripe said", got) |
| + } |
| + if _, err := s.Checkout(ctx, abono.CheckoutRequest{Plan: "annual", Interval: "year", SuccessPath: "/done"}); err != nil { |
| + t.Fatalf("Checkout at the replaced interval: %v", err) |
| + } |
| + if got := d.lastCheckout(t).Get("line_items[0][price]"); got != "price_A2" { |
| + t.Errorf("Checkout sold %q, want price_A2", got) |
| + } |
| + |
| + // Another interval is another sold price, not a replacement. |
| + d.price("price_A_month", "month", 700, "eur") |
| + if err := s.AddPrice(ctx, "annual", "price_A_month", true); err != nil { |
| + t.Fatalf("AddPrice monthly: %v", err) |
| + } |
| + if got := sold(); got != "price_A2\nprice_A_month\n" { |
| + t.Errorf("sold annual prices = %q, want the yearly and the monthly", got) |
| + } |
| + |
| + // A price Stripe does not have changes nothing. |
| + if err := s.AddPrice(ctx, "annual", "price_nowhere", true); err == nil { |
| + t.Error("AddPrice of a price Stripe does not have succeeded") |
| + } |
| + if got := sold(); got != "price_A2\nprice_A_month\n" { |
| + t.Errorf("after a failed AddPrice, sold annual prices = %q", got) |
| + } |
| + if got := db.dump(`SELECT count(*) FROM abono_prices WHERE stripe_price_id = 'price_nowhere'`); got != "0\n" { |
| + t.Error("a price Stripe does not have was recorded") |
| + } |
| +} |