diff --git a/app/Actions/BookAddon.php b/app/Actions/BookAddon.php index e394fca..e9f1093 100644 --- a/app/Actions/BookAddon.php +++ b/app/Actions/BookAddon.php @@ -90,16 +90,6 @@ class BookAddon throw new RuntimeException($closed); } - // Der Deckel, an der Stelle, die ihn halten muss. Gezählt wird über - // ALLE laufenden Buchungen dieses Moduls, nicht je Buchung: drei - // Bestellungen à einem Block sind drei Blöcke, und eine Prüfung je - // Bestellung hätte jede einzeln durchgewinkt. - $overLimit = app(AddonCatalogue::class)->quantityRefusal($subscription, $addonKey, $quantity); - - if ($overLimit !== null) { - throw new RuntimeException($overLimit); - } - try { $addon = $this->book($subscription, $addonKey, $quantity, $order, $price, $overrides); } catch (UniqueConstraintViolationException) { @@ -129,12 +119,28 @@ class BookAddon $catalogue = app(AddonCatalogue::class); // Held while we look and write, for the same reason the shop holds - // the customer row: two clicks in flight would otherwise both find - // nothing booked and both book, which is precisely the double - // charge the check below exists to prevent. Only for a module of - // which there may be one — a second storage pack is meant to - // succeed, and serialising those would buy nothing. - if ($catalogue->isEntitlement($addonKey)) { + // the customer row: two bookings in flight would otherwise both + // find the old state and both write. ONE lock, taken for both + // reasons a booking can be refused on what the contract already + // holds — taking a second one further down would be a second wait + // for a row this transaction is already holding, and the two could + // only ever be taken in the wrong order. + // + // A module of which there may be one: the short-circuit and + // duplicateRefusal() below are read-then-write, and two clicks + // would both find nothing booked and both book. + // + // A CAPPED module: a second storage pack is meant to succeed, so + // this is deliberately not about serialising the booking — it is + // about the count below. Without the lock two different storage + // ORDERS near the cap both read the old total and both insert, and + // the unique index on (order_id, addon_key) never sees it: two + // orders are two different rows. That is a contract over the cap, + // written by two ordinary purchases. + // + // A module with neither property is left unlocked; there is + // nothing here that two of them could get wrong. + if ($catalogue->isEntitlement($addonKey) || $catalogue->maxQuantity($addonKey) !== null) { Subscription::query()->whereKey($subscription->getKey())->lockForUpdate()->first(); } @@ -150,6 +156,29 @@ class BookAddon } } + // Der Deckel, an der Stelle, die ihn halten muss. Gezählt wird über + // ALLE laufenden Buchungen dieses Moduls, nicht je Buchung: drei + // Bestellungen à einem Block sind drei Blöcke, und eine Prüfung je + // Bestellung hätte jede einzeln durchgewinkt. + // + // HIER und nicht in __invoke(): gezählt werden darf erst unter der + // Sperre oben, sonst lesen zwei gleichzeitige Bestellungen + // denselben alten Stand und fügen beide ein — der eindeutige Index + // über (order_id, addon_key) fängt das nicht, weil es zwei + // verschiedene Bestellungen sind. + // + // Und wie die Kapazitätsprüfung darunter ABSICHTLICH hinter dem + // Kurzschluss: ein zweites Mal zugestellter Webhook bekommt seine + // bestehende Buchung zurück, statt gefragt zu werden, ob NOCH ein + // Block passt — es kommt ja keiner hinzu. Davor gefragt, hätte die + // Wiederholung ausgerechnet die Buchung abgelehnt, die den Deckel + // gerade ausgefüllt hat, also jede Buchung des letzten Blocks. + $overLimit = $catalogue->quantityRefusal($subscription, $addonKey, $quantity); + + if ($overLimit !== null) { + throw new RuntimeException($overLimit); + } + // Platz auf DIESER Maschine, nicht irgendwo im Bestand: eine laufende // Instanz zieht nicht um. Dünn belegter Speicher lässt eine // Überbuchung sofort gelingen und erst auffallen, wenn die Gäste diff --git a/app/Livewire/ConfirmBookStorage.php b/app/Livewire/ConfirmBookStorage.php index 70b42eb..655223c 100644 --- a/app/Livewire/ConfirmBookStorage.php +++ b/app/Livewire/ConfirmBookStorage.php @@ -28,6 +28,9 @@ class ConfirmBookStorage extends ModalComponent public int $packGb = 0; + /** Warum es hier nichts zu kaufen gibt. Leer, solange es etwas gibt. */ + public string $refusal = ''; + public function mount(int $packs = 1): void { $catalogue = app(AddonCatalogue::class); @@ -44,13 +47,43 @@ class ConfirmBookStorage extends ModalComponent // absoluten Grenze: ein Vertrag, der schon einen Block hält, darf im // Dialog nicht mehr versprechen, als purchase() nachher wirklich in // den Warenkorb legt. + // + // Kein `max(1, …)` mehr darum herum: bei null buchbaren Blöcken — + // Deckel erreicht oder Modul für dieses Paket nicht vorgesehen — machte + // das daraus wieder einen. Dieselbe Falle, die in purchase() schon + // behoben ist ($lines floorte auf eine Bestellzeile), hatte im Fenster + // überlebt: ein direkt geöffnetes oder veraltetes Fenster versprach + // einen Block und schickte eine Bestellung los, die purchase() danach + // ablehnt. Nach oben wird weiter geklemmt, nach unten nur noch auf + // null. $max = $catalogue->bookableQuantity($subscription, AddonCatalogue::STORAGE); - $this->packs = max(1, min($max, $packs)); + $this->packs = min($max, max(1, $packs)); $this->packGb = $catalogue->packGb(); + + // Absage statt geschlossenem Fenster: der Kunde hat gerade auf eine + // Karte geklickt, die ihm diesen Ausweg angeboten hat, und ein Dialog, + // der sich wortlos wieder schließt, ist von einem kaputten Knopf nicht + // zu unterscheiden. In dem Satz, den er zu dieser Grenze überall sonst + // liest — derselbe, mit dem purchase() und BookAddon ablehnen. + $this->refusal = $this->packs > 0 ? '' : (string) ( + $catalogue->availabilityRefusal($subscription, AddonCatalogue::STORAGE) + ?? $catalogue->quantityRefusal($subscription, AddonCatalogue::STORAGE, 1) + ); } public function proceed(): void { + // Nichts buchbar, nichts losgeschickt — der Knopf, der hier ankommt, + // ist bei einer Absage gar nicht gezeichnet. Die Menge steht in einer + // öffentlichen Eigenschaft und damit im Browser, deshalb ist das hier + // nicht die Grenze, sondern nur die Ehrlichkeit des Fensters: gehalten + // wird sie in purchase() und in BookAddon, wo das Geld fließt. + if ($this->packs < 1) { + $this->closeModal(); + + return; + } + $this->dispatch('storage-packs-confirmed', packs: $this->packs); $this->closeModal(); } diff --git a/lang/de/billing.php b/lang/de/billing.php index d0a3e43..385af3c 100644 --- a/lang/de/billing.php +++ b/lang/de/billing.php @@ -136,6 +136,7 @@ return [ 'storage_includes_packs' => 'Darin enthalten: :count × Zusatzspeicher (+:gb GB).', 'storage_confirm_title' => ':count × Zusatzspeicher buchen?', 'storage_confirm_body' => 'Damit werden :count Speicherpakete (+:gb GB) vorgemerkt. Das sind :total netto pro Monat, monatlich kündbar.', + 'storage_confirm_blocked_title' => 'Kein weiterer Zusatzspeicher buchbar', 'storage_confirm_cancel' => 'Abbrechen', 'storage_confirm_cta' => 'Vormerken', 'storage_remeasured' => 'Ihr Speicherverbrauch wurde neu gemessen.', diff --git a/lang/en/billing.php b/lang/en/billing.php index 7144994..3da3c69 100644 --- a/lang/en/billing.php +++ b/lang/en/billing.php @@ -135,6 +135,7 @@ return [ 'storage_includes_packs' => 'Includes :count × extra storage (+:gb GB).', 'storage_confirm_title' => 'Add :count × extra storage?', 'storage_confirm_body' => 'This places :count storage packs (+:gb GB) in your cart. That is :total net per month, cancellable monthly.', + 'storage_confirm_blocked_title' => 'No further storage can be booked', 'storage_confirm_cancel' => 'Cancel', 'storage_confirm_cta' => 'Add to cart', 'storage_remeasured' => 'Your storage usage has been measured again.', diff --git a/resources/views/livewire/confirm-book-storage.blade.php b/resources/views/livewire/confirm-book-storage.blade.php index e7007eb..b2060df 100644 --- a/resources/views/livewire/confirm-book-storage.blade.php +++ b/resources/views/livewire/confirm-book-storage.blade.php @@ -4,25 +4,44 @@ $loc = app()->getLocale(); $priceCents = (int) config('provisioning.storage_addon.price_cents', 0); @endphp -
- - - -
-

{{ __('billing.storage_confirm_title', ['count' => $packs]) }}

-

- {{ __('billing.storage_confirm_body', [ - 'count' => $packs, - 'gb' => $packs * $packGb, - 'total' => Number::currency($packs * $priceCents / 100, in: 'EUR', locale: $loc), - ]) }} -

+ {{-- Null buchbare Blöcke: Absage statt Kaufzusage. Das Fenster wird von + einer Karte geöffnet, deren Zahl älter sein kann als der Vertrag — und + ein Dialog, der einen Block verspricht, den purchase() danach ablehnt, + ist eine Falle. --}} + @if ($packs < 1) +
+ + + +
+

{{ __('billing.storage_confirm_blocked_title') }}

+

{{ $refusal }}

+
-
-
- {{ __('billing.storage_confirm_cancel') }} - - {{ __('billing.storage_confirm_cta') }} - -
+
+ {{ __('common.close') }} +
+ @else +
+ + + +
+

{{ __('billing.storage_confirm_title', ['count' => $packs]) }}

+

+ {{ __('billing.storage_confirm_body', [ + 'count' => $packs, + 'gb' => $packs * $packGb, + 'total' => Number::currency($packs * $priceCents / 100, in: 'EUR', locale: $loc), + ]) }} +

+
+
+
+ {{ __('billing.storage_confirm_cancel') }} + + {{ __('billing.storage_confirm_cta') }} + +
+ @endif
diff --git a/tests/Feature/Billing/StoragePackLimitTest.php b/tests/Feature/Billing/StoragePackLimitTest.php index b14927b..40302db 100644 --- a/tests/Feature/Billing/StoragePackLimitTest.php +++ b/tests/Feature/Billing/StoragePackLimitTest.php @@ -8,6 +8,7 @@ use App\Models\Order; use App\Models\Subscription; use App\Models\User; use App\Services\Billing\AddonCatalogue; +use Illuminate\Support\Facades\DB; use Livewire\Livewire; /** @@ -32,6 +33,14 @@ function limitUser(Subscription $subscription): User return User::factory()->create(['email' => $subscription->customer->email]); } +/** Eine bezahlte Speicher-Bestellung dieses Kunden — eine Buchung hat eine. */ +function limitOrder(Subscription $subscription): Order +{ + return Order::factory()->create([ + 'customer_id' => $subscription->customer_id, 'type' => 'addon', 'addon_key' => 'storage', 'status' => 'paid', + ]); +} + it('lehnt den vierten Block auf einmal ab', function () { $subscription = limitContract(); @@ -120,3 +129,104 @@ it('klemmt das Bestätigungs-Modal an der Restmenge, nicht an der absoluten Gren ->call('proceed') ->assertDispatched('storage-packs-confirmed', packs: 2); }); + +/** + * Nachtrag aus der Durchsicht (Befund 1): der Deckel stand außerhalb der + * Transaktion, und für Speicher wurde der Vertrag gar nicht gesperrt. + * + * Zwei Speicher-Bestellungen desselben Vertrags, gleichzeitig verarbeitet, + * lasen damit beide denselben alten Stand und fügten beide ein — der eindeutige + * Index über (order_id, addon_key) greift nicht, weil es zwei verschiedene + * Bestellungen sind. Ergebnis: ein Vertrag über dem Deckel. + * + * WAS DIESER TEST BEWEIST: dass der Vertrag auch für ein Mengen-Modul gesperrt + * wird, dass die Zählung NACH dieser Sperre kommt und dass beide innerhalb der + * Buchungstransaktion laufen — die Reihenfolge, ohne die eine Sperre nichts + * bewirkt. + * + * WAS ER NICHT BEWEIST: dass zwei gleichzeitige Buchungen einander wirklich + * ausschließen. Die Suite läuft auf SQLite, dort ist `lockForUpdate()` ein + * No-op (SQLiteGrammar::compileLock() gibt '' zurück) und ein zweiter Prozess + * ist im selben PHP-Aufruf nicht herstellbar. Echte Nebenläufigkeit hängt an + * MariaDB und ist hier nicht nachgestellt; nachgewiesen wird die + * Voraussetzung dafür, nicht die Wirkung. + */ +it('zählt den Deckel unter der Vertragssperre und in derselben Transaktion, die bucht', function () { + $subscription = limitContract(); + $order = limitOrder($subscription); + + // RefreshDatabase hält den ganzen Test in einer Transaktion. Die Tiefe VOR + // dem Aufruf ist deshalb der Nullpunkt, gegen den "innerhalb der + // Buchungstransaktion" gemessen wird — absolut wäre 1 schon außerhalb. + $baseline = DB::transactionLevel(); + $log = []; + + DB::listen(function ($query) use (&$log) { + $log[] = ['sql' => $query->sql, 'level' => DB::transactionLevel()]; + }); + + app(BookAddon::class)($subscription, AddonCatalogue::STORAGE, 1, $order); + + // Die Sperre ist die erste Anweisung der Transaktion und die einzige + // Abfrage auf `subscriptions` in diesem Pfad; gezählt wird über + // sum(quantity) auf den Buchungen. + $lock = collect($log)->search(fn (array $statement) => str_contains($statement['sql'], 'from "subscriptions"')); + $count = collect($log)->search(fn (array $statement) => str_contains($statement['sql'], 'sum("quantity")')); + + expect($lock)->not->toBeFalse() + ->and($count)->not->toBeFalse() + ->and($count)->toBeGreaterThan($lock) + ->and($log[$lock]['level'])->toBeGreaterThan($baseline) + ->and($log[$count]['level'])->toBeGreaterThan($baseline); +}); + +/** + * Und die andere Hälfte derselben Verschiebung: der Kurzschluss bleibt vorn. + * + * Dieselbe Lektion wie bei der Kapazitätsprüfung eine Runde zuvor (siehe + * StoragePackCapacityTest): Stripe stellt denselben Webhook nach einem Timeout + * ein zweites Mal zu. Läge die Mengenprüfung vor dem Kurzschluss, würde die + * Wiederholung fragen "passt NOCH ein Block drauf" — obwohl gar keiner + * hinzukommt — und einen längst bezahlten, längst gebuchten Vorgang genau dann + * mit einem Fehler quittieren, wenn die Buchung selbst den Deckel erreicht hat. + */ +it('gibt bei einem wiederholten Webhook die vorhandene Buchung zurück, auch am erreichten Deckel', function () { + $subscription = limitContract(); + app(BookAddon::class)($subscription, AddonCatalogue::STORAGE, 2, limitOrder($subscription)); + + $order = limitOrder($subscription); + + // Der dritte Block füllt den Deckel genau aus. + $first = app(BookAddon::class)($subscription, AddonCatalogue::STORAGE, 1, $order); + $second = app(BookAddon::class)($subscription, AddonCatalogue::STORAGE, 1, $order); + + expect($second->id)->toBe($first->id) + ->and((int) $subscription->addons()->active()->sum('quantity'))->toBe(3); +}); + +/** + * Nachtrag aus der Durchsicht (Befund 3): dieselbe `max(1, …)`-Falle, die in + * purchase() schon behoben war, hatte im Fenster überlebt. + * + * Bei null buchbaren Blöcken machte `max(1, min($max, $packs))` daraus wieder + * einen — das Fenster versprach einen Block und schickte eine Bestellung los, + * die purchase() danach ablehnt. Ein Knopf, der etwas anbietet, das die Aktion + * dahinter ablehnt, ist eine Falle. + */ +it('bietet am erreichten Deckel keinen Kauf aus dem Bestätigungsfenster an', function () { + $subscription = limitContract(); + app(BookAddon::class)($subscription, AddonCatalogue::STORAGE, 3); + + Livewire::actingAs(limitUser($subscription)) + ->test(ConfirmBookStorage::class, ['packs' => 1]) + ->assertSet('packs', 0) + // Keine Kaufzusage, sondern die Absage in dem Satz, den der Kunde + // überall sonst zu dieser Grenze liest. + ->assertDontSee(__('billing.storage_confirm_cta')) + ->assertSee(__('billing.addon_limit_reached', [ + 'module' => app(AddonCatalogue::class)->name(AddonCatalogue::STORAGE), + 'max' => 3, + ])) + ->call('proceed') + ->assertNotDispatched('storage-packs-confirmed'); +});