From 35b312d5ef438f9d6edb9fc3af641ca4c4696e75 Mon Sep 17 00:00:00 2001 From: nexxo Date: Wed, 29 Jul 2026 19:35:15 +0200 Subject: [PATCH] Invoice every renewal, and tell Stripe when the package changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things Stripe was doing on its own, without us. **A renewal produced no invoice and no mail.** InvoiceMail went out from exactly one place — the first purchase — so a customer paying every month for a year received one invoice, for month one. Stripe already says when the money lands: invoice.paid with billing_reason subscription_cycle. What was missing was the document. A renewal has no Order behind it, and one was not invented for the occasion: an Order is something a customer bought, and writing a purchase nobody made would corrupt the record of what they have actually ordered. IssueInvoice::forBilledPeriod() issues from the CONTRACT instead, at its frozen net price, for the term Stripe billed. Idempotent against the Stripe invoice id, which is unique in the database rather than checked in PHP — and because the number is drawn inside the same transaction as the row, a redelivery takes its number back with it and the series keeps its sequence. Nothing in the paperwork can fail the webhook: issuing and mailing are both caught and logged, exactly as confirmByMail() does. Only a cycle renewal gets a document. The checkout's own invoice already has one; an upgrade's proration was worked out against Stripe's boundaries and would not match a document written from our snapshot; a charge raised by hand is for something this catalogue cannot describe. All three stay in the register, and are logged once as money no document of ours covers. **After a plan change, Stripe went on billing the old price.** There was no way to move a subscription onto another price at all, so a customer who upgraded paid for the smaller package every month afterwards. The client can now swap one, the item id it needs is learnt from Stripe's own events (and asked for once, for contracts older than the column), and the behaviour is chosen per direction: an upgrade settles immediately, so the cycle invoice stays exactly the contract price the renewal document states; a downgrade lands at the period boundary and settles nothing. A granted contract has no Stripe subscription and is left untouched. A change that reaches the machine and not Stripe is never rolled back — it is parked on the contract with the error and the behaviour it needed, logged as an error, and retried hourly by clupilot:sync-stripe-subscriptions. Co-Authored-By: Claude Opus 5 --- app/Actions/ApplyPlanChange.php | 24 +- app/Actions/ApplyStripeBillingEvent.php | 122 ++++++++- app/Actions/IssueRenewalInvoice.php | 127 +++++++++ app/Actions/MoveStripeSubscriptionPrice.php | 218 ++++++++++++++++ .../Commands/SyncStripeSubscriptions.php | 70 +++++ app/Models/Invoice.php | 17 +- app/Models/Subscription.php | 5 + app/Services/Billing/IssueInvoice.php | 127 ++++++++- app/Services/Stripe/FakeStripeClient.php | 82 ++++++ app/Services/Stripe/HttpStripeClient.php | 68 ++++- app/Services/Stripe/StripeClient.php | 67 +++++ ..._link_invoices_to_the_period_they_bill.php | 43 +++ ...e_stripe_price_a_contract_is_billed_at.php | 51 ++++ lang/de/invoice.php | 1 + lang/en/invoice.php | 1 + routes/console.php | 12 + tests/Feature/Billing/RenewalInvoiceTest.php | 233 +++++++++++++++++ .../Feature/Billing/StripePlanChangeTest.php | 245 ++++++++++++++++++ 18 files changed, 1486 insertions(+), 27 deletions(-) create mode 100644 app/Actions/IssueRenewalInvoice.php create mode 100644 app/Actions/MoveStripeSubscriptionPrice.php create mode 100644 app/Console/Commands/SyncStripeSubscriptions.php create mode 100644 database/migrations/2026_07_29_270000_link_invoices_to_the_period_they_bill.php create mode 100644 database/migrations/2026_07_29_280000_track_the_stripe_price_a_contract_is_billed_at.php create mode 100644 tests/Feature/Billing/RenewalInvoiceTest.php create mode 100644 tests/Feature/Billing/StripePlanChangeTest.php diff --git a/app/Actions/ApplyPlanChange.php b/app/Actions/ApplyPlanChange.php index ce5268e..c3af6e4 100644 --- a/app/Actions/ApplyPlanChange.php +++ b/app/Actions/ApplyPlanChange.php @@ -10,6 +10,7 @@ use App\Models\SubscriptionRecord; use App\Provisioning\Jobs\AdvanceRunJob; use App\Services\Billing\CustomDomainAccess; use App\Services\Billing\PlanChange; +use App\Services\Stripe\StripeClient; use Illuminate\Support\Carbon; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Log; @@ -35,7 +36,10 @@ use Throwable; * own domain, which a smaller package can take away; * - the MACHINE is resized, through a provisioning run like every other piece * of remote work in this application, so it is retried, logged and visible in - * the console instead of happening in a web request. + * the console instead of happening in a web request; + * - STRIPE is moved onto the new price, because it owns the billing cycle and + * would otherwise keep charging for the package the customer has left. Last, + * outside the transaction, and unable to fail the change — see below. * * Idempotent from both ends. A contract already on the target version is a * no-op rather than an error — the order is consumed and nothing else moves — @@ -172,6 +176,24 @@ class ApplyPlanChange // there would be two runs writing one router. PlanChange::settleCustomDomain($subscription); + // Stripe bills the next term, so it has to be told which package that + // is — without this a customer who upgraded goes on paying for the one + // they left, every month, for as long as the contract runs. + // + // After the transaction and never inside it. The change has already + // landed on the contract and on the machine, and an unreachable or + // unconfigured Stripe must not roll any of that back: the action never + // throws, and parks what it could not do on the contract for the hourly + // sweep to pick up. A contract with no Stripe subscription behind it — + // a granted package — is left entirely alone. + // + // Resolved from the container here rather than injected, so a test that + // swaps the client in after this action exists still gets its fake. + app(MoveStripeSubscriptionPrice::class)( + $subscription, + $change->isUpgrade ? StripeClient::PRORATE_IMMEDIATELY : StripeClient::PRORATE_NONE, + ); + if ($run !== null) { AdvanceRunJob::dispatch($run->uuid); // after commit diff --git a/app/Actions/ApplyStripeBillingEvent.php b/app/Actions/ApplyStripeBillingEvent.php index 1831fc0..0ad839e 100644 --- a/app/Actions/ApplyStripeBillingEvent.php +++ b/app/Actions/ApplyStripeBillingEvent.php @@ -9,16 +9,23 @@ use Illuminate\Database\UniqueConstraintViolationException; use Illuminate\Support\Carbon; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Log; +use Throwable; /** * What Stripe tells us about the billing cycle, applied to the contract. * * The division of labour: Stripe owns the money — retries, dunning, off-session - * SCA, invoice numbering — and we own capability, because Stripe does not know - * how big the VM should be. So these handlers move period boundaries and status, - * and write the register. They never re-derive an amount: the invoice is the - * authority for what was charged, and recomputing it here from our own - * catalogue would produce a second, disagreeing answer. + * SCA — and we own capability, because Stripe does not know how big the VM + * should be. So these handlers move period boundaries and status, and write the + * register. They never re-derive an amount: the invoice is the authority for + * what was charged, and recomputing it here from our own catalogue would produce + * a second, disagreeing answer. + * + * The one thing Stripe does NOT own is the document. Their invoice numbering is + * theirs and is not a lawful Austrian invoice series; ours is, and it is gapless + * because it has to be. So a renewal Stripe reports as paid produces a document + * of our own and one mail carrying it — see App\Actions\IssueRenewalInvoice, and + * invoicePaid() below for which paid invoices get one and which do not. * * Every handler is idempotent. Stripe retries a webhook until it gets a 2xx, * and it also sends the same event to several endpoints — so "already applied" @@ -40,7 +47,10 @@ class ApplyStripeBillingEvent private const RANK_FAILED = 1; - public function __construct(private RecordCommercialEvent $record) {} + public function __construct( + private RecordCommercialEvent $record, + private IssueRenewalInvoice $issueRenewalInvoice, + ) {} /** * A renewal was paid. Move the period on and enter it in the register. @@ -99,7 +109,7 @@ class ApplyStripeBillingEvent // and both deliveries would pass a check. $event = $isRenewal ? SubscriptionRecord::EVENT_RENEWAL : SubscriptionRecord::EVENT_INVOICE_PAID; - return $this->recordOnce( + $record = $this->recordOnce( fn () => ($this->record)( event: $event, subscription: $subscription->refresh(), @@ -117,6 +127,70 @@ class ApplyStripeBillingEvent eventKey: $invoiceId !== '' ? "{$event}:{$invoiceId}" : null, ) ); + + if ($isRenewal && $start !== null && $end !== null) { + $this->invoiceRenewal($subscription, $invoiceId, $start, $end); + } elseif ($record !== null) { + // Money that gets no document from us, said out loud once — when it + // is first recorded, not on every redelivery. + // + // Which paid invoices those are, and why none of them is invoiced + // from here: + // + // - `subscription_create` is the checkout's own invoice. The + // purchase already has a document, issued the moment the money + // landed; a second one would put two invoices on one sale. (It + // never reaches this line — the handler returns above.) + // - `subscription_update` is an upgrade's proration. Stripe worked + // the amount out against ITS period boundaries and its own view + // of what is already on the account, and a document written from + // our contract snapshot would state a sum that is not the one + // charged. The move itself is in the register as an `upgrade`, + // carrying the pro-rata the customer was quoted. + // - a charge raised by hand in Stripe's dashboard is for something + // this catalogue knows nothing about. We cannot describe what was + // sold, and an invoice line that is a guess is worse than none. + // + // All three are in the register with the amount, which is what the + // console's payments panel reads. This is the trail for whoever has + // to raise the paper. + Log::warning('A payment was received that no document of ours covers.', [ + 'subscription' => $subscription->uuid, + 'stripe_invoice' => $invoiceId ?: null, + 'billing_reason' => $reason ?: null, + ]); + } + + return $record; + } + + /** + * The renewal's document and the one mail that carries it. + * + * Outside recordOnce() on purpose. The register entry and the invoice are + * two different obligations, and tying the second to the first succeeding + * would mean that a delivery which recorded the payment but could not issue + * the document — an incomplete company profile, a mail server that was down + * — could never be finished: every retry would find the register entry + * already there, return, and the customer would never get an invoice. The + * issuing is idempotent on its own, keyed on the Stripe invoice id. + * + * Caught here as well as inside the action. The action guards what it does; + * this guards the rest, because the rule is that no paperwork of ours may + * ever cost Stripe a 2xx — a retried webhook is a second attempt at + * everything, and one of those things moves the customer's term. + */ + private function invoiceRenewal(Subscription $subscription, string $invoiceId, Carbon $start, Carbon $end): void + { + try { + ($this->issueRenewalInvoice)($subscription->refresh(), $invoiceId, $start, $end); + } catch (Throwable $e) { + Log::error('Failed to invoice a renewal that was paid.', [ + 'subscription' => $subscription->uuid, + 'stripe_invoice' => $invoiceId ?: null, + 'error' => $e->getMessage(), + ]); + } } /** @@ -178,6 +252,8 @@ class ApplyStripeBillingEvent [$start, $end] = $this->period($object); + $this->rememberItem($subscription, $object); + // Ended is ended, whatever order events arrive in — and an older // snapshot must never overwrite a newer one it was delivered behind. $this->mutateInOrder($subscription, $eventAt, self::RANK_UPDATED, fn () => array_filter([ @@ -189,6 +265,38 @@ class ApplyStripeBillingEvent return $subscription; } + /** + * Keep the id of the item this subscription bills through. + * + * Stripe names it only on the subscription object, and moving a contract + * onto another price is impossible without it — the price hangs off the + * item, not off the subscription. Learnt here because this event arrives on + * its own for every status change and every renewal, so the ordinary running + * of a contract fills it in without anyone having to ask Stripe. + * + * Written outside the ordering guard on purpose: this is not part of the + * running picture. An item id does not go stale, does not change when the + * price on it does, and a stale event carries exactly the same one. + * + * @param array $object + */ + private function rememberItem(Subscription $subscription, array $object): void + { + if ($subscription->stripe_item_id !== null) { + return; + } + + $itemId = $object['items']['data'][0]['id'] ?? null; + + // One item, because one contract bills one package. A subscription with + // several would need a rule for which of them the package is, and there + // is nothing here to build one from — better to have none than to swap + // the price on whichever happened to be first. + if (is_string($itemId) && $itemId !== '' && count($object['items']['data'] ?? []) === 1) { + $subscription->update(['stripe_item_id' => $itemId]); + } + } + /** * The subscription has ended at Stripe. That is the end of the contract, * so it goes in the register — a cancellation nobody recorded is exactly diff --git a/app/Actions/IssueRenewalInvoice.php b/app/Actions/IssueRenewalInvoice.php new file mode 100644 index 0000000..978f02a --- /dev/null +++ b/app/Actions/IssueRenewalInvoice.php @@ -0,0 +1,127 @@ + $subscription->uuid, + ]); + + return null; + } + + $invoice = $this->issue($subscription, $stripeInvoiceId, $from, $to); + + if ($invoice === null || $invoice->sent_at !== null) { + return $invoice; + } + + $this->send($subscription, $invoice); + + return $invoice; + } + + private function issue(Subscription $subscription, string $stripeInvoiceId, Carbon $from, Carbon $to): ?Invoice + { + $existing = Invoice::query()->where('stripe_invoice_id', $stripeInvoiceId)->first(); + + if ($existing !== null) { + return $existing; + } + + try { + return app(IssueInvoice::class)->forBilledPeriod($subscription, $from, $to, $stripeInvoiceId); + } catch (UniqueConstraintViolationException) { + // A concurrent delivery won the race. Its document is the one to + // send; this one's invoice number went back into the series when the + // transaction rolled back. + return Invoice::query()->where('stripe_invoice_id', $stripeInvoiceId)->first(); + } catch (Throwable $e) { + // Refuses while the company details are incomplete, which is the + // correct outcome — see IssueInvoice. The renewal itself stands: the + // term has moved and the payment is in the register, and the + // document can be issued once the details are there. + Log::warning('Could not issue an invoice for this renewal.', [ + 'subscription' => $subscription->uuid, + 'stripe_invoice' => $stripeInvoiceId, + 'exception' => $e->getMessage(), + ]); + + return null; + } + } + + private function send(Subscription $subscription, Invoice $invoice): void + { + $customer = $subscription->customer; + $address = $customer?->email; + + if (! is_string($address) || $address === '') { + return; + } + + try { + Mail::to($address)->queue(new InvoiceMail($invoice, (string) $customer->name)); + + // Only once it is actually with the mailer. Stamped so a redelivery + // does not send the same invoice a second time, and left unstamped + // on a failure so that one still can. + $invoice->update(['sent_at' => now()]); + } catch (Throwable $e) { + Log::warning('Could not queue the invoice mail for this renewal.', [ + 'invoice' => $invoice->number, + 'exception' => $e->getMessage(), + ]); + } + } +} diff --git a/app/Actions/MoveStripeSubscriptionPrice.php b/app/Actions/MoveStripeSubscriptionPrice.php new file mode 100644 index 0000000..dc99d97 --- /dev/null +++ b/app/Actions/MoveStripeSubscriptionPrice.php @@ -0,0 +1,218 @@ +stripe_subscription_id === null) { + return true; + } + + $priceId = null; + + try { + $priceId = $this->targetPrice($subscription); + + if ($priceId === null) { + throw new RuntimeException( + 'The catalogue has no Stripe price for this plan version on this term. Run stripe:sync-catalogue.' + ); + } + + // Already billing it. A retry of a swap that in fact went through, + // or a second change back onto the price Stripe is on: either way + // asking again would raise a proration for a move that is not one. + if ($subscription->stripe_price_id === $priceId) { + $this->settled($subscription, $priceId); + + return true; + } + + if (! $this->stripe->isConfigured()) { + throw new RuntimeException('Stripe is not configured (STRIPE_SECRET is empty).'); + } + + $itemId = $this->itemId($subscription); + + if ($itemId === null) { + throw new RuntimeException( + 'Stripe reports no item on this subscription, so there is nothing to put another price on.' + ); + } + + $this->stripe->updateSubscriptionPrice( + $subscription->stripe_subscription_id, + $itemId, + $priceId, + $behaviour, + ); + + $this->settled($subscription, $priceId); + + return true; + } catch (Throwable $e) { + $this->park($subscription, $priceId, $behaviour, $e); + + return false; + } + } + + /** + * Try a parked swap again. + * + * The target price is worked out afresh from the contract rather than read + * back from the parked record: another change may have landed in between, + * and the contract is the authority on which package the customer is on. + * The BEHAVIOUR is the one that was chosen at the time, because it belongs + * to the direction the move went, which cannot be read off the contract + * once it has moved. + * + * A retried upgrade prorates from the moment Stripe finally hears about it, + * not from the moment the change landed here — so a long outage charges the + * customer for slightly fewer days than they had the bigger package. In + * their favour, and preferable to the alternative of back-dating a charge. + */ + public function retry(Subscription $subscription): bool + { + $parked = (array) ($subscription->stripe_price_sync ?? []); + + return $this($subscription, (string) ($parked['behaviour'] ?? StripeClient::PRORATE_NONE)); + } + + /** + * The Stripe Price for the package the contract is on now. + * + * By plan VERSION and term, which is what a Stripe Price is: one per priced + * row, written by stripe:sync-catalogue. Resolving it by plan name would + * hand back today's terms for a contract that is grandfathered on older + * ones. + */ + private function targetPrice(Subscription $subscription): ?string + { + $priceId = PlanPrice::query() + ->where('plan_version_id', $subscription->plan_version_id) + ->where('term', $subscription->term) + ->value('stripe_price_id'); + + return is_string($priceId) && $priceId !== '' ? $priceId : null; + } + + /** + * The item the subscription bills through. + * + * Stored when we have it — a `customer.subscription.updated` event carries + * it and ApplyStripeBillingEvent keeps it. A contract older than that column + * has none, so Stripe is asked once and the answer is kept: the item id + * survives a price swap, so this happens at most once per contract. + */ + private function itemId(Subscription $subscription): ?string + { + if ($subscription->stripe_item_id !== null) { + return $subscription->stripe_item_id; + } + + $itemId = $this->stripe->subscriptionItemId((string) $subscription->stripe_subscription_id); + + if ($itemId !== null) { + $subscription->update(['stripe_item_id' => $itemId]); + } + + return $itemId; + } + + /** Stripe and the contract agree; nothing is outstanding. */ + private function settled(Subscription $subscription, string $priceId): void + { + $subscription->update([ + 'stripe_price_id' => $priceId, + 'stripe_price_synced_at' => now(), + 'stripe_price_sync' => null, + ]); + } + + /** + * The change landed here and did not reach Stripe. + * + * Recorded on the contract rather than only logged: a log line scrolls away, + * and what this describes is a customer being charged for a package they are + * no longer on. The row is what the retry sweep reads and what an operator + * can be shown. + */ + private function park(Subscription $subscription, ?string $priceId, string $behaviour, Throwable $e): void + { + $parked = (array) ($subscription->stripe_price_sync ?? []); + $attempts = (int) ($parked['attempts'] ?? 0) + 1; + + Log::error('A plan change landed but did not reach Stripe: this contract is still being billed at the old price.', [ + 'subscription' => $subscription->uuid, + 'plan' => $subscription->plan, + 'stripe_subscription' => $subscription->stripe_subscription_id, + 'stripe_price' => $priceId, + 'attempts' => $attempts, + 'error' => $e->getMessage(), + ]); + + $subscription->update(['stripe_price_sync' => [ + 'price' => $priceId, + 'behaviour' => $behaviour, + 'error' => mb_substr($e->getMessage(), 0, 250), + 'failed_at' => now()->toIso8601String(), + 'attempts' => $attempts, + ]]); + } +} diff --git a/app/Console/Commands/SyncStripeSubscriptions.php b/app/Console/Commands/SyncStripeSubscriptions.php new file mode 100644 index 0000000..af958f3 --- /dev/null +++ b/app/Console/Commands/SyncStripeSubscriptions.php @@ -0,0 +1,70 @@ +whereNotNull('stripe_price_sync') + ->whereNotNull('stripe_subscription_id') + ->where('status', 'active') + ->orderBy('id') + ->limit((int) $this->option('limit')) + ->get(); + + $moved = 0; + + foreach ($parked as $subscription) { + if ($move->retry($subscription)) { + $moved++; + + continue; + } + + // The action has already logged why and counted the attempt. Said + // again here because somebody is watching this command's output + // and not the log. + $this->warn(sprintf( + 'Contract %s: still billed at the old price — %s', + $subscription->uuid, + (string) ($subscription->fresh()?->stripe_price_sync['error'] ?? 'unknown'), + )); + } + + $this->info("{$parked->count()} contract(s) out of step with Stripe, {$moved} moved."); + + return self::SUCCESS; + } +} diff --git a/app/Models/Invoice.php b/app/Models/Invoice.php index 34c15ca..bf4ee62 100644 --- a/app/Models/Invoice.php +++ b/app/Models/Invoice.php @@ -5,6 +5,7 @@ namespace App\Models; use App\Models\Concerns\HasUuid; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\BelongsTo; +use Illuminate\Database\Eloquent\Relations\HasMany; /** * An issued invoice, frozen. @@ -20,7 +21,8 @@ class Invoice extends Model use HasUuid; protected $fillable = [ - 'invoice_series_id', 'customer_id', 'order_id', 'number', 'number_year', 'number_sequence', + 'invoice_series_id', 'customer_id', 'order_id', 'subscription_id', 'stripe_invoice_id', + 'number', 'number_year', 'number_sequence', 'issued_on', 'due_on', 'snapshot', 'net_cents', 'tax_cents', 'gross_cents', 'currency', 'cancels_invoice_id', 'sent_at', ]; @@ -53,8 +55,19 @@ class Invoice extends Model return $this->belongsTo(Order::class); } + /** + * The contract this document billed a term of. + * + * Set on a renewal, which has no order behind it — nobody buys a month + * rolling over. Null on a purchase, where `order` is what it is for. + */ + public function subscription(): BelongsTo + { + return $this->belongsTo(Subscription::class); + } + /** Where this document has been copied to, and where it has not. */ - public function exports(): \Illuminate\Database\Eloquent\Relations\HasMany + public function exports(): HasMany { return $this->hasMany(InvoiceExport::class); } diff --git a/app/Models/Subscription.php b/app/Models/Subscription.php index c039064..e907f97 100644 --- a/app/Models/Subscription.php +++ b/app/Models/Subscription.php @@ -63,6 +63,11 @@ class Subscription extends Model 'pending_effective_at' => 'datetime', 'cancelled_at' => 'datetime', 'stripe_event_at' => 'datetime', + 'stripe_price_synced_at' => 'datetime', + // The swap that has not reached Stripe: the price it was meant to + // move to, the proration behaviour its direction chose, the error + // and how often it has been tried. Null once Stripe agrees with us. + 'stripe_price_sync' => 'array', 'granted_at' => 'datetime', 'granted_until' => 'datetime', 'catalogue_price_cents' => 'integer', diff --git a/app/Services/Billing/IssueInvoice.php b/app/Services/Billing/IssueInvoice.php index 254ef43..575106d 100644 --- a/app/Services/Billing/IssueInvoice.php +++ b/app/Services/Billing/IssueInvoice.php @@ -8,7 +8,9 @@ use App\Models\ExportTarget; use App\Models\Invoice; use App\Models\InvoiceSeries; use App\Models\Order; +use App\Models\Subscription; use App\Support\CompanyProfile; +use Illuminate\Support\Carbon; use Illuminate\Support\Collection; use Illuminate\Support\Facades\DB; use RuntimeException; @@ -53,6 +55,112 @@ final class IssueInvoice throw new RuntimeException('Refusing to issue an invoice with no lines on it.'); } + $lines = $orders->map(fn (Order $order) => [ + 'description' => $order->label(), + 'details' => array_values(array_filter([ + $order->isRecurring() ? __('invoice.line_recurring') : __('invoice.line_once'), + ])), + 'quantity_milli' => 1000, + 'unit' => '', + 'unit_net_cents' => (int) $order->amount_cents, + ])->all(); + + return $this->issue( + customer: $customer, + lines: $lines, + currency: strtoupper((string) ($orders->first()->currency ?: 'EUR')), + series: $series, + attributes: ['order_id' => $orders->first()->id], + ); + } + + /** + * One invoice for a term that was billed — a renewal. + * + * Deliberately NOT forOrders() with an Order made up for the occasion. An + * Order is something a customer bought: it is created at a checkout, it sits + * in their cart until it is paid, and provisioning and plan changes consume + * it. A month rolling over is none of those things, and writing a purchase + * nobody made would corrupt the one record that says what this customer has + * actually ordered. The document points at the CONTRACT instead, which is + * what a renewal renews. + * + * The line is the contract's own frozen price — `price_cents`, the + * catalogue's NET figure — and not Stripe's `amount_paid`, which is a gross + * total. What the customer is owed is what they agreed to; the register + * records beside it what was actually taken, and `stripe_invoice_id` on the + * row is how the two are reconciled afterwards. + * + * Booked modules are NOT on it. They are not items on the Stripe + * subscription, so this renewal did not charge for them — putting them on + * the document would invoice money nobody was asked for. (That they are + * never billed after the month they were booked in is a real gap, and it is + * a gap in the booking, not in this document.) + * + * `$stripeInvoiceId` is what makes issuing this idempotent: the column is + * unique, so a redelivered webhook collides in the database and the + * transaction takes its invoice number back with it. + */ + public function forBilledPeriod( + Subscription $subscription, + Carbon $from, + Carbon $to, + ?string $stripeInvoiceId = null, + ?InvoiceSeries $series = null, + ): Invoice { + $customer = $subscription->customer; + + if ($customer === null) { + throw new RuntimeException('Refusing to issue an invoice for a contract with nobody on it.'); + } + + $lines = [[ + 'description' => __('billing.cart.plan', ['plan' => ucfirst((string) $subscription->plan)]), + 'details' => [__('invoice.line_period', [ + 'from' => $from->local()->format('d.m.Y'), + // Stripe's period_end is the EXCLUSIVE boundary — the first + // moment of the next term, which the next invoice will claim in + // turn. Printed as it comes, the same day would appear on two + // consecutive documents. + 'to' => $to->greaterThan($from) + ? $to->copy()->subDay()->local()->format('d.m.Y') + : $to->local()->format('d.m.Y'), + ])], + 'quantity_milli' => 1000, + 'unit' => '', + 'unit_net_cents' => (int) $subscription->price_cents, + ]]; + + return $this->issue( + customer: $customer, + lines: $lines, + currency: strtoupper((string) ($subscription->currency ?: 'EUR')), + series: $series, + attributes: [ + 'subscription_id' => $subscription->id, + // Left null on purpose. The contract's opening order is the + // FIRST purchase and already has its own invoice; pointing this + // one at it would file two documents under one sale. + 'order_id' => null, + 'stripe_invoice_id' => $stripeInvoiceId, + ], + ); + } + + /** + * The part every document has in common: the number, the freeze, the copy + * to the archive. + * + * @param array> $lines + * @param array $attributes what this document is FOR + */ + private function issue( + Customer $customer, + array $lines, + string $currency, + ?InvoiceSeries $series, + array $attributes, + ): Invoice { $missing = CompanyProfile::missingForInvoicing(); if ($missing !== []) { @@ -69,16 +177,6 @@ final class IssueInvoice $treatment = TaxTreatment::for($customer); $rate = (int) round($treatment->rate * 10000); - $lines = $orders->map(fn (Order $order) => [ - 'description' => $order->label(), - 'details' => array_values(array_filter([ - $order->isRecurring() ? __('invoice.line_recurring') : __('invoice.line_once'), - ])), - 'quantity_milli' => 1000, - 'unit' => '', - 'unit_net_cents' => (int) $order->amount_cents, - ])->all(); - $totals = InvoiceMath::totals($lines, null, $rate) + ['rate_basis_points' => $rate]; $snapshot = [ @@ -89,17 +187,19 @@ final class IssueInvoice 'meta' => $this->meta($customer, $treatment), ]; - $invoice = DB::transaction(function () use ($series, $customer, $orders, $snapshot, $totals) { + $invoice = DB::transaction(function () use ($series, $customer, $snapshot, $totals, $currency, $attributes) { [$number, $sequence, $year] = $this->numbers->next($series); // The number is only true once it is on the document, so both are // written inside the same transaction the counter was advanced in. + // That is also what keeps a redelivered Stripe invoice from eating a + // number: the unique index rejects the row, the transaction rolls + // back, and the counter comes back with it. $snapshot['meta']['number'] = $number; return Invoice::create([ 'invoice_series_id' => $series->id, 'customer_id' => $customer->id, - 'order_id' => $orders->first()->id, 'number' => $number, 'number_year' => $year, 'number_sequence' => $sequence, @@ -109,7 +209,8 @@ final class IssueInvoice 'net_cents' => $totals['net'], 'tax_cents' => $totals['tax'], 'gross_cents' => $totals['gross'], - 'currency' => strtoupper((string) ($orders->first()->currency ?: 'EUR')), + 'currency' => $currency, + ...$attributes, ]); }); diff --git a/app/Services/Stripe/FakeStripeClient.php b/app/Services/Stripe/FakeStripeClient.php index 664f2b8..b2496c1 100644 --- a/app/Services/Stripe/FakeStripeClient.php +++ b/app/Services/Stripe/FakeStripeClient.php @@ -2,6 +2,8 @@ namespace App\Services\Stripe; +use RuntimeException; + /** * A Stripe that records what it was asked to do. * @@ -26,11 +28,61 @@ class FakeStripeClient implements StripeClient /** Idempotency key → the id first returned for it. @var array */ public array $keys = []; + /** + * What Stripe would answer when asked which item a subscription bills + * through: subscription id → item id. + * + * @var array + */ + public array $items = []; + + /** + * Every price swap that was asked for, in order, so a test can assert the + * proration behaviour as well as the price. + * + * @var array + */ + public array $priceChanges = []; + + /** + * Set to make every call fail, the way an outage, a revoked key or a + * network without a route out does. + */ + public ?string $failWith = null; + + /** + * Every checkout that was opened, in order. + * + * @var array + */ + public array $checkouts = []; + public function isConfigured(): bool { return $this->configured; } + public function createCheckoutSession( + string $priceId, + string $successUrl, + string $cancelUrl, + array $metadata = [], + ?string $customerEmail = null, + ?string $idempotencyKey = null, + ): string { + $this->failIfAsked(); + + $this->checkouts[] = [ + 'price' => $priceId, + 'success' => $successUrl, + 'cancel' => $cancelUrl, + 'metadata' => $metadata, + 'email' => $customerEmail, + ]; + + return 'https://checkout.stripe.test/session/'.count($this->checkouts); + } + public function createProduct(string $name, array $metadata = [], ?string $idempotencyKey = null): string { // Replays the first answer for a repeated key, as Stripe does. @@ -80,4 +132,34 @@ class FakeStripeClient implements StripeClient { $this->archived[] = $priceId; } + + public function updateSubscriptionPrice( + string $subscriptionId, + string $itemId, + string $priceId, + string $prorationBehaviour, + ): void { + $this->failIfAsked(); + + $this->priceChanges[] = [ + 'subscription' => $subscriptionId, + 'item' => $itemId, + 'price' => $priceId, + 'proration' => $prorationBehaviour, + ]; + } + + public function subscriptionItemId(string $subscriptionId): ?string + { + $this->failIfAsked(); + + return $this->items[$subscriptionId] ?? null; + } + + private function failIfAsked(): void + { + if ($this->failWith !== null) { + throw new RuntimeException($this->failWith); + } + } } diff --git a/app/Services/Stripe/HttpStripeClient.php b/app/Services/Stripe/HttpStripeClient.php index 3430593..4a95f96 100644 --- a/app/Services/Stripe/HttpStripeClient.php +++ b/app/Services/Stripe/HttpStripeClient.php @@ -2,14 +2,15 @@ namespace App\Services\Stripe; +use App\Services\Secrets\SecretVault; use Illuminate\Http\Client\PendingRequest; use Illuminate\Support\Facades\Http; use RuntimeException; /** - * Stripe's REST API over the HTTP client, rather than the SDK: this touches - * three endpoints, and a fake for the tests is easier to trust than a mocked - * library. Not unit-tested against the real service (live I/O). + * Stripe's REST API over the HTTP client, rather than the SDK: this touches a + * handful of endpoints, and a fake for the tests is easier to trust than a + * mocked library. Not unit-tested against the real service (live I/O). */ class HttpStripeClient implements StripeClient { @@ -18,6 +19,36 @@ class HttpStripeClient implements StripeClient return filled($this->secret()); } + public function createCheckoutSession( + string $priceId, + string $successUrl, + string $cancelUrl, + array $metadata = [], + ?string $customerEmail = null, + ?string $idempotencyKey = null, + ): string { + return (string) $this->request($idempotencyKey) + ->asForm() + ->post($this->url('checkout/sessions'), array_filter([ + 'mode' => 'subscription', + 'line_items[0][price]' => $priceId, + 'line_items[0][quantity]' => 1, + 'success_url' => $successUrl, + 'cancel_url' => $cancelUrl, + // Prefilled, not fixed: Stripe still lets the customer correct + // it, and whatever they confirm is what customer_details.email + // carries back — which is the address the credentials go to. + 'customer_email' => $customerEmail, + ...$this->flatten('metadata', $metadata), + // The same values on the subscription, because a renewal or a + // payment failure months from now arrives with the subscription + // and not with the session that opened it. + ...$this->flatten('subscription_data[metadata]', $metadata), + ], fn ($value) => $value !== null && $value !== '')) + ->throw() + ->json('url'); + } + public function createProduct(string $name, array $metadata = [], ?string $idempotencyKey = null): string { return (string) $this->request($idempotencyKey) @@ -65,6 +96,35 @@ class HttpStripeClient implements StripeClient ->throw(); } + public function updateSubscriptionPrice( + string $subscriptionId, + string $itemId, + string $priceId, + string $prorationBehaviour, + ): void { + // The item is named as well as the price. Without its id Stripe adds a + // SECOND item rather than replacing the first, and the customer is then + // billed for both packages at once. + $this->request() + ->asForm() + ->post($this->url('subscriptions/'.$subscriptionId), [ + 'items[0][id]' => $itemId, + 'items[0][price]' => $priceId, + 'proration_behavior' => $prorationBehaviour, + ]) + ->throw(); + } + + public function subscriptionItemId(string $subscriptionId): ?string + { + $id = $this->request() + ->get($this->url('subscriptions/'.$subscriptionId)) + ->throw() + ->json('items.data.0.id'); + + return is_string($id) && $id !== '' ? $id : null; + } + private function request(?string $idempotencyKey = null): PendingRequest { $secret = (string) $this->secret(); @@ -95,7 +155,7 @@ class HttpStripeClient implements StripeClient */ private function secret(): ?string { - return app(\App\Services\Secrets\SecretVault::class)->get('stripe.secret'); + return app(SecretVault::class)->get('stripe.secret'); } private function url(string $path): string diff --git a/app/Services/Stripe/StripeClient.php b/app/Services/Stripe/StripeClient.php index 37cf2f5..a54967f 100644 --- a/app/Services/Stripe/StripeClient.php +++ b/app/Services/Stripe/StripeClient.php @@ -12,9 +12,49 @@ namespace App\Services\Stripe; */ interface StripeClient { + /** + * Settle an upgrade at once: Stripe works out the days left in the term, + * raises its own invoice for the difference and charges it there and then. + * + * Chosen over letting the proration ride on the next cycle invoice because + * the cycle invoice is the one WE issue a document for, from the contract's + * frozen price. A renewal invoice that said one figure while Stripe took + * that figure plus a proration would be a document that does not match the + * money — see App\Actions\MoveStripeSubscriptionPrice. + */ + public const PRORATE_IMMEDIATELY = 'always_invoice'; + + /** + * Change the price and settle nothing. What the next cycle is billed at + * changes; no credit and no charge is raised for the switch itself. + */ + public const PRORATE_NONE = 'none'; + /** Whether an account is actually configured. */ public function isConfigured(): bool; + /** + * Open a hosted checkout for one plan Price and return the URL to send the + * customer to. + * + * Hosted, not a payment form of ours: card details, 3-D Secure and every + * local payment method are Stripe's problem, and the moment a page of ours + * touches a card number the whole installation is in PCI scope. + * + * `$metadata` is what comes back on the webhook and decides what is built — + * plan, plan version and datacenter. It is set on the SESSION and on the + * subscription: the session's copy is what StripeWebhookController reads, + * the subscription's is what any later billing event carries. + */ + public function createCheckoutSession( + string $priceId, + string $successUrl, + string $cancelUrl, + array $metadata = [], + ?string $customerEmail = null, + ?string $idempotencyKey = null, + ): string; + /** * Create a Product for a plan family; returns its Stripe id. * @@ -39,4 +79,31 @@ interface StripeClient /** Stop a Price being offered. The Price itself stays, as Stripe requires. */ public function archivePrice(string $priceId): void; + + /** + * Move a subscription onto another Price. + * + * The ITEM is what carries a price, not the subscription, so the swap needs + * both ids. `$prorationBehaviour` is one of the constants above and is + * always passed explicitly: Stripe's own default settles a proration + * silently, and a default deciding what a customer is charged is exactly + * the kind of thing that must be written down at the call site. + */ + public function updateSubscriptionPrice( + string $subscriptionId, + string $itemId, + string $priceId, + string $prorationBehaviour, + ): void; + + /** + * The id of the single item on a subscription, or null if it has none. + * + * For contracts opened before the item id was stored: Stripe knows it, we + * did not ask, and asking once is cheaper than being unable to move them. + * One item, because one contract bills one package — a subscription with + * several would need a rule for which of them the package is, and there is + * no such thing to build one from. + */ + public function subscriptionItemId(string $subscriptionId): ?string; } diff --git a/database/migrations/2026_07_29_270000_link_invoices_to_the_period_they_bill.php b/database/migrations/2026_07_29_270000_link_invoices_to_the_period_they_bill.php new file mode 100644 index 0000000..16d9412 --- /dev/null +++ b/database/migrations/2026_07_29_270000_link_invoices_to_the_period_they_bill.php @@ -0,0 +1,43 @@ +foreignId('subscription_id')->nullable()->after('order_id') + ->constrained()->nullOnDelete(); + + $table->string('stripe_invoice_id')->nullable()->unique()->after('subscription_id'); + }); + } + + public function down(): void + { + Schema::table('invoices', function (Blueprint $table) { + $table->dropUnique(['stripe_invoice_id']); + $table->dropColumn('stripe_invoice_id'); + $table->dropConstrainedForeignId('subscription_id'); + }); + } +}; diff --git a/database/migrations/2026_07_29_280000_track_the_stripe_price_a_contract_is_billed_at.php b/database/migrations/2026_07_29_280000_track_the_stripe_price_a_contract_is_billed_at.php new file mode 100644 index 0000000..d3d578d --- /dev/null +++ b/database/migrations/2026_07_29_280000_track_the_stripe_price_a_contract_is_billed_at.php @@ -0,0 +1,51 @@ +string('stripe_item_id')->nullable()->after('stripe_subscription_id'); + $table->string('stripe_price_id')->nullable()->after('stripe_item_id'); + $table->timestamp('stripe_price_synced_at')->nullable()->after('stripe_price_id'); + $table->json('stripe_price_sync')->nullable()->after('stripe_price_synced_at'); + }); + } + + public function down(): void + { + Schema::table('subscriptions', function (Blueprint $table) { + $table->dropColumn([ + 'stripe_item_id', 'stripe_price_id', 'stripe_price_synced_at', 'stripe_price_sync', + ]); + }); + } +}; diff --git a/lang/de/invoice.php b/lang/de/invoice.php index a66585d..c4a61cd 100644 --- a/lang/de/invoice.php +++ b/lang/de/invoice.php @@ -27,6 +27,7 @@ return [ 'line_recurring' => 'monatlich', 'line_once' => 'einmalig', + 'line_period' => 'Leistungszeitraum :from – :to', 'salutation' => 'Sehr geehrte Damen und Herren,', 'intro' => 'wir erlauben uns, für die im Folgenden aufgeführten Leistungen in Rechnung zu stellen.', 'payment_default' => 'Zahlbar ohne Abzug innerhalb von :days Tagen auf das unten angeführte Konto.', diff --git a/lang/en/invoice.php b/lang/en/invoice.php index b442d8d..161e179 100644 --- a/lang/en/invoice.php +++ b/lang/en/invoice.php @@ -27,6 +27,7 @@ return [ 'line_recurring' => 'monthly', 'line_once' => 'one-off', + 'line_period' => 'Service period :from – :to', 'salutation' => 'Dear Sir or Madam,', 'intro' => 'we are pleased to invoice the services listed below.', 'payment_default' => 'Payable in full within :days days to the account shown below.', diff --git a/routes/console.php b/routes/console.php index 25f9b4c..92d5e7d 100644 --- a/routes/console.php +++ b/routes/console.php @@ -62,6 +62,18 @@ Schedule::command('clupilot:archive-invoices') ->hourly() ->withoutOverlapping(); +// Plan changes that landed here and never reached Stripe. The change is applied +// to the contract and to the machine before Stripe is told, and it is not rolled +// back when Stripe is away — so what is left is a customer being billed for a +// package they are no longer on, and nobody would ever go and look. +// +// Hourly, like the archive sweep and for the same reason: the failure it repairs +// lasts as long as an outage lasts, and hammering an API that is down helps +// nobody. +Schedule::command('clupilot:sync-stripe-subscriptions') + ->hourly() + ->withoutOverlapping(); + // Empty a handover directory of folders past their keep-by. Only where a // destination asks for it, and only folders whose name is a date this // application wrote — see the command for why it leaves everything else alone. diff --git a/tests/Feature/Billing/RenewalInvoiceTest.php b/tests/Feature/Billing/RenewalInvoiceTest.php new file mode 100644 index 0000000..c7fb4ee --- /dev/null +++ b/tests/Feature/Billing/RenewalInvoiceTest.php @@ -0,0 +1,233 @@ + 'CluPilot Cloud e.U.', + 'address' => 'Dreherstraße 66/1/8', + 'postcode' => '1110', + 'city' => 'Wien', + 'vat_id' => 'ATU00000000', + ]); +}); + +afterEach(function () { + Carbon::setTestNow(); +}); + +/** A contract Stripe bills, belonging to somebody with an address to write to. */ +function renewingContract(string $stripeId = 'sub_ren'): Subscription +{ + $customer = Customer::factory()->create([ + 'name' => 'Berger GmbH', + 'email' => 'kundin@example.com', + ]); + + return Subscription::factory()->plan('team')->create([ + 'customer_id' => $customer->id, + 'stripe_subscription_id' => $stripeId, + 'current_period_start' => now()->subMonth(), + 'current_period_end' => now(), + ]); +} + +/** The event Stripe raises when a term rolls over and is paid. */ +function renewalPaid(string $invoiceId = 'in_ren'): array +{ + return [ + 'id' => 'evt_'.$invoiceId, 'type' => 'invoice.paid', + 'data' => ['object' => [ + 'id' => $invoiceId, 'subscription' => 'sub_ren', 'amount_paid' => 21480, + 'billing_reason' => 'subscription_cycle', + 'period_start' => now()->timestamp, + 'period_end' => now()->addMonth()->timestamp, + ]], + ]; +} + +it('issues exactly one invoice for a renewal and sends exactly one mail', function () { + Mail::fake(); + $contract = renewingContract(); + + $this->postJson(route('webhooks.stripe'), renewalPaid())->assertOk(); + + $invoice = Invoice::query()->sole(); + + expect($invoice->stripe_invoice_id)->toBe('in_ren') + // The contract is what a renewal renews. There is no order behind it, + // and inventing one would put a purchase nobody made in the shop's own + // records. + ->and($invoice->subscription_id)->toBe($contract->id) + ->and($invoice->order_id)->toBeNull() + // The contract's frozen NET price, not Stripe's gross total: what the + // customer is owed is what they agreed to. + ->and($invoice->net_cents)->toBe(17900) + ->and($invoice->tax_cents)->toBe(3580) + ->and($invoice->number)->toBe('RE-2026-0001') + ->and($invoice->sent_at)->not->toBeNull(); + + // One line, saying which term it covers. Stripe's period_end is the first + // moment of the NEXT term, so printing it would put the same day on two + // consecutive invoices. + expect($invoice->snapshot['lines'])->toHaveCount(1) + ->and($invoice->snapshot['lines'][0]['details'][0]) + ->toBe(__('invoice.line_period', ['from' => '01.08.2026', 'to' => '31.08.2026'])); + + Mail::assertQueuedCount(1); + Mail::assertQueued(InvoiceMail::class, fn (InvoiceMail $mail) => $mail->hasTo('kundin@example.com') + && $mail->invoice->is($invoice)); + + // And the payment is still in the register, where it always was. + expect(SubscriptionRecord::query()->where('event', SubscriptionRecord::EVENT_RENEWAL)->count())->toBe(1); +}); + +it('issues one document and consumes one number however often Stripe delivers the invoice', function () { + Mail::fake(); + renewingContract(); + + $this->postJson(route('webhooks.stripe'), renewalPaid())->assertOk(); + $this->postJson(route('webhooks.stripe'), renewalPaid())->assertOk(); + + // An invoice number comes out of a gapless series and can never be handed + // out twice — so the second delivery must not take one, not even one it + // then throws away. + expect(Invoice::query()->count())->toBe(1) + ->and(InvoiceSeries::query()->where('kind', 'invoice')->value('next_number'))->toBe(2); + + Mail::assertQueuedCount(1); +}); + +it('does not fail the webhook when the invoice cannot be issued, and keeps the payment', function () { + Mail::fake(); + renewingContract(); + + // An invoice without a VAT number is not a valid invoice here, so issuing + // refuses — which is correct, and must not cost Stripe its 2xx. + Settings::forget('company.vat_id'); + + $this->postJson(route('webhooks.stripe'), renewalPaid())->assertOk(); + + expect(Invoice::query()->count())->toBe(0) + // A retry would re-enter this and fail again; the money is what must + // not be lost. + ->and(SubscriptionRecord::query()->where('event', SubscriptionRecord::EVENT_RENEWAL)->count())->toBe(1) + ->and(Subscription::query()->sole()->current_period_end->timestamp)->toBe(now()->addMonth()->timestamp); + + Mail::assertNothingQueued(); +}); + +it('does not fail the webhook when the mail cannot be sent, and leaves the invoice unsent', function () { + renewingContract(); + + // A mail server that is down, mid-send. + Mail::shouldReceive('to')->andThrow(new RuntimeException('Connection refused')); + + $this->postJson(route('webhooks.stripe'), renewalPaid())->assertOk(); + + $invoice = Invoice::query()->sole(); + + // The document is issued and the payment recorded; only the sending failed, + // and it says so rather than claiming to have gone out. + expect($invoice->sent_at)->toBeNull() + ->and(SubscriptionRecord::query()->where('event', SubscriptionRecord::EVENT_RENEWAL)->count())->toBe(1); +}); + +it('sends the invoice a second delivery finds unsent, without issuing a second document', function () { + renewingContract(); + + $mailer = Mail::getFacadeRoot(); + Mail::shouldReceive('to')->once()->andThrow(new RuntimeException('Connection refused')); + $this->postJson(route('webhooks.stripe'), renewalPaid())->assertOk(); + + // Stripe redelivers, and the mail server is back. The register entry is + // already there, so tying the paperwork to it would mean this customer + // never got their invoice. + Mail::swap($mailer); + Mail::fake(); + $this->postJson(route('webhooks.stripe'), renewalPaid())->assertOk(); + + expect(Invoice::query()->count())->toBe(1) + ->and(Invoice::query()->sole()->sent_at)->not->toBeNull(); + + Mail::assertQueuedCount(1); +}); + +it('invoices nothing for a proration or a charge raised by hand', function () { + Mail::fake(); + renewingContract(); + + $this->postJson(route('webhooks.stripe'), [ + 'id' => 'evt_prorata', 'type' => 'invoice.paid', + 'data' => ['object' => [ + 'id' => 'in_prorata', 'subscription' => 'sub_ren', 'amount_paid' => 4200, + 'billing_reason' => 'subscription_update', + 'period_start' => now()->timestamp, 'period_end' => now()->addMonth()->timestamp, + ]], + ])->assertOk(); + + // Stripe worked that amount out against ITS boundaries and its own view of + // the account. A document written from our contract snapshot would state a + // sum that is not the one charged. The money is in the register either way. + expect(Invoice::query()->count())->toBe(0) + ->and(SubscriptionRecord::query()->where('event', SubscriptionRecord::EVENT_INVOICE_PAID)->sole()->gross_cents) + ->toBe(4200); + + Mail::assertNothingQueued(); +}); + +it('still gives the first purchase exactly one invoice and one mail', function () { + Mail::fake(); + Queue::fake(); + + $this->postJson(route('webhooks.stripe'), [ + 'id' => 'evt_checkout', 'type' => 'checkout.session.completed', + 'data' => ['object' => [ + 'id' => 'cs_first', 'payment_status' => 'paid', 'subscription' => 'sub_first', + 'customer_details' => ['email' => 'neu@example.com', 'name' => 'Neu GmbH'], + 'amount_total' => 21480, 'currency' => 'eur', + 'metadata' => ['plan' => 'team', 'datacenter' => 'fsn'], + ]], + ])->assertOk(); + + // Stripe then sends the checkout's OWN invoice. It is not a renewal, and + // invoicing it here would send the same customer a second document for one + // purchase in the same minute. + $this->postJson(route('webhooks.stripe'), [ + 'id' => 'evt_first_invoice', 'type' => 'invoice.paid', + 'data' => ['object' => [ + 'id' => 'in_first', 'subscription' => 'sub_first', 'amount_paid' => 21480, + 'billing_reason' => 'subscription_create', + 'period_start' => now()->timestamp, 'period_end' => now()->addMonth()->timestamp, + ]], + ])->assertOk(); + + expect(Invoice::query()->count())->toBe(1) + ->and(Invoice::query()->sole()->order_id)->not->toBeNull() + ->and(Invoice::query()->sole()->stripe_invoice_id)->toBeNull(); + + Mail::assertQueuedCount(1); + Mail::assertQueued(InvoiceMail::class); +}); diff --git a/tests/Feature/Billing/StripePlanChangeTest.php b/tests/Feature/Billing/StripePlanChangeTest.php new file mode 100644 index 0000000..be9939e --- /dev/null +++ b/tests/Feature/Billing/StripePlanChangeTest.php @@ -0,0 +1,245 @@ +instance(StripeClient::class, $fake); + + // The plan prices need their Stripe ids before anything can be moved onto + // one — that is what stripe:sync-catalogue is for. + app(Kernel::class)->call('stripe:sync-catalogue'); + + return $fake; +} + +/** A paying contract Stripe knows about, with the item it bills through. */ +function stripeContractOn(string $plan, ?string $itemId = 'si_pc'): Subscription +{ + $order = Order::factory()->withSubscription()->create(['datacenter' => 'fsn', 'plan' => $plan]); + + $order->subscription->update([ + 'stripe_subscription_id' => 'sub_pc', + 'stripe_item_id' => $itemId, + ]); + + return $order->subscription->fresh(); +} + +/** The Stripe Price a package is sold at on this term. */ +function stripePriceFor(string $plan, string $term = 'monthly'): string +{ + return (string) PlanPrice::query() + ->where('plan_version_id', Subscription::snapshotFrom($plan, $term)['plan_version_id']) + ->where('term', $term) + ->value('stripe_price_id'); +} + +it('moves an upgrade onto the new price and has Stripe settle it at once', function () { + fakeServices(); + Queue::fake(); + + $stripe = stripeCatalogue(); + $contract = stripeContractOn('team'); + + app(ApplyPlanChange::class)($contract, 'business'); + + expect($stripe->priceChanges)->toHaveCount(1); + + // always_invoice, not a proration riding on the next cycle invoice: the + // cycle invoice is the one WE issue a document for, from the contract's + // frozen price, and it has to be exactly what Stripe took. + expect($stripe->priceChanges[0])->toBe([ + 'subscription' => 'sub_pc', + 'item' => 'si_pc', + 'price' => stripePriceFor('business'), + 'proration' => StripeClient::PRORATE_IMMEDIATELY, + ]); + + $fresh = $contract->fresh(); + + expect($fresh->stripe_price_id)->toBe(stripePriceFor('business')) + ->and($fresh->stripe_price_synced_at)->not->toBeNull() + ->and($fresh->stripe_price_sync)->toBeNull(); +}); + +it('moves a downgrade onto the smaller price and has Stripe settle nothing', function () { + fakeServices(); + Queue::fake(); + + $stripe = stripeCatalogue(); + $contract = stripeContractOn('business'); + + // A downgrade lands exactly at the period end, which is the only moment + // PlanChange allows one. + Carbon::setTestNow($contract->current_period_end->copy()->addHour()); + + app(ApplyPlanChange::class)($contract, 'team'); + + // Nothing to prorate at a boundary, and anything but `none` risks a stray + // credit line for a fraction of a day. The next cycle simply bills less. + expect($stripe->priceChanges)->toHaveCount(1) + ->and($stripe->priceChanges[0]['price'])->toBe(stripePriceFor('team')) + ->and($stripe->priceChanges[0]['proration'])->toBe(StripeClient::PRORATE_NONE); + + Carbon::setTestNow(); +}); + +it('leaves a granted contract alone and does not call Stripe at all', function () { + fakeServices(); + Queue::fake(); + + $stripe = stripeCatalogue(); + + // Any call at all would throw and be recorded as a failure, so a clean + // contract afterwards is proof that none was made. + $stripe->failWith = 'Stripe must not be called for a granted contract.'; + + $customer = Customer::factory()->create(); + $granted = app(GrantSubscription::class)($customer, admin(), 'team', 'monthly', 'fsn', 0); + + app(ApplyPlanChange::class)($granted->fresh(), 'business'); + + $fresh = $granted->fresh(); + + // The change lands exactly as it does for anyone else… + expect($fresh->plan)->toBe('business') + ->and(SubscriptionRecord::query()->where('event', SubscriptionRecord::EVENT_UPGRADE)->count())->toBe(1) + // …and nothing was asked of Stripe, nor recorded as owing it. + ->and($stripe->priceChanges)->toBeEmpty() + ->and($fresh->stripe_price_sync)->toBeNull() + ->and($fresh->stripe_price_id)->toBeNull(); +}); + +it('does not undo a change Stripe could not be told about, and says where it stopped', function () { + fakeServices(); + Queue::fake(); + + $stripe = stripeCatalogue(); + $contract = stripeContractOn('team'); + + $stripe->failWith = 'Connection timed out'; + + app(ApplyPlanChange::class)($contract, 'business'); + + $fresh = $contract->fresh(); + + // The contract and the machine have already moved. Rolling that back + // because an API was away would leave the customer on a package their + // machine no longer matches. + expect($fresh->plan)->toBe('business') + ->and(SubscriptionRecord::query()->where('event', SubscriptionRecord::EVENT_UPGRADE)->count())->toBe(1) + ->and($fresh->stripe_price_id)->toBeNull(); + + // And what did not happen is on the contract, not only in a log line that + // scrolls away: this customer is still being charged the old price. + expect($fresh->stripe_price_sync['error'])->toContain('Connection timed out') + ->and($fresh->stripe_price_sync['price'])->toBe(stripePriceFor('business')) + ->and($fresh->stripe_price_sync['behaviour'])->toBe(StripeClient::PRORATE_IMMEDIATELY) + ->and($fresh->stripe_price_sync['attempts'])->toBe(1); + + // The hourly sweep finishes it once Stripe answers again, with the + // behaviour the direction chose at the time. + $stripe->failWith = null; + $this->artisan('clupilot:sync-stripe-subscriptions')->assertSuccessful(); + + expect($stripe->priceChanges)->toHaveCount(1) + ->and($stripe->priceChanges[0]['proration'])->toBe(StripeClient::PRORATE_IMMEDIATELY) + ->and($contract->fresh()->stripe_price_sync)->toBeNull() + ->and($contract->fresh()->stripe_price_id)->toBe(stripePriceFor('business')); +}); + +it('parks the change when the catalogue has never been mirrored into Stripe', function () { + fakeServices(); + Queue::fake(); + + // No stripe:sync-catalogue: the prices carry no Stripe id, so there is + // nothing to move the subscription onto. + $stripe = new FakeStripeClient; + app()->instance(StripeClient::class, $stripe); + + $contract = stripeContractOn('team'); + + app(ApplyPlanChange::class)($contract, 'business'); + + expect($stripe->priceChanges)->toBeEmpty() + ->and($contract->fresh()->plan)->toBe('business') + ->and($contract->fresh()->stripe_price_sync['error'])->toContain('stripe:sync-catalogue'); +}); + +it('learns the subscription item from Stripe’s own events', function () { + $contract = stripeContractOn('team', itemId: null); + + $this->postJson(route('webhooks.stripe'), [ + 'id' => 'evt_items', 'type' => 'customer.subscription.updated', + 'data' => ['object' => [ + 'id' => 'sub_pc', 'status' => 'active', + 'items' => ['data' => [['id' => 'si_learned', 'price' => ['id' => 'price_x']]]], + 'current_period_start' => now()->timestamp, + 'current_period_end' => now()->addMonth()->timestamp, + ]], + ])->assertOk(); + + // Nothing stored an item id before, and a price hangs off the item — so + // without this a plan change had no handle to swap anything with. + expect($contract->fresh()->stripe_item_id)->toBe('si_learned'); +}); + +it('asks Stripe for the item a contract older than the column never recorded', function () { + fakeServices(); + Queue::fake(); + + $stripe = stripeCatalogue(); + $stripe->items['sub_pc'] = 'si_fetched'; + + $contract = stripeContractOn('team', itemId: null); + + app(ApplyPlanChange::class)($contract, 'business'); + + // Asked once and kept: the item id survives a price swap, so no later + // change has to ask again. + expect($stripe->priceChanges[0]['item'])->toBe('si_fetched') + ->and($contract->fresh()->stripe_item_id)->toBe('si_fetched'); +}); + +it('does not ask Stripe to move a subscription that is already on the price', function () { + fakeServices(); + Queue::fake(); + + $stripe = stripeCatalogue(); + $contract = stripeContractOn('team'); + + app(ApplyPlanChange::class)($contract, 'business'); + expect($stripe->priceChanges)->toHaveCount(1); + + // Straight back through, as a retried trigger or the sweep would come. A + // second swap onto the same price would raise a proration for a move that + // is not one. + app(MoveStripeSubscriptionPrice::class)($contract->fresh(), StripeClient::PRORATE_IMMEDIATELY); + + expect($stripe->priceChanges)->toHaveCount(1); +});